Open Bug 1407354 Opened 7 years ago Updated 2 years ago

Simplify MediaRecorder's Runnables

Categories

(Core :: Audio/Video: Recording, enhancement, P2)

Product:
Core
Component:
Audio/Video: Recording
Type:
enhancement

Tracking

()

Tracking Flags:
Tracking Status
firefox58 --- affected

People

(Reporter: bryce, Unassigned)

Details

Attachments

(3 files, 1 obsolete file)

Bug 1407354 - Replace MediaRecorder Session's DestroyRunnable with a NewRunnableMethod based impl.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1407354 - Remove EncoderErrorNotifierRunnable from MediaRecorder Session as it's no longer used.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1407354 - Replace MediaRecorder Session's DispatchStartEventRunnable with a NewRunnableMethod based impl.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details 
The Runnables used in MediaRecorder can be replaced with MozPromise + InvokeAsync based implementations. This would reduce the amount of boilerplate Runnable code, and allow for greater flexibility in how we call the code currently in Runnables. This bug doesn't entail exposing Promises at the JS API level, but using MozPromise internally is likely to make such a change easier should we chose to undertake it in future.
Comment on attachment 8917178 [details]
Bug 1407354 - Remove EncoderErrorNotifierRunnable from MediaRecorder Session as it's no longer used.

https://reviewboard.mozilla.org/r/188196/#review193504
Attachment #8917178 - Flags: review?(apehrson) → review+
Comment on attachment 8917179 [details]
Bug 1407354 - Replace MediaRecorder Session's DispatchStartEventRunnable with a NewRunnableMethod based impl.

https://reviewboard.mozilla.org/r/188198/#review193548

::: dom/media/MediaRecorder.cpp:441
(Diff revision 1)
>  
>    typedef MozPromise<bool, bool, /* IsExclusive = */ true>
> +    StartEventPromise;
> +
> +  // Fire start event and set mimeType, run in main thread task.
> +  RefPtr<StartEventPromise> DispatchStartEvent()

I don't see how this needs to return a MozPromise.

The return value is not used and there's no extra async step before resolving it.

Could you make it a void method that is wrapped in NewRunnableMethod, [1], and dispatched via NS_DispatchToMainThread, [2]?

[1] http://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/xpcom/threads/nsThreadUtils.h#1388
[2] http://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/xpcom/threads/nsThreadUtils.cpp#275
Attachment #8917179 - Flags: review?(apehrson)
Comment on attachment 8917177 [details]
Bug 1407354 - Replace MediaRecorder Session's PushBlobRunnable with a NewRunnableMethod based impl.

https://reviewboard.mozilla.org/r/188194/#review193552

::: dom/media/MediaRecorder.cpp:499
(Diff revision 1)
> +  typedef MozPromise<bool, bool, /* IsExclusive = */ true>
> +    PushBlobPromise;
> +
> +  // Main thread task.
> +  // Create a blob event and send back to client.
> +  RefPtr<PushBlobPromise> PushBlob()

Like the third patch I don't see why this is a MozPromise. A void method seems OK and would be simpler.

::: dom/media/MediaRecorder.cpp:510
(Diff revision 1)
> +      // We're defensive just in case, but this shouldn't happen
> +      MOZ_ASSERT_UNREACHABLE("PushBlob() called on shutdown session");
> +      return PushBlobPromise::CreateAndResolve(true, __func__);
> +    }
> +
> +    nsresult rv = mRecorder->CreateAndDispatchBlobEvent(GetEncodedData());

Spec says to stop gathering data to the blob (optionally start gathering to a new blob, depending on reason), *then* queue a task that fires a blob event named "dataavailable".

We should look at getting these algorithms right, but a followup is fine.
Attachment #8917177 - Flags: review?(apehrson)
Comment on attachment 8917176 [details]
Bug 1407354 - Replace MediaRecorder Session's DestroyRunnable with a NewRunnableMethod based impl.

