1087944 - Implement the promise version of OfflineAudioContext

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Paul Adenot (:padenot)]

Spec: http://webaudio.github.io/web-audio-api/#widl-OfflineAudioContext-startRendering-Promise-AudioBuffer

Comment 1

[Paul Adenot (:padenot)]

Attachment: Implement the promise version of OfflineAudioContext. r=

Comment 2

[Paul Adenot (:padenot)]

Attachment: Test for the promise returned by OfflineAudioContext.startRendering(). r=

Comment 3

[(no longer active)]

Comment on attachment 8510272 [details] [diff] [review]
Implement the promise version of OfflineAudioContext. r=

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

::: content/media/webaudio/AudioContext.cpp
@@ +629,1 @@
>  AudioContext::StartRendering(ErrorResult& aRv)

Based on the comment below, you can remove [Throws] from this method in the IDL.

@@ +634,3 @@
>    MOZ_ASSERT(mIsOffline, "This should only be called on OfflineAudioContext");
>    if (mIsStarted) {
> +    promise->MaybeReject(NS_ERROR_DOM_INVALID_STATE_ERR);

The spec says that the promise should be created after checking the state here, and the incorrect state should just generate an exception.

FWIW I think that is a spec bug, and your implementation is the right thing to do.  Please make sure to fix the spec accordingly?

::: content/media/webaudio/AudioDestinationNode.cpp
@@ +149,5 @@
>          new OfflineAudioCompletionEvent(context, nullptr, nullptr);
>      event->InitEvent(renderedBuffer);
>      context->DispatchTrustedEvent(event);
> +
> +    aNode->ResolvePromise(renderedBuffer);

This violates the spec by resolving the promise after dispatching the event.  Note that the difference is observable to script.

You should also add a test for this.

Comment 4

[(no longer active)]

Comment on attachment 8510273 [details] [diff] [review]
Test for the promise returned by OfflineAudioContext.startRendering(). r=

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

::: content/media/webaudio/test/test_OfflineAudioContext.html
@@ +13,5 @@
> +var renderedBuffer = null;
> +
> +function setOrCompareRenderedBuffer(aRenderedBuffer) {
> +  if (renderedBuffer) {
> +    is(renderedBuffer, aRenderedBuffer, "Rendered buffers from the event and the promise should be the same");

As I said in the other patch, you need to test the order too.

@@ +66,3 @@
>      compareBuffers(e.renderedBuffer, buf);
>  
> +    ctx.startRendering().then(function() {

Please make sure that startRendering here doesn't throw.

@@ +74,2 @@
>  
> +  ctx.startRendering().then(function(renderedBuffer) {

And here.

Comment 5

[Paul Adenot (:padenot)]

I've addressed your comments, but I have a hard time getting the promise resolved before the event is sent. To make sure the promise is resolved before the callback version, I simply swapped the order of the MaybeResolve and the event dispatch.

I don't know much about promises, but it looks like the function in .then() is called asynchronously in respect to the Promise::MaybeResolve call. Do we have a facility in Gecko to synchronously resolve a promise? i.e., directly calling the js function when calling MaybeResolve from C++.

Then again, even if the promise resolving is async, it should have been enqueued before the event in the event loop, so the function should be called before anyways. I have the feeling I'm really missing something (notably the microtask thing).

Comment 6

[(no longer active)]

Boris is a better person to ask.  It's actually not clear to me what needs to happen here.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> it looks like the function in .then() is called asynchronously in respect to the
> Promise::MaybeResolve call

Correct.

And per spec, it's called async wrt the promise being resolved.

> Do we have a facility in Gecko to synchronously resolve a promise?

Resolving a promise is synchronous.  Per spec it queues up ES Jobs to run the then() callbacks.  The actual running happens async.

Now exactly how it interacts with the event stuff... this is not defined in the specs at the moment as far as I can tell.  If promise handlers are supposed to run at microtask checkpoints, then that would be when the first event listener returns.  That's assuming that the event dispatch happens right after the promise resolution.  Note, however, that this spec probably needs to do the event firing off a task or something; the way it does it right now doesn't really make sense precisely because it means the ordering of the event with other tasks is not really defined.

Comment 8

[Paul Adenot (:padenot)]

Anne, Boris seems unavailable, would you mind having a look at comment 5, 6, 7, and this one, as well as the spec text of `OfflineAudioContext.startRendering` at [0] ?

Here is what I understood:
- When a promise is settled, it queues a microtask to run the function passed to then(), etc...
- microtasks run during microtask checkpoints, and that's when all scripts return, or when the event loop is spining, but after running the "normal" (non-micro) task for this event loop spin. 
- When the rendering is finished on the rendering thread, the Web Audio API specs that a simple event named "complete" should be fired at the OfflineAudioContext, but it does not say that the event must be  fired off a task, so it's not clear when the microtask checkpoint will happen. If we fire the "complete" event off a task, then the function passed into the "then()" function of the Promise will run right after the "oncomplete" callback. 

Does that make sense, or am I missing something ?

In practice, Gecko and Blink (and probably WebKit, this part of their implementation was written before the fork) fire "complete" off a task, so I can just change the spec and land this (with ehsan's comments addressed).

[0]: http://webaudio.github.io/web-audio-api/#methods-1, 4.3.1 and 4.3.2

Comment 9

[Anne (:annevk)]

The specification is wrong. You cannot dispatch an event from an algorithm that runs in parallel with JavaScript. E.g. while that algorithm runs the page might have invoked alert() and your event cannot fire will that alert() is there.

Therefore, what you need to do is queue a task to dispatch the event. That needs to be in the specification, and in your code. And you'll find that such a queued task will run after the promise callbacks.

Comment 10

[Paul Adenot (:padenot)]

Attachment: Implement the promise version of OfflineAudioContext. r=

This implements what Boris and Anne told in this bug, I think.

Comment 11

[Paul Adenot (:padenot)]

Attachment: Test for the promise returned by OfflineAudioContext.startRendering(). r=

And the updated test.

Comment 12

[(no longer active)]

Comment on attachment 8525408 [details] [diff] [review]
Implement the promise version of OfflineAudioContext. r=

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

::: dom/media/webaudio/AudioDestinationNode.cpp
@@ +126,5 @@
>        *aFinished = true;
>      }
>    }
>  
> +  class OnCompleteTask : public nsRunnable

Nit: MOZ_FINAL, please.

::: dom/media/webaudio/AudioDestinationNode.h
@@ +103,5 @@
>  
>    nsCOMPtr<nsIAudioChannelAgent> mAudioChannelAgent;
>  
>    nsRefPtr<EventProxyHandler> mEventProxyHelper;
> +  nsRefPtr<Promise> mOfflineRenderingPromise;

You need to add this new member to the list of CCed members.

Comment 13

[Paul Adenot (:padenot)]

Comment on attachment 8525408 [details] [diff] [review]
Implement the promise version of OfflineAudioContext. r=

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

Hm, I also need a review from a DOM peer.

Smaug, this is like the previous one for decodeAudioContext, we add a promise to an API that was callback base. I've implemented what Anne said in comment 9.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8525408 [details] [diff] [review]
Implement the promise version of OfflineAudioContext. r=

>+already_AddRefed<Promise>
> AudioContext::StartRendering(ErrorResult& aRv)
> {
>+  nsCOMPtr<nsIGlobalObject> parentObject = do_QueryInterface(GetParentObject());
>+  nsRefPtr<Promise> promise = Promise::Create(parentObject, aRv);
>+
>   MOZ_ASSERT(mIsOffline, "This should only be called on OfflineAudioContext");
>   if (mIsStarted) {
>-    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>-    return;
>+    promise->MaybeReject(NS_ERROR_DOM_INVALID_STATE_ERR);
>+    return promise.forget();
>   }
Binding layer deals with exceptions, and rejects the promise automatically.
(that is an odd special case coming from WebIDL spec http://heycam.github.io/webidl/#es-operations "If the operation has a return type that is a promise type, then:  ....")
And I was told you don't even need to create promise. Just throw normally, and binding layer will create a Promise and reject it for you.

That fixed, r+

Comment 15

[Paul Adenot (:padenot)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6dad5fe1d22b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e82170d665d8

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out for Linux64 mochitest-e10s orange.
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4234778&repo=mozilla-inbound

Comment 17

[Paul Adenot (:padenot)]

https://tbpl.mozilla.org/?tree=Try&rev=121706ec57ad

Comment 18

[Paul Adenot (:padenot)]

Relanded after having fixed a race in the test:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c6be963121ae
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c213f97e0eec

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c6be963121ae
https://hg.mozilla.org/mozilla-central/rev/c213f97e0eec

Comment 20

[Chris Mills [:cmills]]

I've documented this at

https://developer.mozilla.org/en-US/docs/Web/API/OfflineAudioContext.startRendering_%28promise%29

A quick tech review would be great, when you get time. Cheers!