Remove deprecated Audio Data API implementation

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

({dev-doc-complete, site-compat})

Trunk
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

Assignee

Description

6 years ago
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.
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.
Flags: needinfo?(etienne)
Assignee

Comment 2

6 years ago
Posted patch wip v0 (obsolete) — Splinter Review
Assignee

Comment 3

6 years ago
Posted file test_waveDecoder.html (obsolete) —
(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.
Assignee

Comment 4

6 years ago
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
(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.
Flags: needinfo?(etienne)

Comment 6

6 years ago
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

6 years ago
CCing some Emscripten folks to keep them in the loop.

Comment 8

6 years ago
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

6 years ago
(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.

Updated

6 years ago
Depends on: 927852
Assignee

Comment 10

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

Comment 11

6 years ago
(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
Assignee

Comment 12

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

Comment 14

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

Updated

6 years ago
Attachment #818194 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #818238 - Attachment is obsolete: true
Assignee

Comment 15

6 years ago
Posted patch bug927245_pref_off_v0.patch (obsolete) — Splinter Review
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Attachment #819513 - Flags: review?(ehsan)

Comment 16

6 years ago
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?
Attachment #819513 - Flags: review?(ehsan) → review-

Updated

6 years ago
Depends on: 929089

Comment 17

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

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

Comment 19

6 years ago
Posted patch bug927245_pref_off_v1.patch (obsolete) — Splinter Review
(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.
Attachment #819513 - Attachment is obsolete: true
Attachment #820121 - Flags: review?(ehsan)

Comment 20

6 years ago
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.
Attachment #820121 - Flags: review?(ehsan) → review+
Assignee

Comment 21

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

Comment 22

6 years ago
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.
Attachment #820121 - Attachment is obsolete: true
Attachment #820753 - Flags: review+
Assignee

Comment 23

6 years ago
Landed (with a better commit message):
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a82f8dbb719
Whiteboard: [leave open]
Assignee

Updated

6 years ago
Attachment #820753 - Flags: checkin+
Assignee

Comment 24

6 years ago
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.
Attachment #820754 - Flags: review+
Assignee

Comment 25

6 years ago
Posted patch bug927245_remove_v0.patch (obsolete) — Splinter Review
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.

Updated

6 years ago
No longer depends on: 929089
Assignee

Updated

6 years ago
Blocks: 948267
Assignee

Comment 29

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

Comment 30

5 years ago
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.
Attachment #820759 - Attachment is obsolete: true
Attachment #8400338 - Flags: review?(cajbir.bugzilla)
Attachment #8400338 - Flags: review?(bugs)

Updated

5 years ago
Attachment #8400338 - Flags: review?(cajbir.bugzilla) → review+
Assignee

Updated

5 years ago
Whiteboard: [leave open]
Attachment #8400338 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/1d5155bd974c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Blocks: 799524
You need to log in before you can comment on or make changes to this bug.