1091008 - MediaDecoderStateMachine::HasLowUndecodedData doesn't work for MSE

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

This is the problem identified in bug 1063323 comment 3. I have patches to fix it.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Bobby Holley (:bholley)]

This one should do it, I think: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=33c41388ee21

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 1 - Discard mCurrentDecoder in TrackBuffer::Shutdown. v1

Otherwise, we end up with a subsequent call to DiscardDecoder() which crashes,
because mParentDecoder (and thus its monitor) is null.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 2 - Factor out IsDataCachedToEndOfResource() into a higher-level question and answer it sensibly for MSE. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 3 - Teach MediaDecoderReader about its start time. v2

Duplicating state is never great, but this lets the reader make calculations
using this immutable state variable without involving the state machine. We
could alternatively punch a hole from MediaDecoderReader to
MediaDecoderStateMachine and access it there, but that would create tighter
coupling, and weird relationships for MSE.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 4 - Remove the aStartTime argument from MediaDecoderReader::GetBuffered. v1

We now have this stashed on the superclass.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 5 - Hoist stream pinning into the MediaDecoderReaders and make MediaDecoderStateMachine::GetBuffered just forward to them. v1

Whether or not we ping a stream depends a lot on what kind of decoder we're
dealing with. In particular, it doesn't really make sense for MSE.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 6 - Implement a sensible GetBuffered override for MediaSourceReader using the existing GetBuffered on MediaSource. v1

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 7 - Remove special handling for MSE in HTMLMediaElement::Buffered. v1

The same code will now be reached by invoking this method on the decoder.

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 8 - Reimplement HasLowUndecodedData in terms of GetBuffered. v1

Since GetBuffered now has a sane implementation for MSE, this should
make this function sane for MSE as well.

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 8515151 [details] [diff] [review]
Part 1 - Discard mCurrentDecoder in TrackBuffer::Shutdown. v1

In bug 1082206 comment 7, I concluded that TrackBuffer::Shutdown() was too late to discard decoders?  Do you know how we get to here with mCurrentDecoder set?

Comment 14

[Bobby Holley (:bholley)]

