493431 - .currentTime is reported as -0.001 for the last frame of some videos

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Justin Dolske [:Dolske]]

Spun off from bug 481106.

Some videos are reporting .currentTime == 1 for the last frame. This is wrong for two reasons:

* It shouldn't ever be < 0
* The time of the last frame should be approximately (exactly?) equal to the actual duration

STR:

Enable videocontrol logging (or just add a timeupdate listener that dumps .currentTime). Open and play http://tinyvid.tv/show/1obk8qgd3j3s5, wait for it to reach the end. The value of .currentTime is -1 both when the last timeupdate event fires, and when the ended event fires.

Comment 1

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

currentTime is actually -0.001, but I guess that's what you meant.

Comment 2

[Justin Dolske [:Dolske]]

Oops, yeah. The videocontrols work with milliseconds, so the value they log is -1 instead of -.001.

Comment 3

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

Attachment: fix

The video stream's video ends before the audio. This causes oggplay_callback_info_get_headers to return null for the video track, then oggplay_callback_info_get_presentation_time returns -1 as an error indicator, but we treat it as a time.

My solution here is to set the frame time and state based on the video data if there is any for this frame, otherwise the audio data.

Comment 4

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

Generally it seems clear that bundling audio and video data into common "frames" is a mistake, we should just get them as independent streams and take responsibility for synchronizing them. We already have to handle synchronization anyway, in practice, and the oggplay frames are just adding complexity. If this means removing liboggplay, then so be it.

Comment 5

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

Attachment: with test

This test doesn't actually work at the moment, it triggers bug 493678 :-(

Comment 6

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

http://hg.mozilla.org/mozilla-central/rev/c7326aaa1a9a

Comment 7

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/368cd4af46f0

Comment 8

[Aaron Train [:aaronmt]]

Rob, 

Tried out your test with the short video on rv:1.9.1pre Gecko/20090521 and it passes. Push?

Comment 9

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

I'm not sure what you mean by 'Push?'. I did check this into 1.9.1 and marked this bug fixed1.9.1.

Comment 10

[Aaron Train [:aaronmt]]

I meant that your test should be included in the suite now.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch - add test

Add test to ensure video file with more audio than video plays and that video.currentTime only increases during playback.

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Pushed testcase to m-c: http://hg.mozilla.org/mozilla-central/rev/0028bf2bbf04

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 394435 [details] [diff] [review]
Patch - add test

Can I push this to 192 to reduce merge conflicts? It's been on trunk for about 2 months now.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Pushed test to 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ebf070248e50