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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

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.

Comment 1

[cajbir (:cajbir)]

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.

Comment 2

[Matthew Gregan [:kinetik]]

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 http://tinyvid.tv/show/2iv30opwi3gcv
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
/Users/kinetik/work/mozilla-central/mozilla/media/liboggplay/src/liboggplay/oggplay_buffer.c:150
#1  0x1354ac82 in oggplay_step_decoding (me=0x1dcd7f10) at
/Users/kinetik/work/mozilla-central/mozilla/media/liboggplay/src/liboggplay/oggplay.c:686
#2  0x1353a1fe in nsOggDecodeStateMachine::DecodeFrame (this=0x1dcd81e0) at
/Users/kinetik/work/mozilla-central/mozilla/content/media/video/src/nsOggDecoder.cpp:535

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

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.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Oops, neeed to fix test_wave_seek8 too.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/759009d95b41

Comment 7

[Justin Dolske [:Dolske]]

Is this a spec ambiguity?

The text in comment 0 is from "4.8.10.10 Seeking", but "4.8.10.6 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."

Comment 8

[Justin Dolske [:Dolske]]

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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/780d303e73aa