New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 822346 link

Starred by 9 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocked on:
issue 910986
issue 813250
issue 857095

Blocking:
issue 717265
issue 789288



Sign in to add a comment

try and remove the use of the Video processing pipeline inside the va video decode accelerator

Project Member Reported by mcasas@chromium.org, Mar 15 2018

Issue description

With NV12 support, consider removing the use of the Video processing 
pipeline inside VaapiWrapper::BlitSurface() for video decoding.

 

Comment 1 by mcasas@chromium.org, Mar 15 2018

Blockedon: 717265 813250

Comment 2 by mcasas@chromium.org, Mar 15 2018

Cc: mcasas@chromium.org dongseong.hwang@chromium.org zelidrag@chromium.org posciak@chromium.org
 Issue 762358  has been merged into this issue.
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9eadd641346087922a0fd5d958ae041464e19436

commit 9eadd641346087922a0fd5d958ae041464e19436
Author: Miguel Casas-Sanchez <mcasas@chromium.org>
Date: Tue Mar 20 05:24:58 2018

vaapi: fold wrapper's CreateUnownedSurface() into caller

This CL folds the method VaapiWrapper::CreateUnownedSurface() into its
only caller. This nicely cuts the amount of lines, reduces the scope
of the |va_lock_|, and improves error mgmt by only having one place
for VA_SUCCESS_OR_RETURN (because the VASurface ctor cannot fail).

Also vavda's CloseGpuMemoryBufferHandle is moved to the unnamed
namespace.

Bug: 822346
Cq-Include-Trybots: luci.chromium.try:linux_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
Change-Id: I7aa690e3d3976540f2edf07600511ca65f76b6f0
Reviewed-on: https://chromium-review.googlesource.com/964591
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544290}
[modify] https://crrev.com/9eadd641346087922a0fd5d958ae041464e19436/media/gpu/vaapi/va_surface.h
[modify] https://crrev.com/9eadd641346087922a0fd5d958ae041464e19436/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/9eadd641346087922a0fd5d958ae041464e19436/media/gpu/vaapi/vaapi_wrapper.cc
[modify] https://crrev.com/9eadd641346087922a0fd5d958ae041464e19436/media/gpu/vaapi/vaapi_wrapper.h

Project Member

Comment 4 by bugdroid1@chromium.org, Apr 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c813adf6312dda8d54aca42dd5dc4460248726ad

commit c813adf6312dda8d54aca42dd5dc4460248726ad
Author: Miguel Casas <mcasas@chromium.org>
Date: Mon Apr 02 23:13:56 2018

vaapi: cleanup a few comments and var names

This CL cleans up a few things that I realized while
debugging other CLs in the area:
- |available_va_surfaces_| is a std::list on ToT, but has
 queue semantics, so it's changed int his CL.
-  s/output_buffers_/available_va_surfaces_/ because that's
 what they are.
- s/TryOutputSurface/TryOutputPicture/ because that's what
 the method does.
- s/pictures_/picture_map_/ to better define what it is. After
 dcastagna@ suggestion, it's made a base::small_map in PS7

Comments updated throughout these variables.

No change in behaviour intended, but tested nonetheless with
vp8/9-h264 crosvideo playback.

Bug: 822346
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I2117ceac29f7d6ed23d698c4abd5c72e0140ae47
Reviewed-on: https://chromium-review.googlesource.com/988512
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547552}
[modify] https://crrev.com/c813adf6312dda8d54aca42dd5dc4460248726ad/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/c813adf6312dda8d54aca42dd5dc4460248726ad/media/gpu/vaapi/vaapi_video_decode_accelerator.h

Cc: acourbot@chromium.org
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 5 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/957c5c2a38613428c47239f8c702c667aa6fb7f9

commit 957c5c2a38613428c47239f8c702c667aa6fb7f9
Author: Miguel Casas <mcasas@chromium.org>
Date: Thu Apr 05 19:04:41 2018

vaapi: extract vaCreateContext() to its own method in VaapiWrapper

This CL extracts the call to vaCreateContext() out of VaapiWrapper's
CreateSurfaces() and into a new method VaapiWrapper::CreateContext().

The former still calls the latter, but this change allows for separating
the allocation of the surfaces from the creation of the context. In
particular, this is needed for decoding directly on client Surfaces
(see the bug and/or the experimental CL crrev.com/c/986353. from
which this code is separated).

Bug: 822346
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I37f28d2a0fe939264d3153f34bd25bbc6c77ba79
Reviewed-on: https://chromium-review.googlesource.com/995623
Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548515}
[modify] https://crrev.com/957c5c2a38613428c47239f8c702c667aa6fb7f9/media/gpu/vaapi/vaapi_wrapper.cc
[modify] https://crrev.com/957c5c2a38613428c47239f8c702c667aa6fb7f9/media/gpu/vaapi/vaapi_wrapper.h

Project Member

Comment 7 by bugdroid1@chromium.org, Apr 10 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b4990829f33305ea94a6649771e331af97d3066d

commit b4990829f33305ea94a6649771e331af97d3066d
Author: Miguel Casas <mcasas@chromium.org>
Date: Tue Apr 10 13:42:52 2018

vaapi: minor cleanups in VaVDA (OutputPicture, mostly)

This CL is a few cleanups to ease review of crrev.com/c/986353:

- PictureById() is moved to the end of the cc file, following the
 order of its declaration. Code untouched (except s/NULL/nullptr/).
 A call to PictureById  is substituted with base::ContainsKey()
 to better reflect intent.

- OutputPicture() loses its last parameter, that is then calculated
 on the spot (it's |available_picture_buffers_.front()|). This makes
 OutputCB unnecessary, so it's made a Closure, and a OnceClosure at
 that.

Test: eve crosvideo vp9 cycling resolutions.

Bug: 822346
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: Ie17f23f2fb20752463e9ebeacd1bf7a9b6150437
Reviewed-on: https://chromium-review.googlesource.com/1000297
Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549506}
[modify] https://crrev.com/b4990829f33305ea94a6649771e331af97d3066d/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/b4990829f33305ea94a6649771e331af97d3066d/media/gpu/vaapi/vaapi_video_decode_accelerator.h

Blocking: 789288
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ba217eddda82e1781ee977c4ad5f4d26feb2bdda

commit ba217eddda82e1781ee977c4ad5f4d26feb2bdda
Author: Miguel Casas <mcasas@chromium.org>
Date: Thu Apr 19 20:33:33 2018

Revert "vaapi: minor cleanups in VaVDA (OutputPicture, mostly)"

This reverts commit b4990829f33305ea94a6649771e331af97d3066d.

Reason for revert: Needed to revert other CLs that caused a
regression in video_VideoSeek:  https://crbug.com/834146 
Bug:  834146 


Original change's description:
> vaapi: minor cleanups in VaVDA (OutputPicture, mostly)
>
> This CL is a few cleanups to ease review of crrev.com/c/986353:
>
> - PictureById() is moved to the end of the cc file, following the
>  order of its declaration. Code untouched (except s/NULL/nullptr/).
>  A call to PictureById  is substituted with base::ContainsKey()
>  to better reflect intent.
>
> - OutputPicture() loses its last parameter, that is then calculated
>  on the spot (it's |available_picture_buffers_.front()|). This makes
>  OutputCB unnecessary, so it's made a Closure, and a OnceClosure at
>  that.
>
> Test: eve crosvideo vp9 cycling resolutions.
>
> Bug: 822346
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel
> Change-Id: Ie17f23f2fb20752463e9ebeacd1bf7a9b6150437
> Reviewed-on: https://chromium-review.googlesource.com/1000297
> Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>
> Commit-Queue: Miguel Casas <mcasas@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#549506}

TBR=mcasas@chromium.org,hoegsberg@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 822346
Change-Id: If0d162b5373c33c266268d9cd82e3de5fd4e4a7e
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/1020003
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552146}
[modify] https://crrev.com/ba217eddda82e1781ee977c4ad5f4d26feb2bdda/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/ba217eddda82e1781ee977c4ad5f4d26feb2bdda/media/gpu/vaapi/vaapi_video_decode_accelerator.h

Project Member

Comment 10 by bugdroid1@chromium.org, Apr 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2edc7ca953140f70f5c5e79defda98511c9fa046

commit 2edc7ca953140f70f5c5e79defda98511c9fa046
Author: Miguel Casas <mcasas@chromium.org>
Date: Thu Apr 19 21:36:05 2018

Revert "vaapi: cleanup a few comments and var names"

This reverts commit c813adf6312dda8d54aca42dd5dc4460248726ad.

Reason for revert: Needed to revert other CLs that caused a
regression in video_VideoSeek:  https://crbug.com/834146 
Bug:  834146 

