|
|
Descriptiondetect when we can filter bitmaps/images directly, w/o a tmp layer
visual bench run on Mac Pro
curr/maxrss loops min median mean max stddev samples config bench
100/100 MB 16 412µs 413µs 413µs 414µs 0% ▄▁▇▄▄▄▄█▄▃▅ gpu warmupbench
101/102 MB 32 547µs 548µs 611µs 1.24ms 34% █▁▁▁▁▁▁▁▁▁▁ gpu image-filter-sprite-draw-image
102/103 MB 32 547µs 548µs 721µs 1.23ms 41% █▁▇▁▁█▁▁▁▁▁ gpu image-filter-sprite-draw-bitmap
103/103 MB 64 546µs 546µs 546µs 547µs 0% ▆▄▂▁▇█▅▇▅▇▃ gpu image-filter-sprite-draw-sprite
Should have no effect on Chrome while SK_SUPPORT_LEGACY_LAYER_BITMAP_IMAGEFILTERS is defined (which it is in chrome)
BUG=skia:1073
Committed: https://skia.googlesource.com/skia/+/262a71b7f95ce98ff3dd8dba845afbd724470903
Patch Set 1 #Patch Set 2 : #
Total comments: 10
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : remove dead (testing) code #
Total comments: 3
Patch Set 6 : update clip-predicate w/ bug number, and add bug to this CL #
Total comments: 6
Patch Set 7 : change to clearer names (drawBitmapAsSprite) #Patch Set 8 : use SkTreadAsSprite #
Total comments: 2
Patch Set 9 : move include to the top #
Messages
Total messages: 62 (26 generated)
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/1491293002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491293002/1
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/1491293002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491293002/20001
reed@google.com changed reviewers: + fmalita@chromium.org, robertphillips@google.com, senorblanco@google.com
Show some small diffs in GMs (e.g. Erode and MatrixConvolution). Investigating them now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
gm to exercise some of the small GM diffs I noticed w/ this CL. https://codereview.chromium.org/1498523002/
precursor for chrome: https://codereview.chromium.org/1489163004/
https://codereview.chromium.org/1491293002/diff/20001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1491293002/diff/20001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:599: while (looper.next(SkDrawFilter::kBitmap_Type)) { \ indent this ? https://codereview.chromium.org/1491293002/diff/20001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:1419: iter.fDevice->filterSprite(iter, bitmap, pos.x(), pos.y(), looper.paint() -> pnt & all on one line ?
senorblanco@chromium.org changed reviewers: + senorblanco@chromium.org
Is this supposed to help you move forward with your Chrome patch? I'm not sure I understand how, if it only works for the raster path (if I'm right about that). Perhaps we should chat about it a bit. https://codereview.chromium.org/1491293002/diff/20001/include/core/SkDevice.h File include/core/SkDevice.h (right): https://codereview.chromium.org/1491293002/diff/20001/include/core/SkDevice.h... include/core/SkDevice.h:386: * - the device itself does not want to handle the filter So this will only work for the raster path, then? I think all the GPU filters are handled by SkGpuDevice. https://codereview.chromium.org/1491293002/diff/20001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1491293002/diff/20001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:1415: if (true) { Remove this? https://codereview.chromium.org/1491293002/diff/20001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:1419: iter.fDevice->filterSprite(iter, bitmap, pos.x(), pos.y(), Do we have any benches which exercise these different codepaths? If not, could we?
Good catch. I was just blindly replicating the pattern from drawSprite, and forgot that the cpu-magic happens in the device method (but only for sprite). Will amend.
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/1491293002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491293002/40001
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
The CQ bit was unchecked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491293002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491293002/40001
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2015-12-04 02:16 UTC
Description was changed from ========== detect when we can filter bitmaps/images directly, w/o a tmp layer BUG=skia: ========== to ========== detect when we can filter bitmaps/images directly, w/o a tmp layer visual bench run on Mac Pro curr/maxrss loops min median mean max stddev samples config bench 100/100 MB 16 412µs 413µs 413µs 414µs 0% ▄▁▇▄▄▄▄█▄▃▅ gpu warmupbench 101/102 MB 32 547µs 548µs 611µs 1.24ms 34% █▁▁▁▁▁▁▁▁▁▁ gpu image-filter-sprite-draw-image 102/103 MB 32 547µs 548µs 721µs 1.23ms 41% █▁▇▁▁█▁▁▁▁▁ gpu image-filter-sprite-draw-bitmap 103/103 MB 64 546µs 546µs 546µs 547µs 0% ▆▄▂▁▇█▅▇▅▇▃ gpu image-filter-sprite-draw-sprite BUG=skia: ==========
ptal https://codereview.chromium.org/1491293002/diff/20001/include/core/SkDevice.h File include/core/SkDevice.h (right): https://codereview.chromium.org/1491293002/diff/20001/include/core/SkDevice.h... include/core/SkDevice.h:386: * - the device itself does not want to handle the filter On 2015/12/02 23:45:15, Stephen White wrote: > So this will only work for the raster path, then? I think all the GPU filters > are handled by SkGpuDevice. Fixed. https://codereview.chromium.org/1491293002/diff/20001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1491293002/diff/20001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:599: while (looper.next(SkDrawFilter::kBitmap_Type)) { \ On 2015/12/02 22:37:18, robertphillips wrote: > indent this ? Done. https://codereview.chromium.org/1491293002/diff/20001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:1415: if (true) { On 2015/12/02 23:45:15, Stephen White wrote: > Remove this? Done. https://codereview.chromium.org/1491293002/diff/20001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:1419: iter.fDevice->filterSprite(iter, bitmap, pos.x(), pos.y(), On 2015/12/02 22:37:18, robertphillips wrote: > looper.paint() -> pnt & all on one line ? Done. https://codereview.chromium.org/1491293002/diff/20001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:1419: iter.fDevice->filterSprite(iter, bitmap, pos.x(), pos.y(), On 2015/12/02 23:45:15, Stephen White wrote: > Do we have any benches which exercise these different codepaths? If not, could > we? Done.
Description was changed from ========== detect when we can filter bitmaps/images directly, w/o a tmp layer visual bench run on Mac Pro curr/maxrss loops min median mean max stddev samples config bench 100/100 MB 16 412µs 413µs 413µs 414µs 0% ▄▁▇▄▄▄▄█▄▃▅ gpu warmupbench 101/102 MB 32 547µs 548µs 611µs 1.24ms 34% █▁▁▁▁▁▁▁▁▁▁ gpu image-filter-sprite-draw-image 102/103 MB 32 547µs 548µs 721µs 1.23ms 41% █▁▇▁▁█▁▁▁▁▁ gpu image-filter-sprite-draw-bitmap 103/103 MB 64 546µs 546µs 546µs 547µs 0% ▆▄▂▁▇█▅▇▅▇▃ gpu image-filter-sprite-draw-sprite BUG=skia: ========== to ========== detect when we can filter bitmaps/images directly, w/o a tmp layer visual bench run on Mac Pro curr/maxrss loops min median mean max stddev samples config bench 100/100 MB 16 412µs 413µs 413µs 414µs 0% ▄▁▇▄▄▄▄█▄▃▅ gpu warmupbench 101/102 MB 32 547µs 548µs 611µs 1.24ms 34% █▁▁▁▁▁▁▁▁▁▁ gpu image-filter-sprite-draw-image 102/103 MB 32 547µs 548µs 721µs 1.23ms 41% █▁▇▁▁█▁▁▁▁▁ gpu image-filter-sprite-draw-bitmap 103/103 MB 64 546µs 546µs 546µs 547µs 0% ▆▄▂▁▇█▅▇▅▇▃ gpu image-filter-sprite-draw-sprite Should have no effect on Chrome while SK_SUPPORT_LEGACY_LAYER_BITMAP_IMAGEFILTERS is defined (which it is in chrome) BUG=skia: ==========
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/1491293002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491293002/80001
The CQ bit was unchecked by reed@google.com
lgtm
https://codereview.chromium.org/1491293002/diff/80001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1491293002/diff/80001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:2200: } Add a bug # ?
Description was changed from ========== detect when we can filter bitmaps/images directly, w/o a tmp layer visual bench run on Mac Pro curr/maxrss loops min median mean max stddev samples config bench 100/100 MB 16 412µs 413µs 413µs 414µs 0% ▄▁▇▄▄▄▄█▄▃▅ gpu warmupbench 101/102 MB 32 547µs 548µs 611µs 1.24ms 34% █▁▁▁▁▁▁▁▁▁▁ gpu image-filter-sprite-draw-image 102/103 MB 32 547µs 548µs 721µs 1.23ms 41% █▁▇▁▁█▁▁▁▁▁ gpu image-filter-sprite-draw-bitmap 103/103 MB 64 546µs 546µs 546µs 547µs 0% ▆▄▂▁▇█▅▇▅▇▃ gpu image-filter-sprite-draw-sprite Should have no effect on Chrome while SK_SUPPORT_LEGACY_LAYER_BITMAP_IMAGEFILTERS is defined (which it is in chrome) BUG=skia: ========== to ========== detect when we can filter bitmaps/images directly, w/o a tmp layer visual bench run on Mac Pro curr/maxrss loops min median mean max stddev samples config bench 100/100 MB 16 412µs 413µs 413µs 414µs 0% ▄▁▇▄▄▄▄█▄▃▅ gpu warmupbench 101/102 MB 32 547µs 548µs 611µs 1.24ms 34% █▁▁▁▁▁▁▁▁▁▁ gpu image-filter-sprite-draw-image 102/103 MB 32 547µs 548µs 721µs 1.23ms 41% █▁▇▁▁█▁▁▁▁▁ gpu image-filter-sprite-draw-bitmap 103/103 MB 64 546µs 546µs 546µs 547µs 0% ▆▄▂▁▇█▅▇▅▇▃ gpu image-filter-sprite-draw-sprite Should have no effect on Chrome while SK_SUPPORT_LEGACY_LAYER_BITMAP_IMAGEFILTERS is defined (which it is in chrome) BUG=skia:1073 ==========
I would like Stephen to weigh in too though
ptal
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/1491293002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491293002/100001
The CQ bit was unchecked by reed@google.com
Using the bench to verify was a great idea. Are there other concerns/questions?
What are the Chrome issues that prevent landing this without an #ifdef? https://codereview.chromium.org/1491293002/diff/80001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/1491293002/diff/80001/include/core/SkCanvas.h... include/core/SkCanvas.h:1437: bool canCallFilterSprite(const SkRect& bounds, const SkPaint&); I'm wondering why this is defined on SkCanvas. Are there (going to be) public callers? https://codereview.chromium.org/1491293002/diff/100001/include/core/SkDevice.h File include/core/SkDevice.h (right): https://codereview.chromium.org/1491293002/diff/100001/include/core/SkDevice.... include/core/SkDevice.h:383: void filterSprite(const SkDraw&, const SkBitmap&, int x, int y, const SkPaint&); Since we're getting rid of drawSprite(), could we just call this filterBitmap(), so it's not the lone Sprite-related thingy? Come to think of it, this name is misleading wrt filterImage(), which actually returns the filtered image (vs. this one, which draws it to the device). Maybe drawBitmapAsSprite? drawBitmapFast()? fastDrawBitmap()? drawFilteredBitmap()? (I think I like the last one best, but if there are other possible optimizations here, maybe drawBitmapFast()?) https://codereview.chromium.org/1491293002/diff/100001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1491293002/diff/100001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:2198: if (ctm.getType() & ~SkMatrix::kTranslate_Mask) { Don't we also need to ensure that the translation is integral?
https://codereview.chromium.org/1491293002/diff/80001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/1491293002/diff/80001/include/core/SkCanvas.h... include/core/SkCanvas.h:1437: bool canCallFilterSprite(const SkRect& bounds, const SkPaint&); On 2015/12/04 19:22:51, Stephen White wrote: > I'm wondering why this is defined on SkCanvas. Are there (going to be) public > callers? (Ignore this comment.. I see now it's private.)
Not sure if we'll see any chrome layout diffs, but we see a few in Skia The draw-with-filter GM shows two of them, and the seem to be tickling edge conditions in Blur and Erode. They may be acceptable to rebase, or we may need to fix the filtesr first, not sure. Hence the defensive flag for chrome. https://codereview.chromium.org/1491293002/diff/100001/include/core/SkDevice.h File include/core/SkDevice.h (right): https://codereview.chromium.org/1491293002/diff/100001/include/core/SkDevice.... include/core/SkDevice.h:383: void filterSprite(const SkDraw&, const SkBitmap&, int x, int y, const SkPaint&); On 2015/12/04 19:22:51, Stephen White wrote: > Since we're getting rid of drawSprite(), could we just call this filterBitmap(), > so it's not the lone Sprite-related thingy? > > Come to think of it, this name is misleading wrt filterImage(), which actually > returns the filtered image (vs. this one, which draws it to the device). > > Maybe drawBitmapAsSprite? drawBitmapFast()? fastDrawBitmap()? > drawFilteredBitmap()? (I think I like the last one best, but if there are other > possible optimizations here, maybe drawBitmapFast()?) Agreed, the name is weird. drawBitmapAsSprite sgtm. https://codereview.chromium.org/1491293002/diff/100001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1491293002/diff/100001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:2198: if (ctm.getType() & ~SkMatrix::kTranslate_Mask) { On 2015/12/04 19:22:51, Stephen White wrote: > Don't we also need to ensure that the translation is integral? Good question. I *think* we ignore fractional translates when there is no scale (i.e. we ignore bilerp). However, I'll double check that. Certainly for now, I can add the check, and we can comment it in case we decide we can relax that constraint.
https://codereview.chromium.org/1491293002/diff/100001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1491293002/diff/100001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:2198: if (ctm.getType() & ~SkMatrix::kTranslate_Mask) { On 2015/12/04 19:33:29, reed1 wrote: > On 2015/12/04 19:22:51, Stephen White wrote: > > Don't we also need to ensure that the translation is integral? > > Good question. I *think* we ignore fractional translates when there is no scale > (i.e. we ignore bilerp). However, I'll double check that. > > Certainly for now, I can add the check, and we can comment it in case we decide > we can relax that constraint. Checked. We do not respect fractional translate today in drawBitmap (see SkDraw and its call to SkTreatAsSprite). I think the test is correct as 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/1491293002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491293002/120001
The CQ bit was unchecked by reed@google.com
https://codereview.chromium.org/1491293002/diff/100001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1491293002/diff/100001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:2198: if (ctm.getType() & ~SkMatrix::kTranslate_Mask) { On 2015/12/04 19:44:03, reed1 wrote: > On 2015/12/04 19:33:29, reed1 wrote: > > On 2015/12/04 19:22:51, Stephen White wrote: > > > Don't we also need to ensure that the translation is integral? > > > > Good question. I *think* we ignore fractional translates when there is no > scale > > (i.e. we ignore bilerp). However, I'll double check that. > > > > Certainly for now, I can add the check, and we can comment it in case we > decide > > we can relax that constraint. > > Checked. We do not respect fractional translate today in drawBitmap (see SkDraw > and its call to SkTreatAsSprite). I think the test is correct as is. Thanks for checking. Perhaps we should be using SkTreatAsSprite here too, then?
On 2015/12/04 20:18:16, Stephen White wrote: > https://codereview.chromium.org/1491293002/diff/100001/src/core/SkCanvas.cpp > File src/core/SkCanvas.cpp (right): > > https://codereview.chromium.org/1491293002/diff/100001/src/core/SkCanvas.cpp#... > src/core/SkCanvas.cpp:2198: if (ctm.getType() & ~SkMatrix::kTranslate_Mask) { > On 2015/12/04 19:44:03, reed1 wrote: > > On 2015/12/04 19:33:29, reed1 wrote: > > > On 2015/12/04 19:22:51, Stephen White wrote: > > > > Don't we also need to ensure that the translation is integral? > > > > > > Good question. I *think* we ignore fractional translates when there is no > > scale > > > (i.e. we ignore bilerp). However, I'll double check that. > > > > > > Certainly for now, I can add the check, and we can comment it in case we > > decide > > > we can relax that constraint. > > > > Checked. We do not respect fractional translate today in drawBitmap (see > SkDraw > > and its call to SkTreatAsSprite). I think the test is correct as is. > > Thanks for checking. Perhaps we should be using SkTreatAsSprite here too, then? SGTM
now w/ SkTreatAsSprite
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/1491293002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491293002/140001
LGTM https://codereview.chromium.org/1491293002/diff/140001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1491293002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:2188: #include "SkMatrixUtils.h" Nit: move this #include to the top-of-file?
The CQ bit was unchecked by reed@google.com
https://codereview.chromium.org/1491293002/diff/140001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/1491293002/diff/140001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:2188: #include "SkMatrixUtils.h" On 2015/12/04 21:41:08, Stephen White wrote: > Nit: move this #include to the top-of-file? Good catch.
The CQ bit was checked by reed@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com, senorblanco@chromium.org Link to the patchset: https://codereview.chromium.org/1491293002/#ps160001 (title: "move include to the top")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1491293002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491293002/160001
The CQ bit was unchecked by reed@google.com
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/1491293002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1491293002/160001
Message was sent while issue was closed.
Description was changed from ========== detect when we can filter bitmaps/images directly, w/o a tmp layer visual bench run on Mac Pro curr/maxrss loops min median mean max stddev samples config bench 100/100 MB 16 412µs 413µs 413µs 414µs 0% ▄▁▇▄▄▄▄█▄▃▅ gpu warmupbench 101/102 MB 32 547µs 548µs 611µs 1.24ms 34% █▁▁▁▁▁▁▁▁▁▁ gpu image-filter-sprite-draw-image 102/103 MB 32 547µs 548µs 721µs 1.23ms 41% █▁▇▁▁█▁▁▁▁▁ gpu image-filter-sprite-draw-bitmap 103/103 MB 64 546µs 546µs 546µs 547µs 0% ▆▄▂▁▇█▅▇▅▇▃ gpu image-filter-sprite-draw-sprite Should have no effect on Chrome while SK_SUPPORT_LEGACY_LAYER_BITMAP_IMAGEFILTERS is defined (which it is in chrome) BUG=skia:1073 ========== to ========== detect when we can filter bitmaps/images directly, w/o a tmp layer visual bench run on Mac Pro curr/maxrss loops min median mean max stddev samples config bench 100/100 MB 16 412µs 413µs 413µs 414µs 0% ▄▁▇▄▄▄▄█▄▃▅ gpu warmupbench 101/102 MB 32 547µs 548µs 611µs 1.24ms 34% █▁▁▁▁▁▁▁▁▁▁ gpu image-filter-sprite-draw-image 102/103 MB 32 547µs 548µs 721µs 1.23ms 41% █▁▇▁▁█▁▁▁▁▁ gpu image-filter-sprite-draw-bitmap 103/103 MB 64 546µs 546µs 546µs 547µs 0% ▆▄▂▁▇█▅▇▅▇▃ gpu image-filter-sprite-draw-sprite Should have no effect on Chrome while SK_SUPPORT_LEGACY_LAYER_BITMAP_IMAGEFILTERS is defined (which it is in chrome) BUG=skia:1073 Committed: https://skia.googlesource.com/skia/+/262a71b7f95ce98ff3dd8dba845afbd724470903 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://skia.googlesource.com/skia/+/262a71b7f95ce98ff3dd8dba845afbd724470903 |