1126465 - MediaSourceReader seek can get scuttled when it mid-airs with the tail-end of an outstanding Request{Audio,Video}Data

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P1)

Description

[Bobby Holley (:bholley)]

This is the source (or a source) of the timeouts I've been investigating in bug 1126052.

The basic problem is that MediaSourceReader doesn't wait for any previous sample requests it has made to be resolved before it starts trying to seek. So if one of those requests get resolved at the right moment during seek, we overwrite mLast{Audio,Video}Time to a time in the old region we've seek away from. This means that the MDSM will just keep requesting and dropping samples in Drop{Audio,Video}UpToSeekTarget, and we'll get stuck.

MediaSourceReader::On{Audio,Video}Decoded _try_ to deal with this problem by checking for {m{Audio,Video}IsSeeking before updating mLast{Audio,Video}Time, but these values don't actually remain true for the full duration that the old sample request might be resolved. And indeed, I'm not sure we can really put a latest bound on when that might happen.

The two decent solutions I can think of are:
(A) Waiting for any outstanding requests to be resolved before initiating the seek, or
(B) Implementing some sort of promise cancellation (probably only for exclusive promises).

I'll investigate solutions here after lunch.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 1 - Stop invoking On*NotDecoded when we didn't actually go through the promise. v1

This isn't right, and it means that we can't assume at the top of
On{Audio,Video}{,Not}Decoded() that we're fresh out of promise dispatch, which
we want to do.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 2 - Don't leave stale value in mPendingSeek when there's no audio track. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 3 - Make ThenValueBase inherit from a publicly-usable type, and refcount it. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 4 - Introduce machinery to hold onto MediaPromise::Consumer references, and use it for MediaSourceReader subdecoders. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 5 - Implement the ability to disconnect outstanding promises. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 6 - Cancel sample requests when seeks start, disallow them while seeks are happening, and assert against seeks when samples arrive. v1

The duplication of the IsSeeking() checks before all the Request{Audio,Video}Data
callsites is ugly. We'll fix this in the next patch by applying the same disconnect
treatment to the seek promise.

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 7 - Pass consistent arguments to RequestVideoData invocations on subdecoders. v1

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 8 - Factor out calls to Request{Audio,Video}Data into a helper method. v1

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 9 - Factor out the seeking phases into helper methods. v1

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 10 - Use a MediaPromiseConsumerHolder to track subdecoder seeks. v1

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8556153 [details] [diff] [review]
Part 10 - Use a MediaPromiseConsumerHolder to track subdecoder seeks. v1

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

::: dom/media/mediasource/MediaSourceReader.cpp
@@ +694,5 @@
>    ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> +  mWaitingForSeekData = false;
> +  mPendingSeekTime = -1;
> +  mSeekRequest.DisconnectIfExists();
> +  mSeekPromise.RejectIfExists(NS_OK, __func__);

Not calling CancelSeek on the sub-decoders seems like it could potentially break things in the future.

With MSR we had the issue where the seek promise might never be resolved without an explicit cancel, if any of the other readers ever add this behaviour then this would break.

Should at least add a comment about this.

::: dom/media/mediasource/MediaSourceReader.h
@@ +175,5 @@
> +    DoVideoRequest();
> +  }
> +
> +  void RejectAudioPromiseWithDecodeError() { mAudioPromise.Reject(DECODE_ERROR, __func__); }
> +  void RejectVideoPromiseWithDecodeError() { mVideoPromise.Reject(DECODE_ERROR, __func__); }

Don't we need to call Complete on mSeekRequest for these too?

Comment 16

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8556144 [details] [diff] [review]
Part 4 - Introduce machinery to hold onto MediaPromise::Consumer references, and use it for MediaSourceReader subdecoders. v1

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