Original change's description:
> vaapi: cleanup a few comments and var names
>
> This CL cleans up a few things that I realized while
> debugging other CLs in the area:
> - |available_va_surfaces_| is a std::list on ToT, but has
>  queue semantics, so it's changed int his CL.
> -  s/output_buffers_/available_va_surfaces_/ because that's
>  what they are.
> - s/TryOutputSurface/TryOutputPicture/ because that's what
>  the method does.
> - s/pictures_/picture_map_/ to better define what it is. After
>  dcastagna@ suggestion, it's made a base::small_map in PS7
>
> Comments updated throughout these variables.
>
> No change in behaviour intended, but tested nonetheless with
> vp8/9-h264 crosvideo playback.
>
> Bug: 822346
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel
> Change-Id: I2117ceac29f7d6ed23d698c4abd5c72e0140ae47
> Reviewed-on: https://chromium-review.googlesource.com/988512
> Commit-Queue: Miguel Casas <mcasas@chromium.org>
> Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#547552}

TBR=mcasas@chromium.org,dcastagna@chromium.org,hoegsberg@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 822346
Change-Id: I7d242ad92a68596766f6cfe0ad2be37da3c4fb20
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/1020005
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552166}
[modify] https://crrev.com/2edc7ca953140f70f5c5e79defda98511c9fa046/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/2edc7ca953140f70f5c5e79defda98511c9fa046/media/gpu/vaapi/vaapi_video_decode_accelerator.h

Project Member

Comment 11 by bugdroid1@chromium.org, Apr 20 2018

Labels: merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f1fa9073428c71ee8f333b6c121c5a853d45402d

commit f1fa9073428c71ee8f333b6c121c5a853d45402d
Author: Miguel Casas <mcasas@chromium.org>
Date: Fri Apr 20 22:07:00 2018

Revert "vaapi: minor cleanups in VaVDA (OutputPicture, mostly)"

This reverts commit b4990829f33305ea94a6649771e331af97d3066d.

Reason for revert: Needed to revert other CLs that caused a
regression in video_VideoSeek:  https://crbug.com/834146 
Bug:  834146 


