Temporary fix for MP3 decoding by duplicating packets before handing them to the decoder.

Turns out packets filled out by av_read_frame() were being modifying on subsequent calls to av_read_frame().  Might be a case of setting the data pointer to internal memory as opposed to allocated memory.

Review URL: http://codereview.chromium.org/115561

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@16525 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 117 insertions, 0 deletions

diff --git a/media/base/media_posix.cc b/media/base/media_posix.cc
index 94c1a89..57e1b77 100644
--- a/media/base/media_posix.cc
+++ b/media/base/media_posix.cc
@@ -27,6 +27,11 @@ int av_get_bits_per_sample_format(enum SampleFormat sample_fmt) {
   return av_get_bits_per_sample_format_ptr(sample_fmt);
 }
 
+int (*av_new_packet_ptr)(AVPacket* pkt, int size);
+int av_new_packet(AVPacket* pkt, int size) {
+  return av_new_packet_ptr(pkt, size);
+}
+
 void (*avcodec_init_ptr)(void) = NULL;
 void avcodec_init(void) {
   avcodec_init_ptr();
@@ -194,6 +199,9 @@ bool InitializeMediaLibrary(const FilePath& module_dir) {
   av_get_bits_per_sample_format_ptr =
       reinterpret_cast<int (*)(enum SampleFormat)>(
           dlsym(libs[FILE_LIBAVCODEC], "av_get_bits_per_sample_format"));
+  av_new_packet_ptr =
+      reinterpret_cast<int (*)(AVPacket*, int)>(
+          dlsym(libs[FILE_LIBAVCODEC], "av_new_packet"));
   avcodec_init_ptr =
       reinterpret_cast<void(*)(void)>(
           dlsym(libs[FILE_LIBAVCODEC], "avcodec_init"));
@@ -251,6 +259,7 @@ bool InitializeMediaLibrary(const FilePath& module_dir) {
 
   // Check that all the symbols were loaded correctly before returning true.
   if (av_get_bits_per_sample_format_ptr &&
+      av_new_packet_ptr &&
       avcodec_init_ptr &&
       avcodec_find_decoder_ptr &&
       avcodec_thread_init_ptr &&
diff --git a/media/filters/ffmpeg_demuxer.cc b/media/filters/ffmpeg_demuxer.cc
index ab3974b..201bd95 100644
--- a/media/filters/ffmpeg_demuxer.cc
+++ b/media/filters/ffmpeg_demuxer.cc
@@ -328,6 +328,25 @@ void FFmpegDemuxer::DemuxTask() {
   DCHECK(packet->stream_index < static_cast<int>(packet_streams_.size()));
   FFmpegDemuxerStream* demuxer_stream = packet_streams_[packet->stream_index];
   if (demuxer_stream) {
+    // Duplicate the entire packet if we're dealing with MP3 due to an issue
+    // where previously demuxed packets can become corrupted by simply demuxing
+    // additional packets.
+    //
+    // TODO(scherkus): fix the MP3 packet copying hack.
+    if (demuxer_stream->av_stream()->codec->codec_id == CODEC_ID_MP3) {
+      scoped_ptr<AVPacket> clone(ClonePacket(packet.get()));
+      if (!clone.get()) {
+        NOTREACHED();
+        return;
+      }
+      // Free FFmpeg-allocated memory and swap original packet into |clone| so
+      // that it gets deleted as |clone| goes out of scope.
+      av_free_packet(packet.get());
+      packet.swap(clone);
+    }
+
+    // Queue the packet with the appropriate stream.  The stream takes
+    // ownership of the AVPacket.
     current_timestamp_ = demuxer_stream->EnqueuePacket(packet.release());
   } else {
     av_free_packet(packet.get());
@@ -350,4 +369,17 @@ bool FFmpegDemuxer::StreamsHavePendingReads() {
   return false;
 }
 
+AVPacket* FFmpegDemuxer::ClonePacket(AVPacket* packet) {
+  scoped_ptr<AVPacket> clone(new AVPacket());
+  if (!clone.get() || av_new_packet(clone.get(), packet->size) < 0) {
+    return NULL;
+  }
+  DCHECK_EQ(clone->size, packet->size);
+  clone->dts = packet->dts;
+  clone->pts = packet->pts;
+  clone->duration = packet->duration;
+  memcpy(clone->data, packet->data, clone->size);
+  return clone.release();
+}
+
 }  // namespace media
diff --git a/media/filters/ffmpeg_demuxer.h b/media/filters/ffmpeg_demuxer.h
index bd785b7..bb9082e 100644
--- a/media/filters/ffmpeg_demuxer.h
+++ b/media/filters/ffmpeg_demuxer.h
@@ -142,6 +142,10 @@ class FFmpegDemuxer : public Demuxer {
   // Safe to call on any thread.
   bool StreamsHavePendingReads();
 
+  // Helper function to deep copy an AVPacket's data, size and timestamps.
+  // Returns NULL if a packet could not be cloned (i.e., out of memory).
+  AVPacket* ClonePacket(AVPacket* packet);
+
   // FFmpeg context handle.
   scoped_ptr_malloc<AVFormatContext, ScopedPtrAVFree> format_context_;
 
diff --git a/media/filters/ffmpeg_demuxer_unittest.cc b/media/filters/ffmpeg_demuxer_unittest.cc
index f4c989a..6b4f06d 100644
--- a/media/filters/ffmpeg_demuxer_unittest.cc
+++ b/media/filters/ffmpeg_demuxer_unittest.cc
@@ -115,6 +115,9 @@ static base::WaitableEvent* g_seek_event = NULL;
 static int64_t g_expected_seek_timestamp = 0;
 static int g_expected_seek_flags = 0;
 
+// Counts outstanding packets allocated by av_new_frame().
+static int g_outstanding_packets_av_new_frame = 0;
+
 int av_open_input_file(AVFormatContext** format, const char* filename,
                        AVInputFormat* input_format, int buffer_size,
                        AVFormatParameters* parameters) {
@@ -140,6 +143,20 @@ int64 av_rescale_q(int64 a, AVRational bq, AVRational cq) {
   return a * num / den;
 }
 
+void DestructPacket(AVPacket* packet) {
+  delete [] packet->data;
+  --g_outstanding_packets_av_new_frame;
+}
+
+int av_new_packet(AVPacket* packet, int size) {
+  memset(packet, 0, sizeof(*packet));
+  packet->data = new uint8[size];
+  packet->size = size;
+  packet->destruct = &DestructPacket;
+  ++g_outstanding_packets_av_new_frame;
+  return 0;
+}
+
 void av_free(void* ptr) {
   if (ptr) {
     EXPECT_EQ(&g_format, ptr);
@@ -604,4 +621,58 @@ TEST_F(FFmpegDemuxerTest, ReadAndSeek) {
   EXPECT_TRUE(PacketQueue::get()->WaitForOutstandingPackets(0));
 }
 
+// This tests our deep-copying workaround for FFmpeg's MP3 demuxer.  When we fix
+// the root cause this test will fail and should be removed.
+TEST_F(FFmpegDemuxerTest, MP3Hack) {
+  // Prepare some test data.
+  const int kPacketAudio = 0;  // Stream index relative to the container.
+  const int kAudio = 0;        // Stream index relative to Demuxer::GetStream().
+  const size_t kDataSize = 4;
+  uint8 audio_data[kDataSize] = {0, 1, 2, 3};
+
+  // Simulate media with a single MP3 audio stream.
+  g_format.nb_streams = 1;
+  g_format.streams[kPacketAudio] = &g_streams[0];
+  g_streams[0].duration = 10;
+  g_streams[0].codec = &g_audio_codec;
+  g_audio_codec.codec_id = CODEC_ID_MP3;
+
+  // Initialize the demuxer.
+  EXPECT_TRUE(demuxer_->Initialize(data_source_.get()));
+  EXPECT_TRUE(filter_host_->WaitForInitialized());
+  EXPECT_TRUE(filter_host_->IsInitialized());
+  EXPECT_EQ(PIPELINE_OK, pipeline_->GetError());
+
+  // Verify the stream was created.
+  EXPECT_EQ(1, demuxer_->GetNumberOfStreams());
+  scoped_refptr<DemuxerStream> audio_stream = demuxer_->GetStream(kAudio);
+  ASSERT_TRUE(audio_stream);
+
+  // Prepare our test audio packet.
+  PacketQueue::get()->Enqueue(kPacketAudio, kDataSize, audio_data);
+
+  // Audio read should perform a deep copy on the packet and instantly release
+  // the original packet.  The data pointers should not be the same, but the
+  // contents should match.
+  scoped_refptr<TestReader> reader = new TestReader();
+  reader->Read(audio_stream);
+  pipeline_->RunAllTasks();
+  EXPECT_TRUE(reader->WaitForRead());
+  EXPECT_TRUE(reader->called());
+  ASSERT_TRUE(reader->buffer());
+  EXPECT_FALSE(reader->buffer()->IsDiscontinuous());
+  EXPECT_NE(audio_data, reader->buffer()->GetData());
+  EXPECT_EQ(kDataSize, reader->buffer()->GetDataSize());
+  EXPECT_EQ(0, memcmp(audio_data, reader->buffer()->GetData(), kDataSize));
+
+  // Original AVPacket from the queue should have been released due to copying.
+  EXPECT_TRUE(PacketQueue::get()->WaitForOutstandingPackets(0));
+  EXPECT_EQ(1, g_outstanding_packets_av_new_frame);
+
+  // Now release our reference, which should destruct the packet allocated by
+  // av_new_packet().
+  reader = NULL;
+  EXPECT_EQ(0, g_outstanding_packets_av_new_frame);
+}
+
 }  // namespace
diff --git a/third_party/ffmpeg/avcodec-52.def b/third_party/ffmpeg/avcodec-52.def
index e567dc9..5c0ebab 100644
--- a/third_party/ffmpeg/avcodec-52.def
+++ b/third_party/ffmpeg/avcodec-52.def
@@ -1,6 +1,7 @@
 LIBRARY avcodec-52
 EXPORTS
   av_get_bits_per_sample_format
+  av_new_packet
   avcodec_alloc_context
   avcodec_alloc_frame
   avcodec_decode_audio2