V4L2SVDA/VAAPIVDA: use visible size from decoder and pass to client

media/gpu/vaapi_video_decode_accelerator.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/gpu/vaapi_video_decode_accelerator.h"
 
 #include <string.h>
 
 #include <memory>
 
@@ skipping 409 matching lines @@
   DVLOG(3) << "Outputting VASurface " << va_surface->id()
            << " into pixmap bound to picture buffer id " << output_id;
 
   RETURN_AND_NOTIFY_ON_FAILURE(picture->DownloadFromSurface(va_surface),
                                "Failed putting surface into pixmap",
                                PLATFORM_FAILURE, );
 
   // Notify the client a picture is ready to be displayed.
   ++num_frames_at_client_;
   TRACE_COUNTER1("Video Decoder", "Textures at client", num_frames_at_client_);
-  DVLOG(4) << "Notifying output picture id " << output_id
-           << " for input " << input_id << " is ready";
-  // TODO(posciak): Use visible size from decoder here instead
-  // (crbug.com/402760). Passing (0, 0) results in the client using the
-  // visible size extracted from the container instead.
+  gfx::Rect visible_rect = decoder_->GetVisibleRect();

[Owen Lin, 2017/06/07 06:11:00]

I am afraid you cannot get the visible size here. The decoder may already saw a
resolution change and switch to a new size.

I would suggest we add a visible_size in H264Picture and set it in the decoders.

[johnylin1, 2017/06/07 15:38:37]

I think it would be better to put visible_size in DecodeSurface.

[Pawel Osciak, 2017/06/08 04:58:24]

Could you explain your reasoning please?

Owen is right, GetVisibleRect() would only be valid at the time of decoding the
particular picture, not at output time.

Moreover, it would be symmetrical to have the visible rect information somehow
contained within all the *Picture classes. For VP9, it's directly in the
Vp9FrameHeader, because the header syntax element contains it explicitly. For
H264, it's calculated from SPS, so it should probably be in H264Picture class
itself, which contains calculated values for each picture. For VP8, it's also
not in the header, so similarly we could put it in VP8Picture as well.

[Owen Lin, 2017/06/09 02:03:17]

I understand why we need this in DecodeSurface. H264Picture is not available in
OutputSurface.

Even though, adding visible size to H264Picture is still better than adding it
to decoders. From the viewpoint of a decoder, the visible size is not
consistent. It is hard to define what GetVisibleSize() returns. (Or you need
document it as only valid at ....) But adding it to H264Picture and set it
insides decoder makes things clear.

We just pass this info from H264Picture to DecodeSurface.

[johnylin1, 2017/06/12 13:41:53]

My thought is same as Owen, we need to have visible_rect in DecodeSurface,
because H264Picture is not available in VDA.

After more tracings, I think maybe we could set visible_rect into H264Picture
when it has been created (same as VP8, VP9). Please see my new modifications on
h264_decoder.cc

+  DVLOG(4) << "Notifying output picture id " << output_id << " for input "
+           << input_id
+           << " is ready. visible rect: " << visible_rect.ToString();
   // TODO(hubbe): Use the correct color space.  http://crbug.com/647725
   if (client_)
-    client_->PictureReady(Picture(output_id, input_id, gfx::Rect(0, 0),
+    client_->PictureReady(Picture(output_id, input_id, visible_rect,
                                   gfx::ColorSpace(), picture->AllowOverlay()));
 }
 
 void VaapiVideoDecodeAccelerator::TryOutputSurface() {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   // Handle Destroy() arriving while pictures are queued for output.
   if (!client_)
     return;
 
@@ skipping 1449 matching lines @@
   return vaapi_pic->dec_surface();
 }
 
 // static
 VideoDecodeAccelerator::SupportedProfiles
 VaapiVideoDecodeAccelerator::GetSupportedProfiles() {
   return VaapiWrapper::GetSupportedDecodeProfiles();
 }
 
 }  // namespace media