Closed Bug 781667 Opened 7 years ago Closed 7 years ago

Audio Data API broken on ARM since bug 775319

Categories

(Core :: Audio/Video, defect, major)

ARM
All
defect
Not set
major

Tracking

()

RESOLVED FIXED
blocking-basecamp -

People

(Reporter: kinetik, Assigned: padenot)

References

Details

Attachments

(1 file, 6 obsolete files)

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?
blocking-basecamp: --- → ?
Assignee: nobody → paul
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.
Attachment #650761 - Flags: review?(cpearce)
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.
Attachment #650762 - Flags: review?(cpearce)
Attachment #650761 - Attachment is obsolete: true
Attachment #650761 - Flags: review?(cpearce)
I accidentally merged the two patches, they are properly separated, now.
Attachment #650899 - Flags: review?(kinetik)
Attachment #650762 - Attachment is obsolete: true
Attachment #650762 - Flags: review?(cpearce)
Attachment #650898 - Flags: review?(kinetik)
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.
blocking-basecamp: ? → -
(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.
(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 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?
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).
Attachment #650995 - Flags: review?(kinetik)
Attachment #650898 - Attachment is obsolete: true
Attachment #650898 - Flags: review?(kinetik)
If we use cubeb on android at some point, this will be handy.
Attachment #651254 - Flags: review?(kinetik)
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.
I also added the volume code needed if we have a cubeb backend at some point, on
top of the two previous fixes.
Attachment #651260 - Flags: review?(kinetik)
Attachment #650899 - Attachment is obsolete: true
Attachment #650899 - Flags: review?(kinetik)
Attachment #650995 - Attachment is obsolete: true
Attachment #650995 - Flags: review?(kinetik)
Attachment #651254 - Attachment is obsolete: true
Attachment #651254 - Flags: review?(kinetik)
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.
Fixed by backout.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Attachment #651260 - Flags: review?(kinetik)
You need to log in before you can comment on or make changes to this bug.