Excessive seeking renders YouTube unplayable

RESOLVED FIXED in Firefox 36

Status

()

Core
Audio/Video
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cpearce, Assigned: Bobby Holley (parental leave - send mail for anything urgent))

Tracking

(Blocks: 1 bug)

unspecified
mozilla38
x86_64
Windows 8.1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed, firefox38 fixed)

Details

Attachments

(8 attachments)

43.56 KB, patch
mattwoodrow
: review+
cpearce
: superreview+
Details | Diff | Splinter Review
4.46 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.87 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
5.79 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
2.73 KB, patch
mattwoodrow
: review+
cpearce
: review+
Details | Diff | Splinter Review
11.29 KB, patch
mattwoodrow
: review+
cpearce
: review+
Details | Diff | Splinter Review
4.19 KB, patch
cpearce
: review+
mattwoodrow
: review+
Details | Diff | Splinter Review
2.44 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
I turned on media.mediasource.decoder-per-segment, and then loaded https://www.youtube.com/watch?v=Cm7mCmncbvA in Nightly. It played for a bit, and I switched to 1080p. Then I kept seeking a little bit ahead of the last buffered position while playing. Once playback reached the end and the media was fully buffered, I clicked the seek bar and scrubbed it back and forward over the timeline erratically and quickly to force excessive seeking. I did that for about 15 seconds. The frame being displayed was stuck at the "Peanuts hua sheng" frame. If I seek, the seek appears to work, and the displayed frame changes, but once I let go of the scrubber the frame changes back to the "Peanuts hua sheng" frame. I also can't play the video anymore; the play buttons don't do anything.

I don't know if this bug occurs with media.mediasource.decoder-per-segment not set.

I think the key here to repro the bug is the excessive scrubbing/seeking once the media is buffered.

Windows 8.1, Nightly 38.0a1 (2015-01-14).
Flags: needinfo?(bobbyholley)
(Reporter)

Comment 1

3 years ago
Seems quite hard to reproduce; I've been trying and could not repro again.
I just reproduced this with a log and have it in a debugger. The MDSM is stuck in a seek state, with an invalid mSeekTarget and a valid mCurrentSeekTarget. I need to catch a bus, but will continue looking at this there.
Flags: needinfo?(bobbyholley)
I think the issue is that our seek logic doesn't have the ability to cancel the current seek and start a new one. When the MDSM is in DECODER_STATE_SEEKING, the state machine is basically stuck, assuming that the seek promise is going to get called back.

However, if we quickly seek twice, it's quite plausible that the youtube player doesn't even bother to initiate a load for the first chunk. So the player dutifully buffers the data near the logical seek point, while MediaSourceReader is stuck waiting on the old seek point, whose data isn't coming.

It seems like we basically want a CancelSeek on MediaSourceReader that rejects the promise and resets all the seek state. Matt, does that sound right to you?

We could even write a test for this. :-)
Flags: needinfo?(matt.woodrow)
MediaSourceReader already handles a second seek request by rejecting the initial seek promise and returning a new one.

We could just update MediaDecoderStateMachine to make use of this and not need CancelSeek at all.

