781667 - Audio Data API broken on ARM since bug 775319

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

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

Attachment: 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)]

Attachment: 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)]

Attachment: Audio Data API broken on ARM since bug 775319. r=

Comment 4

[Paul Adenot (:padenot)]

Attachment: 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]

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

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

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

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

Attachment: 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)]

Attachment: 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]]

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

Attachment: 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]]

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

Fixed by backout.