Closed Bug 496051 Opened 10 years ago Closed 10 years ago

Pressing End key stops video playback

Categories

(Core :: Audio/Video, defect)

1.9.1 Branch
All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.1 --- ?

People

(Reporter: whimboo, Assigned: cpearce)

References

()

Details

(Whiteboard: [Fixed by bug 512327])

Attachments

(1 file, 3 obsolete files)

Chris, you said over in bug 495159 that the end key will work again when the
patch on bug 487519 is checked in.

I cannot see that this key works. Pressing it the video stops playing until you press another key like Space, Right, or Left.

Asking for blocking based on the debate about a non-working key on bug 495159.
Flags: blocking1.9.1?
Not sure that this blocks, though if there's an easy fix, we should take it (consider that as my pre-approval after a safe landing on m-c)
Flags: wanted1.9.0.x?
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Hmm... You're right, that video doesn't work. I'd guess the presentation time of the first frame is still not being calculated correctly by liboggplay. It WFM on other videos, and at least we're not crashing or hanging on that broken one.
Thanks for following up on this Henrik.

I've checked, liboggplay is still giving us an incorrect presentation time for the first frame (or its giving us the second frame instead of the first, I'm not sure which).

I had thought that Chris Double was going to use his psychic powers to fix liboggplay with his patch for bug 487519, but it appears he merely made the annodex libraries not ignore errors and check their pointers before dereferencing.

