Closed Bug 629618 Opened 9 years ago Closed 9 years ago

Artifacts when seeking ogg video

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: cpearce, Assigned: cpearce)

References

()

Details

Attachments

(1 file, 1 obsolete file)

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
Attached patch Patch v1 (obsolete) — Splinter Review
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
Assignee: nobody → chris
Status: NEW → ASSIGNED
Attachment #510766 - Flags: review?(chris.double)
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.
Attachment #510766 - Flags: review?(chris.double) → review+
(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!
Attached patch Patch v2Splinter Review
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.
Attachment #510766 - Attachment is obsolete: true
Attachment #512391 - Flags: review+
Attachment #512391 - Flags: approval2.0?
http://hg.mozilla.org/mozilla-central/rev/301a3058c82a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Cannot reproduce with Firefox 4.0b12pre 20110218.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.