Still visual artifacts while seeking in some cases

RESOLVED FIXED

Status

()

Core
Audio/Video
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

Trunk
Points:
---
Bug Flags:
wanted1.9.2 +
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 obsolete attachments)

(Assignee)

Description

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

Updated

8 years ago
Assignee: nobody → chris
(Assignee)

Comment 1

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

Comment 2

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

Comment 3

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

Comment 4

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

Comment 5

8 years ago
Created attachment 380037 [details] [diff] [review]
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.
(Assignee)

Updated

8 years ago
Depends on: 495366
(Assignee)

Comment 6

8 years ago
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.
Attachment #379638 - Attachment is obsolete: true
Hardware: x86 → All
Chris, what is left to do on this bug? See testcase looks good for me.
(Assignee)

Comment 8

8 years ago
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?
Attachment #380037 - Flags: review?(conrad)
(Assignee)

Comment 9

8 years ago
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. ;)
Flags: wanted1.9.2?
Flags: wanted1.9.1?
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.
(Assignee)

Comment 11

8 years ago
Builds here:

http://build.mozilla.org/tryserver-builds/cpearce@mozilla.com-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!
(In reply to comment #11)
> Builds here:
> 
> http://build.mozilla.org/tryserver-builds/cpearce@mozilla.com-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?
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?
Flags: wanted1.9.1? → wanted1.9.1+
So you're saying that's a regression caused by the patch here?
Doesn't look so. Aaron can reproduce it with latest 1.9.1 build too. We have filed bug 497182 on that.
(Assignee)

Comment 16

8 years ago
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...
Attachment #380037 - Attachment is obsolete: true
Attachment #380037 - Flags: review?(conrad)
(Assignee)

Comment 17

8 years ago
(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.
Flags: wanted1.9.2? → wanted1.9.2+
(Assignee)

Comment 18

8 years ago
Fixed by bug 501031.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.