Closed Bug 566501 Opened 14 years ago Closed 14 years ago

The new Ogg backend has an unnecessary seek during metadata reading

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: cajbir, Assigned: cajbir)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached 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.
Odd that all ours tests pass with this patch applied. Are out tests not testing seeking correctly?
Attached 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)
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+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/4f43e5fbd0fc
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: