Closed Bug 486922 Opened 13 years ago Closed 13 years ago

Clamp seek requests to max(requested, earliestpossible) and min(requested, duration)


(Core :: Audio/Video, defect)

Not set





(Reporter: kinetik, Assigned: roc)


(Keywords: fixed1.9.1)


(1 file, 1 obsolete file)

We currently raise an exception in nsHTMLMediaElement::SetCurrentTime if the requested currentTime is less than 0 and let the decoder handle the greater than duration case.

The spec says:
> 2.  If the new playback position is later than the end of the media resource, then let it be the end of the media resource instead.

> 3.  If the new playback position is less than the earliest possible position, let it be that position instead.

So we should clamp the requested time as described.
Flags: blocking1.9.1?
We probably need to store the earliest possible position then too. Currently 'currentTime' comes from the positions returned from the media file. If it's an oggz-chop file it is the chopped time position. We'll also need to adjust the end position for the duration being added to the initial time.

For example, if the file is chopped from 10s to 20s, then the first initial currentTime is 10s. 

There's some discussion going on at the moment amongst the liboggplay/liboggz developers as to whether that should be the case or not.
This might be another bug, but it's so close to this area that I'll just
mention it here.  It looks like we're not clamping the seek requests to the
duration at all right now, so you can crash the browser by doing the following:

1. Load
2. Open a JS shell
3. Evaluate "v1.currentTime = 99999999999999" about 5-10 times
4. Crash in oggplay_buffer_set_last_data:

150        p->stream_info = OGGPLAY_STREAM_LAST_DATA;
(gdb) p p
$15 = (OggPlayCallbackInfo *) 0x0
(gdb) bt 3
#0  0x1354d7ef in oggplay_buffer_set_last_data (me=0x1dcd7f10,
buffer=0x17185d40) at
#1  0x1354ac82 in oggplay_step_decoding (me=0x1dcd7f10) at
#2  0x1353a1fe in nsOggDecodeStateMachine::DecodeFrame (this=0x1dcd81e0) at
Flags: blocking1.9.1? → blocking1.9.1+
We need to make sure that all videos start at currentTime==0, so we can just clamp the seek position to [0, duration].

We should actually block the release on this since it affects authors quite a bit.
Assignee: nobody → chris.double
Assignee: chris.double → roc
Attached patch fix (obsolete) — Splinter Review
Simple stuff now ... just clamp the requested time to [0, duration]. When we seek to the duration, we actually get the frame before the duration, so the time is a little bit off.
Attachment #378264 - Flags: review?(chris.double)
Whiteboard: [needs review]
Attached patch fix v2Splinter Review
Oops, neeed to fix test_wave_seek8 too.
Attachment #378264 - Attachment is obsolete: true
Attachment #378284 - Flags: review?(chris.double)
Attachment #378264 - Flags: review?(chris.double)
Attachment #378284 - Flags: review?(chris.double) → review+
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review] → [needs 191 landing]
Is this a spec ambiguity?

The text in comment 0 is from " Seeking", but " Offsets into the media resource" says setting .currentTime "will throw an INDEX_SIZE_ERR  exception if the given time is not within the ranges to which the user agent can seek."
FWIW, I think I prefer the clamping. Consider bug 486899, where I'm adding a keyboard shortcut for jumping forward 15 seconds. "video.currentTime += 15" is nice and simple, it seems ugly to have to check what the duration is, manually clamp to that value before setting .currentTime, and then trying to figure out what to do when the the duration isn't known.
The spec says that you should first clamp to [0,duration] and then check if the time is in a region the UA can actually seek to, throwing INDEX_SIZE_ERR if not. The UA is not required to be able to seek everywhere within [0,duration], and should expose the actual seekable ranges through 'seekable'. We currently don't support 'seekable' and assume we can seek everywhere within [0,duration], although that's not always true. (But we can't tell if it's not true, really, since we have no idea what a server will do when we throw it a byte-range request.)
You need to log in before you can comment on or make changes to this bug.