566501 - The new Ogg backend has an unnecessary seek during metadata reading

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[cajbir (:cajbir)]

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.

Comment 1

[cajbir (:cajbir)]

Attachment: fix

Comment 2

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

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.

Comment 3

[cajbir (:cajbir)]

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

Comment 4

[cajbir (:cajbir)]

Attachment: fix

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.

Comment 5

[cajbir (:cajbir)]

I raised bug 566779 for the items mentioned in comment 4.

Comment 6

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

Comment on attachment 446126 [details] [diff] [review]
fix

Looks good.

Comment 7

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

http://hg.mozilla.org/mozilla-central/rev/4f43e5fbd0fc