Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1570133003:
Fix SkTileImageFilter when srcRect is a superset of bitmap bounds. (Closed)
|
Created:
4 years, 11 months ago by Stephen White Modified:
4 years, 11 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionFix SkTileImageFilter when srcRect is a superset of bitmap bounds.
If the input bitmap passed to SkTileImageFilter does not fill the
srcRect, we were tiling this incorrectly (see the first sample
from tileimage filter -- it draws from a srcRect of 12,12 50x50
to a dstRect of 0,0 50x50. There should be no tiling at all
in this case!)
In order to fix this, we need to pad the bitmap out to srcRect,
and tile with that. In order to tile correctly in the GPU case,
we need to request a tileable texture.
NOTE: this will change the results of the tileimagefilter GM (correctness,
and added src / dest rects).
BUG=skia:4774
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1570133003
Committed: https://skia.googlesource.com/skia/+/a9fbd1676cf8bd0078c5be1f1c4abd4c76ea60ad
Patch Set 1 #Patch Set 2 : Win32 fix #
Total comments: 7
Patch Set 3 : Replace bool tile param w/enum; remove clear(); fix formatting #Patch Set 4 : Update to ToT #
Total comments: 4
Patch Set 5 : Fix 100-col issues #
Messages
Total messages: 40 (18 generated)
Description was changed from ========== Fix SkTileImageFilter when srcRect is a superset of bitmap bounds. If the input bitmap passed to SkTileImageFilter does not fill the srcRect, we were tiling this incorrectly (see the first sample from tileimage filter -- it draws from a srcRect of 12,12 50x50 to a dstRect of 0,0 50x50. There should be no tiling at all in this case!) In order to fix this, we need to pad the bitmap out to srcRect, and tile with that. In order to tile correctly in the GPU case, we need to request a tileable texture. BUG=skia:4774 ========== to ========== Fix SkTileImageFilter when srcRect is a superset of bitmap bounds. If the input bitmap passed to SkTileImageFilter does not fill the srcRect, we were tiling this incorrectly (see the first sample from tileimage filter -- it draws from a srcRect of 12,12 50x50 to a dstRect of 0,0 50x50. There should be no tiling at all in this case!) In order to fix this, we need to pad the bitmap out to srcRect, and tile with that. In order to tile correctly in the GPU case, we need to request a tileable texture. BUG=skia:4774 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Fix SkTileImageFilter when srcRect is a superset of bitmap bounds. If the input bitmap passed to SkTileImageFilter does not fill the srcRect, we were tiling this incorrectly (see the first sample from tileimage filter -- it draws from a srcRect of 12,12 50x50 to a dstRect of 0,0 50x50. There should be no tiling at all in this case!) In order to fix this, we need to pad the bitmap out to srcRect, and tile with that. In order to tile correctly in the GPU case, we need to request a tileable texture. BUG=skia:4774 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Fix SkTileImageFilter when srcRect is a superset of bitmap bounds. If the input bitmap passed to SkTileImageFilter does not fill the srcRect, we were tiling this incorrectly (see the first sample from tileimage filter -- it draws from a srcRect of 12,12 50x50 to a dstRect of 0,0 50x50. There should be no tiling at all in this case!) In order to fix this, we need to pad the bitmap out to srcRect, and tile with that. In order to tile correctly in the GPU case, we need to request a tileable texture. BUG=skia:4774 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
senorblanco@chromium.org changed reviewers: + reed@google.com
The CQ bit was checked by senorblanco@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/1570133003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1570133003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
Description was changed from ========== Fix SkTileImageFilter when srcRect is a superset of bitmap bounds. If the input bitmap passed to SkTileImageFilter does not fill the srcRect, we were tiling this incorrectly (see the first sample from tileimage filter -- it draws from a srcRect of 12,12 50x50 to a dstRect of 0,0 50x50. There should be no tiling at all in this case!) In order to fix this, we need to pad the bitmap out to srcRect, and tile with that. In order to tile correctly in the GPU case, we need to request a tileable texture. BUG=skia:4774 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Fix SkTileImageFilter when srcRect is a superset of bitmap bounds. If the input bitmap passed to SkTileImageFilter does not fill the srcRect, we were tiling this incorrectly (see the first sample from tileimage filter -- it draws from a srcRect of 12,12 50x50 to a dstRect of 0,0 50x50. There should be no tiling at all in this case!) In order to fix this, we need to pad the bitmap out to srcRect, and tile with that. In order to tile correctly in the GPU case, we need to request a tileable texture. NOTE: this will change the results of the tileimagefilter GM (correctness, and added src / dest rects). BUG=skia:4774 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by senorblanco@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/1570133003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1570133003/20001
reed@: PTAL. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
reed@: PTAL. Thanks!
reed@google.com changed reviewers: + bsalomon@google.com, robertphillips@google.com
I think an enum is preferable to a bool. https://codereview.chromium.org/1570133003/diff/20001/include/core/SkImageFil... File include/core/SkImageFilter.h (right): https://codereview.chromium.org/1570133003/diff/20001/include/core/SkImageFil... include/core/SkImageFilter.h:114: virtual SkBaseDevice* createDevice(int width, int height, bool tile = false) = 0; Does this mean "make the device tileable"? If so, lest add a comment or a more descriptive name https://codereview.chromium.org/1570133003/diff/20001/src/effects/SkTileImage... File src/effects/SkTileImageFilter.cpp (right): https://codereview.chromium.org/1570133003/diff/20001/src/effects/SkTileImage... src/effects/SkTileImageFilter.cpp:74: canvas.clear(0x0); is the clear needed? I thought our devices were already cleared (but not sure) https://codereview.chromium.org/1570133003/diff/20001/src/image/SkImage.cpp File src/image/SkImage.cpp (right): https://codereview.chromium.org/1570133003/diff/20001/src/image/SkImage.cpp#n... src/image/SkImage.cpp:119: SkBaseDevice* createDevice(int width, int height, bool tile = false) override { I think default args on overrides are very tricky, since the "default" may not agree w/ the base.
https://codereview.chromium.org/1570133003/diff/20001/src/effects/SkTileImage... File src/effects/SkTileImageFilter.cpp (right): https://codereview.chromium.org/1570133003/diff/20001/src/effects/SkTileImage... src/effects/SkTileImageFilter.cpp:68: } else { \n somewhere ? Maybe "static bool kWillTiledResult = true;" ?
A named constant is nicer, but doesn't stop callers from just passing true/false/some_bool_expression
Note that I've replaced the bool param with a copy of the TileUsage param from SkDevice. If we're ok with adding a compile-time dependency (there's already a semantic one, but SkBaseDevice is forward declared), I could just re-use the one from SkDevice. Alternatively, we could move TileUsage enum up to a common place (SkSurface maybe, although it isn't used there.) and use it from both SkDevice and SkImageFilter. https://codereview.chromium.org/1570133003/diff/20001/include/core/SkImageFil... File include/core/SkImageFilter.h (right): https://codereview.chromium.org/1570133003/diff/20001/include/core/SkImageFil... include/core/SkImageFilter.h:114: virtual SkBaseDevice* createDevice(int width, int height, bool tile = false) = 0; On 2016/01/10 02:25:02, reed1 wrote: > Does this mean "make the device tileable"? If so, lest add a comment or a more > descriptive name Done. https://codereview.chromium.org/1570133003/diff/20001/src/effects/SkTileImage... File src/effects/SkTileImageFilter.cpp (right): https://codereview.chromium.org/1570133003/diff/20001/src/effects/SkTileImage... src/effects/SkTileImageFilter.cpp:68: } else { On 2016/01/11 13:31:54, robertphillips wrote: > \n somewhere ? > Maybe "static bool kWillTiledResult = true;" ? Done. https://codereview.chromium.org/1570133003/diff/20001/src/effects/SkTileImage... src/effects/SkTileImageFilter.cpp:74: canvas.clear(0x0); On 2016/01/10 02:25:02, reed1 wrote: > is the clear needed? I thought our devices were already cleared (but not sure) I wasn't sure either. Removed and the tests seem ok.
The CQ bit was checked by senorblanco@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/1570133003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1570133003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac10.9-Clang-Arm7-Debug-iOS-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac10.9-Clang-...) Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
The CQ bit was checked by senorblanco@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/1570133003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1570133003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping: new patch up.
api lgtm
lgtm https://codereview.chromium.org/1570133003/diff/60001/include/core/SkImageFil... File include/core/SkImageFilter.h (right): https://codereview.chromium.org/1570133003/diff/60001/include/core/SkImageFil... include/core/SkImageFilter.h:118: \n ? https://codereview.chromium.org/1570133003/diff/60001/include/core/SkImageFil... include/core/SkImageFilter.h:131: \n ?
https://codereview.chromium.org/1570133003/diff/60001/include/core/SkImageFil... File include/core/SkImageFilter.h (right): https://codereview.chromium.org/1570133003/diff/60001/include/core/SkImageFil... include/core/SkImageFilter.h:118: On 2016/01/11 20:47:12, robertphillips wrote: > \n ? Done. https://codereview.chromium.org/1570133003/diff/60001/include/core/SkImageFil... include/core/SkImageFilter.h:131: On 2016/01/11 20:47:12, robertphillips wrote: > \n ? Done.
The CQ bit was checked by senorblanco@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/1570133003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1570133003/80001
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 senorblanco@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com, reed@google.com Link to the patchset: https://codereview.chromium.org/1570133003/#ps80001 (title: "Fix 100-col issues")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1570133003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1570133003/80001
Message was sent while issue was closed.
Description was changed from ========== Fix SkTileImageFilter when srcRect is a superset of bitmap bounds. If the input bitmap passed to SkTileImageFilter does not fill the srcRect, we were tiling this incorrectly (see the first sample from tileimage filter -- it draws from a srcRect of 12,12 50x50 to a dstRect of 0,0 50x50. There should be no tiling at all in this case!) In order to fix this, we need to pad the bitmap out to srcRect, and tile with that. In order to tile correctly in the GPU case, we need to request a tileable texture. NOTE: this will change the results of the tileimagefilter GM (correctness, and added src / dest rects). BUG=skia:4774 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Fix SkTileImageFilter when srcRect is a superset of bitmap bounds. If the input bitmap passed to SkTileImageFilter does not fill the srcRect, we were tiling this incorrectly (see the first sample from tileimage filter -- it draws from a srcRect of 12,12 50x50 to a dstRect of 0,0 50x50. There should be no tiling at all in this case!) In order to fix this, we need to pad the bitmap out to srcRect, and tile with that. In order to tile correctly in the GPU case, we need to request a tileable texture. NOTE: this will change the results of the tileimagefilter GM (correctness, and added src / dest rects). BUG=skia:4774 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/a9fbd1676cf8bd0078c5be1f1c4abd4c76ea60ad ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/a9fbd1676cf8bd0078c5be1f1c4abd4c76ea60ad |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
