1168008 - Implement Completion MediaPromises

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

At present, one of the biggest conceptual differences between MediaPromises and JS promises is that JS promises can be chained (|foo()->then(bar)->then(baz)|), whereas MediaPromises can't.

This chaining gives JS promises a huge amount of power - the promise that bar() returns can hook into an arbitrarily complex network of asynchronous work, all of which is elegantly abstracted away. I initially avoided implementing this behavior for MediaPromises because I couldn't figure out how to make it work with the type system, and because I didn't have a use case. But I've figured out a way to make it work, and have a use case in bug 1163223.

In that bug, I ran into the situation where we want to quarantine the first sample from one channel (audio or video) until we have the first sample from the other channel, so that we can compute the min of the two start times and adjust the samples (and all subsequent samples) by this offset. This is kind of an icky thing to handle in either the demuxer or the MDSM, but it can be very elegantly handled somewhere in between. ;-)

I have patches to do this, which also unify Then() and RefableThen(). Here's what usage of this looks like (from my patches in bug 1163223).

Before:

mAudioDataRequest.Begin(
  ProxyMediaCall(DecodeTaskQueue(), mReader.get(),
                 __func__, &MediaDecoderReader::RequestAudioData)
    ->RefableThen(TaskQueue(), __func__, this,
                  &MediaDecoderStateMachine::OnAudioDecoded,
                  &MediaDecoderStateMachine::OnAudioNotDecoded)
);

After:

mAudioDataRequest.Begin(
  ProxyMediaCall(DecodeTaskQueue(), mReader.get(),
                 __func__, &MediaDecoderReader::RequestAudioData)
  ->Then(TaskQueue(), __func__, mStartTimeAdjuster.get(),
         &StartTimeAdjuster::AdjustSample<AudioDataPromise>,
         &StartTimeAdjuster::Pass)
  ->CompletionPromise()
  ->Then(TaskQueue(), __func__, this,
         &MediaDecoderStateMachine::OnAudioDecoded,
         &MediaDecoderStateMachine::OnAudioNotDecoded)
);

Comment 1

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6eabde9e938

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

What happens if one of the chained promises rejects? Does the rest of the chain still get called like JS promises?

Comment 3

[Bobby Holley (:bholley)]

(In reply to Chris Pearce (:cpearce) from comment #2)
> What happens if one of the chained promises rejects? Does the rest of the
> chain still get called like JS promises?

Yep. If the resolve or reject callback returns a promise, that promise is chained to the completion promise, so that its result gets propagated through. If the callback doesn't return a promise, the completion promise is immediately resolved or reject with the ResolveOrRejectValue. This allows the use of ::Pass in comment 0 - rejections just propagate through the intermediate processing step.

I'll write a comment explaining how this all works.

Comment 4

[Jean-Yves Avenard [:jya]]

Can this replace the use of the ProxyMediaCall?

Comment 5

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> Can this replace the use of the ProxyMediaCall?

Hm, how? The point of ProxyMediaCall is to dispatch a task to another thread - I'm not sure how completion promises would help with that.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 1 - Fix a typo from the addition of closure support to MediaPromise. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 2 - Improve MediaPromise::ResolveOrRejectValue. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 3 - Get rid of RefableThen and make Then return an nsRefPtr<Consumer>. v1

This incurs an extra addref here and there, but I think it makes the API simpler
to use and understand.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 4 - Fix up InvokeCallbackMethod to templatize more generally on the method type. v1

The hard-coded specificity here is a barrier towards various bits of
generalization in the ensuing patches.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 5 - Make FunctionThenValue dispatch via InvokeCallbackMethod. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 6 - Do the template munging to allow promise callbacks to optionally return a promise, and surface that from InvokeCallbackMethod. v1

I'm open to suggestions on how to do this more elegantly.

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 7 - Implement completion promises. v1

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 8 - Replace 'Consumer' with 'Request' in MediaPromise naming. v1

I think this makes more sense, and it matches the naming convention that all
of the consumers of this stuff are actually using.

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 9 - Add some more comments for MediaPromise. v1

Suggestions of other topics to describe are welcome.

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 10 - MediaPromise gtests. v1

Comment 16

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8611568 [details] [diff] [review]
Part 6 - Do the template munging to allow promise callbacks to optionally return a promise, and surface that from InvokeCallbackMethod. v1

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

Drive-by micro comment (didn't review the full file)

::: dom/media/MediaPromise.h
@@ +58,5 @@
> +};
> +
> +/*
> + * Work around the fact that IsConvertible doesn't support void. It would be nice
> + * to fix this in TypeTraits.h but we're not supposed to touch it. :-(.

stl::is_convertible supports void, so mozilla::IsConvertible should probably be fixed.
iso cpp 20.9.6: "template <class From, class To> struct is_convertible; -- From and To shall be complete types, arrays of unknown bound, or (possibly cv-qualified) void types."

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8611565 [details] [diff] [review]
Part 3 - Get rid of RefableThen and make Then return an nsRefPtr<Consumer>. v1

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

::: dom/media/MediaPromise.h
@@ +392,5 @@
>      nsRefPtr<ThenValueBase> thenValue = new FunctionThenValue<ResolveFunction, RejectFunction>(aResponseThread,
>                                                Move(aResolveFunction), Move(aRejectFunction), aCallSite);
>      ThenInternal(aResponseThread, thenValue, aCallSite);
> +    nsRefPtr<Consumer> c = thenValue.forget();
> +    return c;

Can't we just return thenValue.forget() for already_AddRefed is implicitly convertible to nsRefPtr?

Comment 18

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8611567 [details] [diff] [review]
Part 5 - Make FunctionThenValue dispatch via InvokeCallbackMethod. v1

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

What is the benefit of using InvokeCallbackMethod instead of direct calls to mResolveFunction/mRejectFunction?

Comment 19

[Bobby Holley (:bholley)]

(In reply to JW Wang [:jwwang] from comment #18)
> What is the benefit of using InvokeCallbackMethod instead of direct calls to
> mResolveFunction/mRejectFunction?

It lets us share all the complicated templated invoke overloads in the next method.

Comment 20

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #19)
> It lets us share all the complicated templated invoke overloads in the next
> method.

*next patch.

Comment 21

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8611568 [details] [diff] [review]
Part 6 - Do the template munging to allow promise callbacks to optionally return a promise, and surface that from InvokeCallbackMethod. v1

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

Looks fine to me. However, I am not a c++ guru. MFBT guys should have better suggestions.

::: dom/media/MediaPromise.h
@@ +303,5 @@
>     */
>  
>    template<typename ThisType, typename MethodType, typename ValueType>
> +  static typename EnableIf<ReturnTypeIs<MethodType, nsRefPtr<MediaPromise>>::value,
> +         typename EnableIf<TakesArgument<MethodType>::value, already_AddRefed<MediaPromise>>::Type>::Type

Can we just say:

EnableIf<
  ReturnTypeIs<MethodType, nsRefPtr<MediaPromise>>::value && TakesArgument<MethodType>::value,
  already_AddRefed<MediaPromise>
>::Type

to have one EnableIf only?

Comment 22

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8611564 [details] [diff] [review]
Part 2 - Improve MediaPromise::ResolveOrRejectValue. v1

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

::: dom/media/MediaPromise.h
@@ +72,5 @@
>        MOZ_ASSERT(IsNothing());
>        mRejectValue.emplace(aRejectValue);
>      }
>  
> +    static ResolveOrRejectValue MakeResolve(const ResolveValueType aResolveValue)

