519897 - Support indexed Ogg files

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

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.

Comment 1

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

Attachment: WIP patch, support one index per stream on a separate track

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 .

Comment 2

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

Attachment: WIP patch to support index in skeleton track

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

Comment 3

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

Attachment: WIP Patch to support index in Skeleton 4.0 track

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.

Comment 4

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

(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.

Comment 5

[Biju]

This bug is sitting here for long. 
So Chris I have an alternate proposal, 
see Bug 563574

Comment 6

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

Attachment: Patch: Support Skeleton v4 with OggIndex

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/[e-mail]-0900961859e6/
* OggIndex nightlies (program to add Skeleton4 with keyframe indexes to an Ogg file) here: http://firefogg.org/nightly/

Comment 7

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

Index test at http://pearce.org.nz/video/indexed-seek-demo.html and other indexed ogg files at: http://pearce.org.nz/video/

Comment 8

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

Attachment: Patch: Support Skeleton v4 with OggIndex (v2)

I found a bug in my index parser, causing us to reject indexes with unused space, I fixed it in this patch.

Comment 9

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

Matthew: Any chance of reviewing this in the next few days so we can get it landed before beta 1?

Comment 10

[Matthew Gregan [:kinetik]]

beta1 is already done.  I'll try to get to this next week.

Comment 11

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

Comment on attachment 451456 [details] [diff] [review]
Patch: Support Skeleton v4 with OggIndex (v2)

Patch is bitrotten, I'll rework it.

Comment 12

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

Attachment: Patch: Support Skeleton v4 with OggIndex (v3)

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

Comment 13

[cajbir (:cajbir)]

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.

Comment 14

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

Attachment: Patch: Support Ogg Skeleton 4.0

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.

Comment 15

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

Seems to still pass on TryServer, we're good to land.

Comment 16

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

http://hg.mozilla.org/mozilla-central/rev/7eee00899270

Comment 17

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

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

Comment 18

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

Pushed again:
http://hg.mozilla.org/mozilla-central/rev/d7d9cf4ab76a