media/gpu/vaapi/AcceleratedVideoEncoder: GetPicture() returns always the same instance

AcceleratedVideoEncoder implemented in VaapiVideoEncodeAccelerator
returns one-time Picture on GetPicture(). In other words,
different Picture instances returns every GetPicture(). This
behavior likely leads to a bug. In fact, vp9 metadata is not filled
because it attaches the metadata in the returned Picture and
refers to it later but the pictures are different.
This CL fixes the interface so that it will always returns the same
instance.

Bug: 1030199
Test: tast run webrtc.* on atlas
Test: video_encode_accelerator_tests on atlas
Change-Id: I5ec1cb17e032566ed082469331f89a380d86148f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2371085
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#801697}

media/gpu/vaapi/vaapi_video_encode_accelerator.cc

@@ -103,7 +103,7 @@ class VaapiEncodeJob : public AcceleratedVideoEncoder::EncodeJob {
                  bool keyframe,
                  base::OnceClosure execute_cb,
                  scoped_refptr<VASurface> input_surface,
-                 scoped_refptr<VASurface> reconstructed_surface,
+                 scoped_refptr<CodecPicture> picture,
                  VABufferID coded_buffer_id);
   ~VaapiEncodeJob() override = default;
 
@@ -113,17 +113,13 @@ class VaapiEncodeJob : public AcceleratedVideoEncoder::EncodeJob {
   const scoped_refptr<VASurface> input_surface() const {
     return input_surface_;
   }
-  const scoped_refptr<VASurface> reconstructed_surface() const {
-    return reconstructed_surface_;
-  }
+  const scoped_refptr<CodecPicture> picture() const { return picture_; }
 
  private:
   // Input surface for video frame data or scaled data.
   const scoped_refptr<VASurface> input_surface_;
 
-  // Surface for the reconstructed picture, used for reference
-  // for subsequent frames.
-  const scoped_refptr<VASurface> reconstructed_surface_;
+  const scoped_refptr<CodecPicture> picture_;
 
   // Buffer that will contain the output bitstream data for this frame.
   VABufferID coded_buffer_id_;
@@ -259,6 +255,11 @@ VaapiVideoEncodeAccelerator::~VaapiVideoEncodeAccelerator() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(encoder_sequence_checker_);
 }
 
+CodecPicture* VaapiVideoEncodeAccelerator::GetPictureFromJobForTesting(
+    VaapiEncodeJob* job) {
+  return job->picture().get();
+}
+
 bool VaapiVideoEncodeAccelerator::Initialize(const Config& config,
                                              Client* client) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(child_sequence_checker_);
