The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
Audio/Video
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: Biju, Assigned: cpearce)

Tracking

({polish, verified1.9.1})

Trunk
mozilla1.9.2a1
polish, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 -
wanted1.9.1 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 4 obsolete attachments)

(Reporter)

Description

9 years ago
Created attachment 346618 [details]
video4.html

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

Comment 1

9 years ago
Created attachment 346619 [details]
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

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

Comment 3

9 years ago
Created attachment 346840 [details]
video4.html
Attachment #346618 - Attachment is obsolete: true
(Reporter)

Comment 4

9 years ago
(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
Summary: AV: for video tag with start attribute image displayed ugly → AV: video should seek to keyframe to avoid ugly frames shown first after seeking
Depends on: 449159

Comment 5

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

Comment 6

8 years ago
(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

Updated

8 years ago
Flags: blocking1.9.1?
This seems like it could be hard, and it seems less important than other video/audio blockers.
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
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

8 years ago
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.

Updated

8 years ago
Blocks: 481213

Comment 10

8 years ago
Created attachment 367383 [details]
Seeking back totaly blocks the buffering

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

Comment 11

8 years ago
Created attachment 367384 [details]
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
Keywords: polish

Comment 12

8 years ago
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

8 years ago
I'm pretty sure this is an icecast thing (referring to comment 12). I see the same with other players.
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

8 years ago
Raised liboggplay track ticket 473 for comment 14:

https://trac.annodex.net/ticket/473
(Reporter)

Comment 16

8 years ago
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.
I'm pretty sure that the keyframes are sent more often than that.  Maybe?
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.
Assignee: nobody → chris
Another option would be to not show a preview of the frames when seeking.
That won't help seeking while paused.
And it won't help seeking while playing very much either. The final seek will still show garbage for some period of time.
Yeah, you're right, sorry.
(Assignee)

Comment 23

8 years ago
Created attachment 378319 [details] [diff] [review]
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.
Attachment #378319 - Flags: superreview?(roc)
Attachment #378319 - Flags: review?(chris.double)

Comment 24

8 years ago
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.
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.
Flags: in-testsuite?
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.
(Assignee)

Comment 27

8 years ago
Created attachment 378458 [details] [diff] [review]
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.
Attachment #378319 - Attachment is obsolete: true
Attachment #378458 - Flags: superreview?(roc)
Attachment #378458 - Flags: review?(chris.double)
Attachment #378319 - Flags: superreview?(roc)
Attachment #378319 - Flags: review?(chris.double)
(Assignee)

Comment 28

8 years ago
Created attachment 378459 [details] [diff] [review]
Patch v2 - content/ changes

Changes to content/. Review comments addressed.
Attachment #378459 - Flags: superreview?(roc)
Attachment #378459 - Flags: review?(chris.double)
Attachment #378459 - Flags: superreview?(roc) → superreview+
Attachment #378458 - Flags: superreview?(roc) → superreview+
Comment on attachment 378458 [details] [diff] [review]
Patch v2 - media/libogg* changes

rubber-stamp=me

Updated

8 years ago
Attachment #378458 - Flags: review?(chris.double) → review+

Updated

8 years ago
Attachment #378459 - Flags: review?(chris.double) → review+
(Assignee)

Comment 30

8 years ago
(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.
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [needs landing]
(Assignee)

Comment 31

8 years ago
Damn, tests hang on Linux. Needs new patch.
Keywords: checkin-needed
Whiteboard: [needs landing]
(Assignee)

Comment 32

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

And on Windows on top of current tip. Damn.
(Assignee)

Comment 33

8 years ago
Created attachment 378516 [details] [diff] [review]
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+.
Attachment #378458 - Attachment is obsolete: true
Attachment #378516 - Flags: superreview+
Attachment #378516 - Flags: review+
(Assignee)

Comment 34

8 years ago
Created attachment 378517 [details] [diff] [review]
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.
Attachment #378459 - Attachment is obsolete: true
Attachment #378517 - Flags: superreview+
Attachment #378517 - Flags: review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/6a6854903509
http://hg.mozilla.org/mozilla-central/rev/96715776a9b8
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Attachment #378516 - Flags: approval1.9.1+
Attachment #378517 - Flags: approval1.9.1+
Also pushed patch to fix leak of FrameData objects:
http://hg.mozilla.org/mozilla-central/rev/ad8f7924f18e

Updated

8 years ago
Blocks: 493936
Without this patch on 1.9.1 we crash when seeking around the end of the video. See bug 493936.
Target Milestone: --- → mozilla1.9.2a1

Comment 38

8 years ago
Also pushed patch to fix another FrameData leak:
http://hg.mozilla.org/mozilla-central/rev/b99d3379a6a8
Keywords: checkin-needed
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
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.