Last Comment Bug 650994 - Disentangle Ogg timestamping from decode
: Disentangle Ogg timestamping from decode
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
:
Mentors:
Depends on:
Blocks: 613699 631058 654371
  Show dependency treegraph
 
Reported: 2011-04-18 17:11 PDT by PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
Modified: 2011-05-07 23:33 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (88.60 KB, patch)
2011-04-18 17:14 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch v2 (88.58 KB, patch)
2011-04-18 22:05 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
cajbir.bugzilla: review+
Details | Diff | Splinter Review
Patch v3 (90.33 KB, patch)
2011-05-02 15:01 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
cpearce: review+
Details | Diff | Splinter Review

Description PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2011-04-18 17:11:40 PDT
If we disentangle our Theora/Vorbis timestampping code from our Ogg decode process, and timestamp ogg_packets at demux time instead of timestampping decoded frames/samples, we'd not need to decode and store all the Theora/Vorbis frames/samples until we encounter a page with a non -1 granulepos in order to determine timestamps for those frames/samples.

This means we can refactor nsBuiltinDecoderReader::FindStartTime() and friends to not need to decode and store any frames/samples in order to determine the media start time, which will make the mobile guys happy, a la bug 631058.
Comment 1 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2011-04-18 17:14:33 PDT
Created attachment 526878 [details] [diff] [review]
Patch v1

Timestamp ogg packets at demux time, decode packets less "burstily".

