Closed Bug 476973 Opened 15 years ago Closed 15 years ago

video seeking can break when moving scrubber to beginning or end.

Categories

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

defect

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 1.0- ---

People

(Reporter: Dolske, Assigned: roc)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 1 obsolete file)

I was playing around with the scrubber, using a file:// Ogg, checking out how fast seeking works on a local file.

If I click-and-hold the scrubber, and then stress it by rapidly moving it back and forth (carefully avoiding going full-stop to the beginning or end), everything works fine.

But if I happen to move the scrubber all the way to the beginning (or end) while doing this, the video often gets stuck at that point. [I can also fairly easily reproduce this when moving the scrubber slowly.] The scrubber can still be dragged, and the control's logging shows that they're trying to change the value of video.currentTime, but nothing else happens... No timeupdate events, no seeking events, and clicking pause/play doesn't help.

My CPU gets pegged at 100% when this happens, and remains there after closing the tab with the video.

OS X's sample utility seems to imply multiple threads chugging away in oggplay_callback_info_prepare().
Flags: blocking1.9.1?
Does the scrubber do multiple seeks while its being dragged, or only when the mouse is released and dragging stops?
It does multiple seeks while being dragged.
Given that HTTP seeks are quite slow, do you think it might be a good idea to change it to only seek when you've finished the drag? Otherwise seeks will be much much much slower.

That doesn't affect this bug of course, since the issue still needs to be resolved.
Well, it seems (from observation) that while a seek in in progress, only the most recent of further seeks is queued up. At least that's end result the controls see (they get a timeupdate around the first seek, then a timeupdate around the last seek, and nothing in between). That's good, we wouldn't want stale seeks to be all queued and operated upon in sequence.

At least ideally, seeking while dragging is desirable, so you can see the intermediate steps. Although given our current state of being slow and not caching data, perhaps that should just be disabled... It works well on file:// videos, but that's not going to be the common usage.
I agree it's a nice feature when seeking is fast, but in the current situation until we improve it'd be useful to disable it since it makes seeking at least twice as slow.
I filed bug 476984 to look at changing when the scrubber requests seeking.
Blocks: TrackAVUI
isn't same as bug 475463, if so please mark one duplicate of other
tracking-fennec: --- → ?
OS: Mac OS X → All
Hardware: x86 → All
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Justin, I have now noticed we can recreate similar is issue even when dragging thumb back and forth in the buffered range. Is that same as this bug?

Steps for that.
1. goto
   http://upload.wikimedia.org/wikipedia/commons/6/64/Fraser_island_australia_sand_track.ogg
2. wait for it to load
3. Start playing video
4. Now hold the thumb using mouse
5. drag thumb back and forth few times.
Result:- video getting stuck and controls not responding

If I use another controls interface I dont see this problem.
use following bookmarklet to make a video controls interface
http://www.tapper-ware.net/devel/js/JS.TinyVidPlayer/v002/bookmarklet/index.html
tracking-fennec: ? → 1.0-
(In reply to comment #0)
> OS X's sample utility seems to imply multiple threads chugging away in
> oggplay_callback_info_prepare().

This makes me suspect this is the same as bug 480063.
Depends on: 480063
Whiteboard: [depends on 480063?]
Is this still reproducible on trunk now that bug 480063 has landed?
Yeah, I can still reproduce the breakage with a current trunk build, although I don't see my CPU getting pegged when it happens.
Attached patch fix? (obsolete) — Splinter Review
What seems to happen:
-- nsOggDecoder::Seek is called to seek to 0 (or the end)
-- we start seeking, enter DECODER_STATE_SEEKING, and do the seek
-- meanwhile nsOggDecoder::Seek is called again to seek to the same position. We set mRequestedSeekTime to that value.
-- nsOggDecodeStateMachine::Run eventually finishes the seek and synchronously dispatches SeekingStopped.
-- SeekingStopped does ChangeState(PLAY_STATE_SEEKING) since we have another pending request (mRequestedSeekTime is non-negative). So we stay in PLAY_STATE_SEEKING and nsOggDecodeStateMachine::Seek sets mSeekTime to the new seek position (which is also the current position). It also sets the decoder state to DECODER_STATE_SEEKING, but it already is in that state.
-- After SeekingStopped is done, nsOggDecodeStateMachine::Run notes that mSeekTime is still the requested time it started with, so sets its state to DECODER_STATE_DECODING.

At this point we're in PLAY_STATE_SEEKING but no seek is actually taking place. That's bad, nsOggDecoder::IsSeeking will keep returning true forever. No further seeks can happen either.

This patch fixes the problem by entering DECODER_STATE_DECODING *before* we call SeekingStopped. So if SeekingStopped wants us to seek again, we will, even if its destination is the same as the current time. We should add a check somewhere to avoid actually doing a seek if the requested time is the current time... but that would only mask this bug, not really fix it, so I haven't done that yet.

Unfortunately with this patch we seem to hit all kinds of problems when doing a lot of seeks. I suspect these are latent bugs, not regressions, but I'm not 100% sure.
Assignee: nobody → roc
Whiteboard: [depends on 480063?]
There seem to be two major problems. The first is easily reproducible without this patch: seeking backwards, especially, doesn't work because of some A/V sync issue, almost certainly a regression from bug 466699.

The second problem is only reproducible with this patch: seeking to the end of the stream causes an assertion because we don't have a next frame, and then for some reason or another playback is just stuck, seeking backwards doesn't make us play again. That may be a regression from this patch.
The second problem is simply that we successfully seek to the end of the file and then oggplay_step_decoding returns OGGPLAY_OK to indicate EOF. There is really is no frame to decode there. I'm not sure how we should handle this, it seems we need to seek to a position before the end of file to find the last frame, but how?
(In reply to comment #14)
> There seem to be two major problems. The first is easily reproducible without
> this patch: seeking backwards, especially, doesn't work because of some A/V
> sync issue, almost certainly a regression from bug 466699.
> 

What happens when you try to seek backwards? Seems odd that tests didn't fail for this case. Possibly because the tinderboxes don't have audio hardware and the tests are only testing the system clock.

A quick look at the code (I'm not on a machine that can build/test) doesn't seem to indicate that the audio is paused/resumed during seeking so the audio clock time may not match the frame time resulting in issues. I can have a look tomorrow (err, later today).
Comment on attachment 377845 [details] [diff] [review]
fix?

I think this patch is actually correct.
Attachment #377845 - Flags: review?(chris.double) → review+
Attached patch fixSplinter Review
With test.
Attachment #377845 - Attachment is obsolete: true
Attachment #377918 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/314dbe120a3e
This uncovered some invalid assumptions in seek tests, which I've fixed:
http://hg.mozilla.org/mozilla-central/rev/4c6d522e3075
http://hg.mozilla.org/mozilla-central/rev/485b7d5cc2ee
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs 191 landing]
The first seeking bug in comment #14 is a regression from the A/V sync changes. Chris D is going to file a bug on that. I need to file a new bug on the second issue in comment #14.
You need to log in before you can comment on or make changes to this bug.