Original change's description:
> vaapi: minor cleanups in VaVDA (OutputPicture, mostly)
>
> This CL is a few cleanups to ease review of crrev.com/c/986353:
>
> - PictureById() is moved to the end of the cc file, following the
>  order of its declaration. Code untouched (except s/NULL/nullptr/).
>  A call to PictureById  is substituted with base::ContainsKey()
>  to better reflect intent.
>
> - OutputPicture() loses its last parameter, that is then calculated
>  on the spot (it's |available_picture_buffers_.front()|). This makes
>  OutputCB unnecessary, so it's made a Closure, and a OnceClosure at
>  that.
>
> Test: eve crosvideo vp9 cycling resolutions.
>
> Bug: 822346
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel
> Change-Id: Ie17f23f2fb20752463e9ebeacd1bf7a9b6150437
> Reviewed-on: https://chromium-review.googlesource.com/1000297
> Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>
> Commit-Queue: Miguel Casas <mcasas@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#549506}

TBR=mcasas@chromium.org,hoegsberg@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 822346
Change-Id: If0d162b5373c33c266268d9cd82e3de5fd4e4a7e
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/1020003
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#552146}(cherry picked from commit ba217eddda82e1781ee977c4ad5f4d26feb2bdda)
Reviewed-on: https://chromium-review.googlesource.com/1022634
Cr-Commit-Position: refs/branch-heads/3396@{#179}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/f1fa9073428c71ee8f333b6c121c5a853d45402d/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/f1fa9073428c71ee8f333b6c121c5a853d45402d/media/gpu/vaapi/vaapi_video_decode_accelerator.h

Project Member

Comment 12 by bugdroid1@chromium.org, Apr 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e77a576a01e4d3f7f7e4c69d892f8cf60c4c9440

commit e77a576a01e4d3f7f7e4c69d892f8cf60c4c9440
Author: Miguel Casas <mcasas@chromium.org>
Date: Fri Apr 20 22:07:59 2018

Revert "vaapi: cleanup a few comments and var names"

This reverts commit c813adf6312dda8d54aca42dd5dc4460248726ad.

Reason for revert: Needed to revert other CLs that caused a
regression in video_VideoSeek:  https://crbug.com/834146 
Bug:  834146 

Original change's description:
> vaapi: cleanup a few comments and var names
>
> This CL cleans up a few things that I realized while
> debugging other CLs in the area:
> - |available_va_surfaces_| is a std::list on ToT, but has
>  queue semantics, so it's changed int his CL.
> -  s/output_buffers_/available_va_surfaces_/ because that's
>  what they are.
> - s/TryOutputSurface/TryOutputPicture/ because that's what
>  the method does.
> - s/pictures_/picture_map_/ to better define what it is. After
>  dcastagna@ suggestion, it's made a base::small_map in PS7
>
> Comments updated throughout these variables.
>
> No change in behaviour intended, but tested nonetheless with
> vp8/9-h264 crosvideo playback.
>
> Bug: 822346
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel
> Change-Id: I2117ceac29f7d6ed23d698c4abd5c72e0140ae47
> Reviewed-on: https://chromium-review.googlesource.com/988512
> Commit-Queue: Miguel Casas <mcasas@chromium.org>
> Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#547552}

TBR=mcasas@chromium.org,dcastagna@chromium.org,hoegsberg@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 822346
Change-Id: I7d242ad92a68596766f6cfe0ad2be37da3c4fb20
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/1020005
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#552166}(cherry picked from commit 2edc7ca953140f70f5c5e79defda98511c9fa046)
Reviewed-on: https://chromium-review.googlesource.com/1022911
Cr-Commit-Position: refs/branch-heads/3396@{#181}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/e77a576a01e4d3f7f7e4c69d892f8cf60c4c9440/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/e77a576a01e4d3f7f7e4c69d892f8cf60c4c9440/media/gpu/vaapi/vaapi_video_decode_accelerator.h

Blockedon: -717265
Blocking: 717265
Project Member

Comment 14 by bugdroid1@chromium.org, May 4 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/minigbm/+/222919005b758a0afe24d2c2cc4bac342b575bbd

commit 222919005b758a0afe24d2c2cc4bac342b575bbd
Author: Kristian H. Kristensen <hoegsberg@chromium.org>
Date: Fri May 04 10:02:36 2018

minigbm: Drop drv_bo_get_stride_in_pixels() helper

Computing "stride in pixels" assumes the pixel size divides the
stride, which isn't always the case.  This is only used for the
cros_gralloc implementation, so lets move the calculation there
instead of providing a generic helper for an unhealthy concept.

BUG=822346
TEST=test_that graphics_Gbm

Change-Id: Iab7a363c5471e4bacef7df7095ef77723adf5e93
Reviewed-on: https://chromium-review.googlesource.com/996645
Commit-Ready: Kristian H. Kristensen <hoegsberg@chromium.org>
Tested-by: Kristian H. Kristensen <hoegsberg@chromium.org>
Reviewed-by: Gurchetan Singh <gurchetansingh@chromium.org>

[modify] https://crrev.com/222919005b758a0afe24d2c2cc4bac342b575bbd/drv.h
[modify] https://crrev.com/222919005b758a0afe24d2c2cc4bac342b575bbd/cros_gralloc/cros_gralloc_driver.cc
[modify] https://crrev.com/222919005b758a0afe24d2c2cc4bac342b575bbd/helpers.h
[modify] https://crrev.com/222919005b758a0afe24d2c2cc4bac342b575bbd/helpers.c

Project Member

Comment 15 by bugdroid1@chromium.org, May 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/minigbm/+/e8778f00ce4145eb09f57e2ec954308666cc99bf

commit e8778f00ce4145eb09f57e2ec954308666cc99bf
Author: Kristian H. Kristensen <hoegsberg@chromium.org>
Date: Tue May 08 03:45:47 2018

i915: Align stride and offset per plane

Current code over-aligns the Y-plane dimensions by a factor of two, so
as to make sure alignment requirements are still satisfied when the
subsampled plane stride and height are divided by two. Instead, this
commit changes the approach to align each plane separately after
computing the plane width and height. We stop using
drv_bo_from_format(), which divides the stride and instead loop
through the planes ourselves.

BUG=822346
TEST=test_that graphics_Gbm

Change-Id: I1ea8f2fb8b1780686d4086f51e9bab759f724d78
Reviewed-on: https://chromium-review.googlesource.com/996647
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Kristian H. Kristensen <hoegsberg@chromium.org>
Reviewed-by: Gurchetan Singh <gurchetansingh@chromium.org>

[modify] https://crrev.com/e8778f00ce4145eb09f57e2ec954308666cc99bf/util.h
[modify] https://crrev.com/e8778f00ce4145eb09f57e2ec954308666cc99bf/i915.c

Project Member

Comment 16 by bugdroid1@chromium.org, May 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/minigbm/+/1bd7b04a3ae68c0314bdee06c559093de9e5a304

commit 1bd7b04a3ae68c0314bdee06c559093de9e5a304
Author: Kristian H. Kristensen <hoegsberg@chromium.org>
Date: Sat May 19 03:08:25 2018

i915: Add GBM_BO_USE_HW_VIDEO_DECODER use flag

This flag is used to indicate that the platform video decoder will be
writing into this buffer and that it should be allocated
accordingly. On Intel, this means that we have to allocate y-tiled
NV12 for libva to be able to decode to the buffer.

We force gralloc NV12 allocations to be linear for now, since ARC++
doesn't properly pass modifiers to ChromeOS.

BUG=822346
TEST=test_that graphics_Gbm

Change-Id: I840c30d22355d26816df718b49717407e2e4620f
Reviewed-on: https://chromium-review.googlesource.com/996648
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Kristian H. Kristensen <hoegsberg@chromium.org>
Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>

[modify] https://crrev.com/1bd7b04a3ae68c0314bdee06c559093de9e5a304/i915.c
[modify] https://crrev.com/1bd7b04a3ae68c0314bdee06c559093de9e5a304/drv.h
[modify] https://crrev.com/1bd7b04a3ae68c0314bdee06c559093de9e5a304/cros_gralloc/cros_gralloc_driver.cc
[modify] https://crrev.com/1bd7b04a3ae68c0314bdee06c559093de9e5a304/gbm_helpers.c
[modify] https://crrev.com/1bd7b04a3ae68c0314bdee06c559093de9e5a304/gbm.h

Project Member

Comment 17 by bugdroid1@chromium.org, May 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/minigbm/+/2b682fde2d2fe12b95b434a47466bc69aa08b6bd

commit 2b682fde2d2fe12b95b434a47466bc69aa08b6bd
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Tue May 22 08:06:31 2018

Revert "i915: Add GBM_BO_USE_HW_VIDEO_DECODER use flag"

This reverts commit 1bd7b04a3ae68c0314bdee06c559093de9e5a304.

Reason for revert: Breaks video decoding on most intel platforms, e.g., caroline and samus

Original change's description:
> i915: Add GBM_BO_USE_HW_VIDEO_DECODER use flag
> 
> This flag is used to indicate that the platform video decoder will be
> writing into this buffer and that it should be allocated
> accordingly. On Intel, this means that we have to allocate y-tiled
> NV12 for libva to be able to decode to the buffer.
> 
> We force gralloc NV12 allocations to be linear for now, since ARC++
> doesn't properly pass modifiers to ChromeOS.
> 
> BUG=822346
> TEST=test_that graphics_Gbm
> 
> Change-Id: I840c30d22355d26816df718b49717407e2e4620f
> Reviewed-on: https://chromium-review.googlesource.com/996648
> Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
> Tested-by: Kristian H. Kristensen <hoegsberg@chromium.org>
> Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>

Bug: 822346,  845076 
Change-Id: I7681ddb66e4789951e840821993fc5562a55d1af
Reviewed-on: https://chromium-review.googlesource.com/1066032
Tested-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Kuo Jen Wei <inker@chromium.org>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>

[modify] https://crrev.com/2b682fde2d2fe12b95b434a47466bc69aa08b6bd/i915.c
[modify] https://crrev.com/2b682fde2d2fe12b95b434a47466bc69aa08b6bd/drv.h
[modify] https://crrev.com/2b682fde2d2fe12b95b434a47466bc69aa08b6bd/cros_gralloc/cros_gralloc_driver.cc
[modify] https://crrev.com/2b682fde2d2fe12b95b434a47466bc69aa08b6bd/gbm_helpers.c
[modify] https://crrev.com/2b682fde2d2fe12b95b434a47466bc69aa08b6bd/gbm.h

Project Member

Comment 18 by bugdroid1@chromium.org, May 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/minigbm/+/3cb5bbacc5c8a79105c868875222696f6b9d8296

commit 3cb5bbacc5c8a79105c868875222696f6b9d8296
Author: Kristian H. Kristensen <hoegsberg@chromium.org>
Date: Wed May 30 19:50:33 2018

Reland "i915: Add GBM_BO_USE_HW_VIDEO_DECODER use flag"

This is a reland of 1bd7b04a3ae68c0314bdee06c559093de9e5a304

We've landed CL:1070995 which fixes the missing modifier that
caused tiling corruption on pre-KBL Intel devices and CL:1072638
which makes the failing vda unittests use SCANOUT_VDA_WRITE as
they were supposed to.

Original change's description:
> i915: Add GBM_BO_USE_HW_VIDEO_DECODER use flag
>
> This flag is used to indicate that the platform video decoder will be
> writing into this buffer and that it should be allocated
> accordingly. On Intel, this means that we have to allocate y-tiled
> NV12 for libva to be able to decode to the buffer.
>
> We force gralloc NV12 allocations to be linear for now, since ARC++
> doesn't properly pass modifiers to ChromeOS.
>
> BUG=822346
> TEST=test_that graphics_Gbm
>
> Change-Id: I840c30d22355d26816df718b49717407e2e4620f
> Reviewed-on: https://chromium-review.googlesource.com/996648
> Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
> Tested-by: Kristian H. Kristensen <hoegsberg@chromium.org>
> Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>

Bug: 822346
Change-Id: Icf0921dc91ac422da26371bffea34b26550b8234
Reviewed-on: https://chromium-review.googlesource.com/1073877
Commit-Ready: Kristian H. Kristensen <hoegsberg@chromium.org>
Tested-by: Kristian H. Kristensen <hoegsberg@chromium.org>
Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>

[modify] https://crrev.com/3cb5bbacc5c8a79105c868875222696f6b9d8296/i915.c
[modify] https://crrev.com/3cb5bbacc5c8a79105c868875222696f6b9d8296/drv.h
[modify] https://crrev.com/3cb5bbacc5c8a79105c868875222696f6b9d8296/cros_gralloc/cros_gralloc_driver.cc
[modify] https://crrev.com/3cb5bbacc5c8a79105c868875222696f6b9d8296/gbm_helpers.c
[modify] https://crrev.com/3cb5bbacc5c8a79105c868875222696f6b9d8296/gbm.h

#19 landed in 10738.0.0
The CL doesn't say it's merged...?
You mean #18?
Yeah, I meant that the CL #18 (that I incorrectly misunderstood to be the
same as in #19) is (will be) in the 10738.0.0 SDK.
Project Member

Comment 24 by bugdroid1@chromium.org, Jun 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/852481bbb3820e762043b71a4ebf986f923b1d9b

commit 852481bbb3820e762043b71a4ebf986f923b1d9b
Author: Miguel Casas <mcasas@chromium.org>
Date: Fri Jun 01 16:33:47 2018

Roll third_party/minigbm 6eca368..3cb5bba

$ git slog 6eca36809e185337bfcca95310a1765c34c360e1..3cb5bbacc5c8a79105c868875222696f6b9d8296
3cb5bba Reland "i915: Add GBM_BO_USE_HW_VIDEO_DECODER use flag"
2b682fd Revert "i915: Add GBM_BO_USE_HW_VIDEO_DECODER use flag"
1bd7b04 i915: Add GBM_BO_USE_HW_VIDEO_DECODER use flag
e430ac5 minigbm: remove addrlib dependency for arc-cros-gralloc
f4e12f5 minigbm: Update Android.mk to export the include path
77b7055 minigbm: Use the stride value returned by mapImage
62a9c4e minigbm: amdgpu: switch BO allocation domain to GTT from VRAM
767c538 Revert "minigbm: add support for HAL_PIXEL_FORMAT_RGBA_FP16"
e8778f0 i915: Align stride and offset per plane
a0e602b minigbm: For BO_USE_SW usage buffer need not be linear
90b1458 Revert "minigbm: i915: Add necessary padding to Android YV12 buffers"
bb1f4dd Revert "minigbm: i965: Add 64-byte padding at the end of linear buffers"
2229190 minigbm: Drop drv_bo_get_stride_in_pixels() helper
292da53 minigbm: add support for HAL_PIXEL_FORMAT_RGBA_FP16
2eeaf5a minigbm: cros_gralloc: fix -Wimplicit-function-declaration warnings in gralloctest
478343f minigbm: Consolidate format info in new struct planar_layout
65141c2 Build fix against latest AOSP master.
f8ff0f5 Fix -Wcast-qual warnings.
cdcebd8 minigbm: using dri extensions
9c3fb32 i915: Allow allocating ARGB buffers for scanout
249e863 minigbm: virtio_gpu: select dumb/virgl at runtime.
d8c0455 Update Android.mk for amlogic
bd1b1b5 minigbm: drv_bo_flush --> drv_bo_flush_or_unmap
7cfcc28 minigbm: Enable vc4 driver
3cf8c92 minigbm: amdgpu: Disable explicit synchronization when possible.
0cfaaa5 Use Android log system in helpers.c.
ce717c5 Android: Support building the virtio_virgl backend.
f048a1e Fix minigbm against older kernel DRM versions.
5932c4c Fix the build against AOSP master.
a50131a Build fix for older libdrm versions.
3381588 minigbm: virtio_gpu: set vma->length when mmap.


Bug: 822346
Change-Id: I8358ec3263d3348b173d4f9f396ea5a849856cc8
Reviewed-on: https://chromium-review.googlesource.com/1081024
Reviewed-by: Daniel Nicoara <dnicoara@chromium.org>
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563672}
[modify] https://crrev.com/852481bbb3820e762043b71a4ebf986f923b1d9b/DEPS
[modify] https://crrev.com/852481bbb3820e762043b71a4ebf986f923b1d9b/third_party/minigbm/BUILD.gn

Project Member

Comment 25 by bugdroid1@chromium.org, Jun 4 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/37a2e00b9c5cc8f9f60fb690fecc22a2a09e8fd2

commit 37a2e00b9c5cc8f9f60fb690fecc22a2a09e8fd2
Author: Miguel Casas <mcasas@chromium.org>
Date: Mon Jun 04 19:13:27 2018

simplify VaapiPictureFactory::Create() method and VaapiPicture members

This CL moves part of the logic of VaapiPicture creation from VaVDA to
VaapiPictureFactory, passing the PictureBuffer wholesale since we use
most of it anyway.  The net result is less lines of code.

VaapiPicture and derived classes hold on to a |texture_id_| and a
|client_texture_id_|, the latter only used with |bind_image_cb_|:
- |texture_id_| cannot be zero, either in the tests or in reality, because
  is a GL texture id; this CL enforces this via a DCHECK() in ctor. That
  simplifies the body a few methods.
- |client_texture_id_| can be zero, and is always zero in the v_d_a_unittests,
  but it doesn't matter because |bind_image_cb_| is dummy for tests ([1]),
  so there's no point of enforcing it to be non zero.  Moreover, since
  it's a client-side texture id, it can be any number, so let's skip
  the confusing checks.

Sprinkled a couple of consts and also, there's no need to do
!callback.is_null() because CallbackBase provides bool(), so
s/!callback.is_null()/callback/

[1] https://cs.chromium.org/chromium/src/media/gpu/video_decode_accelerator_unittest.cc?sq=package:chromium&dr&g=0&l=452

Bug: 822346
Test: compiled+run simplechrome, v_d_a_unittests on eve. media_unittests.
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I5610ea4abf87df26a85e909aebfbbaa6bb964163
Reviewed-on: https://chromium-review.googlesource.com/1079834
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564178}
[modify] https://crrev.com/37a2e00b9c5cc8f9f60fb690fecc22a2a09e8fd2/media/gpu/vaapi/vaapi_picture.h
[modify] https://crrev.com/37a2e00b9c5cc8f9f60fb690fecc22a2a09e8fd2/media/gpu/vaapi/vaapi_picture_factory.cc
[modify] https://crrev.com/37a2e00b9c5cc8f9f60fb690fecc22a2a09e8fd2/media/gpu/vaapi/vaapi_picture_factory.h
[modify] https://crrev.com/37a2e00b9c5cc8f9f60fb690fecc22a2a09e8fd2/media/gpu/vaapi/vaapi_picture_native_pixmap_egl.cc
[modify] https://crrev.com/37a2e00b9c5cc8f9f60fb690fecc22a2a09e8fd2/media/gpu/vaapi/vaapi_picture_native_pixmap_ozone.cc
[modify] https://crrev.com/37a2e00b9c5cc8f9f60fb690fecc22a2a09e8fd2/media/gpu/vaapi/vaapi_picture_tfp.cc
[modify] https://crrev.com/37a2e00b9c5cc8f9f60fb690fecc22a2a09e8fd2/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/37a2e00b9c5cc8f9f60fb690fecc22a2a09e8fd2/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc
[modify] https://crrev.com/37a2e00b9c5cc8f9f60fb690fecc22a2a09e8fd2/media/gpu/video_decode_accelerator_unittest.cc

Project Member

Comment 26 by bugdroid1@chromium.org, Jun 11 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b0024ae93bd8c9b92a01d612557588622a030a7c

commit b0024ae93bd8c9b92a01d612557588622a030a7c
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Mon Jun 11 03:44:35 2018

Revert "simplify VaapiPictureFactory::Create() method and VaapiPicture members"

This reverts commit 37a2e00b9c5cc8f9f60fb690fecc22a2a09e8fd2.

Reason for revert: This CL breaks ARC++ video playback.

Original change's description:
> simplify VaapiPictureFactory::Create() method and VaapiPicture members
>
> This CL moves part of the logic of VaapiPicture creation from VaVDA to
> VaapiPictureFactory, passing the PictureBuffer wholesale since we use
> most of it anyway.  The net result is less lines of code.
>
> VaapiPicture and derived classes hold on to a |texture_id_| and a
> |client_texture_id_|, the latter only used with |bind_image_cb_|:
> - |texture_id_| cannot be zero, either in the tests or in reality, because
>   is a GL texture id; this CL enforces this via a DCHECK() in ctor. That
>   simplifies the body a few methods.
> - |client_texture_id_| can be zero, and is always zero in the v_d_a_unittests,
>   but it doesn't matter because |bind_image_cb_| is dummy for tests ([1]),
>   so there's no point of enforcing it to be non zero.  Moreover, since
>   it's a client-side texture id, it can be any number, so let's skip
>   the confusing checks.
>
> Sprinkled a couple of consts and also, there's no need to do
> !callback.is_null() because CallbackBase provides bool(), so
> s/!callback.is_null()/callback/
>
> [1] https://cs.chromium.org/chromium/src/media/gpu/video_decode_accelerator_unittest.cc?sq=package:chromium&dr&g=0&l=452
>
> Bug: 822346
> Test: compiled+run simplechrome, v_d_a_unittests on eve. media_unittests.
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
> Change-Id: I5610ea4abf87df26a85e909aebfbbaa6bb964163
> Reviewed-on: https://chromium-review.googlesource.com/1079834
> Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
> Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>
> Commit-Queue: Miguel Casas <mcasas@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#564178}

TBR=mcasas@chromium.org,dcastagna@chromium.org,hoegsberg@chromium.org,andrescj@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 822346, b:109906289
Change-Id: I58b4968468d99c3f0e4e6b56c1ecd89645550f73
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/1094814
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565922}
[modify] https://crrev.com/b0024ae93bd8c9b92a01d612557588622a030a7c/media/gpu/vaapi/vaapi_picture.h
[modify] https://crrev.com/b0024ae93bd8c9b92a01d612557588622a030a7c/media/gpu/vaapi/vaapi_picture_factory.cc
[modify] https://crrev.com/b0024ae93bd8c9b92a01d612557588622a030a7c/media/gpu/vaapi/vaapi_picture_factory.h
[modify] https://crrev.com/b0024ae93bd8c9b92a01d612557588622a030a7c/media/gpu/vaapi/vaapi_picture_native_pixmap_egl.cc
[modify] https://crrev.com/b0024ae93bd8c9b92a01d612557588622a030a7c/media/gpu/vaapi/vaapi_picture_native_pixmap_ozone.cc
[modify] https://crrev.com/b0024ae93bd8c9b92a01d612557588622a030a7c/media/gpu/vaapi/vaapi_picture_tfp.cc
[modify] https://crrev.com/b0024ae93bd8c9b92a01d612557588622a030a7c/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/b0024ae93bd8c9b92a01d612557588622a030a7c/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc
[modify] https://crrev.com/b0024ae93bd8c9b92a01d612557588622a030a7c/media/gpu/video_decode_accelerator_unittest.cc

