Closed
Bug 1091008
Opened 10 years ago
Closed 10 years ago
MediaDecoderStateMachine::HasLowUndecodedData doesn't work for MSE
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(8 files, 5 obsolete files)
(deleted),
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cajbir
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cajbir
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
This is the problem identified in bug 1063323 comment 3. I have patches to fix it.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
This one should do it, I think: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=33c41388ee21
Assignee | ||
Comment 5•10 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 6•10 years ago
|
||
Attachment #8515152 -
Flags: review?(cpearce)
Assignee | ||
Comment 7•10 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•10 years ago
|
||
We now have this stashed on the superclass.
Attachment #8515154 -
Flags: review?(cpearce)
Assignee | ||
Comment 9•10 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 10•10 years ago
|
||
Attachment #8515158 -
Flags: review?(cajbir.bugzilla)
Assignee | ||
Comment 11•10 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•10 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 13•10 years ago
|
||
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•10 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)
Updated•10 years ago
|
Attachment #8515152 -
Flags: review?(cpearce) → review+
Updated•10 years ago
|
Attachment #8515153 -
Flags: review?(cpearce) → review+
Updated•10 years ago
|
Attachment #8515154 -
Flags: review?(cpearce) → review+
Comment 15•10 years ago
|
||
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 16•10 years ago
|
||
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•10 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•10 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)
Updated•10 years ago
|
Attachment #8515442 -
Flags: review?(cpearce) → review+
Comment 19•10 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•10 years ago
|
Attachment #8515160 -
Flags: review?(cajbir.bugzilla) → review+
Comment 20•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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•10 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•10 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•10 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 | ||
Comment 25•10 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 26•10 years ago
|
||
Assignee | ||
Comment 27•10 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•10 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 29•10 years ago
|
||
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•10 years ago
|
Attachment #8515916 -
Attachment is obsolete: true
Assignee | ||
Comment 30•10 years ago
|
||
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•10 years ago
|
||
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•10 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•10 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•10 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•10 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)
Updated•10 years ago
|
Attachment #8521021 -
Flags: review?(cpearce) → review+
Comment 36•10 years ago
|
||
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 37•10 years ago
|
||
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•10 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
Comment 39•10 years ago
|
||
Bug 1091008 and bug 1089937 collided on inbound. Please review this bustage fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/fda1b680307f
Assignee | ||
Comment 40•10 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!
Comment 41•10 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•