We should still do it for this milestone or the next

Issue 887658 has been merged into this issue.

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

commit eb0d6e13f58c24e88d9c40121981fb861303c45c
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Tue Sep 25 07:04:03 2018

Use ARGB8888 for a dummy buffer for protected content playbkack on ARC++

We have specified R8 format for a dummy buffer for protected content playback
on ARC++.
R8 format is not supported on exynos device. It leads crash on the devices.
ARGB8888 should be used instead, which is widely supported on all Chrome OS
device.

BUG=chromium:787660, chromium:887658
TEST=No crash on peach_pit and Protected content playback on veyron_minnie and eve

Change-Id: Ia550277ad30a0e0fd814106a1610b59a1a86bb03
Reviewed-on: https://chromium-review.googlesource.com/1242564
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593842}
[modify] https://crrev.com/eb0d6e13f58c24e88d9c40121981fb861303c45c/components/arc/video_accelerator/protected_buffer_manager.cc

I will ask merge-request after landing this change on Chrome OS and confirming this doesn't break anything on R71.

hiroh@, thanks for fixing this bug!

No problem.
I have one question; HAL_PIXEL_FORMAT_BLOB is used in many places in Android container. It is regarded as R8 in cros-gralloc. Doesn't this matter?
In exynos devices, ARC++ is not enabled. So I think this doesn't cause crash.
But I heard you (marcheu@) would not like to increase use of R8.

What I don't want to assume is that R8 is usable everywhere (including pre-ARC++ devices) because it isn't generally true (exynos is one example, tegra another).

The ARC++ side is less of a problem. What gralloc does with HAL_PIXEL_FORMAT_BLOB is driver-dependent. So in the future if a board has ARC++ but not R8 we always have the option of changing it inside of cros-gralloc.

marcheu@, thanks for explaining. I got it.
The CL in #c4 was landed on Chrome OS 11101.0.0.
We can wait for one day in order to confirm this will not cause a new failure in CTS/GTS or any Chrome tests.
After making sure it, we can ask for merge-request.

No regression happens so far by this change.
Ask Merge-Request crrev.com/c/1242564 for R70.

This bug requires manual review: M70 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

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

commit 2a0dc825b1c2c7621557bc625c90de6534324e5c
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Tue Oct 02 01:09:22 2018

Use ARGB8888 for a dummy buffer for protected content playbkack on ARC++

We have specified R8 format for a dummy buffer for protected content playback
on ARC++.
R8 format is not supported on exynos device. It leads crash on the devices.
ARGB8888 should be used instead, which is widely supported on all Chrome OS
device.

BUG=chromium:787660, chromium:887658
TEST=No crash on peach_pit and Protected content playback on veyron_minnie and eve

Change-Id: Ia550277ad30a0e0fd814106a1610b59a1a86bb03
Reviewed-on: https://chromium-review.googlesource.com/1242564
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#593842}(cherry picked from commit eb0d6e13f58c24e88d9c40121981fb861303c45c)
Reviewed-on: https://chromium-review.googlesource.com/1256342
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#798}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/2a0dc825b1c2c7621557bc625c90de6534324e5c/components/arc/video_accelerator/protected_buffer_manager.cc

Merged to M70; Let's wait and see the crash rate will decrease. 
https://crash.corp.google.com/d05eabd1fbc05367.

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

Commit: 2a0dc825b1c2c7621557bc625c90de6534324e5c
Author: hiroh@chromium.org
Commiter: hiroh@chromium.org
Date: 2018-10-02 01:09:22 +0000 UTC

Use ARGB8888 for a dummy buffer for protected content playbkack on ARC++

We have specified R8 format for a dummy buffer for protected content playback
on ARC++.
R8 format is not supported on exynos device. It leads crash on the devices.
ARGB8888 should be used instead, which is widely supported on all Chrome OS
device.

BUG=chromium:787660, chromium:887658
TEST=No crash on peach_pit and Protected content playback on veyron_minnie and eve

