MediaDecoderStateMachine::HasLowUndecodedData doesn't work for MSE

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla36
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 5 obsolete attachments)

6.57 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
26.42 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
10.71 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
5.17 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
9.08 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
4.21 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
1.83 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
6.63 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
This is the problem identified in bug 1063323 comment 3. I have patches to fix it.
(Assignee)

Comment 5

5 years ago
Otherwise, we end up with a subsequent call to DiscardDecoder() which crashes,
because mParentDecoder (and thus its monitor) is null.
Attachment #8515151 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 7

5 years ago
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.
Attachment #8515153 - Flags: review?(cpearce)
(Assignee)

Comment 8

5 years ago
We now have this stashed on the superclass.
Attachment #8515154 - Flags: review?(cpearce)
(Assignee)

Comment 9

5 years ago
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.
Attachment #8515155 - Flags: review?(cpearce)
(Assignee)

Comment 11

5 years ago
The same code will now be reached by invoking this method on the decoder.
Attachment #8515160 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 12

5 years ago
Since GetBuffered now has a sane implementation for MSE, this should
make this function sane for MSE as well.
Attachment #8515164 - Flags: review?(cpearce)
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?
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 14

5 years ago
(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
Flags: needinfo?(bobbyholley)
Attachment #8515152 - Flags: review?(cpearce) → review+
Attachment #8515153 - Flags: review?(cpearce) → review+
Attachment #8515154 - Flags: review?(cpearce) → review+
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.
Attachment #8515155 - Flags: review?(cpearce) → review+
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.
Attachment #8515164 - Flags: review?(cpearce) → review-
(Assignee)

Comment 17

5 years ago
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.
Attachment #8515151 - Attachment is obsolete: true
Attachment #8515151 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 18

5 years ago
Thanks for all the super-fast reviews (here and elsewhere) - especially on a
weekend :-)
Attachment #8515164 - Attachment is obsolete: true
Attachment #8515442 - Flags: review?(cpearce)
Attachment #8515442 - Flags: review?(cpearce) → review+

Comment 19

5 years ago
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.
Attachment #8515158 - Flags: review?(cajbir.bugzilla) → review+

Updated

5 years ago
Attachment #8515160 - Flags: review?(cajbir.bugzilla) → review+
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 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 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.
(Assignee)

Comment 23

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

Comment 24

5 years ago
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.
Attachment #8515909 - Flags: review?(jwwang)
(Assignee)

Updated

5 years ago
Attachment #8515153 - Attachment description: Part 2 - Teach MediaDecoderReader about its start time. v2 → Part 3 - Teach MediaDecoderReader about its start time. v2
(Assignee)

Updated

5 years ago
Blocks: 1093020
(Assignee)

Comment 25

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

Comment 27

5 years ago
Rebased this so that it actually fits as part 1 in the series.
Attachment #8515909 - Attachment is obsolete: true
Attachment #8515909 - Flags: review?(jwwang)
Attachment #8515916 - Flags: review?(jwwang)
(Assignee)

Comment 28

5 years ago
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.
Attachment #8515916 - Flags: review?(jwwang) → review?(cpearce)
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).
Attachment #8515916 - Flags: review?(cpearce) → review-
(Assignee)

Updated

5 years ago
Attachment #8515916 - Attachment is obsolete: true
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
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 32

5 years ago
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.
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 33

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

I split this out into bug 1096597.
(Assignee)

Comment 34

5 years ago
If we don't do this, we'll get out of sync with the reader.
Attachment #8521021 - Flags: review?(cpearce)
(Assignee)

Comment 35

5 years ago
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).
Attachment #8515153 - Attachment is obsolete: true
Attachment #8521023 - Flags: review?(cpearce)
Attachment #8521021 - Flags: review?(cpearce) → review+
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.
Attachment #8521023 - Flags: review?(cpearce) → review+
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.
(Assignee)

Comment 38

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

Comment 40

5 years ago
(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!

Updated

4 years ago
Depends on: 1121148
Depends on: 1139874
No longer depends on: 1139874
You need to log in before you can comment on or make changes to this bug.