Determine the sample type in the media code at compile time.

RESOLVED FIXED in mozilla17

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: padenot, Assigned: padenot)

Tracking

16 Branch
mozilla17
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

5 years ago
The sample type should be 16bits integers on arm, and 32bits float on everything else. It should be fixed at compile time so we avoid branching all over the place.
(Assignee)

Comment 1

5 years ago
Created attachment 643613 [details] [diff] [review]
Fix the sample format at compile time. r=
(Assignee)

Comment 2

5 years ago
Created attachment 643614 [details] [diff] [review]
Determine the sample format at compile time using the architecture. r=
(Assignee)

Comment 3

5 years ago
Created attachment 643615 [details] [diff] [review]
Fix the sample format at compile time. r=
(Assignee)

Updated

5 years ago
Attachment #643613 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
I'm not sure if we need to keep the parameter nsAudioStream::Init, because we may need to be able to use floating point on ARM if we want to do signal processing (for example for Web Audio). Maybe attachment 643615 [details] [diff] [review] is too aggressive. Any thoughts before I continue?
Status: NEW → ASSIGNED
We can always add it back later, if it's needed.
I think it's fine for MediaStreamGraph to convert floats to integers itself as needed.
(Assignee)

Comment 7

5 years ago
These patches actually use that cool feature of the MediaStreamGraph to convert from 16bits integers to 32bits floats for getUserMedia kind of automatically.
(Assignee)

Comment 8

5 years ago
Created attachment 648497 [details] [diff] [review]
Decide the sample type to use at compile time.
Attachment #643614 - Attachment is obsolete: true
Attachment #643615 - Attachment is obsolete: true
Attachment #648497 - Flags: review?(cpearce)
Comment on attachment 648497 [details] [diff] [review]
Decide the sample type to use at compile time.

Review of attachment 648497 [details] [diff] [review]:
-----------------------------------------------------------------

Matthew should look at this, he's the audio tech lead.

