Fix Issue 160529 in a nice way with unittest.

Original CL171023 by Song YeWen.

BUG=16020
TEST=test with all media types and ensure there are no memory leaks are
functional differences from previous version.

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

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

6 files changed, 35 insertions, 54 deletions

diff --git a/AUTHORS b/AUTHORS
index 1054b52..0a41a19 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -46,3 +46,4 @@ Paul Wicks <pwicks86@gmail.com>
 Thiago Farina <thiago.farina@gmail.com>
 Viet-Trung Luu <viettrungluu@gmail.com>
 Pierre-Antoine LaFayette <pierre.lafayette@gmail.com>
+Song YeWen <ffmpeg@gmail.com>
diff --git a/media/base/mock_ffmpeg.cc b/media/base/mock_ffmpeg.cc
index 562aff2..541f476 100644
--- a/media/base/mock_ffmpeg.cc
+++ b/media/base/mock_ffmpeg.cc
@@ -77,7 +77,6 @@ void MockFFmpeg::DestructPacket(AVPacket* packet) {
 
 // FFmpeg stubs that delegate to the FFmpegMock instance.
 extern "C" {
-
 void avcodec_init() {
   media::MockFFmpeg::get()->AVCodecInit();
 }
@@ -174,6 +173,9 @@ void av_free(void* ptr) {
   }
 }
 
+int av_dup_packet(AVPacket* packet) {
+  return media::MockFFmpeg::get()->AVDupPacket(packet);
+}
 }  // extern "C"
 
 }  // namespace media
