1169542 - Implement MediaPromise::All

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

More skunkworks.

I've got an plan to use this to reimplement MediaSourceReader::ReadMetadata in terms of MediaPromise::All and completion promises.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Stop copying ResolveOrRejectValues. v1

This moves us closer to being able to use nsTArray, which is
non-implicitly-copyable, as a Resolve/Reject value.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Make MediaPromise more rvalue-friendly. v1

Again, necessary for passing around nsTArrays.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Implement MediaPromise::All. v1

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

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

Comment on attachment 8612725 [details] [diff] [review]
Part 2 - Make MediaPromise more rvalue-friendly. v1

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

r+ for me, as it looks correct.
With a couple of suggestions below, see if they make sense (especially in light of my next paragraph).

One small worry is that by introducing these forwarding references, types in method signatures are dissociated from the enclosing class template types.
E.g.:
The old |void SetResolve(const ResolveValueType&)| was expecting the same 'ResolveValueType' as specified by the template instantiation; easy for the compiler to detect mismatches and errors would hopefully be obvious to the developer.
But now it has become |template<typename ResolveValueType_> void SetResolve(ResolveValueType_&&)| where ResolveValueType_ could be anything!
I think bugs will still be caught somewhere in mResolveValue.emplace(), where the function argument is finally assigned to a 'ResolveValueType' variable. However, the error will probably appear deep in Maybe-land, which could obscure the problem a bit, and delay our brilliant developer by many seconds.
(Note that I haven't actually tested this theory, maybe I'm totally wrong!)
So if you can afford the time, it'd be good to give it a little try with the wrong types, see that at least errors are caught; and assess whether the error is effectively obscured by the forwarding reference. (In which case you could instead painfully write two versions of each method, one with const T&, the other one with non-forwarding T&&).

What do you think? I'd be happy to test my theory during the weekend if you'd like me to.

