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

Issue 832838 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac , Fuchsia
Pri: 2
Type: Bug



Sign in to add a comment

Small blur imagefilter sigmas can cause GPU readbacks

Project Member Reported by robertphillips@chromium.org, Apr 13 2018

Issue description

The Skia CL https://skia-review.googlesource.com/c/skia/+/87420 (Account for border with small sigma) added a readback from the GPU if the blur image filter sigma was small.

This can cause a significant slowdown when drawing what should be a fast image filter case.

 
The skbug.com correlates for this issue are:

skbug.com/7787 (SkBlurImageFilter causes GPU->CPU readbacks resulting in janky animations)
skbug.com/7751 ([DDL] Several GMs broken in DDL mode)
Project Member

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

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/28a142f213cf4d7dd4e6005a96cca6cb67a9887d

commit 28a142f213cf4d7dd4e6005a96cca6cb67a9887d
Author: Robert Phillips <robertphillips@google.com>
Date: Fri Apr 13 19:14:42 2018

Don't try to readback from the GPU for small blur sigmas

This should revert Ganesh's small sigma handling behavior to back before:  https://skia-review.googlesource.com/c/skia/+/52400 (Reorganize blur filter to insert new implementation)

BUG= skia:7787 
BUG= skia:7751 
BUG= 832838 

Change-Id: I0ce6b888be534afb60336abf561e97741fa34684
Reviewed-on: https://skia-review.googlesource.com/121331
Commit-Queue: Robert Phillips <robertphillips@google.com>
Reviewed-by: Herb Derby <herb@google.com>

[modify] https://crrev.com/28a142f213cf4d7dd4e6005a96cca6cb67a9887d/src/core/SkBlurImageFilter.cpp

Cc: hcm@chromium.org
Project Member

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

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

commit 625dceb10fd7d7a620af93a0f362e0f541cc0a23
Author: skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Fri Apr 13 23:26:18 2018

Roll src/third_party/skia/ 21dfd8467..490aa59ce (13 commits)

https://skia.googlesource.com/skia.git/+log/21dfd846724a..490aa59ce2e9

$ git log 21dfd8467..490aa59ce --date=short --no-merges --format='%ad %ae %s'
2018-04-13 reed rewrite iterator to make msvc happy
2018-04-13 brianosman Support downscaling to max texture size when making cross-context images
2018-04-13 borenet Add a MSVS bot to the CQ
2018-04-13 liyuqian Clone the xpos array in drawPosText
2018-04-13 ethannicholas added sk_LastFragColor
2018-04-13 reed Revert "add test for degenerate canvas dimension"
2018-04-13 reed add test for degenerate canvas dimension
2018-04-13 robertphillips Don't try to readback from the GPU for small blur sigmas
2017-12-20 herb Make a GM for checking blur bounds.
2018-04-13 angle-skia-autoroll Roll third_party/externals/angle2/ cc1293775..983c429fa (1 commit)
2018-04-13 fmalita [skottie] Harden json string parsing
2018-04-13 caryclark path is rect bug number nine
2018-04-13 hcm Update Skia milestone to 68

Created with:
  roll-dep src/third_party/skia
BUG= chromium:832838 , chromium:824145 


The AutoRoll server is located here: https://autoroll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;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
TBR=borenet@chromium.org

Change-Id: Ie3362a821e3592adfd5e247ef6fdab0c9562e5d8
Reviewed-on: https://chromium-review.googlesource.com/1013048
Reviewed-by: skia-chromium-autoroll <skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: skia-chromium-autoroll <skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#550812}
[modify] https://crrev.com/625dceb10fd7d7a620af93a0f362e0f541cc0a23/DEPS

Labels: Merge-Request-67
Reportedly, when unpatched, this bug can cause a lot of jank in Chrome. Requesting cherry-pick back to M67.
Labels: -Pri-3 OS-Android OS-Chrome OS-Fuchsia OS-iOS OS-Linux OS-Mac OS-Windows Pri-2
Project Member

Comment 7 by sheriffbot@chromium.org, Apr 16 2018

Labels: -Merge-Request-67 Merge-Review-67 Hotlist-Merge-Review
This bug requires manual review: DEPS changes referenced in bugdroid comments.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Status: Fixed (was: Assigned)

Comment 9 by gov...@chromium.org, Apr 16 2018

Which CL you're requesting a merge to M67? And Is the change looking good in canary?

Also does this need a merge to M66? If yes, pls request a merge ASAP. Thank you.
Labels: Merge-Rejected-66
The merge request is for the Skia CL: https://skia-review.googlesource.com/c/skia/+/121331 (Don't try to readback from the GPU for small blur sigmas). 

I haven't received any complaints from the Canary channel. That said, perf testing this use case of blurs seems to be a hole in both Skia's and Chrome's test suites.

Skia branched for M66 on 3/1 and the causal CL was landed 9/28 so, yeah, this should be cherry-picked back to M66 too.
Cc: abdulsyed@chromium.org
Labels: -Merge-Rejected-66 -Merge-Review-67 Merge-Request-66 Merge-Approved-67
Approving merge for the Skia CL: https://skia-review.googlesource.com/c/skia/+/121331  to M67 branch 3396 based on comment #10.

 robertphillips@, pls clarify cl landed date here (
Skia branched for M66 on 3/1 and the causal CL was landed 9/28 so, yeah, this should be cherry-picked back to M66 too).