Comment 27 by hiroh@chromium.org, Jun 11 2018

Cc: hiroh@chromium.org
Project Member

Comment 28 by bugdroid1@chromium.org, Jun 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b70d8c27e5fbd132355afde2d6bb29bc6ef41614

commit b70d8c27e5fbd132355afde2d6bb29bc6ef41614
Author: Miguel Casas-Sanchez <mcasas@chromium.org>
Date: Thu Jun 14 16:18:01 2018

Reland: simplify VaapiPictureFactory::Create() method and VaapiPicture members

This CL is a reland of crrev.com/c/1079834 and crrev.com/c/1086131
that got reverted (the second is just adding a few forgotten DCHECKs).
This is because  GpuArcVideoDecodeAccelerator sends empty client and
texture id vectors [0]; this CL handles gracefully these cases.

[0] https://cs.chromium.org/chromium/src/components/arc/video_accelerator/gpu_arc_video_decode_accelerator.cc?l=468

original cl description ------------------------------------------------

This CL moves part of the logic of VaapiPicture creation from VaVDA to
VaapiPictureFactory, passing the PictureBuffer wholesale since we use
most of it anyway.  The net result is less lines of code.

VaapiPicture and derived classes hold on to a |texture_id_| and a
|client_texture_id_|, the latter only used with |bind_image_cb_|:
- |texture_id_| cannot be zero, either in the tests or in reality, because
  is a GL texture id; this CL enforces this via a DCHECK() in ctor. That
  simplifies the body a few methods.