::: dom/media/MediaPromise.h
@@ +252,5 @@
>  
>    template<typename TargetType, typename ThisType,
>             typename ResolveMethodType, typename RejectMethodType>
> +  already_AddRefed<Consumer> RefableThen(TargetType* aResponseTarget, const char* aCallSite, ThisType* aThisVal,
> +                                         ResolveMethodType aResolveMethod, RejectMethodType aRejectMethod)

Can we instead just have a single version of Then that returns an nsRefPtr (like we do for functioms that return a promise)?

That way callers that don't care about the Consumer can just ignore the return value and be fine.

It's not a huge deal, I'm just not a big fan of 'RefableThen'.

Comment 17

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8556145 [details] [diff] [review]
Part 5 - Implement the ability to disconnect outstanding promises. v1

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

::: dom/media/MediaPromise.h
@@ +90,5 @@
>    {
>    public:
>      NS_INLINE_DECL_THREADSAFE_REFCOUNTING(Consumer)
> +
> +    void Neuter()

Can we call this Disconnect (which I like more) and stay consistent with MediaPromiseConsumerHolder?

Comment 18

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8556149 [details] [diff] [review]
Part 6 - Cancel sample requests when seeks start, disallow them while seeks are happening, and assert against seeks when samples arrive. v1

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

::: dom/media/mediasource/MediaSourceReader.cpp
@@ +691,5 @@
> +  // Any previous requests we've been waiting on are now unwanted.
> +  mAudioRequest.DisconnectIfExists();
> +  mVideoRequest.DisconnectIfExists();
> +
> +  // Additionally, reject any outstanding promises _we_made that we might have

missing space

Comment 19

[Bobby Holley (:bholley)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #16)
> >    template<typename TargetType, typename ThisType,
> >             typename ResolveMethodType, typename RejectMethodType>
> > +  already_AddRefed<Consumer> RefableThen(TargetType* aResponseTarget, const char* aCallSite, ThisType* aThisVal,
> > +                                         ResolveMethodType aResolveMethod, RejectMethodType aRejectMethod)
> 
> Can we instead just have a single version of Then that returns an nsRefPtr
> (like we do for functioms that return a promise)?
> 
> That way callers that don't care about the Consumer can just ignore the
> return value and be fine.
> 
> It's not a huge deal, I'm just not a big fan of 'RefableThen'.

Yeah, that would be my preference in general. But I got a lot of armchair criticism with the whole "returning an nsRefPtr" thing, because a lot of Gecko hackers think it's evil and it's discouraged in docs like [1]. I figured that it wasn't really necessary here, so I might as well appease them people. Open to better names though. ;-)

[1] https://developer.mozilla.org/en-US/docs/Using_nsCOMPtr/Getting_Started_Guide(In reply to Matt Woodrow (:mattwoodrow) from comment #17)

> Comment on attachment 8556145 [details] [diff] [review]
> > +    void Neuter()
> 
> Can we call this Disconnect (which I like more) and stay consistent with
> MediaPromiseConsumerHolder?

Sure.

Comment 20

[Bobby Holley (:bholley)]

Attachment: Part 10 - Use a MediaPromiseConsumerHolder to track subdecoder seeks. v2

Good point on both counts.

Comment 21

[Matt Woodrow (:mattwoodrow)]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #19)
 
> Yeah, that would be my preference in general. But I got a lot of armchair
> criticism with the whole "returning an nsRefPtr" thing, because a lot of
> Gecko hackers think it's evil and it's discouraged in docs like [1]. I
> figured that it wasn't really necessary here, so I might as well appease
> them people. Open to better names though. ;-)
> 

Fair enough.

There was talk of making already_AddRefed<T> call Release() on the contained pointer during the dtor if it hadn't been assigned to something else before then, that would be sufficient for this use case.

Comment 22

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8556211 [details] [diff] [review]
Part 10 - Use a MediaPromiseConsumerHolder to track subdecoder seeks. v2

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

::: dom/media/mediasource/MediaSourceReader.h
@@ +175,5 @@
> +    DoVideoRequest();
> +  }
> +
> +  void CompleteSeekAndRejectAudioPromise() { mSeekRequest.Complete(); mAudioPromise.Reject(DECODE_ERROR, __func__); }
> +  void CompleteSeekAndRejectVideoPromise() { mSeekRequest.Complete(); mVideoPromise.Reject(DECODE_ERROR, __func__); }

