927245 - Remove deprecated Audio Data API implementation

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

It was deprecated in bug 855570 (shipped in Firefox 22).  Removing it would be nice:

 48 files changed, 8 insertions(+), 1913 deletions(-)

Two mochitests (test_wave_data_*) need to be updated to use Web Audio, the video controls will lose their "sample rate" and "channels" info (unless these values are exposed another way, perhaps via mozGetMetadata()?), and Gaia's dialer and emergency call code needs to be migrated to Web Audio.

Comment 1

[Paul Adenot (:padenot)]

Maybe the code path the ported test would exercise are already covered by the test suite in content/media/webaudio/test. I'll have a look.

I know someone is working on porting the dialer (Etienne, CC-ed), not sure about porting the emergency call code.

Comment 2

[Matthew Gregan [:kinetik]]

Attachment: wip v0

Comment 3

[Matthew Gregan [:kinetik]]

Attachment: test_waveDecoder.html

(In reply to Paul Adenot (:padenot) from comment #1)
> Maybe the code path the ported test would exercise are already covered by
> the test suite in content/media/webaudio/test. I'll have a look.

Couldn't see anything obvious, so I wrote a test, but either my test code is broken or I've found a new bug.  I've attached the test; I get 14 samples returned for each instead of 3, and the sample values are wonky.

Comment 4

[Matthew Gregan [:kinetik]]

Slight updated patch, with the above test (expecting it to fail for now) and the video controls fixed:
https://tbpl.mozilla.org/?tree=Try&rev=89dbb6214a94

Comment 5

[Etienne Segonzac (:etienne)]

(In reply to Paul Adenot (:padenot) from comment #1)
> Maybe the code path the ported test would exercise are already covered by
> the test suite in content/media/webaudio/test. I'll have a look.
> 
> I know someone is working on porting the dialer (Etienne, CC-ed), not sure
> about porting the emergency call code.

Very similar code, I'll probably do both.

Comment 6

[(no longer active)]

I think removing the code whole-sale is a mistake initially.  My plan was to hide the API points behind a pref which is set to false is non-release builds initially and let that bake for one or two cycles, and if nobody screams, flip the pref to false in all release builds, and remove the code shortly afterwards.

Doing that will make it easier for us to back off one release or two if we find out that this breaks some web content out there in a bad way, and seems like better risk mitigation to me.

Comment 7

[(no longer active)]

CCing some Emscripten folks to keep them in the loop.

Comment 8

[Jukka Jylänki]

Emscripten currently has two audio API implementations available for C/C++ code:
  - The OpenAL library utilizes Web Audio API as backend: https://github.com/kripken/emscripten/blob/master/src/library_openal.js
  - The SDL library primarily utilizes the Audio Data API if available, and secondarily Web Audio API if that is present. The detection is made dynamically, so if/when Audio Data API is killed, existing Emscripten-built applications should gracefully start falling back to Web Audio API, see https://github.com/kripken/emscripten/blob/master/src/library_sdl.js#L1449

The reason for currently favoring Audio Data API if available over Web Audio API is that we have not been able to implement a smooth continous audio playback path for Web Audio API, the following bug is related https://bugzilla.mozilla.org/show_bug.cgi?id=913854 . The issue is most notable in non-48kHz/32-bit float audio sources.

I am ok with Audio Data API going down, and feeling hopeful that the above bug can be resolved without  changes to the spec(?), the discussion there makes it feel like the problem would be at implementation level and not the spec level. Hopefully there will not be a time gap between killing Audio Data API, and the above bug being resolved, or the audio experience in running existing SDL audio -based applications like http://clb.demon.fi/html5scummvm/ could be impacted.

Comment 9

[(no longer active)]

(In reply to comment #8)
> Emscripten currently has two audio API implementations available for C/C++
> code:
>   - The OpenAL library utilizes Web Audio API as backend:
> https://github.com/kripken/emscripten/blob/master/src/library_openal.js
>   - The SDL library primarily utilizes the Audio Data API if available, and
> secondarily Web Audio API if that is present. The detection is made
> dynamically, so if/when Audio Data API is killed, existing Emscripten-built
> applications should gracefully start falling back to Web Audio API, see
> https://github.com/kripken/emscripten/blob/master/src/library_sdl.js#L1449

That's great!

> The reason for currently favoring Audio Data API if available over Web Audio
> API is that we have not been able to implement a smooth continous audio
> playback path for Web Audio API, the following bug is related
> https://bugzilla.mozilla.org/show_bug.cgi?id=913854 . The issue is most notable
> in non-48kHz/32-bit float audio sources.

Yes, that use case is not really supported by Web Audio.

> I am ok with Audio Data API going down, and feeling hopeful that the above bug
> can be resolved without  changes to the spec(?)

I don't think that it can unfortunately, since it's not a bug in our implementation per se.

> the discussion there makes it
> feel like the problem would be at implementation level and not the spec level.

The resampling algorithm does make a difference, and that is not mandated in the spec, but lower quality resampling algorithms may "hide" this problem in some implementations.

> Hopefully there will not be a time gap between killing Audio Data API, and the
> above bug being resolved, or the audio experience in running existing SDL audio
> -based applications like http://clb.demon.fi/html5scummvm/ could be impacted.

How many Emscripten games are affected by this?  Off the top of my head I remember scumvm and perhaps one other game I can't remember the name of right now.  In my experience this has been a very rare use case for Emscripten games, and in most cases should be possible to work around by just playing the background audio using an <audio> element.

Comment 10

[Matthew Gregan [:kinetik]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #6)
> I think removing the code whole-sale is a mistake initially.  My plan was to
> hide the API points behind a pref which is set to false is non-release
> builds initially and let that bake for one or two cycles, and if nobody
> screams, flip the pref to false in all release builds, and remove the code
> shortly afterwards.

Okay, we can use the existing media.audio_data.enabled for that.  (See http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLAudioElement.cpp#24).  It'll need to be extended slightly to throw for mozChannels/mozSampleRate too.

Comment 11

[Matthew Gregan [:kinetik]]

(In reply to Matthew Gregan [:kinetik] from comment #3)
> Couldn't see anything obvious, so I wrote a test, but either my test code is
> broken or I've found a new bug.  I've attached the test; I get 14 samples
> returned for each instead of 3, and the sample values are wonky.

Turns out it's just resampling.  The test media is 3 samples at 11025Hz, the AudioContext is running at 48000Hz, 48/11 * 3 ends up with 14 samples.  Simple fix was to rewrite the test media's sample rate with whatever the AudioContext.sampleRate reports to avoid resampling.


Try push for the pref-off approach: https://tbpl.mozilla.org/?tree=Try&rev=717c1f655cef

Comment 12

[Matthew Gregan [:kinetik]]

Removal patch, includes the fixed test (which I'll move to the pref-off patch before review/landing): https://tbpl.mozilla.org/?tree=Try&rev=744d37fb5bfe

Comment 13

[Masatoshi Kimura [:emk]]

(In reply to Matthew Gregan [:kinetik] from comment #10)
> Okay, we can use the existing media.audio_data.enabled for that.  (See
> http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/
> HTMLAudioElement.cpp#24).  It'll need to be extended slightly to throw for
> mozChannels/mozSampleRate too.

It would be sufficient to add [Pref="media.audio_data.enabled"] in .webidl files.

Comment 14

[Matthew Gregan [:kinetik]]

(In reply to Matthew Gregan [:kinetik] from comment #11)
> Try push for the pref-off approach:
> https://tbpl.mozilla.org/?tree=Try&rev=717c1f655cef

Er, with the pref actually off (missed it in all.js): https://tbpl.mozilla.org/?tree=Try&rev=5300a9d7a335

(In reply to Masatoshi Kimura [:emk] from comment #13)
> It would be sufficient to add [Pref="media.audio_data.enabled"] in .webidl
> files.

Thanks, that's what I did.

Comment 15

[Matthew Gregan [:kinetik]]

Attachment: bug927245_pref_off_v0.patch

Comment 16

[(no longer active)]

Comment on attachment 819513 [details] [diff] [review]
bug927245_pref_off_v0.patch

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

Looks mostly good, except that I don't agree that we should remove the tests using the Audio Data APIs.  Instead, we should turn this pref on for those mochitests I think, and get rid of the tests when we actually remove the code that they're testing.

::: content/media/webaudio/test/test_waveDecoder.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

Hmm, is there something in this test which <http://mxr.mozilla.org/mozilla-central/source/content/media/webaudio/test/test_mediaDecoding.html?force=1> doesn't cover?

Comment 17

[Jukka Jylänki]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #9)
> How many Emscripten games are affected by this?  Off the top of my head I
> remember scumvm and perhaps one other game I can't remember the name of
> right now.  In my experience this has been a very rare use case for
> Emscripten games, and in most cases should be possible to work around by
> just playing the background audio using an <audio> element.

I'm not sure how many released emscripten sites there are that have been released, but this is not at all rare for Emscripten games - any C/C++ project that uses OpenAL or SDL audio is affected by this, since both of these backends push audio through circular buffers. All audio playback using SDL audio is affected, since SDL mixes its streams manually even if you do short audio clip playback in the code. Streaming audio in OpenAL side is affected.

Manually writing a path that uses <audio> elements is a workaround, but naturally not feasible for all cases.

Comment 18

[(no longer active)]

(In reply to comment #17)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #9)
> > How many Emscripten games are affected by this?  Off the top of my head I
> > remember scumvm and perhaps one other game I can't remember the name of
> > right now.  In my experience this has been a very rare use case for
> > Emscripten games, and in most cases should be possible to work around by
> > just playing the background audio using an <audio> element.
> 
> I'm not sure how many released emscripten sites there are that have been
> released, but this is not at all rare for Emscripten games - any C/C++ project
> that uses OpenAL or SDL audio is affected by this, since both of these backends
> push audio through circular buffers. All audio playback using SDL audio is
> affected, since SDL mixes its streams manually even if you do short audio clip
> playback in the code. Streaming audio in OpenAL side is affected.

Well, those projects are only affected "in theory", since not all games use those APIs for streaming audio tracks.

> Manually writing a path that uses <audio> elements is a workaround, but
> naturally not feasible for all cases.

Yes, I understand that.  Ideally we would enable Web Audio to handle this use case, but honestly very few people have asked for it so far.

Comment 19

[Matthew Gregan [:kinetik]]

Attachment: bug927245_pref_off_v1.patch

(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #16)
> Looks mostly good, except that I don't agree that we should remove the tests
> using the Audio Data APIs.  Instead, we should turn this pref on for those
> mochitests I think, and get rid of the tests when we actually remove the
> code that they're testing.

Okay, I've fixed the tests where necessary.

> Hmm, is there something in this test which
> <http://mxr.mozilla.org/mozilla-central/source/content/media/webaudio/test/
> test_mediaDecoding.html?force=1> doesn't cover?

8 and 16 bit sample formats without resampling.

Comment 20

[(no longer active)]

Comment on attachment 820121 [details] [diff] [review]
bug927245_pref_off_v1.patch

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

Looks great!

::: content/media/webaudio/test/mochitest.ini
@@ +11,5 @@
>    noaudio.webm
>    small-shot-expected.wav
>    small-shot-mono-expected.wav
>    small-shot.ogg
> +  ting-11k-1ch-8b.wav

This file seems to be unused, and I can't find it in the patch.

Comment 21

[Matthew Gregan [:kinetik]]

(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #20)
> > +  ting-11k-1ch-8b.wav
> 
> This file seems to be unused, and I can't find it in the patch.

Gah, sorry, that's from an attempt to integrate the waveDecoder tests into mediaDecoding that ended up being too complicated.  I'll remove it before landing.

Comment 22

[Matthew Gregan [:kinetik]]

Attachment: bug927245_pref_v0.patch

With ting-11k-1ch-8b.wav removed.  Carrying forward Ehsan's review.

I have also removed the all.js change (that actually disables the API) from this version, I'm going to land it now to avoid having to rebase, then all we need to do once bug 927852 and bug 929089 have been fixed is land the pref change itself.

Comment 23

[Matthew Gregan [:kinetik]]

Landed (with a better commit message):
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a82f8dbb719

Comment 24

[Matthew Gregan [:kinetik]]

Attachment: bug927245_pref_off_only_v0.patch

Actually pref off.  Need Shumway and Gaia fixed before we can land this.  Carrying forward Ehsan's review, since this is extracted from the previously reviewed patch.

Comment 25

[Matthew Gregan [:kinetik]]

Attachment: bug927245_remove_v0.patch

And actually remove it.  This can't land for a couple of cycles, and will almost certainly bitrot somewhat in the meantime, but it'll at least serve as a record of all the places that need to be touched to remove this.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a82f8dbb719

Comment 27

[Matthew Gregan [:kinetik]]

Comment on attachment 820754 [details] [diff] [review]
bug927245_pref_off_only_v0.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f5a37dd7a4e

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f5a37dd7a4e

Comment 29

[Matthew Gregan [:kinetik]]

This will hit release in around three weeks when beta (28) becomes release.  Once that happens, I plan to commit the code removal patch to nightly (31).

Comment 30

[Matthew Gregan [:kinetik]]

Attachment: bug927245_remove_v1.patch

Rebased on top of current tree.  Builds locally, try run started: https://tbpl.mozilla.org/?tree=Try&rev=897dc0a16b31

cajbir for the media bits, smaug for the dom bits.

Comment 31

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d5155bd974c

Comment 32

[(no longer active)]

(In reply to comment #31)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1d5155bd974c

\o/

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d5155bd974c

Comment 34

[Kohei Yoshino [:kohei]]

Somehow I have missed this. Added to the docs:

https://developer.mozilla.org/en-US/Firefox/Releases/28/Site_Compatibility
https://developer.mozilla.org/en-US/Firefox/Releases/31/Site_Compatibility

Comment 35

[Kohei Yoshino [:kohei]]

And updated https://developer.mozilla.org/en-US/docs/Introducing_the_Audio_API_Extension