Ozone: Dont hardcode format to YUV when using Overlay Composition.

ui/ozone/platform/drm/gpu/drm_window.cc

Index: ui/ozone/platform/drm/gpu/drm_window.cc
diff --git a/ui/ozone/platform/drm/gpu/drm_window.cc b/ui/ozone/platform/drm/gpu/drm_window.cc
index 29c9a8a58b54dd5748846260f219925aa33484f8..93df9ce02c939f65b9f85139b0f1f934fba83ad0 100644
--- a/ui/ozone/platform/drm/gpu/drm_window.cc
+++ b/ui/ozone/platform/drm/gpu/drm_window.cc
@@ -11,6 +11,7 @@
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkDevice.h"
 #include "third_party/skia/include/core/SkSurface.h"
+#include "ui/gfx/geometry/size_conversions.h"
 #include "ui/ozone/common/gpu/ozone_gpu_message_params.h"
 #include "ui/ozone/platform/drm/common/drm_util.h"
 #include "ui/ozone/platform/drm/gpu/crtc_controller.h"
@@ -155,15 +156,21 @@ std::vector<OverlayCheck_Params> DrmWindow::TestPageFlip(
     OverlayCheck_Params overlay_params(overlay);
     gfx::Size size =
         (overlay.plane_z_order == 0) ? bounds().size() : overlay.buffer_size;
+
+    // TODO(kalyank): We assume that it's always a Video buffer in case
+    // plane_z_order > 1. This needs to be revisited when we add Overlay support
+    // for other layers.
+    OverlayBufferConfiguration buffer_config = GetOverlayBufferConfiguration(
+        overlay.display_rect, overlay.crop_rect, size, overlay.plane_z_order,

[dnicoara, 2015/11/09 20:53:23]

I think that the way |size| is being used in GetOverlayBufferconfiguration() may
lead to issues when z_order == 0 and when deciding if scaling is needed. In that
cause, I think reporting the display size may cause |needs_scaling| to be false
even though it should be true.

Thinking more, I'm not really sure why the size is reported differently for the
primary plane. If I remember correctly the primary plane should always have a
size equal to the window/display size. I guess, if we're in the middle of a
display configuration this may happen, but in that case should we just fail the
TestPageFlip()? Or maybe just proceed with the requested value otherwise we'll
be testing with a different configuration?

[kalyank, 2015/11/09 21:44:05]

Everytime, display bounds of window changes we will be re-testing all the
overlay configurations. So, I think we should be covered by that. |display_rect|
should be same as bounds() in primary case. Or are you thinking why in line 157
we set size to bounds() when plane z_order is 0 else to overlay.buffer_size ?

Also, TestPageFlip will already fail on platforms which cannot support scaling
with Primary. There is a bug in i915 driver that commit passes even though
scaling is not supported by an Overlay plane. This is something we are fixing,
after that I will add checks for plane scaling support here.

[dnicoara, 2015/11/09 22:26:42]

Yes, line 157 is making me scratch my head. It sounds like we could just always
use |overlay.buffer_size|, or am I missing something?

[kalyank, 2015/11/09 23:03:40]

You are right. I think this is a bug in Compositor side when creating
OverlayCandidate. I will take a look

+        overlay.plane_z_order > 1);
+
+    size = buffer_config.buffer_size;
+    gfx::BufferFormat buffer_format = buffer_config.buffer_format;
+
     scoped_refptr<ScanoutBuffer> buffer;
     // Check if we can re-use existing buffers.
     for (const auto& plane : last_submitted_planes_) {
-      uint32_t format = GetFourCCFormatFromBufferFormat(overlay.format);

[dnicoara, 2015/11/09 20:53:23]

This is no longer looking at the requested format. Should we default to that? In
particular I can imagine that we'd eventually want to support multiple formats
for the primary plane as well.

-      // We always use a storage type of XRGB, even if the pixel format
-      // is ARGB.
-      if (format == DRM_FORMAT_ARGB8888)
-        format = DRM_FORMAT_XRGB8888;
-
+      uint32_t format = GetFourCCFormatFromBufferFormat(buffer_format);
       if (plane.buffer->GetFramebufferPixelFormat() == format &&
           plane.z_order == overlay.plane_z_order &&
           plane.display_bounds == overlay.display_rect &&
@@ -174,13 +181,13 @@ std::vector<OverlayCheck_Params> DrmWindow::TestPageFlip(
     }
 
     if (!buffer)
-      buffer = buffer_generator->Create(drm, overlay.format, size);
+      buffer = buffer_generator->Create(drm, buffer_format, size);
 
     if (!buffer)
       continue;
 
-    OverlayPlane plane(buffer, overlay.plane_z_order, overlay.transform,
-                       overlay.display_rect, overlay.crop_rect);
+    OverlayPlane plane_config(buffer, overlay.plane_z_order, overlay.transform,
+                              overlay.display_rect, overlay.crop_rect);
 
     // Buffer for Primary plane should always be present for compatibility test.
     if (!compatible_test_list.size() && overlay.plane_z_order != 0) {
@@ -188,11 +195,11 @@ std::vector<OverlayCheck_Params> DrmWindow::TestPageFlip(
           *OverlayPlane::GetPrimaryPlane(last_submitted_planes_));
     }
 
-    compatible_test_list.push_back(plane);
+    compatible_test_list.push_back(plane_config);
 
     if (controller_->TestPageFlip(compatible_test_list)) {
       overlay_params.plane_ids =
-          controller_->GetCompatibleHardwarePlaneIds(plane);
+          controller_->GetCompatibleHardwarePlaneIds(plane_config);
       params.push_back(overlay_params);
     }
 
@@ -203,6 +210,32 @@ std::vector<OverlayCheck_Params> DrmWindow::TestPageFlip(
   return params;
 }
 
+OverlayBufferConfiguration DrmWindow::GetOverlayBufferConfiguration(
+    const gfx::Rect& display_bounds,
+    const gfx::RectF& crop_rect,
+    const gfx::Size& buffer_size,
+    uint32_t z_order,
+    bool video_content) {
+  gfx::Size required_size = buffer_size;
+  if (crop_rect.width() && crop_rect.height()) {

[dnicoara, 2015/11/09 20:53:23]

nit: !crop_rect.IsEmpty()

[kalyank, 2015/11/19 16:10:41]

Done.

+    required_size = gfx::ToCeiledSize(
+        gfx::SizeF(display_bounds.width() / crop_rect.width(),
+                   display_bounds.height() / crop_rect.height()));
+  }
+
+  bool needs_scaling = false;
+  if (buffer_size != required_size)
+    needs_scaling = true;
+
+  gfx::BufferFormat format = gfx::BufferFormat::BGRX_8888;
+  if (video_content && display_bounds != bounds_) {

[dnicoara, 2015/11/09 20:53:23]

Why do you need to check for different bounds? What if the video playing has
native bounds?

[kalyank, 2015/11/09 21:44:05]

This was to check if we are displaying the content in full screen. In that case
we want to fallback to a format which is supported by Primary. Instead of
assuming UYVY is not supported in this case, I guess we can query it from Plane
Manager or this whole logic of choosing the right format can be moved there.

[dnicoara, 2015/11/09 22:26:42]

Sorry, I think I'm confused. If it isn't fullscreen we try to show it as a UYVY
buffer otherwise we default to BGRX. Perhaps this ties in with my comment on
line 161 on the left?

If it is fullscreen, why wouldn't we try to use UYVY? OH ... the overlay is
getting promoted to primary and on non-atomic we'd have issues using a different
buffer format. Could you add a comment explaining this? It isn't obvious at
first and it looks like a mistake unless the reader is intimately familiar with
the code.

[kalyank, 2015/11/09 23:03:40]

Sure.

[kalyank, 2015/11/19 16:10:41]

Done.

+    if (controller_->IsFormatSupported(DRM_FORMAT_UYVY, z_order))
+      format = gfx::BufferFormat::UYVY_422;
+  }
+
+  return OverlayBufferConfiguration(format, required_size, needs_scaling);
+}
+
 const OverlayPlane* DrmWindow::GetLastModesetBuffer() {
   return OverlayPlane::GetPrimaryPlane(last_submitted_planes_);
 }