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

RESOLVED FIXED

Status

()

Core
Audio/Video
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: kinetik, Assigned: roc)

Tracking

({fixed1.9.1})

unspecified
x86
Mac OS X
fixed1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

9 years ago
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?

Comment 1

9 years ago
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.
(Reporter)

Comment 2

9 years ago
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
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
Created attachment 378264 [details] [diff] [review]
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.
Attachment #378264 - Flags: review?(chris.double)
Whiteboard: [needs review]
Created attachment 378284 [details] [diff] [review]
fix v2

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)

Updated

9 years ago
Attachment #378284 - Flags: review?(chris.double) → review+
http://hg.mozilla.org/mozilla-central/rev/759009d95b41
Status: NEW → RESOLVED
Last Resolved: 9 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 "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."
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.