That said, I'm not opposed to having an explicit CancelSeek instead if you prefer.
Flags: needinfo?(matt.woodrow)
(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> MediaSourceReader already handles a second seek request by rejecting the
> initial seek promise and returning a new one.
> 
> We could just update MediaDecoderStateMachine to make use of this and not
> need CancelSeek at all.
> 
> That said, I'm not opposed to having an explicit CancelSeek instead if you
> prefer.

Oh, I see. The advantage of an explicit CancelSeek is that we wouldn't have to disambiguate the promise rejections, and could wait for the rejection before starting the new seek. We could probably make it work either way though. I'll hack something up tomorrow.
Assignee: nobody → bobbyholley
Created attachment 8549967 [details] [diff] [review]
Part 1 - Remove unnecessary arguments to ::Seek. v1
Attachment #8549967 - Flags: superreview?(cpearce)
Attachment #8549967 - Flags: review?(matt.woodrow)
Created attachment 8549968 [details] [diff] [review]
Part 2 - Stop honoring aEndTime in MediaSourceReader::Seek. v1

Tracking it is a pain, and it's only used by OggReader.
Attachment #8549968 - Flags: review?(matt.woodrow)
Created attachment 8549969 [details] [diff] [review]
Part 3 - Fix potential race condition with mWaitingForSeekData. v1

mWaitingForSeekData is modified in NotifyTimeRangesChanged, which is
invoked on the main thread. One would hope that all of these other members
are only touched on the decode task queue.
Attachment #8549969 - Flags: review?(matt.woodrow)
Created attachment 8549971 [details] [diff] [review]
Part 4 - Clean up semantics around m{Audio,Video}IsSeeking. v1

The comments indicate that they're supposed to be used for setting mDiscontinuity
on the samples, but that never actually happens (and appears to happen in
MP4Reader.cpp). Resetting them in Request{Audio,Video}Data doesn't make any sense
at all. So we repurpose them to track our seek stage.
Attachment #8549971 - Flags: review?(matt.woodrow)
Created attachment 8549972 [details] [diff] [review]
Part 5 - Move the interesting seek state logic into DecodeSeek. v1

This is necessary so that we can call CancelSeek at the same moment we determine
that it needs to be called without round-tripping between threads.
Attachment #8549972 - Flags: review?(matt.woodrow)
Attachment #8549972 - Flags: review?(cpearce)
Created attachment 8549974 [details] [diff] [review]
Part 6 - Make seeks cancelable. v1

This is the part I'm least sure about. Hopefully cpearce can point out any
pitfalls.
Attachment #8549974 - Flags: review?(matt.woodrow)
Attachment #8549974 - Flags: review?(cpearce)
Created attachment 8549976 [details] [diff] [review]
Part 7 - Tests. v1
Attachment #8549976 - Flags: review?(matt.woodrow)
Attachment #8549976 - Flags: review?(cpearce)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c26019e4a254
Comment on attachment 8549967 [details] [diff] [review]
Part 1 - Remove unnecessary arguments to ::Seek. v1

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

Skimmed through this a bit, looks like the only meaningful change is to use mStartTime instead of aStartTime, which should be safe.
Attachment #8549967 - Flags: review?(matt.woodrow) → review+
Attachment #8549968 - Flags: review?(matt.woodrow) → review+
(Reporter)

Updated

3 years ago
Attachment #8549967 - Flags: superreview?(cpearce) → superreview+
Attachment #8549969 - Flags: review?(matt.woodrow) → review+
(Reporter)

Updated

3 years ago
Attachment #8549972 - Flags: review?(cpearce) → review+
Attachment #8549971 - Flags: review?(matt.woodrow) → review+
Attachment #8549972 - Flags: review?(matt.woodrow) → review+
(Reporter)

Updated

3 years ago
Attachment #8549974 - Flags: review?(cpearce) → review+
Comment on attachment 8549976 [details] [diff] [review]
Part 7 - Tests. v1

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

::: dom/media/mediasource/test/test_SeekTwice_mp4.html
@@ +44,5 @@
> +      return p;
> +    }).then(function() {
> +      ok(true, "Got seeking event");
> +      var p = once(el, 'seeked');
> +      el.currentTime = 6; // Seek past the gap.

return p;
Attachment #8549976 - Flags: review?(matt.woodrow) → review+
(Reporter)

Updated

3 years ago
Attachment #8549976 - Flags: review?(cpearce) → review+
Comment on attachment 8549974 [details] [diff] [review]
Part 6 - Make seeks cancelable. v1

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

r=mattwoodrow with the change to SeekCompleted mentioned in irc.
Attachment #8549974 - Flags: review?(matt.woodrow) → review+
Thanks for the fast reviews guys!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9c89a1ecfbe
(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #17)
> Thanks for the fast reviews guys!
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9c89a1ecfbe

There were 2 failures on the above push:
(1) I forgot to disable the test on linux, where we don't support H264.
(2) We were hitting an assertion I added in one of the patches. I'm attaching a followup patch just to detect that case and bail out. We'd really like to get this patch into the next beta and today is saturday in New Zealand. Given that this patch is quite safe, I'm flagging Chris for retroactive review. I'll also file a followup bug to fix up the MDSM to not violate the invariants.
(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #18)
> We were hitting an assertion I added in one of the patches. I'm
> attaching a followup patch just to detect that case and bail out.

(Note that I managed to reproduce the failure locally, and verify my fix).
Depends on: 1119757, 1120266
I had to rebase this on top of bug 1119757, which landed on inbound this morning. Still seems to work.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e073609378c7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/60fd49cbcb18
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/eb34c34e49af
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/552b8f18897c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a011bfe9487a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5609699a8076
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9bf200fcadbc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3d0dd2ae7e6c
Created attachment 8550435 [details] [diff] [review]
Part 6.5 - Handle mid-seek Request{Audio,Video}Data calls. v1 rpending=cpearce

The MDSM really shouldn't be issuing these. The correct solution (which I'll
file a followup bug for) is to fix the MDSM to not do that. But rejecting the
promise with CANCELED is a lot safer, so we do that for now.
Attachment #8550435 - Flags: review?(cpearce)
(Reporter)

Updated

3 years ago
Attachment #8550435 - Flags: review?(cpearce) → review+
Test disabled across the board because at this point it fails for sure on Windows (as your last Try push showed???) and Android (which your Try pushes didn't actually test - yay chunking). Rather than keep the tree closed while I wait to find out what other platforms it's broken on, I've disabled it across the board. You sort it out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cde7eff18b1a
Fails intermittently on OSX too.
Depends on: 1122765
https://hg.mozilla.org/mozilla-central/rev/e073609378c7
https://hg.mozilla.org/mozilla-central/rev/60fd49cbcb18
https://hg.mozilla.org/mozilla-central/rev/eb34c34e49af
https://hg.mozilla.org/mozilla-central/rev/552b8f18897c
https://hg.mozilla.org/mozilla-central/rev/a011bfe9487a
https://hg.mozilla.org/mozilla-central/rev/5609699a8076
https://hg.mozilla.org/mozilla-central/rev/9bf200fcadbc
https://hg.mozilla.org/mozilla-central/rev/3d0dd2ae7e6c
https://hg.mozilla.org/mozilla-central/rev/cde7eff18b1a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Bobby, I didn't nominate this for 36b2 over the weekend because I was concerned about the test failures Ryan reported in comment #22. Do you think they're ok for 36beta3 as-is, or if I should wait for the the follow-up bug?
Flags: needinfo?(bobbyholley)
(In reply to Ralph Giles (:rillian) from comment #25)
> Bobby, I didn't nominate this for 36b2 over the weekend because I was
> concerned about the test failures Ryan reported in comment #22. Do you think
> they're ok for 36beta3 as-is, or if I should wait for the the follow-up bug?

We should definitely uplift this - hopefully it's not too late?

The tests did fail, but they were the first/only MSE mp4 tests that anybody ever attempted to put in the tree. This is kind of terrible because that's the exact configuration we're shipping in 4 weeks, but I wouldn't attribute the test failures to this bug. We definitely want this fix in the beta.
Flags: needinfo?(bobbyholley)
Comment on attachment 8549967 [details] [diff] [review]
Part 1 - Remove unnecessary arguments to ::Seek. v1

Ok, thanks for clarifying, and now this has had the weekend to settle. Requesting uplift approval for all patches on this bug for Aurora and Beta, 36 beta 2 if possible. This was another of our major MSE blockers for 36.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: YouTube playback sometimes doesn't work after seeking.
[Describe test coverage new/current, TBPL]: Landed on m-c. In Nightly over the weekend.

[Risks and why]: Risk is moderate. There are a lot of changes here, and in general video playback code. We think it works, or is at least better than what we have in-tree now. We'd like to get this in all branches to maximize early testing of the non-MSE changes.

[String/UUID change made/needed]: None.
Attachment #8549967 - Flags: approval-mozilla-beta?
Attachment #8549967 - Flags: approval-mozilla-aurora?
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → fixed
Attachment #8549967 - Flags: approval-mozilla-beta?
Attachment #8549967 - Flags: approval-mozilla-beta+
Attachment #8549967 - Flags: approval-mozilla-aurora?
Attachment #8549967 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/19e89299b366
https://hg.mozilla.org/releases/mozilla-aurora/rev/c8c80f89364d
https://hg.mozilla.org/releases/mozilla-aurora/rev/d84cc3f47d94
https://hg.mozilla.org/releases/mozilla-aurora/rev/dcd2e226ad99
https://hg.mozilla.org/releases/mozilla-aurora/rev/a05c0a8774ca
https://hg.mozilla.org/releases/mozilla-aurora/rev/ffbca1189afb
https://hg.mozilla.org/releases/mozilla-aurora/rev/8168584206ed
https://hg.mozilla.org/releases/mozilla-aurora/rev/472db2af9fe6

https://hg.mozilla.org/releases/mozilla-beta/rev/d7e079df1b3d
https://hg.mozilla.org/releases/mozilla-beta/rev/67f6899c6221
https://hg.mozilla.org/releases/mozilla-beta/rev/871ab0d29bb8
https://hg.mozilla.org/releases/mozilla-beta/rev/35f5cf685186
https://hg.mozilla.org/releases/mozilla-beta/rev/3e1dd9e96598
https://hg.mozilla.org/releases/mozilla-beta/rev/2195dc79a65f
https://hg.mozilla.org/releases/mozilla-beta/rev/4f059ea15ecf
https://hg.mozilla.org/releases/mozilla-beta/rev/56744595737c
status-firefox36: affected → fixed
status-firefox37: affected → fixed
Flags: in-testsuite+
Flags: qe-verify-
Blocks: 1126052
You need to log in before you can comment on or make changes to this bug.