1000264 - Implement the new prototype for decodeAudioData, that returns a promise

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Paul Adenot (:padenot)]

The new spec text is at [1].

[1]: http://webaudio.github.io/web-audio-api/#widl-AudioContext-decodeAudioData-Promise-AudioBuffer--ArrayBuffer-audioData-DecodeSuccessCallback-successCallback-DecodeErrorCallback-errorCallback

Comment 1

[(no longer active)]

Yay! :-)

Comment 2

[Paul Adenot (:padenot)]

Attachment: Allow an object that inherits from nsWrapperCache but not nsISupports to be passed as a Promise::MaybeResolve argument. r=

Boris, Ehsan told me this would work, but to ask you for this one.

Basically, we need to pass in an AudioBuffer when a Promise return from
AudioContext.decodeAudioData is resolved, and AudioBuffers are not inheriting
from nsISupports. Do you think this is this the right thing to do?

Comment 3

[Paul Adenot (:padenot)]

Attachment: decodeaudiodata-promises

Comment 4

[Paul Adenot (:padenot)]

Attachment: test

And some tests. This is separate from test_mediaDecode.html, because it only test that the promise work properly, not that the decoding works properly.

Comment 5

[(no longer active)]

Comment on attachment 8509676 [details] [diff] [review]
decodeaudiodata-promises

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

::: content/media/webaudio/MediaBufferDecoder.cpp
@@ +591,5 @@
>      ErrorResult rv;
>      mFailureCallback->Call(rv);
>    }
>  
> +  mPromise->MaybeReject(NS_ERROR_FAILURE);

The spec says that we should reject the promise with EncodingError in this case, right?

Comment 6

[(no longer active)]

Comment on attachment 8509679 [details] [diff] [review]
test

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

::: content/media/webaudio/MediaBufferDecoder.cpp
@@ +544,5 @@
>    // do about it here.
>    ErrorResult rv;
> +  if (mSuccessCallback) {
> +    mSuccessCallback->Call(*mOutput, rv);
> +  }

I'd collapse all of these patches into one when landing, FWIW.

::: content/media/webaudio/test/test_decodeAudioDataPromise.html
@@ +30,5 @@
> +  p.then(function(data) {
> +    ok(false, "Promise should not resolve with an invalid source buffer.");
> +    finish();
> +  }).catch(function() {
> +    ok(true, "Promise should be rejected with an invalid source buffer.");

Please test the error that the promise is rejected with (see my comment about the other patch.)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8509670 [details] [diff] [review]
Allow an object that inherits from nsWrapperCache but not nsISupports to be passed as a Promise::MaybeResolve argument. r=

Yeah, this seems fine.  Especially given the IsDOMBinding() assert there.

r=me

Comment 8

[Paul Adenot (:padenot)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #6)
> Comment on attachment 8509679 [details] [diff] [review]
> test
> 
> Review of attachment 8509679 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/webaudio/MediaBufferDecoder.cpp
> @@ +544,5 @@
> >    // do about it here.
> >    ErrorResult rv;
> > +  if (mSuccessCallback) {
> > +    mSuccessCallback->Call(*mOutput, rv);
> > +  }
> 
> I'd collapse all of these patches into one when landing, FWIW.

Ha yes, for some reason I qrefed C++ code in the test. But yes, I'll squash them all into one.

> ::: content/media/webaudio/test/test_decodeAudioDataPromise.html
> @@ +30,5 @@
> > +  p.then(function(data) {
> > +    ok(false, "Promise should not resolve with an invalid source buffer.");
> > +    finish();
> > +  }).catch(function() {
> > +    ok(true, "Promise should be rejected with an invalid source buffer.");
> 
> Please test the error that the promise is rejected with (see my comment
> about the other patch.)

Hrm, yes. It confuses me that EncodingError is defined in the StringEncoding spec, whereas we use it here to refer to audio encoding. That said, the message is not wrong per se ("EncodingError: The given encoding is not supported."), but decodeAudioData could fail in other ways with legal files.

Comment 9

[Paul Adenot (:padenot)]

Attachment: Make AudioContext.decodeAudioData return a promise. r=

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 8510190 [details] [diff] [review]
Make AudioContext.decodeAudioData return a promise. r=

I forgot I needed another review for the WebIDL change.

Spec is here: http://webaudio.github.io/web-audio-api/#widl-AudioContext-decodeAudioData-Promise-AudioBuffer--ArrayBuffer-audioData-DecodeSuccessCallback-successCallback-DecodeErrorCallback-errorCallback

According to annevk, in the spec, the callback should not be fired directly, but off a task, I filed a bug and written a patch for the spec here: https://github.com/WebAudio/web-audio-api/issues/418. This is effectively what we do in Gecko (NS_DispatchToMainThread from the decoding thread, and then call the callback from there).

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8510190 [details] [diff] [review]
Make AudioContext.decodeAudioData return a promise. r=

>+  if (rv.Failed()) {

Why not do this right after the CreatePromise call?

r=me

Comment 12

[Paul Adenot (:padenot)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/488700194519
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c2c37b2e9fac
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0c9407b0e481

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for e10s test failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/292ed84594c1

https://treeherder.mozilla.org/logviewer.html#?job_id=3864318&repo=mozilla-inbound

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Looks like it was a straight-up Linux failure (not e10s related).

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Make that a failure across the board. Please make sure this is green on Try before relanding ;)

Comment 16

[Paul Adenot (:padenot)]

Attachment: Stop checking exceptions on decodeAudioData now that it uses promises, and check it in the promise test file instead. r=

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8522394 [details] [diff] [review]
Stop checking exceptions on decodeAudioData now that it uses promises, and check it in the promise test file instead. r=

>+    ok(e.name == "TypeError", "The error should be EncodingError");

I don't see how the name check there says anything about it being EncodingError.

r=me apart from that

Comment 18

[Paul Adenot (:padenot)]

Yes sorry, it was jus me being confused.

Comment 19

[Paul Adenot (:padenot)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/32ffa20c5e8f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6ead619835d3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ceed59c28910
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/854091113b03

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/32ffa20c5e8f
https://hg.mozilla.org/mozilla-central/rev/6ead619835d3
https://hg.mozilla.org/mozilla-central/rev/ceed59c28910
https://hg.mozilla.org/mozilla-central/rev/854091113b03