494305 - Still visual artifacts while seeking in some cases

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

Sometimes when you seek, you still get visual artifacts in video. See testcase. 

When we seek back to the keyframe and then decode forwards, in these failing cases we get a frame from before the key frame time. I'm not sure why this happens, but I'm sure it's related.

Comment 1

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

The problem is that the seek algorithm does a bisection search to find the page which contains the granulepos which corresponds to the seek time, but that timestamp is the time of the last frame that *ends* in that page. It does not imply that the frame *starts* in that page. We need to seek to the page where the frame at that time *starts*. This is particularly important when we're trying to exactly seek to the page which has the key frame.

Video key frames are often large, and span several pages. Such spanning pages are preceded by pages with granulepos=-1. So to be correct when we seek, the seek algorithm needs to change so that after the bisection stops, instead of backtracking while current page granulepos is greater than target, we must backtrack until all active streams have a page with a frame which starts before the seek target. We must take into account pages whose continuation is interleaved by other streams, e.g.:

If we're seeking to time corresponding to gp=z in the pages:

theora page gp = x
theora page gp = -1 (continuation of page at gp=x)
theora page gp = -1 (continuation of page at gp=x)
vorbis page gp = y
theora page gp = z (end of page at gp=x)

We must actually finish our seek at page with gp=x, as the frame that finishes in page with gp=z actually starts in page with gp=x. We can't finish our seek at page with gp=y, even though the time of gp=y must be less than time of gp=z (our seek target) as we don't have a frame for the theora stream which starts before gp=z.

We were still getting artifacts in the cases such as the above, where we were ending our seek-to-keyframe at pages which were the end of key frames, not the start.

I have patches to fix this, will attach.

Comment 2

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

Attachment: Patch v1 part 1 of 2

Fixes liboggz's oggz_set_read_page() and oggz_get_next_page(). These were sometimes returning incorrect results. They were often adding the length of the following page to the offset of the current page to report the offset of the following page. You can use the following function to verify that oggz_set_read_page() and oggz_get_next_page() are returning offsets to page starts:

// Returns 1 on success, 0 on failure.
static int
verify_page_start(oggz_off_t offset)
{
  size_t length;
  char buf[4];
  int x = 0;
  FILE* f = fopen(PATH_TO_FILE_READING, "rb");
  if (!f)
    return 0;
  fseek(f, 0L, SEEK_END);
  length = ftell(f);
  if (offset == length || offset == 0) {
    fclose(f);
    return 1;
  }
  fseek(f, offset, SEEK_SET);  
  x = fread(buf, 1, 4, f);
  fclose(f);
  if (x != 4)
    return 0;
  x = *(int*)buf == *(int*)"OggS";
  if (!x)
    printf("verify_page_start(%d) FAILED!\n", offset);
  return x;
}

Comment 3

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

Attachment: Patch v1 part 2 of 2

This fixes the seek algorithm as described in comment #1.

I'll try and get Conrad to look at the changes to liboggz and make sure they're sane.

Comment 4

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

Attachment: Patch v2

I asked Conrad, and it seems if we're a bit cleverer, we don't need to do so much work to do this. I'll wait until I hear back from Conrad on this, but here's a simpler fix.

Comment 5

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

Attachment: Patch v2 part 2

We still need to do this second change to liboggz too. I had to update it to sit ontop of new "patch v2 part 1", and had to fix a test too.

Comment 6

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

Comment on attachment 379638 [details] [diff] [review]
Patch v2

I've moved "Patch v2" over to bug 495366 so that we can land it separately, and block191 if need be.

Comment 7

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

Chris, what is left to do on this bug? See testcase looks good for me.

Comment 8

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

Comment on attachment 380037 [details] [diff] [review]
Patch v2 part 2

(In reply to comment #7)
> Chris, what is left to do on this bug? See testcase looks good for me.

I was waiting for Conrad to get back to me about this patch.

Conrad: Can you review this patch please?

Comment 9

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

Do we want this in 1.9.1 or 1.9.2? We'd need some QA guys hammering on seeking if we were to try for 1.9.1, as if it causes regressions, it could break seeking entirely... Not that I think that's likely of course. ;)

Comment 10

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

Chris, you can create a tryserver build for us so we can't take care of it whether it will be wanted for 1.9.1 or not.

Comment 11

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

Builds here:

http://build.mozilla.org/tryserver-builds/[e-mail]-cpearceSeek

I need you to pound on seeking, to make sure that there aren't any visual artifacts when you seek, and to ensure that I've not otherwise broken seeking! ;) Cheers!

Comment 12

[Aaron Train [:aaronmt]]

(In reply to comment #11)
> Builds here:
> 
> http://build.mozilla.org/tryserver-builds/[e-mail]-cpearceSeek
> 
> I need you to pound on seeking, to make sure that there aren't any visual
> artifacts when you seek, and to ensure that I've not otherwise broken seeking!
> ;) Cheers!

Chris, 

Would love to pound on seeking, to make sure that there aren't any visual artifacts when seeking but I am confused about what I am looking for in the aforementioned test case. 

Trying out your TS build, I see the same seek (2 frames (gentlemen looking down/gentlemen looking up @ 0:10), as I do in latest 1.9.1 branch and trunk and b99

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b99) Gecko/20090604 Firefox/3.5b99

Any idea?

Comment 13

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

Chris, I can see something strange with http://tinyvid.tv/show/1xqns9wvihbql. Whether I seek forward or backward it takes a long time until the video is played again. Do we somehow destroy our buffered data?

Comment 14

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

So you're saying that's a regression caused by the patch here?

Comment 15

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

Doesn't look so. Aaron can reproduce it with latest 1.9.1 build too. We have filed bug 497182 on that.

Comment 16

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

Comment on attachment 380037 [details] [diff] [review]
Patch v2 part 2

(In reply to comment #12)
> Trying out your TS build, I see the same seek (2 frames (gentlemen looking
> down/gentlemen looking up @ 0:10), as I do in latest 1.9.1 branch and trunk and
> b99

Looks like something's changed since I wrote my patch, and the current builds work and fail in different places now. My patch also doesn't fix the problem any more. :(

I can reproduce seeks with visual artifacts, with or without my patch, especially with this video: http://media.tinyvid.tv/18poav133o32h.ogg e.g. seek to 508s, and you'll see visual artifacts. I'd not tested seeks in that video recently, the videos I was testing on all worked...

Thanks for picking this up guys! Back to the drawing board...

Comment 17

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

(In reply to comment #13)
> Chris, I can see something strange with http://tinyvid.tv/show/1xqns9wvihbql.
> Whether I seek forward or backward it takes a long time until the video is
> played again. Do we somehow destroy our buffered data?

See bug 497182 comment 5.

Comment 18

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

Fixed by bug 501031.