https://reviewboard.mozilla.org/r/188192/#review193558

::: dom/media/MediaRecorder.cpp:579
(Diff revision 1)
> +    DestroySessionPromise;
> +
> +  // Perform the steps needed to destroy a session. May reenter as this
> +  // function needs to stop the recorder before the session can be properly
> +  // shut down.
> +  RefPtr<DestroySessionPromise> Destroy()

Like the third patch I don't see why this needs to be a MozPromise. Here you are doing some stuff async, but the returned promise is not used.

::: dom/media/MediaRecorder.cpp:593
(Diff revision 1)
> +    RefPtr<DestroySessionPromise> destroyPromise =
> +      mDestroyPromise.Ensure(__func__);
> +
> +    if (!mRecorder) {
> +      // We're defensive just in case, but this shouldn't happen
> +      MOZ_ASSERT_UNREACHABLE("Destroy() called on shutdown session");

This is ambiguous. Consider making it "already shutdown session" -- I keep reading it as "Destroy() called on shutdown-session".

::: dom/media/MediaRecorder.cpp:622
(Diff revision 1)
> +      [self]() {
> +      gSessions.RemoveEntry(self);
> +      if (gSessions.Count() == 0 && gMediaRecorderShutdownBlocker) {
> +        // All sessions finished before shutdown, no need to keep the blocker.
> +        RefPtr<nsIAsyncShutdownClient> barrier = GetShutdownBarrier();
> +        barrier->RemoveBlocker(gMediaRecorderShutdownBlocker);
> +        gMediaRecorderShutdownBlocker = nullptr;
> +      }
> +      self->mDestroyPromise.Resolve(true, __func__);
> +    },

Indentation

::: dom/media/MediaRecorder.cpp:632
(Diff revision 1)
> +      [self]() {
> +      MOZ_ASSERT_UNREACHABLE("Unexpected reject");
> +      self->mDestroyPromise.Reject(false, __func__);
> +    });

Indentation

