Closed Bug 1087944 Opened 10 years ago Closed 9 years ago

Implement the promise version of OfflineAudioContext

Categories

(Core :: Web Audio, defect)

32 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: padenot, Assigned: padenot)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 2 obsolete files)

Attachment #8510272 - Flags: review?(ehsan.akhgari)
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.
Attachment #8510272 - Flags: review?(ehsan.akhgari) → review-
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.
Attachment #8510273 - Flags: review?(ehsan.akhgari) → review+
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).
Flags: needinfo?(ehsan.akhgari)
Boris is a better person to ask.  It's actually not clear to me what needs to happen here.
Flags: needinfo?(ehsan.akhgari) → needinfo?(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.
Flags: needinfo?(bzbarsky)
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
Flags: needinfo?(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.
Flags: needinfo?(annevk)
This implements what Boris and Anne told in this bug, I think.
Attachment #8525408 - Flags: review?(ehsan.akhgari)
Attachment #8510272 - Attachment is obsolete: true
Attachment #8510273 - Attachment is obsolete: true
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.
Attachment #8525408 - Flags: review?(ehsan.akhgari) → review+
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.
Attachment #8525408 - Flags: review?(bugs)
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+
Attachment #8525408 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/c6be963121ae
https://hg.mozilla.org/mozilla-central/rev/c213f97e0eec
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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!
You need to log in before you can comment on or make changes to this bug.