Implement the promise version of OfflineAudioContext

RESOLVED FIXED in mozilla37

Status

()

Core
Web Audio
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: padenot, Assigned: padenot)

Tracking

({dev-doc-complete})

32 Branch
mozilla37
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Comment 1

4 years ago
Created attachment 8510272 [details] [diff] [review]
Implement the promise version of OfflineAudioContext. r=
Attachment #8510272 - Flags: review?(ehsan.akhgari)
(Assignee)

Comment 2

4 years ago
Created attachment 8510273 [details] [diff] [review]
Test for the promise returned by OfflineAudioContext.startRendering(). r=
Attachment #8510273 - Flags: review?(ehsan.akhgari)

Comment 3

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

4 years ago
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+
(Assignee)

Comment 5

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

Updated

4 years ago
Flags: needinfo?(ehsan.akhgari)

Comment 6

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

Comment 8

4 years ago
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)

Comment 9

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

Comment 10

4 years ago
Created attachment 8525408 [details] [diff] [review]
Implement the promise version of OfflineAudioContext. r=

This implements what Boris and Anne told in this bug, I think.
Attachment #8525408 - Flags: review?(ehsan.akhgari)
(Assignee)

Updated

4 years ago
Attachment #8510272 - Attachment is obsolete: true
(Assignee)

Comment 11

4 years ago
Created attachment 8525409 [details] [diff] [review]
Test for the promise returned by OfflineAudioContext.startRendering(). r=

And the updated test.
(Assignee)

Updated

4 years ago
Attachment #8510273 - Attachment is obsolete: true

Comment 12

4 years ago
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+
(Assignee)

Comment 13

4 years ago
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+
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/c6be963121ae
https://hg.mozilla.org/mozilla-central/rev/c213f97e0eec
Status: NEW → RESOLVED
Last Resolved: 4 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!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.