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.

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?

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.

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

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

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.

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.

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.

(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"

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

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

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

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.

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

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.

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.

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

Green on Try. https://tbpl.mozilla.org/?tree=Try&rev=3d0f1291cf01 https://hg.mozilla.org/integration/mozilla-inbound/rev/79e9fb28b8e1