Where is this function used?

Comment 23

[Bobby Holley (:bholley)]

(In reply to JW Wang [:jwwang] from comment #22)
> Where is this function used?

In the tests.

Comment 24

[Bobby Holley (:bholley)]

(In reply to JW Wang [:jwwang] from comment #17)
> Can't we just return thenValue.forget() for already_AddRefed is implicitly
> convertible to nsRefPtr?

Hm, that's slightly more confusing, but definitely more compact. I'll switch to it.

Thanks for the reviews!

Comment 25

[Bobby Holley (:bholley)]

Attachment: Part 5.5 - Make IsConvertible handle void. v1

Comment 26

[Bobby Holley (:bholley)]

Attachment: Part 6 - Do the template munging to allow promise callbacks to optionally return a promise, and surface that from InvokeCallbackMethod. v3

Comment 27

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20c5c018a377

Comment 28

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8612094 [details] [diff] [review]
Part 5.5 - Make IsConvertible handle void. v1

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

r+ as it works and it's probably all you need for your use, which I don't want to delay.

If possible I would suggest the following small changes.
And/or just file a separate bug to get IsConvertible fully STL-compliant.

Also some unit tests, e.g.:
  static_assert(IsConvertible<void, void>::value, "void is void");
  static_assert(!IsConvertible<A, void>::value, "A shouldn't convert to void");
  static_assert(!IsConvertible<void, B>::value, "void shouldn't convert to B");

::: mfbt/TypeTraits.h
@@ +655,5 @@
> +
> +template<>
> +struct IsConvertible<void, void> {
> +    static const bool value = true;
> +};

These do not have a 'Type' member type. You may just follow the same style as the initial template to get all that's needed, e.g.:
"""
template<typename B>
struct IsConvertible<void, B>
  : IntegralConstant<bool, IsVoid<B>::value>
{};
template<typename A>
struct IsConvertible<A, void>
  : IntegralConstant<bool, IsVoid<A>::value>
{};
template<>
struct IsConvertible<void, void>
  : TrueType
{};
"""

Also, these won't handle const/volatile void properly, probably rare but it should probably be documented above.

Comment 29

[Bobby Holley (:bholley)]

Attachment: Part 7.5 - Use a base class for refcounting MediaPromise::Consumer as well. v1

It looks like the subsequent rename to "Request" causes trouble with the
leak log tables.

Comment 30

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a819c7b8df5c

Comment 31

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e8614a5c9c9

Comment 32

[Jean-Yves Avenard [:jya]]

Comment on attachment 8612430 [details] [diff] [review]
Part 7.5 - Use a base class for refcounting MediaPromise::Consumer as well. v1

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

No fan of the name MediaPromiseRefcountable...

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/831c5d95d197
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b6b0af6e032
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4f5c50385af
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6ad9ecaaefc
https://hg.mozilla.org/integration/mozilla-inbound/rev/743cd6e023c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/be83609966a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec08b62b49f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3d613bbf185
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bea013b498a
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb72777f68d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0593eed31d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/d29db7f0d3f0

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/831c5d95d197
https://hg.mozilla.org/mozilla-central/rev/4b6b0af6e032
https://hg.mozilla.org/mozilla-central/rev/b4f5c50385af
https://hg.mozilla.org/mozilla-central/rev/e6ad9ecaaefc
https://hg.mozilla.org/mozilla-central/rev/743cd6e023c5
https://hg.mozilla.org/mozilla-central/rev/be83609966a1
https://hg.mozilla.org/mozilla-central/rev/ec08b62b49f4
https://hg.mozilla.org/mozilla-central/rev/e3d613bbf185
https://hg.mozilla.org/mozilla-central/rev/7bea013b498a
https://hg.mozilla.org/mozilla-central/rev/eb72777f68d3
https://hg.mozilla.org/mozilla-central/rev/a0593eed31d4
https://hg.mozilla.org/mozilla-central/rev/d29db7f0d3f0