::: content/media/nsAudioStream.cpp
@@ +491,3 @@
>  
> +  if (sa_stream_write(static_cast<sa_stream_t*>(mAudioHandle),
> +        s_data.get(),

Indentation is off here.

@@ +1170,5 @@
>        if (scaled_volume == 1.0) {
>          memcpy(output, input[i], input_size[i]);
>          output += input_size[i];
>        } else {
> +        //// Adjust volume as each sample is copied out.

//// should be //
Attachment #648497 - Flags: review?(cpearce) → review?(kinetik)
(Assignee)

Comment 10

5 years ago
Created attachment 648796 [details] [diff] [review]
Added a chunk that was not qref'ed + addressed cpearce comments
Attachment #648497 - Attachment is obsolete: true
Attachment #648497 - Flags: review?(kinetik)
Attachment #648796 - Flags: review?(kinetik)
Comment on attachment 648796 [details] [diff] [review]
Added a chunk that was not qref'ed + addressed cpearce comments

Review of attachment 648796 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: configure.in
@@ +5230,5 @@
>  
>  dnl ========================================================
>  dnl = Enable Raw Codecs
>  dnl ========================================================
> +MOZ_ARG_DISABLE_BOOL(floating-point,

The comment is out of date.  Also, do we need a configure flag for this?

::: content/media/nsAudioStream.cpp
@@ +938,5 @@
>  
>    cubeb_stream_params params;
>    params.rate = aRate;
>    params.channels = aNumChannels;
> +  params.format = CUBEB_SAMPLE_FLOAT32LE;

Bug 779187 changes this, so please be careful when rebasing.
Attachment #648796 - Flags: review?(kinetik) → review+
(Assignee)

Comment 12

5 years ago
Created attachment 649855 [details] [diff] [review]
Determine the sample format at compile time for all media code.

I took extra care for the CUBEB_SAMPLE_FLOAT32LE -> CUBEB_SAMPLE_FLOAT32NE
change.

 I find the compilation flag useful to debug things like bug 780531
(comment says that doing a MOZ_TREMOR build helps diagnose an issue), and to
test fixed point version of libraries without leaving the comfort of the desktop.
(Assignee)

Updated

5 years ago
Attachment #648796 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcb9299974a0
Keywords: checkin-needed
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/dcb9299974a0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 781667

Comment 15

5 years ago
Audio volume is broken on Android. It plays very loud and distorted. I plan to back this out shortly and it can be relanded when the fixes in bug 781667 are ready.

Comment 16

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1659b2b5ac0
https://hg.mozilla.org/integration/mozilla-inbound/rev/0707699ac549

Backout. Please leave bug open after merge to m-c.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla17 → ---
(In reply to Chris Double (:doublec) from comment #16)
> Backout. Please leave bug open after merge to m-c.

Per https://wiki.mozilla.org/Tree_Rules/Inbound#Please_do_the_following_after_pushing_to_inbound and the dev.platform post, bug comments like this won't be seen by the merge tool we now use. Please make sure to add [leave open] to the whiteboard if the bug must remain open.

In this particular instance, the merge tool should hopefully recognise that the commit message is in the form of a backout. However one of the commits is typoed (Backup rather than backout/backed out/...), so not sure how it will handle them in combo:
"Backup bug 775319 due to audio issues on android and b2g"

Comment 18

5 years ago
Thanks Ed, I hadn't noticed the process had changed. Whiteboard updated as per new rules.
Whiteboard: [leave open]
Duplicate of this bug: 781997
The answer is that the tool will not know what to do with "Backup" instead of "Backout", but our smart merge monkeys will :)
(Assignee)

Comment 21

5 years ago
Created attachment 651263 [details] [diff] [review]
Determine the sample format at compile time, without busting everything.
Attachment #651263 - Flags: review?(kinetik)
(Assignee)

Updated

5 years ago
Attachment #649855 - Attachment is obsolete: true
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
(Assignee)

Comment 22

5 years ago
Comment on attachment 651263 [details] [diff] [review]
Determine the sample format at compile time, without busting everything.

Found a mistake.
Attachment #651263 - Attachment is obsolete: true
Attachment #651263 - Flags: review?(kinetik)
While you're fixing that, can you please also reconcile MOZ_FLOATING_POINT_AUDIO vs MOZ_SAMPLE_TYPE_* (defined in nsBuiltinDecoderReader.h)?  We don't need both, and MOZ_SAMPLE_TYPE_* seems more descriptive to me.
(Assignee)

Comment 24

5 years ago
Created attachment 652641 [details] [diff] [review]
Determine the sample format at compile time for all media code.  r=

Here is a new version, where MOZ_SAMPLE_TYPE_{SE16, FLOAT32} are used instead of
the MOZ_FLOATING_POINT_AUDIO.

I made sure it is working on desktop, fennec and b2g (playback + mozWriteAudio
each time, apart for fennec where mozWriteAudio is busted).
Attachment #652641 - Flags: review?(kinetik)
Thanks, I'll get to this shortly.

(In reply to Paul ADENOT (:padenot) from comment #24)
> apart for fennec where mozWriteAudio is busted

Can you file a bug for this, with details of what you're using to test and what breakage you're seeing?
Comment on attachment 652641 [details] [diff] [review]
Determine the sample format at compile time for all media code.  r=

Thanks, looks good.

Can you please file a followup to untangle S16LE vs S16(NE)?  We should rename S16LE to just S16 (i.e. native) and do the endian conversion in nsWaveReader.cpp; I think everywhere else that uses S16LE is acting as if it's NE.

(In reply to Matthew Gregan [:kinetik] from comment #25)
> Can you file a bug for this, with details of what you're using to test and
> what breakage you're seeing?

And that's bug 783456.
Attachment #652641 - Flags: review?(kinetik) → review+
AudioSegment::WriteTo generates little-endian when writing to an nsAudioStream with FORMAT_S16_LE. Changing FORMAT_S16_LE to FORMAT_S16 makes sense to me if cubeb can handle it, but don't forget to update AudioSegment.
(Assignee)

Comment 28

5 years ago
That's what my mxr query told me as well. This will be taken care of.
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [leave open]
(Assignee)

Comment 29

5 years ago
Created attachment 653570 [details] [diff] [review]
(added r= in the commit message)
Attachment #652641 - Attachment is obsolete: true
Attachment #653570 - Flags: review+
Green on Try.
https://tbpl.mozilla.org/?tree=Try&rev=3d0f1291cf01

https://hg.mozilla.org/integration/mozilla-inbound/rev/79e9fb28b8e1
Flags: in-testsuite-
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/79e9fb28b8e1
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Blocks: 639587
You need to log in before you can comment on or make changes to this bug.