Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(5)

Issue 23295017: In image filters, apply the CTM and offset to the crop rect. This is necessary to compensate for bo… (Closed)

Created:
5 years, 10 months ago by Stephen White
Modified:
5 years, 10 months ago
Reviewers:
bsalomon, reed1
CC:
skia-review_googlegroups.com
Visibility:
Public.

Description

In image filters, apply the CTM and offset to the crop rect. This is necessary to compensate for both clipping applied by the compositor (communicated via the CTM) and for cropping applied in upstream image filters (communicated via the offset). This requires a few ugly conversions, since the crop rect is an SkIRect, and the ctm is an SkMatrix. I also had to offset the matrix passed to filter evaluation by drawSprite() and internalDrawBitmap() by the primitive position. This is the same offset that is applied when drawing the primitive, to compensate for the internal saveLayer(). Also apply the total matrix to the filter params in asNewEffect(), so that (for example) lighting params are offset by both the compositor clipping and upstream crop rects. R=reed@google.com Committed: https://code.google.com/p/skia/source/detail?r=10961

Patch Set 1 #

Patch Set 2 : Reset the translation component of the matrix passed to image filters. #

Patch Set 3 : New version using "global" positioning for crop rects and light positions #

Patch Set 4 : New approach: simply offset the CTM by the primitive origin. Revert changes to internalDrawBitmap(). #

Patch Set 5 : Use SkIntToScalar() where necessary. Remove unused operator+. #

Patch Set 6 : Cleanup #

Patch Set 7 : Fix typo in SkSpotlight::transform() (caught by GM/lighting). #

Patch Set 8 : Back out changes to adjust matrix by parent's crop rect. #

Patch Set 9 : Modify comment to reflect new API. #

Patch Set 10 : Use SkLight::transform() for transforming lights in GPU backend as well. #

Total comments: 3

Patch Set 11 : Fix documentation comment. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+156 lines, -94 lines) Patch
M gm/lighting.cpp View 1 2 3 2 chunks +10 lines, -3 lines 0 comments Download
M include/core/SkImageFilter.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +10 lines, -7 lines 0 comments Download
M include/effects/SkMagnifierImageFilter.h View 1 chunk +1 line, -1 line 0 comments Download
M include/effects/SkMatrixConvolutionImageFilter.h View 1 chunk +1 line, -1 line 0 comments Download
M src/core/SkCanvas.cpp View 1 2 3 4 5 6 2 chunks +6 lines, -3 lines 0 comments Download
M src/core/SkImageFilter.cpp View 1 2 3 4 5 5 chunks +17 lines, -6 lines 0 comments Download
M src/effects/SkBlurImageFilter.cpp View 1 2 3 4 5 6 7 2 chunks +2 lines, -2 lines 0 comments Download
M src/effects/SkColorFilterImageFilter.cpp View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M src/effects/SkLightingImageFilter.cpp View 1 2 3 4 5 6 7 8 9 35 chunks +99 lines, -65 lines 0 comments Download
M src/effects/SkMagnifierImageFilter.cpp View 2 chunks +2 lines, -2 lines 0 comments Download
M src/effects/SkMatrixConvolutionImageFilter.cpp View 1 chunk +1 line, -1 line 0 comments Download
M src/gpu/SkGpuDevice.cpp View 1 2 3 4 5 6 2 chunks +6 lines, -2 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
Stephen White
I think this is ready for review. The easiest fix for the problem we were ...
5 years, 10 months ago (2013-08-26 21:00:51 UTC) #1
reed1
afaik, cpu side lgtm deferring to brian for gpu stuff https://codereview.chromium.org/23295017/diff/31001/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/23295017/diff/31001/src/core/SkImageFilter.cpp#newcode163 ...
5 years, 10 months ago (2013-08-27 19:54:44 UTC) #2
bsalomon
I thought we had a bicubic image filter too... does that not need to change? ...
5 years, 10 months ago (2013-08-27 20:00:02 UTC) #3
Stephen White
https://codereview.chromium.org/23295017/diff/31001/src/core/SkImageFilter.cpp File src/core/SkImageFilter.cpp (right): https://codereview.chromium.org/23295017/diff/31001/src/core/SkImageFilter.cpp#newcode105 src/core/SkImageFilter.cpp:105: return this->asNewEffect(NULL, NULL, SkMatrix::I()); On 2013/08/27 20:00:02, bsalomon wrote: ...
5 years, 10 months ago (2013-08-27 21:21:38 UTC) #4
Stephen White
On 2013/08/27 21:21:38, Stephen White wrote: > https://codereview.chromium.org/23295017/diff/31001/src/core/SkImageFilter.cpp > File src/core/SkImageFilter.cpp (right): > > https://codereview.chromium.org/23295017/diff/31001/src/core/SkImageFilter.cpp#newcode105 ...
5 years, 10 months ago (2013-08-27 21:35:13 UTC) #5
Stephen White
5 years, 10 months ago (2013-08-27 21:37:10 UTC) #6
Message was sent while issue was closed.
Committed patchset #11 manually as r10961 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698