Small blur imagefilter sigmas can cause GPU readbacks |
|||||||||||||
Issue descriptionThe 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.
,
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
,
Apr 13 2018
,
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
,
Apr 16 2018
Reportedly, when unpatched, this bug can cause a lot of jank in Chrome. Requesting cherry-pick back to M67.
,
Apr 16 2018
,
Apr 16 2018
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
,
Apr 16 2018
,
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.
,
Apr 16 2018
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.
,
Apr 16 2018
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.
,
Apr 16 2018
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
,
Apr 16 2018
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.
,
Apr 16 2018
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.
,
Apr 16 2018
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.
,
Apr 16 2018
Pls merge to M67. I will let abdulsyed@ decide on M66 merge.
,
Apr 16 2018
how safe is this fix overall? Has it been well tested in Canary/Dev?
,
Apr 16 2018
If this bug has been present since at least 12/20, I'm inclined not to take this for 66 and targeting 67 instead.
,
Apr 16 2018
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.
,
Apr 16 2018
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
,
Apr 16 2018
Per comment #20, this is already merged to M67. If nothing is pending, pls remove "Merge-Approved-67" label. Thank you.
,
Apr 16 2018
,
Apr 16 2018
,
Apr 17 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
,
Apr 24 2018
|
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by robertphillips@chromium.org
, Apr 13 2018