- |client_texture_id_| can be zero, and is always zero in the v_d_a_unittests,
  but it doesn't matter because |bind_image_cb_| is dummy for tests ([1]),
  so there's no point of enforcing it to be non zero.  Moreover, since
  it's a client-side texture id, it can be any number, so let's skip
  the confusing checks.

Sprinkled a couple of consts and also, there's no need to do
!callback.is_null() because CallbackBase provides bool(), so
s/!callback.is_null()/callback/

[1] https://cs.chromium.org/chromium/src/media/gpu/video_decode_accelerator_unittest.cc?sq=package:chromium&dr&g=0&l=452

Bug: 822346
Test: compiled+run simplechrome, v_d_a_unittests on eve. media_unittests.
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I8b211d97e6bcb3f86149224a1c130ca6a24984e8
Reviewed-on: https://chromium-review.googlesource.com/1100535
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567293}
[modify] https://crrev.com/b70d8c27e5fbd132355afde2d6bb29bc6ef41614/media/gpu/vaapi/vaapi_picture.h
[modify] https://crrev.com/b70d8c27e5fbd132355afde2d6bb29bc6ef41614/media/gpu/vaapi/vaapi_picture_factory.cc
[modify] https://crrev.com/b70d8c27e5fbd132355afde2d6bb29bc6ef41614/media/gpu/vaapi/vaapi_picture_factory.h
[modify] https://crrev.com/b70d8c27e5fbd132355afde2d6bb29bc6ef41614/media/gpu/vaapi/vaapi_picture_native_pixmap_egl.cc
[modify] https://crrev.com/b70d8c27e5fbd132355afde2d6bb29bc6ef41614/media/gpu/vaapi/vaapi_picture_native_pixmap_ozone.cc
[modify] https://crrev.com/b70d8c27e5fbd132355afde2d6bb29bc6ef41614/media/gpu/vaapi/vaapi_picture_tfp.cc
[modify] https://crrev.com/b70d8c27e5fbd132355afde2d6bb29bc6ef41614/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/b70d8c27e5fbd132355afde2d6bb29bc6ef41614/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc
[modify] https://crrev.com/b70d8c27e5fbd132355afde2d6bb29bc6ef41614/media/gpu/video_decode_accelerator_unittest.cc

Project Member

Comment 29 by bugdroid1@chromium.org, Jun 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/398ca322d30e0be0ccecebb9fbb9a03a87fc6766

commit 398ca322d30e0be0ccecebb9fbb9a03a87fc6766
Author: Miguel Casas <mcasas@chromium.org>
Date: Mon Jun 18 16:25:14 2018

Ozone DRM: use GBM_BO_USE_HW_VIDEO_DECODER

This CL adds the GBM_BO_USE_HW_VIDEO_DECODER flag to
gfx::BufferUsage::SCANOUT_VDA_WRITE buffers. It's a spinoff of a
comment [1] in crrev.com/c/1021675.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1021675/18/ui/ozone/platform/drm/gpu/drm_thread.cc#147

Bug: 822346
Test: simplechrome video playback on nautilus, vaapi engaged.
Change-Id: I795c6c3f74574ffab60c24d1ef746b22d214120a
Reviewed-on: https://chromium-review.googlesource.com/1104394
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568027}
[modify] https://crrev.com/398ca322d30e0be0ccecebb9fbb9a03a87fc6766/ui/ozone/platform/drm/gpu/drm_thread.cc

Project Member

Comment 30 by bugdroid1@chromium.org, Jun 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bd4386f594d74f13353e6d35b59e2ef9c80e361b

commit bd4386f594d74f13353e6d35b59e2ef9c80e361b
Author: Miguel Casas <mcasas@chromium.org>
Date: Wed Jun 20 02:43:16 2018

VaapiPictureFactory: correct DCHECK

crrev.com/c/1100535 landed a DCHECK that does not verify from ARC++,


[1814:1814:0619/160631.868318:FATAL:vaapi_picture_factory.cc(48)] Check failed: picture_buffer.texture_target() == GetGLTextureTarget() (0 vs. 36197)
#0 0x59c50521b4dc base::debug::StackTrace::StackTrace()
#1 0x59c50518f620 logging::LogMessage::~LogMessage()
#2 0x59c5026cc721 media::VaapiPictureFactory::Create()
#3 0x59c5026d2a94 media::VaapiVideoDecodeAccelerator::AssignPictureBuffers()
#4 0x59c506967623 arc::GpuArcVideoDecodeAccelerator::AssignPictureBuffers()
#5 0x59c50269c433 arc::mojom::VideoDecodeAcceleratorStubDispatch::Accept()
...

This CL corrects it.

Bug: 822346
Test: YT App playing back correctly with dcheck_always_on=true on nautilus.
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: Icff42847518df24dcc38998acdc2903e7928469f
Reviewed-on: https://chromium-review.googlesource.com/1106670
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568703}
[modify] https://crrev.com/bd4386f594d74f13353e6d35b59e2ef9c80e361b/media/gpu/vaapi/vaapi_picture_factory.cc

Project Member

Comment 31 by bugdroid1@chromium.org, Jun 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c1ee562516e98cb7066bca4614f94ea7beec06aa

commit c1ee562516e98cb7066bca4614f94ea7beec06aa
Author: Miguel Casas <mcasas@chromium.org>
Date: Wed Jun 20 16:53:02 2018

vaapi: decode on client NativePixmaps

This CL teaches VaVDA to use client VASurfaceIDs to decode onto, saving
a buffer copy and removing a costly blit (DownloadFromSurface) on the
GPU Main thread.  Three groups of changes:

1. In AssignPictureBuffers(): if |vaapi_picture_factory_| Create()s a
 VaapiPicture with a VASurfaceID, we use those to decode onto and set a new
 flag: |decode_using_client_picture_buffers_|

2. When the decoder calls CreateVASurface(), instead of giving it the first
 |available_va_surfaces_|, we need to figure out the first VASurfaceID in
 this |available_va_surfaces_| such that the corresponding VaapiPicture in
 |pictures_| is available (i.e. in |available_picture_buffers_|).
 Reason: libva holds on to some VASurfaceIDs, there's no simple one-to-one
 correspondence like on ToT.

3. When we're ready to OutputPicture() to |client_|, instead of using the first
 |available_picture_buffers_|, we find the corresponding one for the
 passed |va_surface_id| (we could search all over |pictures_| but there's
 always less |available_picture_buffers_|.

Some other notable developments:

- s/output_buffers_/available_picture_buffers_/
- OutputPicture() loses a parameter.
- |decode_using_client_picture_buffers_| is still false for e.g. VP9 Profile 2
 or the ImportBufferForPicture() path.
- Pictures is made a base::small_map. (Originally from crrev.com/c/988512).

Bug: 822346, 717265
Test: v_d_a_unittest vp8/vp9/h264 passing on eve, running crosvideo changing
resolutions for a long time. No hickups, no dropped frames.

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I3d5a4d3c83002c8d0977702c9b97ffec52b1db23
Reviewed-on: https://chromium-review.googlesource.com/1021675
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568880}
[modify] https://crrev.com/c1ee562516e98cb7066bca4614f94ea7beec06aa/media/gpu/vaapi/vaapi_picture.cc
[modify] https://crrev.com/c1ee562516e98cb7066bca4614f94ea7beec06aa/media/gpu/vaapi/vaapi_picture.h
[modify] https://crrev.com/c1ee562516e98cb7066bca4614f94ea7beec06aa/media/gpu/vaapi/vaapi_picture_native_pixmap.cc
[modify] https://crrev.com/c1ee562516e98cb7066bca4614f94ea7beec06aa/media/gpu/vaapi/vaapi_picture_native_pixmap.h
[modify] https://crrev.com/c1ee562516e98cb7066bca4614f94ea7beec06aa/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/c1ee562516e98cb7066bca4614f94ea7beec06aa/media/gpu/vaapi/vaapi_video_decode_accelerator.h
[modify] https://crrev.com/c1ee562516e98cb7066bca4614f94ea7beec06aa/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc

Comment 32 by hiroh@chromium.org, Jun 27 2018

Protected content playback on ARC++ at ARM platform doesn't work due to
https://chromium-review.googlesource.com/c/chromium/src/+/1104394.

In protected content playback case on ARC++, the output buffers are allocated at Chromium, not inside Android container.
https://cs.chromium.org/chromium/src/components/arc/video_accelerator/protected_buffer_manager.cc?sq=package:chromium&dr&g=0&l=167

The allocating failure because GBM_BO_USE_HW_VIDEO_DECODER is not supported at minigbm on non i915 platform.


You can test protected content playback at ARC++ by using ExoPlayerDemo app and playing "WV: Secure SD(MP4, H264)".
It can be installed by "adb install vendor/widevine/libwvdrmengine/test/demo/ExoPlayerDemo.apk" in ARC++N directory.

Project Member

Comment 33 by bugdroid1@chromium.org, Jun 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1ac57fb9e8c76796d8fa0e6d8a1a7b8f30e24c4d

