Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1426993003:
Ozone: Dont hardcode format to YUV when using Overlay Composition. (Closed)
DescriptionOzone: Dont hardcode format to YUV when using Overlay Composition.
We currently hardcode formats to YUV for Video used during
Hardware Overlay composition. All platforms might
not support this format and also in future we might want to
support other formats like NV12. Also, in certain use cases like
FullScreen using YUV might not be right format as Primary plane
might not support it. Instead of hardcoding it, we should rather
use OverlayValidator to decide the right format. This CL adds
needed support.
BUG=553264
Committed: https://crrev.com/ba1c3b03daef7adfd3e684b5228b4f65e95a113c
Cr-Commit-Position: refs/heads/master@{#369546}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Add support for querying formats from GbmSurfaceless #Patch Set 3 : cosmetic fixes #
Total comments: 6
Patch Set 4 : Rebase #Patch Set 5 : Rebase #Patch Set 6 : Rebase #Patch Set 7 : Rebase #Patch Set 8 : Nit fix #Patch Set 9 : Remove obsolete code #
Total comments: 21
Patch Set 10 : Review fixes #Patch Set 11 : Fix unit tests #Patch Set 12 : Handle Buffer processing in Pixmap #Patch Set 13 : Fix comments #Patch Set 14 : Remove redundant plane sorting #Patch Set 15 : Nit fixes #Patch Set 16 : Comments #
Total comments: 6
Patch Set 17 : Review fixes #Messages
Total messages: 38 (16 generated)
Description was changed from ========== WIP: Add support to query optimal buffer configuration With Media, we want to use formats other than RGBX(i.e. NV12, YUV) to reduce the read bandwidth in Display controller side. Also, currently we always hardcode the format to be RGBX during test flip and use YUV as buffer format when binding buffer to Overlay. This patch adds support to use these formats only when supported and unifies the logic of choosing format during test commit and actual page flip. BUG=553264 ========== to ========== WIP: Add support to query optimal buffer configuration With Media, we want to use formats other than RGBX(i.e. NV12, YUV) to reduce the read bandwidth in Display controller side. Also, currently we always hardcode the format to be RGBX during test flip and use YUV as buffer format when binding buffer to Overlay. This patch adds support to use these formats only when supported and unifies the logic of choosing format during test commit and actual page flip. BUG=553264 ==========
kalyan.kondapally@intel.com changed reviewers: + achaulk@chromium.org, alexst@chromium.org, dnicoara@chromium.org, spang@chromium.org
This is a WIP patch. Looking for advice on how we can query for the buffer configuration from GBMBuffer.
On 2015/11/09 19:57:44, kalyank wrote: > This is a WIP patch. Looking for advice on how we can query for the buffer > configuration from GBMBuffer. @dnicoara @spang
Description was changed from ========== WIP: Add support to query optimal buffer configuration With Media, we want to use formats other than RGBX(i.e. NV12, YUV) to reduce the read bandwidth in Display controller side. Also, currently we always hardcode the format to be RGBX during test flip and use YUV as buffer format when binding buffer to Overlay. This patch adds support to use these formats only when supported and unifies the logic of choosing format during test commit and actual page flip. BUG=553264 ========== to ========== WIP: Add support to query optimal buffer configuration With Media, we want to use formats other than RGBX(i.e. NV12, YUV) to reduce the read bandwidth in Display controller side. Also, currently we always hardcode the format to be RGBX during test commit and use YUV as buffer format when binding buffer to Overlay in actual commit. This patch adds support to use these formats only when supported and unifies the logic of choosing format during test commit and actual page flip. BUG=553264 ==========
Description was changed from ========== WIP: Add support to query optimal buffer configuration With Media, we want to use formats other than RGBX(i.e. NV12, YUV) to reduce the read bandwidth in Display controller side. Also, currently we always hardcode the format to be RGBX during test commit and use YUV as buffer format when binding buffer to Overlay in actual commit. This patch adds support to use these formats only when supported and unifies the logic of choosing format during test commit and actual page flip. BUG=553264 ========== to ========== WIP: Add support to query optimal buffer configuration With Media, we want to use formats other than RGBX(i.e. NV12, YUV) to reduce the read bandwidth in Display controller side. Also, currently we always hardcode the format to be RGBX during test commit and use YUV as buffer format when binding buffer to Overlay in actual commit. This patch adds support to use these formats only when supported and unifies the logic of choosing format during test commit and actual page flip. BUG=553264 ==========
Description was changed from ========== WIP: Add support to query optimal buffer configuration With Media, we want to use formats other than RGBX(i.e. NV12, YUV) to reduce the read bandwidth in Display controller side. Also, currently we always hardcode the format to be RGBX during test commit and use YUV as buffer format when binding buffer to Overlay in actual commit. This patch adds support to use these formats only when supported and unifies the logic of choosing format during test commit and actual page flip. BUG=553264 ========== to ========== WIP: Add support to query optimal buffer configuration With Media, we want to use formats other than RGBX(i.e. NV12, YUV) to reduce the read bandwidth in Display controller side. Also, currently we always hardcode the format to be RGBX during test commit and use YUV as buffer format when binding buffer to Overlay in actual commit. This patch adds support to use these formats only when supported and unifies the logic of choosing format during test commit and actual page flip. BUG=553264 ==========
https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/d... File ui/ozone/platform/drm/gpu/drm_window.cc (left): https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/d... ui/ozone/platform/drm/gpu/drm_window.cc:161: uint32_t format = GetFourCCFormatFromBufferFormat(overlay.format); 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. https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/d... File ui/ozone/platform/drm/gpu/drm_window.cc (right): https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/d... ui/ozone/platform/drm/gpu/drm_window.cc:164: overlay.display_rect, overlay.crop_rect, size, overlay.plane_z_order, 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? https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/d... ui/ozone/platform/drm/gpu/drm_window.cc:220: if (crop_rect.width() && crop_rect.height()) { nit: !crop_rect.IsEmpty() https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/d... ui/ozone/platform/drm/gpu/drm_window.cc:231: if (video_content && display_bounds != bounds_) { Why do you need to check for different bounds? What if the video playing has native bounds? https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/h... File ui/ozone/platform/drm/gpu/hardware_display_controller.cc (right): https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/h... ui/ozone/platform/drm/gpu/hardware_display_controller.cc:161: if (!crtc_controllers_[0]->IsFormatSupported(fourcc_format, z_order)) This can be handled in the for-loop. https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/o... File ui/ozone/platform/drm/gpu/overlay_plane.h (right): https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/o... ui/ozone/platform/drm/gpu/overlay_plane.h:46: struct OZONE_EXPORT OverlayBufferConfiguration { This seems to only be used in DrmWindow. Maybe just declare it in there, in the anonymous namespace. https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/o... ui/ozone/platform/drm/gpu/overlay_plane.h:48: gfx::Size buffer_size, nit: const gfx::Size&
https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/d... File ui/ozone/platform/drm/gpu/drm_window.cc (right): https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/d... ui/ozone/platform/drm/gpu/drm_window.cc:164: overlay.display_rect, overlay.crop_rect, size, overlay.plane_z_order, On 2015/11/09 20:53:23, dnicoara wrote: > 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? 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. https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/d... ui/ozone/platform/drm/gpu/drm_window.cc:231: if (video_content && display_bounds != bounds_) { On 2015/11/09 20:53:23, dnicoara wrote: > Why do you need to check for different bounds? What if the video playing has > native bounds? 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. https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/o... File ui/ozone/platform/drm/gpu/overlay_plane.h (right): https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/o... ui/ozone/platform/drm/gpu/overlay_plane.h:46: struct OZONE_EXPORT OverlayBufferConfiguration { On 2015/11/09 20:53:23, dnicoara wrote: > This seems to only be used in DrmWindow. Maybe just declare it in there, in the > anonymous namespace. Eventually, we want to query this information in GbmBuffer and other places interested in this information i.e. VaapiDrmPicture. Currently, we have logic in GbmBuffer as to when scaling is needed and use VPP hardware unit(in case of Video buffer)for this purpose. Also, https://codereview.chromium.org/1422563002/ extends support in GbmBuffer to add YUV support for Overlay when playing Videobuffer. We should consolidate all this logic in one place and make sure we are using the same during test commits.
https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/d... File ui/ozone/platform/drm/gpu/drm_window.cc (right): https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/d... ui/ozone/platform/drm/gpu/drm_window.cc:164: overlay.display_rect, overlay.crop_rect, size, overlay.plane_z_order, On 2015/11/09 21:44:05, kalyank wrote: > On 2015/11/09 20:53:23, dnicoara wrote: > > 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? > > 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. 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? https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/d... ui/ozone/platform/drm/gpu/drm_window.cc:231: if (video_content && display_bounds != bounds_) { On 2015/11/09 21:44:05, kalyank wrote: > On 2015/11/09 20:53:23, dnicoara wrote: > > Why do you need to check for different bounds? What if the video playing has > > native bounds? > > 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. 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. https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/o... File ui/ozone/platform/drm/gpu/overlay_plane.h (right): https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/o... ui/ozone/platform/drm/gpu/overlay_plane.h:46: struct OZONE_EXPORT OverlayBufferConfiguration { On 2015/11/09 21:44:05, kalyank wrote: > On 2015/11/09 20:53:23, dnicoara wrote: > > This seems to only be used in DrmWindow. Maybe just declare it in there, in > the > > anonymous namespace. > > Eventually, we want to query this information in GbmBuffer and other places > interested in this information i.e. VaapiDrmPicture. > Currently, we have logic in GbmBuffer as to when scaling is needed and use VPP > hardware unit(in case of Video buffer)for this purpose. > Also, https://codereview.chromium.org/1422563002/ extends support in GbmBuffer > to add YUV support for Overlay when playing Videobuffer. We should consolidate > all this logic in one place and make sure we are using the same during test > commits. > > Acknowledged.
https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/d... File ui/ozone/platform/drm/gpu/drm_window.cc (right): https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/d... ui/ozone/platform/drm/gpu/drm_window.cc:164: overlay.display_rect, overlay.crop_rect, size, overlay.plane_z_order, On 2015/11/09 22:26:42, dnicoara wrote: > On 2015/11/09 21:44:05, kalyank wrote: > > On 2015/11/09 20:53:23, dnicoara wrote: > > > 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? > > > > 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. > > 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? You are right. I think this is a bug in Compositor side when creating OverlayCandidate. I will take a look https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/d... ui/ozone/platform/drm/gpu/drm_window.cc:231: if (video_content && display_bounds != bounds_) { On 2015/11/09 22:26:42, dnicoara wrote: > On 2015/11/09 21:44:05, kalyank wrote: > > On 2015/11/09 20:53:23, dnicoara wrote: > > > Why do you need to check for different bounds? What if the video playing has > > > native bounds? > > > > 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. > > 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. Sure.
https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/d... File ui/ozone/platform/drm/gpu/drm_window.cc (right): https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/d... ui/ozone/platform/drm/gpu/drm_window.cc:220: if (crop_rect.width() && crop_rect.height()) { On 2015/11/09 20:53:23, dnicoara wrote: > nit: !crop_rect.IsEmpty() Done. https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/d... ui/ozone/platform/drm/gpu/drm_window.cc:231: if (video_content && display_bounds != bounds_) { On 2015/11/09 22:26:42, dnicoara wrote: > On 2015/11/09 21:44:05, kalyank wrote: > > On 2015/11/09 20:53:23, dnicoara wrote: > > > Why do you need to check for different bounds? What if the video playing has > > > native bounds? > > > > 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. > > 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. Done. https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/o... File ui/ozone/platform/drm/gpu/overlay_plane.h (right): https://codereview.chromium.org/1426993003/diff/1/ui/ozone/platform/drm/gpu/o... ui/ozone/platform/drm/gpu/overlay_plane.h:46: struct OZONE_EXPORT OverlayBufferConfiguration { On 2015/11/09 20:53:23, dnicoara wrote: > This seems to only be used in DrmWindow. Maybe just declare it in there, in the > anonymous namespace. Removed this and saved needed data as part of overlaycheck_params https://codereview.chromium.org/1426993003/diff/40001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/gbm_surfaceless.cc (right): https://codereview.chromium.org/1426993003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/gbm_surfaceless.cc:81: window_->GetOverlayBufferConfigurations( I am still looking if we can pass target size and format as part of ScheduleOverlayPlane. This would avoid the need for the callback.
Could this be split out as multiple changes? It is becoming harder to follow given the amount of changes. https://codereview.chromium.org/1426993003/diff/40001/ui/ozone/common/gpu/ozo... File ui/ozone/common/gpu/ozone_gpu_message_params.h (right): https://codereview.chromium.org/1426993003/diff/40001/ui/ozone/common/gpu/ozo... ui/ozone/common/gpu/ozone_gpu_message_params.h:51: enum State { kTest, kInvalid, kCompatible, kOverlay }; nit: Could you please have kInvalid as the first state? It'll better align with states elsewhere. https://codereview.chromium.org/1426993003/diff/40001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/drm_overlay_candidate.cc (right): https://codereview.chromium.org/1426993003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_overlay_candidate.cc:145: void DrmOverlayCandidate::ValidateConfiguration( Could you document what this does? My understanding is that this function tests to see if the overlay itself can be shown alone (+primary plane if z_order != 0). Later in TestPageFlip() you'll also check that the whole configuration is valid. Wondering why this would be needed. Why not just test just in TestPageFlip()? Ultimately you'll need to test with the list of overlays requested to see how much hardware resources you have. https://codereview.chromium.org/1426993003/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_overlay_candidate.cc:209: format = gfx::BufferFormat::BGRX_8888; Shouldn't this be in the following if-statement? We don't want to change the format if the overlay isn't promoted, right? https://codereview.chromium.org/1426993003/diff/40001/ui/ozone/platform/drm/h... File ui/ozone/platform/drm/host/drm_overlay_candidates_host.h (right): https://codereview.chromium.org/1426993003/diff/40001/ui/ozone/platform/drm/h... ui/ozone/platform/drm/host/drm_overlay_candidates_host.h:87: std::vector<OverlayCheck_Params> in_use_params_; I'm not sure that this is really needed or that it's description is correct. Just because we've checked a configuration doesn't mean that it is in use. In particular I can see CC querying and caching some valid configurations and just switching between them as needed without querying again. For example it could be that it overlayed a video. When the video finishes it would just go back to the 1 plane configuration without querying again. Could we do without this? https://codereview.chromium.org/1426993003/diff/40001/ui/ozone/public/surface... File ui/ozone/public/surface_ozone_egl.h (right): https://codereview.chromium.org/1426993003/diff/40001/ui/ozone/public/surface... ui/ozone/public/surface_ozone_egl.h:27: OverlayParamCallback; I don't think this should be defined here. It isn't used in the interface and I think this mostly has platform internal uses.
On 2015/11/23 19:22:54, dnicoara wrote: > Could this be split out as multiple changes? It is becoming harder to follow > given the amount of changes. > Will do that.
On 2015/11/23 20:14:02, kalyank wrote: > On 2015/11/23 19:22:54, dnicoara wrote: > > Could this be split out as multiple changes? It is becoming harder to follow > > given the amount of changes. > > > > Will do that. Sorry for the delay, I should get back to this by end of the week.
On 2015/12/01 16:36:22, kalyank wrote: > On 2015/11/23 20:14:02, kalyank wrote: > > On 2015/11/23 19:22:54, dnicoara wrote: > > > Could this be split out as multiple changes? It is becoming harder to follow > > > given the amount of changes. > > > > > > > Will do that. > > Sorry for the delay, I should get back to this by end of the week. I have split up the changes needed to consolidate all checks in GPU side here:https://codereview.chromium.org/1502353005/ I am working on passing the necessary data (i.e. optimal storage format and when scaling is needed to be done on chrome side)via ScheduleOverlayPlane and expect some changes in 1502353005.
On 2015/12/09 17:04:50, kalyank wrote: > I am working on passing the necessary data (i.e. optimal storage format > and when scaling is needed to be done on chrome side)via ScheduleOverlayPlane > and expect some changes in 1502353005. Created https://codereview.chromium.org/1513283002/ for passing necessary data via ScheduleOverlayplane, instead of maintaining a cache of the params in GPU process side.
Description was changed from ========== WIP: Add support to query optimal buffer configuration With Media, we want to use formats other than RGBX(i.e. NV12, YUV) to reduce the read bandwidth in Display controller side. Also, currently we always hardcode the format to be RGBX during test commit and use YUV as buffer format when binding buffer to Overlay in actual commit. This patch adds support to use these formats only when supported and unifies the logic of choosing format during test commit and actual page flip. BUG=553264 ========== to ========== Add support to query optimal buffer configuration We currently hardcode formats to YUV for Video used during Hardware Overlay composition. All platforms might not support this format and also in future we might want to support other formats like NV12. Also, in certain use cases like FullScreen using YUV might not be right format as Primary plane might not support it. Instead of hardcoding it, we should rather use OverlayValidator to decide the right format. This CL adds needed support. BUG=553264 ==========
Description was changed from ========== Add support to query optimal buffer configuration We currently hardcode formats to YUV for Video used during Hardware Overlay composition. All platforms might not support this format and also in future we might want to support other formats like NV12. Also, in certain use cases like FullScreen using YUV might not be right format as Primary plane might not support it. Instead of hardcoding it, we should rather use OverlayValidator to decide the right format. This CL adds needed support. BUG=553264 ========== to ========== Add support to query optimal buffer configuration We currently hardcode formats to YUV for Video used during Hardware Overlay composition. All platforms might not support this format and also in future we might want to support other formats like NV12. Also, in certain use cases like FullScreen using YUV might not be right format as Primary plane might not support it. Instead of hardcoding it, we should rather use OverlayValidator to decide the right format. This CL adds needed support. BUG=553264 ==========
Description was changed from ========== Add support to query optimal buffer configuration We currently hardcode formats to YUV for Video used during Hardware Overlay composition. All platforms might not support this format and also in future we might want to support other formats like NV12. Also, in certain use cases like FullScreen using YUV might not be right format as Primary plane might not support it. Instead of hardcoding it, we should rather use OverlayValidator to decide the right format. This CL adds needed support. BUG=553264 ========== to ========== Add support to query optimal buffer configuration We currently hardcode formats to YUV for Video used during Hardware Overlay composition. All platforms might not support this format and also in future we might want to support other formats like NV12. Also, in certain use cases like FullScreen using YUV might not be right format as Primary plane might not support it. Instead of hardcoding it, we should rather use OverlayValidator to decide the right format. This CL adds needed support. BUG=553264 ==========
Description was changed from ========== Add support to query optimal buffer configuration We currently hardcode formats to YUV for Video used during Hardware Overlay composition. All platforms might not support this format and also in future we might want to support other formats like NV12. Also, in certain use cases like FullScreen using YUV might not be right format as Primary plane might not support it. Instead of hardcoding it, we should rather use OverlayValidator to decide the right format. This CL adds needed support. BUG=553264 ========== to ========== Add support to query optimal buffer configuration We currently hardcode formats to YUV for Video used during Hardware Overlay composition. All platforms might not support this format and also in future we might want to support other formats like NV12. Also, in certain use cases like FullScreen using YUV might not be right format as Primary plane might not support it. Instead of hardcoding it, we should rather use OverlayValidator to decide the right format. This CL adds needed support. BUG=553264 ==========
Description was changed from ========== Add support to query optimal buffer configuration We currently hardcode formats to YUV for Video used during Hardware Overlay composition. All platforms might not support this format and also in future we might want to support other formats like NV12. Also, in certain use cases like FullScreen using YUV might not be right format as Primary plane might not support it. Instead of hardcoding it, we should rather use OverlayValidator to decide the right format. This CL adds needed support. BUG=553264 ========== to ========== Add support to query optimal buffer configuration We currently hardcode formats to YUV for Video used during Hardware Overlay composition. All platforms might not support this format and also in future we might want to support other formats like NV12. Also, in certain use cases like FullScreen using YUV might not be right format as Primary plane might not support it. Instead of hardcoding it, we should rather use OverlayValidator to decide the right format. This CL adds needed support. BUG=553264 ==========
Description was changed from ========== Add support to query optimal buffer configuration We currently hardcode formats to YUV for Video used during Hardware Overlay composition. All platforms might not support this format and also in future we might want to support other formats like NV12. Also, in certain use cases like FullScreen using YUV might not be right format as Primary plane might not support it. Instead of hardcoding it, we should rather use OverlayValidator to decide the right format. This CL adds needed support. BUG=553264 ========== to ========== Add support to query optimal buffer configuration We currently hardcode formats to YUV for Video used during Hardware Overlay composition. All platforms might not support this format and also in future we might want to support other formats like NV12. Also, in certain use cases like FullScreen using YUV might not be right format as Primary plane might not support it. Instead of hardcoding it, we should rather use OverlayValidator to decide the right format. This CL adds needed support. BUG=553264 ==========
Description was changed from ========== Add support to query optimal buffer configuration We currently hardcode formats to YUV for Video used during Hardware Overlay composition. All platforms might not support this format and also in future we might want to support other formats like NV12. Also, in certain use cases like FullScreen using YUV might not be right format as Primary plane might not support it. Instead of hardcoding it, we should rather use OverlayValidator to decide the right format. This CL adds needed support. BUG=553264 ========== to ========== Ozone: Dont hardcode format to YUV when using Overlay Composition. We currently hardcode formats to YUV for Video used during Hardware Overlay composition. All platforms might not support this format and also in future we might want to support other formats like NV12. Also, in certain use cases like FullScreen using YUV might not be right format as Primary plane might not support it. Instead of hardcoding it, we should rather use OverlayValidator to decide the right format. This CL adds needed support. BUG=553264 ==========
@dnicoara PTAL https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:88: processing_callback_ = processing_callback; This is called on Main thread while CopyBuffer is called in Drmthread. However, SetProcessingCallback is never called after initial setup. https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:95: return nullptr; This is not used any more. Call to CreateNativePixmap From CopyBuffer results in a deadlock, as CopyBuffer is called from DrmThread and CreateNativePixmap try to post a synchronous task to DrmThread. https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:157: DCHECK(source_buffer->GetHandle() == buffer_->GetHandle()); This will be called in Drmthread by OverlayValidator.
https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/drm_overlay_validator.h (right): https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/drm_overlay_validator.h:32: OverlayPlaneList ValidatePlanesForPageFlip( Could you please add some comments describing this. In particular, focus on the fact that this optimizes formats and such but doesn't actually change anything in how planes are presented, etc. Also, the name isn't quite right. At first I though this was a second "test page flip" which didn't seem right. Maybe something like OptimizeBuffersForPageFlip() or HandleBufferHints()? Feel free to suggest others. https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/drm_overlay_validator.h:34: ScanoutBufferGenerator* buffer_generator); Maybe we should pass in the buffer generator in the constructor. It'll simplify this call and TestPageFlip(). https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/drm_overlay_validator.h:59: // Expectation is that we have same combination of layers being used over Do you have an idea of what the overhead would be? I'm wondering if the extra complexity is needed since changing the overlay configuration would not be a frequent operation and this would only help the first frame after the change. In fact, I'm almost tempted to say this is optimized at the wrong layer. If we needed to apply scaling, I imagine we'd want to cache the new buffer within the GbmPixmap such that we'd re-use the buffer every time. It seems like the "void CopyBuffer(const scoped_refptr<ScanoutBuffer>, const scoped_refptr<ScanoutBuffer>)" should be more like: "scoped_refptr<ScanoutBuffer> GetProcessedBuffer(BufferFormat)" https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/drm_thread.cc (right): https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/drm_thread.cc:114: window->SchedulePageFlip(planes, callback, buffer_generator_.get()); Looking over the change and I'm thinking that the buffer_generator_ should just be passed in the constructor. There's no reason we need it passed in on every call. https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:95: return nullptr; On 2016/01/11 08:19:51, kalyank wrote: > This is not used any more. Call to CreateNativePixmap From CopyBuffer results in > a deadlock, as CopyBuffer is called from DrmThread and CreateNativePixmap try to > post a synchronous task to DrmThread. Took a quick look and this isn't used anywhere anymore, right? Perhaps we can remove this API entry in a followup? https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:157: DCHECK(source_buffer->GetHandle() == buffer_->GetHandle()); On 2016/01/11 08:19:51, kalyank wrote: > This will be called in Drmthread by OverlayValidator. Perhaps add some comments explaining this since it won't be obvious why the pixmap was initialized this way. https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:159: ui::SurfaceFactoryOzone* factory = platform->GetSurfaceFactoryOzone(); You could just drop this since this is |surface_manager_|
https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/drm_overlay_validator.h (right): https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/drm_overlay_validator.h:32: OverlayPlaneList ValidatePlanesForPageFlip( On 2016/01/13 17:26:51, dnicoara wrote: > Could you please add some comments describing this. In particular, focus on the > fact that this optimizes formats and such but doesn't actually change anything > in how planes are presented, etc. > > Also, the name isn't quite right. At first I though this was a second "test page > flip" which didn't seem right. Maybe something like OptimizeBuffersForPageFlip() > or HandleBufferHints()? Feel free to suggest others. Done. https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/drm_overlay_validator.h:34: ScanoutBufferGenerator* buffer_generator); On 2016/01/13 17:26:51, dnicoara wrote: > Maybe we should pass in the buffer generator in the constructor. It'll simplify > this call and TestPageFlip(). Done. https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/drm_overlay_validator.h:59: // Expectation is that we have same combination of layers being used over On 2016/01/13 17:26:51, dnicoara wrote: > Do you have an idea of what the overhead would be? I'm wondering if the extra > complexity is needed since changing the overlay configuration would not be a > frequent operation and this would only help the first frame after the change. We would end up allocating new buffers every frame. > In fact, I'm almost tempted to say this is optimized at the wrong layer. If we > needed to apply scaling, I imagine we'd want to cache the new buffer within the > GbmPixmap such that we'd re-use the buffer every time. > > It seems like the "void CopyBuffer(const scoped_refptr<ScanoutBuffer>, const > scoped_refptr<ScanoutBuffer>)" should be more like: > "scoped_refptr<ScanoutBuffer> GetProcessedBuffer(BufferFormat)" I started of on this but needed to have access to DrmDevice/Buffer generator etc here. https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/drm_thread.cc (right): https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/drm_thread.cc:114: window->SchedulePageFlip(planes, callback, buffer_generator_.get()); On 2016/01/13 17:26:51, dnicoara wrote: > Looking over the change and I'm thinking that the buffer_generator_ should just > be passed in the constructor. There's no reason we need it passed in on every > call. Done. https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:95: return nullptr; On 2016/01/13 17:26:51, dnicoara wrote: > On 2016/01/11 08:19:51, kalyank wrote: > > This is not used any more. Call to CreateNativePixmap From CopyBuffer results > in > > a deadlock, as CopyBuffer is called from DrmThread and CreateNativePixmap try > to > > post a synchronous task to DrmThread. > > Took a quick look and this isn't used anywhere anymore, right? Perhaps we can > remove this API entry in a followup? We should be, this is public interface and wasn't sure if any other ozone implementations depend on this. https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:157: DCHECK(source_buffer->GetHandle() == buffer_->GetHandle()); On 2016/01/13 17:26:51, dnicoara wrote: > On 2016/01/11 08:19:51, kalyank wrote: > > This will be called in Drmthread by OverlayValidator. > > Perhaps add some comments explaining this since it won't be obvious why the > pixmap was initialized this way. Done. https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:159: ui::SurfaceFactoryOzone* factory = platform->GetSurfaceFactoryOzone(); On 2016/01/13 17:26:51, dnicoara wrote: > You could just drop this since this is |surface_manager_| Done.
https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/drm_overlay_validator.h (right): https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/drm_overlay_validator.h:59: // Expectation is that we have same combination of layers being used over On 2016/01/13 19:52:33, kalyank wrote: > On 2016/01/13 17:26:51, dnicoara wrote: > > Do you have an idea of what the overhead would be? I'm wondering if the extra > > complexity is needed since changing the overlay configuration would not be a > > frequent operation and this would only help the first frame after the change. > > We would end up allocating new buffers every frame. > I think we can just drop this. The more I read it the more I'm convinced that it'll cause issues (Based on my reading it looks like it'll re-use buffers incorrectly as well). Anyways, I think the old approach you were trying (the one mentioned bellow) can work. Could you try that first? > > In fact, I'm almost tempted to say this is optimized at the wrong layer. If we > > needed to apply scaling, I imagine we'd want to cache the new buffer within > the > > GbmPixmap such that we'd re-use the buffer every time. > > > > It seems like the "void CopyBuffer(const scoped_refptr<ScanoutBuffer>, const > > scoped_refptr<ScanoutBuffer>)" should be more like: > > "scoped_refptr<ScanoutBuffer> GetProcessedBuffer(BufferFormat)" > > I started of on this but needed to have access to DrmDevice/Buffer generator etc > here. > The buffer has access to DrmDevice and we could just allocate without the buffer generator. We're replacing one buffer with another and given that we start with a GbmBuffer, I think it is fair to assume you'll get a GbmBuffer. Let me know if you still have concerns. https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:95: return nullptr; On 2016/01/13 19:52:33, kalyank wrote: > On 2016/01/13 17:26:51, dnicoara wrote: > > On 2016/01/11 08:19:51, kalyank wrote: > > > This is not used any more. Call to CreateNativePixmap From CopyBuffer > results > > in > > > a deadlock, as CopyBuffer is called from DrmThread and CreateNativePixmap > try > > to > > > post a synchronous task to DrmThread. > > > > Took a quick look and this isn't used anywhere anymore, right? Perhaps we can > > remove this API entry in a followup? > > We should be, this is public interface and wasn't sure if any other ozone > implementations depend on this. Acknowledged. Haven't seen any, so lets just remove it in a followup.
https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/drm_overlay_validator.h (right): https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/drm_overlay_validator.h:59: // Expectation is that we have same combination of layers being used over On 2016/01/13 20:21:53, dnicoara wrote: > On 2016/01/13 19:52:33, kalyank wrote: > > On 2016/01/13 17:26:51, dnicoara wrote: > > > Do you have an idea of what the overhead would be? I'm wondering if the > extra > > > complexity is needed since changing the overlay configuration would not be a > > > frequent operation and this would only help the first frame after the > change. > > > > We would end up allocating new buffers every frame. > > > > I think we can just drop this. The more I read it the more I'm convinced that > it'll cause issues (Based on my reading it looks like it'll re-use buffers > incorrectly as well). Anyways, I think the old approach you were trying (the one > mentioned bellow) can work. Could you try that first? > > > > In fact, I'm almost tempted to say this is optimized at the wrong layer. If > we > > > needed to apply scaling, I imagine we'd want to cache the new buffer within > > the > > > GbmPixmap such that we'd re-use the buffer every time. > > > > > > It seems like the "void CopyBuffer(const scoped_refptr<ScanoutBuffer>, const > > > scoped_refptr<ScanoutBuffer>)" should be more like: > > > "scoped_refptr<ScanoutBuffer> GetProcessedBuffer(BufferFormat)" > > > > I started of on this but needed to have access to DrmDevice/Buffer generator > etc > > here. > > > > The buffer has access to DrmDevice and we could just allocate without the buffer > generator. We're replacing one buffer with another and given that we start with > a GbmBuffer, I think it is fair to assume you'll get a GbmBuffer. > > Let me know if you still have concerns. Done. https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1426993003/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:95: return nullptr; On 2016/01/13 20:21:53, dnicoara wrote: > On 2016/01/13 19:52:33, kalyank wrote: > > On 2016/01/13 17:26:51, dnicoara wrote: > > > On 2016/01/11 08:19:51, kalyank wrote: > > > > This is not used any more. Call to CreateNativePixmap From CopyBuffer > > results > > > in > > > > a deadlock, as CopyBuffer is called from DrmThread and CreateNativePixmap > > try > > > to > > > > post a synchronous task to DrmThread. > > > > > > Took a quick look and this isn't used anywhere anymore, right? Perhaps we > can > > > remove this API entry in a followup? > > > > We should be, this is public interface and wasn't sure if any other ozone > > implementations depend on this. > > Acknowledged. Haven't seen any, so lets just remove it in a followup. k, will post a followup patch for this.
lgtm % some minor changes https://codereview.chromium.org/1426993003/diff/300001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1426993003/diff/300001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:167: gbm, buffer_format, size, gfx::BufferUsage::SCANOUT); For usage, I think it'd be better to use the old buffer's usage flag. https://codereview.chromium.org/1426993003/diff/300001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer.h (right): https://codereview.chromium.org/1426993003/diff/300001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.h:82: scoped_refptr<GbmPixmap> processed_pixmap_; nit: Would you mind adding a comment for this. https://codereview.chromium.org/1426993003/diff/300001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer_base.h (right): https://codereview.chromium.org/1426993003/diff/300001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer_base.h:43: scoped_refptr<DrmDevice> drm_; This could be updated to GbmDevice. That'd help with the casts. The only reason I never updated this to a GbmDevice was because it wasn't needed at the time.
https://codereview.chromium.org/1426993003/diff/300001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1426993003/diff/300001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:167: gbm, buffer_format, size, gfx::BufferUsage::SCANOUT); On 2016/01/14 19:30:02, dnicoara wrote: > For usage, I think it'd be better to use the old buffer's usage flag. Done. https://codereview.chromium.org/1426993003/diff/300001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer.h (right): https://codereview.chromium.org/1426993003/diff/300001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.h:82: scoped_refptr<GbmPixmap> processed_pixmap_; On 2016/01/14 19:30:02, dnicoara wrote: > nit: Would you mind adding a comment for this. Done. https://codereview.chromium.org/1426993003/diff/300001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer_base.h (right): https://codereview.chromium.org/1426993003/diff/300001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer_base.h:43: scoped_refptr<DrmDevice> drm_; On 2016/01/14 19:30:02, dnicoara wrote: > This could be updated to GbmDevice. That'd help with the casts. > > The only reason I never updated this to a GbmDevice was because it wasn't needed > at the time. Done.
The CQ bit was checked by kalyan.kondapally@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from dnicoara@chromium.org Link to the patchset: https://codereview.chromium.org/1426993003/#ps320001 (title: "Review fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1426993003/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1426993003/320001
Message was sent while issue was closed.
Description was changed from ========== Ozone: Dont hardcode format to YUV when using Overlay Composition. We currently hardcode formats to YUV for Video used during Hardware Overlay composition. All platforms might not support this format and also in future we might want to support other formats like NV12. Also, in certain use cases like FullScreen using YUV might not be right format as Primary plane might not support it. Instead of hardcoding it, we should rather use OverlayValidator to decide the right format. This CL adds needed support. BUG=553264 ========== to ========== Ozone: Dont hardcode format to YUV when using Overlay Composition. We currently hardcode formats to YUV for Video used during Hardware Overlay composition. All platforms might not support this format and also in future we might want to support other formats like NV12. Also, in certain use cases like FullScreen using YUV might not be right format as Primary plane might not support it. Instead of hardcoding it, we should rather use OverlayValidator to decide the right format. This CL adds needed support. BUG=553264 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Ozone: Dont hardcode format to YUV when using Overlay Composition. We currently hardcode formats to YUV for Video used during Hardware Overlay composition. All platforms might not support this format and also in future we might want to support other formats like NV12. Also, in certain use cases like FullScreen using YUV might not be right format as Primary plane might not support it. Instead of hardcoding it, we should rather use OverlayValidator to decide the right format. This CL adds needed support. BUG=553264 ========== to ========== Ozone: Dont hardcode format to YUV when using Overlay Composition. We currently hardcode formats to YUV for Video used during Hardware Overlay composition. All platforms might not support this format and also in future we might want to support other formats like NV12. Also, in certain use cases like FullScreen using YUV might not be right format as Primary plane might not support it. Instead of hardcoding it, we should rather use OverlayValidator to decide the right format. This CL adds needed support. BUG=553264 Committed: https://crrev.com/ba1c3b03daef7adfd3e684b5228b4f65e95a113c Cr-Commit-Position: refs/heads/master@{#369546} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/ba1c3b03daef7adfd3e684b5228b4f65e95a113c Cr-Commit-Position: refs/heads/master@{#369546} |