To resolve this bug we can either:
1. Wait until liboggplay is fixed upstream (there's a patch in progress from upstream in bug 487519), or
2. add a simple work-around which clamps seek times inside nsOggDecodeStateMachine::Seek() (this is easy, but a bit of a hack), or
3. not seek to the end on 'end' key press.

Why are we seeking to the end on 'end' key press? It's just another way to pause, except that it's slower, as seeks require HTTP transactions... If we could play backwards, it would be worthwhile seeking to the end, but as is, the only thing we can do when we seek to the end is display the last frame or seek somewhere else. How does this behaviour benefit our users? I can't recall any instance where I've wanted to seek to the end in all the years I've been using digital video.
(In reply to comment #3)

> Why are we seeking to the end on 'end' key press?

It helps with keyboard navigation; not so much as a final destination but as a step along the way... EG, if you want to watch something near the end of the video it's easier to seek to the end and then make a short jump back (instead of many jumps forward from the beginning).

    It's just a jump to the end
    And then a step to the left
    Put your hands on your hips
    You bring your knees in tight
    ...
Rock on Dolske.

(In reply to comment #3)
> To resolve this bug we can either:
> 1. Wait until liboggplay is fixed upstream (there's a patch in progress from
> upstream in bug 487519), or
> 2. add a simple work-around which clamps seek times inside
> nsOggDecodeStateMachine::Seek() (this is easy, but a bit of a hack), or

(2) won't work for oggzchopped videos, we can't do a quick fix. We must wait for upstream to fix liboggplay... Once that happens we also need to change the decode-forwards-from-key-frame-to-target-frame loop so that it breaks if we encounter a frame without video (if we have a video track) near the target. This happens in the test video, as the last frame is audio only.
Attached patch Patch v1 (obsolete) — Splinter Review
Actually we can clamp to hack seek-to-end to work. We need to clamp the seek target using the timestamp of the last frame, which we can get from the nsChannelReader. So when you press END, you seek to the last frame. If the last frame has audio, and you're playing, it will play the audio from the last frame. To prevent this behaviour, we'd have to do a special case for seek-to-end.
Assignee: nobody → chris
Attachment #381415 - Flags: superreview?(roc)
Attachment #381415 - Flags: review?(chris.double)
+  // Clamp seek time to be less than the timestamp of the last frame.

less than or equal to

+  mSeekTime = PR_MIN(aTime + mPlaybackStartTime, mDecoder->mReader->duration());

what if duration() is not known, it returns -1 right? Then you're stuffed!

You need a comment explaining that this should not be necessary due to the clamping in nsHTMLMediaElement::SetCurrenTime, but it is due to the fact that mPlaybackStartTime may not be accurate.

+          if (!nextFrame || (mVideoTrack != -1 && !nextFrame->mVideoHeader))

what's this for?
Attachment #381415 - Attachment is obsolete: true
Attachment #381415 - Flags: superreview?(roc)
Attachment #381415 - Flags: review?(chris.double)
Comment on attachment 381415 [details] [diff] [review]
Patch v1

This can't possibly work. We add the timestamp of the first frame to the nsChannelReader's last frame time when we get a duration from headers. I don't know how it worked for me earlier today... I must have only tested on video without a duration header.
We don't have ogg support for 1.9.0 so I believe it was meant to set wanted1.9.1.x.
Flags: wanted1.9.0.x? → wanted1.9.1.x?
status1.9.1: --- → ?
Flags: wanted1.9.1.x?
I have a patch for this.

From bug 495159 comment 1, (modified) STR:

1. Open a page like http://tinyvid.tv/show/36mvdjoflfskb
2. Wait until the video has been loaded (optional)
3. Press play
4. Press Fn+Right or End

Observed result:
* Video playback stops, and the playback head is not at the end.

Expected result:
* Video playback stops, displays last frame, video playback head should be at the end.
(In reply to comment #10)
> Expected result:
> * Video playback stops, displays last frame, video playback head should be at
> the end.

Actually, though displaying the last frame would be very nice, it's a lot harder than I thought at first glance. I'll post the patch for fixing the presentation time, so that seek-to-end works, and so that I can close a bunch of bugs, and I'll come back to fixing the "display the last frame" issue in a separate bug
Blocks: 495869
liboggplay was calculating the presentation time to be (frame_number * frame_duration), with the frame_number starting at 1, so it was actually the end time of the frame, not the start. This caused all our frame time and duration calculations to be off by 1 frame duration, and caused seeks to video.duration to be out of bounds of the actual (internally known) duration. 

This patch fixes liboggplay to correctly calculate the presentation time for frames. This also causes the oggplay_step_decode() function to terminate as soon as it runs out of data; it was previously calculating "overhangs" based on the incorrect presentation time. I had to make a minor change to nsOggDecoder so that we handle transition to COMPLETED state without playing and with nsOggDecodeStateMachine::mDecodedFrames empty. Without those changes, mochitest test_load.html fails when trying to load bug506094.ogv, which consists of only header packets.

Tests pass on Windows. I've pushed to tryserver, and will report if the tests fail there. With this patch, if you press the end key, we correctly seek to the end of resource. I'll make a testcase for this when I figure out why the seek mochitests are randomly failing!

I will send this liboggplay patch upstream for review.
Attachment #394760 - Flags: review?(chris.double)
Attached patch Patch v3 - simpler patch (obsolete) — Splinter Review
Basically the same as v3, but simpler.
Attachment #394760 - Attachment is obsolete: true
Attachment #394769 - Flags: review?(chris.double)
Attachment #394760 - Flags: review?(chris.double)
Created annodex trac ticket for liboggplay bug:
https://trac.annodex.net/ticket/494
Additionally we can get stuck in an infinite loop in oggplay_step_decoding() in the "overhang" handling case, just after seeking to the end. In such a case, we always try to read more data, but there's no more data to read, and we end up in an infinite loop. That's why my patch changes oggplay_step_decoding().
I spoke on IRC to wiking this morning (18 Apr 09 NZT). He's taken my presentation time fix on liboggplay trunk already (so we can assume that's an r=wiking), but we talked about my change to oggplay_step_decoding() and it turns out the reason we're going into an infinite loop is that we're calling oggplay_step_decoding() after it's returned E_OGGPLAY_OK. oggplay_step_decoding() returns E_OGGPLAY_OK to denote that it's hit EOF while decoding. So we can prevent this by transitioning the decode state machine to COMPLETED when DecodeFrame() returns E_OGGPLAY_OK.

Yes, oggplay_step_decoding() should not go into a loop in this condition, but I'd rather they dealt with that problem...

I will revise the patch...

[09:56]	<wiking>	so, yeah i've checked the patch you've sent and took the presentation time fix of the patch
[09:56]	<wiking>	it's already pushed to the repo
[09:57]	<cpearce>	wiking: ok, so it all makes sense I take it? ;)
[09:57]	<wiking>	well
[09:57]	<wiking>	the other part of it
[09:57]	<wiking>	for oggplay_step_decoding
[09:58]	<cpearce>	wiking: yeah, we were getting stuck in an infinite loop in oggplay_step_decode() when seeking in very large files.
[09:59]	<wiking>	cpearce: on the end of the file?
[09:59]	<cpearce>	when seeking to end of file actually, the overhang handling was failing there.
[09:59]	<wiking>	because i was just wondering how is it possible to hang there
[09:59]	<wiking>	because an EOF should come from oggz_read
[09:59]	<cpearce>	seeking to t=duration end of file, yeah.
[09:59]	<wiking>	and EOF never comes? :)
[10:00]	<cpearce>	yeah, we get stuck thinking that there's more data or something, and never break out of that read loop.
[10:00]	<wiking>	hmm
[10:00]	<wiking>	do you have an example
[10:02]	<wiking>	have a local clone of the mozilla repo
[10:03]	<cpearce>	wiking: ok, if you apply patch from bug 496051 and rebuild you can repro the infinite loop by focusing a video and pressing the END key.
[10:04]	<wiking>	cpearce: so what's the test case ;)
[10:05]	<cpearce>	wiking: sorry, you shouldn't apply the oggplay_step_decode() part of the patch of course.
[10:05]	<cpearce>	wiking: I think it should work on all videos, I'm just testing now.
[10:09]	<cpearce>	wiking: looks like it works on all videos. the problem definintely happens on this one: http://pearce.org.nz/mozilla/320x240.ogg just click somewhere in the video, and then press END.
[10:09]	<wiking>	cpearce: checking
[10:10]	<wiking>	cpearce: i wonder if it does the same with oggplayer ;P
[10:10]	<cpearce>	wiking: it probably would, except you can't repro it in oggplayer as it can't seek to a specific time.
[10:11]	<wiking>	cpearce: lesse basically i just need to have a seek to the duration time ;)
[10:12]	<cpearce>	yeah, would be easy to add it to oggplayer.
[10:12]	<wiking>	cpearce: oggplay_seek (player, player->duration)
[10:13]	<cpearce>	does oggplay_step_decoding() return E_OGGPLAY_OK when it reaches EOF?
[10:13]	<cpearce>	that is iff it's reached EOF?
[10:14]	<wiking>	cpearce: yep
[10:15]	<wiking>	cpearce: if you are looking at the latest rev of the master branch then oggplay.c:695
[10:15]	<wiking>	cpearce: and yep it does return E_OGGPLAY_OK
[10:24]	<cpearce>	wiking: it looks like we're acutally calling oggplay_step_decoding() after it's returned E_OGGPLAY_OK after decoding the last frame, and then oggplay_step_decoding() is going into an infinite loop. So we are calling liboggplay wrong. oggplay_step_decoding() still shouldn't go into an infinite loop in that case though. It would be nice if the return values were documented...
[10:24]	<wiking>	cpearce: :D yepyep
Attached patch Patch v4Splinter Review
This patch includes the presentation time fix (which has already been upstreamed to liboggplay) and also changes our nsOggStateDecodeMachine::DecodeFrame() function so that we transition to COMPLETED state when liboggplay reaches EOF. We then don't try to call DecodeFrame when we're in COMPLETED state, avoiding going into an infinite in oggplay_step_decoding(). We no longer need the change to oggplay_step_decoding() from the previous patch.
Attachment #394769 - Attachment is obsolete: true
Attachment #394945 - Flags: review?(chris.double)
Attachment #394769 - Flags: review?(chris.double)
Attachment #394945 - Flags: review?(chris.double) → review+
Depends on: CVE-2009-3378
Fixed by bug 512328.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20090831 Minefield/3.7a1pre ID:20090831030956

