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

Issue 781238 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Picture getting cut when using translate, rotate and blur at the same Time

Reported by snapstro...@gmail.com, Nov 3 2017

Issue description

UserAgent: 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:
 
test.html
420 bytes View Download
error.PNG
74.7 KB View Download
correct.PNG
263 KB View Download
Labels: Needs-Triage-M62 Needs-Bisect
Cc: vamshi.k...@techmahindra.com
Labels: -Type-Bug -Pri-2 -Needs-Bisect hasbisect-per-revision Triaged-ET M-64 RB-Stable OS-Linux OS-Mac Pri-1 Type-Bug-Regression
Owner: khushals...@chromium.org
Status: Assigned (was: Unconfirmed)
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!
Status: Started (was: Assigned)
Cc: reed@chromium.org mtklein@chromium.org khushals...@chromium.org
Owner: msarett@chromium.org
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).
Status: Assigned (was: Started)

Comment 6 by ajha@chromium.org, Nov 7 2017

Labels: -RB-Stable ReleaseBlock-Stable
Owner: mtklein@chromium.org
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?
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? 
Cc: divya.pa...@techmahindra.com
 Issue 782618  has been merged into this issue.
 Issue 786352  has been merged into this issue.

Comment 11 by ajha@chromium.org, Nov 27 2017

Friendly ping for an update on this.
Ack.  Will be looking at this this week.
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.
Project Member

Comment 14 by bugdroid1@chromium.org, 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

Project Member

Comment 15 by bugdroid1@chromium.org, 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

Project Member

Comment 16 by bugdroid1@chromium.org, 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

Project Member

Comment 17 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)
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.
Labels: Merge-TBD
[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.
Labels: -Merge-TBD
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