SurfaceControl: Use only valid region of pixels to calculate src rect.

SurfaceControl was currently using AHardwareBuffer dimensions as a valid
region of pixels to calculate src rect. This do not work always since
actual region of valid pixels could be smaller than the buffer dimensions.
Hence use valid region of pixel instead of buffer dimensions.

Bug: 1027766
Change-Id: Ic289712b71b22be2fc645c771ddcb297b1c6687c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1929632Reviewed-by: Zhenyao Mo <zmo@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Commit-Queue: vikas soni <vikassoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#719462}

gpu/command_buffer/service/image_reader_gl_owner.cc

@@ -304,6 +304,27 @@ ImageReaderGLOwner::GetAHardwareBuffer() {
       current_image_ref_->GetReadyFence());
 }
 
+gfx::Rect ImageReaderGLOwner::GetCropRect() {
+  if (!current_image_ref_)
+    return gfx::Rect();
+
+  // Note that to query the crop rectangle, we don't need to wait for the
+  // AImage to be ready by checking the associated image ready fence.
+  AImageCropRect crop_rect;
+  media_status_t return_code =
+      loader_.AImage_getCropRect(current_image_ref_->image(), &crop_rect);
+  if (return_code != AMEDIA_OK) {
+    DLOG(ERROR) << "Error querying crop rectangle from the image : "
+                << return_code;
+    return gfx::Rect();
+  }
+  DCHECK_GE(crop_rect.right, crop_rect.left);
+  DCHECK_GE(crop_rect.bottom, crop_rect.top);
+  return gfx::Rect(crop_rect.left, crop_rect.top,
+                   crop_rect.right - crop_rect.left,
+                   crop_rect.bottom - crop_rect.top);
+}
+
 void ImageReaderGLOwner::RegisterRefOnImage(AImage* image) {
   DCHECK(image_reader_);
 
@@ -351,22 +372,12 @@ void ImageReaderGLOwner::GetTransformMatrix(float mtx[]) {
                                                 0, 0, 1, 0, 0, 1,  0, 1};
   memcpy(mtx, kYInvertedIdentity, sizeof(kYInvertedIdentity));
 
-  // Compute the transform matrix only if we have an image available.
-  if (!current_image_ref_)
-    return;
 
   // Get the crop rectangle associated with this image. The crop rectangle
   // specifies the region of valid pixels in the image.
-  // Note that to query the crop rectangle, we don't need to wait for the
-  // AImage to be ready by checking the associated image ready fence.
-  AImageCropRect crop_rect;
-  media_status_t return_code =
-      loader_.AImage_getCropRect(current_image_ref_->image(), &crop_rect);
-  if (return_code != AMEDIA_OK) {
-    DLOG(ERROR) << "Error querying crop rectangle from the image : "
-                << return_code;
+  gfx::Rect crop_rect = GetCropRect();
+  if (crop_rect.IsEmpty())
     return;
-  }
 
   // Get the AHardwareBuffer to query its dimensions.
   AHardwareBuffer* buffer = nullptr;
@@ -411,11 +422,10 @@ void ImageReaderGLOwner::GetTransformMatrix(float mtx[]) {
       shrink_amount = 1.0;
   }
 
-  int32_t crop_rect_width = (crop_rect.right - crop_rect.left);
-  int32_t crop_rect_height = (crop_rect.bottom - crop_rect.top);
-  DCHECK_GE(crop_rect_width, 0);
-  DCHECK_GE(crop_rect_height, 0);
-
+  int32_t crop_rect_width = crop_rect.width();
+  int32_t crop_rect_height = crop_rect.height();
+  int32_t crop_rect_left = crop_rect.x();
+  int32_t crop_rect_bottom = crop_rect.y() + crop_rect_height;
   int32_t buffer_width = desc.width;
   int32_t buffer_height = desc.height;
   DCHECK_GT(buffer_width, 0);
@@ -423,12 +433,12 @@ void ImageReaderGLOwner::GetTransformMatrix(float mtx[]) {
 
   // Only shrink the dimensions that are not the size of the buffer.
   if (crop_rect_width < buffer_width) {
-    tx = (float(crop_rect.left) + shrink_amount) / buffer_width;
+    tx = (float(crop_rect_left) + shrink_amount) / buffer_width;
     sx = (float(crop_rect_width) - (2.0f * shrink_amount)) / buffer_width;
   }
 
   if (crop_rect_height < buffer_height) {
-    ty = (float(buffer_height - crop_rect.bottom) + shrink_amount) /
+    ty = (float(buffer_height - crop_rect_bottom) + shrink_amount) /
          buffer_height;
     sy = (float(crop_rect_height) - (2.0f * shrink_amount)) / buffer_height;
   }

gpu/command_buffer/service/image_reader_gl_owner.h

@@ -41,6 +41,7 @@ class GPU_GLES2_EXPORT ImageReaderGLOwner : public TextureOwner {
   void ReleaseBackBuffers() override;
   std::unique_ptr<base::android::ScopedHardwareBufferFenceSync>
   GetAHardwareBuffer() override;
+  gfx::Rect GetCropRect() override;
 
   const AImageReader* image_reader_for_testing() const { return image_reader_; }
   int32_t max_images_for_testing() const { return max_images_; }

gpu/command_buffer/service/mock_texture_owner.h

@@ -44,9 +44,15 @@ class MockTextureOwner : public TextureOwner {
     return nullptr;
   }
 
+  gfx::Rect GetCropRect() override {
+    ++get_crop_rect_count;
+    return gfx::Rect();
+  }
+
   gl::GLContext* fake_context;
   gl::GLSurface* fake_surface;
   int get_a_hardware_buffer_count = 0;
+  int get_crop_rect_count = 0;
   bool expect_update_tex_image;
 
  protected:

gpu/command_buffer/service/surface_texture_gl_owner.cc

@@ -101,4 +101,9 @@ SurfaceTextureGLOwner::GetAHardwareBuffer() {
   return nullptr;
 }
 
+gfx::Rect SurfaceTextureGLOwner::GetCropRect() {
+  NOTREACHED() << "Don't use GetCropRect with SurfaceTextureGLOwner";
+  return gfx::Rect();
+}
+
 }  // namespace gpu

gpu/command_buffer/service/surface_texture_gl_owner.h

@@ -37,6 +37,7 @@ class GPU_EXPORT SurfaceTextureGLOwner : public TextureOwner {
   void ReleaseBackBuffers() override;
   std::unique_ptr<base::android::ScopedHardwareBufferFenceSync>
   GetAHardwareBuffer() override;
+  gfx::Rect GetCropRect() override;
 
  protected:
   void OnTextureDestroyed(gles2::AbstractTexture*) override;

gpu/command_buffer/service/texture_owner.h

@@ -92,6 +92,10 @@ class GPU_GLES2_EXPORT TextureOwner
   virtual std::unique_ptr<base::android::ScopedHardwareBufferFenceSync>
   GetAHardwareBuffer() = 0;
 
+  // Provides the crop rectangle associated with the most recent image. The
+  // crop rectangle specifies the region of valid pixels in the image.
+  virtual gfx::Rect GetCropRect() = 0;
+
   // Set the callback function to run when a new frame is available.
   // |frame_available_cb| is thread safe and can be called on any thread. This
   // method should be called only once, i.e., once a callback is provided, it

media/gpu/android/codec_image.cc

@@ -355,6 +355,12 @@ CodecImage::GetAHardwareBuffer() {
   return codec_buffer_wait_coordinator_->texture_owner()->GetAHardwareBuffer();
 }
 
+gfx::Rect CodecImage::GetCropRect() {
+  if (!codec_buffer_wait_coordinator_)
+    return gfx::Rect();
+  return codec_buffer_wait_coordinator_->texture_owner()->GetCropRect();
+}
+
 bool CodecImage::HasMutableState() const {
   return false;
 }

media/gpu/android/codec_image.h

@@ -90,6 +90,7 @@ class MEDIA_GPU_EXPORT CodecImage
                     const std::string& dump_name) override;
   std::unique_ptr<base::android::ScopedHardwareBufferFenceSync>
   GetAHardwareBuffer() override;
+  gfx::Rect GetCropRect() override;
   // gpu::gles2::GLStreamTextureMatrix implementation
   void GetTextureMatrix(float xform[16]) override;
   // Currently this API is implemented by the NotifyOverlayPromotion, since this

media/gpu/android/codec_image_unittest.cc

@@ -414,4 +414,13 @@ TEST_F(CodecImageTest, GetAHardwareBufferAfterRelease) {
   EXPECT_FALSE(i->GetAHardwareBuffer());
 }
 
+TEST_F(CodecImageTest, GetCropRect) {
+  auto i = NewImage(kTextureOwner);
+  EXPECT_EQ(
+      codec_buffer_wait_coordinator_->texture_owner()->get_crop_rect_count, 0);
+  i->GetCropRect();
+  EXPECT_EQ(
+      codec_buffer_wait_coordinator_->texture_owner()->get_crop_rect_count, 1);
+}
+
 }  // namespace media

ui/gl/gl_image.cc

@@ -34,6 +34,10 @@ std::unique_ptr<base::android::ScopedHardwareBufferFenceSync>
 GLImage::GetAHardwareBuffer() {
   return nullptr;
 }
+
+gfx::Rect GLImage::GetCropRect() {
+  return gfx::Rect();
+}
 #endif
 
 bool GLImage::HasMutableState() const {

ui/gl/gl_image.h

@@ -139,6 +139,10 @@ class GL_EXPORT GLImage : public base::RefCounted<GLImage> {
   // returned.
   virtual std::unique_ptr<base::android::ScopedHardwareBufferFenceSync>
   GetAHardwareBuffer();
+
+  // Provides the crop rectangle associated with the image. The crop rectangle
+  // specifies the region of valid pixels in the image.
+  virtual gfx::Rect GetCropRect();
 #endif
 
   // An identifier for subclasses. Necessary for safe downcasting.

ui/gl/gl_surface_egl_surface_control.cc

@@ -329,12 +329,27 @@ bool GLSurfaceEGLSurfaceControl::ScheduleOverlayPlane(
   if (hardware_buffer) {
     gfx::Rect dst = bounds_rect;
 
+    // Get the crop rectangle from the image which is the actual region of valid
+    // pixels. This region could be smaller than the buffer dimensions. Hence
+    // scale the |crop_rect| according to the valid pixel area rather than
+    // buffer dimensions. crbug.com/1027766 for more details.
+    gfx::Rect valid_pixel_rect = image->GetCropRect();
     gfx::Size buffer_size = GetBufferSize(hardware_buffer);
-    gfx::RectF scaled_rect =
-        gfx::RectF(crop_rect.x() * buffer_size.width(),
-                   crop_rect.y() * buffer_size.height(),
-                   crop_rect.width() * buffer_size.width(),
-                   crop_rect.height() * buffer_size.height());
+
+    // If the image doesn't provide a |valid_pixel_rect|, assume the entire
+    // buffer is valid.
+    if (valid_pixel_rect.IsEmpty()) {
+      valid_pixel_rect.set_size(buffer_size);
+    } else {
+      // Clamp the |valid_pixel_rect| to the buffer dimensions to make sure for
+      // some reason it does not overflows.
+      valid_pixel_rect.Intersect(gfx::Rect(buffer_size));
+    }
+    gfx::RectF scaled_rect = gfx::RectF(
+        crop_rect.x() * valid_pixel_rect.width() + valid_pixel_rect.x(),
+        crop_rect.y() * valid_pixel_rect.height() + valid_pixel_rect.y(),
+        crop_rect.width() * valid_pixel_rect.width(),
+        crop_rect.height() * valid_pixel_rect.height());
     gfx::Rect src = gfx::ToEnclosedRect(scaled_rect);
 
     if (uninitialized || surface_state.src != src || surface_state.dst != dst ||