Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1905673003:
use new skia imagefilter-ctm handling (Closed)
|
Created:
4 years, 8 months ago by reed1 Modified:
4 years, 8 months ago CC:
chromium-reviews, krit, pdr+graphicswatchlist_chromium.org, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, jbroman, slimming-paint-reviews_chromium.org, danakj+watch_chromium.org, Rik, f(malita), Justin Novosad, blink-reviews-paint_chromium.org, blink-reviews, kinuko+watch, Stephen Chennney, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionuse new skia imagefilter-ctm handling
BUG=
Committed: https://crrev.com/ae451614df577ad43766659bf9c51af85a1d5517
Cr-Commit-Position: refs/heads/master@{#389561}
Patch Set 1 #
Total comments: 1
Patch Set 2 : delete support code #Patch Set 3 : update TestExpectations #Patch Set 4 : should be good to go #Patch Set 5 : #
Messages
Total messages: 39 (15 generated)
reed@google.com changed reviewers: + fmalita@chromium.org, robertphillips@chromium.org, senorblanco@chromium.org
just removed def to see the layout diff
Looking at the 2nd diff (with 2 shapes) ... Safari draws the same as the new output.
On 2016/04/21 16:41:33, reed1 wrote: > Looking at the 2nd diff (with 2 shapes) ... Safari draws the same as the new > output. Hmm. I'm really surprised that these CSS tests (which don't create an SkMatrixImageFilter) changed, while the SVG filter ones (e.g., svg/batik/filters/feTile.svg) didn't. Anyway I can live with the changes.
https://codereview.chromium.org/1905673003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/SVGFilterPainter.cpp (right): https://codereview.chromium.org/1905673003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/SVGFilterPainter.cpp:17: //#define CHECK_CTM_FOR_TRANSFORMED_IMAGEFILTER For landing, we should also delete the related code.
svg/batik/filters/feTile.svg did change, but before I removed the flag. It was just a low bit along one edge, so I've marked it for rebaselining. see https://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/...
... which means we're not seeing it here if it did change. I will run it locally now.
The result was different, but not clearly better or worse (to me and florin). tile_1 looked a little better (no vertical white gap between tiles) tile_3 showed a different internal scale for each tile. Hard to know which was better. I do note that the prev math to compute the "scale" matrix is different from the new code in skia. The old code seems to read [0][0] and [1][1] for the scale, but the new code computes the effective scale by taking the magnitude of each basis vector.
On 2016/04/21 18:50:04, reed1 wrote: > The result was different, but not clearly better or worse (to me and florin). > > tile_1 looked a little better (no vertical white gap between tiles) > > tile_3 showed a different internal scale for each tile. Hard to know which was > better. > > I do note that the prev math to compute the "scale" matrix is different from the > new code in skia. The old code seems to read [0][0] and [1][1] for the scale, > but the new code computes the effective scale by taking the magnitude of each > basis vector. That could be it. Or it could be the resampling quality. The CSS ones concern me a little. We shouldn't be hitting the new Skia code, and if we are, I suspect we're adding an extra blit. This would suggest we've got rotation in the matrix that filters see, which I don't think should be the case -- they should be rendered with a scale-only matrix, and rotated post-filter by the compositor (or Blink).
If I can get a debugger to work, I'll try to set a break on the new code and try loading one of the CSS examples. If there is rotation in the CTM, are you suggesting that is a bug up-stack in blink or compositor, or something else?
The CQ bit was checked by reed@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1905673003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1905673003/20001
The CQ bit was unchecked by reed@google.com
The CQ bit was checked by reed@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1905673003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1905673003/40001
The CQ bit was checked by reed@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1905673003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1905673003/60001
After https://codereview.chromium.org/1908173006 landed, we now just seeing svg changes (as expected), and those only in the low bits. PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by reed@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1905673003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1905673003/80001
The CQ bit was unchecked by reed@google.com
The CQ bit was checked by reed@google.com
The CQ bit was unchecked by reed@google.com
The CQ bit was checked by reed@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1905673003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1905673003/80001
any comments?
Thanks for optimizing the color filter case. I'm still disturbed that we've been sending non-scale/translate matrices from CSS filters (software path). I think there may be a Blink-side bug there, since the compositor path doesn't do that. Let me see if I can conjure up a test case.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
OK, there definitely is a bug, with or without this change. I've logged it as https://bugs.chromium.org/p/chromium/issues/detail?id=606452. With this change, it is still incorrect, but clips the blurred edges. I think we should proceed anyway, and fix the other bug independently, since it looks like it's been there for over a year. LGTM
robertphillips@google.com changed reviewers: + robertphillips@google.com
lgtm
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1905673003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1905673003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== use new skia imagefilter-ctm handling BUG= ========== to ========== use new skia imagefilter-ctm handling BUG= Committed: https://crrev.com/ae451614df577ad43766659bf9c51af85a1d5517 Cr-Commit-Position: refs/heads/master@{#389561} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ae451614df577ad43766659bf9c51af85a1d5517 Cr-Commit-Position: refs/heads/master@{#389561} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
