The new Ogg backend has an unnecessary seek during metadata reading

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: cajbir, Assigned: cajbir)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

fix
5.71 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
Assignee

Description

9 years ago
The new Ogg backend has includes code from the original backend that was needed to workaround a liboggplay bug. This causes a seek after metadata reading. This should be removed.
Assignee

Comment 1

9 years ago
Posted patch fix (obsolete) — Splinter Review
Attachment #445879 - Flags: review?(chris)
Comment on attachment 445879 [details] [diff] [review]
fix

This actually breaks seeking over HTTP. We use nsBuiltinDecoderReader::FindStartTime() inside nsBuiltinDecoderReader::GetBufferedBytes() in order to determine what time ranges are buffered. If we remove that seek, FindStartTime() returns incorrect values, and our seek tries to look for the seek target inside the wrong range, and gets confused. Sorry, when we were discussing this I totally forgot that we reused FindStartTime() this way!

Perhaps we should instead move the seek out of nsBuiltinDecoderReader::FindStartTime() and into nsBuiltinDecoderReader::GetBufferedBytes(), and remove the aOffset parameter from nsBuiltinDecoderReader::FindStartTime()? Then we wouldn't be doing the seek after loading metadata, and we'd still be able to seek in buffered ranges.
Assignee

Comment 3

9 years ago
Odd that all ours tests pass with this patch applied. Are out tests not testing seeking correctly?
Assignee

Comment 4

9 years ago
Posted patch fixSplinter Review
This moves the seek from the nsBuiltin* start and end finders into the Ogg backend itself. I've done the minimum needed to move it out of the base classes and into Ogg.

Longer term I think we need to refactor the way this is done. The FindStart/End and GetBufferedBytes stuff should be moved into the Ogg backend. We can then get rid of mDataOffset from the VideoData structure. I'll raise a bug for this.
Attachment #445879 - Attachment is obsolete: true
Attachment #446126 - Flags: review?(chris)
Attachment #445879 - Flags: review?(chris)
Assignee

Comment 5

9 years ago
I raised bug 566779 for the items mentioned in comment 4.
Comment on attachment 446126 [details] [diff] [review]
fix

Looks good.
Attachment #446126 - Flags: review?(chris) → review+
Assignee

Updated

9 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/4f43e5fbd0fc
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.