Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1566943002:
SkTreatAsSprite should take AA into account (Closed)
DescriptionSkTreatAsSprite should take AA into account
Currently we always call SkTreatAsSprite with 0 subpixel bits, which means
subpixel translations are ignored. This is incorrect for the anti-aliased
case (drawSprite always pixel-snaps, so we lose edge AA).
The CL updates SkTreatAsSprite to take an SkPaint argument and use 8 subpixel
bits when AA is requested.
Also remove unused SkTreatAsSpriteFilter.
BUG=skia:4761
R=reed@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1566943002
Committed: https://skia.googlesource.com/skia/+/983dc2541a729609037a05eba731b3eb9788c517
Committed: https://skia.googlesource.com/skia/+/c7e211acd0c9201688de7ff0c9a2271c67440adf
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 2
Patch Set 4 : 4 subpixel bits #
Total comments: 2
Patch Set 5 : comment #Patch Set 6 : SkLeftShift #
Messages
Total messages: 37 (17 generated)
Description was changed from ========== SkTreatAsSprite should take AA into account BUG=skia:4761 ========== to ========== SkTreatAsSprite should take AA into account BUG=skia:4761 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== SkTreatAsSprite should take AA into account BUG=skia:4761 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkTreatAsSprite should take AA into account Currently we always call SkTreatAsSprite with 0 subpixel bits, which means subpixel translations are ignored. This is incorrect for the anti-aliased case (drawSprite always pixel-snaps, so we lose edge AA). The CL updates SkTreatAsSprite to take an SkPaint argument and use 8 subpixel bits when AA is requested. Also remove unused SkTreatAsSpriteFilter. BUG=skia:4761 R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
fmalita@chromium.org changed reviewers: + reed@google.com
This also improves GM:image_scale_aligned: the anti-aliased cases are now properly aligned.
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1566943002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1566943002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/01/07 03:09:45, f(malita) wrote: > This also improves GM:image_scale_aligned: the anti-aliased cases are now > properly aligned. Try GM diffs: https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
https://codereview.chromium.org/1566943002/diff/40001/src/core/SkMatrix.cpp File src/core/SkMatrix.cpp (right): https://codereview.chromium.org/1566943002/diff/40001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:1628: static const unsigned kAntiAliasSubpixelBits = 8; we have various AA techniques: paths get 2 subpixel bits per coordinate, rects are special and get 8. I wonder if we can lower this const to 2 or 4 and see if things still look pretty. If they do, we might get a perf win. Speaking of that, we should consider realistic ways to measure the perf win of treating-as-sprite, just to be sure all of this complexity is actually worth it.
https://codereview.chromium.org/1566943002/diff/40001/src/core/SkMatrix.cpp File src/core/SkMatrix.cpp (right): https://codereview.chromium.org/1566943002/diff/40001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:1628: static const unsigned kAntiAliasSubpixelBits = 8; On 2016/01/07 14:40:50, reed1 wrote: > we have various AA techniques: paths get 2 subpixel bits per coordinate, rects > are special and get 8. I wonder if we can lower this const to 2 or 4 and see if > things still look pretty. If they do, we might get a perf win. 4 seems to work just as well, and it matches the prev filter subpixel value. Updated. > Speaking of that, we should consider realistic ways to measure the perf win of > treating-as-sprite, just to be sure all of this complexity is actually worth it. Agreed, we should add a bench if we don't already have one.
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1566943002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1566943002/60001
lgtm
https://codereview.chromium.org/1566943002/diff/60001/src/core/SkMatrix.cpp File src/core/SkMatrix.cpp (right): https://codereview.chromium.org/1566943002/diff/60001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:1627: static const unsigned kAntiAliasSubpixelBits = 4; maybe a comment, like "our path aa is 2-bits, and our rect aa is 8, so we could use 8, but in practice 4 seems enough (still looks smooth) and allows more slightly fractional cases to fall into the fast (sprite) case."
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I haven't noticed any locally, but there's a chance this will yield some minor layout test diffs. I'll keep an eye on the bots and cowboy-roll if needed. https://codereview.chromium.org/1566943002/diff/60001/src/core/SkMatrix.cpp File src/core/SkMatrix.cpp (right): https://codereview.chromium.org/1566943002/diff/60001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:1627: static const unsigned kAntiAliasSubpixelBits = 4; On 2016/01/07 15:29:59, reed1 wrote: > maybe a comment, like "our path aa is 2-bits, and our rect aa is 8, so we could > use 8, but in practice 4 seems enough (still looks smooth) and allows more > slightly fractional cases to fall into the fast (sprite) case." Done.
The CQ bit was checked by fmalita@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/1566943002/#ps80001 (title: "comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1566943002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1566943002/80001
Message was sent while issue was closed.
Description was changed from ========== SkTreatAsSprite should take AA into account Currently we always call SkTreatAsSprite with 0 subpixel bits, which means subpixel translations are ignored. This is incorrect for the anti-aliased case (drawSprite always pixel-snaps, so we lose edge AA). The CL updates SkTreatAsSprite to take an SkPaint argument and use 8 subpixel bits when AA is requested. Also remove unused SkTreatAsSpriteFilter. BUG=skia:4761 R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkTreatAsSprite should take AA into account Currently we always call SkTreatAsSprite with 0 subpixel bits, which means subpixel translations are ignored. This is incorrect for the anti-aliased case (drawSprite always pixel-snaps, so we lose edge AA). The CL updates SkTreatAsSprite to take an SkPaint argument and use 8 subpixel bits when AA is requested. Also remove unused SkTreatAsSpriteFilter. BUG=skia:4761 R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/983dc2541a729609037a05eba731b3eb9788c517 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/983dc2541a729609037a05eba731b3eb9788c517
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1569873003/ by reed@google.com. The reason for reverting is: Need to use SkLeftShift since the arg could be negative.
Message was sent while issue was closed.
Description was changed from ========== SkTreatAsSprite should take AA into account Currently we always call SkTreatAsSprite with 0 subpixel bits, which means subpixel translations are ignored. This is incorrect for the anti-aliased case (drawSprite always pixel-snaps, so we lose edge AA). The CL updates SkTreatAsSprite to take an SkPaint argument and use 8 subpixel bits when AA is requested. Also remove unused SkTreatAsSpriteFilter. BUG=skia:4761 R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/983dc2541a729609037a05eba731b3eb9788c517 ========== to ========== SkTreatAsSprite should take AA into account Currently we always call SkTreatAsSprite with 0 subpixel bits, which means subpixel translations are ignored. This is incorrect for the anti-aliased case (drawSprite always pixel-snaps, so we lose edge AA). The CL updates SkTreatAsSprite to take an SkPaint argument and use 8 subpixel bits when AA is requested. Also remove unused SkTreatAsSpriteFilter. BUG=skia:4761 R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/983dc2541a729609037a05eba731b3eb9788c517 ==========
On 2016/01/07 17:15:13, reed1 wrote: > A revert of this CL (patchset #5 id:80001) has been created in > https://codereview.chromium.org/1569873003/ by mailto:reed@google.com. > > The reason for reverting is: Need to use SkLeftShift since the arg could be > negative. Updated, PTAL.
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1566943002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1566943002/100001
The CQ bit was unchecked by fmalita@chromium.org
On 2016/01/07 17:27:17, f(malita) wrote: > On 2016/01/07 17:15:13, reed1 wrote: > > A revert of this CL (patchset #5 id:80001) has been created in > > https://codereview.chromium.org/1569873003/ by mailto:reed@google.com. > > > > The reason for reverting is: Need to use SkLeftShift since the arg could be > > negative. > > Updated, PTAL. Now passing the ASAN bot (https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX...), I think this is ready to reland.
mtklein@google.com changed reviewers: + mtklein@google.com
The CQ bit was checked by mtklein@google.com
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/1566943002/#ps100001 (title: "SkLeftShift")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1566943002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1566943002/100001
Message was sent while issue was closed.
Description was changed from ========== SkTreatAsSprite should take AA into account Currently we always call SkTreatAsSprite with 0 subpixel bits, which means subpixel translations are ignored. This is incorrect for the anti-aliased case (drawSprite always pixel-snaps, so we lose edge AA). The CL updates SkTreatAsSprite to take an SkPaint argument and use 8 subpixel bits when AA is requested. Also remove unused SkTreatAsSpriteFilter. BUG=skia:4761 R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/983dc2541a729609037a05eba731b3eb9788c517 ========== to ========== SkTreatAsSprite should take AA into account Currently we always call SkTreatAsSprite with 0 subpixel bits, which means subpixel translations are ignored. This is incorrect for the anti-aliased case (drawSprite always pixel-snaps, so we lose edge AA). The CL updates SkTreatAsSprite to take an SkPaint argument and use 8 subpixel bits when AA is requested. Also remove unused SkTreatAsSpriteFilter. BUG=skia:4761 R=reed@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/983dc2541a729609037a05eba731b3eb9788c517 Committed: https://skia.googlesource.com/skia/+/c7e211acd0c9201688de7ff0c9a2271c67440adf ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/c7e211acd0c9201688de7ff0c9a2271c67440adf |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
