Switched over to scoped_ptr_malloc and heap-allocated FFmpeg structures.

Using heap-allocated FFmmpeg structures (notably AVFrame) improves our binary compatability with differing versions of FFmpeg.

Some minor code cleanup as well.

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

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

8 files changed, 58 insertions, 43 deletions

diff --git a/media/filters/ffmpeg_audio_decoder.cc b/media/filters/ffmpeg_audio_decoder.cc
index 714d0a9..914a8ed8 100644
--- a/media/filters/ffmpeg_audio_decoder.cc
+++ b/media/filters/ffmpeg_audio_decoder.cc
@@ -9,13 +9,13 @@
 
 namespace media {
 
+// Size of the decoded audio buffer.
 const size_t FFmpegAudioDecoder::kOutputBufferSize =
     AVCODEC_MAX_AUDIO_FRAME_SIZE;
 
 FFmpegAudioDecoder::FFmpegAudioDecoder()
     : DecoderBase<AudioDecoder, Buffer>(NULL),
-      codec_context_(NULL),
-      output_buffer_(NULL) {
+      codec_context_(NULL) {
 }
 
 FFmpegAudioDecoder::~FFmpegAudioDecoder() {
@@ -64,8 +64,8 @@ bool FFmpegAudioDecoder::OnInitialize(DemuxerStream* demuxer_stream) {
   }
 
   // Prepare the output buffer.
-  output_buffer_ = static_cast<uint8*>(av_malloc(kOutputBufferSize));
-  if (!output_buffer_) {
+  output_buffer_.reset(static_cast<uint8*>(av_malloc(kOutputBufferSize)));
+  if (!output_buffer_.get()) {
     host_->Error(PIPELINE_ERROR_OUT_OF_MEMORY);
     return false;
   }
@@ -73,20 +73,16 @@ bool FFmpegAudioDecoder::OnInitialize(DemuxerStream* demuxer_stream) {
 }
 
 void FFmpegAudioDecoder::OnStop() {
-  if (output_buffer_)
-    av_free(output_buffer_);
 }
 
 void FFmpegAudioDecoder::OnDecode(Buffer* input) {
-  const uint8_t* input_buffer = input->GetData();
-  size_t input_buffer_size = input->GetDataSize();
-
+  int16_t* output_buffer = reinterpret_cast<int16_t*>(output_buffer_.get());
   int output_buffer_size = kOutputBufferSize;
   int result = avcodec_decode_audio2(codec_context_,
-                                     reinterpret_cast<int16_t*>(output_buffer_),
+                                     output_buffer,
                                      &output_buffer_size,
-                                     input_buffer,
-                                     input_buffer_size);
+                                     input->GetData(),
+                                     input->GetDataSize());
 
   if (result < 0 || output_buffer_size > kOutputBufferSize) {
     host_->Error(PIPELINE_ERROR_DECODE);
@@ -96,7 +92,7 @@ void FFmpegAudioDecoder::OnDecode(Buffer* input) {
   } else {
     DataBuffer* result_buffer = new DataBuffer();
     memcpy(result_buffer->GetWritableData(output_buffer_size),
-      output_buffer_, output_buffer_size);
+           output_buffer, output_buffer_size);
     result_buffer->SetTimestamp(input->GetTimestamp());
     result_buffer->SetDuration(input->GetDuration());
     EnqueueResult(result_buffer);
diff --git a/media/filters/ffmpeg_audio_decoder.h b/media/filters/ffmpeg_audio_decoder.h
index 045fdf8..ffb99f8 100644
--- a/media/filters/ffmpeg_audio_decoder.h
+++ b/media/filters/ffmpeg_audio_decoder.h
@@ -2,7 +2,6 @@
 // source code is governed by a BSD-style license that can be found in the
 // LICENSE file.
 
-
 #ifndef MEDIA_FILTERS_FFMPEG_AUDIO_DECODER_H_
 #define MEDIA_FILTERS_FFMPEG_AUDIO_DECODER_H_
 
@@ -13,7 +12,8 @@ struct AVCodecContext;
 
 namespace media {
 
-//------------------------------------------------------------------------------
+// Forward declaration for scoped_ptr_malloc.
+class ScopedPtrAVFree;
 
 class FFmpegAudioDecoder : public DecoderBase<AudioDecoder, Buffer> {
  public:
@@ -39,8 +39,8 @@ class FFmpegAudioDecoder : public DecoderBase<AudioDecoder, Buffer> {
   AVCodecContext* codec_context_;
 
   // Data buffer to carry decoded raw PCM samples. This buffer is created by
-  // av_malloc and is used throughout the lifetime of this class.
-  uint8* output_buffer_;
+  // av_malloc() and is used throughout the lifetime of this class.
+  scoped_ptr_malloc<uint8, ScopedPtrAVFree> output_buffer_;
 
   static const size_t kOutputBufferSize;
 
diff --git a/media/filters/ffmpeg_common.h b/media/filters/ffmpeg_common.h
index 6a5f0e9..d82dfef 100644
--- a/media/filters/ffmpeg_common.h
+++ b/media/filters/ffmpeg_common.h
@@ -22,6 +22,15 @@ MSVC_POP_WARNING();
 
 namespace media {
 
+// Wraps FFmpeg's av_free() in a class that can be passed as a template argument
+// to scoped_ptr_malloc.
+class ScopedPtrAVFree {
+ public:
+  inline void operator()(void* x) const {
+    av_free(x);
+  }
+};
+
 // MediaFormat key identifying the CodecID.
 extern const char kFFmpegCodecID[];
 
diff --git a/media/filters/ffmpeg_demuxer.cc b/media/filters/ffmpeg_demuxer.cc
index c29b8a1..f3e4b79 100644
--- a/media/filters/ffmpeg_demuxer.cc
+++ b/media/filters/ffmpeg_demuxer.cc
@@ -183,15 +183,16 @@ bool FFmpegDemuxerStream::FulfillPendingReads() {
 // FFmpegDemuxer
 //
 FFmpegDemuxer::FFmpegDemuxer()
-    : format_context_(NULL),
-      thread_("DemuxerThread") {
+    : thread_("DemuxerThread") {
 }
 
 FFmpegDemuxer::~FFmpegDemuxer() {
   Stop();
-  if (format_context_) {
-    av_free(format_context_);
-  }
+  // TODO(scherkus): I believe we need to use av_close_input_file() here
+  // instead of scoped_ptr_malloc calling av_free().
+  //
+  // Note that av_close_input_file() doesn't close the codecs so we need to
+  // figure out who's responsible for closing the them.
 }
 
 void FFmpegDemuxer::PostDemuxTask() {
@@ -222,8 +223,9 @@ bool FFmpegDemuxer::Initialize(DataSource* data_source) {
   std::string key = FFmpegGlue::get()->AddDataSource(data_source);
 
   // Open FFmpeg AVFormatContext.
-  DCHECK(!format_context_);
-  int result = av_open_input_file(&format_context_, key.c_str(), NULL, 0, NULL);
+  DCHECK(!format_context_.get());
+  AVFormatContext* context = NULL;
+  int result = av_open_input_file(&context, key.c_str(), NULL, 0, NULL);
 
   // Remove our data source.
   FFmpegGlue::get()->RemoveDataSource(data_source);
@@ -233,8 +235,12 @@ bool FFmpegDemuxer::Initialize(DataSource* data_source) {
     return false;
   }
 
+  // Assign to our scoped_ptr_malloc.
+  DCHECK(context);
+  format_context_.reset(context);
+
   // Fully initialize AVFormatContext by parsing the stream a little.
-  result = av_find_stream_info(format_context_);
+  result = av_find_stream_info(format_context_.get());
   if (result < 0) {
     host_->Error(DEMUXER_ERROR_COULD_NOT_PARSE);
     return false;
@@ -293,7 +299,8 @@ void FFmpegDemuxer::SeekTask(base::TimeDelta time) {
     flags |= AVSEEK_FLAG_BACKWARD;
   }
 
-  if (av_seek_frame(format_context_, -1, time.InMicroseconds(), flags) < 0) {
+  if (av_seek_frame(format_context_.get(), -1, time.InMicroseconds(),
+                    flags) < 0) {
     // TODO(scherkus): signal error.
     NOTIMPLEMENTED();
   }
@@ -307,7 +314,7 @@ void FFmpegDemuxer::DemuxTask() {
 
   // Allocate and read an AVPacket from the media.
   scoped_ptr<AVPacket> packet(new AVPacket());
-  int result = av_read_frame(format_context_, packet.get());
+  int result = av_read_frame(format_context_.get(), packet.get());
   if (result < 0) {
     // TODO(scherkus): handle end of stream by marking Buffer with the end
     // of stream flag.
diff --git a/media/filters/ffmpeg_demuxer.h b/media/filters/ffmpeg_demuxer.h
index dc9cfb5..72866af 100644
--- a/media/filters/ffmpeg_demuxer.h
+++ b/media/filters/ffmpeg_demuxer.h
@@ -41,6 +41,9 @@ namespace media {
 
 class FFmpegDemuxer;
 
+// Forward declaration for scoped_ptr_malloc.
+class ScopedPtrAVFree;
+
 class FFmpegDemuxerStream : public DemuxerStream {
  public:
   // Maintains a reference to |demuxer| and initializes itself using information
@@ -138,7 +141,7 @@ class FFmpegDemuxer : public Demuxer {
   bool StreamsHavePendingReads();
 
   // FFmpeg context handle.
-  AVFormatContext* format_context_;
+  scoped_ptr_malloc<AVFormatContext, ScopedPtrAVFree> format_context_;
 
   // Latest timestamp read on the demuxer thread.
   base::TimeDelta current_timestamp_;
diff --git a/media/filters/ffmpeg_demuxer_unittest.cc b/media/filters/ffmpeg_demuxer_unittest.cc
index b18e049..1f9fbe7 100644
--- a/media/filters/ffmpeg_demuxer_unittest.cc
+++ b/media/filters/ffmpeg_demuxer_unittest.cc
@@ -115,7 +115,9 @@ int av_find_stream_info(AVFormatContext* format) {
 }
 
 void av_free(void* ptr) {
-  EXPECT_EQ(&g_format, ptr);
+  if (ptr) {
+    EXPECT_EQ(&g_format, ptr);
+  }
 }
 
 int av_read_frame(AVFormatContext* format, AVPacket* packet) {
diff --git a/media/filters/ffmpeg_video_decoder.cc b/media/filters/ffmpeg_video_decoder.cc
index 7847f54..85b74ba 100644
--- a/media/filters/ffmpeg_video_decoder.cc
+++ b/media/filters/ffmpeg_video_decoder.cc
@@ -9,7 +9,6 @@
 
 namespace media {
 
-
 FFmpegVideoDecoder::FFmpegVideoDecoder()
     : DecoderBase<VideoDecoder, VideoFrame>(NULL),
       width_(0),
@@ -66,10 +65,12 @@ void FFmpegVideoDecoder::OnDecode(Buffer* buffer) {
   const uint8_t* data_in = buffer->GetData();
   const size_t size_in = buffer->GetDataSize();
 
+  // We don't allocate AVFrame on the stack since different versions of FFmpeg
+  // may change the size of AVFrame, causing stack corruption.  The solution is
+  // to let FFmpeg allocate the structure via avcodec_alloc_frame().
   int decoded = 0;
-  AVFrame yuv_frame;
-  avcodec_get_frame_defaults(&yuv_frame);
-  int result = avcodec_decode_video(codec_context_, &yuv_frame, &decoded,
+  scoped_ptr_malloc<AVFrame, ScopedPtrAVFree> yuv_frame(avcodec_alloc_frame());
+  int result = avcodec_decode_video(codec_context_, yuv_frame.get(), &decoded,
                                     data_in, size_in);
 
   if (result < 0) {
@@ -101,13 +102,13 @@ void FFmpegVideoDecoder::OnDecode(Buffer* buffer) {
       host_->Error(PIPELINE_ERROR_DECODE);
       return;
   }
-  if (!EnqueueVideoFrame(surface_format, yuv_frame)) {
+  if (!EnqueueVideoFrame(surface_format, yuv_frame.get())) {
     host_->Error(PIPELINE_ERROR_DECODE);
   }
 }
 
 bool FFmpegVideoDecoder::EnqueueVideoFrame(VideoSurface::Format surface_format,
-                                           const AVFrame& frame) {
+                                           const AVFrame* frame) {
   // Dequeue the next time tuple and create a VideoFrame object with
   // that timestamp and duration.
   TimeTuple time = time_queue_.top();
@@ -138,11 +139,11 @@ bool FFmpegVideoDecoder::EnqueueVideoFrame(VideoSurface::Format surface_format,
 
 void FFmpegVideoDecoder::CopyPlane(size_t plane,
                                    const VideoSurface& surface,
-                                   const AVFrame& frame) {
+                                   const AVFrame* frame) {
   DCHECK(surface.width % 4 == 0);
   DCHECK(surface.height % 2 == 0);
-  const uint8* source = frame.data[plane];
-  const size_t source_stride = frame.linesize[plane];
+  const uint8* source = frame->data[plane];
+  const size_t source_stride = frame->linesize[plane];
   uint8* dest = surface.data[plane];
   const size_t dest_stride = surface.strides[plane];
   size_t bytes_per_line = surface.width;
diff --git a/media/filters/ffmpeg_video_decoder.h b/media/filters/ffmpeg_video_decoder.h
index 1997cd6..7f20054e 100644
--- a/media/filters/ffmpeg_video_decoder.h
+++ b/media/filters/ffmpeg_video_decoder.h
@@ -2,7 +2,6 @@
 // source code is governed by a BSD-style license that can be found in the
 // LICENSE file.
 
-
 #ifndef MEDIA_FILTERS_FFMPEG_VIDEO_DECODER_H_
 #define MEDIA_FILTERS_FFMPEG_VIDEO_DECODER_H_
 
@@ -17,8 +16,6 @@ struct AVFrame;
 
 namespace media {
 
-//------------------------------------------------------------------------------
-
 class FFmpegVideoDecoder : public DecoderBase<VideoDecoder, VideoFrame> {
  public:
   static FilterFactory* CreateFactory() {
@@ -37,10 +34,10 @@ class FFmpegVideoDecoder : public DecoderBase<VideoDecoder, VideoFrame> {
   virtual ~FFmpegVideoDecoder();
 
   bool EnqueueVideoFrame(VideoSurface::Format surface_format,
-                         const AVFrame& frame);
+                         const AVFrame* frame);
 
   void CopyPlane(size_t plane, const VideoSurface& surface,
-                 const AVFrame& frame);
+                 const AVFrame* frame);
 
   size_t width_;
   size_t height_;