Split these onto multiple lines.

Comment 23

[Bobby Holley (:bholley)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #21)
> There was talk of making already_AddRefed<T> call Release() on the contained
> pointer during the dtor if it hadn't been assigned to something else before
> then, that would be sufficient for this use case.

Yes, and the MediaPromise use-case. Though then it starts to smell a lot like nsRefPtr, and doesn't solve the footguns in comment 19.

(In reply to Matt Woodrow (:mattwoodrow) from comment #22) 
> Split these onto multiple lines.

Will do - thanks for the fast reviews!

Comment 24

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/408b00141b82
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/212c4e3377f8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/22aff1448376
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d8045d89bfda
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dd957ede2d36
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/47301816c705
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/96e70a10440c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ebe589cc0d92
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c96bac2df9a4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a0a572ab4614

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3f102935baa3 for B2G OSX build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=6047886&repo=mozilla-inbound

Comment 26

[Bobby Holley (:bholley)]

(In reply to Wes Kocher (:KWierso) from comment #25)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3f102935baa3 for B2G
> OSX build bustage:
> https://treeherder.mozilla.org/logviewer.html#?job_id=6047886&repo=mozilla-
> inbound

Gah, sorry for not noticing that - I had previous infra-related bustage on that platform, and my eyes just glazed over it. Indeed, the fact that it fails only on that configuration (and that the failure doesn't seem correct) makes me think this is a compiler bug.

Let's see if this fixes it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e66d5b914a50

Comment 27

[Phil Ringnalda (:philor)]

Be sure to look at the web-platform-tests-4 results on that, too, since you had multiple failures in it.

Comment 28

[Bobby Holley (:bholley)]

(In reply to Phil Ringnalda (:philor) from comment #27)
> Be sure to look at the web-platform-tests-4 results on that, too, since you
> had multiple failures in it.

Thanks for pointing that out. Looks like we need to track audio and video seek requests separately, since really we're only guaranteed that the MDSM will not issue two sample requests for the same media type, whereas it may indeed dispatch an audio request while an existing video request is doing its pre-request seek (or vice versa).

Comment 29

[Bobby Holley (:bholley)]

Attachment: Part 10.5 - Track audio and video seek requests separately. v1

I don't know why this wasn't obvious to me before.

Comment 30

[Bobby Holley (:bholley)]

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

Comment 31

[Bobby Holley (:bholley)]

The backout was caused by b2g osx debug build failures, which turned out to be a compile error that I've worked around. Green try run here:

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

and web-platform-tests-4 failures, which I fixed with a combination of Part 10.5 (attached in this bug), and bumping the timeout on one test:

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

Landed to m-i:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=b0e2b65a0cce

Comment 32

[Ryan VanderMeulen [:RyanVM]]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #31)
> and web-platform-tests-4 failures, which I fixed with a combination of Part
> 10.5 (attached in this bug), and bumping the timeout on one test:

Wasn't enough. We're still seeing frequent Windows timeouts in redundant-seek since your push.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Since nobody's around on IRC to look into the failures and we'd like to merge inbound some time in the near future, I've gone ahead and disabled the test.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b9ee0581d40

Comment 34

[Bobby Holley (:bholley)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #33)
> Since nobody's around on IRC to look into the failures and we'd like to
> merge inbound some time in the near future, I've gone ahead and disabled the
> test.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6b9ee0581d40

Thanks for doing that instead of backing out! I'll look into this in bug 1127920.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42b47d13f842
https://hg.mozilla.org/mozilla-central/rev/c3d0d1a59d83
https://hg.mozilla.org/mozilla-central/rev/0cbf60e4d0c1
https://hg.mozilla.org/mozilla-central/rev/b5f0920defbe
https://hg.mozilla.org/mozilla-central/rev/33588deed131
https://hg.mozilla.org/mozilla-central/rev/7e85ee706260
https://hg.mozilla.org/mozilla-central/rev/8b4f0dec6cdd
https://hg.mozilla.org/mozilla-central/rev/784042074702
https://hg.mozilla.org/mozilla-central/rev/2f9a8ce6ad9e
https://hg.mozilla.org/mozilla-central/rev/07ae1aee7121
https://hg.mozilla.org/mozilla-central/rev/b0e2b65a0cce
https://hg.mozilla.org/mozilla-central/rev/6b9ee0581d40

Comment 36

[Ralph Giles (:rillian)]

Comment on attachment 8556141 [details] [diff] [review]
Part 1 - Stop invoking On*NotDecoded when we didn't actually go through the promise. v1

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Youtube playback can fail after seeking.
[Describe test coverage new/current, TreeHerder]: landed on m-c.

[Risks and why]: This is quite a large set of changes. There are two components. Most of the seek changes are MSE-specific, so they're moderately risky for that feature, but since we can't ship without them, and not shipping means the code is disabled, the risk of regression outside the MSE code is low.

The second component involves adding new methods to the MediaPromise framework. That is used by non-MSE playback code, but Bobby described the risk of those additions as low as well.

[String/UUID change made/needed]: None

Comment 37

[Ralph Giles (:rillian)]

Request is for all patches on this bug.

Comment 38

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8556141 [details] [diff] [review]
Part 1 - Stop invoking On*NotDecoded when we didn't actually go through the promise. v1

I am approving all the patches on this bug. However, for the record, even if I am sure you know what you are doing, I would have hope for smaller patches that late in the cycle...

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/53dd616969b2
https://hg.mozilla.org/releases/mozilla-aurora/rev/e1bf08bf1b92
https://hg.mozilla.org/releases/mozilla-aurora/rev/a56baaff46e4
https://hg.mozilla.org/releases/mozilla-aurora/rev/167fe8649c3b
https://hg.mozilla.org/releases/mozilla-aurora/rev/2c6f2f3bc091
https://hg.mozilla.org/releases/mozilla-aurora/rev/fc2da6049ed0
https://hg.mozilla.org/releases/mozilla-aurora/rev/0ad7476ae24d
https://hg.mozilla.org/releases/mozilla-aurora/rev/09b6f3a2018f
https://hg.mozilla.org/releases/mozilla-aurora/rev/c036868bfc62
https://hg.mozilla.org/releases/mozilla-aurora/rev/1282656870a2
https://hg.mozilla.org/releases/mozilla-aurora/rev/7a947ff487be
https://hg.mozilla.org/releases/mozilla-aurora/rev/c90b2e01a9d8

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dbb452162854
https://hg.mozilla.org/releases/mozilla-beta/rev/ebd1573c5911
https://hg.mozilla.org/releases/mozilla-beta/rev/6e44bfd1e0f8
https://hg.mozilla.org/releases/mozilla-beta/rev/54d7f88c8b75
https://hg.mozilla.org/releases/mozilla-beta/rev/29c741d65b11
https://hg.mozilla.org/releases/mozilla-beta/rev/26df0dd2cceb
https://hg.mozilla.org/releases/mozilla-beta/rev/5daace5690d6
https://hg.mozilla.org/releases/mozilla-beta/rev/e76764c0e076
https://hg.mozilla.org/releases/mozilla-beta/rev/cc2a8374de88
https://hg.mozilla.org/releases/mozilla-beta/rev/169b4de5b199
https://hg.mozilla.org/releases/mozilla-beta/rev/361d53acbf3a

mediasource-redundant-seek.html is already completely disabled on beta, so the follow-up patch I landed was dropped for that branch.