Remove VideoDecoderConfig::aspect_ratio_xxx methods.

BUG=122913

Review URL: https://chromiumcodereview.appspot.com/10830110

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

media/base/video_decoder_config.cc

@@ -4,8 +4,6 @@
 
 #include "media/base/video_decoder_config.h"
 
-#include <cmath>
-
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 
@@ -15,8 +13,6 @@ VideoDecoderConfig::VideoDecoderConfig()
     : codec_(kUnknownVideoCodec),
       profile_(VIDEO_CODEC_PROFILE_UNKNOWN),
       format_(VideoFrame::INVALID),
-      aspect_ratio_numerator_(0),
-      aspect_ratio_denominator_(0),
       extra_data_size_(0) {
 }
 
@@ -25,12 +21,10 @@ VideoDecoderConfig::VideoDecoderConfig(VideoCodec codec,
                                        VideoFrame::Format format,
                                        const gfx::Size& coded_size,
                                        const gfx::Rect& visible_rect,
-                                       int aspect_ratio_numerator,
-                                       int aspect_ratio_denominator,
+                                       const gfx::Size& natural_size,
                                        const uint8* extra_data,
                                        size_t extra_data_size) {
-  Initialize(codec, profile, format, coded_size, visible_rect,
-             aspect_ratio_numerator, aspect_ratio_denominator,
+  Initialize(codec, profile, format, coded_size, visible_rect, natural_size,
              extra_data, extra_data_size, true);
 }
 
@@ -63,8 +57,7 @@ void VideoDecoderConfig::Initialize(VideoCodec codec,
                                     VideoFrame::Format format,
                                     const gfx::Size& coded_size,
                                     const gfx::Rect& visible_rect,
-                                    int aspect_ratio_numerator,
-                                    int aspect_ratio_denominator,
+                                    const gfx::Size& natural_size,
                                     const uint8* extra_data,
                                     size_t extra_data_size,
                                     bool record_stats) {
@@ -88,8 +81,7 @@ void VideoDecoderConfig::Initialize(VideoCodec codec,
   format_ = format;
   coded_size_ = coded_size;
   visible_rect_ = visible_rect;
-  aspect_ratio_numerator_ = aspect_ratio_numerator;
-  aspect_ratio_denominator_ = aspect_ratio_denominator;
+  natural_size_ = natural_size;
   extra_data_size_ = extra_data_size;
 
   if (extra_data_size_ > 0) {
@@ -98,22 +90,6 @@ void VideoDecoderConfig::Initialize(VideoCodec codec,
   } else {
     extra_data_.reset();
   }
-
-  // Calculate the natural size given the aspect ratio and visible rect.
-  if (aspect_ratio_denominator == 0) {
-    natural_size_.SetSize(0, 0);
-    return;
-  }
-
-  double aspect_ratio = aspect_ratio_numerator /
-      static_cast<double>(aspect_ratio_denominator);
-
-  int width = floor(visible_rect.width() * aspect_ratio + 0.5);
-  int height = visible_rect.height();
-
-  // An even width makes things easier for YV12 and appears to be the behavior
-  // expected by WebKit layout tests.
-  natural_size_.SetSize(width & ~1, height);
 }
 
 void VideoDecoderConfig::CopyFrom(const VideoDecoderConfig& video_config) {
@@ -122,8 +98,7 @@ void VideoDecoderConfig::CopyFrom(const VideoDecoderConfig& video_config) {
              video_config.format(),
              video_config.coded_size(),
              video_config.visible_rect(),
-             video_config.aspect_ratio_numerator(),
-             video_config.aspect_ratio_denominator(),
+             video_config.natural_size(),
              video_config.extra_data(),
              video_config.extra_data_size(),
              false);
@@ -131,8 +106,8 @@ void VideoDecoderConfig::CopyFrom(const VideoDecoderConfig& video_config) {
 
 bool VideoDecoderConfig::IsValidConfig() const {
   return codec_ != kUnknownVideoCodec &&
-      aspect_ratio_numerator_ > 0 &&
-      aspect_ratio_denominator_ > 0 &&
+      natural_size_.width() > 0 &&
+      natural_size_.height() > 0 &&
       VideoFrame::IsValidConfig(
           format_, natural_size_.width(), natural_size_.height());
 }
@@ -160,9 +135,7 @@ std::string VideoDecoderConfig::AsHumanReadableString() const {
     << "," << visible_rect().width()
     << "," << visible_rect().height() << "]"
     << " natural size: [" << natural_size().width()
-    << "," << natural_size().height() << "]"
-    << " aspect ratio: " << aspect_ratio_numerator()
-    << "/" << aspect_ratio_denominator();
+    << "," << natural_size().height() << "]";
   return s.str();
 }
 
@@ -190,14 +163,6 @@ gfx::Size VideoDecoderConfig::natural_size() const {
   return natural_size_;
 }
 
-int VideoDecoderConfig::aspect_ratio_numerator() const {
-  return aspect_ratio_numerator_;
-}
-
-int VideoDecoderConfig::aspect_ratio_denominator() const {
-  return aspect_ratio_denominator_;
-}
-
 uint8* VideoDecoderConfig::extra_data() const {
   return extra_data_.get();
 }

media/base/video_decoder_config.h

@@ -72,7 +72,7 @@ class MEDIA_EXPORT VideoDecoderConfig {
                      VideoFrame::Format format,
                      const gfx::Size& coded_size,
                      const gfx::Rect& visible_rect,
-                     int aspect_ratio_numerator, int aspect_ratio_denominator,
+                     const gfx::Size& natural_size,
                      const uint8* extra_data, size_t extra_data_size);
 
   ~VideoDecoderConfig();
@@ -83,7 +83,7 @@ class MEDIA_EXPORT VideoDecoderConfig {
                   VideoFrame::Format format,
                   const gfx::Size& coded_size,
                   const gfx::Rect& visible_rect,
-                  int aspect_ratio_numerator, int aspect_ratio_denominator,
+                  const gfx::Size& natural_size,
                   const uint8* extra_data, size_t extra_data_size,
                   bool record_stats);
 
@@ -119,13 +119,6 @@ class MEDIA_EXPORT VideoDecoderConfig {
   // into account.
   gfx::Size natural_size() const;
 
-  // Aspect ratio of the decoded video frame expressed as a fraction.
-  //
-  // TODO(scherkus): think of a better way to avoid having video decoders
-  // handle tricky aspect ratio dimension calculations.
-  int aspect_ratio_numerator() const;
-  int aspect_ratio_denominator() const;
-
   // Optional byte data required to initialize video decoders, such as H.264
   // AAVC data.
   uint8* extra_data() const;
@@ -141,9 +134,6 @@ class MEDIA_EXPORT VideoDecoderConfig {
   gfx::Rect visible_rect_;
   gfx::Size natural_size_;
 
-  int aspect_ratio_numerator_;
-  int aspect_ratio_denominator_;
-
   scoped_array<uint8> extra_data_;
   size_t extra_data_size_;
 

media/base/video_util.cc

@@ -4,11 +4,32 @@
 
 #include "media/base/video_util.h"
 
+#include <cmath>
+
 #include "base/logging.h"
 #include "media/base/video_frame.h"
 
 namespace media {
 
+gfx::Size GetNaturalSize(const gfx::Size& visible_size,
+                         int aspect_ratio_numerator,
+                         int aspect_ratio_denominator) {
+  if (aspect_ratio_denominator == 0 ||
+      aspect_ratio_numerator < 0 ||
+      aspect_ratio_denominator < 0)
+    return gfx::Size();
+
+  double aspect_ratio = aspect_ratio_numerator /
+      static_cast<double>(aspect_ratio_denominator);
+
+  int width = floor(visible_size.width() * aspect_ratio + 0.5);
+  int height = visible_size.height();
+
+  // An even width makes things easier for YV12 and appears to be the behavior
+  // expected by WebKit layout tests.
+  return gfx::Size(width & ~1, height);
+}
+
 static void CopyPlane(size_t plane, const uint8* source, int stride, int rows,
                       VideoFrame* frame) {
   uint8* dest = frame->data(plane);

media/base/video_util.h

@@ -7,11 +7,17 @@
 
 #include "base/basictypes.h"
 #include "media/base/media_export.h"
+#include "ui/gfx/size.h"
 
 namespace media {
 
 class VideoFrame;
 
+// Computes the size of |visible_size| for a given aspect ratio.
+MEDIA_EXPORT gfx::Size GetNaturalSize(const gfx::Size& visible_size,
+                                      int aspect_ratio_numerator,
+                                      int aspect_ratio_denominator);
+
 // Copies a plane of YUV source into a VideoFrame object, taking into account
 // source and destinations dimensions.
 //

media/ffmpeg/ffmpeg_common.cc

@@ -5,6 +5,7 @@
 #include "media/ffmpeg/ffmpeg_common.h"
 
 #include "base/logging.h"
+#include "media/base/video_util.h"
 
 namespace media {
 
@@ -250,12 +251,12 @@ void AVStreamToVideoDecoderConfig(
   VideoCodec codec = CodecIDToVideoCodec(stream->codec->codec_id);
   VideoCodecProfile profile = (codec == kCodecVP8) ? VP8PROFILE_MAIN :
       ProfileIDToVideoCodecProfile(stream->codec->profile);
+  gfx::Size natural_size = GetNaturalSize(
+      visible_rect.size(), aspect_ratio.num, aspect_ratio.den);
   config->Initialize(codec,
                      profile,
                      PixelFormatToVideoFormat(stream->codec->pix_fmt),
-                     coded_size, visible_rect,
-                     aspect_ratio.num,
-                     aspect_ratio.den,
+                     coded_size, visible_rect, natural_size,
                      stream->codec->extradata,
                      stream->codec->extradata_size,
                      true);

media/filters/ffmpeg_demuxer_unittest.cc

@@ -242,8 +242,8 @@ TEST_F(FFmpegDemuxerTest, Initialize_Successful) {
   EXPECT_EQ(0, video_config.visible_rect().y());
   EXPECT_EQ(320, video_config.visible_rect().width());
   EXPECT_EQ(240, video_config.visible_rect().height());
-  EXPECT_EQ(1, video_config.aspect_ratio_numerator());
-  EXPECT_EQ(1, video_config.aspect_ratio_denominator());
+  EXPECT_EQ(320, video_config.natural_size().width());
+  EXPECT_EQ(240, video_config.natural_size().height());
   EXPECT_FALSE(video_config.extra_data());
   EXPECT_EQ(0u, video_config.extra_data_size());
 

media/filters/ffmpeg_video_decoder_unittest.cc

@@ -18,6 +18,7 @@
 #include "media/base/test_data_util.h"
 #include "media/base/video_decoder.h"
 #include "media/base/video_frame.h"
+#include "media/base/video_util.h"
 #include "media/ffmpeg/ffmpeg_common.h"
 #include "media/filters/ffmpeg_decoder_unittest.h"
 #include "media/filters/ffmpeg_glue.h"
@@ -36,7 +37,7 @@ namespace media {
 static const VideoFrame::Format kVideoFormat = VideoFrame::YV12;
 static const gfx::Size kCodedSize(320, 240);
 static const gfx::Rect kVisibleRect(320, 240);
-static const AVRational kAspectRatio = { 1, 1 };
+static const gfx::Size kNaturalSize(320, 240);
 static const uint8 kFakeKeyId[] = { 0x4b, 0x65, 0x79, 0x20, 0x49, 0x44 };
 static const uint8 kFakeIv[DecryptConfig::kDecryptionKeySize] = { 0 };
 static const uint8 kFakeCheckSum[] = { 0, 0 };
@@ -87,8 +88,7 @@ class FFmpegVideoDecoderTest : public testing::Test {
     encrypted_i_frame_buffer_ = CreateFakeEncryptedBuffer();
 
     config_.Initialize(kCodecVP8, VIDEO_CODEC_PROFILE_UNKNOWN,
-                       kVideoFormat, kCodedSize, kVisibleRect,
-                       kAspectRatio.num, kAspectRatio.den,
+                       kVideoFormat, kCodedSize, kVisibleRect, kNaturalSize,
                        NULL, 0, true);
   }
 
@@ -246,8 +246,7 @@ TEST_F(FFmpegVideoDecoderTest, Initialize_UnsupportedDecoder) {
   // Test avcodec_find_decoder() returning NULL.
   VideoDecoderConfig config(kUnknownVideoCodec, VIDEO_CODEC_PROFILE_UNKNOWN,
                             kVideoFormat,
-                            kCodedSize, kVisibleRect,
-                            kAspectRatio.num, kAspectRatio.den,
+                            kCodedSize, kVisibleRect, kNaturalSize,
                             NULL, 0);
   InitializeWithConfigAndStatus(config, PIPELINE_ERROR_DECODE);
 }
@@ -256,8 +255,7 @@ TEST_F(FFmpegVideoDecoderTest, Initialize_UnsupportedPixelFormat) {
   // Ensure decoder handles unsupport pixel formats without crashing.
   VideoDecoderConfig config(kCodecVP8, VIDEO_CODEC_PROFILE_UNKNOWN,
                             VideoFrame::INVALID,
-                            kCodedSize, kVisibleRect,
-                            kAspectRatio.num, kAspectRatio.den,
+                            kCodedSize, kVisibleRect, kNaturalSize,
                             NULL, 0);
   InitializeWithConfigAndStatus(config, PIPELINE_ERROR_DECODE);
 }
@@ -266,8 +264,64 @@ TEST_F(FFmpegVideoDecoderTest, Initialize_OpenDecoderFails) {
   // Specify Theora w/o extra data so that avcodec_open2() fails.
   VideoDecoderConfig config(kCodecTheora, VIDEO_CODEC_PROFILE_UNKNOWN,
                             kVideoFormat,
-                            kCodedSize, kVisibleRect,
-                            kAspectRatio.num, kAspectRatio.den,
+                            kCodedSize, kVisibleRect, kNaturalSize,
+                            NULL, 0);
+  InitializeWithConfigAndStatus(config, PIPELINE_ERROR_DECODE);
+}
+
+TEST_F(FFmpegVideoDecoderTest, Initialize_AspectRatioNumeratorZero) {
+  gfx::Size natural_size = GetNaturalSize(kVisibleRect.size(), 0, 1);
+  VideoDecoderConfig config(kCodecVP8, VP8PROFILE_MAIN,
+                            kVideoFormat,
+                            kCodedSize, kVisibleRect, natural_size,
+                            NULL, 0);
+  InitializeWithConfigAndStatus(config, PIPELINE_ERROR_DECODE);
+}
+
+TEST_F(FFmpegVideoDecoderTest, Initialize_AspectRatioDenominatorZero) {
+  gfx::Size natural_size = GetNaturalSize(kVisibleRect.size(), 1, 0);
+  VideoDecoderConfig config(kCodecVP8, VP8PROFILE_MAIN,
+                            kVideoFormat,
+                            kCodedSize, kVisibleRect, natural_size,
+                            NULL, 0);
+  InitializeWithConfigAndStatus(config, PIPELINE_ERROR_DECODE);
+}
+
+TEST_F(FFmpegVideoDecoderTest, Initialize_AspectRatioNumeratorNegative) {
+  gfx::Size natural_size = GetNaturalSize(kVisibleRect.size(), -1, 1);
+  VideoDecoderConfig config(kCodecVP8, VP8PROFILE_MAIN,
+                            kVideoFormat,
+                            kCodedSize, kVisibleRect, natural_size,
+                            NULL, 0);
+  InitializeWithConfigAndStatus(config, PIPELINE_ERROR_DECODE);
+}
+
+TEST_F(FFmpegVideoDecoderTest, Initialize_AspectRatioDenominatorNegative) {
+  gfx::Size natural_size = GetNaturalSize(kVisibleRect.size(), 1, -1);
+  VideoDecoderConfig config(kCodecVP8, VP8PROFILE_MAIN,
+                            kVideoFormat,
+                            kCodedSize, kVisibleRect, natural_size,
+                            NULL, 0);
+  InitializeWithConfigAndStatus(config, PIPELINE_ERROR_DECODE);
+}
+
+TEST_F(FFmpegVideoDecoderTest, Initialize_AspectRatioNumeratorTooLarge) {
+  int width = kVisibleRect.size().width();
+  int num = ceil(static_cast<double>(limits::kMaxDimension + 1) / width);
+  gfx::Size natural_size = GetNaturalSize(kVisibleRect.size(), num, 1);
+  VideoDecoderConfig config(kCodecVP8, VP8PROFILE_MAIN,
+                            kVideoFormat,
+                            kCodedSize, kVisibleRect, natural_size,
+                            NULL, 0);
+  InitializeWithConfigAndStatus(config, PIPELINE_ERROR_DECODE);
+}
+
+TEST_F(FFmpegVideoDecoderTest, Initialize_AspectRatioDenominatorTooLarge) {
+  int den = kVisibleRect.size().width() + 1;
+  gfx::Size natural_size = GetNaturalSize(kVisibleRect.size(), 1, den);
+  VideoDecoderConfig config(kCodecVP8, VP8PROFILE_MAIN,
+                            kVideoFormat,
+                            kCodedSize, kVisibleRect, natural_size,
                             NULL, 0);
   InitializeWithConfigAndStatus(config, PIPELINE_ERROR_DECODE);
 }

media/filters/pipeline_integration_test.cc

@@ -283,6 +283,13 @@ TEST_F(PipelineIntegrationTest, BasicPlayback_MediaSource) {
   Stop();
 }
 
+TEST_F(PipelineIntegrationTest, BasicPlayback_16x9AspectRatio) {
+  ASSERT_TRUE(Start(GetTestDataURL("bear-320x240-16x9-aspect.webm"),
+                    PIPELINE_OK));
+  Play();
+  ASSERT_TRUE(WaitUntilOnEnded());
+}
+
 // TODO(fgalligan): Enable test when encrypted test data is updated and new
 // decryption code is landed. http://crbug.com/132801
 TEST_F(PipelineIntegrationTest, DISABLED_EncryptedPlayback) {

media/mp4/mp4_stream_parser.cc

@@ -11,6 +11,7 @@
 #include "media/base/audio_decoder_config.h"
 #include "media/base/stream_parser_buffer.h"
 #include "media/base/video_decoder_config.h"
+#include "media/base/video_util.h"
 #include "media/mp4/box_definitions.h"
 #include "media/mp4/box_reader.h"
 #include "media/mp4/es_descriptor.h"
@@ -218,14 +219,16 @@ bool MP4StreamParser::ParseMoov(BoxReader* reader) {
       RCHECK(EmitKeyNeeded(entry.sinf.info.track_encryption));
 
       // TODO(strobe): Recover correct crop box
+      gfx::Size coded_size(entry.width, entry.height);
+      gfx::Rect visible_rect(coded_size);
+      gfx::Size natural_size = GetNaturalSize(visible_rect.size(),
+                                              entry.pixel_aspect.h_spacing,
+                                              entry.pixel_aspect.v_spacing);
       video_config.Initialize(kCodecH264, H264PROFILE_MAIN,  VideoFrame::YV12,
-                              gfx::Size(entry.width, entry.height),
-                              gfx::Rect(0, 0, entry.width, entry.height),
-                              entry.pixel_aspect.h_spacing,
-                              entry.pixel_aspect.v_spacing,
+                              coded_size, visible_rect, natural_size,
                               // No decoder-specific buffer needed for AVC;
                               // SPS/PPS are embedded in the video stream
-                              NULL, 0, false);
+                              NULL, 0, true);
       has_video_ = true;
       video_track_id_ = track->header.track_id;
     }