commit 1ac57fb9e8c76796d8fa0e6d8a1a7b8f30e24c4d
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Wed Jun 27 10:16:20 2018

Revert "Ozone DRM: use GBM_BO_USE_HW_VIDEO_DECODER"

This reverts commit 398ca322d30e0be0ccecebb9fbb9a03a87fc6766.

Reason for revert: Breaks protected content playback in ARM platform.

Original change's description:
> Ozone DRM: use GBM_BO_USE_HW_VIDEO_DECODER
> 
> This CL adds the GBM_BO_USE_HW_VIDEO_DECODER flag to
> gfx::BufferUsage::SCANOUT_VDA_WRITE buffers. It's a spinoff of a
> comment [1] in crrev.com/c/1021675.
> 
> [1] https://chromium-review.googlesource.com/c/chromium/src/+/1021675/18/ui/ozone/platform/drm/gpu/drm_thread.cc#147
> 
> Bug: 822346
> Test: simplechrome video playback on nautilus, vaapi engaged.
> Change-Id: I795c6c3f74574ffab60c24d1ef746b22d214120a
> Reviewed-on: https://chromium-review.googlesource.com/1104394
> Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
> Commit-Queue: Miguel Casas <mcasas@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#568027}

TBR=mcasas@chromium.org,dcastagna@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 822346
Change-Id: I8b9af48859886f0ef9c203a06079dadd53f0e477
Reviewed-on: https://chromium-review.googlesource.com/1116738
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570720}
[modify] https://crrev.com/1ac57fb9e8c76796d8fa0e6d8a1a7b8f30e24c4d/ui/ozone/platform/drm/gpu/drm_thread.cc

Project Member

Comment 34 by bugdroid1@chromium.org, Jun 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9921f96141b3172c6a38dd1f42d4acfb0507ae96

commit 9921f96141b3172c6a38dd1f42d4acfb0507ae96
Author: Miguel Casas <mcasas@chromium.org>
Date: Wed Jun 27 13:56:44 2018

Revert "vaapi: decode on client NativePixmaps"

This reverts commit c1ee562516e98cb7066bca4614f94ea7beec06aa.

Reason for revert: The necessary predecessor crrev.com/c/1104394
has been reverted (for other reasons, see crbug.com/822346#c33)

Original change's description:
> vaapi: decode on client NativePixmaps
> 
> This CL teaches VaVDA to use client VASurfaceIDs to decode onto, saving
> a buffer copy and removing a costly blit (DownloadFromSurface) on the
> GPU Main thread.  Three groups of changes:
> 
> 1. In AssignPictureBuffers(): if |vaapi_picture_factory_| Create()s a
>  VaapiPicture with a VASurfaceID, we use those to decode onto and set a new
>  flag: |decode_using_client_picture_buffers_|
> 
> 2. When the decoder calls CreateVASurface(), instead of giving it the first
>  |available_va_surfaces_|, we need to figure out the first VASurfaceID in
>  this |available_va_surfaces_| such that the corresponding VaapiPicture in
>  |pictures_| is available (i.e. in |available_picture_buffers_|).
>  Reason: libva holds on to some VASurfaceIDs, there's no simple one-to-one
>  correspondence like on ToT.
> 
> 3. When we're ready to OutputPicture() to |client_|, instead of using the first
>  |available_picture_buffers_|, we find the corresponding one for the
>  passed |va_surface_id| (we could search all over |pictures_| but there's
>  always less |available_picture_buffers_|.
> 
> Some other notable developments:
> 
> - s/output_buffers_/available_picture_buffers_/
> - OutputPicture() loses a parameter.
> - |decode_using_client_picture_buffers_| is still false for e.g. VP9 Profile 2
>  or the ImportBufferForPicture() path.
> - Pictures is made a base::small_map. (Originally from crrev.com/c/988512).
> 
> Bug: 822346, 717265
> Test: v_d_a_unittest vp8/vp9/h264 passing on eve, running crosvideo changing
> resolutions for a long time. No hickups, no dropped frames.
> 
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
> Change-Id: I3d5a4d3c83002c8d0977702c9b97ffec52b1db23
> Reviewed-on: https://chromium-review.googlesource.com/1021675
> Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
> Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>
> Commit-Queue: Miguel Casas <mcasas@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#568880}

TBR=mcasas@chromium.org,dcastagna@chromium.org,hoegsberg@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 822346, 717265
Change-Id: I5e8cb9431f72a65f67782e59cb5ce52f7e1d4ce7
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/1116938
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570750}
[modify] https://crrev.com/9921f96141b3172c6a38dd1f42d4acfb0507ae96/media/gpu/vaapi/vaapi_picture.cc
[modify] https://crrev.com/9921f96141b3172c6a38dd1f42d4acfb0507ae96/media/gpu/vaapi/vaapi_picture.h
[modify] https://crrev.com/9921f96141b3172c6a38dd1f42d4acfb0507ae96/media/gpu/vaapi/vaapi_picture_native_pixmap.cc
[modify] https://crrev.com/9921f96141b3172c6a38dd1f42d4acfb0507ae96/media/gpu/vaapi/vaapi_picture_native_pixmap.h
[modify] https://crrev.com/9921f96141b3172c6a38dd1f42d4acfb0507ae96/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/9921f96141b3172c6a38dd1f42d4acfb0507ae96/media/gpu/vaapi/vaapi_video_decode_accelerator.h
[modify] https://crrev.com/9921f96141b3172c6a38dd1f42d4acfb0507ae96/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc

Blockedon: 857095
Project Member

Comment 36 by bugdroid1@chromium.org, Jul 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1cb7337ead74107f32239e0d8386ada3355ce9c1

commit 1cb7337ead74107f32239e0d8386ada3355ce9c1
Author: Miguel Casas <mcasas@chromium.org>
Date: Tue Jul 03 00:31:48 2018

Reland: Ozone DRM: use GBM_BO_USE_HW_VIDEO_DECODER

The original CL was reverted because BO_USE_HW_VIDEO_DECODER is not
correctly handled in ARM platforms _for protected buffers_ (i.e.
allocations coming from ARC++). crrev.com/c/1117606 added support
for the flag on those platforms.

CQ-DEPEND:1117025
TBR=dcastagna@chromium.org (no changes)

Original CL -----------------------------------------------------------
This CL adds the GBM_BO_USE_HW_VIDEO_DECODER flag to
gfx::BufferUsage::SCANOUT_VDA_WRITE buffers. It's a spinoff of a
comment [1] in crrev.com/c/1021675.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1021675/18/ui/ozone/platform/drm/gpu/drm_thread.cc#147

Bug: 822346,  857095 
Test: simplechrome video playback on nautilus, vaapi engaged.
Change-Id: I1c11a8fffa662b69049d121ccb48ebfd85097228
Reviewed-on: https://chromium-review.googlesource.com/1104394
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1117025
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572053}
[modify] https://crrev.com/1cb7337ead74107f32239e0d8386ada3355ce9c1/ui/ozone/platform/drm/gpu/drm_thread.cc

Project Member

Comment 37 by bugdroid1@chromium.org, Jul 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ae29b15515cf03a9b53086a141b8a070b03f6ae5

commit ae29b15515cf03a9b53086a141b8a070b03f6ae5
Author: Miguel Casas <mcasas@chromium.org>
Date: Mon Jul 23 20:02:11 2018

RELAND: vaapi: decode on client NativePixmaps

This CL relands crrev.com/c/1021675 that got reverted as predecessor
to crrev.com/c/1104394 (but was otherwise innocent AFAIK). The said
predecessor situation has been corrected by crrev.com/c/1117606 (that
landed in ChromeOS 10840.0.0 [1])

[1] https://crosland.corp.google.com/log/10837.0.0..10840.0.0

TBR=dcastagna@chromium.org (same code as reviewed before)

Original CL description -----------------------------------------------
vaapi: decode on client NativePixmaps

This CL teaches VaVDA to use client VASurfaceIDs to decode onto, saving
a buffer copy and removing a costly blit (DownloadFromSurface) on the
GPU Main thread.  Three groups of changes:

1. In AssignPictureBuffers(): if |vaapi_picture_factory_| Create()s a
 VaapiPicture with a VASurfaceID, we use those to decode onto and set a new
 flag: |decode_using_client_picture_buffers_|

2. When the decoder calls CreateVASurface(), instead of giving it the first
 |available_va_surfaces_|, we need to figure out the first VASurfaceID in
 this |available_va_surfaces_| such that the corresponding VaapiPicture in
 |pictures_| is available (i.e. in |available_picture_buffers_|).
 Reason: libva holds on to some VASurfaceIDs, there's no simple one-to-one
 correspondence like on ToT.