::: dom/media/MediaPromise.h
@@ +143,3 @@
>      {
>        ResolveOrRejectValue val;
>        val.SetResolve(aResolveValue);

Why not use a forwarding reference here? It would suit your improved SetResolve quite nicely. (I don't see where&how MakeResolve is used, so maybe it's not appropriate)

@@ +150,3 @@
>      {
>        ResolveOrRejectValue val;
>        val.SetReject(aRejectValue);

Same as above, use forwarding reference unless it doesn't make sense.

Comment 6

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

(In reply to Gerald Squelart [:gerald] from comment #5)
> Comment on attachment 8612725 [details] [diff] [review]
> Part 2 - Make MediaPromise more rvalue-friendly. v1
> 
> Review of attachment 8612725 [details] [diff] [review]:

jya pointed out that there are some |return Move(...)|'s in the above code.
Move is not necessary when returning an object (the compiler already knows it won't be used anymore in this function), and it could even prevent the compiler from applying RVO. Please lose the Moves!

Comment 7

[Bobby Holley (:bholley)]

(In reply to Gerald Squelart [:gerald] from comment #5)
> So if you can afford the time, it'd be good to give it a little try with the
> wrong types, see that at least errors are caught; and assess whether the
> error is effectively obscured by the forwarding reference. (In which case
> you could instead painfully write two versions of each method, one with
> const T&, the other one with non-forwarding T&&).
> 
> What do you think? I'd be happy to test my theory during the weekend if
> you'd like me to.

This is the error I get when passing a cstring to a resolve method that expects an int: https://pastebin.mozilla.org/8835157 , which seems clear enough to me (but maybe I've been spending too much time in this code). I'd really rather not duplicate everything.

> ::: dom/media/MediaPromise.h
> Why not use a forwarding reference here? It would suit your improved
> SetResolve quite nicely. (I don't see where&how MakeResolve is used, so
> maybe it's not appropriate)

I did so originally, and then decided that it wasn't necessary since this is a very infrequently used codepath (currently used only for tests). But I guess it's actually more confusing for it to be different than the methods above it, so I'll change it.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Gerald Squelart [:gerald] from comment #6)
> jya pointed out that there are some |return Move(...)|'s in the above code.
> Move is not necessary when returning an object (the compiler already knows
> it won't be used anymore in this function), and it could even prevent the
> compiler from applying RVO. Please lose the Moves!

Those exist because nsRefPtr<Derived> is not implicitly convertible to nsRefPtr<Base>. But jww recently thought of a cuter trick, which is to return .forget() (since already_AddRefed<Derived> _is_ implicitly convertible to nsRefPtr<Base>). I'll add a patch to do that.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 4 (Bonus) - Use .forget() instead of Move() to convert from nsRefPtr<Derived> to nsRefPtr<Base>. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 2 - Make MediaPromise more rvalue-friendly. v2 r=gerald

Comment 11

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

(In reply to Bobby Holley (:bholley) from comment #7)
> (In reply to Gerald Squelart [:gerald] from comment #5)
> > So if you can afford the time, it'd be good to give it a little try with the
> > wrong types, see that at least errors are caught; and assess whether the
> > error is effectively obscured by the forwarding reference.
> 
> This is the error I get when passing a cstring to a resolve method that
> expects an int: https://pastebin.mozilla.org/8835157 , which seems clear
> enough to me (but maybe I've been spending too much time in this code). I'd
> really rather not duplicate everything.

Thanks for checking. Agreed, it's clear enough, so it's all good.

> > ::: dom/media/MediaPromise.h
> > Why not use a forwarding reference here?
> 
> I did so originally, and then decided that it wasn't necessary since this is
> a very infrequently used codepath (currently used only for tests). But I
> guess it's actually more confusing for it to be different than the methods
> above it, so I'll change it.

Thanks.

Comment 12

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

Comment on attachment 8612724 [details] [diff] [review]
Part 1 - Stop copying ResolveOrRejectValues. v1

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

::: dom/media/MediaPromise.h
@@ +409,5 @@
>      mThisVal = nullptr;
>    }
>  
>    protected:
> +    virtual already_AddRefed<MediaPromise> DoResolveOrRejectInternal(const ResolveOrRejectValue& aValue) override

There is a discussion about omitting 'virtual' when 'override' is already there.

Comment 13

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

Comment on attachment 8613028 [details] [diff] [review]
Part 2 - Make MediaPromise more rvalue-friendly. v2 r=gerald

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

::: dom/media/MediaPromise.h
@@ +378,5 @@
>    template<typename ThisType, typename MethodType, typename ValueType>
>    static typename EnableIf<ReturnTypeIs<MethodType, nsRefPtr<MediaPromise>>::value &&
>                             !TakesArgument<MethodType>::value,
>                             already_AddRefed<MediaPromise>>::Type
> +  InvokeCallbackMethod(ThisType* aThisVal, MethodType aMethod, const ValueType& aValue)

Why not taking ValueType&& so it is consistent with the code above?

Comment 14

[Bobby Holley (:bholley)]

(In reply to JW Wang [:jwwang] from comment #12)
> There is a discussion about omitting 'virtual' when 'override' is already
> there.

Yeah, orthogonal to this patch though.

(In reply to JW Wang [:jwwang] from comment #13)
> Why not taking ValueType&& so it is consistent with the code above?

I remember the compiler having trouble with it (since the argument is unused), but I just tested again and now it seems fine. Good catch!

Comment 15

[Bobby Holley (:bholley)]

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

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe733c938922
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbe9941afa55
https://hg.mozilla.org/integration/mozilla-inbound/rev/84b169ac3bcc
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5754fa7eeb5

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fe733c938922
https://hg.mozilla.org/mozilla-central/rev/bbe9941afa55
https://hg.mozilla.org/mozilla-central/rev/84b169ac3bcc
https://hg.mozilla.org/mozilla-central/rev/a5754fa7eeb5

Comment 18

[Jean-Yves Avenard [:jya]]

Could we have a way to find out which promise caused the reject (like the index in the nsTArray)?

Comment 19

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #18)
> Could we have a way to find out which promise caused the reject (like the
> index in the nsTArray)?

I don't think that's wise to expose - given that All() rejects when the first sub-promise rejects, you really can't infer anything about the success or failure of any of the other promises (they may not even have completed yet) - the only sane thing to do is to abort the entire transaction.

This is the interface that JS promises provide, and I'm guessing that the reasoning is similar.