Last Comment Bug 781667 - Audio Data API broken on ARM since bug 775319
: Audio Data API broken on ARM since bug 775319
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: ARM All
: -- major (vote)
: ---
Assigned To: Paul Adenot (:padenot)
:
: Maire Reavy [:mreavy]
Mentors:
Depends on:
Blocks: 775319
  Show dependency treegraph
 
Reported: 2012-08-09 14:53 PDT by Matthew Gregan [:kinetik]
Modified: 2012-08-14 22:57 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Convert to short before writing samples to the nsAudioStream. (170 bytes, patch)
2012-08-09 19:36 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Fix the volume scaling when using fixed point audio. (3.20 KB, patch)
2012-08-09 19:39 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Audio Data API broken on ARM since bug 775319. r= (1.57 KB, patch)
2012-08-10 09:00 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Fix the volume scaling when using fixed point audio. r= (1.79 KB, patch)
2012-08-10 09:01 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Audio Data API broken on ARM since bug 775319. r= (1.66 KB, patch)
2012-08-10 14:11 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Also fix volume on ARM when using cubeb. (1.61 KB, patch)
2012-08-12 19:40 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Determine the sample format at compile time for all media code. r= (23.04 KB, patch)
2012-08-12 20:52 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review

Description Matthew Gregan [:kinetik] 2012-08-09 14:53:31 PDT
Unfortunately I missed this during review, but the changes to nsHTMLAudioElement in bug 775319 open the nsAudioStream with the sint16 format but continue to pass float32 data to Write(), which is completely wrong.  Among other things, this has broken the dialer app on b2g.

I'm away for the next couple of days.  Paul can you organize a fix or a backout of the patch?
Comment 1 Paul Adenot (:padenot) 2012-08-09 19:36:27 PDT
Created attachment 650761 [details] [diff] [review]
Convert to short before writing samples to the nsAudioStream.

This should fix the problem. I wanted to check on Fennec since I haven't my b2g device with me, but it appears mozWriteAudio is broken there.
Comment 2 Paul Adenot (:padenot) 2012-08-09 19:39:51 PDT
Created attachment 650762 [details] [diff] [review]
Fix the volume scaling when using fixed point audio.

Reading the code, I found that I forgot to do proper volume scaling when using fixed point audio, and it unfortunately wasn't caught during the review.

This fixes it, tested on a device, and basically re-add the old code under a #define.
Comment 3 Paul Adenot (:padenot) 2012-08-10 09:00:08 PDT
Created attachment 650898 [details] [diff] [review]
Audio Data API broken on ARM since bug 775319. r=
Comment 4 Paul Adenot (:padenot) 2012-08-10 09:01:15 PDT
Created attachment 650899 [details] [diff] [review]
Fix the volume scaling when using fixed point audio. r=

I accidentally merged the two patches, they are properly separated, now.
Comment 5 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-08-10 09:36:04 PDT
I would say this is not a blocker since this API is mozilla specific and not yet used a lot on the web. So we're probably not losing meaningful web compat by not having this.
Comment 6 Michael Wu [:mwu] 2012-08-10 09:45:46 PDT
(In reply to Jonas Sicking (:sicking) from comment #5)
> I would say this is not a blocker since this API is mozilla specific and not
> yet used a lot on the web. So we're probably not losing meaningful web
> compat by not having this.
Comment 7 Michael Wu [:mwu] 2012-08-10 09:46:51 PDT
(In reply to Jonas Sicking (:sicking) from comment #5)
> I would say this is not a blocker since this API is mozilla specific and not
> yet used a lot on the web. So we're probably not losing meaningful web
> compat by not having this.

The dialer doesn't work without this fixed.

However, the original regressing bug has been backed out so it's ok.
Comment 8 Chris Peterson [:cpeterson] 2012-08-10 11:08:56 PDT
Comment on attachment 650898 [details] [diff] [review]
Audio Data API broken on ARM since bug 775319. r=

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

::: content/html/content/src/nsHTMLAudioElement.cpp
@@ +182,5 @@
> +    }
> +  }
> +#endif
> +
> +  nsresult rv = mAudioStream->Write(shortsArray, writeLen);

Paul, does `shortsArray` need to be declared for #ifdef MOZ_FLOAT_POINT_AUDIO?
Comment 9 Paul Adenot (:padenot) 2012-08-10 14:11:18 PDT
Created attachment 650995 [details] [diff] [review]
Audio Data API broken on ARM since bug 775319. r=

As cpeterson pointed out, the previous patch was completly wrong, this ought to
work properly. I can't test properly as I haven't my b2g device now, and
mozWriteAudio is busted on fennec right now (at least on the two devices I tries
on).
Comment 10 Paul Adenot (:padenot) 2012-08-12 19:40:13 PDT
Created attachment 651254 [details] [diff] [review]
Also fix volume on ARM when using cubeb.

If we use cubeb on android at some point, this will be handy.
Comment 11 Matthew Gregan [:kinetik] 2012-08-12 20:05:24 PDT
Since bug 775319 was backed out, fold these fixes into the main patch and I'll re-review the whole thing.  It doesn't make sense to land these fixes in separate changesets to the main patch, since it introduces broken tree states between the main patch and the fixes landing.
Comment 12 Paul Adenot (:padenot) 2012-08-12 20:52:07 PDT
Created attachment 651260 [details] [diff] [review]
Determine the sample format at compile time for all media code.  r=

I also added the volume code needed if we have a cubeb backend at some point, on
top of the two previous fixes.
Comment 13 Matthew Gregan [:kinetik] 2012-08-12 21:01:27 PDT
Thanks. Attach it to the other bug, since that's the one tracking this feature.  This bug tracks the fallout, which was fixed by the backout, so we can close this one.
Comment 14 Paul Adenot (:padenot) 2012-08-12 21:08:51 PDT
Fixed by backout.

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