@@ -355,10 +356,10 @@ void VaapiVideoEncodeAccelerator::InitializeTask(const Config& config) {
   DCHECK_EQ(state_, kUninitialized);
   VLOGF(2);
 
-  VideoCodec codec = VideoCodecProfileToVideoCodec(config.output_profile);
+  output_codec_ = VideoCodecProfileToVideoCodec(config.output_profile);
   AcceleratedVideoEncoder::Config ave_config{};
   DCHECK_EQ(IsConfiguredForTesting(), !!encoder_);
-  switch (codec) {
+  switch (output_codec_) {
     case kCodecH264:
       if (!IsConfiguredForTesting()) {
         encoder_ = std::make_unique<H264Encoder>(
@@ -384,7 +385,7 @@ void VaapiVideoEncodeAccelerator::InitializeTask(const Config& config) {
           kConstantQuantizationParameter;
       break;
     default:
-      NOTREACHED() << "Unsupported codec type " << GetCodecName(codec);
+      NOTREACHED() << "Unsupported codec type " << GetCodecName(output_codec_);
       return;
   }
 
@@ -769,11 +770,26 @@ std::unique_ptr<VaapiEncodeJob> VaapiVideoEncodeAccelerator::CreateEncodeJob(
                     kVaSurfaceFormat, base::BindOnce(va_surface_release_cb_));
   available_va_surface_ids_.pop_back();
 
+  scoped_refptr<CodecPicture> picture;
+  switch (output_codec_) {
+    case kCodecH264:
+      picture = new VaapiH264Picture(std::move(reconstructed_surface));
+      break;
+    case kCodecVP8:
+      picture = new VaapiVP8Picture(std::move(reconstructed_surface));
+      break;
+    case kCodecVP9:
+      picture = new VaapiVP9Picture(std::move(reconstructed_surface));
+      break;
+    default:
+      return nullptr;
+  }
+
   auto job = std::make_unique<VaapiEncodeJob>(
       frame, force_keyframe,
       base::BindOnce(&VaapiVideoEncodeAccelerator::ExecuteEncode,
                      encoder_weak_this_, input_surface->id()),
-      input_surface, std::move(reconstructed_surface), coded_buffer_id);
+      input_surface, std::move(picture), coded_buffer_id);
 
   if (!native_input_mode_) {
     job->AddSetupCallback(base::BindOnce(
@@ -1000,14 +1016,14 @@ VaapiEncodeJob::VaapiEncodeJob(scoped_refptr<VideoFrame> input_frame,
                                bool keyframe,
                                base::OnceClosure execute_cb,
                                scoped_refptr<VASurface> input_surface,
-                               scoped_refptr<VASurface> reconstructed_surface,
+                               scoped_refptr<CodecPicture> picture,
                                VABufferID coded_buffer_id)
     : EncodeJob(input_frame, keyframe, std::move(execute_cb)),
       input_surface_(input_surface),
-      reconstructed_surface_(reconstructed_surface),
+      picture_(std::move(picture)),
       coded_buffer_id_(coded_buffer_id) {
   DCHECK(input_surface_);
-  DCHECK(reconstructed_surface_);
+  DCHECK(picture_);
   DCHECK_NE(coded_buffer_id_, VA_INVALID_ID);
 }
 
@@ -1200,8 +1216,8 @@ bool VaapiVideoEncodeAccelerator::H264Accelerator::SubmitFrameParameters(
 scoped_refptr<H264Picture>
 VaapiVideoEncodeAccelerator::H264Accelerator::GetPicture(
     AcceleratedVideoEncoder::EncodeJob* job) {
-  return base::MakeRefCounted<VaapiH264Picture>(
-      job->AsVaapiEncodeJob()->reconstructed_surface());
+  return base::WrapRefCounted(
+      reinterpret_cast<H264Picture*>(job->AsVaapiEncodeJob()->picture().get()));
 }
 
 bool VaapiVideoEncodeAccelerator::H264Accelerator::SubmitPackedHeaders(
@@ -1242,8 +1258,8 @@ bool VaapiVideoEncodeAccelerator::H264Accelerator::SubmitPackedHeaders(
 scoped_refptr<VP8Picture>
 VaapiVideoEncodeAccelerator::VP8Accelerator::GetPicture(
     AcceleratedVideoEncoder::EncodeJob* job) {
-  return base::MakeRefCounted<VaapiVP8Picture>(
-      job->AsVaapiEncodeJob()->reconstructed_surface());
+  return base::WrapRefCounted(
+      reinterpret_cast<VP8Picture*>(job->AsVaapiEncodeJob()->picture().get()));
 }
 
 bool VaapiVideoEncodeAccelerator::VP8Accelerator::SubmitFrameParameters(
@@ -1417,8 +1433,8 @@ bool VaapiVideoEncodeAccelerator::VP8Accelerator::SubmitFrameParameters(
 scoped_refptr<VP9Picture>
 VaapiVideoEncodeAccelerator::VP9Accelerator::GetPicture(
     AcceleratedVideoEncoder::EncodeJob* job) {
-  return base::MakeRefCounted<VaapiVP9Picture>(
-      job->AsVaapiEncodeJob()->reconstructed_surface());
+  return base::WrapRefCounted(
+      reinterpret_cast<VP9Picture*>(job->AsVaapiEncodeJob()->picture().get()));
 }
 
 bool VaapiVideoEncodeAccelerator::VP9Accelerator::SubmitFrameParameters(

media/gpu/vaapi/vaapi_video_encode_accelerator.h

@@ -158,6 +158,8 @@ class MEDIA_GPU_EXPORT VaapiVideoEncodeAccelerator
     return !supported_profiles_for_testing_.empty();
   }
 
+  static CodecPicture* GetPictureFromJobForTesting(VaapiEncodeJob* job);
+
   // The unchanged values are filled upon the construction. The varied values
   // (e.g. ScalingSettings) are filled properly during encoding.
   VideoEncoderInfo encoder_info_;
@@ -173,6 +175,9 @@ class MEDIA_GPU_EXPORT VaapiVideoEncodeAccelerator
   // is false.
   gfx::Size expected_input_coded_size_;
 
+  // The codec of the stream to be produced. Set during initialization.
+  VideoCodec output_codec_;
+
   // The visible rect to be encoded.
   gfx::Rect visible_rect_;
 

media/gpu/vaapi/vaapi_video_encode_accelerator_unittest.cc

@@ -11,6 +11,7 @@
 #include "base/run_loop.h"
 #include "base/test/gmock_callback_support.h"
 #include "base/test/task_environment.h"
+#include "media/gpu/vp9_picture.h"
 #include "media/video/video_encode_accelerator.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -42,9 +43,13 @@ MATCHER_P2(MatchesAcceleratedVideoEncoderConfig,
          arg.bitrate_control == bitrate_control;
 }
 
-MATCHER_P2(MatchesBitstreamBufferMetadata, payload_size_bytes, key_frame, "") {
+MATCHER_P3(MatchesBitstreamBufferMetadata,
+           payload_size_bytes,
+           key_frame,
+           has_vp9_metadata,
+           "") {
   return arg.payload_size_bytes == payload_size_bytes &&
-         arg.key_frame == key_frame;
+         arg.key_frame == key_frame && arg.vp9.has_value() == has_vp9_metadata;
 }
 
 class MockVideoEncodeAcceleratorClient : public VideoEncodeAccelerator::Client {
@@ -93,6 +98,7 @@ class MockAcceleratedVideoEncoder : public AcceleratedVideoEncoder {
   MOCK_CONST_METHOD0(GetCodedSize, gfx::Size());
   MOCK_CONST_METHOD0(GetBitstreamBufferSize, size_t());
   MOCK_CONST_METHOD0(GetMaxNumOfRefFrames, size_t());
+  MOCK_METHOD2(GetMetadata, BitstreamBufferMetadata(EncodeJob*, size_t));
   MOCK_METHOD1(PrepareEncodeJob, bool(EncodeJob*));
   MOCK_METHOD1(BitrateControlUpdate, void(uint64_t));
   bool UpdateRates(const VideoBitrateAllocation&, uint32_t) override {
@@ -186,7 +192,7 @@ class VaapiVideoEncodeAcceleratorTest
     run_loop.Run();
   }
 
-  void EncodeSequenceForVP9() {
+  void EncodeSequenceForVP9(bool use_temporal_layer_encoding) {
     base::RunLoop run_loop;
     base::Closure quit_closure = run_loop.QuitClosure();
     ::testing::InSequence s;
@@ -203,7 +209,16 @@ class VaapiVideoEncodeAcceleratorTest
     EXPECT_CALL(*mock_encoder_, PrepareEncodeJob(_))
         .WillOnce(WithArgs<0>(
             [encoder = encoder_.get(), kCodedBufferId,
+             use_temporal_layer_encoding,
              kInputSurfaceId](AcceleratedVideoEncoder::EncodeJob* job) {
+              if (use_temporal_layer_encoding) {
+                // Set Vp9Metadata on temporal layer encoding.
+                CodecPicture* picture =
+                    VaapiVideoEncodeAccelerator::GetPictureFromJobForTesting(
+                        job->AsVaapiEncodeJob());
+                reinterpret_cast<VP9Picture*>(picture)->metadata_for_encoding =
+                    Vp9Metadata();
+              }
               job->AddPostExecuteCallback(base::BindOnce(
                   &VaapiVideoEncodeAccelerator::NotifyEncodedChunkSize,
                   base::Unretained(
@@ -235,11 +250,24 @@ class VaapiVideoEncodeAcceleratorTest
         }));
     EXPECT_CALL(*mock_vaapi_wrapper_, DestroyVABuffer(kCodedBufferId))
         .WillOnce(Return());
-
+    EXPECT_CALL(*mock_encoder_, GetMetadata(_, kEncodedChunkSize))
+        .WillOnce(WithArgs<0, 1>(
+            [](AcceleratedVideoEncoder::EncodeJob* job, size_t payload_size) {
+              // Same implementation in VP9Encoder.
+              BitstreamBufferMetadata metadata(
+                  payload_size, job->IsKeyframeRequested(), job->timestamp());
+              CodecPicture* picture =
+                  VaapiVideoEncodeAccelerator::GetPictureFromJobForTesting(
+                      job->AsVaapiEncodeJob());
+              metadata.vp9 =
+                  reinterpret_cast<VP9Picture*>(picture)->metadata_for_encoding;
+              return metadata;
+            }));
     constexpr int32_t kBitstreamId = 12;
     EXPECT_CALL(client_, BitstreamBufferReady(kBitstreamId,
                                               MatchesBitstreamBufferMetadata(
-                                                  kEncodedChunkSize, false)))
+                                                  kEncodedChunkSize, false,
+                                                  use_temporal_layer_encoding)))
         .WillOnce(RunClosure(quit_closure));
 
     auto region = base::UnsafeSharedMemoryRegion::Create(output_buffer_size_);
@@ -314,7 +342,7 @@ TEST_P(VaapiVideoEncodeAcceleratorTest, EncodeVP9WithSingleSpatialLayer) {
   SetDefaultMocksBehavior(config);
 
   InitializeSequenceForVP9(config);
-  EncodeSequenceForVP9();
+  EncodeSequenceForVP9(spatial_layer.num_of_temporal_layers > 1u);
 }
 
 INSTANTIATE_TEST_SUITE_P(,