* Change content/base/test/file_mozfiledataurl_inner.html to rely on "loadeddata" event, rather than "canplay" event, as we can fire multiple "canplay" events, and the test assumes that we only get one. This was failing since we load ogg metadata much quicker now, but without loading as much data. When we finish downloading the entire resource, we'll switch to readyState HAVE_ENOUGH_DATA, but shortly after we'll switch back to readyState HAVE_CURRENT_DATA (and fire "canplay" again) because we may not have enough data decoded to be in readyState > HAVE_CURRENT_DATA. I imagine this issue also exists with WebM, which also doesn't need to decode a lot of data when reading metadata.
* Add "aActive" parameter to nsOggCodecState constructor, so we can set unknown ogg bitstreams to inactive and finished-reading-headers by default.
* Separate ogg packet demux and timestamping from decode. Now all the Vorbis and Theora timestamping logic has been pushed down into nsOggCodecState, and operates on packets rather than decoded frames/samples. This means when the nsOggReader requests a packet from an nsOggCodecState, it's guaranteed to have a non -1 granulepos (timestamp). We no longer have to decode a whole page of Vorbis, or several pages of Theora in order to timestamp the samples/frames.
* Each nsOggCodecState stores queues of untimestampped and timestampped packets. When packets are demuxed from pages, they're stored in the untimestampped queue, and when we encounter a packet with a non -1 granulepos, we'll use that to infer the timestamp of the packets in the untimestampped queue, and move the timestampped packets over to the timestampped packets queue, which is what we return when nsOggReader asks for a packet.
* The Theora timestamping code in nsTheoraState::ReconstructTheoraGranulepos() is the same as it was when it was in nsOggReader, except that we always capture granulepos, we never count upwards from a known granulepos (bug 613699). nsVorbisState::ReconstructVorbisGranulepos() predicts the number of samples which will be decoded out of each packet, and uses that to calculate the granulepos. I added validation code (which is disabled by default) which verifies that the prediction is correct (and it is correct!).
* Add a virtual IsHeader(ogg_packet*) method to nsOggCodecState. This means we can distinguish non-data packets when we're decoding, and not try to decode them as data. This enables us to remove mDataOffset. This means we handle Oggs with header packets incorrectly muxed more robustly.
* Changed nsOggReader::ReadMetadata() so that it's less loopy. It reads the BOS pages, then reads and decodes header packets on the active streams until they're all done reading headers. This is a bit cleaner, and also means we handle Oggs with header packets incorrectly muxed more robustly.
Comment 2 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2011-04-18 17:15:14 PDT
(In reply to comment #1)
> Created attachment 526878 [details] [diff] [review]
> Patch v1

This was greenish on Try:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=fa9a37476c9f
Comment 3 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2011-04-18 22:05:29 PDT
Created attachment 526932 [details] [diff] [review]
Patch v2

I discovered that the keyframe skipping logic was wrong with my patch v1, we'd end up decoding & displaying interframes after we encountered a keyframe which had time < currentTime. Fixed in this patch.
Comment 4 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2011-04-28 14:57:30 PDT
(In reply to comment #3)
> Created attachment 526932 [details] [diff] [review]
> Patch v2

doublec: Any idea when you'll have time to do the review for this patch?
Comment 5 cajbir (:cajbir) 2011-04-28 22:38:52 PDT
Comment on attachment 526932 [details] [diff] [review]
Patch v2

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

r+ with the mostly minor changes below.

::: content/media/ogg/nsOggCodecState.cpp
@@ +101,5 @@
+{
+  for (PRUint32 i = 0; i < mUnstamped.Length(); ++i) {
+    ogg_packet* packet = mUnstamped[i];
+    if (packet)
+      delete packet->packet;

This should be the array form of operator delete[]. Maybe share ReleasePacket in nsOggReader.cpp so the code doesn't have to be duplicated.

@@ +138,5 @@
+    "Must have recorded packet samples");
+#endif
+}
+
+ogg_packet* Clone(ogg_packet* aPacket) {

This should be a static function, or in an anonymous namespace.

@@ +160,5 @@
+
+nsresult nsOggCodecState::PageIn(ogg_page* aPage) {
+  if (!mActive)
+    return NS_OK;
+  NS_ASSERTION(ogg_page_serialno(aPage) == mSerial, "Page must be for this stream!");

This gives a 'comparison between signed and unsigned' warning on linux

@@ +170,5 @@
+    r = ogg_stream_packetout(&mState, &packet);
+    if (r == 1) {
+      mPackets.Append(Clone(&packet));
+    }
+  } while (r != 0);

Apparently a return value of 0 from ogg_stream_packetout is used for both 'need more data' and 'unrecoverable error'. Is there a way to different the second case to return a failure?

@@ +187,5 @@
+    r = ogg_stream_packetout(&mState, &packet);
+    if (r == 1) {
+      if (IsHeader(&packet)) {
+        // Header packets go straight into the packet queue.
+        mPackets.Append(Clone(&packet));

You might want to lift the Clone call out of the 'if' since both branches call it.

@@ +360,5 @@
+nsTheoraState::PageIn(ogg_page* aPage)
+{
+  if (!mActive)
+    return NS_OK;
+  NS_ASSERTION(ogg_page_serialno(aPage) == mSerial, "Page must be for this stream!");

This gives a 'comparison between signed and unsigned warning on linux.

@@ +605,5 @@
+nsVorbisState::PageIn(ogg_page* aPage)
+{
+  if (!mActive)
+    return NS_OK;
+  NS_ASSERTION(ogg_page_serialno(aPage) == mSerial, "Page must be for this stream!");

This gives a 'comparison between signed and unsigned warning on linux.

@@ +647,5 @@
+  NS_ASSERTION(last->e_o_s || last->granulepos >= 0,
+    "Must know last granulepos!");
+  if (mUnstamped.Length() == 1) {
+    ogg_packet* packet = mUnstamped[0];
+    long blockSize = vorbis_packet_blocksize(&mInfo, packet);

check and handle error values from vorbis_packet_blocksize

@@ +666,5 @@
+    ogg_packet* prev = mUnstamped[i-1];
+    ogg_int64_t granulepos = packet->granulepos;
+    NS_ASSERTION(granulepos != -1, "Must know granulepos!");
+    long prevBlockSize = vorbis_packet_blocksize(&mInfo, prev);
+    long blockSize = vorbis_packet_blocksize(&mInfo, packet);

check and handle errors from vorbis_packet_blocksize

::: content/media/ogg/nsOggCodecState.h
@@ +55,5 @@
+// of Vorbis samples in each packet correctly.
+//#define VALIDATE_VORBIS_SAMPLE_CALCULATION
+#ifdef  VALIDATE_VORBIS_SAMPLE_CALCULATION
+#include <map>
+using namespace std;

Don't use 'using namespace' in header files. Either use std::map or typedef it.

@@ +247,5 @@
+#ifdef VALIDATE_VORBIS_SAMPLE_CALCULATION
+  // When validating that we've correctly predicted Vorbis packets' number
+  // of samples, we store each packet's predicted number of samples in this
+  // map, and verify we decode the predicted number of samples.
+  map<ogg_packet*, long> mVorbisPacketSamples;

I welcome our new std collection class using overlords.

::: content/media/ogg/nsOggReader.cpp
@@ +97,5 @@
 // Chunk size to read when reading Ogg files. Average Ogg page length
 // is about 4300 bytes, so we read the file in chunks larger than that.
 static const int PAGE_STEP = 8192;
 
+void ReleasePacket(ogg_packet* aPacket) {

This function should be static or in an anonymous namespace, or shared so it can be re-used in nsOggCodecState.cpp.
Comment 6 Timothy B. Terriberry (:derf) 2011-04-29 07:19:49 PDT
(In reply to comment #5)
> Apparently a return value of 0 from ogg_stream_packetout is used for both 'need
> more data' and 'unrecoverable error'. Is there a way to different the second
> case to return a failure?

The only "unrecoverable internal errors" it checks are passing a NULL ogg_stream_state and memory allocation failure (we didn't used to check for the latter, which is why this got retconned into the 0 return value; -1 was already taken for sync gaps). You can do the same tests by calling ogg_stream_check() after ogg_stream_packetout returns 0 if you have libogg 1.1.4 or later.
Comment 7 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2011-05-02 15:01:39 PDT
Created attachment 529580 [details] [diff] [review]
Patch v3

Patch with review comments addressed. Carrying forward r=doublec.

Mostly greenish on Try, but still has an assertion failure on crashtest, so I will fix that before landing.
http://tbpl.mozilla.org/?tree=Try&rev=0d3b09d89c22
Comment 8 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2011-05-07 23:33:26 PDT
http://hg.mozilla.org/mozilla-central/rev/a8f07cad55e2

Note You need to log in before you can comment on or make changes to this bug.