Closed Bug 1132851 Opened 9 years ago Closed 9 years ago

New seek doesn't cancel existing seek

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED DUPLICATE of bug 1135170

People

(Reporter: jya, Assigned: bholley)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Observed in YouTube with very low eviction threshold (1MB).

When evicting data very rapidly, YouTube attempt to recover by appending a new segment and seeking to it.

A few times I've managed to reproduce a problem where YouTube seeks rapidly. The MediaSourceReader initiate a seek which never completes.

The MediaDecoder would call MDSM::Seek, which schedule a new run of the MDSM and queue a task to MediaDecoderStateMachine::DecodeSeek

This one aborts early as mCurrentSeekTarget is valid.

And then JS initiate a seek again, which just does nothing.

Per spec, a new seek should replace the existing seek:
"The currentTime attribute must, on getting, return the media element's default playback start position, unless that is zero, in which case it must return the element's official playback position. The returned value must be expressed in seconds. On setting, if the media element has a current media controller, then the user agent must throw an InvalidStateError exception; otherwise, if the media element's readyState is HAVE_NOTHING, then it must set the media element's default playback start position to the new value; otherwise, it must set the official playback position to the new value and then seek to the new value. The new value must be interpreted as being in seconds."

http://dev.w3.org/html5/spec-preview/media-elements.html#dom-media-seek:
"If the element's seeking IDL attribute is true, then another instance of this algorithm is already running. Abort that other instance of the algorithm without waiting for the step that it is running to complete.
"

What actually happens: new seek is ignored
What should happen: Currently running seek is aborted and the new one runs to completion.

Only one seeked event is to be fired.
Priority: -- → P2
I reproduced the conditions of this bug with my checkpoint infrastructure, but it actually still works. The reason is that MDSM::SeekCompleted checks mSeekTarget and, if it exists, initiates another seek to that target.

jya, what am I missing?
Flags: needinfo?(jyavenard)
You're right.

I only traced the call to seek, and failed to realise it would be requeued in OnSeekFailed or in OnSeekCompleted.
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: needinfo?(jyavenard)
Resolution: --- → WORKSFORME
Actually no... I am seeing the issue.

When we reach MediaDecoderStateMachine::DecodeSeek

we exit early because:
mCurrentSeekTarget.IsValid()

However, mWaitingForDecoderSeek = false and mCancelingSeek = false.
and it returns early

Seems like mCurrentSeekTarget isn't cleared under some circumstances. This cause new seeks to never be resolved
Status: RESOLVED → REOPENED
Flags: needinfo?(bobbyholley)
Resolution: WORKSFORME → ---
Attached file log.txt.zip
the last successful seek was at target seek = 48251691

since that time, all new seek request aborted early.
From reading the log.
The seek succeeded, with a return time value of 45.88s (time of last i-frame).
The state machine enters the mode where it decode and drop all of frames < 48.25s

YouTube calls RangeRemoval with 0,54. So while we used to have enough data to decode up to 48.25s, now we don't. And never will again.

New seek request returns early as we've completed seek, but mCurrentSeekTarget is still valid.

While we are in the Drop*UpToTarget and a new seek is called, we should cancel Drop*UpToTarget and act on the new seek instead.
Properly interrupt seeking should a new seek occurs. Also, as per spec, the seeking event must be fired whenever a new seek occurs (we can have multiple seeking event of one seeked event). This wasn't the case.
Assignee: nobody → jyavenard
Status: REOPENED → ASSIGNED
(In reply to Jean-Yves Avenard [:jya] from comment #6)
>  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9d1f2a9a43d

This patch improves our results somewhat on the stress tests in bug 1134719, but they still time out after ~70 runs for me. I think that we should target this bug to making those tests go green.

I'm happy to do that today, but if you want to keep going on this I'm happy to do that too and go work on other things. Please let me know what you want to do by ~12:30PST, so I can know whether to work on this on the plane or do other things.
Flags: needinfo?(bobbyholley) → needinfo?(jyavenard)
Happy either way.

The core issue fixed in this patch is handling data being evicted while we're decoding between the time returned by the reader when Seek completes and the actual seek time. The MDSM would get stuck waiting for more data that will never come.

Also YouTube is particularly sensitive on the "seeking" event being fired each time they seek, they stop appending data otherwise.

There are improvements I wanted to do on this patch (that's why I haven't asked for review yet). But you can take it from here. I'll ask cpearce to review this one once done.
Flags: needinfo?(jyavenard)
seems to make one of the EME test consistantly timeout, yet dom/media/test/test_eme_playback.html doesn't even seek !
Ok. I'll rewrite get the stress tests green and then we'll see where we are.
Assignee: jyavenard → bobbyholley
Add more comments to the code. Happy how it is now.
Attachment #8566781 - Flags: review?(cpearce)
Attachment #8566460 - Attachment is obsolete: true
Assignee: bobbyholley → jyavenard
Assignee: jyavenard → bobbyholley
Comment on attachment 8566781 [details] [diff] [review]
Abort current seek operation if new seek called

I might refactor this differently, so canceling review for now.
Attachment #8566781 - Flags: review?(cpearce)
Depends on: 1135170
My patches in bug 1135170 should make the issue described in comment 5 a non-issue.

Those patches may not be backported to beta though, so we should consider whether we want jya's patch on beta, or whether we just want to live with this issue for a cycle.
Is this still an issue, Bobby, or can we close?
Flags: needinfo?(bobbyholley)
Bug 1135170 should fix the issue as described. We can file new bugs for any additional problems we discover.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Flags: needinfo?(bobbyholley)
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: