Picture getting cut when using translate, rotate and blur at the same Time
Reported by
snapstro...@gmail.com,
Nov 3 2017
|
||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.75 Safari/537.36 Steps to reproduce the problem: You can see this in the attached File or here: https://codepen.io/Snapstromegon/pen/mqVZjm 1. Create an img element 2. Add the transform "translate" and "rotate" width values other than 0 3. Add the css Filter "blur" with a value other than 0 What is the expected behavior? The picture should just be transformed and scroll with the site normally. What went wrong? At certain scrollstates parts of the Image disappear and the element underneath gets shown. Did this work before? N/A Does this work in other browsers? Yes Chrome version: 62.0.3202.75 Channel: stable OS Version: 10.0 Flash Version:
,
Nov 6 2017
Able to reprodce this issue on reported version 62.0.3202.75 and on latest dev 64.0.3260.0 using Ubuntu 14.04, Windows 10 and Mac 10.12.6. Manual Bisect Info: =============== Good Build: 62.0.3167.0 Bad build: 62.0.3168.0 You are probably looking for a change made after 489800 (known good), but no later than 489801 (first known bad). CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/1eb2db5b44e5076cac3d634ffd5332cb96b92ea3..d880a1511779ef31871522ef88605911848f4282 Review URL: https://chromium-review.googlesource.com/582330 Suspecting same from changelog. @khushalsagar: Please confirm the bug and help in re-assigning if it is not related to your change. Thanks!
,
Nov 6 2017
,
Nov 7 2017
This only happens when raster is using SkColorSpaceXformCanvas. The clip is not set correctly on the canvas wrapping the fTarget. cc/paint code uses quickReject on this wrapping canvas, which thinks all ops are clipped out because the clip is empty. The place where we ended up with the wrong clip narrowed down precisely to: https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkCanvas.cpp?q=SkCanvas.cpp&sq=package:chromium&l=968. The problem is happening here: https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkCanvas.cpp?q=SkCanvas.cpp&sq=package:chromium&l=1085. Before internalSaveLayer is called, the target canvas has already performed the saveLayer operation. When the wrapping canvas performs this operation, it uses the internal matrix, but calls the overridden method for querying getDeviceClipBounds, which has the final clip after the operation. The reason the bug happened in this case was because we only do this rejecting of ops for images we predecode before executing the op. msarett@, would you be the correct owner for this? I looked for the change that introduced these overrides (https://skia-review.googlesource.com/c/skia/+/10805).
,
Nov 7 2017
,
Nov 7 2017
,
Nov 7 2017
I don't think Matt is contributing to Chromium anymore, so I'll take this one. What CL caused the regression that's blocking stable here? https://chromium-review.googlesource.com/c/chromium/src/+/582330? It's probably not a good idea to use quickReject() and SkColorSpaceXformCanvas. Can you just not do that?
,
Nov 7 2017
It happened as a result of the change above because earlier we were wrapping the SkColorSpaceXformCanvas in an SkNWayCanvas which would end up being used for quickReject tests since that was the top level canvas used during raster. Now we use SkColorSpaceXformCanvas directly. The bug is because SkColorSpaceXformCanvas does not have the correct clip/transform state, which I think should be fixed in that class. A canvas should always be able to provide the correct state based on the ops it has seen, the fact that it is forwarding to a different canvas is a detail callers shouldn't need to care about. Is there a reason why the clip querying overrides were added to it? I'm guessing the intent was to ensure callers see the correct state. Shouldn't SkNoDrawCanvas already be taking care of it?
,
Nov 9 2017
,
Nov 20 2017
Issue 786352 has been merged into this issue.
,
Nov 27 2017
Friendly ping for an update on this.
,
Nov 27 2017
Ack. Will be looking at this this week.
,
Nov 27 2017
I can reproduce this locally, and while I don't quite have a solid grasp on all the interactions yet, I agree that it is looking like the four clip-querying methods on SkCanvas should not be overridden by SkColorSpaceXformCanvas. Removing those fixes the issue for me locally. Few SkCanvas subclasses override these methods, and I'm in the process of removing overrides one subclass at a time as a way to make sure I understand what's going on. (None of the others are as important as SkColorSpaceXformCanvas.) I'm pretty sure it's a bad idea for these methods to be virtual at all... the base SkCanvas can and does track enough to answer these queries. SkNoDrawCanvas and SkNWayCanvas didn't override these methods, so removing the overrides here should make SkColorSpaceXformCanvas consistent. It should be fairly easy to write a fix for this that can be backported cleanly, and then I'll follow up with all the devirtualization cleanup only at head. Before I do land this, I want to make sure I've got a better understanding of the interactions, and to have a small unit test or Skia GM that reproduces this. Right now none of our tests draw any differently through SkColorSpaceXformCanvas with or without those overrides... I'd like to make sure we have a regression test in the works before committing the fix.
,
Nov 28 2017
The following revision refers to this bug: https://skia.googlesource.com/skia/+/399def9595f94f557cd4ea84e028c2c37dbae695 commit 399def9595f94f557cd4ea84e028c2c37dbae695 Author: Mike Klein <mtklein@chromium.org> Date: Tue Nov 28 17:08:47 2017 remove clip-bounds query overrides from SkColorSpaceXformCanvas This is the only SkCanvas subclass currently overriding these, and this leads to buggy interactions with this canvas, saveLayer(), and image filters. The base SkCanvas tracks and can answer these queries just as well as fTarget. A couple other SkCanvas subclasses do override isClipEmpty() and isClipRect(), and these conservative clip tracking SkCanvases can't really answer isClipRect() correctly, so I've left those both. I'm intentionally keeping this CL minimal in case it needs backporting to other branches. I'm going to follow up separately with a GM or unit test that would reproduce the Chrome bug, and with a CL that de-virtualizes these two methods. BUG= chromium:781238 Change-Id: I75bf6b0910115fafe17fa721082acc415bfdae23 Reviewed-on: https://skia-review.googlesource.com/76802 Reviewed-by: Mike Reed <reed@google.com> Commit-Queue: Mike Klein <mtklein@chromium.org> [modify] https://crrev.com/399def9595f94f557cd4ea84e028c2c37dbae695/src/core/SkColorSpaceXformCanvas.cpp
,
Nov 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/04f4824dd3e9ff81d39072d83daecee65a4b9781 commit 04f4824dd3e9ff81d39072d83daecee65a4b9781 Author: skia-deps-roller@chromium.org <skia-deps-roller@chromium.org> Date: Tue Nov 28 19:39:52 2017 Roll src/third_party/skia/ 12b69eede..0f8e4dbd2 (4 commits) https://skia.googlesource.com/skia.git/+log/12b69eedeed5..0f8e4dbd28d1 $ git log 12b69eede..0f8e4dbd2 --date=short --no-merges --format='%ad %ae %s' 2017-11-28 rmistry Make RecreateSKPs bot fail if capturing SKPs fails + exclude failing page sets 2017-11-28 benjaminwagner Upgrade GalaxyS6. 2017-11-28 mtklein remove clip-bounds query overrides from SkColorSpaceXformCanvas 2017-11-28 angle-deps-roller Roll skia/third_party/externals/angle2/ 0b684ce3c..df68ca50b (1 commit) Created with: roll-dep src/third_party/skia BUG= 781238 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;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 TBR=allanmac@chromium.org Change-Id: Id2a7286132925b14aae4d1358ddb0b1c32ad01e0 Reviewed-on: https://chromium-review.googlesource.com/794019 Reviewed-by: Skia Deps Roller <skia-deps-roller@chromium.org> Commit-Queue: Skia Deps Roller <skia-deps-roller@chromium.org> Cr-Commit-Position: refs/heads/master@{#519798} [modify] https://crrev.com/04f4824dd3e9ff81d39072d83daecee65a4b9781/DEPS
,
Nov 29 2017
The following revision refers to this bug: https://skia.googlesource.com/skia/+/83c8dd9b1a20ec5b866a6c5a557313c13ae8904f commit 83c8dd9b1a20ec5b866a6c5a557313c13ae8904f Author: Mike Klein <mtklein@chromium.org> Date: Wed Nov 29 22:43:41 2017 devirtualize SkCanvas getClipBounds methods We no longer have any subclasses overriding the virtual hooks, and we've seen subtle bugs come up back when they did. In all modes, SkCanvas can answer these queries itself. BUG= chromium:781238 Change-Id: I37c7511c7bd00c638faacbe4bee89f785691453f Reviewed-on: https://skia-review.googlesource.com/77202 Reviewed-by: Mike Reed <reed@google.com> Commit-Queue: Mike Klein <mtklein@chromium.org> [modify] https://crrev.com/83c8dd9b1a20ec5b866a6c5a557313c13ae8904f/include/core/SkCanvas.h [modify] https://crrev.com/83c8dd9b1a20ec5b866a6c5a557313c13ae8904f/src/core/SkCanvas.cpp
,
Nov 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/066de975da576364b8d268be45ebdaf786b487dd commit 066de975da576364b8d268be45ebdaf786b487dd Author: skia-deps-roller@chromium.org <skia-deps-roller@chromium.org> Date: Thu Nov 30 03:17:34 2017 Roll src/third_party/skia/ ede860e91..ccd285277 (10 commits) https://skia.googlesource.com/skia.git/+log/ede860e91c2a..ccd2852773eb $ git log ede860e91..ccd285277 --date=short --no-merges --format='%ad %ae %s' 2017-11-29 angle-deps-roller Roll skia/third_party/externals/angle2/ f13cadd8b..c677795f9 (6 commits) 2017-11-28 mtklein devirtualize SkCanvas getClipBounds methods 2017-11-29 benjaminwagner Revert "Temporarily add Ubuntu IntelHD4400 jobs." 2017-11-29 csmartdalton Revert "Make sure to visit clips and dst proxies during gather" 2017-11-29 bsalomon Ignore deserialized path convexity and first direction. 2017-11-29 kjlubick Isolate the biggest CIPD assets to trim down on overhead 2017-11-29 mtklein SkRasterPipeline::dump() in forward order 2017-11-29 csmartdalton Make sure to visit clips and dst proxies during gather 2017-11-29 borenet Update CT bots to Debian-9.2 2017-11-29 borenet [infra] Provide Buildbucket Build ID in dm.json Created with: roll-dep src/third_party/skia BUG= 772653 , 781238 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;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 TBR=allanmac@chromium.org Change-Id: Ib5ff8ecb634495f9d85c3675bd89ec247c941bd3 Reviewed-on: https://chromium-review.googlesource.com/798430 Reviewed-by: Skia Deps Roller <skia-deps-roller@chromium.org> Commit-Queue: Skia Deps Roller <skia-deps-roller@chromium.org> Cr-Commit-Position: refs/heads/master@{#520409} [modify] https://crrev.com/066de975da576364b8d268be45ebdaf786b487dd/DEPS
,
Dec 4 2017
I think this is fixed. I wasn't ever able to get all the pieces together to reproduce this outside Chromium, but the "devirtualize SkCanvas getClipBounds methods" CL should make regressing in this particular way impossible.
,
Dec 4 2017
[Auto-generated comment by a script] We noticed that this issue is targeted for M-64; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-64 label, otherwise remove Merge-TBD label. Thanks.
,
Dec 4 2017
Looks like both of those CLs made the M64 cut (and only the first is required to fix is the issue). |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by manoranj...@chromium.org
, Nov 3 2017