Chris, pressing the end key while we are at the end triggers play for a split of second. Is it already covered by another bug?
Status: RESOLVED → VERIFIED
Whiteboard: [fixed by bug 512328]
Target Milestone: --- → mozilla1.9.3a1
(In reply to comment #19)
> Chris, pressing the end key while we are at the end triggers play for a split
> of second. Is it already covered by another bug?

I'd not realized that, well spotted! There's not another bug for that, can you file one please? Cheers!
I've backed out bug 512328 to see if that fixes a very common random orange on Linux, so reopening. Until bug 512328 is fixed, this will be unfixed.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(In reply to comment #17)
> Created an attachment (id=394945) [details]
> Patch v4
> 
> This patch includes the presentation time fix (which has already been
> upstreamed to liboggplay) and also changes our
> nsOggStateDecodeMachine::DecodeFrame() function so that we transition to
> COMPLETED state when liboggplay reaches EOF. We then don't try to call
> DecodeFrame when we're in COMPLETED state, avoiding going into an infinite in
> oggplay_step_decoding(). We no longer need the change to
> oggplay_step_decoding() from the previous patch.

I've again debugged this bug with various content and I've found out that for some reason for some ogg content (e.g. http://people.xiph.org/~wiking/Paragliding.Video.wikipedi-1156794597.can.ogg ) oggz_read will never return with 0, i.e. that we've reached the EOF.
Fixed (on Linux at least) by liboggz update (bug 512327), which includes a changeset of a similar vein to the presentation time fix in the liboggplay update. I think we may still need liboggplay's prestime fix, but I'll need to verify that.

Henrik: can you verify this bug is fixed please?
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [fixed by bug 512328] →
Yes, works as expected. Verified with builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20090917 Minefield/3.7a1pre ID:20090917032114

Thanks Chris.
Status: RESOLVED → VERIFIED
Whiteboard:
Depends on: CVE-2009-3377
No longer depends on: CVE-2009-3378
Whiteboard: [Fixed by bug 512327]
You need to log in before you can comment on or make changes to this bug.