Clean up VideoDecoderConfig and replace VideoCodecInfo with a bool.

Similar to AudioDecoderConfig that was introduced in r102183, add Initialize() and IsValidConfig() to VideoDecoderConfig and update documentation. This helps pave the way to remove DemuxerStream::GetAVStream().

Since natural_size isn't used by neither VideoDecoderConfig nor VideoDecodeEngines, remove it from both and replace VideoCodecInfo with a bool.


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

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

10 files changed, 109 insertions, 111 deletions

diff --git a/media/base/video_decoder_config.cc b/media/base/video_decoder_config.cc
index 52104cb..ef5e0cb 100644
--- a/media/base/video_decoder_config.cc
+++ b/media/base/video_decoder_config.cc
@@ -8,29 +8,56 @@
 
 namespace media {
 
+VideoDecoderConfig::VideoDecoderConfig()
+    : codec_(kUnknownVideoCodec),
+      frame_rate_numerator_(0),
+      frame_rate_denominator_(0),
+      extra_data_size_(0) {
+}
+
 VideoDecoderConfig::VideoDecoderConfig(VideoCodec codec,
                                        const gfx::Size& coded_size,
                                        const gfx::Rect& visible_rect,
-                                       const gfx::Size& natural_size,
                                        int frame_rate_numerator,
                                        int frame_rate_denominator,
                                        const uint8* extra_data,
-                                       size_t extra_data_size)
-    : codec_(codec),
-      coded_size_(coded_size),
-      visible_rect_(visible_rect),
-      natural_size_(natural_size),
-      frame_rate_numerator_(frame_rate_numerator),
-      frame_rate_denominator_(frame_rate_denominator),
-      extra_data_size_(extra_data_size) {
-  CHECK(extra_data_size_ == 0 || extra_data);
+                                       size_t extra_data_size) {
+  Initialize(codec, coded_size, visible_rect,
+             frame_rate_numerator, frame_rate_denominator,
+             extra_data, extra_data_size);
+}
+
+VideoDecoderConfig::~VideoDecoderConfig() {}
+
+void VideoDecoderConfig::Initialize(VideoCodec codec,
+                                    const gfx::Size& coded_size,
+                                    const gfx::Rect& visible_rect,
+                                    int frame_rate_numerator,
+                                    int frame_rate_denominator,
+                                    const uint8* extra_data,
+                                    size_t extra_data_size) {
+  CHECK((extra_data_size != 0) == (extra_data != NULL));
+
+  codec_ = codec;
+  coded_size_ = coded_size;
+  visible_rect_ = visible_rect;
+  frame_rate_numerator_ = frame_rate_numerator;
+  frame_rate_denominator_ = frame_rate_denominator;
+  extra_data_size_ = extra_data_size;
+
   if (extra_data_size_ > 0) {
     extra_data_.reset(new uint8[extra_data_size_]);
     memcpy(extra_data_.get(), extra_data, extra_data_size_);
+  } else {
+    extra_data_.reset();
   }
 }
 
-VideoDecoderConfig::~VideoDecoderConfig() {}
+bool VideoDecoderConfig::IsValidConfig() const {
+  return codec_ != kUnknownVideoCodec &&
+      frame_rate_numerator_ > 0 &&
+      frame_rate_denominator_ > 0;
+}
 
 VideoCodec VideoDecoderConfig::codec() const {
   return codec_;
@@ -44,10 +71,6 @@ gfx::Rect VideoDecoderConfig::visible_rect() const {
   return visible_rect_;
 }
 
-gfx::Size VideoDecoderConfig::natural_size() const {
-  return natural_size_;
-}
-
 int VideoDecoderConfig::frame_rate_numerator() const {
   return frame_rate_numerator_;
 }
diff --git a/media/base/video_decoder_config.h b/media/base/video_decoder_config.h
index 2c6bd45..305bad6 100644
--- a/media/base/video_decoder_config.h
+++ b/media/base/video_decoder_config.h
@@ -30,46 +30,61 @@ enum VideoCodec {
 
 class MEDIA_EXPORT VideoDecoderConfig {
  public:
+  // Constructs an uninitialized object. Clients should call Initialize() with
+  // appropriate values before using.
+  VideoDecoderConfig();
+
+  // Constructs an initialized object. It is acceptable to pass in NULL for
+  // |extra_data|, otherwise the memory is copied.
   VideoDecoderConfig(VideoCodec codec, const gfx::Size& coded_size,
                      const gfx::Rect& visible_rect,
-                     const gfx::Size& natural_size,
                      int frame_rate_numerator, int frame_rate_denominator,
                      const uint8* extra_data, size_t extra_data_size);
+
   ~VideoDecoderConfig();
 
+  // Resets the internal state of this object.
+  void Initialize(VideoCodec codec, const gfx::Size& coded_size,
+                  const gfx::Rect& visible_rect,
+                  int frame_rate_numerator, int frame_rate_denominator,
+                  const uint8* extra_data, size_t extra_data_size);
+
+  // Returns true if this object has appropriate configuration values, false
+  // otherwise.
+  bool IsValidConfig() const;
+
   VideoCodec codec() const;
+
+  // Width and height of video frame immediately post-decode. Not all pixels
+  // in this region are valid.
   gfx::Size coded_size() const;
+
+  // Region of |coded_size_| that is visible.
   gfx::Rect visible_rect() const;
-  gfx::Size natural_size() const;
+
+  // Frame rate in seconds expressed as a fraction.
+  // TODO(scherkus): fairly certain decoders don't require frame rates.
   int frame_rate_numerator() const;
   int frame_rate_denominator() const;
+
+  // Optional byte data required to initialize video decoders, such as H.264
+  // AAVC data.
   uint8* extra_data() const;
   size_t extra_data_size() const;
 
  private:
   VideoCodec codec_;
 
-  // Width and height of video frame immediately post-decode. Not all pixels
-  // in this region are valid.
   gfx::Size coded_size_;
-
-  // Region of |coded_size_| that is visible.
   gfx::Rect visible_rect_;
 
-  // Natural width and height of the video, i.e. the visible dimensions
-  // after aspect ratio is applied.
-  gfx::Size natural_size_;
-
-  // Frame rate in seconds expressed as a fraction.
-  // TODO(scherkus): fairly certain decoders don't require frame rates.
   int frame_rate_numerator_;
   int frame_rate_denominator_;
 
-  // Optional byte data required to initialize video decoders.
   scoped_array<uint8> extra_data_;
   size_t extra_data_size_;
 
-  DISALLOW_IMPLICIT_CONSTRUCTORS(VideoDecoderConfig);
+  DISALLOW_COPY_AND_ASSIGN(VideoDecoderConfig);
 };
 
 }  // namespace media
diff --git a/media/ffmpeg/ffmpeg_common.cc b/media/ffmpeg/ffmpeg_common.cc
index 76cc36d..f1835ce 100644
--- a/media/ffmpeg/ffmpeg_common.cc
+++ b/media/ffmpeg/ffmpeg_common.cc
@@ -220,25 +220,20 @@ base::TimeDelta GetFrameDuration(AVStream* stream) {
   return ConvertFromTimeBase(time_base, 1);
 }
 
-int GetNaturalHeight(AVStream* stream) {
-  return stream->codec->height;
-}
-
-int GetNaturalWidth(AVStream* stream) {
-  double aspect_ratio;
+gfx::Size GetNaturalSize(AVStream* stream) {
+  double aspect_ratio = 1.0;
 
   if (stream->sample_aspect_ratio.num)
     aspect_ratio = av_q2d(stream->sample_aspect_ratio);
   else if (stream->codec->sample_aspect_ratio.num)
     aspect_ratio = av_q2d(stream->codec->sample_aspect_ratio);
-  else
-    aspect_ratio = 1.0;
 
+  int height = stream->codec->height;
   int width = floor(stream->codec->width * aspect_ratio + 0.5);
 
   // An even width makes things easier for YV12 and appears to be the behavior
   // expected by WebKit layout tests.
-  return width & ~1;
+  return gfx::Size(width & ~1, height);
 }
 
 void DestroyAVFormatContext(AVFormatContext* format_context) {
diff --git a/media/ffmpeg/ffmpeg_common.h b/media/ffmpeg/ffmpeg_common.h
index 9248e18..cc15e58 100644
--- a/media/ffmpeg/ffmpeg_common.h
+++ b/media/ffmpeg/ffmpeg_common.h
@@ -15,6 +15,7 @@
 #include "media/base/channel_layout.h"
 #include "media/base/media_export.h"
 #include "media/video/video_decode_engine.h"
+#include "ui/gfx/size.h"
 
 // Include FFmpeg header files.
 extern "C" {
@@ -87,8 +88,7 @@ base::TimeDelta GetFrameDuration(AVStream* stream);
 
 // Calculates the natural width and height of the video using the video's
 // encoded dimensions and sample_aspect_ratio.
-int GetNaturalHeight(AVStream* stream);
-int GetNaturalWidth(AVStream* stream);
+gfx::Size GetNaturalSize(AVStream* stream);
 
 // Closes & destroys all AVStreams in the context and then closes &
 // destroys the AVFormatContext.
diff --git a/media/filters/ffmpeg_video_decoder.cc b/media/filters/ffmpeg_video_decoder.cc
index a7785ec..677fae5 100644
--- a/media/filters/ffmpeg_video_decoder.cc
+++ b/media/filters/ffmpeg_video_decoder.cc
@@ -28,7 +28,6 @@ FFmpegVideoDecoder::FFmpegVideoDecoder(MessageLoop* message_loop,
       state_(kUnInitialized),
       decode_engine_(new FFmpegVideoDecodeEngine()),
       decode_context_(decode_context) {
-  memset(&info_, 0, sizeof(info_));
 }
 
 FFmpegVideoDecoder::~FFmpegVideoDecoder() {}
@@ -61,8 +60,7 @@ void FFmpegVideoDecoder::Initialize(DemuxerStream* demuxer_stream,
 
   AVStream* av_stream = demuxer_stream->GetAVStream();
   if (!av_stream) {
-    VideoCodecInfo info = {0};
-    OnInitializeComplete(info);
+    OnInitializeComplete(false);
     return;
   }
 
@@ -74,33 +72,31 @@ void FFmpegVideoDecoder::Initialize(DemuxerStream* demuxer_stream,
   // for now, but may not always be true forever. Fix this in the future.
   gfx::Rect visible_rect(
       av_stream->codec->width, av_stream->codec->height);
-  gfx::Size natural_size(
-      GetNaturalWidth(av_stream), GetNaturalHeight(av_stream));
-
-  if (natural_size.width() > Limits::kMaxDimension ||
-      natural_size.height() > Limits::kMaxDimension ||
-      natural_size.GetArea() > Limits::kMaxCanvas) {
-    VideoCodecInfo info = {0};
-    OnInitializeComplete(info);
+
+  natural_size_ = GetNaturalSize(av_stream);
+  if (natural_size_.width() > Limits::kMaxDimension ||
+      natural_size_.height() > Limits::kMaxDimension ||
+      natural_size_.GetArea() > Limits::kMaxCanvas) {
+    OnInitializeComplete(false);
     return;
   }
 
   VideoDecoderConfig config(CodecIDToVideoCodec(av_stream->codec->codec_id),
-                            coded_size, visible_rect, natural_size,
+                            coded_size, visible_rect,
                             av_stream->r_frame_rate.num,
                             av_stream->r_frame_rate.den,
                             av_stream->codec->extradata,
                             av_stream->codec->extradata_size);
+
   state_ = kInitializing;
   decode_engine_->Initialize(message_loop_, this, NULL, config);
 }
 
-void FFmpegVideoDecoder::OnInitializeComplete(const VideoCodecInfo& info) {
+void FFmpegVideoDecoder::OnInitializeComplete(bool success) {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
   DCHECK(!initialize_callback_.is_null());
 
-  info_ = info;
-  if (info.success) {
+  if (success) {
     state_ = kNormal;
   } else {
     host()->SetError(PIPELINE_ERROR_DECODE);
@@ -130,7 +126,7 @@ void FFmpegVideoDecoder::OnUninitializeComplete() {
   DCHECK(!uninitialize_callback_.is_null());
 
   state_ = kStopped;
-  // TODO(jiesun): Destroy the decoder context.
+
   ResetAndRunCB(&uninitialize_callback_);
 }
 
@@ -247,9 +243,6 @@ void FFmpegVideoDecoder::OnReadCompleteTask(scoped_refptr<Buffer> buffer) {
   // Push all incoming timestamps into the priority queue as long as we have
   // not yet received an end of stream buffer.  It is important that this line
   // stay below the state transition into kFlushCodec done above.
-  //
-  // TODO(ajwong): This push logic, along with the pop logic below needs to
-  // be reevaluated to correctly handle decode errors.
   if (state_ == kNormal) {
     pts_stream_.EnqueuePts(buffer.get());
   }
@@ -328,8 +321,7 @@ void FFmpegVideoDecoder::ProduceVideoSample(
 }
 
 gfx::Size FFmpegVideoDecoder::natural_size() {
-  DCHECK(info_.success);
-  return info_.natural_size;
+  return natural_size_;
 }
 
 void FFmpegVideoDecoder::FlushBuffers() {
diff --git a/media/filters/ffmpeg_video_decoder.h b/media/filters/ffmpeg_video_decoder.h
index bc2b532..cd00bd3 100644
--- a/media/filters/ffmpeg_video_decoder.h
+++ b/media/filters/ffmpeg_video_decoder.h
@@ -42,7 +42,7 @@ class MEDIA_EXPORT FFmpegVideoDecoder
 
  private:
   // VideoDecodeEngine::EventHandler interface.
-  virtual void OnInitializeComplete(const VideoCodecInfo& info) OVERRIDE;
+  virtual void OnInitializeComplete(bool success) OVERRIDE;
   virtual void OnUninitializeComplete() OVERRIDE;
   virtual void OnFlushComplete() OVERRIDE;
   virtual void OnSeekComplete() OVERRIDE;
@@ -105,7 +105,9 @@ class MEDIA_EXPORT FFmpegVideoDecoder
   // Hold video frames when flush happens.
   std::deque<scoped_refptr<VideoFrame> > frame_queue_flushed_;
 
-  VideoCodecInfo info_;
+  // TODO(scherkus): I think this should be calculated by VideoRenderers based
+  // on information provided by VideoDecoders (i.e., aspect ratio).
+  gfx::Size natural_size_;
 
   // Pointer to the demuxer stream that will feed us compressed buffers.
   scoped_refptr<DemuxerStream> demuxer_stream_;
diff --git a/media/filters/ffmpeg_video_decoder_unittest.cc b/media/filters/ffmpeg_video_decoder_unittest.cc
index 23d14f7..8b6a8e6 100644
--- a/media/filters/ffmpeg_video_decoder_unittest.cc
+++ b/media/filters/ffmpeg_video_decoder_unittest.cc
@@ -67,11 +67,9 @@ class MockVideoDecodeEngine : public VideoDecodeEngine {
   MOCK_METHOD0(Flush, void());
   MOCK_METHOD0(Seek, void());
 
-  MockVideoDecodeEngine() : event_handler_(NULL) {
-    memset(&info_, 0, sizeof(info_));
-  }
+  MockVideoDecodeEngine() : event_handler_(NULL) {}
+
   VideoDecodeEngine::EventHandler* event_handler_;
-  VideoCodecInfo info_;
 };
 
 // Class that just mocks the private functions.
@@ -97,9 +95,7 @@ class DecoderPrivateMock : public FFmpegVideoDecoder {
 
 ACTION_P2(EngineInitialize, engine, success) {
   engine->event_handler_ = arg1;
-  engine->info_.success = success;
-  engine->info_.natural_size = kNaturalSize;
-  engine->event_handler_->OnInitializeComplete(engine->info_);
+  engine->event_handler_->OnInitializeComplete(success);
 }
 
 ACTION_P(EngineUninitialize, engine) {
diff --git a/media/video/ffmpeg_video_decode_engine.cc b/media/video/ffmpeg_video_decode_engine.cc
index 9a89262..8d547b9 100644
--- a/media/video/ffmpeg_video_decode_engine.cc
+++ b/media/video/ffmpeg_video_decode_engine.cc
@@ -92,18 +92,17 @@ void FFmpegVideoDecodeEngine::Initialize(
   const CommandLine* cmd_line = CommandLine::ForCurrentProcess();
   std::string threads(cmd_line->GetSwitchValueASCII(switches::kVideoThreads));
   if ((!threads.empty() &&
-      !base::StringToInt(threads, &decode_threads)) ||
+       !base::StringToInt(threads, &decode_threads)) ||
       decode_threads < 0 || decode_threads > kMaxDecodeThreads) {
     decode_threads = kDecodeThreads;
   }
 
+  codec_context_->thread_count = decode_threads;
+
   // 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().
   av_frame_.reset(avcodec_alloc_frame());
-  VideoCodecInfo info;
-  info.success = false;
-  info.natural_size = config.natural_size();
 
   // If we do not have enough buffers, we will report error too.
   frame_queue_available_.clear();
@@ -119,14 +118,10 @@ void FFmpegVideoDecodeEngine::Initialize(
     frame_queue_available_.push_back(video_frame);
   }
 
-  codec_context_->thread_count = decode_threads;
-  if (codec &&
-      avcodec_open(codec_context_, codec) >= 0 &&
-      av_frame_.get()) {
-    info.success = true;
-  }
+  // Open the codec!
+  bool success = codec && avcodec_open(codec_context_, codec) >= 0;
   event_handler_ = event_handler;
-  event_handler_->OnInitializeComplete(info);
+  event_handler_->OnInitializeComplete(success);
 }
 
 void FFmpegVideoDecodeEngine::ConsumeVideoSample(
diff --git a/media/video/ffmpeg_video_decode_engine_unittest.cc b/media/video/ffmpeg_video_decode_engine_unittest.cc
index 2ab4b3f..581fb3d 100644
--- a/media/video/ffmpeg_video_decode_engine_unittest.cc
+++ b/media/video/ffmpeg_video_decode_engine_unittest.cc
@@ -36,7 +36,7 @@ class FFmpegVideoDecodeEngineTest
       public VideoDecodeEngine::EventHandler {
  public:
   FFmpegVideoDecodeEngineTest()
-      : config_(kCodecVP8, kCodedSize, kVisibleRect, kNaturalSize,
+      : config_(kCodecVP8, kCodedSize, kVisibleRect,
                 kFrameRate.num, kFrameRate.den, NULL, 0) {
     CHECK(FFmpegGlue::GetInstance());
 
@@ -56,11 +56,8 @@ class FFmpegVideoDecodeEngineTest
   }
 
   void Initialize() {
-    VideoCodecInfo info;
-    EXPECT_CALL(*this, OnInitializeComplete(_))
-        .WillOnce(SaveArg<0>(&info));
+    EXPECT_CALL(*this, OnInitializeComplete(true));
     test_engine_->Initialize(MessageLoop::current(), this, NULL, config_);
-    EXPECT_TRUE(info.success);
   }
 
   // Decodes the single compressed frame in |buffer| and writes the
@@ -114,12 +111,9 @@ class FFmpegVideoDecodeEngineTest
 
   // VideoDecodeEngine::EventHandler implementation.
   MOCK_METHOD2(ConsumeVideoFrame,
-               void(scoped_refptr<VideoFrame> video_frame,
-                    const PipelineStatistics& statistics));
-  MOCK_METHOD1(ProduceVideoSample,
-               void(scoped_refptr<Buffer> buffer));
-  MOCK_METHOD1(OnInitializeComplete,
-               void(const VideoCodecInfo& info));
+               void(scoped_refptr<VideoFrame>, const PipelineStatistics&));
+  MOCK_METHOD1(ProduceVideoSample, void(scoped_refptr<Buffer>));
+  MOCK_METHOD1(OnInitializeComplete, void(bool));
   MOCK_METHOD0(OnUninitializeComplete, void());
   MOCK_METHOD0(OnFlushComplete, void());
   MOCK_METHOD0(OnSeekComplete, void());
@@ -149,27 +143,21 @@ TEST_F(FFmpegVideoDecodeEngineTest, Initialize_Normal) {
 
 TEST_F(FFmpegVideoDecodeEngineTest, Initialize_FindDecoderFails) {
   VideoDecoderConfig config(kUnknownVideoCodec, kCodedSize, kVisibleRect,
-                            kNaturalSize, kFrameRate.num, kFrameRate.den,
+                            kFrameRate.num, kFrameRate.den,
                             NULL, 0);
 
   // Test avcodec_find_decoder() returning NULL.
-  VideoCodecInfo info;
-  EXPECT_CALL(*this, OnInitializeComplete(_))
-     .WillOnce(SaveArg<0>(&info));
+  EXPECT_CALL(*this, OnInitializeComplete(false));
   test_engine_->Initialize(MessageLoop::current(), this, NULL, config);
-  EXPECT_FALSE(info.success);
 }
 
 TEST_F(FFmpegVideoDecodeEngineTest, Initialize_OpenDecoderFails) {
   // Specify Theora w/o extra data so that avcodec_open() fails.
   VideoDecoderConfig config(kCodecTheora, kCodedSize, kVisibleRect,
-                            kNaturalSize, kFrameRate.num, kFrameRate.den,
+                            kFrameRate.num, kFrameRate.den,
                             NULL, 0);
-  VideoCodecInfo info;
-  EXPECT_CALL(*this, OnInitializeComplete(_))
-     .WillOnce(SaveArg<0>(&info));
+  EXPECT_CALL(*this, OnInitializeComplete(false));
   test_engine_->Initialize(MessageLoop::current(), this, NULL, config);
-  EXPECT_FALSE(info.success);
 }
 
 TEST_F(FFmpegVideoDecodeEngineTest, DecodeFrame_Normal) {
diff --git a/media/video/video_decode_engine.h b/media/video/video_decode_engine.h
index 4ec7cf5..cd1f986 100644
--- a/media/video/video_decode_engine.h
+++ b/media/video/video_decode_engine.h
@@ -21,20 +21,12 @@ class VideoDecodeContext;
 
 struct PipelineStatistics;
 
-struct VideoCodecInfo {
-  // Other parameter is only meaningful when this is true.
-  bool success;
-
-  // Natural dimensions of video. May be different from coded and visible sizes.
-  gfx::Size natural_size;
-};
-
 class MEDIA_EXPORT VideoDecodeEngine {
  public:
   struct MEDIA_EXPORT EventHandler {
    public:
     virtual ~EventHandler() {}
-    virtual void OnInitializeComplete(const VideoCodecInfo& info) = 0;
+    virtual void OnInitializeComplete(bool success) = 0;
     virtual void OnUninitializeComplete() = 0;
     virtual void OnFlushComplete() = 0;
     virtual void OnSeekComplete() = 0;