1121692 - Excessive seeking renders YouTube unplayable

Status: RESOLVED FIXED

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

Description

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

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).

Comment 1

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

Seems quite hard to reproduce; I've been trying and could not repro again.

Comment 2

[Bobby Holley (:bholley)]

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.

Comment 3

[Bobby Holley (:bholley)]

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. :-)

Comment 4

[Matt Woodrow (:mattwoodrow)]

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.

Comment 5

[Bobby Holley (:bholley)]

(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.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove unnecessary arguments to ::Seek. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 2 - Stop honoring aEndTime in MediaSourceReader::Seek. v1

Tracking it is a pain, and it's only used by OggReader.

Comment 8

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 9

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 10

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 6 - Make seeks cancelable. v1

This is the part I'm least sure about. Hopefully cpearce can point out any
pitfalls.

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 7 - Tests. v1

Comment 13

[Bobby Holley (:bholley)]

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

Comment 14

[Matt Woodrow (:mattwoodrow)]

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.

Comment 15

[Matt Woodrow (:mattwoodrow)]

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;

Comment 16

[Matt Woodrow (:mattwoodrow)]

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.

Comment 17

[Bobby Holley (:bholley)]

Thanks for the fast reviews guys!

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

Comment 18

[Bobby Holley (:bholley)]

(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.

Comment 19

[Bobby Holley (:bholley)]

(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).

Comment 20

[Bobby Holley (:bholley)]

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

Comment 21

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Fails intermittently on OSX too.

Comment 24

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

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

Comment 25

[Ralph Giles (:rillian)]

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?

Comment 26

[Bobby Holley (:bholley)]

(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.

Comment 27

[Ralph Giles (:rillian)]

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.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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