566779 - Refactor the way HTML 5 media backends handle find start/end time

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[cajbir (:cajbir)]

This is a follow on from bug 566501 comment 4. 

The Ogg backend needs to seek to the end of the resource to find the end time to compute duration. It also requires seeking to the beginning of the codec data (ie the end of the header data) when looking for the start time. Why I'm unsure since we should be there since we've already read the metadata.

Other backends don't generally need to do this and the 'generic' code in the base class is quite Ogg specific. We should refactor this and move stuff back into the Ogg decoder and remove mDataOffset from VideoData.

Comment 1

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

Since I'm messing with metadata loading in bug 592833, I'll do this too.

Comment 2

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

Attachment: Patch 1: Push down mDataOffset to nsOggReader

Pushes nsBuiltinDecoderReader::mDataOffset down to nsOggReader. It's not used in the other readers, so doesn't make sense for it to exist there.

More patches/parts to follow, once I've cleaned them up.

This patch is based on top of the ms-to-usecs patch from bug 641718.

Comment 3

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

Comment on attachment 523249 [details] [diff] [review]
Patch 1: Push down mDataOffset to nsOggReader

I've landed the "push down mDataOffset" patch in bug 650157, so marking it here as obsolete.

Comment 4

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

Attachment: Patch 1

* We only needed the aOffset parameter in nsBuiltinReader::FindStartTime() for the benefit of nsOggReader, but we don't need that now, so remove the parameter. nsBuiltinReader::FindStartTime() now finds the first frame and start time if you start decoding at the current stream position.
* We overrode nsBuiltinDecoderStateMachine::LoadMetadata() in nsOggDecoderStateMachine just so we can call back up into nsBuiltinDecoderStateMachine::FindEndTime() which calls back down into nsOggReader::FindEndTime(). So I moved the ogg end time calculation down into nsOggReader::ReadMetadata(), and removed nsOggDecoderStateMachine::LoadMetadata().
* I had to add nsBuiltinDecoderStateMachine::SetEndTime(), since for chopped Ogg media we need a way to set the end time without setting the start time (SetDuration() assumes a 0 start time if the start time is unknown).
* Rename nsOggReader::FindStartTime() to RangeStartTime() to distinguish it from nsBuiltinDecoderStateMachine::FindStartTime(). nsOggReader::RangeStartTime() now returns the start time instead of returning the next VideoData and having the the start time as an out param. The VideoData was never used by the callers of nsOggReader::RangeStartTime(), but the start time out param was.
* Rename nsOggReader::FindEndTime(PRInt64 aEndOffset) to RangeEndTime(PRInt64 aEndOffset) to match RangeStartTime().
* nsOggReader::RangeEndTime() (formerly FindEndTime()) used to take an ogg_sync_state parameter. We were passing nsOggReader::mOggState into this, but this would mess up the sync state when I removed the seek(0), so change RangeEndTime() to instead create a new ogg_sync_state for every invocation. Manage the ogg_sync_state with a stack-based class.
* Based on my patch from bug 650994.
* Greenish on Try: http://tbpl.mozilla.org/?tree=Try&rev=b96a33ce33c2

Comment 5

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

Attachment: Patch 2: Don't go to HAVE_ENOUGH_DATA if we don't know whether the resource is valid yet

With my Patch 1 applied, we'd intermittently fail content/media/test/test_decode_error.html because we were switching to readyState HAVE_ENOUGH_DATA when the resource was fully loaded but before we'd loaded metadata and switched to a readyState >= HAVE_METADATA. So we'd claim we'd be able to play through before we knew whether we could play at all.

This patch changes us to not switch to readyState HAVE_ENOUGH_DATA when the resource is fully loaded if we've not yet reached HAVE_METADATA. We'll still end up switching to HAVE_ENOUGH_DATA if we determine we can play the resource with our normal readyState updates.

Comment 6

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

Comment on attachment 530436 [details] [diff] [review]
Patch 2: Don't go to HAVE_ENOUGH_DATA if we don't know whether the resource is valid yet

Review of attachment 530436 [details] [diff] [review]:

Comment 7

[cajbir (:cajbir)]

Comment on attachment 530435 [details] [diff] [review]
Patch 1

Review of attachment 530435 [details] [diff] [review]:

I get a compiler warning in nsOggReader.cpp about AllFrameTimesIncrease being defined but unused. Looks like usage was removed but the static function remains in a previous bug. Could it be cleaned up in this one as well?

::: content/media/ogg/nsOggReader.h
@@ +184,5 @@
+  PRInt64 RangeEndTime(PRInt64 aStartOffset,
+                       PRInt64 aEndOffset,
+                       PRBool aCachedDataOnly);
+
+  PRInt64 RangeStartTime(PRInt64 aOffset);

RangeStartTime needs a comment.

Comment 8

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

Landed, with review comments by doublec addressed.

http://hg.mozilla.org/mozilla-central/rev/a4bf69cf2f78
http://hg.mozilla.org/mozilla-central/rev/615cffa67cfa