3. When we're ready to OutputPicture() to |client_|, instead of using the first
 |available_picture_buffers_|, we find the corresponding one for the
 passed |va_surface_id| (we could search all over |pictures_| but there's
 always less |available_picture_buffers_|.

Some other notable developments:

- s/output_buffers_/available_picture_buffers_/
- OutputPicture() loses a parameter.
- |decode_using_client_picture_buffers_| is still false for e.g. VP9 Profile 2
 or the ImportBufferForPicture() path.
- Pictures is made a base::small_map. (Originally from crrev.com/c/988512).

resolutions for a long time. No hickups, no dropped frames.

Bug: 822346, 717265
Test: v_d_a_unittest vp8/vp9/h264 passing on eve, running crosvideo changing
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: Ie1fb21a7a302476bceee3dd905278d53f13b1475
Reviewed-on: https://chromium-review.googlesource.com/1021675
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1147143
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577226}
[modify] https://crrev.com/ae29b15515cf03a9b53086a141b8a070b03f6ae5/media/gpu/vaapi/vaapi_picture.cc
[modify] https://crrev.com/ae29b15515cf03a9b53086a141b8a070b03f6ae5/media/gpu/vaapi/vaapi_picture.h
[modify] https://crrev.com/ae29b15515cf03a9b53086a141b8a070b03f6ae5/media/gpu/vaapi/vaapi_picture_native_pixmap.cc
[modify] https://crrev.com/ae29b15515cf03a9b53086a141b8a070b03f6ae5/media/gpu/vaapi/vaapi_picture_native_pixmap.h
[modify] https://crrev.com/ae29b15515cf03a9b53086a141b8a070b03f6ae5/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/ae29b15515cf03a9b53086a141b8a070b03f6ae5/media/gpu/vaapi/vaapi_video_decode_accelerator.h
[modify] https://crrev.com/ae29b15515cf03a9b53086a141b8a070b03f6ae5/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc

Project Member

Comment 38 by bugdroid1@chromium.org, Aug 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/148b331759e9e346d6f214d63805832a1ee6232c

commit 148b331759e9e346d6f214d63805832a1ee6232c
Author: Pawel Osciak <posciak@chromium.org>
Date: Fri Aug 03 16:26:12 2018

Revert "RELAND: vaapi: decode on client NativePixmaps"

This reverts commit ae29b15515cf03a9b53086a141b8a070b03f6ae5.

Reason for revert: crbug.com/868190.

Original change's description:
> RELAND: vaapi: decode on client NativePixmaps
> 
> This CL relands crrev.com/c/1021675 that got reverted as predecessor
> to crrev.com/c/1104394 (but was otherwise innocent AFAIK). The said
> predecessor situation has been corrected by crrev.com/c/1117606 (that
> landed in ChromeOS 10840.0.0 [1])
> 
> [1] https://crosland.corp.google.com/log/10837.0.0..10840.0.0
> 
> TBR=dcastagna@chromium.org (same code as reviewed before)
> 
> Original CL description -----------------------------------------------
> vaapi: decode on client NativePixmaps
> 
> This CL teaches VaVDA to use client VASurfaceIDs to decode onto, saving
> a buffer copy and removing a costly blit (DownloadFromSurface) on the
> GPU Main thread.  Three groups of changes:
> 
> 1. In AssignPictureBuffers(): if |vaapi_picture_factory_| Create()s a
>  VaapiPicture with a VASurfaceID, we use those to decode onto and set a new
>  flag: |decode_using_client_picture_buffers_|
> 
> 2. When the decoder calls CreateVASurface(), instead of giving it the first
>  |available_va_surfaces_|, we need to figure out the first VASurfaceID in
>  this |available_va_surfaces_| such that the corresponding VaapiPicture in
>  |pictures_| is available (i.e. in |available_picture_buffers_|).
>  Reason: libva holds on to some VASurfaceIDs, there's no simple one-to-one
>  correspondence like on ToT.
> 
> 3. When we're ready to OutputPicture() to |client_|, instead of using the first
>  |available_picture_buffers_|, we find the corresponding one for the
>  passed |va_surface_id| (we could search all over |pictures_| but there's
>  always less |available_picture_buffers_|.
> 
> Some other notable developments:
> 
> - s/output_buffers_/available_picture_buffers_/
> - OutputPicture() loses a parameter.
> - |decode_using_client_picture_buffers_| is still false for e.g. VP9 Profile 2
>  or the ImportBufferForPicture() path.
> - Pictures is made a base::small_map. (Originally from crrev.com/c/988512).
> 
> resolutions for a long time. No hickups, no dropped frames.
> 
> Bug: 822346, 717265
> Test: v_d_a_unittest vp8/vp9/h264 passing on eve, running crosvideo changing
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
> Change-Id: Ie1fb21a7a302476bceee3dd905278d53f13b1475
> Reviewed-on: https://chromium-review.googlesource.com/1021675
> Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
> Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>
> Reviewed-on: https://chromium-review.googlesource.com/1147143
> Commit-Queue: Miguel Casas <mcasas@chromium.org>
> Reviewed-by: Miguel Casas <mcasas@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#577226}

TBR=mcasas@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 822346, 717265
Change-Id: I8b91c852d94061771fc0ff8caea6616c74f9454a
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/1161621
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580567}
[modify] https://crrev.com/148b331759e9e346d6f214d63805832a1ee6232c/media/gpu/vaapi/vaapi_picture.cc
[modify] https://crrev.com/148b331759e9e346d6f214d63805832a1ee6232c/media/gpu/vaapi/vaapi_picture.h
[modify] https://crrev.com/148b331759e9e346d6f214d63805832a1ee6232c/media/gpu/vaapi/vaapi_picture_native_pixmap.cc
[modify] https://crrev.com/148b331759e9e346d6f214d63805832a1ee6232c/media/gpu/vaapi/vaapi_picture_native_pixmap.h
[modify] https://crrev.com/148b331759e9e346d6f214d63805832a1ee6232c/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/148b331759e9e346d6f214d63805832a1ee6232c/media/gpu/vaapi/vaapi_video_decode_accelerator.h
[modify] https://crrev.com/148b331759e9e346d6f214d63805832a1ee6232c/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc

Project Member

Comment 39 by bugdroid1@chromium.org, Aug 8

Labels: merge-merged-release-R68-10718.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/minigbm/+/b207c74b02488d1fe6610f594fb2206d40c110e4

commit b207c74b02488d1fe6610f594fb2206d40c110e4
Author: Kristian H. Kristensen <hoegsberg@chromium.org>
Date: Wed Aug 08 00:16:34 2018

Reland "i915: Add GBM_BO_USE_HW_VIDEO_DECODER use flag"

This is a reland of 1bd7b04a3ae68c0314bdee06c559093de9e5a304

We've landed CL:1070995 which fixes the missing modifier that
caused tiling corruption on pre-KBL Intel devices and CL:1072638
which makes the failing vda unittests use SCANOUT_VDA_WRITE as
they were supposed to.

Original change's description:
> i915: Add GBM_BO_USE_HW_VIDEO_DECODER use flag
>
> This flag is used to indicate that the platform video decoder will be
> writing into this buffer and that it should be allocated
> accordingly. On Intel, this means that we have to allocate y-tiled
> NV12 for libva to be able to decode to the buffer.
>
> We force gralloc NV12 allocations to be linear for now, since ARC++
> doesn't properly pass modifiers to ChromeOS.
>
> BUG=822346
> TEST=test_that graphics_Gbm
>
> Change-Id: I840c30d22355d26816df718b49717407e2e4620f
> Reviewed-on: https://chromium-review.googlesource.com/996648
> Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
> Tested-by: Kristian H. Kristensen <hoegsberg@chromium.org>
> Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>

BUG= chromium:822346
BUG= b:111662470
Change-Id: Icf0921dc91ac422da26371bffea34b26550b8234
Reviewed-on: https://chromium-review.googlesource.com/1073877
Commit-Ready: Kristian H. Kristensen <hoegsberg@chromium.org>
Tested-by: Kristian H. Kristensen <hoegsberg@chromium.org>
Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>
(cherry picked from commit 3cb5bbacc5c8a79105c868875222696f6b9d8296)
Reviewed-on: https://chromium-review.googlesource.com/1166268
Reviewed-by: Gurchetan Singh <gurchetansingh@chromium.org>
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Tested-by: Hirokazu Honda <hiroh@chromium.org>
Trybot-Ready: Hirokazu Honda <hiroh@chromium.org>

[modify] https://crrev.com/b207c74b02488d1fe6610f594fb2206d40c110e4/i915.c
[modify] https://crrev.com/b207c74b02488d1fe6610f594fb2206d40c110e4/drv.h
[modify] https://crrev.com/b207c74b02488d1fe6610f594fb2206d40c110e4/cros_gralloc/cros_gralloc_driver.cc
[modify] https://crrev.com/b207c74b02488d1fe6610f594fb2206d40c110e4/gbm_helpers.c
[modify] https://crrev.com/b207c74b02488d1fe6610f594fb2206d40c110e4/gbm.h

Project Member

Comment 40 by bugdroid1@chromium.org, Aug 18

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6727aea8c39de749494e520bc955416e0a2d0e77