Change-Id: Ia550277ad30a0e0fd814106a1610b59a1a86bb03
Reviewed-on: https://chromium-review.googlesource.com/1242564
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Pawel Osciak <posciak@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#593842}(cherry picked from commit eb0d6e13f58c24e88d9c40121981fb861303c45c)
Reviewed-on: https://chromium-review.googlesource.com/1256342
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#798}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}

Just want to confirm if this merge from Oct 02 made it into 70.0.3538.41 which was pushed out soon after on the 3rd. Still seeing about a 3% contribution to the version with about 800 crash counts since it rolled out. CPM graph looks similar to previous version.

https://crash.corp.google.com/browse?q=product_name%3D%27Chrome_ChromeOS%27+AND+product.Version+LIKE+%2770.0.3538.41%27+AND+expanded_custom_data.ChromeCrashProto.magic_signature_1.name%3D%27gbm_wrapper%3A%3ABuffer%3A%3AGetHandle%27

My fix CL was landed on Chrome 70.0.3538.45, not 41.
It is not upreved even on the latest Chrome OS R70 image.
https://chromium.googlesource.com/chromium/src/+log/70.0.3538.44..70.0.3538.45?n=10000

We should wait for a few days.

Hi Hiro, thanks for the clarification.

I'm not too familiar with the uprev process so not sure if this is also expected but
I am currently checking 70.0.3538.58 and see crashes. Is this still expected?

https://crash.corp.google.com/browse?q=product_name%3D%27Chrome_ChromeOS%27+AND+product.Version%3D%2770.0.3538.58%27+AND+expanded_custom_data.ChromeCrashProto.magic_signature_1.name%3D%27gbm_wrapper%3A%3ABuffer%3A%3AGetHandle%27

> I'm not too familiar with the uprev process so not sure if this is also expected but
> I am currently checking 70.0.3538.58 and see crashes. Is this still expected?

This is not expected. Pawel found the real crash reason today. I created the fix CL, crrev.com/c/1282523.
Since the format change is not related, I will revert it. It causes some other crash on AMD devices. We should reland it in the future.

The crash reports also go to zero between 71.0.3558.0 and 71.0.3567.0.

https://chromium.googlesource.com/chromium/src/+log/71.0.3558.0..71.0.3567.0?n=10000

I suspect backporting this will also fix the issue:


https://chromium-review.googlesource.com/c/chromium/src/+/1219087

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

commit e67d13c840a98ac22ff70bf45835847b130f0ac5
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Tue Oct 16 02:43:59 2018

Revert "Use ARGB8888 for a dummy buffer for protected content playbkack on ARC++"

This reverts commit eb0d6e13f58c24e88d9c40121981fb861303c45c.

Reason for revert: This CL causes a crash on AMD device, b/117398307

Original change's description:
> Use ARGB8888 for a dummy buffer for protected content playbkack on ARC++
> 
> We have specified R8 format for a dummy buffer for protected content playback
> on ARC++.
> R8 format is not supported on exynos device. It leads crash on the devices.
> ARGB8888 should be used instead, which is widely supported on all Chrome OS
> device.
> 
> BUG=chromium:787660, chromium:887658
> TEST=No crash on peach_pit and Protected content playback on veyron_minnie and eve
> 
> Change-Id: Ia550277ad30a0e0fd814106a1610b59a1a86bb03
> Reviewed-on: https://chromium-review.googlesource.com/1242564
> Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Pawel Osciak <posciak@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#593842}

TBR=posciak@chromium.org,gurchetansingh@chromium.org,hiroh@chromium.org

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

Bug: chromium:787660, chromium:887658
Change-Id: I7cc4e8607b0dd267f73a2f32879abae74f9fa24e
Reviewed-on: https://chromium-review.googlesource.com/c/1282505
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599824}
[modify] https://crrev.com/e67d13c840a98ac22ff70bf45835847b130f0ac5/components/arc/video_accelerator/protected_buffer_manager.cc

