Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1899263002:
post apply non-scale transforms after imagefilters have run (Closed)
Descriptionpost apply non-scale transforms after imagefilters have run
may choose to eliminate the final matrix-filter buffer before the sprite blit, but at the moment want to defer that change to a 2nd CL.
heavily inspired by https://codereview.chromium.org/1140943004
Need these CLs to land first:
https://codereview.chromium.org/1898193005/#
https://codereview.chromium.org/1902253003/
BUG=skia:3288
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1899263002
Committed: https://skia.googlesource.com/skia/+/8c30a8196dd5903d2d23b4d0a5dc888e802bf698
Patch Set 1 #Patch Set 2 : now with gm #Patch Set 3 : make aa a runtime option #Patch Set 4 : #Patch Set 5 : fission out gm to separate pre-CL #
Total comments: 2
Patch Set 6 : removed troublesome aa parameter (that was added earlier) #
Total comments: 3
Messages
Total messages: 30 (12 generated)
Description was changed from ========== post apply non-scale transforms after imagefilters have run heavily inspired by https://codereview.chromium.org/1140943004 BUG=skia:3288 ========== to ========== post apply non-scale transforms after imagefilters have run heavily inspired by https://codereview.chromium.org/1140943004 BUG=skia:3288 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
reed@google.com changed reviewers: + robertphillips@google.com, senorblanco@chromium.org
will now add a test for aa edges, since that seems to be what caused the earlier revert
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/1899263002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899263002/1
Description was changed from ========== post apply non-scale transforms after imagefilters have run heavily inspired by https://codereview.chromium.org/1140943004 BUG=skia:3288 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== post apply non-scale transforms after imagefilters have run may choose to eliminate the final matrix-filter buffer before the sprite blit, but at the moment want to defer that change to a 2nd CL. heavily inspired by https://codereview.chromium.org/1140943004 BUG=skia:3288 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was unchecked by reed@google.com
Don't we need a new GM to make this doesn't just get reverted again?
Description was changed from ========== post apply non-scale transforms after imagefilters have run may choose to eliminate the final matrix-filter buffer before the sprite blit, but at the moment want to defer that change to a 2nd CL. heavily inspired by https://codereview.chromium.org/1140943004 BUG=skia:3288 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== post apply non-scale transforms after imagefilters have run may choose to eliminate the final matrix-filter buffer before the sprite blit, but at the moment want to defer that change to a 2nd CL. heavily inspired by https://codereview.chromium.org/1140943004 Need these CLs to land first: https://codereview.chromium.org/1898193005/# https://codereview.chromium.org/1902253003/ BUG=skia:3288 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
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/1899263002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899263002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ptal
With more testing, I think it is a fool's paradise to think we can "promise" to sometimes *not* AA the edge of the image during the affine stage ... since we filter it, and the buffer may have a border of transparency, the visual effect is that of AA, even if the caller's Paint didn't specify it. I plan to remove the feature of trying to infer the intent from the primitive's paint, and just always AA the edges (as we always filter the interior already). This at least will be predictable and somewhat self-consistent (filtering interior and edges together).
On 2016/04/20 14:54:21, reed1 wrote: > With more testing, I think it is a fool's paradise to think we can "promise" to > sometimes *not* AA the edge of the image during the affine stage ... since we > filter it, and the buffer may have a border of transparency, the visual effect > is that of AA, even if the caller's Paint didn't specify it. > > I plan to remove the feature of trying to infer the intent from the primitive's > paint, and just always AA the edges (as we always filter the interior already). > This at least will be predictable and somewhat self-consistent (filtering > interior and edges together). This sounds reasonable (especially since this is something of a niche case anyway), but I'm just trying to understand the consequences: if antialiasing is off in the SkPaint when we do a saveLayer(), we'll still do AA (and filtering) when drawing the primitive in the SkMatrixImageFilter? Or does this also affect the case where antialiasing was on in the paint where saveLayer() is called? What do Chrome's layout test results look like in that case? Are they better, worse, "just different"?
My comment is only about the drawImage call that is issued by the matrixfilter, not the original primitive/content (which always respects its paint). The quoted reason for the revert was unantialiased edges on the rotated result. This CL started to fix that by only adding AA if the layer's paint had AA. However, my recent experiments lead me to think we should always AA during the rotate.
Since the revert didn't like "hard edges", I am guessing the effect on layouttests will be that it will be better/same.
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/1899263002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899263002/100001
https://codereview.chromium.org/1899263002/diff/80001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/1899263002/diff/80001/include/core/SkCanvas.h... include/core/SkCanvas.h:1474: void internalDrawPaint(const SkPaint& paint); This flag is going away right?
https://codereview.chromium.org/1899263002/diff/100001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1899263002/diff/100001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1229: SkFilterQuality::kLow_SkFilterQuality, Blink currently uses High quality in SkiaImageFilterBuilder::buildTransform(). Won't using Low here cause a quality loss?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1899263002/diff/80001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/1899263002/diff/80001/include/core/SkCanvas.h... include/core/SkCanvas.h:1474: void internalDrawPaint(const SkPaint& paint); On 2016/04/20 18:34:53, robertphillips wrote: > This flag is going away right? Done in #6 https://codereview.chromium.org/1899263002/diff/100001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1899263002/diff/100001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1229: SkFilterQuality::kLow_SkFilterQuality, On 2016/04/20 18:39:34, Stephen White wrote: > Blink currently uses High quality in SkiaImageFilterBuilder::buildTransform(). > Won't using Low here cause a quality loss? My thinking was that since we decompose the scale, the filter should just see rotations etc, meaning that there should be minimal advantage to using something fancier than bilerp (e.g. High or Medium). Additionally, those latter two cache their labors, which will have zero value here, since we don't have a reliable key for a saveLayer as src. Are you ok if we try with Low, and examine the layout results, and see if they are acceptable?
LGTM https://codereview.chromium.org/1899263002/diff/100001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1899263002/diff/100001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:1229: SkFilterQuality::kLow_SkFilterQuality, On 2016/04/20 19:42:50, reed1 wrote: > On 2016/04/20 18:39:34, Stephen White wrote: > > Blink currently uses High quality in SkiaImageFilterBuilder::buildTransform(). > > Won't using Low here cause a quality loss? > > My thinking was that since we decompose the scale, the filter should just see > rotations etc, meaning that there should be minimal advantage to using something > fancier than bilerp (e.g. High or Medium). Additionally, those latter two cache > their labors, which will have zero value here, since we don't have a reliable > key for a saveLayer as src. > > Are you ok if we try with Low, and examine the layout results, and see if they > are acceptable? OK, let's give it a try.
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/1899263002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1899263002/100001
Message was sent while issue was closed.
Description was changed from ========== post apply non-scale transforms after imagefilters have run may choose to eliminate the final matrix-filter buffer before the sprite blit, but at the moment want to defer that change to a 2nd CL. heavily inspired by https://codereview.chromium.org/1140943004 Need these CLs to land first: https://codereview.chromium.org/1898193005/# https://codereview.chromium.org/1902253003/ BUG=skia:3288 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== post apply non-scale transforms after imagefilters have run may choose to eliminate the final matrix-filter buffer before the sprite blit, but at the moment want to defer that change to a 2nd CL. heavily inspired by https://codereview.chromium.org/1140943004 Need these CLs to land first: https://codereview.chromium.org/1898193005/# https://codereview.chromium.org/1902253003/ BUG=skia:3288 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/8c30a8196dd5903d2d23b4d0a5dc888e802bf698 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/8c30a8196dd5903d2d23b4d0a5dc888e802bf698 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
