[media] Scale |natural_size| to match |visible_rect| for HW decoders.

For some media, the container's |natural_size| is actually just an
aspect ratio, and therefore we should not use it directly as the
VideoFrame's |natural_size|. Instead it should be scaled based on the
|visible_rect| of the decoded frame.

FFmpegVideoDecoder already does this, this CL applies the same logic in
GpuVideoDecoder and VdaVideoDecoder.

Note: This change will make it possible for HW decoders to emit frames
of different sizes without a config change, which was not possible
before.

Bug: 766657
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I7b263245c401c845bca30fa0683eb3e337ed6f81
Reviewed-on: https://chromium-review.googlesource.com/1022968
Commit-Queue: Dan Sanders <sandersd@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553384}

media/base/video_util.cc

@@ -73,6 +73,32 @@ gfx::Size GetNaturalSize(const gfx::Size& visible_size,
                    round(visible_size.height() / aspect_ratio));
 }
 
+gfx::Size GetNaturalSizeWithDAR(const gfx::Size& visible_size,
+                                const gfx::Size& display_aspect) {
+  // No reasonable aspect interpretation, return an empty size.
+  // TODO(sandersd): Is it more useful to return the original |visible_size|?
+  if (visible_size.width() <= 0 || visible_size.height() <= 0 ||
+      display_aspect.width() <= 0 || display_aspect.height() <= 0) {
+    return gfx::Size();
+  }
+
+  double visible_aspect_ratio =
+      visible_size.width() / static_cast<double>(visible_size.height());
+
+  double display_aspect_ratio =
+      display_aspect.width() / static_cast<double>(display_aspect.height());
+
+  if (display_aspect_ratio > visible_aspect_ratio) {
+    // |display_aspect| is wider than |visible_size|; increase width.
+    return gfx::Size(round(visible_size.height() * display_aspect_ratio),
+                     visible_size.height());
+  }
+
+  // |display_aspect| is narrower than |visible_size|; increase height.
+  return gfx::Size(visible_size.width(),
+                   round(visible_size.width() / display_aspect_ratio));
+}
+
 void FillYUV(VideoFrame* frame, uint8_t y, uint8_t u, uint8_t v) {
   // Fill the Y plane.
   uint8_t* y_plane = frame->data(VideoFrame::kYPlane);

media/base/video_util.h

@@ -16,11 +16,22 @@ namespace media {
 
 class VideoFrame;
 
-// Computes the size of |visible_size| for a given aspect ratio.
+// Computes the size of |visible_size| for a given sample aspect ratio.
+//
+// TODO(sandersd): Rename as GetNaturalSizeWithSAR() to make it more clear
+// at the call site what this does. Perhaps some wrapper classes would be
+// best:
+//   GetNaturalSize(visible_size, SampleAspectRatio(n, d))
+//   GetNaturalSize(visible_size, DisplayAspectRatio(w, h))
 MEDIA_EXPORT gfx::Size GetNaturalSize(const gfx::Size& visible_size,
                                       int aspect_ratio_numerator,
                                       int aspect_ratio_denominator);
 
+// Increases (at most) one of the dimensions of |visible_size| such that
+// the display aspect ratio matches |display_aspect|.
+MEDIA_EXPORT gfx::Size GetNaturalSizeWithDAR(const gfx::Size& visible_size,
+                                             const gfx::Size& display_aspect);
+
 // Fills |frame| containing YUV data to the given color values.
 MEDIA_EXPORT void FillYUV(VideoFrame* frame, uint8_t y, uint8_t u, uint8_t v);
 

media/base/video_util_unittest.cc

@@ -219,6 +219,38 @@ TEST_F(VideoUtilTest, GetNaturalSize) {
   EXPECT_EQ(gfx::Size(320, 371), GetNaturalSize(visible_size, 11, 17));
 }
 
+TEST_F(VideoUtilTest, GetNaturalSizeWithDAR) {
+  gfx::Size visible_size(320, 240);
+
+  // Test 0 sizes.
+  EXPECT_EQ(gfx::Size(), GetNaturalSizeWithDAR(gfx::Size(0, 0), visible_size));
+  EXPECT_EQ(gfx::Size(), GetNaturalSizeWithDAR(gfx::Size(0, 1), visible_size));
+  EXPECT_EQ(gfx::Size(), GetNaturalSizeWithDAR(gfx::Size(1, 0), visible_size));
+
+  // Test abnormal ratios.
+  EXPECT_EQ(gfx::Size(), GetNaturalSizeWithDAR(visible_size, gfx::Size(0, 0)));
+  EXPECT_EQ(gfx::Size(), GetNaturalSizeWithDAR(visible_size, gfx::Size(0, 1)));
+  EXPECT_EQ(gfx::Size(), GetNaturalSizeWithDAR(visible_size, gfx::Size(1, 0)));
+  EXPECT_EQ(gfx::Size(), GetNaturalSizeWithDAR(visible_size, gfx::Size(1, -1)));
+  EXPECT_EQ(gfx::Size(), GetNaturalSizeWithDAR(visible_size, gfx::Size(-1, 1)));
+
+  // Test normal sizes and ratios.
+  EXPECT_EQ(gfx::Size(320, 320),
+            GetNaturalSizeWithDAR(visible_size, gfx::Size(1, 1)));
+  EXPECT_EQ(gfx::Size(480, 240),
+            GetNaturalSizeWithDAR(visible_size, gfx::Size(2, 1)));
+  EXPECT_EQ(gfx::Size(320, 640),
+            GetNaturalSizeWithDAR(visible_size, gfx::Size(1, 2)));
+  EXPECT_EQ(gfx::Size(320, 240),
+            GetNaturalSizeWithDAR(visible_size, gfx::Size(4, 3)));
+  EXPECT_EQ(gfx::Size(320, 427),
+            GetNaturalSizeWithDAR(visible_size, gfx::Size(3, 4)));
+  EXPECT_EQ(gfx::Size(427, 240),
+            GetNaturalSizeWithDAR(visible_size, gfx::Size(16, 9)));
+  EXPECT_EQ(gfx::Size(320, 569),
+            GetNaturalSizeWithDAR(visible_size, gfx::Size(9, 16)));
+}
+
 namespace {
 
 uint8_t src6x4[] = {0,  1,  2,  3,  4,  5,  6,  7,  8,  9,  10, 11,

media/filters/gpu_video_decoder.cc

@@ -29,6 +29,7 @@
 #include "media/base/pipeline_status.h"
 #include "media/base/surface_manager.h"
 #include "media/base/video_decoder_config.h"
+#include "media/base/video_util.h"
 #include "media/media_buildflags.h"
 #include "media/video/gpu_video_accelerator_factories.h"
 #include "third_party/skia/include/core/SkBitmap.h"
@@ -651,7 +652,8 @@ void GpuVideoDecoder::PictureReady(const media::Picture& picture) {
       BindToCurrentLoop(base::Bind(
           &GpuVideoDecoder::ReleaseMailbox, weak_factory_.GetWeakPtr(),
           factories_, picture.picture_buffer_id(), pb.client_texture_ids())),
-      pb.size(), visible_rect, natural_size, timestamp));
+      pb.size(), visible_rect,
+      GetNaturalSizeWithDAR(visible_rect.size(), natural_size), timestamp));
   if (!frame) {
     DLOG(ERROR) << "Create frame failed for: " << picture.picture_buffer_id();
     NotifyError(VideoDecodeAccelerator::PLATFORM_FAILURE);

media/gpu/fake_command_buffer_helper.cc

@@ -4,15 +4,22 @@
 
 #include "media/gpu/fake_command_buffer_helper.h"
 
+#include "base/logging.h"
+
 namespace media {
 
 FakeCommandBufferHelper::FakeCommandBufferHelper(
     scoped_refptr<base::SingleThreadTaskRunner> task_runner)
-    : task_runner_(std::move(task_runner)) {}
+    : task_runner_(std::move(task_runner)) {
+  DVLOG(1) << __func__;
+}
 
-FakeCommandBufferHelper::~FakeCommandBufferHelper() = default;
+FakeCommandBufferHelper::~FakeCommandBufferHelper() {
+  DVLOG(1) << __func__;
+}
 
 void FakeCommandBufferHelper::StubLost() {
+  DVLOG(1) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   has_stub_ = false;
   is_context_lost_ = true;
@@ -22,22 +29,26 @@ void FakeCommandBufferHelper::StubLost() {
 }
 
 void FakeCommandBufferHelper::ContextLost() {
+  DVLOG(1) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   is_context_lost_ = true;
   is_context_current_ = false;
 }
 
 void FakeCommandBufferHelper::CurrentContextLost() {
+  DVLOG(2) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   is_context_current_ = false;
 }
 
 bool FakeCommandBufferHelper::HasTexture(GLuint service_id) {
+  DVLOG(4) << __func__ << "(" << service_id << ")";
   DCHECK(task_runner_->BelongsToCurrentThread());
   return service_ids_.count(service_id);
 }
 
 void FakeCommandBufferHelper::ReleaseSyncToken(gpu::SyncToken sync_token) {
+  DVLOG(3) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(waits_.count(sync_token));
   task_runner_->PostTask(FROM_HERE, std::move(waits_[sync_token]));
@@ -45,11 +56,13 @@ void FakeCommandBufferHelper::ReleaseSyncToken(gpu::SyncToken sync_token) {
 }
 
 gl::GLContext* FakeCommandBufferHelper::GetGLContext() {
+  DVLOG(4) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   return nullptr;
 }
 
 bool FakeCommandBufferHelper::MakeContextCurrent() {
+  DVLOG(3) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   is_context_current_ = !is_context_lost_;
   return is_context_current_;
@@ -61,6 +74,7 @@ GLuint FakeCommandBufferHelper::CreateTexture(GLenum target,
                                               GLsizei height,
                                               GLenum format,
                                               GLenum type) {
+  DVLOG(2) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(is_context_current_);
   GLuint service_id = next_service_id_++;
@@ -69,6 +83,7 @@ GLuint FakeCommandBufferHelper::CreateTexture(GLenum target,
 }
 
 void FakeCommandBufferHelper::DestroyTexture(GLuint service_id) {
+  DVLOG(2) << __func__ << "(" << service_id << ")";
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(is_context_current_);
   DCHECK(service_ids_.count(service_id));
@@ -76,6 +91,7 @@ void FakeCommandBufferHelper::DestroyTexture(GLuint service_id) {
 }
 
 void FakeCommandBufferHelper::SetCleared(GLuint service_id) {
+  DVLOG(2) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(service_ids_.count(service_id));
 }
@@ -83,6 +99,7 @@ void FakeCommandBufferHelper::SetCleared(GLuint service_id) {
 bool FakeCommandBufferHelper::BindImage(GLuint service_id,
                                         gl::GLImage* image,
                                         bool can_bind_to_sampler) {
+  DVLOG(2) << __func__ << "(" << service_id << ")";
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(service_ids_.count(service_id));
   DCHECK(image);
@@ -90,16 +107,21 @@ bool FakeCommandBufferHelper::BindImage(GLuint service_id,
 }
 
 gpu::Mailbox FakeCommandBufferHelper::CreateMailbox(GLuint service_id) {
+  DVLOG(2) << __func__ << "(" << service_id << ")";
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(service_ids_.count(service_id));
+  if (!has_stub_)
+    return gpu::Mailbox();
   return gpu::Mailbox::Generate();
 }
 
 void FakeCommandBufferHelper::WaitForSyncToken(gpu::SyncToken sync_token,
                                                base::OnceClosure done_cb) {
+  DVLOG(2) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(!waits_.count(sync_token));
-  waits_.emplace(sync_token, std::move(done_cb));
+  if (has_stub_)
+    waits_.emplace(sync_token, std::move(done_cb));
 }
 
 }  // namespace media

media/gpu/ipc/service/picture_buffer_manager_unittest.cc

@@ -47,7 +47,14 @@ class PictureBufferManagerImplTest : public testing::Test {
     pbm_ = PictureBufferManager::Create(reuse_cb_.Get());
   }
 
-  ~PictureBufferManagerImplTest() override {}
+  ~PictureBufferManagerImplTest() override {
+    // Drop ownership of anything that may have an async destruction process,
+    // then allow destruction to complete.
+    cbh_->StubLost();
+    cbh_ = nullptr;
+    pbm_ = nullptr;
+    environment_.RunUntilIdle();
+  }
 
  protected:
   void Initialize() {

media/gpu/ipc/service/vda_video_decoder.cc

@@ -19,6 +19,7 @@
 #include "media/base/decoder_buffer.h"
 #include "media/base/video_codecs.h"
 #include "media/base/video_types.h"
+#include "media/base/video_util.h"
 #include "media/gpu/gpu_video_accelerator_util.h"
 #include "media/gpu/gpu_video_decode_accelerator_factory.h"
 #include "media/video/picture.h"
@@ -527,7 +528,8 @@ void VdaVideoDecoder::PictureReadyOnParentThread(Picture picture) {
 
   // Create a VideoFrame for the picture.
   scoped_refptr<VideoFrame> frame = picture_buffer_manager_->CreateVideoFrame(
-      picture, timestamp_it->second, visible_rect, config_.natural_size());
+      picture, timestamp_it->second, visible_rect,
+      GetNaturalSizeWithDAR(visible_rect.size(), config_.natural_size()));
   if (!frame) {
     EnterErrorState();
     return;

media/gpu/ipc/service/vda_video_decoder_unittest.cc

@@ -121,6 +121,7 @@ class VdaVideoDecoderTest : public testing::Test {
   ~VdaVideoDecoderTest() override {
     // Drop ownership of anything that may have an async destruction process,
     // then allow destruction to complete.
+    cbh_->StubLost();
     cbh_ = nullptr;
     owned_vda_ = nullptr;
     pbm_ = nullptr;
@@ -353,6 +354,36 @@ TEST_F(VdaVideoDecoderTest, Decode_OutputAndDismiss) {
   environment_.RunUntilIdle();
 }
 
+TEST_F(VdaVideoDecoderTest, Decode_Output_MaintainsAspect) {
+  Initialize();
+  int32_t bitstream_id = Decode(base::TimeDelta());
+  NotifyEndOfBitstreamBuffer(bitstream_id);
+  int32_t picture_buffer_id = ProvidePictureBuffer();
+
+  // Ask VdaVideoDecoder to produce a frame with:
+  //   - |natural_size| = 1920x1080 (VideoDecoderConfig)
+  //   - |coded_size| = 1920x1088 (PictureBuffer)
+  //   - |visible_rect| = 640x480 (Picture)
+  // VdaVideoDecoder should produce a frame with:
+  //   - |natural_size| = 853x480 (picture aspect ratio matches |natural_size|)
+  //   - |coded_size| = 1920x1088 (must match PictureBuffer)
+  //   - |visible_rect| = 640x480 (must match Picture)
+  scoped_refptr<VideoFrame> frame;
+  EXPECT_CALL(output_cb_, Run(_)).WillOnce(SaveArg<0>(&frame));
+  client_->PictureReady(Picture(picture_buffer_id, bitstream_id,
+                                gfx::Rect(640, 480),
+                                gfx::ColorSpace::CreateSRGB(), true));
+  environment_.RunUntilIdle();
+
+  ASSERT_TRUE(frame);
+  EXPECT_EQ(frame->natural_size().width(), 853);
+  EXPECT_EQ(frame->natural_size().height(), 480);
+  EXPECT_EQ(frame->coded_size().width(), 1920);
+  EXPECT_EQ(frame->coded_size().height(), 1088);
+  EXPECT_EQ(frame->visible_rect().width(), 640);
+  EXPECT_EQ(frame->visible_rect().height(), 480);
+}
+
 TEST_F(VdaVideoDecoderTest, Flush) {
   Initialize();
   vdavd_->Decode(DecoderBuffer::CreateEOSBuffer(), decode_cb_.Get());