Audio Data API broken on ARM since bug 775319

RESOLVED FIXED

Status

()

Core
Audio/Video
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kinetik, Assigned: padenot)

Tracking

Trunk
ARM
All
Points:
---

Firefox Tracking Flags

(blocking-basecamp:-)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

5 years ago
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?

Updated

5 years ago
blocking-basecamp: --- → ?
(Assignee)

Updated

5 years ago
Assignee: nobody → paul
(Assignee)

Comment 1

5 years ago
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.
Attachment #650761 - Flags: review?(cpearce)
(Assignee)

Comment 2

5 years ago
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.
Attachment #650762 - Flags: review?(cpearce)
(Assignee)

Comment 3

5 years ago
Created attachment 650898 [details] [diff] [review]
Audio Data API broken on ARM since bug 775319. r=
(Assignee)

Updated

5 years ago
Attachment #650761 - Attachment is obsolete: true
Attachment #650761 - Flags: review?(cpearce)
(Assignee)

Comment 4

5 years ago
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.
Attachment #650899 - Flags: review?(kinetik)
(Assignee)

Updated

5 years ago
Attachment #650762 - Attachment is obsolete: true
Attachment #650762 - Flags: review?(cpearce)
(Assignee)

Updated

5 years ago
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: ? → -

Comment 6

5 years ago
(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

5 years ago
(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?
(Assignee)

Comment 9

5 years ago
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).
Attachment #650995 - Flags: review?(kinetik)
(Assignee)

Updated

5 years ago
Attachment #650898 - Attachment is obsolete: true
Attachment #650898 - Flags: review?(kinetik)
(Assignee)

Comment 10

5 years ago
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.
Attachment #651254 - Flags: review?(kinetik)
(Reporter)

Comment 11

5 years ago
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.
(Assignee)

Comment 12

5 years ago
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.
Attachment #651260 - Flags: review?(kinetik)
(Assignee)

Updated

5 years ago
Attachment #650899 - Attachment is obsolete: true
Attachment #650899 - Flags: review?(kinetik)
(Assignee)

Updated

5 years ago
Attachment #650995 - Attachment is obsolete: true
Attachment #650995 - Flags: review?(kinetik)
(Assignee)

Updated

5 years ago
Attachment #651254 - Attachment is obsolete: true
Attachment #651254 - Flags: review?(kinetik)
(Reporter)

Comment 13

5 years ago
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.
(Assignee)

Comment 14

5 years ago
Fixed by backout.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
Attachment #651260 - Flags: review?(kinetik)
You need to log in before you can comment on or make changes to this bug.