Last Comment Bug 775319 - Determine the sample type in the media code at compile time.
: Determine the sample type in the media code at compile time.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: 16 Branch
: x86_64 Linux
: -- normal (vote)
: mozilla17
Assigned To: Paul Adenot (:padenot)
:
Mentors:
: 781997 (view as bug list)
Depends on: 781667
Blocks: 495040 639587
  Show dependency treegraph
 
Reported: 2012-07-18 14:51 PDT by Paul Adenot (:padenot)
Modified: 2012-08-22 22:37 PDT (History)
7 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Fix the sample format at compile time. r= (18.22 KB, patch)
2012-07-18 15:10 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Determine the sample format at compile time using the architecture. r= (2.83 KB, patch)
2012-07-18 15:10 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Fix the sample format at compile time. r= (18.22 KB, patch)
2012-07-18 15:10 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Decide the sample type to use at compile time. (21.05 KB, patch)
2012-08-02 14:50 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Added a chunk that was not qref'ed + addressed cpearce comments (21.02 KB, patch)
2012-08-03 12:04 PDT, Paul Adenot (:padenot)
kinetik: review+
Details | Diff | Splinter Review
Determine the sample format at compile time for all media code. (21.04 KB, patch)
2012-08-07 16:05 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Determine the sample format at compile time, without busting everything. (23.04 KB, patch)
2012-08-12 21:08 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Determine the sample format at compile time for all media code. r= (22.38 KB, patch)
2012-08-16 18:26 PDT, Paul Adenot (:padenot)
kinetik: review+
Details | Diff | Splinter Review
(added r= in the commit message) (22.38 KB, patch)
2012-08-20 16:17 PDT, Paul Adenot (:padenot)
padenot: review+
Details | Diff | Splinter Review

Description Paul Adenot (:padenot) 2012-07-18 14:51:55 PDT
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) 2012-07-18 15:10:18 PDT
Created attachment 643613 [details] [diff] [review]
Fix the sample format at compile time. r=
Comment 2 Paul Adenot (:padenot) 2012-07-18 15:10:31 PDT
Created attachment 643614 [details] [diff] [review]
Determine the sample format at compile time using the architecture. r=
Comment 3 Paul Adenot (:padenot) 2012-07-18 15:10:51 PDT
Created attachment 643615 [details] [diff] [review]
Fix the sample format at compile time. r=
Comment 4 Paul Adenot (:padenot) 2012-07-18 15:16:53 PDT
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] 2012-07-18 15:21:48 PDT
We can always add it back later, if it's needed.
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-18 20:13:26 PDT
I think it's fine for MediaStreamGraph to convert floats to integers itself as needed.
Comment 7 Paul Adenot (:padenot) 2012-07-18 20:16:10 PDT
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) 2012-08-02 14:50:07 PDT
Created attachment 648497 [details] [diff] [review]
Decide the sample type to use at compile time.
Comment 9 Chris Pearce (:cpearce) 2012-08-02 14:54:10 PDT
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) 2012-08-03 12:04:56 PDT
Created attachment 648796 [details] [diff] [review]
Added a chunk that was not qref'ed + addressed cpearce comments
Comment 11 Matthew Gregan [:kinetik] 2012-08-05 21:39:04 PDT
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) 2012-08-07 16:05:51 PDT
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.
Comment 13 Matthew Gregan [:kinetik] 2012-08-07 16:43:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcb9299974a0
Comment 14 Ed Morley [:emorley] 2012-08-08 09:30:55 PDT
https://hg.mozilla.org/mozilla-central/rev/dcb9299974a0
Comment 15 cajbir (:cajbir) 2012-08-09 22:17:27 PDT
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 17 Ed Morley [:emorley] 2012-08-10 01:23:08 PDT
(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) 2012-08-10 01:27:42 PDT
Thanks Ed, I hadn't noticed the process had changed. Whiteboard updated as per new rules.
Comment 19 dclarke@mozilla.com [:onecyrenus] 2012-08-11 12:14:59 PDT
*** Bug 781997 has been marked as a duplicate of this bug. ***
Comment 20 Ryan VanderMeulen [:RyanVM] 2012-08-11 19:57:43 PDT
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) 2012-08-12 21:08:14 PDT
Created attachment 651263 [details] [diff] [review]
Determine the sample format at compile time, without busting everything.
Comment 22 Paul Adenot (:padenot) 2012-08-13 11:57:07 PDT
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] 2012-08-13 16:46:47 PDT
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) 2012-08-16 18:26:20 PDT
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).
Comment 25 Matthew Gregan [:kinetik] 2012-08-16 19:43:38 PDT
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] 2012-08-19 23:24:28 PDT
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) (Exited; email my personal email if necessary) 2012-08-20 04:57:10 PDT
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) 2012-08-20 08:45:47 PDT
That's what my mxr query told me as well. This will be taken care of.
Comment 29 Paul Adenot (:padenot) 2012-08-20 16:17:15 PDT
Created attachment 653570 [details] [diff] [review]
(added r= in the commit message)
Comment 31 Ryan VanderMeulen [:RyanVM] 2012-08-21 19:10:55 PDT
https://hg.mozilla.org/mozilla-central/rev/79e9fb28b8e1

Note You need to log in before you can comment on or make changes to this bug.