Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1422563002:
[Ozone] Enables overlay render format setting path and by default use UYVY (Closed)
|
Created:
5 years, 2 months ago by william.xie1 Modified:
5 years, 1 month ago Reviewers:
Ken Russell (switch to Gerrit), marcheu1, reveman, ccameron, piman, kalyank, alexst (slow to review), marcheu, seanvk, Pawel Osciak, spang CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, ozone-reviews_chromium.org, piman+watch_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Ozone] Enables overlay render format setting path and by default use UYVY
When play on overlay, current video render format is XRGB8888 which bpp is 32 bits,
this CL enables the overlay render format setting path and by default use UYVY which
bpp is 16 bits.So this optimization saves half display bandwidth and thus save power
consumption too.
BUG=
TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top,
calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer
by data_size/(width*height), it should be 2(16bits).
Committed: https://crrev.com/3c17cd45fa74186399b453d34fb4a441933e7c2e
Cr-Commit-Position: refs/heads/master@{#358836}
Patch Set 1 #
Total comments: 14
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #Patch Set 6 : #
Total comments: 8
Patch Set 7 : #
Total comments: 10
Patch Set 8 : #
Total comments: 8
Patch Set 9 : #
Total comments: 18
Patch Set 10 : #
Total comments: 9
Patch Set 11 : #
Total comments: 10
Patch Set 12 : #Patch Set 13 : #
Total comments: 11
Patch Set 14 : #
Total comments: 4
Patch Set 15 : #Messages
Total messages: 128 (40 generated)
Description was changed from ========== [Ozone] Optimize video render format from XRGB8888 to UYVY Current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits. So this optimization saves half display bandwidth and thus save power consumption too. BUG= ========== to ========== [Ozone] Optimize video render format from XRGB8888 to UYVY Current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits. So this optimization saves half display bandwidth and thus save power consumption too. BUG= ==========
william.xie@intel.com changed reviewers: + alexst@chromium.org, marcheu@chromium.org, spang@chromium.org
PTAL
https://codereview.chromium.org/1422563002/diff/1/ui/ozone/public/native_pixm... File ui/ozone/public/native_pixmap.h (right): https://codereview.chromium.org/1422563002/diff/1/ui/ozone/public/native_pixm... ui/ozone/public/native_pixmap.h:31: virtual int GetPixelFormat() = 0; This should return gfx::BufferFormat, not the internal format.
kalyan.kondapally@intel.com changed reviewers: + kalyan.kondapally@intel.com
https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_drm_picture.cc:25: return VA_FOURCC_BGRX; Hmm do we need this conversion ? GBM_FORMAT_* are FOURCC formats https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_drm_picture.cc:180: scaled_pixmap_ = CreateNativePixmap(new_size, gfx::BufferFormat::UYVY_422); We use UYUY format when scaling is needed otherwise BGRX, why ? https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_wrapper.cc:943: const scoped_refptr<VASurface>& va_surface_src, These changes seem unrelated, can you separate them to another CL?
marcheu@google.com changed reviewers: + marcheu@google.com
https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_drm_picture.cc:25: return VA_FOURCC_BGRX; On 2015/10/21 19:48:41, kalyank wrote: > Hmm do we need this conversion ? GBM_FORMAT_* are FOURCC formats Sadly, there are formats like GBM_BO_FORMAT_XRGB8888 which aren't fourcc ones. Now granted they aren't used here, so maybe it doesn't matter. But if we just pass through the same format, we should at least test those two.
We might want to drop addfb (non-2) usage completely as well. I don't think there's a reason to use it at this point.
On 2015/10/21 20:00:11, marcheu1 wrote: > We might want to drop addfb (non-2) usage completely as well. I don't think > there's a reason to use it at this point. I was thinking this is useful for NV12 support, but ya no reason to use it at this point.
https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_drm_picture.cc:25: return VA_FOURCC_BGRX; On 2015/10/21 19:58:57, marcheu1 wrote: > On 2015/10/21 19:48:41, kalyank wrote: > > Hmm do we need this conversion ? GBM_FORMAT_* are FOURCC formats > > Sadly, there are formats like GBM_BO_FORMAT_XRGB8888 which aren't fourcc ones. > Now granted they aren't used here, so maybe it doesn't matter. But if we just > pass through the same format, we should at least test those two. We shouldn't be using GBM_BO_FORMAT_XRGB8888 at all in chrome side. We could have DCHECK in place during va surface creation to ensure that we don't pass GBM_BO_* accidently in future ?
On 2015/10/21 20:20:00, kalyank wrote: > https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... > File content/common/gpu/media/vaapi_drm_picture.cc (right): > > https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... > content/common/gpu/media/vaapi_drm_picture.cc:25: return VA_FOURCC_BGRX; > On 2015/10/21 19:58:57, marcheu1 wrote: > > On 2015/10/21 19:48:41, kalyank wrote: > > > Hmm do we need this conversion ? GBM_FORMAT_* are FOURCC formats > > > > Sadly, there are formats like GBM_BO_FORMAT_XRGB8888 which aren't fourcc ones. > > Now granted they aren't used here, so maybe it doesn't matter. But if we just > > pass through the same format, we should at least test those two. > > We shouldn't be using GBM_BO_FORMAT_XRGB8888 at all in chrome side. Sorry, I meant both GBM_BO_FORMAT_XRGB and GBM_BO_FORMAT_ARGB type > We could have DCHECK in place during va surface creation to ensure that we don't > pass GBM_BO_* accidently in future ?
https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_drm_picture.cc:180: scaled_pixmap_ = CreateNativePixmap(new_size, gfx::BufferFormat::UYVY_422); Using YUV formats has benefits when video buffer can be directly consumed by display controller. Otherwise, I don't see any benefit of this as Compositor will need to convert to RGB before rendering to back buffer. We should have checks here to use this format only when Overlay support is enabled in Chrome.
sean.v.kelley@intel.com changed reviewers: + sean.v.kelley@intel.com
https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_drm_picture.cc:180: scaled_pixmap_ = CreateNativePixmap(new_size, gfx::BufferFormat::UYVY_422); Agree with @kalyank. We should not be assuming hardware overlay is always present.
Thank you for the patch. Could you please provide a TEST= line describing how and on which platforms this was tested and verified on? Does this change pass all ChromeOS video tests? Thank you.
william.xie@intel.com changed reviewers: + posciak@chromium.org
Description was changed from ========== [Ozone] Optimize video render format from XRGB8888 to UYVY Current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits. So this optimization saves half display bandwidth and thus save power consumption too. BUG= ========== to ========== [Ozone] Optimize video render format from XRGB8888 to UYVY Current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits. So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, check video render framebuffer bpp size is 16 in file:/sys/kernel/debug/dri/0/i915_gem_framebuffer file ==========
Description was changed from ========== [Ozone] Optimize video render format from XRGB8888 to UYVY Current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits. So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, check video render framebuffer bpp size is 16 in file:/sys/kernel/debug/dri/0/i915_gem_framebuffer file ========== to ========== [Ozone] Optimize video render format from XRGB8888 to UYVY Current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits. So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, check video render framebuffer bpp size is 16 in file:/sys/kernel/debug/dri/0/i915_gem_framebuffer file ==========
Description was changed from ========== [Ozone] Optimize video render format from XRGB8888 to UYVY Current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits. So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, check video render framebuffer bpp size is 16 in file:/sys/kernel/debug/dri/0/i915_gem_framebuffer file ========== to ========== [Ozone] Optimize video render format from XRGB8888 to UYVY Current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits. So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, check video render framebuffer bpp size is 16 in file:/sys/kernel/debug/dri/0/i915_gem_framebuffer ==========
Thank you, guys, PTAL. https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_drm_picture.cc:25: return VA_FOURCC_BGRX; On 2015/10/21 20:20:00, kalyank wrote: > On 2015/10/21 19:58:57, marcheu1 wrote: > > On 2015/10/21 19:48:41, kalyank wrote: > > > Hmm do we need this conversion ? GBM_FORMAT_* are FOURCC formats > > > > Sadly, there are formats like GBM_BO_FORMAT_XRGB8888 which aren't fourcc ones. > > Now granted they aren't used here, so maybe it doesn't matter. But if we just > > pass through the same format, we should at least test those two. > > We shouldn't be using GBM_BO_FORMAT_XRGB8888 at all in chrome side. > We could have DCHECK in place during va surface creation to ensure that we don't > pass GBM_BO_* accidently in future ? > Acknowledged. https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_drm_picture.cc:25: return VA_FOURCC_BGRX; On 2015/10/21 19:48:41, kalyank wrote: > Hmm do we need this conversion ? GBM_FORMAT_* are FOURCC formats Suggested by Spang, here is gfx::BufferFormat to VA surface format conversion now. https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_drm_picture.cc:180: scaled_pixmap_ = CreateNativePixmap(new_size, gfx::BufferFormat::UYVY_422); On 2015/10/21 21:38:34, seanvk wrote: > Agree with @kalyank. We should not be assuming hardware overlay is always > present. Done. https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_drm_picture.cc:180: scaled_pixmap_ = CreateNativePixmap(new_size, gfx::BufferFormat::UYVY_422); On 2015/10/21 21:32:23, kalyank wrote: > Using YUV formats has benefits when video buffer can be directly consumed by > display controller. Otherwise, I don't see any benefit of this as Compositor > will need to convert to RGB before rendering to back buffer. We should have > checks here to use this format only when Overlay support is enabled in Chrome. Done. https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... content/common/gpu/media/vaapi_wrapper.cc:943: const scoped_refptr<VASurface>& va_surface_src, On 2015/10/21 19:48:41, kalyank wrote: > These changes seem unrelated, can you separate them to another CL? Done. https://codereview.chromium.org/1422563002/diff/1/ui/ozone/public/native_pixm... File ui/ozone/public/native_pixmap.h (right): https://codereview.chromium.org/1422563002/diff/1/ui/ozone/public/native_pixm... ui/ozone/public/native_pixmap.h:31: virtual int GetPixelFormat() = 0; On 2015/10/21 18:27:01, spang wrote: > This should return gfx::BufferFormat, not the internal format. Done.
Thanks for the TEST= line, could you please also say: - which platforms this was tested on? - was this tested without overlays? - was this tested with ChromeOS video unittests?
On 2015/10/21 21:32:23, kalyank wrote: > https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... > File content/common/gpu/media/vaapi_drm_picture.cc (right): > > https://codereview.chromium.org/1422563002/diff/1/content/common/gpu/media/va... > content/common/gpu/media/vaapi_drm_picture.cc:180: scaled_pixmap_ = > CreateNativePixmap(new_size, gfx::BufferFormat::UYVY_422); > Using YUV formats has benefits when video buffer can be directly consumed by > display controller. Otherwise, I don't see any benefit of this as Compositor > will need to convert to RGB before rendering to back buffer. We should have > checks here to use this format only when Overlay support is enabled in Chrome. scaled_pixmap_ is only used for overlay, so as long as AllowOverlay() is true and scaled_pixmap_ is created, it means this pixmap will be issued to overlay.
On 2015/10/22 07:38:20, Pawel Osciak wrote: > Thanks for the TEST= line, could you please also say: > - which platforms this was tested on? > - was this tested without overlays? > - was this tested with ChromeOS video unittests? Dear Pawel, Platform: BDW Tested passed with overlay and without overlay. could you kindly let me know which unit test sets should I run?
On 2015/10/22 08:43:21, william.xie wrote: > On 2015/10/22 07:38:20, Pawel Osciak wrote: > > Thanks for the TEST= line, could you please also say: > > - which platforms this was tested on? > > - was this tested without overlays? > > - was this tested with ChromeOS video unittests? > > Dear Pawel, > > Platform: BDW > Tested passed with overlay and without overlay. > could you kindly let me know which unit test sets should I run? Thank you. Are you testing on ChromeOS? ChromeOS tests are in https://chromium.googlesource.com/chromiumos/third_party/autotest/+/HEAD/clie... (video_*), but it should be enough to just run video_VideoDecodeAccelerator, which uses content/common/gpu/media/video_decode_accelerator_unittest.cc in Chromium source.
On 2015/10/22 09:02:13, Pawel Osciak wrote: > On 2015/10/22 08:43:21, william.xie wrote: > > On 2015/10/22 07:38:20, Pawel Osciak wrote: > > > Thanks for the TEST= line, could you please also say: > > > - which platforms this was tested on? > > > - was this tested without overlays? > > > - was this tested with ChromeOS video unittests? > > > > Dear Pawel, > > > > Platform: BDW > > Tested passed with overlay and without overlay. > > could you kindly let me know which unit test sets should I run? > > > Thank you. Are you testing on ChromeOS? > > ChromeOS tests are in > https://chromium.googlesource.com/chromiumos/third_party/autotest/+/HEAD/clie... > (video_*), but it should be enough to just run video_VideoDecodeAccelerator, > which uses content/common/gpu/media/video_decode_accelerator_unittest.cc in > Chromium source. Dear Pawel, I tested video_decode_accelerator_unittest, all the test cases were passed. So there is no regression of this CL.
https://codereview.chromium.org/1422563002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1422563002/diff/20001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:181: // Fallback to RGB format is hardware overlay is not allowed as YUV format Apologies, I find this comment a bit confusing. Could you re-word/clarify a bit. https://codereview.chromium.org/1422563002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.h (right): https://codereview.chromium.org/1422563002/diff/20001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.h:12: #include <gbm.h> Please move this include to the file that uses its types. https://codereview.chromium.org/1422563002/diff/20001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/drm_device.h (right): https://codereview.chromium.org/1422563002/diff/20001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device.h:96: // CRTC will remove the newline here. https://codereview.chromium.org/1422563002/diff/20001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/gbm_buffer_base.cc (right): https://codereview.chromium.org/1422563002/diff/20001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/gbm_buffer_base.cc:30: if (fb_pixel_format_ == GBM_FORMAT_ARGB8888) Please refactor this into a switch statement, there is a lot going on here.
Thanks Alex's review, PTAL. https://codereview.chromium.org/1422563002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1422563002/diff/20001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:181: // Fallback to RGB format is hardware overlay is not allowed as YUV format On 2015/10/22 13:00:19, alexst (slow to review) wrote: > Apologies, I find this comment a bit confusing. Could you re-word/clarify a bit. Dear Alex, Actually it's safe to always create UYVY format for scaled_pixmap_ as scaled_pixmap_ is only used for overlay, so I would prefer to remove the fallback. Also aligned with Kalyan by IM on this. https://codereview.chromium.org/1422563002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.h (right): https://codereview.chromium.org/1422563002/diff/20001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.h:12: #include <gbm.h> On 2015/10/22 13:00:19, alexst (slow to review) wrote: > Please move this include to the file that uses its types. Done. https://codereview.chromium.org/1422563002/diff/20001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/gbm_buffer_base.cc (right): https://codereview.chromium.org/1422563002/diff/20001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/gbm_buffer_base.cc:30: if (fb_pixel_format_ == GBM_FORMAT_ARGB8888) On 2015/10/22 13:00:19, alexst (slow to review) wrote: > Please refactor this into a switch statement, there is a lot going on here. Done.
https://codereview.chromium.org/1422563002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1422563002/diff/20001/content/common/gpu/medi... content/common/gpu/media/vaapi_drm_picture.cc:181: // Fallback to RGB format is hardware overlay is not allowed as YUV format On 2015/10/22 14:18:10, william.xie wrote: > On 2015/10/22 13:00:19, alexst (slow to review) wrote: > > Apologies, I find this comment a bit confusing. Could you re-word/clarify a > bit. > > Dear Alex, > Actually it's safe to always create UYVY format for scaled_pixmap_ as > scaled_pixmap_ is only used for overlay, so I would prefer to remove the > fallback. Also aligned with Kalyan by IM on this. ScalePixmap is called only when we need to scale original pixmap. With these changes, what is happening is we use UYVY only if scaling is needed. You need to pass UYUY as format in Initialize() also. Understanding is that UYVY format will result in bandwidth saving during VPP write and 3D Engine read (In GPU composition case). But that's something which needs to be measured and understood. Also, you need to add support in cc layer for GPU composition of UYUY buffers. Mac recently added the needed support: https://code.google.com/p/chromium/issues/detail?id=510252 Not sure if we can reuse that for ChromeOS.
On 2015/10/22 12:45:42, william.xie wrote: > On 2015/10/22 09:02:13, Pawel Osciak wrote: > > On 2015/10/22 08:43:21, william.xie wrote: > > > On 2015/10/22 07:38:20, Pawel Osciak wrote: > > > > Thanks for the TEST= line, could you please also say: > > > > - which platforms this was tested on? > > > > - was this tested without overlays? > > > > - was this tested with ChromeOS video unittests? > > > > > > Dear Pawel, > > > > > > Platform: BDW > > > Tested passed with overlay and without overlay. > > > could you kindly let me know which unit test sets should I run? > > > > > > Thank you. Are you testing on ChromeOS? > > > > ChromeOS tests are in > > > https://chromium.googlesource.com/chromiumos/third_party/autotest/+/HEAD/clie... > > (video_*), but it should be enough to just run video_VideoDecodeAccelerator, > > which uses content/common/gpu/media/video_decode_accelerator_unittest.cc in > > Chromium source. > > Dear Pawel, > I tested video_decode_accelerator_unittest, all the test cases were passed. > So there is no regression of this CL. William, Have you done any GPU benchmarking to provide comparison data? It would be good to quantify the improvements. Thanks, Sean
On 2015/10/22 15:39:59, kalyank wrote: > https://codereview.chromium.org/1422563002/diff/20001/content/common/gpu/medi... > File content/common/gpu/media/vaapi_drm_picture.cc (right): > > https://codereview.chromium.org/1422563002/diff/20001/content/common/gpu/medi... > content/common/gpu/media/vaapi_drm_picture.cc:181: // Fallback to RGB format is > hardware overlay is not allowed as YUV format > On 2015/10/22 14:18:10, william.xie wrote: > > On 2015/10/22 13:00:19, alexst (slow to review) wrote: > > > Apologies, I find this comment a bit confusing. Could you re-word/clarify a > > bit. > > > > Dear Alex, > > Actually it's safe to always create UYVY format for scaled_pixmap_ as > > scaled_pixmap_ is only used for overlay, so I would prefer to remove the > > fallback. Also aligned with Kalyan by IM on this. > > ScalePixmap is called only when we need to scale original pixmap. With these > changes, what is happening is we use UYVY only if scaling is needed. You need to > pass UYUY as format in Initialize() also. > > Understanding is that UYVY format will result in bandwidth saving during VPP > write and 3D Engine read (In GPU composition case). But that's something which > needs to be measured and understood. > > Also, you need to add support in cc layer for GPU composition of UYUY buffers. > Mac recently added the needed support: > https://code.google.com/p/chromium/issues/detail?id=510252 > Not sure if we can reuse that for ChromeOS. Thanks Kalyan, this CL optimizes video render format on overlay from BGRA to UYVY, for the UYVY composition, it is better to be a separated CL as they are two different optimizations from different perspective.
On 2015/10/22 16:20:38, seanvk wrote: > On 2015/10/22 12:45:42, william.xie wrote: > > On 2015/10/22 09:02:13, Pawel Osciak wrote: > > > On 2015/10/22 08:43:21, william.xie wrote: > > > > On 2015/10/22 07:38:20, Pawel Osciak wrote: > > > > > Thanks for the TEST= line, could you please also say: > > > > > - which platforms this was tested on? > > > > > - was this tested without overlays? > > > > > - was this tested with ChromeOS video unittests? > > > > > > > > Dear Pawel, > > > > > > > > Platform: BDW > > > > Tested passed with overlay and without overlay. > > > > could you kindly let me know which unit test sets should I run? > > > > > > > > > Thank you. Are you testing on ChromeOS? > > > > > > ChromeOS tests are in > > > > > > https://chromium.googlesource.com/chromiumos/third_party/autotest/+/HEAD/clie... > > > (video_*), but it should be enough to just run video_VideoDecodeAccelerator, > > > which uses content/common/gpu/media/video_decode_accelerator_unittest.cc in > > > Chromium source. > > > > Dear Pawel, > > I tested video_decode_accelerator_unittest, all the test cases were passed. > > So there is no regression of this CL. > > William, > > Have you done any GPU benchmarking to provide comparison data? It would be good > to quantify the improvements. > > Thanks, > > Sean Dear Sean, Yes, I measured the total system power consumption, it improved 6% on BDW platform with 720p h264 fullscreen playback on overlay.
On 2015/10/22 21:23:59, william.xie wrote: > On 2015/10/22 15:39:59, kalyank wrote: > > > https://codereview.chromium.org/1422563002/diff/20001/content/common/gpu/medi... > > File content/common/gpu/media/vaapi_drm_picture.cc (right): > > > > > https://codereview.chromium.org/1422563002/diff/20001/content/common/gpu/medi... > > content/common/gpu/media/vaapi_drm_picture.cc:181: // Fallback to RGB format > is > > hardware overlay is not allowed as YUV format > > On 2015/10/22 14:18:10, william.xie wrote: > > > On 2015/10/22 13:00:19, alexst (slow to review) wrote: > > > > Apologies, I find this comment a bit confusing. Could you re-word/clarify > a > > > bit. > > > > > > Dear Alex, > > > Actually it's safe to always create UYVY format for scaled_pixmap_ as > > > scaled_pixmap_ is only used for overlay, so I would prefer to remove the > > > fallback. Also aligned with Kalyan by IM on this. > > > > ScalePixmap is called only when we need to scale original pixmap. With these > > changes, what is happening is we use UYVY only if scaling is needed. You need > to > > pass UYUY as format in Initialize() also. > > > > Understanding is that UYVY format will result in bandwidth saving during VPP > > write and 3D Engine read (In GPU composition case). But that's something which > > needs to be measured and understood. > > > > Also, you need to add support in cc layer for GPU composition of UYUY buffers. > > Mac recently added the needed support: > > https://code.google.com/p/chromium/issues/detail?id=510252 > > Not sure if we can reuse that for ChromeOS. > > Thanks Kalyan, this CL optimizes video render format on overlay from BGRA to > UYVY, > for the UYVY composition, it is better to be a separated CL as they are two > different optimizations from different perspective. What we are doing here is using UYVY format only in case of using Overlay and need to scale the original video buffer(Check GbmPixmap::ScheduleOverlayPlane). So, even in Overlay case we are not always going to use it. That's the reason you haven't seen any graphics artifacts when switching between GL and Overlay composition i.e. if you move the cursor over video which should bring up volume bar and force GL composition. I am ok if you want to focus this CL only to optimize Scaling case. If so, can you update the title so its clear. The part which doesn't look good is that we are doing all atomic tests (Before promoting a layer to use overlays), with particular configuration and end up using entirely different in this case. i.e. RGB vs YUV
https://codereview.chromium.org/1422563002/diff/40001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/gbm_buffer_base.cc (right): https://codereview.chromium.org/1422563002/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/gbm_buffer_base.cc:35: if (!drm_->AddFramebuffer(gbm_bo_get_width(bo), gbm_bo_get_height(bo), As @marcheu1 pointed out we should just use AddFramebuffer2 here.
On 2015/10/22 22:22:05, kalyank wrote: > On 2015/10/22 21:23:59, william.xie wrote: > > On 2015/10/22 15:39:59, kalyank wrote: > > > > > > https://codereview.chromium.org/1422563002/diff/20001/content/common/gpu/medi... > > > File content/common/gpu/media/vaapi_drm_picture.cc (right): > > > > > > > > > https://codereview.chromium.org/1422563002/diff/20001/content/common/gpu/medi... > > > content/common/gpu/media/vaapi_drm_picture.cc:181: // Fallback to RGB format > > is > > > hardware overlay is not allowed as YUV format > > > On 2015/10/22 14:18:10, william.xie wrote: > > > > On 2015/10/22 13:00:19, alexst (slow to review) wrote: > > > > > Apologies, I find this comment a bit confusing. Could you > re-word/clarify > > a > > > > bit. > > > > > > > > Dear Alex, > > > > Actually it's safe to always create UYVY format for scaled_pixmap_ as > > > > scaled_pixmap_ is only used for overlay, so I would prefer to remove the > > > > fallback. Also aligned with Kalyan by IM on this. > > > > > > ScalePixmap is called only when we need to scale original pixmap. With these > > > changes, what is happening is we use UYVY only if scaling is needed. You > need > > to > > > pass UYUY as format in Initialize() also. > > > > > > Understanding is that UYVY format will result in bandwidth saving during VPP > > > write and 3D Engine read (In GPU composition case). But that's something > which > > > needs to be measured and understood. > > > > > > Also, you need to add support in cc layer for GPU composition of UYUY > buffers. > > > Mac recently added the needed support: > > > https://code.google.com/p/chromium/issues/detail?id=510252 > > > Not sure if we can reuse that for ChromeOS. > > > > Thanks Kalyan, this CL optimizes video render format on overlay from BGRA to > > UYVY, > > for the UYVY composition, it is better to be a separated CL as they are two > > different optimizations from different perspective. > > What we are doing here is using UYVY format only in case of using Overlay and > need to scale the original video buffer(Check GbmPixmap::ScheduleOverlayPlane). > So, even in Overlay case we are not always going to use it. That's the reason > you haven't seen any graphics artifacts when switching between GL and Overlay > composition i.e. if you move the cursor over video which should bring up volume > bar and force GL composition. I am ok if you want to focus this CL only to > optimize > Scaling case. If so, can you update the title so its clear. > > The part which doesn't look good is that we are doing all atomic tests > (Before promoting a layer to use overlays), with particular configuration and > end up using entirely different in this case. i.e. RGB vs YUV Thanks so much, Kalyan, I changed the title to make it clear.
Thank you, guys, PTAL
PTAL https://codereview.chromium.org/1422563002/diff/40001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/gbm_buffer_base.cc (right): https://codereview.chromium.org/1422563002/diff/40001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/gbm_buffer_base.cc:35: if (!drm_->AddFramebuffer(gbm_bo_get_width(bo), gbm_bo_get_height(bo), On 2015/10/22 22:22:28, kalyank wrote: > As @marcheu1 pointed out we should just use AddFramebuffer2 here. Done.
Description was changed from ========== [Ozone] Optimize video render format from XRGB8888 to UYVY Current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits. So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, check video render framebuffer bpp size is 16 in file:/sys/kernel/debug/dri/0/i915_gem_framebuffer ========== to ========== [Ozone] Optimize video render format from XRGB8888 to UYVY when play on overlay with scaling When play on overlay with scaling, current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits. So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, check video render framebuffer bpp size is 16 in file:/sys/kernel/debug/dri/0/i915_gem_framebuffer ==========
Description was changed from ========== [Ozone] Optimize video render format from XRGB8888 to UYVY when play on overlay with scaling When play on overlay with scaling, current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits. So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, check video render framebuffer bpp size is 16 in file:/sys/kernel/debug/dri/0/i915_gem_framebuffer ========== to ========== [Ozone] Optimize video render format from XRGB8888 to UYVY when play on overlay with scaling When play on overlay with scaling, current video render format is XRGB8888 which bpp is 32 bits,this CL optimizes it to UYVY format which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, check video render framebuffer bpp size is 16 in file:/sys/kernel/debug/dri/0/i915_gem_framebuffer ==========
Description was changed from ========== [Ozone] Optimize video render format from XRGB8888 to UYVY when play on overlay with scaling When play on overlay with scaling, current video render format is XRGB8888 which bpp is 32 bits,this CL optimizes it to UYVY format which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, check video render framebuffer bpp size is 16 in file:/sys/kernel/debug/dri/0/i915_gem_framebuffer ========== to ========== [Ozone] Optimize video render format from XRGB8888 to UYVY when play on overlay with scaling When play on overlay with scaling, current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, check video render framebuffer bpp size is 16 in file:/sys/kernel/debug/dri/0/i915_gem_framebuffer ==========
Description was changed from ========== [Ozone] Optimize video render format from XRGB8888 to UYVY when play on overlay with scaling When play on overlay with scaling, current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, check video render framebuffer bpp size is 16 in file:/sys/kernel/debug/dri/0/i915_gem_framebuffer ========== to ========== [Ozone] Optimize video render format from XRGB8888 to UYVY when play on overlay with scaling When play on overlay with scaling, current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, check video render framebuffer bpp size is 16 in file:/sys/kernel/debug/dri/0/i915_gem_framebuffer ==========
Description was changed from ========== [Ozone] Optimize video render format from XRGB8888 to UYVY when play on overlay with scaling When play on overlay with scaling, current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, check video render framebuffer bpp size is 16 in file:/sys/kernel/debug/dri/0/i915_gem_framebuffer ========== to ========== [Ozone] Optimize video render format from XRGB8888 to UYVY when play on overlay with scaling When play on overlay with scaling, current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size in file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(widthxheight), it should be 16. ==========
Description was changed from ========== [Ozone] Optimize video render format from XRGB8888 to UYVY when play on overlay with scaling When play on overlay with scaling, current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size in file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(widthxheight), it should be 16. ========== to ========== [Ozone] Optimize video render format from XRGB8888 to UYVY when play on overlay with scaling When play on overlay with scaling, current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(widthxheight), it should be 16. ==========
Description was changed from ========== [Ozone] Optimize video render format from XRGB8888 to UYVY when play on overlay with scaling When play on overlay with scaling, current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(widthxheight), it should be 16. ========== to ========== [Ozone] Optimize video render format from XRGB8888 to UYVY when play on overlay with scaling When play on overlay with scaling, current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(widthxheight), it should be 2(16bits). ==========
Description was changed from ========== [Ozone] Optimize video render format from XRGB8888 to UYVY when play on overlay with scaling When play on overlay with scaling, current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(widthxheight), it should be 2(16bits). ========== to ========== [Ozone] Optimize video render format from XRGB8888 to UYVY when play on overlay with scaling When play on overlay with scaling, current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(width*height), it should be 2(16bits). ==========
https://codereview.chromium.org/1422563002/diff/60001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/drm_device.h (right): https://codereview.chromium.org/1422563002/diff/60001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device.h:85: // will join with next line https://codereview.chromium.org/1422563002/diff/60001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/gbm_buffer.h (right): https://codereview.chromium.org/1422563002/diff/60001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/gbm_buffer.h:54: gfx::BufferFormat GetPixelFormat() override; It's gfx::BufferFormat so GetBufferFormat().
Thanks Michael, PTAL. https://codereview.chromium.org/1422563002/diff/60001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/drm_device.h (right): https://codereview.chromium.org/1422563002/diff/60001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/drm_device.h:85: // will On 2015/10/23 17:00:50, spang wrote: > join with next line Done. https://codereview.chromium.org/1422563002/diff/60001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/gbm_buffer.h (right): https://codereview.chromium.org/1422563002/diff/60001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/gbm_buffer.h:54: gfx::BufferFormat GetPixelFormat() override; On 2015/10/23 17:00:50, spang wrote: > It's gfx::BufferFormat so GetBufferFormat(). Done.
We actually didn't take Fullscreen case into account. Think about the case where we have Overlays support enabled and Video is in full screen: Currently, we request display controller to update both primary and overlay. If primary is obscured by overlay, we want to bound the buffer meant for Overlay to primary (provided primary can support the buffer format) and avoid the usage of overlay plane completely. This would gives us more Bandwidth savings in full screen case(I have a patch fixing this). This combined with avoiding any GL composition in this case i.e. https://codereview.chromium.org/1330563004/ would give us potential savings even on Kernels which don't have Atomic page flip support i.e. We aren't going to use more than one plane. However, UYVY is not well supported with Primary planes and would force us to use both Primary and Overlay in full screen case. I am probably ignoring the benefits of FrameBufferCompression in case of Primary here. So, I think we will benefit from this only when scaling is needed in non-fullscreen cases and should be restricted to that usage only. Did your tests prove other wise ?
On 2015/10/26 01:21:51, kalyank wrote: > We actually didn't take Fullscreen case into account. > > Think about the case where we have Overlays support enabled and > Video is in full screen: > Currently, we request display controller to update both primary and overlay. > If primary is obscured by overlay, we want to bound the buffer meant > for Overlay to primary (provided primary can support the buffer format) > and avoid the usage of overlay plane completely. This would gives us > more Bandwidth savings in full screen case(I have a patch fixing this). > This combined with avoiding any GL composition in this case i.e. > https://codereview.chromium.org/1330563004/ > would give us potential savings even on Kernels which don't have Atomic > page flip support i.e. We aren't going to use more than one plane. > > However, UYVY is not well supported with Primary planes and would force us > to use both Primary and Overlay in full screen case. I am probably ignoring > the benefits of FrameBufferCompression in case of Primary here. So, I think > we will benefit from this only when scaling is needed in non-fullscreen cases > and should be restricted to that usage only. > Did your tests prove other wise ? Thanks Kalyan. From our measurement, both fullscreen and non-fullscreen case, there are power benefit. And even better in fullscreen mode as browser will mute the browser UI. In this case, using overlay in UYVY format is much better than primary plane, just as some Windows player does.
> From our measurement, both fullscreen and non-fullscreen case, there are power > benefit. > And even better in fullscreen mode as browser will mute the browser UI. In this > case, > using overlay in UYVY format is much better than primary plane, just as some > Windows player does. This is expected as we currently don't collapse the planes to one in fullscreen case. Also, consider cases where Primary of given HW can actually support these formats i.e. NV12, YUV etc. Hardcoding the format in this case seems wrong.
On 2015/10/26 16:13:15, kalyank wrote: > > From our measurement, both fullscreen and non-fullscreen case, there are power > > benefit. > > And even better in fullscreen mode as browser will mute the browser UI. In > this > > case, > > using overlay in UYVY format is much better than primary plane, just as some > > Windows player does. > > This is expected as we currently don't collapse the planes to one in fullscreen > case. Also, consider cases where Primary of given HW can actually support these > formats i.e. NV12, YUV etc. Hardcoding the format in this case seems wrong. Can you also drop addfb2 completely? There is no reason to leave cruft behind.
On 2015/10/26 18:08:13, marcheu wrote: > On 2015/10/26 16:13:15, kalyank wrote: > > > From our measurement, both fullscreen and non-fullscreen case, there are > power > > > benefit. > > > And even better in fullscreen mode as browser will mute the browser UI. In > > this > > > case, > > > using overlay in UYVY format is much better than primary plane, just as some > > > Windows player does. > > > > This is expected as we currently don't collapse the planes to one in > fullscreen > > case. Also, consider cases where Primary of given HW can actually support > these > > formats i.e. NV12, YUV etc. Hardcoding the format in this case seems wrong. > > Can you also drop addfb2 completely? There is no reason to leave cruft behind. Oops I meant drop addfb (non-2) of course
On 2015/10/26 16:13:15, kalyank wrote: > > From our measurement, both fullscreen and non-fullscreen case, there are power > > benefit. > > And even better in fullscreen mode as browser will mute the browser UI. In > this > > case, > > using overlay in UYVY format is much better than primary plane, just as some > > Windows player does. > > This is expected as we currently don't collapse the planes to one in fullscreen > case. Also, consider cases where Primary of given HW can actually support these > formats i.e. NV12, YUV etc. Hardcoding the format in this case seems wrong. Thank you Kalyank.
On 2015/10/26 18:09:01, marcheu wrote: > On 2015/10/26 18:08:13, marcheu wrote: > > On 2015/10/26 16:13:15, kalyank wrote: > > > > From our measurement, both fullscreen and non-fullscreen case, there are > > power > > > > benefit. > > > > And even better in fullscreen mode as browser will mute the browser UI. In > > > this > > > > case, > > > > using overlay in UYVY format is much better than primary plane, just as > some > > > > Windows player does. > > > > > > This is expected as we currently don't collapse the planes to one in > > fullscreen > > > case. Also, consider cases where Primary of given HW can actually support > > these > > > formats i.e. NV12, YUV etc. Hardcoding the format in this case seems wrong. > > > > Can you also drop addfb2 completely? There is no reason to leave cruft behind. > > Oops I meant drop addfb (non-2) of course Done, PTAL.
Patchset #6 (id:100001) has been deleted
did you forget to upload the latest change?
On 2015/10/26 22:15:46, marcheu wrote: > did you forget to upload the latest change? Oops, Sorry, Michael, PTAL.
lgtm
https://codereview.chromium.org/1422563002/diff/120001/content/common/gpu/med... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1422563002/diff/120001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:22: int GetVASurfaceFormatFromPixmapFormat(gfx::BufferFormat fmt) { s/GetVASurfaceFormatFromPixmapFormat/BufferFormatToVAFourCC/ s/int/uint32_t/ as this is fourcc Also, please put this in an anonymous namespace. https://codereview.chromium.org/1422563002/diff/120001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:96: va_attrib_extbuf.pixel_format == VA_FOURCC_UYVY ? VA_RT_FORMAT_YUV422 Please have a method for this conversion (VA_FOURCC -> VA_RT_FORMAT) instead. https://codereview.chromium.org/1422563002/diff/120001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:122: pixmap_ = CreateNativePixmap(size(), gfx::BufferFormat::BGRX_8888); Please instead of using constants directly in code, have these defined at the top of the file as constants with comments. https://codereview.chromium.org/1422563002/diff/120001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:181: scaled_pixmap_ = CreateNativePixmap(new_size, gfx::BufferFormat::UYVY_422); It's not obvious that "ScalePixmap" also does automatic conversion to 422 from whatever the source format was. Please extend this method to take format as an argument. Also, is there a way to somehow query in the upper layers what the overlay format is?
Thanks Pawel, PTAL https://codereview.chromium.org/1422563002/diff/120001/content/common/gpu/med... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1422563002/diff/120001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:22: int GetVASurfaceFormatFromPixmapFormat(gfx::BufferFormat fmt) { On 2015/10/29 11:28:36, Pawel Osciak wrote: > s/GetVASurfaceFormatFromPixmapFormat/BufferFormatToVAFourCC/ > s/int/uint32_t/ as this is fourcc > > Also, please put this in an anonymous namespace. Done. https://codereview.chromium.org/1422563002/diff/120001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:96: va_attrib_extbuf.pixel_format == VA_FOURCC_UYVY ? VA_RT_FORMAT_YUV422 On 2015/10/29 11:28:36, Pawel Osciak wrote: > Please have a method for this conversion (VA_FOURCC -> VA_RT_FORMAT) instead. Done. https://codereview.chromium.org/1422563002/diff/120001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:122: pixmap_ = CreateNativePixmap(size(), gfx::BufferFormat::BGRX_8888); On 2015/10/29 11:28:36, Pawel Osciak wrote: > Please instead of using constants directly in code, have these defined at the > top of the file as constants with comments. Done. https://codereview.chromium.org/1422563002/diff/120001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:181: scaled_pixmap_ = CreateNativePixmap(new_size, gfx::BufferFormat::UYVY_422); On 2015/10/29 11:28:36, Pawel Osciak wrote: > It's not obvious that "ScalePixmap" also does automatic conversion to 422 from > whatever the source format was. Please extend this method to take format as an > argument. > > Also, is there a way to somehow query in the upper layers what the overlay > format is? Dear Pawel, since this method is a callback function, do you really want to add an argument for that with many files changes? how do you think taking it(video_render_format_) as a new member of this class? and upper layers could query video render format too.
lgtm
lgtm lgtm
https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:197: scaled_pixmap_ = CreateNativePixmap(new_size, kPictureRenderFormat); Sorry, if I wasn't clear earlier, hardcoding the format here is wrong. In fullscreen case, the overall benefits for collapsing planes to one will outweigh using UYVY on platforms where primary cannot support this. I still think that there should be some pre-validation (by the caller ?) as which format is optimal, if it is supported by the hardware and use it in here.
https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:22: // Format for storing the video decoded pictures Nit: please use dots at the end of sentences. Also, I don't think this is actually for "storing", we decode into YUV420, but for usage with GLImages we have to convert to this format. https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:24: // Format for renderring the video pictures This is also not precise enough. We don't use this format when not using overlays. https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:197: scaled_pixmap_ = CreateNativePixmap(new_size, kPictureRenderFormat); On 2015/10/29 17:49:40, kalyank wrote: > Sorry, if I wasn't clear earlier, hardcoding the format here is wrong. In > fullscreen case, the overall benefits for collapsing planes to one will outweigh > using UYVY on platforms where primary cannot support this. I still think that > there should be some pre-validation (by the caller ?) as which format is > optimal, if it is supported by the hardware and use it in here. +1 to this, this is basically what I was hoping for when asking for a way to query upper layers for optimal format, both for overlays and non-overlay scenarios. How could we query for the optimal format?
Description was changed from ========== [Ozone] Optimize video render format from XRGB8888 to UYVY when play on overlay with scaling When play on overlay with scaling, current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(width*height), it should be 2(16bits). ========== to ========== [Ozone] Optimize video render format from XRGB8888 to UYVY when play on overlay When play on overlay, current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(width*height), it should be 2(16bits). ==========
Description was changed from ========== [Ozone] Optimize video render format from XRGB8888 to UYVY when play on overlay When play on overlay, current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(width*height), it should be 2(16bits). ========== to ========== [Ozone] Optimize video render format from XRGB8888 to UYVY when play on overlay When play on overlay, current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(width*height), it should be 2(16bits). ==========
PTAL https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:22: // Format for storing the video decoded pictures On 2015/10/30 02:38:34, Pawel Osciak wrote: > Nit: please use dots at the end of sentences. > Also, I don't think this is actually for "storing", we decode into YUV420, but > for usage with GLImages we have to convert to this format. Done. https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:24: // Format for renderring the video pictures On 2015/10/30 02:38:34, Pawel Osciak wrote: > This is also not precise enough. We don't use this format when not using > overlays. Done. https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:197: scaled_pixmap_ = CreateNativePixmap(new_size, kPictureRenderFormat); On 2015/10/29 17:49:40, kalyank wrote: > Sorry, if I wasn't clear earlier, hardcoding the format here is wrong. In > fullscreen case, the overall benefits for collapsing planes to one will outweigh > using UYVY on platforms where primary cannot support this. I still think that > there should be some pre-validation (by the caller ?) as which format is > optimal, if it is supported by the hardware and use it in here. Done. https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:197: scaled_pixmap_ = CreateNativePixmap(new_size, kPictureRenderFormat); On 2015/10/30 02:38:34, Pawel Osciak wrote: > On 2015/10/29 17:49:40, kalyank wrote: > > Sorry, if I wasn't clear earlier, hardcoding the format here is wrong. In > > fullscreen case, the overall benefits for collapsing planes to one will > outweigh > > using UYVY on platforms where primary cannot support this. I still think that > > there should be some pre-validation (by the caller ?) as which format is > > optimal, if it is supported by the hardware and use it in here. > > +1 to this, this is basically what I was hoping for when asking for a way to > query upper layers for optimal format, both for overlays and non-overlay > scenarios. > > How could we query for the optimal format? Done. https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:197: scaled_pixmap_ = CreateNativePixmap(new_size, kPictureRenderFormat); On 2015/10/30 02:38:34, Pawel Osciak wrote: > On 2015/10/29 17:49:40, kalyank wrote: > > Sorry, if I wasn't clear earlier, hardcoding the format here is wrong. In > > fullscreen case, the overall benefits for collapsing planes to one will > outweigh > > using UYVY on platforms where primary cannot support this. I still think that > > there should be some pre-validation (by the caller ?) as which format is > > optimal, if it is supported by the hardware and use it in here. > > +1 to this, this is basically what I was hoping for when asking for a way to > query upper layers for optimal format, both for overlays and non-overlay > scenarios. > > How could we query for the optimal format? We get the optimal format from ozone platform, currently we use the well known best format UYVY for overlay, but it is possible to change the render format dynamically based on the scenarios in the future.
https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:197: scaled_pixmap_ = CreateNativePixmap(new_size, kPictureRenderFormat); On 2015/10/30 08:52:17, william.xie wrote: > On 2015/10/30 02:38:34, Pawel Osciak wrote: > > On 2015/10/29 17:49:40, kalyank wrote: > > > Sorry, if I wasn't clear earlier, hardcoding the format here is wrong. In > > > fullscreen case, the overall benefits for collapsing planes to one will > > outweigh > > > using UYVY on platforms where primary cannot support this. I still think > that > > > there should be some pre-validation (by the caller ?) as which format is > > > optimal, if it is supported by the hardware and use it in here. > > > > +1 to this, this is basically what I was hoping for when asking for a way to > > query upper layers for optimal format, both for overlays and non-overlay > > scenarios. > > > > How could we query for the optimal format? > > We get the optimal format from ozone platform, currently we use the well known > best format UYVY for overlay, but it is possible to change the render format > dynamically based on the scenarios in the future. Keep in mind too that SoC x86 platforms may use other formats in the future for overlay. So I tend to agree that hardcoding this format is not a good idea. Sean
On 2015/10/30 15:20:43, seanvk wrote: > https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... > File content/common/gpu/media/vaapi_drm_picture.cc (right): > > https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... > content/common/gpu/media/vaapi_drm_picture.cc:197: scaled_pixmap_ = > CreateNativePixmap(new_size, kPictureRenderFormat); > On 2015/10/30 08:52:17, william.xie wrote: > > On 2015/10/30 02:38:34, Pawel Osciak wrote: > > > On 2015/10/29 17:49:40, kalyank wrote: > > > > Sorry, if I wasn't clear earlier, hardcoding the format here is wrong. In > > > > fullscreen case, the overall benefits for collapsing planes to one will > > > outweigh > > > > using UYVY on platforms where primary cannot support this. I still think > > that > > > > there should be some pre-validation (by the caller ?) as which format is > > > > optimal, if it is supported by the hardware and use it in here. > > > > > > +1 to this, this is basically what I was hoping for when asking for a way to > > > query upper layers for optimal format, both for overlays and non-overlay > > > scenarios. > > > > > > How could we query for the optimal format? > > > > We get the optimal format from ozone platform, currently we use the well known > > best format UYVY for overlay, but it is possible to change the render format > > dynamically based on the scenarios in the future. > > Keep in mind too that SoC x86 platforms may use other formats in the future for > overlay. So I tend to agree that hardcoding this format is not a good idea. > > Sean Thanks, Sean. In this CL, we have provided the capacity(API and parameter) to change the overlay format dynamically for future development. But for now, it should be fine to use the default well know format UYVY.
On 2015/10/30 02:38:34, Pawel Osciak wrote: > https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... > File content/common/gpu/media/vaapi_drm_picture.cc (right): > > https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... > content/common/gpu/media/vaapi_drm_picture.cc:22: // Format for storing the > video decoded pictures > Nit: please use dots at the end of sentences. > Also, I don't think this is actually for "storing", we decode into YUV420, but > for usage with GLImages we have to convert to this format. > > https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... > content/common/gpu/media/vaapi_drm_picture.cc:24: // Format for renderring the > video pictures > This is also not precise enough. We don't use this format when not using > overlays. > > https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... > content/common/gpu/media/vaapi_drm_picture.cc:197: scaled_pixmap_ = > CreateNativePixmap(new_size, kPictureRenderFormat); > On 2015/10/29 17:49:40, kalyank wrote: > > Sorry, if I wasn't clear earlier, hardcoding the format here is wrong. In > > fullscreen case, the overall benefits for collapsing planes to one will > outweigh > > using UYVY on platforms where primary cannot support this. I still think that > > there should be some pre-validation (by the caller ?) as which format is > > optimal, if it is supported by the hardware and use it in here. > > +1 to this, this is basically what I was hoping for when asking for a way to > query upper layers for optimal format, both for overlays and non-overlay > scenarios. > > How could we query for the optimal format? HardwarePlaneManager knows about all the formats supported by different planes. The factors which affect the choice: 1)Buffer size of current buffer. 2)Output size. 3)Formats supported by primary and Overlay. We need to take these as inputs and decide which is the optimal format for a given case(Overlay/Non-Overlay, Fullscreen/Non-Fullscreen case). We could do some basic plumbing via DrmWindow to query this information or have something like DrmOverlayCandidateHost in GPU side too.
On 2015/10/30 21:56:27, kalyank wrote: > On 2015/10/30 02:38:34, Pawel Osciak wrote: > > > https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... > > File content/common/gpu/media/vaapi_drm_picture.cc (right): > > > > > https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... > > content/common/gpu/media/vaapi_drm_picture.cc:22: // Format for storing the > > video decoded pictures > > Nit: please use dots at the end of sentences. > > Also, I don't think this is actually for "storing", we decode into YUV420, but > > for usage with GLImages we have to convert to this format. > > > > > https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... > > content/common/gpu/media/vaapi_drm_picture.cc:24: // Format for renderring the > > video pictures > > This is also not precise enough. We don't use this format when not using > > overlays. > > > > > https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... > > content/common/gpu/media/vaapi_drm_picture.cc:197: scaled_pixmap_ = > > CreateNativePixmap(new_size, kPictureRenderFormat); > > On 2015/10/29 17:49:40, kalyank wrote: > > > Sorry, if I wasn't clear earlier, hardcoding the format here is wrong. In > > > fullscreen case, the overall benefits for collapsing planes to one will > > outweigh > > > using UYVY on platforms where primary cannot support this. I still think > that > > > there should be some pre-validation (by the caller ?) as which format is > > > optimal, if it is supported by the hardware and use it in here. > > > > +1 to this, this is basically what I was hoping for when asking for a way to > > query upper layers for optimal format, both for overlays and non-overlay > > scenarios. > > > > How could we query for the optimal format? > > HardwarePlaneManager knows about all the formats supported > by different planes. The factors which affect the choice: > 1)Buffer size of current buffer. > 2)Output size. > 3)Formats supported by primary and Overlay. > > We need to take these as inputs and decide which is the optimal > format for a given case(Overlay/Non-Overlay, Fullscreen/Non-Fullscreen case). > We could do some basic plumbing via DrmWindow to query this information > or have something like DrmOverlayCandidateHost in GPU side too. Thank you so much, Kalyan, I would prefer to create another CL for that as this CL is increasing too much. This CL enables the overlay render format setting path and by default use UYVY. I have changed the title and description.
Description was changed from ========== [Ozone] Optimize video render format from XRGB8888 to UYVY when play on overlay When play on overlay, current video render format is XRGB8888 which bpp is 32 bits, this CL optimizes it to UYVY format which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(width*height), it should be 2(16bits). ========== to ========== [Ozone] Enables the overlay render format setting path and by default use UYVY When play on overlay, current video render format is XRGB8888 which bpp is 32 bits, this CL enables the overlay render format setting path and by default use UYVY which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(width*height), it should be 2(16bits). ==========
Description was changed from ========== [Ozone] Enables the overlay render format setting path and by default use UYVY When play on overlay, current video render format is XRGB8888 which bpp is 32 bits, this CL enables the overlay render format setting path and by default use UYVY which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(width*height), it should be 2(16bits). ========== to ========== [Ozone] Enables the overlay render format setting path and by default use UYVY When play on overlay, current video render format is XRGB8888 which bpp is 32 bits, this CL enables the overlay render format setting path and by default use UYVY which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(width*height), it should be 2(16bits). ==========
Description was changed from ========== [Ozone] Enables the overlay render format setting path and by default use UYVY When play on overlay, current video render format is XRGB8888 which bpp is 32 bits, this CL enables the overlay render format setting path and by default use UYVY which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(width*height), it should be 2(16bits). ========== to ========== [Ozone] Enables the overlay render format setting path and by default use UYVY When play on overlay, current video render format is XRGB8888 which bpp is 32 bits, this CL enables the overlay render format setting path and by default use UYVY which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(width*height), it should be 2(16bits). ==========
Description was changed from ========== [Ozone] Enables the overlay render format setting path and by default use UYVY When play on overlay, current video render format is XRGB8888 which bpp is 32 bits, this CL enables the overlay render format setting path and by default use UYVY which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(width*height), it should be 2(16bits). ========== to ========== [Ozone] Enables the overlay render format setting path and by default use UYVY When play on overlay, current video render format is XRGB8888 which bpp is 32 bits, this CL enables the overlay render format setting path and by default use UYVY which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(width*height), it should be 2(16bits). ==========
Description was changed from ========== [Ozone] Enables the overlay render format setting path and by default use UYVY When play on overlay, current video render format is XRGB8888 which bpp is 32 bits, this CL enables the overlay render format setting path and by default use UYVY which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(width*height), it should be 2(16bits). ========== to ========== [Ozone] Enables the overlay render format setting path and by default use UYVY When play on overlay, current video render format is XRGB8888 which bpp is 32 bits, this CL enables the overlay render format setting path and by default use UYVY which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(width*height), it should be 2(16bits). ==========
Description was changed from ========== [Ozone] Enables the overlay render format setting path and by default use UYVY When play on overlay, current video render format is XRGB8888 which bpp is 32 bits, this CL enables the overlay render format setting path and by default use UYVY which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(width*height), it should be 2(16bits). ========== to ========== [Ozone] Enables overlay render format setting path and by default use UYVY When play on overlay, current video render format is XRGB8888 which bpp is 32 bits, this CL enables the overlay render format setting path and by default use UYVY which bpp is 16 bits.So this optimization saves half display bandwidth and thus save power consumption too. BUG= TEST=Enable Ozone hardware overlay, play h264 video on overlay by waiting for video is single on top, calculate video render framebuffer bpp size from file:/sys/kernel/debug/dri/0/i915_gem_framebuffer by data_size/(width*height), it should be 2(16bits). ==========
On 2015/10/30 22:08:20, william.xie wrote: > On 2015/10/30 21:56:27, kalyank wrote: > > On 2015/10/30 02:38:34, Pawel Osciak wrote: > > > > > > https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... > > > File content/common/gpu/media/vaapi_drm_picture.cc (right): > > > > > > > > > https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... > > > content/common/gpu/media/vaapi_drm_picture.cc:22: // Format for storing the > > > video decoded pictures > > > Nit: please use dots at the end of sentences. > > > Also, I don't think this is actually for "storing", we decode into YUV420, > but > > > for usage with GLImages we have to convert to this format. > > > > > > > > > https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... > > > content/common/gpu/media/vaapi_drm_picture.cc:24: // Format for renderring > the > > > video pictures > > > This is also not precise enough. We don't use this format when not using > > > overlays. > > > > > > > > > https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... > > > content/common/gpu/media/vaapi_drm_picture.cc:197: scaled_pixmap_ = > > > CreateNativePixmap(new_size, kPictureRenderFormat); > > > On 2015/10/29 17:49:40, kalyank wrote: > > > > Sorry, if I wasn't clear earlier, hardcoding the format here is wrong. In > > > > fullscreen case, the overall benefits for collapsing planes to one will > > > outweigh > > > > using UYVY on platforms where primary cannot support this. I still think > > that > > > > there should be some pre-validation (by the caller ?) as which format is > > > > optimal, if it is supported by the hardware and use it in here. > > > > > > +1 to this, this is basically what I was hoping for when asking for a way to > > > query upper layers for optimal format, both for overlays and non-overlay > > > scenarios. > > > > > > How could we query for the optimal format? > > > > HardwarePlaneManager knows about all the formats supported > > by different planes. The factors which affect the choice: > > 1)Buffer size of current buffer. > > 2)Output size. > > 3)Formats supported by primary and Overlay. > > > > We need to take these as inputs and decide which is the optimal > > format for a given case(Overlay/Non-Overlay, Fullscreen/Non-Fullscreen case). > > We could do some basic plumbing via DrmWindow to query this information > > or have something like DrmOverlayCandidateHost in GPU side too. > > Thank you so much, Kalyan, I would prefer to create another CL for that as this > CL is increasing too much. > This CL enables the overlay render format setting path and by default use UYVY. > I have changed the title and description. Dear Pawel, PTAL the new added parameter for rendering format setting. Please be noted that the policy of select the render format dynamically based on the scenarios are under developping, it is not covered by this CL.
https://codereview.chromium.org/1422563002/diff/160001/content/common/gpu/med... File content/common/gpu/media/vaapi_drm_picture.h (right): https://codereview.chromium.org/1422563002/diff/160001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.h:73: // Ozone buffer, the storage of the processed buffer for overlay. I like it as it now. I don't think we need this renaming. @spang ? https://codereview.chromium.org/1422563002/diff/160001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1422563002/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:26: const gfx::BufferFormat kOverlayRenderFormat = gfx::BufferFormat::UYVY_422; nit: Rendering https://codereview.chromium.org/1422563002/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:92: const ProcessingCallback& processing_callback) { I like the way it is currently, any reason for this renaming? https://codereview.chromium.org/1422563002/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:181: // TODO:Figure out the optimal render foramt for overlay Nit: format
On 2015/10/30 22:08:20, william.xie wrote: > On 2015/10/30 21:56:27, kalyank wrote: > > On 2015/10/30 02:38:34, Pawel Osciak wrote: > > > > > > https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... > > > File content/common/gpu/media/vaapi_drm_picture.cc (right): > > > > > > > > > https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... > > > content/common/gpu/media/vaapi_drm_picture.cc:22: // Format for storing the > > > video decoded pictures > > > Nit: please use dots at the end of sentences. > > > Also, I don't think this is actually for "storing", we decode into YUV420, > but > > > for usage with GLImages we have to convert to this format. > > > > > > > > > https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... > > > content/common/gpu/media/vaapi_drm_picture.cc:24: // Format for renderring > the > > > video pictures > > > This is also not precise enough. We don't use this format when not using > > > overlays. > > > > > > > > > https://codereview.chromium.org/1422563002/diff/140001/content/common/gpu/med... > > > content/common/gpu/media/vaapi_drm_picture.cc:197: scaled_pixmap_ = > > > CreateNativePixmap(new_size, kPictureRenderFormat); > > > On 2015/10/29 17:49:40, kalyank wrote: > > > > Sorry, if I wasn't clear earlier, hardcoding the format here is wrong. In > > > > fullscreen case, the overall benefits for collapsing planes to one will > > > outweigh > > > > using UYVY on platforms where primary cannot support this. I still think > > that > > > > there should be some pre-validation (by the caller ?) as which format is > > > > optimal, if it is supported by the hardware and use it in here. > > > > > > +1 to this, this is basically what I was hoping for when asking for a way to > > > query upper layers for optimal format, both for overlays and non-overlay > > > scenarios. > > > > > > How could we query for the optimal format? > > > > HardwarePlaneManager knows about all the formats supported > > by different planes. The factors which affect the choice: > > 1)Buffer size of current buffer. > > 2)Output size. > > 3)Formats supported by primary and Overlay. > > > > We need to take these as inputs and decide which is the optimal > > format for a given case(Overlay/Non-Overlay, Fullscreen/Non-Fullscreen case). > > We could do some basic plumbing via DrmWindow to query this information > > or have something like DrmOverlayCandidateHost in GPU side too. > > Thank you so much, Kalyan, I would prefer to create another CL for that as this > CL is increasing too much. I will take a look into this. > This CL enables the overlay render format setting path and by default use UYVY. > I have changed the title and description. We cannot use UYVY as default currently. It was correct as it was to use this only in Scaled case.
https://codereview.chromium.org/1422563002/diff/160001/content/common/gpu/med... File content/common/gpu/media/vaapi_drm_picture.h (right): https://codereview.chromium.org/1422563002/diff/160001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.h:73: // Ozone buffer, the storage of the processed buffer for overlay. On 2015/10/30 22:19:32, kalyank wrote: > I like it as it now. I don't think we need this renaming. > @spang ? Dear kalyank, the reason we renaming it is: we not only scaling the pixmap, but also converting its format, and in future we may add other processing operations.
> I will take a look into this. > > > This CL enables the overlay render format setting path and by default use > UYVY. > > I have changed the title and description. > > We cannot use UYVY as default currently. It was correct as it was to use this > only in > Scaled case. I mean along with the checks to make sure UYVY is supported by hardware and is the correct choice (Non full screen case). I will look into this and put up a separate CL. The reason we are safe in scaled case is that by this time we know we are going to use Overlay and dont fall back to GPU composition.
On 2015/10/30 22:27:48, kalyank wrote: > > I will take a look into this. > > > > > This CL enables the overlay render format setting path and by default use > > UYVY. > > > I have changed the title and description. > > > > We cannot use UYVY as default currently. It was correct as it was to use this > > only in > > Scaled case. > > I mean along with the checks to make sure UYVY is supported by hardware and is > the correct choice (Non full screen case). I will look into this and put up a > separate CL. The reason we are safe in scaled case is that by this time we > know we are going to use Overlay and dont fall back to GPU composition. Exactly, Processed_pixmap_ only goes to overlay!
> > I mean along with the checks to make sure UYVY is supported by hardware and is > > the correct choice (Non full screen case). I will look into this and put up a > > separate CL. The reason we are safe in scaled case is that by this time we > > know we are going to use Overlay and dont fall back to GPU composition. > > Exactly, Processed_pixmap_ only goes to overlay! Ha k. Sorry missed that
@Pawel, PTAL, thank you!
PTAL https://codereview.chromium.org/1422563002/diff/160001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1422563002/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:26: const gfx::BufferFormat kOverlayRenderFormat = gfx::BufferFormat::UYVY_422; On 2015/10/30 22:19:32, kalyank wrote: > nit: Rendering Done. https://codereview.chromium.org/1422563002/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:92: const ProcessingCallback& processing_callback) { On 2015/10/30 22:19:32, kalyank wrote: > I like the way it is currently, any reason for this renaming? processing=scaling+csc https://codereview.chromium.org/1422563002/diff/160001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:181: // TODO:Figure out the optimal render foramt for overlay On 2015/10/30 22:19:32, kalyank wrote: > Nit: format Done.
https://chromiumcodereview.appspot.com/1422563002/diff/180001/content/common/... File content/common/gpu/media/va_surface.h (right): https://chromiumcodereview.appspot.com/1422563002/diff/180001/content/common/... content/common/gpu/media/va_surface.h:101: const unsigned int GetVAFormat() const { return format_; } The coding style is to call getters the same as the members without the "_" and without capitalization. Thus, this should be called format(), just like size() is used to get size_. https://chromiumcodereview.appspot.com/1422563002/diff/180001/content/common/... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://chromiumcodereview.appspot.com/1422563002/diff/180001/content/common/... content/common/gpu/media/vaapi_drm_picture.cc:22: // Format for storing the video decoded pictures. Please correct this documentation, explaining why this uses BGRX, as per my previous comment on earlier patchset. https://chromiumcodereview.appspot.com/1422563002/diff/180001/content/common/... content/common/gpu/media/vaapi_drm_picture.cc:25: uint32_t BufferFormatToVAFourCC(gfx::BufferFormat fmt) { Please put these functions and the constant in an anonymous namespace. https://chromiumcodereview.appspot.com/1422563002/diff/180001/content/common/... content/common/gpu/media/vaapi_drm_picture.cc:161: if (!image->Initialize(pixmap_.get(), kPictureForGLImageFormat)) { We should perhaps allow querying format from pixmap_ instead? https://chromiumcodereview.appspot.com/1422563002/diff/180001/content/common/... content/common/gpu/media/vaapi_drm_picture.cc:198: VAFourCCToVARTFormat(BufferFormatToVAFourCC(target_format))) { We seem to always need BufferFormat->VAFourCC conversion (unless I'm missing something), so perhaps we should have BufferFormatToVARTFormat instead of BufferFormatToVAFourCC()? https://chromiumcodereview.appspot.com/1422563002/diff/180001/content/common/... File content/common/gpu/media/vaapi_drm_picture.h (right): https://chromiumcodereview.appspot.com/1422563002/diff/180001/content/common/... content/common/gpu/media/vaapi_drm_picture.h:56: // Use VPP to process underlying pixmap_ to |target_size| and |target_format|. Use VPP to process underlying pixmap_, scaling to |target_size| and converting to |target_format| https://chromiumcodereview.appspot.com/1422563002/diff/180001/content/common/... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/1422563002/diff/180001/content/common/... content/common/gpu/media/vaapi_video_encode_accelerator.cc:587: VA_RT_FORMAT_YUV420, va_surface_release_cb_); We shouldn't be hardcoding the format in multiple places like this, as I mentioned previously. It's easy to not change this later, or have some other mismatch somewhere. We'd have to use a separate struct instead of VASurfaceID vector in VaapiWrapper for this though. But since the surface set is tied to current VaapiWrapper state, I'm ok adding a surface_format_ member to VaapiWrapper and an ability to query for it instead. So VaapiWrapper::CreateSurfaces() would create surfaces and store the format of the created surfaces in VaapiWrapper. Then we could query the wrapper for the current format here and in other places. https://chromiumcodereview.appspot.com/1422563002/diff/180001/ui/ozone/platfo... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://chromiumcodereview.appspot.com/1422563002/diff/180001/ui/ozone/platfo... ui/ozone/platform/drm/gpu/gbm_buffer.cc:26: const gfx::BufferFormat kOverlayRenderFormat = gfx::BufferFormat::UYVY_422; Anonymous namespace please. https://chromiumcodereview.appspot.com/1422563002/diff/180001/ui/ozone/platfo... File ui/ozone/platform/drm/gpu/gbm_buffer_base.cc (left): https://chromiumcodereview.appspot.com/1422563002/diff/180001/ui/ozone/platfo... ui/ozone/platform/drm/gpu/gbm_buffer_base.cc:42: DCHECK(fb_pixel_format_ == GBM_FORMAT_XRGB8888); Is it really ok to remove this?
william.xie@intel.com changed reviewers: + ccameron@chromium.org, kbr@chromium.org, piman@chromium.org
Thanks Pawel, PTAL Also @Ken Russell @ccameron @piman for ui/gl and content/common/gpu/ https://codereview.chromium.org/1422563002/diff/180001/content/common/gpu/med... File content/common/gpu/media/va_surface.h (right): https://codereview.chromium.org/1422563002/diff/180001/content/common/gpu/med... content/common/gpu/media/va_surface.h:101: const unsigned int GetVAFormat() const { return format_; } On 2015/11/05 10:23:51, Pawel Osciak wrote: > The coding style is to call getters the same as the members without the "_" and > without capitalization. Thus, this should be called format(), just like size() > is used to get size_. Done. https://codereview.chromium.org/1422563002/diff/180001/content/common/gpu/med... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1422563002/diff/180001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:22: // Format for storing the video decoded pictures. On 2015/11/05 10:23:51, Pawel Osciak wrote: > Please correct this documentation, explaining why this uses BGRX, as per my > previous comment on earlier patchset. Done. https://codereview.chromium.org/1422563002/diff/180001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:25: uint32_t BufferFormatToVAFourCC(gfx::BufferFormat fmt) { On 2015/11/05 10:23:51, Pawel Osciak wrote: > Please put these functions and the constant in an anonymous namespace. Done. https://codereview.chromium.org/1422563002/diff/180001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:161: if (!image->Initialize(pixmap_.get(), kPictureForGLImageFormat)) { On 2015/11/05 10:23:51, Pawel Osciak wrote: > We should perhaps allow querying format from pixmap_ instead? Done. https://codereview.chromium.org/1422563002/diff/180001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:198: VAFourCCToVARTFormat(BufferFormatToVAFourCC(target_format))) { On 2015/11/05 10:23:51, Pawel Osciak wrote: > We seem to always need BufferFormat->VAFourCC conversion (unless I'm missing > something), so perhaps we should have BufferFormatToVARTFormat instead of > BufferFormatToVAFourCC()? Done. https://codereview.chromium.org/1422563002/diff/180001/content/common/gpu/med... File content/common/gpu/media/vaapi_drm_picture.h (right): https://codereview.chromium.org/1422563002/diff/180001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.h:56: // Use VPP to process underlying pixmap_ to |target_size| and |target_format|. On 2015/11/05 10:23:51, Pawel Osciak wrote: > Use VPP to process underlying pixmap_, scaling to |target_size| and converting > to |target_format| Done. https://codereview.chromium.org/1422563002/diff/180001/content/common/gpu/med... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/1422563002/diff/180001/content/common/gpu/med... content/common/gpu/media/vaapi_video_encode_accelerator.cc:587: VA_RT_FORMAT_YUV420, va_surface_release_cb_); On 2015/11/05 10:23:51, Pawel Osciak wrote: > We shouldn't be hardcoding the format in multiple places like this, as I > mentioned previously. It's easy to not change this later, or have some other > mismatch somewhere. > > We'd have to use a separate struct instead of VASurfaceID vector in VaapiWrapper > for this though. But since the surface set is tied to current VaapiWrapper > state, I'm ok adding a surface_format_ member to VaapiWrapper and an ability to > query for it instead. > > So VaapiWrapper::CreateSurfaces() would create surfaces and store the format of > the created surfaces in VaapiWrapper. Then we could query the wrapper for the > current format here and in other places. Done. https://codereview.chromium.org/1422563002/diff/180001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1422563002/diff/180001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:26: const gfx::BufferFormat kOverlayRenderFormat = gfx::BufferFormat::UYVY_422; On 2015/11/05 10:23:51, Pawel Osciak wrote: > Anonymous namespace please. Done. https://codereview.chromium.org/1422563002/diff/180001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer_base.cc (left): https://codereview.chromium.org/1422563002/diff/180001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer_base.cc:42: DCHECK(fb_pixel_format_ == GBM_FORMAT_XRGB8888); On 2015/11/05 10:23:51, Pawel Osciak wrote: > Is it really ok to remove this? Done.
lgtm
https://codereview.chromium.org/1422563002/diff/200001/content/common/gpu/med... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1422563002/diff/200001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:20: // we decode video into YUV420, but for usage with GLImages we have to convert nit: namespace { all the variables and switch statements } https://codereview.chromium.org/1422563002/diff/200001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/drm_device.cc (right): https://codereview.chromium.org/1422563002/diff/200001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/drm_device.cc:313: strides, offsets, framebuffer, 0); we should set whatever is passed in |flags| here and not 0. https://codereview.chromium.org/1422563002/diff/200001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/drm_device.h (right): https://codereview.chromium.org/1422563002/diff/200001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/drm_device.h:80: // Register any format buffer with the CRTC. On successful registration, the Ozone Tests need fixing too i.e. MockDrmDevice etc https://codereview.chromium.org/1422563002/diff/200001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1422563002/diff/200001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:181: // TODO(william.xie): Figure out the optimal render foramt for overlay. nit: format
PTAL https://codereview.chromium.org/1422563002/diff/200001/content/common/gpu/med... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1422563002/diff/200001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:20: // we decode video into YUV420, but for usage with GLImages we have to convert On 2015/11/06 17:30:26, kalyank wrote: > nit: > namespace { > all the variables and switch statements > } Done. https://codereview.chromium.org/1422563002/diff/200001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/drm_device.cc (right): https://codereview.chromium.org/1422563002/diff/200001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/drm_device.cc:313: strides, offsets, framebuffer, 0); On 2015/11/06 17:30:26, kalyank wrote: > we should set whatever is passed in |flags| here and not 0. Done. https://codereview.chromium.org/1422563002/diff/200001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/drm_device.h (right): https://codereview.chromium.org/1422563002/diff/200001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/drm_device.h:80: // Register any format buffer with the CRTC. On successful registration, the On 2015/11/06 17:30:26, kalyank wrote: > Ozone Tests need fixing too i.e. MockDrmDevice etc Thanks for reminder, Kalyan, but how about creating another new CL for that soon after this CL to avoid this CL is getting too too large. Do you agree with this? https://codereview.chromium.org/1422563002/diff/200001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1422563002/diff/200001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:181: // TODO(william.xie): Figure out the optimal render foramt for overlay. On 2015/11/06 17:30:26, kalyank wrote: > nit: format Done.
https://codereview.chromium.org/1422563002/diff/200001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/drm_device.h (right): https://codereview.chromium.org/1422563002/diff/200001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/drm_device.h:80: // Register any format buffer with the CRTC. On successful registration, the On 2015/11/06 18:24:02, william.xie wrote: > On 2015/11/06 17:30:26, kalyank wrote: > > Ozone Tests need fixing too i.e. MockDrmDevice etc > > Thanks for reminder, Kalyan, but how about creating another new CL for that soon > after this CL to avoid this CL is getting too too large. Do you agree with this? Fixing tests must be done here not in a followup. Patches that break tests are reverted.
The CQ bit was checked by spang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422563002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422563002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://chromiumcodereview.appspot.com/1422563002/diff/220001/content/common/... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://chromiumcodereview.appspot.com/1422563002/diff/220001/content/common/... content/common/gpu/media/vaapi_drm_picture.cc:21: // we decode video into YUV420, but for usage with GLImages we have to convert Nit: please start comments with a capital letter. https://chromiumcodereview.appspot.com/1422563002/diff/220001/content/common/... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/1422563002/diff/220001/content/common/... content/common/gpu/media/vaapi_wrapper.cc:518: DCHECK(va_surface_ids_.empty()); DCHECK_EQ(va_surface_format_, 0); https://chromiumcodereview.appspot.com/1422563002/diff/220001/content/common/... content/common/gpu/media/vaapi_wrapper.cc:520: va_surface_format_ = va_format; Please set this at l.546. https://chromiumcodereview.appspot.com/1422563002/diff/220001/content/common/... content/common/gpu/media/vaapi_wrapper.cc:565: va_context_id_ = VA_INVALID_ID; va_surface_format_ = 0; https://chromiumcodereview.appspot.com/1422563002/diff/220001/ui/ozone/platfo... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://chromiumcodereview.appspot.com/1422563002/diff/220001/ui/ozone/platfo... ui/ozone/platform/drm/gpu/gbm_buffer.cc:24: const gfx::BufferFormat kOverlayRenderFormat = gfx::BufferFormat::UYVY_422; Please use an anonymous namespace. https://chromiumcodereview.appspot.com/1422563002/diff/220001/ui/ozone/platfo... ui/ozone/platform/drm/gpu/gbm_buffer.cc:181: // TODO(william.xie): Figure out the optimal render format for overlay. Could you please submit a crbug.com for this and put the bug number here? Thanks.
PTAL https://codereview.chromium.org/1422563002/diff/220001/content/common/gpu/med... File content/common/gpu/media/vaapi_drm_picture.cc (right): https://codereview.chromium.org/1422563002/diff/220001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.cc:21: // we decode video into YUV420, but for usage with GLImages we have to convert On 2015/11/09 03:52:06, Pawel Osciak wrote: > Nit: please start comments with a capital letter. Done. https://codereview.chromium.org/1422563002/diff/220001/content/common/gpu/med... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/1422563002/diff/220001/content/common/gpu/med... content/common/gpu/media/vaapi_wrapper.cc:518: DCHECK(va_surface_ids_.empty()); On 2015/11/09 03:52:06, Pawel Osciak wrote: > DCHECK_EQ(va_surface_format_, 0); Done. https://codereview.chromium.org/1422563002/diff/220001/content/common/gpu/med... content/common/gpu/media/vaapi_wrapper.cc:520: va_surface_format_ = va_format; On 2015/11/09 03:52:06, Pawel Osciak wrote: > Please set this at l.546. Done. https://codereview.chromium.org/1422563002/diff/220001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1422563002/diff/220001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:24: const gfx::BufferFormat kOverlayRenderFormat = gfx::BufferFormat::UYVY_422; On 2015/11/09 03:52:06, Pawel Osciak wrote: > Please use an anonymous namespace. Done.
The CQ bit was checked by william.xie@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422563002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422563002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Thank you. I guess the remaining issues would be to address the compile error on the bot and tests situation.
The CQ bit was checked by william.xie@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422563002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422563002/260001
Patchset #13 (id:260001) has been deleted
The CQ bit was checked by william.xie@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422563002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422563002/280001
Patchset #13 (id:280001) has been deleted
The CQ bit was checked by william.xie@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422563002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422563002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/11/09 08:44:53, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. @Pawel, Dear Pawel, compile error and tests situation are fixed, PTAL
william.xie@intel.com changed reviewers: + reveman@chromium.org
On 2015/11/09 09:04:02, william.xie wrote: > On 2015/11/09 08:44:53, commit-bot: I haz the power wrote: > > Dry run: This issue passed the CQ dry run. > > @Pawel, > Dear Pawel, compile error and tests situation are fixed, PTAL @reveman@ for gpu_memory_buffer_factory_ozone_native_pixmap.cc, PTAL
content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc lgtm
non-owner lgtm Have a WIP CL here for unifying the logic for buffer selection in actual commit and Test commit and also testing the format is actually supported. https://codereview.chromium.org/1426993003/
content/common/gpu/media lgtm, with nits. Please also wait for spang@'s approval. https://chromiumcodereview.appspot.com/1422563002/diff/300001/content/common/... File content/common/gpu/media/va_surface.h (right): https://chromiumcodereview.appspot.com/1422563002/diff/300001/content/common/... content/common/gpu/media/va_surface.h:94: const ReleaseCB& release_cb); Nit: const in format not needed. https://chromiumcodereview.appspot.com/1422563002/diff/300001/content/common/... content/common/gpu/media/va_surface.h:101: const unsigned int format() const { return format_; } Nit: const in return type not needed. https://chromiumcodereview.appspot.com/1422563002/diff/300001/content/common/... File content/common/gpu/media/vaapi_drm_picture.h (right): https://chromiumcodereview.appspot.com/1422563002/diff/300001/content/common/... content/common/gpu/media/vaapi_drm_picture.h:58: scoped_refptr<ui::NativePixmap> ProcessPixmap( Nit: dot at the end of sentences please. https://chromiumcodereview.appspot.com/1422563002/diff/300001/content/common/... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/1422563002/diff/300001/content/common/... content/common/gpu/media/vaapi_wrapper.cc:566: va_context_id_ = VA_INVALID_ID; We are still missing va_surface_format_ = 0; here. https://chromiumcodereview.appspot.com/1422563002/diff/300001/content/common/... File content/common/gpu/media/vaapi_wrapper.h (right): https://chromiumcodereview.appspot.com/1422563002/diff/300001/content/common/... content/common/gpu/media/vaapi_wrapper.h:198: // Get the created surfaces format Nit: dot at end of sentences please. https://chromiumcodereview.appspot.com/1422563002/diff/300001/content/common/... content/common/gpu/media/vaapi_wrapper.h:199: unsigned int va_surface_format() { return va_surface_format_; } unsigned int va_surface_format() const
Thanks Pawel, fixed the nits. @spang, PTAL for the final approval. https://codereview.chromium.org/1422563002/diff/300001/content/common/gpu/med... File content/common/gpu/media/va_surface.h (right): https://codereview.chromium.org/1422563002/diff/300001/content/common/gpu/med... content/common/gpu/media/va_surface.h:94: const ReleaseCB& release_cb); On 2015/11/10 06:09:24, Pawel Osciak wrote: > Nit: const in format not needed. Done. https://codereview.chromium.org/1422563002/diff/300001/content/common/gpu/med... content/common/gpu/media/va_surface.h:101: const unsigned int format() const { return format_; } On 2015/11/10 06:09:24, Pawel Osciak wrote: > Nit: const in return type not needed. Done. https://codereview.chromium.org/1422563002/diff/300001/content/common/gpu/med... File content/common/gpu/media/vaapi_drm_picture.h (right): https://codereview.chromium.org/1422563002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_drm_picture.h:58: scoped_refptr<ui::NativePixmap> ProcessPixmap( On 2015/11/10 06:09:24, Pawel Osciak wrote: > Nit: dot at the end of sentences please. Done. https://codereview.chromium.org/1422563002/diff/300001/content/common/gpu/med... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/1422563002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_wrapper.h:198: // Get the created surfaces format On 2015/11/10 06:09:25, Pawel Osciak wrote: > Nit: dot at end of sentences please. Done. https://codereview.chromium.org/1422563002/diff/300001/content/common/gpu/med... content/common/gpu/media/vaapi_wrapper.h:199: unsigned int va_surface_format() { return va_surface_format_; } On 2015/11/10 06:09:25, Pawel Osciak wrote: > unsigned int va_surface_format() const Done.
lgtm https://codereview.chromium.org/1422563002/diff/320001/ui/ozone/platform/cast... File ui/ozone/platform/cast/surface_factory_cast.cc (right): https://codereview.chromium.org/1422563002/diff/320001/ui/ozone/platform/cast... ui/ozone/platform/cast/surface_factory_cast.cc:194: return gfx::BufferFormat::LAST; LAST doesn't really make sense since it will change as new types are added. Probably just BGRA_8888. https://codereview.chromium.org/1422563002/diff/320001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/drm_util.cc (right): https://codereview.chromium.org/1422563002/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/drm_util.cc:298: return gfx::BufferFormat::LAST; same here, although this is not supposed to be reachable I think it is odd to use LAST.
fixed the nits https://codereview.chromium.org/1422563002/diff/320001/ui/ozone/platform/cast... File ui/ozone/platform/cast/surface_factory_cast.cc (right): https://codereview.chromium.org/1422563002/diff/320001/ui/ozone/platform/cast... ui/ozone/platform/cast/surface_factory_cast.cc:194: return gfx::BufferFormat::LAST; On 2015/11/10 15:46:18, spang wrote: > LAST doesn't really make sense since it will change as new types are added. > Probably just BGRA_8888. Done. https://codereview.chromium.org/1422563002/diff/320001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/drm_util.cc (right): https://codereview.chromium.org/1422563002/diff/320001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/drm_util.cc:298: return gfx::BufferFormat::LAST; On 2015/11/10 15:46:18, spang wrote: > same here, although this is not supposed to be reachable I think it is odd to > use LAST. Done.
The CQ bit was checked by william.xie@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from marcheu@chromium.org, sean.v.kelley@intel.com, piman@chromium.org, reveman@chromium.org, kalyan.kondapally@intel.com, posciak@chromium.org, spang@chromium.org Link to the patchset: https://codereview.chromium.org/1422563002/#ps340001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422563002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422563002/340001
Message was sent while issue was closed.
Committed patchset #15 (id:340001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/3c17cd45fa74186399b453d34fb4a441933e7c2e Cr-Commit-Position: refs/heads/master@{#358836} |
