Closed Bug 519897 Opened 10 years ago Closed 9 years ago

Support indexed Ogg files

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: cpearce, Assigned: cpearce)

References

()

Details

Attachments

(1 file, 6 obsolete files)

I've been working on and evangelizing adding a keyframe-index track to the Ogg format. See URL for the thread which I started... This is to speed up seeking over HTTP, as with an index we only need 1 HTTP request to seek. We should be able to play indexed files, and use the index when present to speed up seeking.
OS: Windows NT → All
Hardware: x86 → All
This patch adds support for indexes generated by my indexer with the index as a separate track, and one index per stream, as per http://github.com/cpearce/OggIndex/tree/index-per-stream .
Work In Progress patch which supports one index packet per stream in the skeleton track, as per http://github.com/cpearce/OggIndex/blob/skeleton-index-per-stream/Index-in-skeleton-spec.txt
Patch to support seeking using keyframe indexes from Skeleton 4.0 tracks. Supports index as per: http://github.com/cpearce/OggIndex/blob/master/Skeleton-4.0-Index-Specification.txt

I'll request review once we've got community consensus on the Skeleton 4.0 track.
Attachment #406547 - Attachment is obsolete: true
Attachment #407453 - Attachment is obsolete: true
(In reply to comment #3)
> Created an attachment (id=442310) [details]
> WIP Patch to support index in Skeleton 4.0 track

This also includes changes to the seek code so that if the seek target is less than the "maximum distance between keyframes" ahead of the current playback position, we just decode forwards to the target, rather than doing a potentially expensive bisection search. This would fix bug 539605 and bug 481213.
Blocks: 539605, 481213
This bug is sitting here for long. 
So Chris I have an alternate proposal, 
see Bug 563574
Patch to add support for Skeleton version 4 (http://wiki.xiph.org/Ogg_Skeleton_4) to Firefox trunk.
* This patch is based on the HTMLMediaElement.buffered patches from bug 462957.
* Uses indexes to speed up seeking and to determine the duration of the media. Adds indexed file to seek tests.
* Refactors the Ogg seeking code so that the logic is clearer.
* If the seek target is less than the max keyframe interval ahead of the current playback position (or 2000ms if we don't have video) we just decode ahead instead of doing a bisection search or index assisted seek. This makes small seeks ahead very fast, and fixes the dependent bugs.
* Try server builds here: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/cpearce@mozilla.com-0900961859e6/
* OggIndex nightlies (program to add Skeleton4 with keyframe indexes to an Ogg file) here: http://firefogg.org/nightly/
Attachment #442310 - Attachment is obsolete: true
Attachment #451215 - Flags: review?(kinetik)
I found a bug in my index parser, causing us to reject indexes with unused space, I fixed it in this patch.
Attachment #451215 - Attachment is obsolete: true
Attachment #451456 - Flags: review?(kinetik)
Attachment #451215 - Flags: review?(kinetik)
Matthew: Any chance of reviewing this in the next few days so we can get it landed before beta 1?
beta1 is already done.  I'll try to get to this next week.
Blocks: 576539
Comment on attachment 451456 [details] [diff] [review]
Patch: Support Skeleton v4 with OggIndex (v2)

Patch is bitrotten, I'll rework it.
Attachment #451456 - Flags: review?(kinetik)
The previous patch unbitrotten, and based on top of the patches from bug 576539. This plays skeleton 4.0 files. Test files are at http://pearce.org.nz/video spec is at http://wiki.xiph.org/Ogg_Skeleton_4
Attachment #451456 - Attachment is obsolete: true
Attachment #463993 - Flags: review?(chris.double)
No longer blocks: 576539
Comment on attachment 463993 [details] [diff] [review]
Patch: Support Skeleton v4 with OggIndex (v3)

r+ with following points addressed.

+++ b/content/media/ogg/nsOggCodecState.cpp
+// Minimum length in bytes of a Skeleton 4.0 header packet.
+#define SKELETON_4_0_MIN_HEADER_LEN 80

Please include an URL to the Skeleton 4 spec somewhere in the comments

+// Reads a variable length encoded integer at p. Will not read
+// past aLimit. Returns pointer to character after end of integer.
+static const unsigned char* ReadVariableLengthInt(const unsigned char* p,

+                                                  const unsigned char* aLimit,

+                                                  PRInt64& n)


Some lines here (and further on in the file) have ^M line endings.

+  PRInt64 minPacketSize = INDEX_KEYPOINT_OFFSET + numKeyPoints * MIN_KEY_POINT_SIZE;

This can overflow - numKeyPoints is a PRInt64 read from the file.

+nsresult nsSkeletonState::IndexedSeekTargetForTrack(PRUint32 aSerialno,
+                                                    PRInt64 aTarget,
+                                                    nsKeyPoint& aResult)

More bad line endings in this function.

+    mIndex.Get(aTracks[i], &index);


Line ending.

 PRBool nsSkeletonState::DecodeHeader(ogg_packet* aPacket)
 {
+    PRUint16 verMajor = LEUint16(aPacket->packet + 8);
+    PRUint16 verMinor = LEUint16(aPacket->packet + 10);

Magic numbers 8 and 10 should be defined somewhere.

+    // Initialize the serianlno-to-index map.
+    PRBool init = mIndex.Init();
+    NS_ASSERTION(init, "Failed to initialize Ogg skeleton serialno-to-index map");
+    if (!init) {
+      mActive = PR_FALSE;
+      return mDoneReadingHeaders = PR_TRUE;
+    }

Do we need the assertion if we have the conditional check?

+++ b/content/media/ogg/nsOggReader.cpp
   // Deactivate any non-primary bitstreams.
   for (PRUint32 i = 0; i < bitstreams.Length(); i++) {
     nsOggCodecState* s = bitstreams[i];
-    if (s != mVorbisState && s != mTheoraState) {
+    if (s != mVorbisState && s != mTheoraState && s != mSkeletonState) {
       s->Deactivate();
     }
   }

If there is a Skeleton but no vorbis and theora we should still
deactivate shouldn't we? There's not much that can be done with a
skeleton only file.

+  // Minimize the bisection search space using the known timestamps from the
+  // buffered ranges.
+  ByteRange k = GetSeekRange(aRanges, seekTarget, aStartTime, aEndTime, PR_FALSE);
+  nsresult res = SeekBisection(seekTarget, k, 500);

Where did '500' come from? Might want to make this a defined constant
somewhere. I see this existed in the old code but might be worth
changing anyway.
Attachment #463993 - Flags: review?(chris.double) → review+
Patch with review comments addressed, carrying forward r=doublec.

Can I have approval2.0 for this please? We've been driving indexing of Ogg files in the Ogg community for almost a year now, so it would be a shame if we didn't ship support in Firefox 4.
Attachment #463993 - Attachment is obsolete: true
Attachment #465461 - Flags: review+
Attachment #465461 - Flags: approval2.0?
Seems to still pass on TryServer, we're good to land.
Keywords: checkin-needed
Whiteboard: [needs-landing]
http://hg.mozilla.org/mozilla-central/rev/7eee00899270
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs-landing]
Backed out along with because it landed with bug 548523 which caused test failures. I had to add a new changeset with a commit message with an a=something in it so that I could push to a tree which requires approval, so I added a new changeset which changed some windows line endings to unix with a commit message with the magic words.

http://hg.mozilla.org/mozilla-central/rev/55953a91b4d6
http://hg.mozilla.org/mozilla-central/rev/69022341cd9c
http://hg.mozilla.org/mozilla-central/rev/c19ddab49753
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed again:
http://hg.mozilla.org/mozilla-central/rev/d7d9cf4ab76a
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Depends on: 589039
You need to log in before you can comment on or make changes to this bug.