Hi, TPM, could you review CL#23 for merge-request M71?
It is needed to fix GTS crash on AMD device at M71 branch, b/117574845

This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Has the change in #23 been verified?  Pending that feedback....

I verified on AMD device by locally running GTS.

Approving merge to M71 Chrome OS.

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

commit 876c7f6737e4b70a0967031984acaf6f2269e142
Author: Hirokazu Honda <hiroh@chromium.org>
Date: Fri Oct 19 00:47:21 2018

Revert "Use ARGB8888 for a dummy buffer for protected content playbkack on ARC++"

This reverts commit eb0d6e13f58c24e88d9c40121981fb861303c45c.

Reason for revert: This CL causes a crash on AMD device, b/117398307

Original change's description:
> Use ARGB8888 for a dummy buffer for protected content playbkack on ARC++
> 
> We have specified R8 format for a dummy buffer for protected content playback
> on ARC++.
> R8 format is not supported on exynos device. It leads crash on the devices.
> ARGB8888 should be used instead, which is widely supported on all Chrome OS
> device.
> 
> BUG=chromium:787660, chromium:887658
> TEST=No crash on peach_pit and Protected content playback on veyron_minnie and eve
> 
> Change-Id: Ia550277ad30a0e0fd814106a1610b59a1a86bb03
> Reviewed-on: https://chromium-review.googlesource.com/1242564
> Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Pawel Osciak <posciak@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#593842}

TBR=posciak@chromium.org,gurchetansingh@chromium.org,hiroh@chromium.org

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

Bug: chromium:787660, chromium:887658
Change-Id: I7cc4e8607b0dd267f73a2f32879abae74f9fa24e
Reviewed-on: https://chromium-review.googlesource.com/c/1282505
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#599824}(cherry picked from commit e67d13c840a98ac22ff70bf45835847b130f0ac5)
Reviewed-on: https://chromium-review.googlesource.com/c/1290429
Cr-Commit-Position: refs/branch-heads/3578@{#142}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/876c7f6737e4b70a0967031984acaf6f2269e142/components/arc/video_accelerator/protected_buffer_manager.cc

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

Commit: 876c7f6737e4b70a0967031984acaf6f2269e142
Author: hiroh@chromium.org
Commiter: hiroh@chromium.org
Date: 2018-10-19 00:47:21 +0000 UTC

Revert "Use ARGB8888 for a dummy buffer for protected content playbkack on ARC++"

This reverts commit eb0d6e13f58c24e88d9c40121981fb861303c45c.

Reason for revert: This CL causes a crash on AMD device, b/117398307

Original change's description:
> Use ARGB8888 for a dummy buffer for protected content playbkack on ARC++
> 
> We have specified R8 format for a dummy buffer for protected content playback
> on ARC++.
> R8 format is not supported on exynos device. It leads crash on the devices.
> ARGB8888 should be used instead, which is widely supported on all Chrome OS
> device.
> 
> BUG=chromium:787660, chromium:887658
> TEST=No crash on peach_pit and Protected content playback on veyron_minnie and eve
> 
> Change-Id: Ia550277ad30a0e0fd814106a1610b59a1a86bb03
> Reviewed-on: https://chromium-review.googlesource.com/1242564
> Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Pawel Osciak <posciak@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#593842}

TBR=posciak@chromium.org,gurchetansingh@chromium.org,hiroh@chromium.org

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

Bug: chromium:787660, chromium:887658
Change-Id: I7cc4e8607b0dd267f73a2f32879abae74f9fa24e
Reviewed-on: https://chromium-review.googlesource.com/c/1282505
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#599824}(cherry picked from commit e67d13c840a98ac22ff70bf45835847b130f0ac5)
Reviewed-on: https://chromium-review.googlesource.com/c/1290429
Cr-Commit-Position: refs/branch-heads/3578@{#142}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}