463358 - AV: video should seek to keyframe to avoid ugly frames shown first after seeking

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Biju]

Attachment: video4.html

AV: for video tag with start attribute after the SEEK is over the initial image displayed looks ugly.

Comment 1

[Biju]

Attachment: seek_ugly.png

steps:-
1. open attachment 346618 [details] (video4.html)
2. wait the video to download
3. user see seek_ugly.png

expected:
A clear picture at the seeked position

Comment 2

[cajbir (:cajbir)]

The start attribute has been removed, so I think this bug is now invalid.

On the other hand, the reason for it is that seeks don't seek to the nearest keyframe, instead the seek is to the frame for the given timstamp frame, which contains partial data, which could be construed to be a bug. Perhaps change the bug title to reflect that?

Comment 3

[Biju]

Attachment: video4.html

Comment 4

[Biju]

(In reply to comment #2)
> Perhaps change the bug title to reflect that?

done.

Steps:-
1. open attachment 346840 [details], new video4.html
2. click +5 sec two times
3. click play
4. wait the video to download, skip and start playing

Result
video starts but first few frames looks ugly
(if you dont see ugly frames, please try -5 sec once, then play)

alternately step. 
use  https://bugzilla.mozilla.org/attachment.cgi?id=346840#sikpto=10&play
or http://tinyurl.com/6rtv33


PS: On Gecko/20081106 Minefield/3.1b2pre I see things getting better
I have to goto 10 seconds first, then back -5, to see some ugly spots

Comment 5

[cajbir (:cajbir)]

It shouldn't seek to a keyframe, since that's not the requested time period. A keyframe maybe quite a distance from the requested time. What it should probably do is seek to the requested position, find the previous keyframe, decode from there back to the requested position.

Comment 6

[Biju]

(In reply to comment #5)
> It shouldn't seek to a keyframe, since that's not the requested time period. A
> keyframe maybe quite a distance from the requested time. What it should
> probably do is seek to the requested position, find the previous keyframe,
> decode from there back to the requested position.

you are right

Comment 7

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

This seems like it could be hard, and it seems less important than other video/audio blockers.

Comment 8

[Justin Dolske [:Dolske]]

FWIW, kind of problem annoys me a lot on YouTube videos. It seems like their player will only seek to keyframes, but there's no consistency in how often keyframes occur. The end result is the that some videos have surprisingly large areas that are unseekable, so trying to replay some interesting part means you're forced to watch the preceding 5-15 seconds.

Comment 9

[michael dale]

Would like to quickly point out bug 481213  That way when the "avoid ugly frames after seeking bug" is address we can take into account frame accurate seeking use case.

Comment 10

[diaa.abdelmoneim]

Attachment: Seeking back totaly blocks the buffering

When one seeks back in time the video freezes, stops buffering and has this ugly image.

Comment 11

[diaa.abdelmoneim]

Attachment: Another image showing the blocking when seeking in ogg videos

Here is another image showing the seek problem on ogg videos on wikicommons. Happens in firefox 3.1 beta 3 on http://commons.wikimedia.org/wiki/File:Creative_Commons_-_Get_Creative.ogg 

Hope this gets fixed

Comment 12

[Asa Dotzler [:asa]]

related or a new bug?  when we live stream, autoplay picks up the stream at interframes when it should wait to display until it hits the first keyframe.

Comment 13

[cajbir (:cajbir)]

I'm pretty sure this is an icecast thing (referring to comment 12). I see the same with other players.

Comment 14

[Christopher Blizzard (:blizzard)]

Pretty sure that Icecast will happily start you on a frame that's not a keyframe - shouldn't we wait for a keyframe before we start displaying?

Comment 15

[cajbir (:cajbir)]

Raised liboggplay track ticket 473 for comment 14:

https://trac.annodex.net/ticket/473

Comment 16

[Biju]

Assume the keyframes are only send every 30sec.
Then user have to wait watching the throbber cranking.
I think at this time user may be given an option to play
by pressing the play button

so really we may need an event/state telling player finished seeking to the frame we want, but still seeking for keyframe.
then a method playnow(), or displaynow() to start playing if user dont want to wait for keyframe.

Comment 17

[Christopher Blizzard (:blizzard)]

I'm pretty sure that the keyframes are sent more often than that.  Maybe?

Comment 18

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

Keyframe frequency is variable, depends on the video and encoder.

This is perhaps my most annoying bug right now, it would be great if we could fix it. Seeking while paused is almost completely useless as-is. I'm hoping we can still get something in.

Comment 19

[Christopher Blizzard (:blizzard)]

Another option would be to not show a preview of the frames when seeking.

Comment 20

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

That won't help seeking while paused.

Comment 21

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

And it won't help seeking while playing very much either. The final seek will still show garbage for some period of time.

Comment 22

[Christopher Blizzard (:blizzard)]

Yeah, you're right, sorry.

Comment 23

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

Attachment: Patch v1

Implements seeking to keyframe in liboggz/liboggplay, and uses this to seek to the closest key frame and then advance to the desired seek position on seek, to give the appearance of smooth seeking. I also use the "try buffered ranges first" approach to speed up seeking. Along with with media cache, this makes <video> considerably more awesome.

Comment 24

[cajbir (:cajbir)]

Comment on attachment 378319 [details] [diff] [review]
Patch v1

I'd prefer the liboggz/liboggplay stuff to be in a separate patch.

+    rv = oggplay_seek_to_keyframe(mPlayer,
+                                  tracks,
+                                  2,
+                                  ogg_int64_t(aTime * 1000),
+                                  0,
+                                  stream->GetLength());

That '2' should be numTracks I think.

+  serial_nos = oggplay_malloc(sizeof(long)*num_tracks);
+  for (i=0; i<num_tracks; i++) {
+    serial_nos[i] = me->decode_data[tracks[i]]->serialno;
+  }

Need to check for failure of the malloc.

+is_any(ogg_int64_t* a, int n, ogg_int64_t val)
+find(long* a, int n, ogg_int64_t val)
+minimum(ogg_int64_t* a, int n) {

Need comments. What do these do? What are they for? What are the arguments?

+  // Seek backwards 6 pages so that when we're reading backwards we don't
+  // trigger an HTTP byte-range request every time.

Where did the '6 page' value come from? Is it matching something we do in our code? Also this is in generic liboggz code so talking about triggering HTTP byte-range requests doesn't really make sense since liboggz doesn't deal with HTTP. Maybe reference that it triggers it in our seeking code specifically.

+    seek_underrun_pos = MAX(0, offset_at - 6 * CHUNKSIZE);
+    oggz_seek_raw (oggz, seek_underrun_pos, SEEK_SET);
+    oggz_io_read(oggz, &discard, sizeof(discard));
+    oggz_seek_raw (oggz, offset_at, SEEK_SET);

Check return values and handle errors.

+  key_frames = oggz_malloc(sizeof(ogg_int64_t) * num_serialno);
+  memset(key_frames, -1, sizeof(ogg_int64_t) * num_serialno);

Handle failure from oggz_malloc call.

+	  } while (granule_at < 0);
+	  
+	  idx = find(serial_nos, num_serialno, serialno);
+	  if (idx == -1 || key_frames[idx] != -1)
+	    continue;
+	
Indenting

+  // Seek to 100ms before the earliest of all the streams' key frames.

Why 100ms? Maybe document the reason for this value.

Comment 25

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

I'll trust Chris' review on the liboggz stuff. The nsOggDecoder changes look good, great even.

+          if (mState == DECODER_STATE_SHUTDOWN)
+           break;

indent "break";" properly

We should be able to write some good reftests or mochitests for frame-accurate seeking. We'd want to use filters or canvas getImageData() to allow for compression/decompression lossiness.

Comment 26

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

Oh, one more thing.

+        } while (frame && frame->mDecodedFrameTime < seekTime);

I think we might want some kind of epsilon tolerance value here, so that seekTime is 1.0 and mDecodedFrameTime is 0.99999 we use that frame not the next one. We may even want to use mCallbackPeriod/2.

Comment 27

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

Attachment: Patch v2 - media/libogg* changes

Changes to media/liboggz and media/liboggplay split out into another patch at Mr Double's request. Changes to content/ in next patch.

Comment 28

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

Attachment: Patch v2 - content/ changes

Changes to content/. Review comments addressed.

Comment 29

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

Comment on attachment 378458 [details] [diff] [review]
Patch v2 - media/libogg* changes

rubber-stamp=me

Comment 30

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

(In reply to comment #24)
> +  // Seek backwards 6 pages so that when we're reading backwards we don't
> +  // trigger an HTTP byte-range request every time.
> 
> Where did the '6 page' value come from?

I took this chunk out, I'll try to prove to myself that it helps and put it back in a later patch if it helps. I have some other ideas for reducing HTTP transactions on seek, which I'll try as well.

Comment 31

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

Damn, tests hang on Linux. Needs new patch.

Comment 32

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

(In reply to comment #31)
> Damn, tests hang on Linux. Needs new patch.

And on Windows on top of current tip. Damn.

Comment 33

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

Attachment: Patch v3 - media/libogg* changes

Patch v3, as v2, but minor change to oggz_keyframe_seek_set() to handle seeking to 0 properly. Carrying forward r+/sr+.

Comment 34

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

Attachment: Patch v3 - content/ changes

As Patch v2, but fixes nsOggDecodeStateMachine::Seek() so that it treats error codes other than -1 as errors. With this patch, all mochitests pass on Windows and Linux (for me...). Carrying forward r+/sr+ with Roc's permission.

Comment 35

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

http://hg.mozilla.org/mozilla-central/rev/6a6854903509
http://hg.mozilla.org/mozilla-central/rev/96715776a9b8

Comment 36

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

Also pushed patch to fix leak of FrameData objects:
http://hg.mozilla.org/mozilla-central/rev/ad8f7924f18e

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Without this patch on 1.9.1 we crash when seeking around the end of the video. See bug 493936.

Comment 38

[cajbir (:cajbir)]

Also pushed patch to fix another FrameData leak:
http://hg.mozilla.org/mozilla-central/rev/b99d3379a6a8

Comment 39

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2f3019e2d823
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/75a36024cdea
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2895e3e4ac6e
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d4dcb532be6e

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This looks great. Even that we fail sometimes for parts of a second. But it is much better as before and I don't know of those remaining issues could be fixed at all.

Marking verified fixed on trunk and 1.9.1 with builds on all three major platforms.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090521 Minefield/3.6a1pre ID:20090521030951

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090521 Shiretoko/3.5pre ID:20090521135222