diff --git a/media/base/mock_ffmpeg.h b/media/base/mock_ffmpeg.h
index 4ba38fc..9471dec 100644
--- a/media/base/mock_ffmpeg.h
+++ b/media/base/mock_ffmpeg.h
@@ -47,6 +47,7 @@ class MockFFmpeg {
   MOCK_METHOD2(AVNewPacket, int(AVPacket* packet, int size));
   MOCK_METHOD1(AVFreePacket, void(AVPacket* packet));
   MOCK_METHOD1(AVFree, void(void* ptr));
+  MOCK_METHOD1(AVDupPacket, int(AVPacket* packet));
 
   // Used for verifying check points during tests.
   MOCK_METHOD1(CheckPoint, void(int id));
diff --git a/media/filters/ffmpeg_demuxer.cc b/media/filters/ffmpeg_demuxer.cc
index d106a51..af0d71e 100644
--- a/media/filters/ffmpeg_demuxer.cc
+++ b/media/filters/ffmpeg_demuxer.cc
@@ -11,25 +11,6 @@
 #include "media/filters/ffmpeg_demuxer.h"
 #include "media/filters/ffmpeg_glue.h"
 
-namespace {
-
-// 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) {
-  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
-
 namespace media {
 
 //
@@ -300,8 +281,8 @@ size_t FFmpegDemuxer::GetNumberOfStreams() {
 }
 
 scoped_refptr<DemuxerStream> FFmpegDemuxer::GetStream(int stream) {
-  DCHECK(stream >= 0);
-  DCHECK(stream < static_cast<int>(streams_.size()));
+  DCHECK_GE(stream, 0);
+  DCHECK_LT(stream, static_cast<int>(streams_.size()));
   return streams_[stream].get();
 }
 
@@ -493,21 +474,15 @@ void FFmpegDemuxer::DemuxTask() {
   // TODO(scherkus): should we post this back to the pipeline thread?  I'm
   // worried about downstream filters (i.e., decoders) executing on this
   // thread.
-  DCHECK(packet->stream_index >= 0);
-  DCHECK(packet->stream_index < static_cast<int>(packet_streams_.size()));
+  DCHECK_GE(packet->stream_index, 0);
+  DCHECK_LT(packet->stream_index, static_cast<int>(packet_streams_.size()));
   FFmpegDemuxerStream* demuxer_stream = packet_streams_[packet->stream_index];
   if (demuxer_stream) {
-    // Duplicate the entire packet to avoid aliasing.
-    // Directly affects MP3, but do all formats to be safe.
-    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);
+    // If a packet is returned by FFmpeg's av_parser_parse2()
+    // the packet will reference an inner memory of FFmpeg.
+    // In this case, the packet's "destruct" member is NULL,
+    // and it MUST be duplicated.  Fixes issue with MP3.
+    av_dup_packet(packet.get());
 
     // Queue the packet with the appropriate stream.  The stream takes
     // ownership of the AVPacket.
diff --git a/media/filters/ffmpeg_demuxer_unittest.cc b/media/filters/ffmpeg_demuxer_unittest.cc
index 4861b4f..078f54b 100644
--- a/media/filters/ffmpeg_demuxer_unittest.cc
+++ b/media/filters/ffmpeg_demuxer_unittest.cc
@@ -320,8 +320,8 @@ TEST_F(FFmpegDemuxerTest, Read) {
   EXPECT_CALL(*MockFFmpeg::get(), AVFreePacket(_)).WillOnce(FreePacket());
   EXPECT_CALL(*MockFFmpeg::get(), AVReadFrame(&format_context_, _))
       .WillOnce(CreatePacket(AV_STREAM_AUDIO, kAudioData, kDataSize));
-  EXPECT_CALL(*MockFFmpeg::get(), AVNewPacket(_, _)).WillOnce(NewPacket());
-  EXPECT_CALL(*MockFFmpeg::get(), AVFreePacket(_)).WillOnce(FreePacket());
+  EXPECT_CALL(*MockFFmpeg::get(), AVDupPacket(_))
+      .WillOnce(Return(0));
 
   // ...then we'll free it with some sanity checkpoints...
   EXPECT_CALL(*MockFFmpeg::get(), CheckPoint(1));
@@ -331,8 +331,8 @@ TEST_F(FFmpegDemuxerTest, Read) {
   // ...then we'll read a video packet...
   EXPECT_CALL(*MockFFmpeg::get(), AVReadFrame(&format_context_, _))
       .WillOnce(CreatePacket(AV_STREAM_VIDEO, kVideoData, kDataSize));
-  EXPECT_CALL(*MockFFmpeg::get(), AVNewPacket(_, _)).WillOnce(NewPacket());
-  EXPECT_CALL(*MockFFmpeg::get(), AVFreePacket(_)).WillOnce(FreePacket());
+  EXPECT_CALL(*MockFFmpeg::get(), AVDupPacket(_))
+      .WillOnce(Return(0));
 
   // ...then we'll free it with some sanity checkpoints...
   EXPECT_CALL(*MockFFmpeg::get(), CheckPoint(3));
@@ -449,16 +449,16 @@ TEST_F(FFmpegDemuxerTest, Seek) {
   // inside FFmpegDemuxer...
   EXPECT_CALL(*MockFFmpeg::get(), AVReadFrame(&format_context_, _))
       .WillOnce(CreatePacket(AV_STREAM_AUDIO, kAudioData, kDataSize));
-  EXPECT_CALL(*MockFFmpeg::get(), AVNewPacket(_, _)).WillOnce(NewPacket());
-  EXPECT_CALL(*MockFFmpeg::get(), AVFreePacket(_)).WillOnce(FreePacket());
+  EXPECT_CALL(*MockFFmpeg::get(), AVDupPacket(_))
+      .WillOnce(Return(0));
   EXPECT_CALL(*MockFFmpeg::get(), AVReadFrame(&format_context_, _))
       .WillOnce(CreatePacket(AV_STREAM_AUDIO, kAudioData, kDataSize));
-  EXPECT_CALL(*MockFFmpeg::get(), AVNewPacket(_, _)).WillOnce(NewPacket());
-  EXPECT_CALL(*MockFFmpeg::get(), AVFreePacket(_)).WillOnce(FreePacket());
+  EXPECT_CALL(*MockFFmpeg::get(), AVDupPacket(_))
+      .WillOnce(Return(0));
   EXPECT_CALL(*MockFFmpeg::get(), AVReadFrame(&format_context_, _))
       .WillOnce(CreatePacket(AV_STREAM_VIDEO, kVideoData, kDataSize));
-  EXPECT_CALL(*MockFFmpeg::get(), AVNewPacket(_, _)).WillOnce(NewPacket());
-  EXPECT_CALL(*MockFFmpeg::get(), AVFreePacket(_)).WillOnce(FreePacket());
+  EXPECT_CALL(*MockFFmpeg::get(), AVDupPacket(_))
+      .WillOnce(Return(0));
 
   // ...then we'll release our video packet...
   EXPECT_CALL(*MockFFmpeg::get(), AVFreePacket(_)).WillOnce(FreePacket());
@@ -489,25 +489,25 @@ TEST_F(FFmpegDemuxerTest, Seek) {
   // ...followed by two audio packet reads we'll trigger...
   EXPECT_CALL(*MockFFmpeg::get(), AVReadFrame(&format_context_, _))
       .WillOnce(CreatePacket(AV_STREAM_AUDIO, kAudioData, kDataSize));
-  EXPECT_CALL(*MockFFmpeg::get(), AVNewPacket(_, _)).WillOnce(NewPacket());
-  EXPECT_CALL(*MockFFmpeg::get(), AVFreePacket(_)).WillOnce(FreePacket());
+  EXPECT_CALL(*MockFFmpeg::get(), AVDupPacket(_))
+      .WillOnce(Return(0));
   EXPECT_CALL(*MockFFmpeg::get(), AVFreePacket(_)).WillOnce(FreePacket());
   EXPECT_CALL(*MockFFmpeg::get(), AVReadFrame(&format_context_, _))
       .WillOnce(CreatePacket(AV_STREAM_AUDIO, kAudioData, kDataSize));
-  EXPECT_CALL(*MockFFmpeg::get(), AVNewPacket(_, _)).WillOnce(NewPacket());
-  EXPECT_CALL(*MockFFmpeg::get(), AVFreePacket(_)).WillOnce(FreePacket());
+  EXPECT_CALL(*MockFFmpeg::get(), AVDupPacket(_))
+      .WillOnce(Return(0));
   EXPECT_CALL(*MockFFmpeg::get(), AVFreePacket(_)).WillOnce(FreePacket());
 
   // ...followed by two video packet reads...
   EXPECT_CALL(*MockFFmpeg::get(), AVReadFrame(&format_context_, _))
       .WillOnce(CreatePacket(AV_STREAM_VIDEO, kVideoData, kDataSize));
-  EXPECT_CALL(*MockFFmpeg::get(), AVNewPacket(_, _)).WillOnce(NewPacket());
-  EXPECT_CALL(*MockFFmpeg::get(), AVFreePacket(_)).WillOnce(FreePacket());
+  EXPECT_CALL(*MockFFmpeg::get(), AVDupPacket(_))
+      .WillOnce(Return(0));
   EXPECT_CALL(*MockFFmpeg::get(), AVFreePacket(_)).WillOnce(FreePacket());
   EXPECT_CALL(*MockFFmpeg::get(), AVReadFrame(&format_context_, _))
       .WillOnce(CreatePacket(AV_STREAM_VIDEO, kVideoData, kDataSize));
-  EXPECT_CALL(*MockFFmpeg::get(), AVNewPacket(_, _)).WillOnce(NewPacket());
-  EXPECT_CALL(*MockFFmpeg::get(), AVFreePacket(_)).WillOnce(FreePacket());
+  EXPECT_CALL(*MockFFmpeg::get(), AVDupPacket(_))
+      .WillOnce(Return(0));
   EXPECT_CALL(*MockFFmpeg::get(), AVFreePacket(_)).WillOnce(FreePacket());
 
   // ...and finally a sanity checkpoint to make sure everything was released.
@@ -595,7 +595,8 @@ TEST_F(FFmpegDemuxerTest, Seek) {
 
 // A mocked callback specialization for calling Read().  Since RunWithParams()
 // is mocked we don't need to pass in object or method pointers.
-typedef CallbackImpl<FFmpegDemuxerTest, void (FFmpegDemuxerTest::*)(Buffer*),
+typedef CallbackImpl<FFmpegDemuxerTest,
+                     void (FFmpegDemuxerTest::*)(Buffer*),
                      Tuple1<Buffer*> > ReadCallback;
 class MockReadCallback : public ReadCallback {
  public:
diff --git a/third_party/ffmpeg/avcodec-52.sigs b/third_party/ffmpeg/avcodec-52.sigs
index 0e4daf2..a7474c8 100644
--- a/third_party/ffmpeg/avcodec-52.sigs
+++ b/third_party/ffmpeg/avcodec-52.sigs
@@ -15,5 +15,6 @@ int avcodec_close(AVCodecContext *avctx);
 int avcodec_thread_init(AVCodecContext *s, int thread_count);
 void av_free_packet(AVPacket *pkt);
 void av_init_packet(AVPacket *pkt);
+int av_dup_packet(AVPacket *pkt);
 void avcodec_flush_buffers(AVCodecContext *avctx);
 void avcodec_init(void);