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)
Core
Audio/Video
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)
5.15 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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?
Comment 1•15 years ago
|
||
Does the scrubber do multiple seeks while its being dragged, or only when the mouse is released and dragging stops?
Reporter | ||
Comment 2•15 years ago
|
||
It does multiple seeks while being dragged.
Comment 3•15 years ago
|
||
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.
Reporter | ||
Comment 4•15 years ago
|
||
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.
Comment 5•15 years ago
|
||
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.
Reporter | ||
Comment 6•15 years ago
|
||
I filed bug 476984 to look at changing when the scrubber requests seeking.
isn't same as bug 475463, if so please mark one duplicate of other
Assignee | ||
Updated•15 years ago
|
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
Updated•15 years ago
|
tracking-fennec: ? → 1.0-
Assignee | ||
Comment 10•15 years ago
|
||
(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
Assignee | ||
Updated•15 years ago
|
Whiteboard: [depends on 480063?]
Assignee | ||
Comment 11•15 years ago
|
||
Is this still reproducible on trunk now that bug 480063 has landed?
Reporter | ||
Comment 12•15 years ago
|
||
Yeah, I can still reproduce the breakage with a current trunk build, although I don't see my CPU getting pegged when it happens.
Assignee | ||
Comment 13•15 years ago
|
||
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
Assignee | ||
Updated•15 years ago
|
Whiteboard: [depends on 480063?]
Assignee | ||
Comment 14•15 years ago
|
||
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.
Assignee | ||
Comment 15•15 years ago
|
||
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?
Comment 16•15 years ago
|
||
(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).
Assignee | ||
Updated•15 years ago
|
Attachment #377845 -
Flags: review?(chris.double)
Assignee | ||
Comment 17•15 years ago
|
||
Comment on attachment 377845 [details] [diff] [review] fix? I think this patch is actually correct.
Updated•15 years ago
|
Attachment #377845 -
Flags: review?(chris.double) → review+
Assignee | ||
Comment 18•15 years ago
|
||
With test.
Attachment #377845 -
Attachment is obsolete: true
Attachment #377918 -
Flags: review+
Assignee | ||
Comment 19•15 years ago
|
||
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]
Assignee | ||
Comment 20•15 years ago
|
||
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.
Assignee | ||
Comment 21•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f1a1ed90a9a7 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ae5dd47f6a03 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/43df36f6cdd1 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d4eb25ecfcad
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
You need to log in
before you can comment on or make changes to this bug.
Description
•