+Abdul (M66 TPM) for M66 merge review.


Project Member

Comment 12 by sheriffbot@chromium.org, Apr 16 2018

Labels: -Merge-Request-66 Merge-Review-66
This bug requires manual review: Less than -2 days to go before AppStore submit on M66
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
The bug was introduced in two stages:

1st: 9/28 https://skia-review.googlesource.com/c/skia/+/52400 (Reorganize blur filter to insert new implementation)
2nd: 12/20 https://skia-review.googlesource.com/c/skia/+/87420 (Account for border with small sigma.)

The fix (4/13 https://skia-review.googlesource.com/c/skia/+/121331 (Don't try to readback from the GPU for small blur sigmas)) reverts the behavior of the blur to back before the 1st portion of the bug was landed.
Thank you  robertphillips@. 

As per comment #13, this bug has been exists since 09/28 and 12/20. Is there any reason this can't wait until M68?

Pls note M66 is going to stable tomorrow and M67 was already branched on 04/12.
Perhaps Stephen can comment on the severity of the jank. When hit, the performance degradation would be pretty severe since the buggy code reads back from the GPU.

I cannot speak to how often web sites would hit this case.
Pls merge to M67.

I will let abdulsyed@ decide on M66 merge.
how safe is this fix overall? Has it been well tested in Canary/Dev?
If this bug has been present since at least 12/20, I'm inclined not to take this for 66 and targeting 67 instead. 
Responding to #17, the change is pretty safe. It is reverting to the behavior before the issue was introduced and is a very targeted change.

Testing-wise, the issue would always have been a performance/jank problem and I believe neither Skia nor Chrome has any coverage of this use case. 

Responding to #18, given the long existence of the bug and the fact that no site out in the wild is screaming about jankiness, it is probably pretty safe to not cherry-pick this back to M66.
Project Member

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

Labels: merge-merged-m67
The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/49e34766f668622f2ce3199c9084c085e8b6d26d

commit 49e34766f668622f2ce3199c9084c085e8b6d26d
Author: Robert Phillips <robertphillips@google.com>
Date: Mon Apr 16 19:07:46 2018

[M67 cherry-pick] Don't try to readback from the GPU for small blur sigmas

This should revert Ganesh's small sigma handling behavior to back before:  https://skia-review.googlesource.com/c/skia/+/52400 (Reorganize blur filter to insert new implementation)

BUG= skia:7787 
BUG= skia:7751 
BUG= 832838 

No-Tree-Checks: true
No-Try: true
No-Presubmit: true
Change-Id: I5daa281ab9f320dc3299f16cfc7daa2c38f9aa18
Reviewed-on: https://skia-review.googlesource.com/121662
Reviewed-by: Brian Salomon <bsalomon@google.com>

[modify] https://crrev.com/49e34766f668622f2ce3199c9084c085e8b6d26d/src/core/SkBlurImageFilter.cpp

Per comment #20, this is already merged to M67. If nothing is pending, pls remove "Merge-Approved-67" label. Thank you.
Labels: -Merge-Approved-67
Labels: -Merge-Review-66 Merge-Rejected-66
Project Member

Comment 24 by bugdroid1@chromium.org, Apr 17 2018

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

commit 625dceb10fd7d7a620af93a0f362e0f541cc0a23
Author: skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Fri Apr 13 23:26:18 2018

Roll src/third_party/skia/ 21dfd8467..490aa59ce (13 commits)

https://skia.googlesource.com/skia.git/+log/21dfd846724a..490aa59ce2e9

$ git log 21dfd8467..490aa59ce --date=short --no-merges --format='%ad %ae %s'
2018-04-13 reed rewrite iterator to make msvc happy
2018-04-13 brianosman Support downscaling to max texture size when making cross-context images
2018-04-13 borenet Add a MSVS bot to the CQ
2018-04-13 liyuqian Clone the xpos array in drawPosText
2018-04-13 ethannicholas added sk_LastFragColor
2018-04-13 reed Revert "add test for degenerate canvas dimension"
2018-04-13 reed add test for degenerate canvas dimension
2018-04-13 robertphillips Don't try to readback from the GPU for small blur sigmas
2017-12-20 herb Make a GM for checking blur bounds.
2018-04-13 angle-skia-autoroll Roll third_party/externals/angle2/ cc1293775..983c429fa (1 commit)
2018-04-13 fmalita [skottie] Harden json string parsing
2018-04-13 caryclark path is rect bug number nine
2018-04-13 hcm Update Skia milestone to 68

Created with:
  roll-dep src/third_party/skia
BUG= chromium:832838 , chromium:824145 


The AutoRoll server is located here: https://autoroll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;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
TBR=borenet@chromium.org

Change-Id: Ie3362a821e3592adfd5e247ef6fdab0c9562e5d8
Reviewed-on: https://chromium-review.googlesource.com/1013048
Reviewed-by: skia-chromium-autoroll <skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: skia-chromium-autoroll <skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#550812}
[modify] https://crrev.com/625dceb10fd7d7a620af93a0f362e0f541cc0a23/DEPS

Comment 25 by junov@chromium.org, Apr 24 2018

Cc: ethannicholas@chromium.org
 Issue 832172  has been merged into this issue.

Sign in to add a comment