Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2648633005:
cros: Support YUYV format for GPU memory buffer video frames
|
Created:
3 years, 11 months ago by dshwang Modified:
3 years, 10 months ago CC:
Aaron Boodman, abarth-chromium, cc-bugs_chromium.org, chromium-reviews, creis+watch_chromium.org, danakj+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, jbauman+watch_chromium.org, kalyank, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, ozone-reviews_chromium.org, piman+watch_chromium.org, posciak+watch_chromium.org, qsr+mojo_chromium.org, rjkroege, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncros: Support YUYV format for GPU memory buffer video frames
YUYV is only format supported by all linux stack such as kernel, mesa, minigbm,
and gpu decoder. So ChromeOS IA prefers to use YUYV for video decoding.
This CL makes --enable-gpu-memory-buffer-video-frames choose YUYV if possible.
This CL is based on Johnson Lin's implementaion
https://codereview.chromium.org/2636433003/
BUG=683347
TEST=run relevant unittests
run chrome on http://www.quirksmode.org/html5/tests/video.html with all possible configurations
* decoding configuration
* gpu decoding: N/A
* pure software decoding: --disable-accelerated-video-decode
* GMB video frame: --disable-accelerated-video-decode --enable-native-gpu-memory-buffers --enable-gpu-memory-buffer-video-frames
* overlay configurations
* atomic mode setting: --enable-hardware-overlays --enable-drm-atomic
* legacy mode setting: --enable-hardware-overlays
* no overlay: N/A
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Patch Set 1 #
Total comments: 18
Patch Set 2 : fix nits and bots #Patch Set 3 : Enable YUYV GPU memory buffer video frames #
Messages
Total messages: 49 (33 generated)
Description was changed from ========== Enable YUV video overlay on Skylake ChromeOS. Add YUYV (i.e YUY2) for video decoder output and using those formats for overlay YUYV is only format supported by all linux stack such as kernel, mesa, and minigbm, so ChromeOS IA prefer to use YUYV for video decoding. This CL changes 1. make gpu decoder (only vaapi) decode on YUYV plane 2. make --enable-gpu-memory-buffer-video-frames choose YUYV if possible 3. make Ozone GBM prefer YUYV rather than UYUV About 1, check the change of media/gpu/vaapi_video_decode_accelerator.cc About 2, check the change of content/renderer/media/renderer_gpu_video_accelerator_factories.cc and gpu/command_buffer/service/feature_info.cc About 3, check the change of ui/ozone/platform/drm/gpu/drm_overlay_validator.cc This CL is based on Johnson Lin's implementaion https://codereview.chromium.org/2636433003/ Test results of power consumption: 8.8% power save. Check more detail in crbug.com/683347 BUG=683347 TEST=run relevant unittests run chrome on http://www.quirksmode.org/html5/tests/video.html with all possible configurations * decoding configuration * gpu decoding: N/A * pure software decoding: --disable-accelerated-video-decode * GMB video frame: --disable-accelerated-video-decode --enable-native-gpu-memory-buffers --enable-gpu-memory-buffer-video-frames * overlay configurations * atomic mode setting: --enable-hardware-overlays --enable-drm-atomic * legacy mode setting: --enable-hardware-overlays * no overlay: N/A gpu: merge gpu/ipc/host/gpu_memory_buffer_support.cc to gpu/ipc/common/ Renderer needs to check if native GBM is supported. So this CL changes * notify kEnableNativeGpuMemoryBuffer command line to renderer * merge gpu/ipc/host/gpu_memory_buffer_support.cc to gpu/ipc/common/gpu_memory_buffer_support.cc ChromeOS will decode YUV video on YUYV buffer only if native GBM is supported. It's because there is not regular texture fallback in OpenGL ES, while EGL EXT_image_dma_buf_import extensions support YUYV EGL image. It's why Renderer needs to know. Actually, current code violates dependency policy, because child process code uses gpu/ipc/host/ e.g. services/ui/common/server_gpu_memory_buffer_manager.h content/renderer/render_thread_impl_browsertest.cc This change fixes these violation. BUG=683347 ========== to ========== Enable YUV video overlay on Skylake ChromeOS. Add YUYV (i.e YUY2) for video decoder output and using those formats for overlay YUYV is only format supported by all linux stack such as kernel, mesa, and minigbm, so ChromeOS IA prefer to use YUYV for video decoding. This CL changes 1. make gpu decoder (only vaapi) decode on YUYV plane 2. make --enable-gpu-memory-buffer-video-frames choose YUYV if possible 3. make Ozone GBM prefer YUYV rather than UYUV About 1, check the change of media/gpu/vaapi_video_decode_accelerator.cc About 2, check the change of content/renderer/media/renderer_gpu_video_accelerator_factories.cc and gpu/command_buffer/service/feature_info.cc About 3, check the change of ui/ozone/platform/drm/gpu/drm_overlay_validator.cc This CL is based on Johnson Lin's implementaion https://codereview.chromium.org/2636433003/ Test results of power consumption: 8.8% power save. Check more detail in crbug.com/683347 BUG=683347 TEST=run relevant unittests run chrome on http://www.quirksmode.org/html5/tests/video.html with all possible configurations * decoding configuration * gpu decoding: N/A * pure software decoding: --disable-accelerated-video-decode * GMB video frame: --disable-accelerated-video-decode --enable-native-gpu-memory-buffers --enable-gpu-memory-buffer-video-frames * overlay configurations * atomic mode setting: --enable-hardware-overlays --enable-drm-atomic * legacy mode setting: --enable-hardware-overlays * no overlay: N/A gpu: merge gpu/ipc/host/gpu_memory_buffer_support.cc to gpu/ipc/common/ Renderer needs to check if native GBM is supported. So this CL changes * notify kEnableNativeGpuMemoryBuffer command line to renderer * merge gpu/ipc/host/gpu_memory_buffer_support.cc to gpu/ipc/common/gpu_memory_buffer_support.cc ChromeOS will decode YUV video on YUYV buffer only if native GBM is supported. It's because there is not regular texture fallback in OpenGL ES, while EGL EXT_image_dma_buf_import extensions support YUYV EGL image. It's why Renderer needs to know. Actually, current code violates dependency policy, because child process code uses gpu/ipc/host/ e.g. services/ui/common/server_gpu_memory_buffer_manager.h content/renderer/render_thread_impl_browsertest.cc This change fixes these violation. BUG=683347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Enable YUV video overlay on Skylake ChromeOS. Add YUYV (i.e YUY2) for video decoder output and using those formats for overlay YUYV is only format supported by all linux stack such as kernel, mesa, and minigbm, so ChromeOS IA prefer to use YUYV for video decoding. This CL changes 1. make gpu decoder (only vaapi) decode on YUYV plane 2. make --enable-gpu-memory-buffer-video-frames choose YUYV if possible 3. make Ozone GBM prefer YUYV rather than UYUV About 1, check the change of media/gpu/vaapi_video_decode_accelerator.cc About 2, check the change of content/renderer/media/renderer_gpu_video_accelerator_factories.cc and gpu/command_buffer/service/feature_info.cc About 3, check the change of ui/ozone/platform/drm/gpu/drm_overlay_validator.cc This CL is based on Johnson Lin's implementaion https://codereview.chromium.org/2636433003/ Test results of power consumption: 8.8% power save. Check more detail in crbug.com/683347 BUG=683347 TEST=run relevant unittests run chrome on http://www.quirksmode.org/html5/tests/video.html with all possible configurations * decoding configuration * gpu decoding: N/A * pure software decoding: --disable-accelerated-video-decode * GMB video frame: --disable-accelerated-video-decode --enable-native-gpu-memory-buffers --enable-gpu-memory-buffer-video-frames * overlay configurations * atomic mode setting: --enable-hardware-overlays --enable-drm-atomic * legacy mode setting: --enable-hardware-overlays * no overlay: N/A gpu: merge gpu/ipc/host/gpu_memory_buffer_support.cc to gpu/ipc/common/ Renderer needs to check if native GBM is supported. So this CL changes * notify kEnableNativeGpuMemoryBuffer command line to renderer * merge gpu/ipc/host/gpu_memory_buffer_support.cc to gpu/ipc/common/gpu_memory_buffer_support.cc ChromeOS will decode YUV video on YUYV buffer only if native GBM is supported. It's because there is not regular texture fallback in OpenGL ES, while EGL EXT_image_dma_buf_import extensions support YUYV EGL image. It's why Renderer needs to know. Actually, current code violates dependency policy, because child process code uses gpu/ipc/host/ e.g. services/ui/common/server_gpu_memory_buffer_manager.h content/renderer/render_thread_impl_browsertest.cc This change fixes these violation. BUG=683347 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Enable YUV video overlay on Skylake ChromeOS. Add YUYV (i.e YUY2) for video decoder output and using those formats for overlay YUYV is only format supported by all linux stack such as kernel, mesa, and minigbm, so ChromeOS IA prefer to use YUYV for video decoding. This CL changes 1. make gpu decoder (only vaapi) decode on YUYV plane 2. make --enable-gpu-memory-buffer-video-frames choose YUYV if possible 3. make Ozone GBM prefer YUYV rather than UYUV About 1, check the change of media/gpu/vaapi_video_decode_accelerator.cc About 2, check the change of content/renderer/media/renderer_gpu_video_accelerator_factories.cc and gpu/command_buffer/service/feature_info.cc About 3, check the change of ui/ozone/platform/drm/gpu/drm_overlay_validator.cc This CL is based on Johnson Lin's implementaion https://codereview.chromium.org/2636433003/ Test results of power consumption: 8.8% power save. Check more detail in crbug.com/683347 BUG=683347 TEST=run relevant unittests run chrome on http://www.quirksmode.org/html5/tests/video.html with all possible configurations * decoding configuration * gpu decoding: N/A * pure software decoding: --disable-accelerated-video-decode * GMB video frame: --disable-accelerated-video-decode --enable-native-gpu-memory-buffers --enable-gpu-memory-buffer-video-frames * overlay configurations * atomic mode setting: --enable-hardware-overlays --enable-drm-atomic * legacy mode setting: --enable-hardware-overlays * no overlay: N/A gpu: merge gpu/ipc/host/gpu_memory_buffer_support.cc to gpu/ipc/common/ Renderer needs to check if native GBM is supported. So this CL changes * notify kEnableNativeGpuMemoryBuffer command line to renderer * merge gpu/ipc/host/gpu_memory_buffer_support.cc to gpu/ipc/common/gpu_memory_buffer_support.cc ChromeOS will decode YUV video on YUYV buffer only if native GBM is supported. It's because there is not regular texture fallback in OpenGL ES, while EGL EXT_image_dma_buf_import extensions support YUYV EGL image. It's why Renderer needs to know. Actually, current code violates dependency policy, because child process code uses gpu/ipc/host/ e.g. services/ui/common/server_gpu_memory_buffer_manager.h content/renderer/render_thread_impl_browsertest.cc This change fixes these violation. BUG=683347 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Enable YUV video overlay on Skylake ChromeOS. Add YUYV (i.e YUY2) for video decoder output and using those formats for overlay YUYV is only format supported by all linux stack such as kernel, mesa, and minigbm, so ChromeOS IA prefer to use YUYV for video decoding. This CL changes 1. make gpu decoder (only vaapi) decode on YUYV plane 2. make --enable-gpu-memory-buffer-video-frames choose YUYV if possible 3. make Ozone GBM prefer YUYV rather than UYUV About 1, check the change of media/gpu/vaapi_video_decode_accelerator.cc About 2, check the change of content/renderer/media/renderer_gpu_video_accelerator_factories.cc and gpu/command_buffer/service/feature_info.cc About 3, check the change of ui/ozone/platform/drm/gpu/drm_overlay_validator.cc This CL is based on Johnson Lin's implementaion https://codereview.chromium.org/2636433003/ Test results of power consumption: 8.8% power save. Check more detail in crbug.com/683347 BUG=683347 TEST=run relevant unittests run chrome on http://www.quirksmode.org/html5/tests/video.html with all possible configurations * decoding configuration * gpu decoding: N/A * pure software decoding: --disable-accelerated-video-decode * GMB video frame: --disable-accelerated-video-decode --enable-native-gpu-memory-buffers --enable-gpu-memory-buffer-video-frames * overlay configurations * atomic mode setting: --enable-hardware-overlays --enable-drm-atomic * legacy mode setting: --enable-hardware-overlays * no overlay: N/A gpu: merge gpu/ipc/host/gpu_memory_buffer_support.cc to gpu/ipc/common/ Renderer needs to check if native GBM is supported. So this CL changes * notify kEnableNativeGpuMemoryBuffer command line to renderer * merge gpu/ipc/host/gpu_memory_buffer_support.cc to gpu/ipc/common/gpu_memory_buffer_support.cc ChromeOS will decode YUV video on YUYV buffer only if native GBM is supported. It's because there is not regular texture fallback in OpenGL ES, while EGL EXT_image_dma_buf_import extensions support YUYV EGL image. It's why Renderer needs to know. Actually, current code violates dependency policy, because child process code uses gpu/ipc/host/ e.g. services/ui/common/server_gpu_memory_buffer_manager.h content/renderer/render_thread_impl_browsertest.cc This change fixes these violation. BUG=683347 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Enable YUV video overlay on Skylake ChromeOS. Add YUYV (i.e YUY2) for video decoder output and using those formats for overlay YUYV is only format supported by all linux stack such as kernel, mesa, and minigbm, so ChromeOS IA prefer to use YUYV for video decoding. This CL changes 1. make gpu decoder (only vaapi) decode on YUYV plane 2. make --enable-gpu-memory-buffer-video-frames choose YUYV if possible 3. make Ozone GBM prefer YUYV rather than UYUV About 1, check the change of media/gpu/vaapi_video_decode_accelerator.cc About 2, check the change of content/renderer/media/renderer_gpu_video_accelerator_factories.cc and gpu/command_buffer/service/feature_info.cc About 3, check the change of ui/ozone/platform/drm/gpu/drm_overlay_validator.cc This CL is based on Johnson Lin's implementaion https://codereview.chromium.org/2636433003/ Test results of power consumption: 8.8% power save. Check more detail in crbug.com/683347 BUG=683347 TEST=run relevant unittests run chrome on http://www.quirksmode.org/html5/tests/video.html with all possible configurations * decoding configuration * gpu decoding: N/A * pure software decoding: --disable-accelerated-video-decode * GMB video frame: --disable-accelerated-video-decode --enable-native-gpu-memory-buffers --enable-gpu-memory-buffer-video-frames * overlay configurations * atomic mode setting: --enable-hardware-overlays --enable-drm-atomic * legacy mode setting: --enable-hardware-overlays * no overlay: N/A gpu: merge gpu/ipc/host/gpu_memory_buffer_support.cc to gpu/ipc/common/ Renderer needs to check if native GBM is supported. So this CL changes * notify kEnableNativeGpuMemoryBuffer command line to renderer * merge gpu/ipc/host/gpu_memory_buffer_support.cc to gpu/ipc/common/gpu_memory_buffer_support.cc ChromeOS will decode YUV video on YUYV buffer only if native GBM is supported. It's because there is not regular texture fallback in OpenGL ES, while EGL EXT_image_dma_buf_import extensions support YUYV EGL image. It's why Renderer needs to know. Actually, current code violates dependency policy, because child process code uses gpu/ipc/host/ e.g. services/ui/common/server_gpu_memory_buffer_manager.h content/renderer/render_thread_impl_browsertest.cc This change fixes these violation. BUG=683347 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
dongseong.hwang@intel.com changed reviewers: + dcastagna@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
Hi Daniele, could you review overall of this CL? This CL make ChromeOS decoder use YUYV like you support UYVY for Mac before. https://codereview.chromium.org/1305153005 https://codereview.chromium.org/1419733005 In my opinion, a bit controversial part is this CL reuse GL_CHROMIUM_ycbcr_422_image extensions for YUYV on ChromeOS, while Mac use it for UYVY. If you want, I can introduce another extension. This CL is be splited like https://codereview.chromium.org/2649553003/ already extract gpu_memory_buffer_support.cc change.
Description was changed from ========== Enable YUV video overlay on Skylake ChromeOS. Add YUYV (i.e YUY2) for video decoder output and using those formats for overlay YUYV is only format supported by all linux stack such as kernel, mesa, and minigbm, so ChromeOS IA prefer to use YUYV for video decoding. This CL changes 1. make gpu decoder (only vaapi) decode on YUYV plane 2. make --enable-gpu-memory-buffer-video-frames choose YUYV if possible 3. make Ozone GBM prefer YUYV rather than UYUV About 1, check the change of media/gpu/vaapi_video_decode_accelerator.cc About 2, check the change of content/renderer/media/renderer_gpu_video_accelerator_factories.cc and gpu/command_buffer/service/feature_info.cc About 3, check the change of ui/ozone/platform/drm/gpu/drm_overlay_validator.cc This CL is based on Johnson Lin's implementaion https://codereview.chromium.org/2636433003/ Test results of power consumption: 8.8% power save. Check more detail in crbug.com/683347 BUG=683347 TEST=run relevant unittests run chrome on http://www.quirksmode.org/html5/tests/video.html with all possible configurations * decoding configuration * gpu decoding: N/A * pure software decoding: --disable-accelerated-video-decode * GMB video frame: --disable-accelerated-video-decode --enable-native-gpu-memory-buffers --enable-gpu-memory-buffer-video-frames * overlay configurations * atomic mode setting: --enable-hardware-overlays --enable-drm-atomic * legacy mode setting: --enable-hardware-overlays * no overlay: N/A gpu: merge gpu/ipc/host/gpu_memory_buffer_support.cc to gpu/ipc/common/ Renderer needs to check if native GBM is supported. So this CL changes * notify kEnableNativeGpuMemoryBuffer command line to renderer * merge gpu/ipc/host/gpu_memory_buffer_support.cc to gpu/ipc/common/gpu_memory_buffer_support.cc ChromeOS will decode YUV video on YUYV buffer only if native GBM is supported. It's because there is not regular texture fallback in OpenGL ES, while EGL EXT_image_dma_buf_import extensions support YUYV EGL image. It's why Renderer needs to know. Actually, current code violates dependency policy, because child process code uses gpu/ipc/host/ e.g. services/ui/common/server_gpu_memory_buffer_manager.h content/renderer/render_thread_impl_browsertest.cc This change fixes these violation. BUG=683347 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Enable YUV video overlay on Skylake ChromeOS. Add YUYV (i.e YUY2) for video decoder output and using those formats for overlay YUYV is only format supported by all linux stack such as kernel, mesa, and minigbm, so ChromeOS IA prefer to use YUYV for video decoding. This CL changes 1. make gpu decoder (only vaapi) decode on YUYV plane 2. make --enable-gpu-memory-buffer-video-frames choose YUYV if possible 3. make Ozone GBM prefer YUYV rather than UYUV About 1, check the change of media/gpu/vaapi_video_decode_accelerator.cc About 2, check the change of content/renderer/media/renderer_gpu_video_accelerator_factories.cc and gpu/command_buffer/service/feature_info.cc About 3, check the change of ui/ozone/platform/drm/gpu/drm_overlay_validator.cc This CL is based on Johnson Lin's implementaion https://codereview.chromium.org/2636433003/ Test results of power consumption: 8.8% power save. Check more detail in crbug.com/683347 BUG=683347 TEST=run relevant unittests run chrome on http://www.quirksmode.org/html5/tests/video.html with all possible configurations * decoding configuration * gpu decoding: N/A * pure software decoding: --disable-accelerated-video-decode * GMB video frame: --disable-accelerated-video-decode --enable-native-gpu-memory-buffers --enable-gpu-memory-buffer-video-frames * overlay configurations * atomic mode setting: --enable-hardware-overlays --enable-drm-atomic * legacy mode setting: --enable-hardware-overlays * no overlay: N/A CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Haven't look at the code, but it's great you're working on this. 8.8% power save compared to what? A big part of the power consumption is the screen, so turning the light up or down would completely change that percentage. Can you tell me how many W are saved and what's the stand by power consumption? Which device did you use to test this with? Did you just try --enable-drm-atomic and it just worked? Btw, I don't think we're planning to ship overlays with legacy pageflip.
marcheu@google.com changed reviewers: + marcheu@google.com
https://codereview.chromium.org/2648633005/diff/1/content/renderer/media/rend... File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right): https://codereview.chromium.org/2648633005/diff/1/content/renderer/media/rend... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:281: // Mesa EGL supports not UYVY but YUYV image. pleaes don't add platform-specific code, even comments like this. If a platform does or doesn't support a feature, it should be detected at runtime. In this case it seems you only need to remove that comment. https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/common/g... File gpu/command_buffer/common/gpu_memory_buffer_support.cc (right): https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/common/g... gpu/command_buffer/common/gpu_memory_buffer_support.cc:48: // Mesa EGL supports not UYVY but YUYV image. ditto https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/feature_info.cc (right): https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/feature_info.cc:1037: // TODO(dshwang): when ARM support YUYV, remove ARCH_CPU_X86_FAMILY check. Som ARM platforms already support YUYV, please don't add the X86 check it isn't needed. https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/feature_info.cc:1040: gl::GLSurfaceEGL::HasEGLExtension("EGL_EXT_image_dma_buf_import"); we might not need to test this, dma_buf_import is a hard requirement for Chrome OS https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/feature_info.cc:1042: AddExtensionString("GL_CHROMIUM_ycbcr_422_image"); I think it's incorrect to prevent allocation of images based on a GL extension? gbm already provides the ability to tell if allocation is possible. The ability to import into GL should be a separate thing. https://codereview.chromium.org/2648633005/diff/1/gpu/ipc/common/gpu_memory_b... File gpu/ipc/common/gpu_memory_buffer_support.cc (right): https://codereview.chromium.org/2648633005/diff/1/gpu/ipc/common/gpu_memory_b... gpu/ipc/common/gpu_memory_buffer_support.cc:21: // Disable native buffers when using Mesa. this comment is misleading, this is osmesa not mesa https://codereview.chromium.org/2648633005/diff/1/gpu/ipc/common/gpu_memory_b... gpu/ipc/common/gpu_memory_buffer_support.cc:49: usage == gfx::BufferUsage::SCANOUT || usage == gfx::BufferUsage::GPU_READ; what does scanout have to do with cpu read? Don't we have a flag for CPU read? https://codereview.chromium.org/2648633005/diff/1/gpu/ipc/common/gpu_memory_b... gpu/ipc/common/gpu_memory_buffer_support.cc:109: // Disable native buffers only when using Mesa. again this is osmesa https://codereview.chromium.org/2648633005/diff/1/gpu/ipc/common/gpu_memory_b... gpu/ipc/common/gpu_memory_buffer_support.cc:118: gfx::BufferFormat::YVU_420, gfx::BufferFormat::YUV_420_BIPLANAR}; why are you hardcoding this table here?
On 2017/01/21 00:24:16, Daniele Castagna wrote: > Haven't look at the code, but it's great you're working on this. Thank you for kind words :) > 8.8% power save compared to what? A big part of the power consumption is the > screen, so turning the light up or down would completely change that percentage. > Can you tell me how many W are saved and what's the stand by power consumption? The test detail is in the description in crbug.com/683347 I compare as-is and total combination (atomic page flip + YUYV decoding + overlay) Here's actual W * BGRA: 6.6462 W * BGRA with overlay: 6.1734 W * YUYV: 6.1729 W * YUYV with overlay: 6.0593 W > Which device did you use to test this with? ACER Chromebook 14, which is Skylake Celeron. > Did you just try --enable-drm-atomic and it just worked? No, I build ChromeOS image with upstream kernel (4.10.0-rc2-11547-gce85c6d) Note upstream kernel has one issue. here's workaround. https://bugs.chromium.org/p/chromium/issues/detail?id=680333 > Btw, I don't think we're planning to ship overlays with legacy pageflip. I think so :) We want to ship overlays with atomic pageflip. The first target might be Kabylake, as it requires at least kernel v4.4. BTW, as we can see power measurement, BGRA to YUYV change for decoder saves a lot of power. Most code of this CL is about YUYV decoding, rather than overlay. I'll extract overlay related code from this CL.
On 2017/01/21 at 01:01:16, dongseong.hwang wrote: > On 2017/01/21 00:24:16, Daniele Castagna wrote: > > Haven't look at the code, but it's great you're working on this. > > Thank you for kind words :) > > > 8.8% power save compared to what? A big part of the power consumption is the > > screen, so turning the light up or down would completely change that percentage. > > Can you tell me how many W are saved and what's the stand by power consumption? > > The test detail is in the description in crbug.com/683347 > I saw it after writing the comment, I think we interleaved our messages. I did the same with the screen (dimming it but not turining it off) and I got similar numbers. > I compare as-is and total combination (atomic page flip + YUYV decoding + overlay) > Here's actual W > * BGRA: 6.6462 W > * BGRA with overlay: 6.1734 W > * YUYV: 6.1729 W > * YUYV with overlay: 6.0593 W > > > Which device did you use to test this with? > > ACER Chromebook 14, which is Skylake Celeron. > Got it. > > Did you just try --enable-drm-atomic and it just worked? > > No, I build ChromeOS image with upstream kernel (4.10.0-rc2-11547-gce85c6d) > Note upstream kernel has one issue. here's workaround. https://bugs.chromium.org/p/chromium/issues/detail?id=680333 > Do you have any idea how much work it'd be to merge all the atomic patches and the work-around to v4.4? We'll follow up via email next week on this. > > Btw, I don't think we're planning to ship overlays with legacy pageflip. > > I think so :) We want to ship overlays with atomic pageflip. The first target might be Kabylake, as it requires at least kernel v4.4. > BTW, as we can see power measurement, BGRA to YUYV change for decoder saves a lot of power. Most code of this CL is about YUYV decoding, rather than overlay. I'll extract overlay related code from this CL. Yes please, split this CL in many CLs, it'll be much easier to review them and to get them in. Thank you and have a good weekend!
The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/21 00:25:18, marcheu1 wrote: > https://codereview.chromium.org/2648633005/diff/1/content/renderer/media/rend... Thanks for reviewing. I fixed nits and explain hardcode in gpu_memory_buffer_support.cc On 2017/01/21 01:21:32, Daniele Castagna wrote: > > > Did you just try --enable-drm-atomic and it just worked? > > > > No, I build ChromeOS image with upstream kernel (4.10.0-rc2-11547-gce85c6d) > > Note upstream kernel has one issue. here's workaround. > https://bugs.chromium.org/p/chromium/issues/detail?id=680333 > > > Do you have any idea how much work it'd be to merge all the atomic patches and > the work-around to v4.4? > We'll follow up via email next week on this. I asked my coworker. When I know, answer in the issue and notify you :) > > > Btw, I don't think we're planning to ship overlays with legacy pageflip. > > > > I think so :) We want to ship overlays with atomic pageflip. The first target > might be Kabylake, as it requires at least kernel v4.4. > > BTW, as we can see power measurement, BGRA to YUYV change for decoder saves a > lot of power. Most code of this CL is about YUYV decoding, rather than overlay. > I'll extract overlay related code from this CL. > > Yes please, split this CL in many CLs, it'll be much easier to review them and > to get them in. Yes, after overall review here, I'll split. > Thank you and have a good weekend! Thank you! you too :) https://codereview.chromium.org/2648633005/diff/1/content/renderer/media/rend... File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right): https://codereview.chromium.org/2648633005/diff/1/content/renderer/media/rend... content/renderer/media/renderer_gpu_video_accelerator_factories.cc:281: // Mesa EGL supports not UYVY but YUYV image. On 2017/01/21 00:25:18, marcheu1 wrote: > pleaes don't add platform-specific code, even comments like this. If a platform > does or doesn't support a feature, it should be detected at runtime. In this > case it seems you only need to remove that comment. Done. https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/common/g... File gpu/command_buffer/common/gpu_memory_buffer_support.cc (right): https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/common/g... gpu/command_buffer/common/gpu_memory_buffer_support.cc:48: // Mesa EGL supports not UYVY but YUYV image. On 2017/01/21 00:25:18, marcheu1 wrote: > ditto Done. https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/feature_info.cc (right): https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/feature_info.cc:1037: // TODO(dshwang): when ARM support YUYV, remove ARCH_CPU_X86_FAMILY check. On 2017/01/21 00:25:18, marcheu1 wrote: > Som ARM platforms already support YUYV, please don't add the X86 check it isn't > needed. Ok, remove the X86 check. and the code is protected by native GBM flags, so it's safe to allow ARM anyway. https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/feature_info.cc:1040: gl::GLSurfaceEGL::HasEGLExtension("EGL_EXT_image_dma_buf_import"); On 2017/01/21 00:25:18, marcheu1 wrote: > we might not need to test this, dma_buf_import is a hard requirement for Chrome > OS In ChromeOS, GLImageOzoneNativePixmap binds YUYV dmabuf to EGLImage using dma_buf_import extensions. SO chromium_image_ycbcr_422 in ChromeOS needs the extensions. https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/feature_info.cc:1042: AddExtensionString("GL_CHROMIUM_ycbcr_422_image"); On 2017/01/21 00:25:18, marcheu1 wrote: > I think it's incorrect to prevent allocation of images based on a GL extension? > gbm already provides the ability to tell if allocation is possible. The ability > to import into GL should be a separate thing. gbm already provides the ability to tell if allocation is possible, but gbm doesn't provide the information if the buffer can be bound to EGLImage. There is not EGL query API for this. We don't know before actuall glEGLImageTargetTexture2DOES() call, in which GLImageEGL::BindTexImage(). If it isn't supported, EGL emits egl error. chromium_image_ycbcr_422 in ChromeOS is EGL Image backed by YUYV dma buf. https://codereview.chromium.org/2648633005/diff/1/gpu/ipc/common/gpu_memory_b... File gpu/ipc/common/gpu_memory_buffer_support.cc (right): https://codereview.chromium.org/2648633005/diff/1/gpu/ipc/common/gpu_memory_b... gpu/ipc/common/gpu_memory_buffer_support.cc:21: // Disable native buffers when using Mesa. On 2017/01/21 00:25:18, marcheu1 wrote: > this comment is misleading, this is osmesa not mesa Thx for noting. Done. This part is from existing gpu/ipc/host/gpu_memory_buffer_support.cc tho. https://codereview.chromium.org/2648633005/diff/1/gpu/ipc/common/gpu_memory_b... gpu/ipc/common/gpu_memory_buffer_support.cc:49: usage == gfx::BufferUsage::SCANOUT || usage == gfx::BufferUsage::GPU_READ; On 2017/01/21 00:25:18, marcheu1 wrote: > what does scanout have to do with cpu read? must use gfx::BufferUsage::GPU_READ_CPU_READ_WRITE or gfx::BufferUsage::GPU_READ_CPU_READ_WRITE_PERSISTENT > Don't we have a flag for CPU read? This confusing is from --enable-native-gpu-buffer-memory flags Even though native GBM is disabled, scanout native GBM is used. e.g. output_surface, video, webgl. --enable-native-gpu-buffer-memory may should be changed to --enable-native-gpu-buffer-memory-"map" see GetNativeGpuMemoryBufferConfigurations() calling this function, even if native GBM is disabled. It needs refactorying. I'll do it in next CL, as this CL is already big. https://codereview.chromium.org/2648633005/diff/1/gpu/ipc/common/gpu_memory_b... gpu/ipc/common/gpu_memory_buffer_support.cc:109: // Disable native buffers only when using Mesa. On 2017/01/21 00:25:18, marcheu1 wrote: > again this is osmesa Done. https://codereview.chromium.org/2648633005/diff/1/gpu/ipc/common/gpu_memory_b... gpu/ipc/common/gpu_memory_buffer_support.cc:118: gfx::BufferFormat::YVU_420, gfx::BufferFormat::YUV_420_BIPLANAR}; On 2017/01/21 00:25:18, marcheu1 wrote: > why are you hardcoding this table here? I just merge existing gpu/ipc/host/gpu_memory_buffer_support.cc to here. Short answer is because the functions were seperated in host/ and common/, but now we can hardcode only one time. I'll refactor this in next CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run
Patchset #2 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run
Patchset #2 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/23 at 23:13:43, dongseong.hwang wrote: > On 2017/01/21 00:25:18, marcheu1 wrote: > > https://codereview.chromium.org/2648633005/diff/1/content/renderer/media/rend... > > Thanks for reviewing. I fixed nits and explain hardcode in gpu_memory_buffer_support.cc > > On 2017/01/21 01:21:32, Daniele Castagna wrote: > > > > Did you just try --enable-drm-atomic and it just worked? > > > > > > No, I build ChromeOS image with upstream kernel (4.10.0-rc2-11547-gce85c6d) > > > Note upstream kernel has one issue. here's workaround. > > https://bugs.chromium.org/p/chromium/issues/detail?id=680333 > > > > > Do you have any idea how much work it'd be to merge all the atomic patches and > > the work-around to v4.4? > > We'll follow up via email next week on this. > > I asked my coworker. When I know, answer in the issue and notify you :) > > > > > Btw, I don't think we're planning to ship overlays with legacy pageflip. > > > > > > I think so :) We want to ship overlays with atomic pageflip. The first target > > might be Kabylake, as it requires at least kernel v4.4. I was planning to start testing atomic pageflips on Apollo Lake in a few weeks. It will take a while to get overlay support though. > > > BTW, as we can see power measurement, BGRA to YUYV change for decoder saves a > > lot of power. Most code of this CL is about YUYV decoding, rather than overlay. > > I'll extract overlay related code from this CL. > > Are you suggesting that with vs without overlay is saving only ~1% of power? That's surprisingly low compared to what we noticed on other devices. I also don't understand how overlays can work with YUV when OverlayCandidate::format is hard coded to RGB at the moment and I don't see you changing it in any CL. Are you using the GbmBuffer processing callback before scanning out to convert the buffer? > > Yes please, split this CL in many CLs, it'll be much easier to review them and > > to get them in. > > Yes, after overall review here, I'll split. > Splitting the CL is mainly to help reviewers, so they can review smaller CLs more quickly and thoroughly. There are also less chances of CL rejection. It'd be great if you could do it before sending CLs out for review. > > Thank you and have a good weekend! > > Thank you! you too :) > > https://codereview.chromium.org/2648633005/diff/1/content/renderer/media/rend... > File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right): > > https://codereview.chromium.org/2648633005/diff/1/content/renderer/media/rend... > content/renderer/media/renderer_gpu_video_accelerator_factories.cc:281: // Mesa EGL supports not UYVY but YUYV image. > On 2017/01/21 00:25:18, marcheu1 wrote: > > pleaes don't add platform-specific code, even comments like this. If a platform > > does or doesn't support a feature, it should be detected at runtime. In this > > case it seems you only need to remove that comment. > > Done. > > https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/common/g... > File gpu/command_buffer/common/gpu_memory_buffer_support.cc (right): > > https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/common/g... > gpu/command_buffer/common/gpu_memory_buffer_support.cc:48: // Mesa EGL supports not UYVY but YUYV image. > On 2017/01/21 00:25:18, marcheu1 wrote: > > ditto > > Done. > > https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/service/... > File gpu/command_buffer/service/feature_info.cc (right): > > https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/feature_info.cc:1037: // TODO(dshwang): when ARM support YUYV, remove ARCH_CPU_X86_FAMILY check. > On 2017/01/21 00:25:18, marcheu1 wrote: > > Som ARM platforms already support YUYV, please don't add the X86 check it isn't > > needed. > > Ok, remove the X86 check. and the code is protected by native GBM flags, so it's safe to allow ARM anyway. > > https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/feature_info.cc:1040: gl::GLSurfaceEGL::HasEGLExtension("EGL_EXT_image_dma_buf_import"); > On 2017/01/21 00:25:18, marcheu1 wrote: > > we might not need to test this, dma_buf_import is a hard requirement for Chrome > > OS > > In ChromeOS, GLImageOzoneNativePixmap binds YUYV dmabuf to EGLImage using dma_buf_import extensions. SO chromium_image_ycbcr_422 in ChromeOS needs the extensions. > > https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/feature_info.cc:1042: AddExtensionString("GL_CHROMIUM_ycbcr_422_image"); > On 2017/01/21 00:25:18, marcheu1 wrote: > > I think it's incorrect to prevent allocation of images based on a GL extension? > > gbm already provides the ability to tell if allocation is possible. The ability > > to import into GL should be a separate thing. > > gbm already provides the ability to tell if allocation is possible, but gbm doesn't provide the information if the buffer can be bound to EGLImage. There is not EGL query API for this. > We don't know before actuall glEGLImageTargetTexture2DOES() call, in which GLImageEGL::BindTexImage(). If it isn't supported, EGL emits egl error. > > chromium_image_ycbcr_422 in ChromeOS is EGL Image backed by YUYV dma buf. > > https://codereview.chromium.org/2648633005/diff/1/gpu/ipc/common/gpu_memory_b... > File gpu/ipc/common/gpu_memory_buffer_support.cc (right): > > https://codereview.chromium.org/2648633005/diff/1/gpu/ipc/common/gpu_memory_b... > gpu/ipc/common/gpu_memory_buffer_support.cc:21: // Disable native buffers when using Mesa. > On 2017/01/21 00:25:18, marcheu1 wrote: > > this comment is misleading, this is osmesa not mesa > > Thx for noting. Done. > This part is from existing gpu/ipc/host/gpu_memory_buffer_support.cc tho. > > https://codereview.chromium.org/2648633005/diff/1/gpu/ipc/common/gpu_memory_b... > gpu/ipc/common/gpu_memory_buffer_support.cc:49: usage == gfx::BufferUsage::SCANOUT || usage == gfx::BufferUsage::GPU_READ; > On 2017/01/21 00:25:18, marcheu1 wrote: > > what does scanout have to do with cpu read? > > must use gfx::BufferUsage::GPU_READ_CPU_READ_WRITE or gfx::BufferUsage::GPU_READ_CPU_READ_WRITE_PERSISTENT > > > Don't we have a flag for CPU read? > > This confusing is from --enable-native-gpu-buffer-memory flags > Even though native GBM is disabled, scanout native GBM is used. e.g. output_surface, video, webgl. > > --enable-native-gpu-buffer-memory may should be changed to --enable-native-gpu-buffer-memory-"map" > > see GetNativeGpuMemoryBufferConfigurations() calling this function, even if native GBM is disabled. > > It needs refactorying. I'll do it in next CL, as this CL is already big. > > https://codereview.chromium.org/2648633005/diff/1/gpu/ipc/common/gpu_memory_b... > gpu/ipc/common/gpu_memory_buffer_support.cc:109: // Disable native buffers only when using Mesa. > On 2017/01/21 00:25:18, marcheu1 wrote: > > again this is osmesa > > Done. > > https://codereview.chromium.org/2648633005/diff/1/gpu/ipc/common/gpu_memory_b... > gpu/ipc/common/gpu_memory_buffer_support.cc:118: gfx::BufferFormat::YVU_420, gfx::BufferFormat::YUV_420_BIPLANAR}; > On 2017/01/21 00:25:18, marcheu1 wrote: > > why are you hardcoding this table here? > > I just merge existing gpu/ipc/host/gpu_memory_buffer_support.cc to here. Short answer is because the functions were seperated in host/ and common/, but now we can hardcode only one time. I'll refactor this in next CL.
On 2017/01/26 21:47:00, Daniele Castagna wrote:
> > > > > Btw, I don't think we're planning to ship overlays with legacy
pageflip.
> > > >
> > > > I think so :) We want to ship overlays with atomic pageflip. The first
> target
> > > might be Kabylake, as it requires at least kernel v4.4.
>
> I was planning to start testing atomic pageflips on Apollo Lake in a few
weeks.
> It will take a while to get overlay support though.
Nice. I'll test Apollo Lake soon too.
> > > > BTW, as we can see power measurement, BGRA to YUYV change for decoder
> saves a
> > > lot of power. Most code of this CL is about YUYV decoding, rather than
> overlay.
> > > I'll extract overlay related code from this CL.
> > >
>
> Are you suggesting that with vs without overlay is saving only ~1% of power?
> That's surprisingly low compared to what we noticed on other devices.
That's good to know. GPU still need to work for other layers, so memory
bandwidth itself is not very different, because video plane goes to display
module, rather than gpu. In theory, consumed bandwidth same, though GPU is more
power hungry than display module. Do I miss something?
However, fullscreen video playback is different story, because GPU will be
totally idle.
> I also don't understand how overlays can work with YUV when
> OverlayCandidate::format is hard coded to RGB at the moment and I don't see
you
> changing it in any CL.
That's good point. I investigate what's going on and find surprising bugs.
It behaves like
1. test if RGBA plane is overlayable
2. page flip actually YUYV plane, because GPU process track actual format.
OverlayStrategySingleOnTop::TryOverlay does #1. note it sends format to gpu
process.
bool OverlayStrategySingleOnTop::TryOverlay(
QuadList* quad_list,
OverlayCandidateList* candidate_list,
const OverlayCandidate& candidate,
QuadList::Iterator candidate_iterator) {
...
capability_checker_->CheckOverlaySupport(&new_candidate_list);
...
}
GLRenderer::ScheduleOverlays does #2. note it doesn't sent format to gpu
process, and gpu process knows actual format of plane.
void GLRenderer::ScheduleOverlays(DrawingFrame* frame) {
....
context_support_->ScheduleOverlayPlane(
overlay.plane_z_order, overlay.transform, texture_id,
ToNearestRect(overlay.display_rect), overlay.uv_rect);
}
}
I'll fix the hardcode in next patch set.
> Are you using the GbmBuffer processing callback before scanning out to convert
> the buffer?
Nope. On the other hands, Skylake or new generation can support scaler of
hardware overlay, while currently drm overlay code use VPP to scale.
I'll make drm overlay code use scaler in next step. :)
> > > Yes please, split this CL in many CLs, it'll be much easier to review them
> and
> > > to get them in.
> >
> > Yes, after overall review here, I'll split.
> >
>
> Splitting the CL is mainly to help reviewers, so they can review smaller CLs
> more quickly and thoroughly. There are also less chances of CL rejection. It'd
> be great if you could do it before sending CLs out for review.
>
> > > Thank you and have a good weekend!
> >
> > Thank you! you too :)
> >
> >
>
https://codereview.chromium.org/2648633005/diff/1/content/renderer/media/rend...
> > File content/renderer/media/renderer_gpu_video_accelerator_factories.cc
> (right):
> >
> >
>
https://codereview.chromium.org/2648633005/diff/1/content/renderer/media/rend...
> > content/renderer/media/renderer_gpu_video_accelerator_factories.cc:281: //
> Mesa EGL supports not UYVY but YUYV image.
> > On 2017/01/21 00:25:18, marcheu1 wrote:
> > > pleaes don't add platform-specific code, even comments like this. If a
> platform
> > > does or doesn't support a feature, it should be detected at runtime. In
this
> > > case it seems you only need to remove that comment.
> >
> > Done.
> >
> >
>
https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/common/g...
> > File gpu/command_buffer/common/gpu_memory_buffer_support.cc (right):
> >
> >
>
https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/common/g...
> > gpu/command_buffer/common/gpu_memory_buffer_support.cc:48: // Mesa EGL
> supports not UYVY but YUYV image.
> > On 2017/01/21 00:25:18, marcheu1 wrote:
> > > ditto
> >
> > Done.
> >
> >
>
https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/service/...
> > File gpu/command_buffer/service/feature_info.cc (right):
> >
> >
>
https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/service/...
> > gpu/command_buffer/service/feature_info.cc:1037: // TODO(dshwang): when ARM
> support YUYV, remove ARCH_CPU_X86_FAMILY check.
> > On 2017/01/21 00:25:18, marcheu1 wrote:
> > > Som ARM platforms already support YUYV, please don't add the X86 check it
> isn't
> > > needed.
> >
> > Ok, remove the X86 check. and the code is protected by native GBM flags, so
> it's safe to allow ARM anyway.
> >
> >
>
https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/service/...
> > gpu/command_buffer/service/feature_info.cc:1040:
> gl::GLSurfaceEGL::HasEGLExtension("EGL_EXT_image_dma_buf_import");
> > On 2017/01/21 00:25:18, marcheu1 wrote:
> > > we might not need to test this, dma_buf_import is a hard requirement for
> Chrome
> > > OS
> >
> > In ChromeOS, GLImageOzoneNativePixmap binds YUYV dmabuf to EGLImage using
> dma_buf_import extensions. SO chromium_image_ycbcr_422 in ChromeOS needs the
> extensions.
> >
> >
>
https://codereview.chromium.org/2648633005/diff/1/gpu/command_buffer/service/...
> > gpu/command_buffer/service/feature_info.cc:1042:
> AddExtensionString("GL_CHROMIUM_ycbcr_422_image");
> > On 2017/01/21 00:25:18, marcheu1 wrote:
> > > I think it's incorrect to prevent allocation of images based on a GL
> extension?
> > > gbm already provides the ability to tell if allocation is possible. The
> ability
> > > to import into GL should be a separate thing.
> >
> > gbm already provides the ability to tell if allocation is possible, but gbm
> doesn't provide the information if the buffer can be bound to EGLImage. There
is
> not EGL query API for this.
> > We don't know before actuall glEGLImageTargetTexture2DOES() call, in which
> GLImageEGL::BindTexImage(). If it isn't supported, EGL emits egl error.
> >
> > chromium_image_ycbcr_422 in ChromeOS is EGL Image backed by YUYV dma buf.
> >
> >
>
https://codereview.chromium.org/2648633005/diff/1/gpu/ipc/common/gpu_memory_b...
> > File gpu/ipc/common/gpu_memory_buffer_support.cc (right):
> >
> >
>
https://codereview.chromium.org/2648633005/diff/1/gpu/ipc/common/gpu_memory_b...
> > gpu/ipc/common/gpu_memory_buffer_support.cc:21: // Disable native buffers
when
> using Mesa.
> > On 2017/01/21 00:25:18, marcheu1 wrote:
> > > this comment is misleading, this is osmesa not mesa
> >
> > Thx for noting. Done.
> > This part is from existing gpu/ipc/host/gpu_memory_buffer_support.cc tho.
> >
> >
>
https://codereview.chromium.org/2648633005/diff/1/gpu/ipc/common/gpu_memory_b...
> > gpu/ipc/common/gpu_memory_buffer_support.cc:49: usage ==
> gfx::BufferUsage::SCANOUT || usage == gfx::BufferUsage::GPU_READ;
> > On 2017/01/21 00:25:18, marcheu1 wrote:
> > > what does scanout have to do with cpu read?
> >
> > must use gfx::BufferUsage::GPU_READ_CPU_READ_WRITE or
> gfx::BufferUsage::GPU_READ_CPU_READ_WRITE_PERSISTENT
> >
> > > Don't we have a flag for CPU read?
> >
> > This confusing is from --enable-native-gpu-buffer-memory flags
> > Even though native GBM is disabled, scanout native GBM is used. e.g.
> output_surface, video, webgl.
> >
> > --enable-native-gpu-buffer-memory may should be changed to
> --enable-native-gpu-buffer-memory-"map"
> >
> > see GetNativeGpuMemoryBufferConfigurations() calling this function, even if
> native GBM is disabled.
> >
> > It needs refactorying. I'll do it in next CL, as this CL is already big.
> >
> >
>
https://codereview.chromium.org/2648633005/diff/1/gpu/ipc/common/gpu_memory_b...
> > gpu/ipc/common/gpu_memory_buffer_support.cc:109: // Disable native buffers
> only when using Mesa.
> > On 2017/01/21 00:25:18, marcheu1 wrote:
> > > again this is osmesa
> >
> > Done.
> >
> >
>
https://codereview.chromium.org/2648633005/diff/1/gpu/ipc/common/gpu_memory_b...
> > gpu/ipc/common/gpu_memory_buffer_support.cc:118: gfx::BufferFormat::YVU_420,
> gfx::BufferFormat::YUV_420_BIPLANAR};
> > On 2017/01/21 00:25:18, marcheu1 wrote:
> > > why are you hardcoding this table here?
> >
> > I just merge existing gpu/ipc/host/gpu_memory_buffer_support.cc to here.
Short
> answer is because the functions were seperated in host/ and common/, but now
we
> can hardcode only one time. I'll refactor this in next CL.
Yes, I'll do it soon. Thank you.
johnson.lin@intel.com changed reviewers: + johnson.lin@intel.com
This line is the same with above? Why add the same ENUM?
+const gfx::BufferFormat kAllocatePictureFormat = gfx::BufferFormat::YUYV_422; This line still hardcoding the video output format. Is there any mechanism to check the best format that hardware support? That will make it extensible. For eg, on later platforms, NV12 might be supported
+ if (controller->IsFormatSupported(DRM_FORMAT_YUYV, z_order)) {
+ return DRM_FORMAT_YUYV;
Here is another hardcoding...
+ if (controller->IsFormatSupported(DRM_FORMAT_YUYV, z_order)) {
+ return DRM_FORMAT_YUYV;
Here is another hardcoding...
Thank you for reviewing. I'm recoverying slowly after BlinkOn7
On 2017/02/01 12:56:14, johnson.lin wrote:
> This line is the same with above? Why add the same ENUM?
Which line do you talk about? You can add comment in the code. double-click the
code which you want to add comment and click "Publish+Mail Comments"
On 2017/02/01 13:00:45, johnson.lin wrote:
> +const gfx::BufferFormat kAllocatePictureFormat = gfx::BufferFormat::YUYV_422;
>
> This line still hardcoding the video output format.
> Is there any mechanism to check the best format that hardware support? That
will
> make it extensible. For eg, on later platforms, NV12 might be supported
As this CL is already big, I'll add the mechanism when I support NV12 for VAAPI
decoder.
On 2017/02/01 13:05:54, johnson.lin wrote:
> + if (controller->IsFormatSupported(DRM_FORMAT_YUYV, z_order)) {
> + return DRM_FORMAT_YUYV;
>
> Here is another hardcoding...
It's explicitly note YUYV is preferable, rather than hardcoding, IMO.
Description was changed from ========== Enable YUV video overlay on Skylake ChromeOS. Add YUYV (i.e YUY2) for video decoder output and using those formats for overlay YUYV is only format supported by all linux stack such as kernel, mesa, and minigbm, so ChromeOS IA prefer to use YUYV for video decoding. This CL changes 1. make gpu decoder (only vaapi) decode on YUYV plane 2. make --enable-gpu-memory-buffer-video-frames choose YUYV if possible 3. make Ozone GBM prefer YUYV rather than UYUV About 1, check the change of media/gpu/vaapi_video_decode_accelerator.cc About 2, check the change of content/renderer/media/renderer_gpu_video_accelerator_factories.cc and gpu/command_buffer/service/feature_info.cc About 3, check the change of ui/ozone/platform/drm/gpu/drm_overlay_validator.cc This CL is based on Johnson Lin's implementaion https://codereview.chromium.org/2636433003/ Test results of power consumption: 8.8% power save. Check more detail in crbug.com/683347 BUG=683347 TEST=run relevant unittests run chrome on http://www.quirksmode.org/html5/tests/video.html with all possible configurations * decoding configuration * gpu decoding: N/A * pure software decoding: --disable-accelerated-video-decode * GMB video frame: --disable-accelerated-video-decode --enable-native-gpu-memory-buffers --enable-gpu-memory-buffer-video-frames * overlay configurations * atomic mode setting: --enable-hardware-overlays --enable-drm-atomic * legacy mode setting: --enable-hardware-overlays * no overlay: N/A CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Enable YUYV GPU memory buffer video frames YUYV is only format supported by all linux stack such as kernel, mesa, minigbm, and gpu decoder. So ChromeOS IA prefers to use YUYV for video decoding. This CL makes --enable-gpu-memory-buffer-video-frames choose YUYV if possible. This CL is based on Johnson Lin's implementaion https://codereview.chromium.org/2636433003/ BUG=683347 TEST=run relevant unittests run chrome on http://www.quirksmode.org/html5/tests/video.html with all possible configurations * decoding configuration * gpu decoding: N/A * pure software decoding: --disable-accelerated-video-decode * GMB video frame: --disable-accelerated-video-decode --enable-native-gpu-memory-buffers --enable-gpu-memory-buffer-video-frames * overlay configurations * atomic mode setting: --enable-hardware-overlays --enable-drm-atomic * legacy mode setting: --enable-hardware-overlays * no overlay: N/A CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Enable YUYV GPU memory buffer video frames YUYV is only format supported by all linux stack such as kernel, mesa, minigbm, and gpu decoder. So ChromeOS IA prefers to use YUYV for video decoding. This CL makes --enable-gpu-memory-buffer-video-frames choose YUYV if possible. This CL is based on Johnson Lin's implementaion https://codereview.chromium.org/2636433003/ BUG=683347 TEST=run relevant unittests run chrome on http://www.quirksmode.org/html5/tests/video.html with all possible configurations * decoding configuration * gpu decoding: N/A * pure software decoding: --disable-accelerated-video-decode * GMB video frame: --disable-accelerated-video-decode --enable-native-gpu-memory-buffers --enable-gpu-memory-buffer-video-frames * overlay configurations * atomic mode setting: --enable-hardware-overlays --enable-drm-atomic * legacy mode setting: --enable-hardware-overlays * no overlay: N/A CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cros: Enable YUYV GPU memory buffer video frames YUYV is only format supported by all linux stack such as kernel, mesa, minigbm, and gpu decoder. So ChromeOS IA prefers to use YUYV for video decoding. This CL makes --enable-gpu-memory-buffer-video-frames choose YUYV if possible. This CL is based on Johnson Lin's implementaion https://codereview.chromium.org/2636433003/ BUG=683347 TEST=run relevant unittests run chrome on http://www.quirksmode.org/html5/tests/video.html with all possible configurations * decoding configuration * gpu decoding: N/A * pure software decoding: --disable-accelerated-video-decode * GMB video frame: --disable-accelerated-video-decode --enable-native-gpu-memory-buffers --enable-gpu-memory-buffer-video-frames * overlay configurations * atomic mode setting: --enable-hardware-overlays --enable-drm-atomic * legacy mode setting: --enable-hardware-overlays * no overlay: N/A CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
As reviewer requested, I'm splitting CLs. This CL depends on cc: UYVY video is not premultiplied rgba https://codereview.chromium.org/2684073006/ Introduce gfx::BufferFormat::YUYV_422 https://codereview.chromium.org/2689453002 Make OverlayCandidate use gfx::BufferFormat instead of cc::ResourceFormat https://codereview.chromium.org/2683763003/ gpu: merge gpu/ipc/host/gpu_memory_buffer_support.cc to gpu/ipc/common/ https://codereview.chromium.org/2649553003/ chromeos: gpu video decoder on Intel decodes video in YUYV format, instead of RGBA. https://codereview.chromium.org/2678343011/ ozone: prefer YUYV format for overlay. https://codereview.chromium.org/2686903003/ I'll request review again when prerequsites are landed. Thank you.
On 2017/02/09 04:55:29, dshwang wrote: > As reviewer requested, I'm splitting CLs. This CL depends on > cc: UYVY video is not premultiplied rgba > https://codereview.chromium.org/2684073006/ > Introduce gfx::BufferFormat::YUYV_422 https://codereview.chromium.org/2689453002 > Make OverlayCandidate use gfx::BufferFormat instead of cc::ResourceFormat > https://codereview.chromium.org/2683763003/ > gpu: merge gpu/ipc/host/gpu_memory_buffer_support.cc to gpu/ipc/common/ > https://codereview.chromium.org/2649553003/ > chromeos: gpu video decoder on Intel decodes video in YUYV format, instead of > RGBA. https://codereview.chromium.org/2678343011/ > ozone: prefer YUYV format for overlay. > https://codereview.chromium.org/2686903003/ > > I'll request review again when prerequsites are landed. Thank you. Before reenabling GMBs, can you fix the issues with dmabuf mmap() for these? I don't think it's fit for reenabling otherwise, and I don't want to start relying on it until then. I actually suspect there might be lifetime issues around these, which causes the issues you are seeing.
Description was changed from ========== cros: Enable YUYV GPU memory buffer video frames YUYV is only format supported by all linux stack such as kernel, mesa, minigbm, and gpu decoder. So ChromeOS IA prefers to use YUYV for video decoding. This CL makes --enable-gpu-memory-buffer-video-frames choose YUYV if possible. This CL is based on Johnson Lin's implementaion https://codereview.chromium.org/2636433003/ BUG=683347 TEST=run relevant unittests run chrome on http://www.quirksmode.org/html5/tests/video.html with all possible configurations * decoding configuration * gpu decoding: N/A * pure software decoding: --disable-accelerated-video-decode * GMB video frame: --disable-accelerated-video-decode --enable-native-gpu-memory-buffers --enable-gpu-memory-buffer-video-frames * overlay configurations * atomic mode setting: --enable-hardware-overlays --enable-drm-atomic * legacy mode setting: --enable-hardware-overlays * no overlay: N/A CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cros: Support YUYV format for GPU memory buffer video frames YUYV is only format supported by all linux stack such as kernel, mesa, minigbm, and gpu decoder. So ChromeOS IA prefers to use YUYV for video decoding. This CL makes --enable-gpu-memory-buffer-video-frames choose YUYV if possible. This CL is based on Johnson Lin's implementaion https://codereview.chromium.org/2636433003/ BUG=683347 TEST=run relevant unittests run chrome on http://www.quirksmode.org/html5/tests/video.html with all possible configurations * decoding configuration * gpu decoding: N/A * pure software decoding: --disable-accelerated-video-decode * GMB video frame: --disable-accelerated-video-decode --enable-native-gpu-memory-buffers --enable-gpu-memory-buffer-video-frames * overlay configurations * atomic mode setting: --enable-hardware-overlays --enable-drm-atomic * legacy mode setting: --enable-hardware-overlays * no overlay: N/A CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
On 2017/02/10 23:46:19, marcheu wrote: > Before reenabling GMBs, can you fix the issues with dmabuf mmap() for these? I > don't think it's fit for reenabling otherwise, and I don't want to start relying > on it until then. I actually suspect there might be lifetime issues around > these, which causes the issues you are seeing. Hi marcheu, sorry for taking time to fix GMB issue. Daniele also mentioned we need to fix GMB issue. I also want to fix it. On the other hands, the prerequisite 6 CLs is not related GMB mmap issue. Only this CL depends on GMB mmap enablement. Above 6 CLs are about gpu video decoding, and gpu decoder uses scanline GBM buffer like output_surface. In my opinion, we can review above 6 CLs in parallel. It makes sense to review this CL after fixing GMB mmap issue. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