::: dom/media/MediaRecorder.cpp:900
(Diff revision 1)
> -    if (NS_FAILED(NS_DispatchToMainThread(new DestroyRunnable(this)))) {
> -      MOZ_ASSERT(false, "NS_DispatchToMainThread DestroyRunnable failed");
> +    InvokeAsync(
> +      GetMainThreadSerialEventTarget(), this, __func__, &Session::Destroy);

Without too much thought on the consequences I think a nicer pattern here would be to just call Destroy() and let that handle the asyncness.
Attachment #8917176 - Flags: review?(apehrson)
Comment on attachment 8917179 [details]
Bug 1407354 - Replace MediaRecorder Session's DispatchStartEventRunnable with a NewRunnableMethod based impl.

https://reviewboard.mozilla.org/r/188198/#review193548

> I don't see how this needs to return a MozPromise.
> 
> The return value is not used and there's no extra async step before resolving it.
> 
> Could you make it a void method that is wrapped in NewRunnableMethod, [1], and dispatched via NS_DispatchToMainThread, [2]?
> 
> [1] http://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/xpcom/threads/nsThreadUtils.h#1388
> [2] http://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/xpcom/threads/nsThreadUtils.cpp#275

Agreed that we don't need to use MozPromise return by any of the altered functions here. My thinking here is that the recorder currently has a lot of functionality with potentially aysnc tasks, and it's not always clear when these tasks complete. In these trivial cases I may have gone overboard, as it's fairly aparrent that there is nothing async happening (though to someone unfamiliar they may be unsure if the dispatch is sync or not). However, I want to provide some sort programmatic contract to indicate completetion of various tasks. With these changes those contacts are not being levaraged, and I'm receptive to the idea that only when we can leverage the contract should we actually add it to the code.

Thoughts? I'm not opposed to the recommended changes, but would appreciate if you have ideas on the above.
Rank: 25
Priority: -- → P2
(In reply to Bryce Van Dyk (:SingingTree) from comment #9)
> Agreed that we don't need to use MozPromise return by any of the altered
> functions here. My thinking here is that the recorder currently has a lot of
> functionality with potentially aysnc tasks, and it's not always clear when
> these tasks complete. In these trivial cases I may have gone overboard, as
> it's fairly aparrent that there is nothing async happening (though to
> someone unfamiliar they may be unsure if the dispatch is sync or not).
> However, I want to provide some sort programmatic contract to indicate
> completetion of various tasks. With these changes those contacts are not
> being levaraged, and I'm receptive to the idea that only when we can
> leverage the contract should we actually add it to the code.
> 
> Thoughts? I'm not opposed to the recommended changes, but would appreciate
> if you have ideas on the above.

There's a lot of asyncness but I don't think any calls to these method would be made except from within the class itself. Then it makes sense to only MozPromisify them when we need the functionality of tracking when the asyncness has completed.

I think if you get rid of the classes like your patches do and use NewRunnableMethod instead, switching to a MozPromise in the future is simple enough to tag along on other changes that require them.
Comment on attachment 8917176 [details]
Bug 1407354 - Replace MediaRecorder Session's DestroyRunnable with a NewRunnableMethod based impl.

https://reviewboard.mozilla.org/r/188192/#review193558

> Without too much thought on the consequences I think a nicer pattern here would be to just call Destroy() and let that handle the asyncness.

There's a whole lot of implicit aysnc scheduling hangning off this call which I'd like to see ironed out (I guess with a full internal promisificaiton revamp). For now I'll leave it as is, as changing to a direct call (either with mNEedSessionEndTake = false; before or after) breaks existing behaviour.
Summary: Promisify MediaRecorder's Runnables → Simplify MediaRecorder's Runnables
Comment on attachment 8917176 [details]
Bug 1407354 - Replace MediaRecorder Session's DestroyRunnable with a NewRunnableMethod based impl.

https://reviewboard.mozilla.org/r/188192/#review193558

> There's a whole lot of implicit aysnc scheduling hangning off this call which I'd like to see ironed out (I guess with a full internal promisificaiton revamp). For now I'll leave it as is, as changing to a direct call (either with mNEedSessionEndTake = false; before or after) breaks existing behaviour.

Makes sense!
Comment on attachment 8917176 [details]
Bug 1407354 - Replace MediaRecorder Session's DestroyRunnable with a NewRunnableMethod based impl.

https://reviewboard.mozilla.org/r/188192/#review194360

::: dom/media/MediaRecorder.cpp:601
(Diff revision 2)
> +    if (!mStopIssued) {
> +      // Need to stop our session before we destroy it
> +      mRecorder->StopForSessionDestruction();
> +      // Queue up another Destroy call for after the stop has taken place
> +      NS_DispatchToMainThread(NewRunnableMethod(
> +        "dom::MediaRecorder:Session::Destroy()", this, &Session::Destroy));

My quick search for NewRunnableMethod indicated the `()` shouldn't be part of the label.

::: dom/media/MediaRecorder.cpp:888
(Diff revision 2)
> -    if (NS_FAILED(NS_DispatchToMainThread(new DestroyRunnable(this)))) {
> -      MOZ_ASSERT(false, "NS_DispatchToMainThread DestroyRunnable failed");
> +    NS_DispatchToMainThread(NewRunnableMethod(
> +      "dom::MediaRecorder:Session::Destroy()", this, &Session::Destroy));

Remove the `()`

::: dom/media/MediaRecorder.cpp:944
(Diff revision 2)
> -    if (NS_FAILED(NS_DispatchToMainThread(
> -                  new DestroyRunnable(this)))) {
> +    NS_DispatchToMainThread(NewRunnableMethod(
> +      "dom::MediaRecorder:Session::Destroy()", this, &Session::Destroy));

Remove the `()`
Attachment #8917176 - Flags: review?(apehrson) → review+
Comment on attachment 8917177 [details]
Bug 1407354 - Replace MediaRecorder Session's PushBlobRunnable with a NewRunnableMethod based impl.

https://reviewboard.mozilla.org/r/188194/#review194362

::: dom/media/MediaRecorder.cpp:453
(Diff revision 2)
> -    if (NS_FAILED(NS_DispatchToMainThread(new PushBlobRunnable(this)))) {
> -      MOZ_ASSERT(false, "RequestData NS_DispatchToMainThread failed");
> +    NS_DispatchToMainThread(NewRunnableMethod(
> +      "dom::MediaRecorder:Session::PushBlob()", this, &Session::PushBlob));

We should still handle the dispatch failure -- this goes for the other patches too.

::: dom/media/MediaRecorder.cpp:550
(Diff revision 2)
> -      if (NS_FAILED(NS_DispatchToMainThread(new PushBlobRunnable(this)))) {
> -        MOZ_ASSERT(false, "NS_DispatchToMainThread PushBlobRunnable failed");
> +      NS_DispatchToMainThread(NewRunnableMethod(
> +        "dom::MediaRecorder:Session::PushBlob()", this, &Session::PushBlob));

Add back the assert if the dispatch fails

::: dom/media/MediaRecorder.cpp:867
(Diff revision 2)
> -      if (NS_FAILED(NS_DispatchToMainThread(new PushBlobRunnable(this)))) {
> -        MOZ_ASSERT(false, "NS_DispatchToMainThread PushBlobRunnable failed");
> +      NS_DispatchToMainThread(NewRunnableMethod(
> +        "dom::MediaRecorder:Session::PushBlob()", this, &Session::PushBlob));

Add back the assert if the dispatch fails
Attachment #8917177 - Flags: review?(apehrson) → review+
Comment on attachment 8917176 [details]
Bug 1407354 - Replace MediaRecorder Session's DestroyRunnable with a NewRunnableMethod based impl.

https://reviewboard.mozilla.org/r/188192/#review194366

::: dom/media/MediaRecorder.cpp:600
(Diff revision 2)
> +      NS_DispatchToMainThread(NewRunnableMethod(
> +        "dom::MediaRecorder:Session::Destroy()", this, &Session::Destroy));

Add back the assert if the dispatch fails

::: dom/media/MediaRecorder.cpp:888
(Diff revision 2)
> -    if (NS_FAILED(NS_DispatchToMainThread(new DestroyRunnable(this)))) {
> -      MOZ_ASSERT(false, "NS_DispatchToMainThread DestroyRunnable failed");
> +    NS_DispatchToMainThread(NewRunnableMethod(
> +      "dom::MediaRecorder:Session::Destroy()", this, &Session::Destroy));

Add back the assert if the dispatch fails

::: dom/media/MediaRecorder.cpp:944
(Diff revision 2)
> -    if (NS_FAILED(NS_DispatchToMainThread(
> -                  new DestroyRunnable(this)))) {
> +    NS_DispatchToMainThread(NewRunnableMethod(
> +      "dom::MediaRecorder:Session::Destroy()", this, &Session::Destroy));

Add back the assert if the dispatch fails
Comment on attachment 8917179 [details]
Bug 1407354 - Replace MediaRecorder Session's DispatchStartEventRunnable with a NewRunnableMethod based impl.

https://reviewboard.mozilla.org/r/188198/#review194368

::: dom/media/MediaRecorder.cpp:814
(Diff revision 2)
> -        new DispatchStartEventRunnable(this, NS_LITERAL_STRING("start")));
> +        NewRunnableMethod("dom::MediaRecorder:Session::DispatchStartEvent()",
> +                          this,
> +                          &Session::DispatchStartEvent));

This didn't handle failures before, but I think an assert if the dispatch fails makes sense to add.

::: dom/media/MediaRecorder.cpp:864
(Diff revision 2)
> -        new DispatchStartEventRunnable(this, NS_LITERAL_STRING("start")));
> +        NewRunnableMethod("dom::MediaRecorder:Session::DispatchStartEvent()",
> +                          this,
> +                          &Session::DispatchStartEvent));

This didn't handle failures before, but I think an assert if the dispatch fails makes sense to add.
Attachment #8917179 - Flags: review?(apehrson) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 50d56b7b3f9c -d 398ef4e2ae5d: rebasing 426679:50d56b7b3f9c "Bug 1407354 - Replace MediaRecorder Session's DestroyRunnable with a NewRunnableMethod based impl. r=pehrsons"
merging dom/media/MediaRecorder.cpp
warning: conflicts while merging dom/media/MediaRecorder.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Looks like there are some moderate sized clashes here. I'm gonna take some time to rework this.
Attachment #8917177 - Attachment is obsolete: true
Requesting re-review as the request has changed a fair amount. I'd to keep going and use promises so we can do PushBlob()->Then(Destroy), but I'm not sure it's the most efficient usage of my time at the moment, so will leave it at this pending feedback.
Flags: needinfo?(apehrson)
I assume I should look at all the patches. But what happened to the patch for PushBlobRunnable?
Flags: needinfo?(apehrson) → needinfo?(bvandyk)
Comment on attachment 8917176 [details]
Bug 1407354 - Replace MediaRecorder Session's DestroyRunnable with a NewRunnableMethod based impl.

https://reviewboard.mozilla.org/r/188192/#review194794

::: dom/media/MediaRecorder.cpp:940
(Diff revision 4)
> -    // Set mNeedSessionEndTask to false because the
> -    // ExtractRunnable/DestroyRunnable will take the response to
> +    // Encoder is initialized, we don't need an explicit end task as normal
> +    // shutdown procedure will handle things correctly

I'd try to be more detailed here, and capture that the Shutdown of MediaEncoder will trigger the end task.
Comment on attachment 8917179 [details]
Bug 1407354 - Replace MediaRecorder Session's DispatchStartEventRunnable with a NewRunnableMethod based impl.

https://reviewboard.mozilla.org/r/188198/#review194798

::: dom/media/MediaRecorder.cpp:546
(Diff revision 4)
>    {
>      MOZ_ASSERT(NS_IsMainThread());
>      MOZ_ASSERT(mShutdownPromise);
>      LOG(LogLevel::Debug, ("Session.~Session (%p)", this));
>    }
>    // Pull encoded media data from MediaEncoder and put into MutableBlobStorage.

This comment must belong to `Extract()`

::: dom/media/MediaRecorder.cpp:547
(Diff revision 4)
>      MOZ_ASSERT(NS_IsMainThread());
>      MOZ_ASSERT(mShutdownPromise);
>      LOG(LogLevel::Debug, ("Session.~Session (%p)", this));
>    }
>    // Pull encoded media data from MediaEncoder and put into MutableBlobStorage.
> +  // Fire start event and set mimeType, run in main thread task.

Please mention that it's synchronously fired.
(In reply to Andreas Pehrson [:pehrsons] from comment #31)
> I assume I should look at all the patches. But what happened to the patch
> for PushBlobRunnable?

I had a look at PushBlobRunnable (it has changed to encompass more complex behaviour since I started my original patches for this), moving its behaviour to a Session method(s), and using promises to deal with callbacks/destruction. I had something mostly working, but there were some promise resolution issues at shutdown. I time boxed the issue and decided to put it down since I didn't have something robust by the end of the time box.

I would like to have bits like this rewritten to use promises to flatten some of our async calls and make them easier to understand. Right now I find some of the logic fairly finicky and do see value in making it easier to understand/maintain. However, I'm mindful of spending too much time working on what amounts to internal changes to the recorder.

If you think it worthwhile I'm more than happy to keep looking into it, but I don't want to allocate further time here if it could be better spent elsewhere.
Flags: needinfo?(bvandyk)
I think these simplifications are valuable to everyone maintaining MediaRecorder now and in the future.
Noted. Will look into seeing through the changes to PushBlob as well.
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.