Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla6



7 years ago
6 years ago


(Reporter: cajbir, Assigned: cpearce)



Firefox Tracking Flags

(Not tracked)



(2 attachments, 1 obsolete attachment)



7 years ago
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

6 years ago
Since I'm messing with metadata loading in bug 592833, I'll do this too.
Assignee: nobody → chris

Comment 2

6 years ago
Created attachment 523249 [details] [diff] [review]
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.
Attachment #523249 - Flags: review?(chris.double)


6 years ago
Attachment #523249 - Flags: review?(chris.double) → review+


6 years ago
Depends on: 650157

Comment 3

6 years ago
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.
Attachment #523249 - Attachment is obsolete: true

Comment 4

6 years ago
Created attachment 530435 [details] [diff] [review]
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:
Attachment #530435 - Flags: review?(chris.double)

Comment 5

6 years ago
Created 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

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.
Attachment #530436 - Flags: review?(roc)
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]:
Attachment #530436 - Flags: review?(roc) → review+

Comment 7

6 years ago
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.
Attachment #530435 - Flags: review?(chris.double) → review+

Comment 8

6 years ago
Landed, with review comments by doublec addressed.
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6


6 years ago
Blocks: 608634
No longer blocks: 608634
You need to log in before you can comment on or make changes to this bug.