commit aef112a9847383aa841bd6d602ccee4718cb631f
author Marco Nelissen <marcone@google.com> Mon Sep 21 16:03:59 2020 -0700
committer Marco Nelissen <marcone@google.com> Wed Sep 23 21:50:29 2020 +0000
tree f245aab8f32fb75e5032255bcf68045c6f4b4632
parent c7f2011ccbaede45c36119caff71928ef423ff2e

Fix Ogg extractor and Vorbis decoder

Ignore empty packets, and only set the "valid page samples" property
for the last page.

Bug: 167535700
Test: CTS, test file from bug
Change-Id: Ic82b476996dabbede2767dc067a3162658d2e34c

media/codec2/components/vorbis/C2SoftVorbisDec.cpp

diff --git a/media/codec2/components/vorbis/C2SoftVorbisDec.cpp b/media/codec2/components/vorbis/C2SoftVorbisDec.cpp
index a8b5377..d3b6e31 100644
--- a/media/codec2/components/vorbis/C2SoftVorbisDec.cpp
+++ b/media/codec2/components/vorbis/C2SoftVorbisDec.cpp
@@ -359,6 +359,10 @@
     }
     memcpy(&numPageFrames, data + inSize - sizeof(numPageFrames), sizeof(numPageFrames));
     inSize -= sizeof(numPageFrames);
+    if (inSize == 0) {
+        // empty buffer, ignore
+        return;
+    }
     if (numPageFrames >= 0) {
         mNumFramesLeftOnPage = numPageFrames;
     }
@@ -409,7 +413,7 @@
                 mState,  reinterpret_cast<int16_t *> (wView.data()),
                 kMaxNumSamplesPerChannel);
         if (numFrames < 0) {
-            ALOGD("vorbis_dsp_pcmout returned %d", numFrames);
+            ALOGD("vorbis_dsp_pcmout returned %d frames", numFrames);
             numFrames = 0;
         }
     }

media/extractors/ogg/OggExtractor.cpp

diff --git a/media/extractors/ogg/OggExtractor.cpp b/media/extractors/ogg/OggExtractor.cpp
index 828bcd6..ab2b553 100644
--- a/media/extractors/ogg/OggExtractor.cpp
+++ b/media/extractors/ogg/OggExtractor.cpp
@@ -43,6 +43,9 @@
     long vorbis_packet_blocksize(vorbis_info *vi,ogg_packet *op);
 }
 
+static constexpr int OGG_PAGE_FLAG_CONTINUED_PACKET = 1;
+static constexpr int OGG_PAGE_FLAG_END_OF_STREAM = 4;
+
 namespace android {
 
 struct OggSource : public MediaTrackHelper {
@@ -297,7 +300,8 @@
     AMediaFormat_setInt32(meta, AMEDIAFORMAT_KEY_IS_SYNC_FRAME, 1);
 
     *out = packet;
-    ALOGV("returning buffer %p", packet);
+    ALOGV("returning buffer %p, size %zu, length %zu",
+          packet, packet->size(), packet->range_length());
     return AMEDIA_OK;
 }
 
@@ -358,10 +362,10 @@
 
         if (!memcmp(signature, "OggS", 4)) {
             if (*pageOffset > startOffset) {
-                ALOGV("skipped %lld bytes of junk to reach next frame",
-                     (long long)(*pageOffset - startOffset));
+                ALOGV("skipped %lld bytes of junk at %lld to reach next frame",
+                     (long long)(*pageOffset - startOffset), (long long)(startOffset));
             }
-
+            ALOGV("found frame at %lld", (long long)(*pageOffset));
             return OK;
         }
 
@@ -629,7 +633,8 @@
     // Calculate timestamps by accumulating durations starting from the first sample of a page;
     // We assume that we only seek to page boundaries.
     AMediaFormat *meta = (*out)->meta_data();
-    if (AMediaFormat_getInt32(meta, AMEDIAFORMAT_KEY_VALID_SAMPLES, &currentPageSamples)) {
+    if (AMediaFormat_getInt32(meta, AMEDIAFORMAT_KEY_VALID_SAMPLES, &currentPageSamples) &&
+            (mCurrentPage.mFlags & OGG_PAGE_FLAG_END_OF_STREAM)) {
         // first packet in page
         if (mOffset == mFirstDataOffset) {
             currentPageSamples -= mStartGranulePosition;
@@ -812,6 +817,7 @@
             }
             buffer = tmp;
 
+            ALOGV("reading %zu bytes @ %zu", packetSize, size_t(dataOffset));
             ssize_t n = mSource->readAt(
                     dataOffset,
                     (uint8_t *)buffer->data() + buffer->range_length(),
@@ -830,8 +836,9 @@
 
             if (gotFullPacket) {
                 // We've just read the entire packet.
+                ALOGV("got full packet, size %zu", fullSize);
 
-                if (mFirstPacketInPage) {
+                if (mFirstPacketInPage && (mCurrentPage.mFlags & OGG_PAGE_FLAG_END_OF_STREAM)) {
                     AMediaFormat *meta = buffer->meta_data();
                     AMediaFormat_setInt32(
                             meta, AMEDIAFORMAT_KEY_VALID_SAMPLES, mCurrentPageSamples);
@@ -864,6 +871,9 @@
             }
 
             // fall through, the buffer now contains the start of the packet.
+            ALOGV("have start of packet, getting rest");
+        } else {
+            ALOGV("moving to next page");
         }
 
         CHECK_EQ(mNextLaceIndex, mCurrentPage.mNumSegments);
@@ -899,9 +909,10 @@
         mNextLaceIndex = 0;
 
         if (buffer != NULL) {
-            if ((mCurrentPage.mFlags & 1) == 0) {
+            if ((mCurrentPage.mFlags & OGG_PAGE_FLAG_CONTINUED_PACKET) == 0) {
                 // This page does not continue the packet, i.e. the packet
                 // is already complete.
+                ALOGV("packet was already complete?!");
 
                 if (timeUs >= 0) {
                     AMediaFormat *meta = buffer->meta_data();
@@ -909,8 +920,10 @@
                 }
 
                 AMediaFormat *meta = buffer->meta_data();
-                AMediaFormat_setInt32(
-                        meta, AMEDIAFORMAT_KEY_VALID_SAMPLES, mCurrentPageSamples);
+                if (mCurrentPage.mFlags & OGG_PAGE_FLAG_END_OF_STREAM) {
+                    AMediaFormat_setInt32(
+                            meta, AMEDIAFORMAT_KEY_VALID_SAMPLES, mCurrentPageSamples);
+                }
                 mFirstPacketInPage = false;
 
                 *out = buffer;
@@ -929,6 +942,7 @@
     for (size_t i = 0; i < mNumHeaders; ++i) {
         // ignore timestamp for configuration packets
         if ((err = _readNextPacket(&packet, /* calcVorbisTimestamp = */ false)) != AMEDIA_OK) {
+            ALOGV("readNextPacket failed");
             return err;
         }
         ALOGV("read packet of size %zu\n", packet->range_length());
@@ -1008,6 +1022,10 @@
 
     size_t size = buffer->range_length();
 
+    if (size == 0) {
+        return 0;
+    }
+
     ogg_buffer buf;
     buf.data = (uint8_t *)data;
     buf.size = size;