commit 6727aea8c39de749494e520bc955416e0a2d0e77
Author: Miguel Casas <mcasas@chromium.org>
Date: Sat Aug 18 00:23:23 2018

VaVDA: preparation for decode on client NativePixmaps

This CL extracts a few cleanups/preparations of crrev.com/c/1171566
(that is a big CL and might get reverted a few times).

- VaapiPicture::va_surface_id() allows for accessing the underlying
 VASurfaceID, if this is present (otherwise we return VA_INVALID_ID).

- Pictures is made a base::small_map (originally crrev.com/c/988512),
 PictureById() is removed in favour of using base::ContainsKey()
 on 3 callsites.

Bug: 822346, 717265
Test: v_d_a_unittest vp8/vp9/h264 passing on nautilus
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I18a0335462b321d25550d295cb628b4ddd3e2269
Reviewed-on: https://chromium-review.googlesource.com/1180295
Reviewed-by: Emircan Uysaler <emircan@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584256}
[modify] https://crrev.com/6727aea8c39de749494e520bc955416e0a2d0e77/media/gpu/vaapi/vaapi_picture.cc
[modify] https://crrev.com/6727aea8c39de749494e520bc955416e0a2d0e77/media/gpu/vaapi/vaapi_picture.h
[modify] https://crrev.com/6727aea8c39de749494e520bc955416e0a2d0e77/media/gpu/vaapi/vaapi_picture_native_pixmap.cc
[modify] https://crrev.com/6727aea8c39de749494e520bc955416e0a2d0e77/media/gpu/vaapi/vaapi_picture_native_pixmap.h
[modify] https://crrev.com/6727aea8c39de749494e520bc955416e0a2d0e77/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/6727aea8c39de749494e520bc955416e0a2d0e77/media/gpu/vaapi/vaapi_video_decode_accelerator.h

Project Member

Comment 41 by bugdroid1@chromium.org, Aug 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/49dc6464052f4cc335ffb65895244836dec30673

commit 49dc6464052f4cc335ffb65895244836dec30673
Author: Miguel Casas <mcasas@chromium.org>
Date: Mon Aug 20 20:31:41 2018

vaapi: decode on client NativePixmaps (reduced)

crrev.com/c/1171566 got reverted due to a mishmash of performance
improvements and regressions.  Instead of relanding it verbatim, the
innocent/innocuous parts were landed in crrev.com/c/1180295; this CL
gets the tough parts, including restricting it to VP9 and Kabylakes.

I also repeated the power consumption results (https://goo.gl/r88qPf),
and in a nutshell I see improvements:
w/ this CL:  1.88   0.63   0.41  0.52  (pkg - pp1 - gfx - dram)
ToT       :  2.72   0.76   1.03  0.57
Savings   : 30.78% 17.51% 59.95% 7.60%

TBR=dcastagna@, hoegsberg@

Original CL description -----------------------------------------------
vaapi: decode on client NativePixmaps

This CL teaches VaVDA to use client VASurfaceIDs to decode onto, saving
a buffer copy and removing a costly blit (DownloadFromSurface) on the
GPU Main thread.  Three groups of changes:

1. In AssignPictureBuffers(): if |vaapi_picture_factory_| Create()s a
 VaapiPicture with a VASurfaceID, we use those to decode onto and set a new
 flag: |decode_using_client_picture_buffers_|

2. When the decoder calls CreateVASurface(), instead of giving it the first
 |available_va_surfaces_|, we need to figure out the first VASurfaceID in
 this |available_va_surfaces_| such that the corresponding VaapiPicture in
 |pictures_| is available (i.e. in |available_picture_buffers_|).
 Reason: libva holds on to some VASurfaceIDs, there's no simple one-to-one
 correspondence like on ToT.

3. When we're ready to OutputPicture() to |client_|, instead of using the first
 |available_picture_buffers_|, we find the corresponding one for the
 passed |va_surface_id| (we could search all over |pictures_| but there's
 always less |available_picture_buffers_|.

Some other notable developments:

- s/output_buffers_/available_picture_buffers_/
- OutputPicture() loses a parameter.
- |decode_using_client_picture_buffers_| is still false for e.g. VP9 Profile 2
 or the ImportBufferForPicture() path.

Bug: 822346, 717265

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: Icf1a9a1fa71264a7367fa351e3b7d28fab346744
Reviewed-on: https://chromium-review.googlesource.com/1181464
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584553}
[modify] https://crrev.com/49dc6464052f4cc335ffb65895244836dec30673/media/gpu/vaapi/vaapi_video_decode_accelerator.cc
[modify] https://crrev.com/49dc6464052f4cc335ffb65895244836dec30673/media/gpu/vaapi/vaapi_video_decode_accelerator.h
[modify] https://crrev.com/49dc6464052f4cc335ffb65895244836dec30673/media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc

#41 landed in 70.0.3529.0 and there doesn't seem to have been any regression
around that point (there's one [1] but it's posterior, bw 3232 and 3238).

[1] https://chromeperf.appspot.com/report?sid=467558c6d328b9cec5433dbeb1e7b2b71c5a387c4c4287c712f7a8c78ca41ec2&start_rev=35190031097200000&end_rev=35440001105100000


Cc: sreerenj...@intel.corp-partner.google.com
Cc: -sreerenj...@intel.corp-partner.google.com -zelidrag@chromium.org sreerenj...@intel.com
I did some testing on Soraka with 720p h264 and vp8 clips. Both showing better performance in terms of power when avoiding explicit vpp usage.

For eg: H264 Decode: Power measured using "dump_intel_rapl_consumption --interval_ms=2000 --repeat --verbose" : Took an average of 10 measurements from a stable state sample space.

Case1: DECODE USING CLIENT BUF = 0 (With VPP)
(1.371556+1.371666+1.402654+1.537448+1.383362+1.373745+1.375320+1.369713+1.588510+1.365518)/10
1.4139491999999998

Case2: DECODE USING CLIENT BUF = 1 (No VPP)
>>> (1.282750+1.381442+1.415762+1.300139+1.274943+1.296528+1.286514+1.449177+1.284462+1.292342)/10
1.3264059 

Miguel's patch in https://chromium-review.googlesource.com/c/chromium/src/+/1356019 will enable the non-vpp path by default for H264.
Project Member

Comment 46 by bugdroid1@chromium.org, Nov 30

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f0e7045ffa0c7ea4e88a33462906f9658f35566d

commit f0e7045ffa0c7ea4e88a33462906f9658f35566d
Author: Miguel Casas <mcasas@chromium.org>
Date: Fri Nov 30 01:50:40 2018

Vaapi: Activate |decode_using_client_picture_buffers_| for h264

This CL cleans up and activates |decode_using_client_picture_buffers_|
after recent power consumption measurements done by the Intel folks
reveal that there should be no regression, and also because skipping
the Vpp has a huge impact in memory consumption since Va doesn't need
to allocate ~13/14 Buffer Objects inside (see crbug.com/909926).

Bug: 909926, 822346
Change-Id: Ie8d6b41521f5d8ee62568f9674ae071a67c290bf
Reviewed-on: https://chromium-review.googlesource.com/c/1356019
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612498}
[modify] https://crrev.com/f0e7045ffa0c7ea4e88a33462906f9658f35566d/media/gpu/vaapi/vaapi_video_decode_accelerator.cc

Project Member

Comment 47 by bugdroid1@chromium.org, Dec 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d1df6533e0ac7d5e63fcf6e6339e023263da8e35

commit d1df6533e0ac7d5e63fcf6e6339e023263da8e35
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Mon Dec 03 03:42:56 2018

Revert "Vaapi: Activate |decode_using_client_picture_buffers_| for h264"

This reverts commit f0e7045ffa0c7ea4e88a33462906f9658f35566d.

Reason for revert: Power regression on eve crbug.com/910986

Original change's description:
> Vaapi: Activate |decode_using_client_picture_buffers_| for h264
> 
> This CL cleans up and activates |decode_using_client_picture_buffers_|
> after recent power consumption measurements done by the Intel folks
> reveal that there should be no regression, and also because skipping
> the Vpp has a huge impact in memory consumption since Va doesn't need
> to allocate ~13/14 Buffer Objects inside (see crbug.com/909926).
> 
> Bug: 909926, 822346
> Change-Id: Ie8d6b41521f5d8ee62568f9674ae071a67c290bf
> Reviewed-on: https://chromium-review.googlesource.com/c/1356019
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> Commit-Queue: Miguel Casas <mcasas@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#612498}

TBR=mcasas@chromium.org,hiroh@chromium.org,sreerenj.balachandran@intel.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 909926, 822346 910986
Change-Id: I1cb5d54064413d27cd3472e9918c5fcd62385a1c
Reviewed-on: https://chromium-review.googlesource.com/c/1358099
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612996}
[modify] https://crrev.com/d1df6533e0ac7d5e63fcf6e6339e023263da8e35/media/gpu/vaapi/vaapi_video_decode_accelerator.cc

Blockedon: 910986

Sign in to add a comment