629618 - Artifacts when seeking ogg video

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

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

There are artifacts when seeking in the Ogg video at URL above. The keyframes are up to 6 seconds apart, maybe that's a contributing factor. It seeks fine in FF3.6

Comment 1

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

Attachment: Patch v1

When the Ogg bisection code can't read enough data at it's bisection point to determine the time there, it "backs off" exponentially from its guess position until it can. The main problem here is that the exponential backoff wasn't being reset correctly when we determined that we'd backed off enough and didn't need to backoff further. Then if we decided we needed to backoff in a subsequent bisection, we'd backoff too much.

I also fixed some incorrect assignments to nsOggReader::mPageOffset, and added comments to try to make it clearer how the code works.

Green on TryServer: http://tbpl.mozilla.org/?tree=MozillaTry&rev=f1e8bf3e125d

Comment 2

[cajbir (:cajbir)]

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

Lines 102, 197 and 219 of the patch have trailing whitespace.

       guess = startOffset + startLength +
-              (ogg_int64_t)((double)interval * target) - backoff;
+              static_cast<PRInt64>((double)interval * target);

Why the change from ogg_int64_t to PRInt64? The type of all the variables involved seem to be ogg_int64_t.

Comment 3

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

(In reply to comment #2)
> Comment on attachment 510766 [details] [diff] [review]
> Patch v1
> 
> Lines 102, 197 and 219 of the patch have trailing whitespace.
> 
>        guess = startOffset + startLength +
> -              (ogg_int64_t)((double)interval * target) - backoff;
> +              static_cast<PRInt64>((double)interval * target);
> 
> Why the change from ogg_int64_t to PRInt64? The type of all the variables
> involved seem to be ogg_int64_t.

To save me typing 4 characters? ;) No real reason, I'll change it back.

Thanks for the review!

Comment 4

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

Attachment: Patch v2

With whitespace and type change as per review comments. Carrying forward r=doublec.

Can I have approval2.0 to land this patch? It fixes a regression in Ogg seeking introduced in the new decoder backend.

Comment 5

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

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

Test?

Comment 6

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

I guess a test would be hard.

Comment 7

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

http://hg.mozilla.org/mozilla-central/rev/301a3058c82a

Comment 8

[u279076]

Cannot reproduce with Firefox 4.0b12pre 20110218.