Implement Completion MediaPromises

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks: 1 bug)

unspecified
mozilla41
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(12 attachments, 1 obsolete attachment)

1.06 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
1.72 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
34.30 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
3.25 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
1.57 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
8.15 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
23.95 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
3.23 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
6.29 KB, patch
jwwang
: review+
Details | Diff | Splinter Review
1.06 KB, patch
gerald
: review+
Details | Diff | Splinter Review
7.02 KB, patch
gerald
: review+
Details | Diff | Splinter Review
2.30 KB, patch
jya
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
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)
);
What happens if one of the chained promises rejects? Does the rest of the chain still get called like JS promises?
(Assignee)

Comment 3

4 years ago
(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.
Can this replace the use of the ProxyMediaCall?
(Assignee)

Comment 5

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

Comment 6

4 years ago
Created attachment 8611563 [details] [diff] [review]
Part 1 - Fix a typo from the addition of closure support to MediaPromise. v1
Attachment #8611563 - Flags: review?(jwwang)
(Assignee)

Comment 7

4 years ago
Created attachment 8611564 [details] [diff] [review]
Part 2 - Improve MediaPromise::ResolveOrRejectValue. v1
Attachment #8611564 - Flags: review?(jwwang)
(Assignee)

Comment 8

4 years ago
Created attachment 8611565 [details] [diff] [review]
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.
Attachment #8611565 - Flags: review?(jwwang)
(Assignee)

Comment 9

4 years ago
Created attachment 8611566 [details] [diff] [review]
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.
Attachment #8611566 - Flags: review?(jwwang)
(Assignee)

Comment 10

4 years ago
Created attachment 8611567 [details] [diff] [review]
Part 5 - Make FunctionThenValue dispatch via InvokeCallbackMethod. v1
Attachment #8611567 - Flags: review?(jwwang)
(Assignee)

Comment 11

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

I'm open to suggestions on how to do this more elegantly.
Attachment #8611568 - Flags: review?(jwwang)
(Assignee)

Comment 12

4 years ago
Created attachment 8611569 [details] [diff] [review]
Part 7 - Implement completion promises. v1
Attachment #8611569 - Flags: review?(jwwang)
(Assignee)

Comment 13

4 years ago
Created attachment 8611570 [details] [diff] [review]
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.
Attachment #8611570 - Flags: review?(jwwang)
(Assignee)

Comment 14

4 years ago
Created attachment 8611571 [details] [diff] [review]
Part 9 - Add some more comments for MediaPromise. v1

Suggestions of other topics to describe are welcome.
Attachment #8611571 - Flags: review?(jwwang)
(Assignee)

Comment 15

4 years ago
Created attachment 8611572 [details] [diff] [review]
Part 10 - MediaPromise gtests. v1
Attachment #8611572 - Flags: review?(jwwang)
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."
Attachment #8611563 - Flags: review?(jwwang) → review+
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?
Attachment #8611565 - Flags: review?(jwwang) → review+
Attachment #8611566 - Flags: review?(jwwang) → review+
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?
Attachment #8611567 - Flags: review?(jwwang) → review+
(Assignee)

Comment 19

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

Comment 20

4 years ago
(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 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?
Attachment #8611568 - Flags: review?(jwwang) → feedback+
Attachment #8611569 - Flags: review?(jwwang) → review+
Attachment #8611570 - Flags: review?(jwwang) → review+
Attachment #8611571 - Flags: review?(jwwang) → review+
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?
Attachment #8611564 - Flags: review?(jwwang) → review+
(Assignee)

Comment 23

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

In the tests.
Attachment #8611572 - Flags: review?(jwwang) → review+
(Assignee)

Comment 24

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

Comment 25

4 years ago
Created attachment 8612094 [details] [diff] [review]
Part 5.5 - Make IsConvertible handle void. v1
Attachment #8612094 - Flags: review?(gsquelart)
(Assignee)

Comment 26

4 years ago
Created attachment 8612095 [details] [diff] [review]
Part 6 - Do the template munging to allow promise callbacks to optionally return a promise, and surface that from InvokeCallbackMethod. v3
Attachment #8611568 - Attachment is obsolete: true
Attachment #8612095 - Flags: review?(gsquelart)
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.
Attachment #8612094 - Flags: review?(gsquelart) → review+
Attachment #8612095 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 29

4 years ago
Created attachment 8612430 [details] [diff] [review]
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.
Attachment #8612430 - Flags: review?(jyavenard)
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...
Attachment #8612430 - Flags: review?(jyavenard) → review+
You need to log in before you can comment on or make changes to this bug.