(In reply to Karl Tomlinson (:karlt) from comment #13)
> In bug 1082206 comment 7, I concluded that TrackBuffer::Shutdown() was too
> late to discard decoders?  Do you know how we get to here with
> mCurrentDecoder set?

It was crashing when I wrote the patch, but I can't seem to get it to crash anymore. I've done a try push with that part reverted - if it's green, I'm totally happy to ditch that piece:

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

Comment 15

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

Comment on attachment 8515155 [details] [diff] [review]
Part 5 - Hoist stream pinning into the MediaDecoderReaders and make MediaDecoderStateMachine::GetBuffered just forward to them. v1

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

Nice.

Comment 16

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

Comment on attachment 8515164 [details] [diff] [review]
Part 8 - Reimplement HasLowUndecodedData in terms of GetBuffered. v1

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

Almost...

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1875,5 @@
>  
> +  MOZ_ASSERT(HasVideo() || HasAudio());
> +  int64_t endOfDecodedVideoData = INT64_MAX;
> +  if (HasVideo()) {
> +    endOfDecodedVideoData = VideoQueue().Peek() ? VideoQueue().Peek()->GetEndTime() : mCurrentFrameTime;

Maybe you want mVideoFrameEndTime instead of mCurrentFrameTime here?

@@ +1884,2 @@
>    }
> +  int64_t endOfDecodedData = std::min(endOfDecodedVideoData, endOfDecodedAudioData);

I think you only want the min of streams that have not reached their end (AtEndOfStream() returns true).

For example if there's 10s more audio than video (which can happen), once playback reaches the end of the audio stream we'll always assume we're low on undecoded data with your code here, right? More commonly there'd be 100ms or so more of 1 stream.

One solution would be to go:

  if (HasVideo() && !VideoQueue().AtEndOfStream()) { endOfDecodedVideoData = ...}

Then you need to handle both endOfDecoded*Data being INT64_MAX of course.

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 8515151 [details] [diff] [review]
Part 1 - Discard mCurrentDecoder in TrackBuffer::Shutdown. v1

(In reply to Bobby Holley (:bholley) from comment #14)
> It was crashing when I wrote the patch, but I can't seem to get it to crash
> anymore. I've done a try push with that part reverted - if it's green, I'm
> totally happy to ditch that piece:
> 
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=df090515d167

This is green. Removing that patch from the series.

Comment 18

[Bobby Holley (:bholley)]

Attachment: Part 8 - Reimplement HasLowUndecodedData in terms of GetBuffered. v2

Thanks for all the super-fast reviews (here and elsewhere) - especially on a
weekend :-)

Comment 19

[cajbir (:cajbir)]

Comment on attachment 8515158 [details] [diff] [review]
Part 6 - Implement a sensible GetBuffered override for MediaSourceReader using the existing GetBuffered on MediaSource. v1

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

With the patches applied in this bug there is a substantial delay (30 seconds or so) before starting playback in the YouTube demo player: 

http://cd.pn/mse/ytdemo/dash-player.html?url=http://cd.pn/mse/ytdemo/feelings2.mpd
http://cd.pn/mse/ytdemo/dash-player.html?url=http://cd.pn/mse/ytdemo/feelings_vp9-20130806-manifest.mpd

Playback starts almost immediately without these patches.

r+ with the startup delay in the demo player investigated and a bug spawned for it if it's an issue.

Comment 20

[Karl Tomlinson (:karlt)]

Comment on attachment 8515155 [details] [diff] [review]
Part 5 - Hoist stream pinning into the MediaDecoderReaders and make MediaDecoderStateMachine::GetBuffered just forward to them. v1

>+ * RAII class that handles pinning and unpinning for MediaStream and derived.
>+ * This should be used when making calculations that involve potentially-cached
>+ * MediaStream data, so that the state of the world can't change out from under

s/MediaStream/MediaResource/ in both places.

Comment 21

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

Comment on attachment 8515154 [details] [diff] [review]
Part 4 - Remove the aStartTime argument from MediaDecoderReader::GetBuffered. v1

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1503,2 @@
>    nsRefPtr<dom::TimeRanges> buffered = new dom::TimeRanges();
> +  if (mDecoder->IsInfinite() && (mStartTime != -1) &&

Since you want to reduce the coupling, it might be good to let MediaDecoderReader::GetBuffered figure out when to return a failure without checking |mStartTime| here.

Comment 22

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

Comment on attachment 8515155 [details] [diff] [review]
Part 5 - Hoist stream pinning into the MediaDecoderReaders and make MediaDecoderStateMachine::GetBuffered just forward to them. v1

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

::: dom/media/MediaResource.h
@@ +723,5 @@
> + * This should be used when making calculations that involve potentially-cached
> + * MediaStream data, so that the state of the world can't change out from under
> + * us.
> + */
> +template<class T>

Will T be something other than MediaResource? It seems the class doesn't need to be templated.

Comment 23

[Bobby Holley (:bholley)]

(In reply to JW Wang [:jwwang] from comment #21)
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +1503,2 @@
> >    nsRefPtr<dom::TimeRanges> buffered = new dom::TimeRanges();
> > +  if (mDecoder->IsInfinite() && (mStartTime != -1) &&
> 
> Since you want to reduce the coupling, it might be good to let
> MediaDecoderReader::GetBuffered figure out when to return a failure without
> checking |mStartTime| here.

We could very easily handle that case, which would basically trade tighter invariants (i.e. MOZ_ASSERTs which we'll notice) for robustness (quietly handling the error cases). I opted for tighter invariants, especially since the handling would have to be duplicated over a lot of readers. I could be convinced to the contrary though.

(In reply to JW Wang [:jwwang] from comment #22)
> Will T be something other than MediaResource? It seems the class doesn't
> need to be templated.

There are a variety of MediaResource subclasses that people may want to pin while still using the guard-object-as-auto-pointer functionality for derived methods, which is why I templated it.

Comment 24

[Bobby Holley (:bholley)]

Attachment: Part 1 - Fix bogus HasLowUndecodedData(mBufferingWait * USECS_PER_S) call and remove overload. v1

Thanks for noticing that playback issue cajbir. I'd actually noticed the
corresponding problem in the code, but was planning on doing it as a followup.
Given that this may negatively impact MSE UX on nightly, I'd like to land
these together.

Flagging jwwang for review, in case he checks his bugmail before bed. :-)

The units on this call don't make sense - mBufferingWait is supposed to be a
measure of how long we buffer, not how much data we want to buffer. And since
that member is generally 30 seconds, we're waiting for an enormous amount of
data here.

Comment 25

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #24)
> Created attachment 8515909 [details] [diff] [review]
> Part 1 - Fix bogus HasLowUndecodedData(mBufferingWait * USECS_PER_S) call
> and remove overload. v1
> 
> Thanks for noticing that playback issue cajbir. I'd actually noticed the
> corresponding problem in the code, but was planning on doing it as a
> followup.
> Given that this may negatively impact MSE UX on nightly, I'd like to land
> these together.

See also bug 1093020, which may further improve our robustness here.

Comment 26

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8bcb9dba25d9

Comment 27

[Bobby Holley (:bholley)]

Attachment: Part 1 - Fix bogus HasLowUndecodedData(mBufferingWait * USECS_PER_S) call and remove overload. v2

Rebased this so that it actually fits as part 1 in the series.

Comment 28

[Bobby Holley (:bholley)]

Comment on attachment 8515916 [details] [diff] [review]
Part 1 - Fix bogus HasLowUndecodedData(mBufferingWait * USECS_PER_S) call and remove overload. v2

jwwang has presumably gone to bed, so switching this over to cpearce.

Comment 29

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

Comment on attachment 8515916 [details] [diff] [review]
Part 1 - Fix bogus HasLowUndecodedData(mBufferingWait * USECS_PER_S) call and remove overload. v2

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

This is actually the behaviour that we want, with non-MSE video at least.

mLowDataThresholdUsecs's defaults to 5 seconds. We normally should only hit the buffering case when the network isn't fast enough to keep up with playback. Rarely we'd expect a high-entropy segment of video to cause playback to fall behind the download and it could trigger buffering mode, but in general, when we're hitting buffering, we're pretty much screwed anyway.

If we came out of buffering after having downloaded only 5s of data, we will likely start up again soon and then run out of data very soon and revert back to buffering again, which would be a bad user experience.

(Note: I'm not arguing the code as it stands is clear and well written, it's obviously not, I just don't think the behaviour change here is warranted).

Comment 30

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8aaa10a8d956
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/856016c0118a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9de4746aa59a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/966093bbc26a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6f270b2d90f4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/649bfc6dad4d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/16f58d7e1e17

Comment 31

[Carsten Book [:Tomcat]]

Hi Bobby,

sorry had to back this out in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=43a51201545a since this push caused windows m2 permanent test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3612062&repo=mozilla-inbound

Comment 32

[Bobby Holley (:bholley)]

So the MSE stuff is racey here. I've compared TBPL NSPR logs between a good and bad run, and here's what's happening:

(1) We append data to the source buffer. It gets piped down into the track buffer, which schedules the state machine thread. It also queues up decoder initialization to happen asynchronously on the decode thread.
(2A) The state machine thread wakes up, and requests data from the reader.
(2B) The decoder gets initialized and added to mInitializedDecoders.
(3) The reader tries to process the request for video data. If (2A) happens faster than (2B), it doesn't find anything in mInitializedDecoders with the appropriate range, and fires OnNotDecoded(WAITING_FOR_DATA), which causes the state machine to hang.

One easy fix here is to put the state machine into buffering mode when we receiving OnNotDecoded(WAITING_FOR_DATA) - this will ensure that it tries again, and generally feels like the correct thing to do.

We could also have the reader do additional checks to see if there are any not-yet-initialized decoders, and wait to respond to the state machine until that's sorted out. That generally feels a lot less robust, though.

I'll attach a patch.

Comment 33

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #32)
> I'll attach a patch.

I split this out into bug 1096597.

Comment 34

[Bobby Holley (:bholley)]

Attachment: Part 3a - Make sure to always set mStartTime via SetStartTime. v1

If we don't do this, we'll get out of sync with the reader.

Comment 35

[Bobby Holley (:bholley)]

Attachment: Part 3b - Teach MediaDecoderReader about its start time. v3

A few tweaks to the last version of this patch, mostly around ensuring that
we have a start time before invoking MediaReader::GetBuffered (which will
assert in that case).

Comment 36

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

Comment on attachment 8521023 [details] [diff] [review]
Part 3b - Teach MediaDecoderReader about its start time. v3

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +3133,5 @@
> +  // It's possible for JS to query .buffered before we've determined the start
> +  // time from metadata, in which case the reader isn't ready to be asked this
> +  // question.
> +  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> +  NS_ENSURE_TRUE(mStartTime >= 0, NS_OK);

I don't think this should be an NS_ENSURE_TRUE, since that logs a warning on failure, and we should only really be logging when something exceptional happens, not when JS does something silly.

Just a simple if then return will suffice.

Comment 37

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

Comment on attachment 8521023 [details] [diff] [review]
Part 3b - Teach MediaDecoderReader about its start time. v3

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +3133,5 @@
> +  // It's possible for JS to query .buffered before we've determined the start
> +  // time from metadata, in which case the reader isn't ready to be asked this
> +  // question.
> +  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> +  NS_ENSURE_TRUE(mStartTime >= 0, NS_OK);

This check fails to work if MediaDecoderStateMachine::SetDuration changes mStartTime to 0 where we don't actually have the start time. We should only continue when the 1st frame is decoded.

Comment 38

[Bobby Holley (:bholley)]

(In reply to JW Wang [:jwwang] from comment #37)
> Comment on attachment 8521023 [details] [diff] [review]
> Part 3b - Teach MediaDecoderReader about its start time. v3
> 
> Review of attachment 8521023 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +3133,5 @@
> > +  // It's possible for JS to query .buffered before we've determined the start
> > +  // time from metadata, in which case the reader isn't ready to be asked this
> > +  // question.
> > +  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> > +  NS_ENSURE_TRUE(mStartTime >= 0, NS_OK);
> 
> This check fails to work if MediaDecoderStateMachine::SetDuration changes
> mStartTime to 0 where we don't actually have the start time. We should only
> continue when the 1st frame is decoded.

Let's handle that in a followup (perhaps bug 1096823?) - this stuff has been waiting to land for more than a week now.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d057f1ed36eb
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=dc17724b71ef

Comment 39

[(Away)]

Bug 1091008 and bug 1089937 collided on inbound. Please review this bustage fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/fda1b680307f

Comment 40

[Bobby Holley (:bholley)]

(In reply to David Major [:dmajor] (UTC+13) from comment #39)
> Bug 1091008 and bug 1089937 collided on inbound. Please review this bustage
> fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/fda1b680307f

lgtm - thanks for fixing that and averting the backout!

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/26b1bd849bb6
https://hg.mozilla.org/mozilla-central/rev/9255ba0d9aef
https://hg.mozilla.org/mozilla-central/rev/c5672eee4666
https://hg.mozilla.org/mozilla-central/rev/6907c1cb8bc0
https://hg.mozilla.org/mozilla-central/rev/95acf66c3675
https://hg.mozilla.org/mozilla-central/rev/d938d5ac11e3
https://hg.mozilla.org/mozilla-central/rev/deae19d73b34
https://hg.mozilla.org/mozilla-central/rev/fda1b680307f