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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Paul Adenot (:padenot)]

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.

Comment 1

[Paul Adenot (:padenot)]

Attachment: Fix the sample format at compile time. r=

Comment 2

[Paul Adenot (:padenot)]

Attachment: Determine the sample format at compile time using the architecture. r=

Comment 3

[Paul Adenot (:padenot)]

Attachment: Fix the sample format at compile time. r=

Comment 4

[Paul Adenot (:padenot)]

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?

Comment 5

[Matthew Gregan [:kinetik]]

We can always add it back later, if it's needed.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it's fine for MediaStreamGraph to convert floats to integers itself as needed.

Comment 7

[Paul Adenot (:padenot)]

These patches actually use that cool feature of the MediaStreamGraph to convert from 16bits integers to 32bits floats for getUserMedia kind of automatically.

Comment 8

[Paul Adenot (:padenot)]

Attachment: Decide the sample type to use at compile time.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 //

Comment 10

[Paul Adenot (:padenot)]

Attachment: Added a chunk that was not qref'ed + addressed cpearce comments

Comment 11

[Matthew Gregan [: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.

Comment 12

[Paul Adenot (:padenot)]

Attachment: 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.

Comment 13

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dcb9299974a0

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/dcb9299974a0

Comment 15

[cajbir (:cajbir)]

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

[cajbir (:cajbir)]

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.

Comment 17

[Ed Morley [:emorley]]

(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

[cajbir (:cajbir)]

Thanks Ed, I hadn't noticed the process had changed. Whiteboard updated as per new rules.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

The answer is that the tool will not know what to do with "Backup" instead of "Backout", but our smart merge monkeys will :)

Comment 21

[Paul Adenot (:padenot)]

Attachment: Determine the sample format at compile time, without busting everything.

Comment 22

[Paul Adenot (:padenot)]

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

Found a mistake.

Comment 23

[Matthew Gregan [: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.

Comment 24

[Paul Adenot (:padenot)]

Attachment: 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).

Comment 25

[Matthew Gregan [: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 26

[Matthew Gregan [:kinetik]]

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.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 28

[Paul Adenot (:padenot)]

That's what my mxr query told me as well. This will be taken care of.

Comment 29

[Paul Adenot (:padenot)]

Attachment: (added r= in the commit message)

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Green on Try.
https://tbpl.mozilla.org/?tree=Try&rev=3d0f1291cf01

https://hg.mozilla.org/integration/mozilla-inbound/rev/79e9fb28b8e1

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/79e9fb28b8e1