Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2120333002:
deferred canvas (Closed)
Descriptiondeferred canvas
tries to eliminate unneeded clipRects and concats (and their associated save/restores).
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2120333002
Committed: https://skia.googlesource.com/skia/+/babc3de2ce7c2f7b32b1b6b9f36d2ac4a4d8dee1
Patch Set 1 #Patch Set 2 : check for looper #Patch Set 3 : add --defer to dump_record #Patch Set 4 : rebase #Patch Set 5 : fix restore #Patch Set 6 : fold trans/clip into previous entry if possible #Patch Set 7 : fix release warning #Patch Set 8 : handle scaling #Patch Set 9 : override makeSurface #Patch Set 10 : more overrides #Patch Set 11 : fix misplaced endif #Patch Set 12 : disable from dm for now #Patch Set 13 : doh #Patch Set 14 : check for unknown info in tiles, add override for drawables #Patch Set 15 : disable defer by default in dm #Patch Set 16 : make imageInfo and getProps virtual so we can proxy them #Patch Set 17 : 'E' to trigger deferred in sampleapp, unroll drawables and pictures for now #Patch Set 18 : make defer runtime in dm #Patch Set 19 : make flush virtual #
Total comments: 2
Patch Set 20 : rebase #Patch Set 21 : remove prev change to dmsrcsink #
Total comments: 10
Patch Set 22 : reintegrate w/ DM following Via pattern #Patch Set 23 : apply comments from #87 #Patch Set 24 : override drawTextRSXform #
Messages
Total messages: 105 (52 generated)
Description was changed from ========== deferred canvas BUG=skia: ========== to ========== deferred canvas BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2120333002 ==========
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
reed@google.com changed reviewers: + djsollen@google.com, fmalita@chromium.org, mtklein@google.com
Description was changed from ========== deferred canvas BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2120333002 ========== to ========== deferred canvas tries to eliminate unneeded clipRects and concats (and their associated save/restores). BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2120333002 ==========
reed@google.com changed reviewers: + bsalomon@google.com
I presume this is aimed at Android UI, which doesn't flatten into a single picture. So now some optimizations live in base SkCanvas (deferred save/restore), most are in the SKP peephole optimizer, and some in SkDeferredCanvas. I wonder if there's a clean way to consolidate going forward: make SkDeferredCanvas composable (proxy) and relocate all optimizations there?
indeed, this is 1) aimed at android call patterns, and 2) written separately just to isolate the experiment. If we want to keep some of this, we may decide to fold it into canvas directly, or at least have canvas know about it (e.g. may not need lazy save AND this). For now, mostly interested in it as a stand-alone optimization pass: is it correct (enough)? is it efficient? can it do more to speed up android's patterns?
> So now some optimizations live in base SkCanvas (deferred save/restore), most > are in the SKP peephole optimizer, and some in SkDeferredCanvas. Yeah, if by "most" we mean "two". SkRecordOpts has I think served decently as a platform for exploring these sorts of transformations, but doesn't actually do much today. I think it'd be fairly straightforward to move SaveLayerDrawRestoreNooper over here. It doesn't need to track a lot of context. SvgOpacityAndFilterLayerMergePass might not be so bad either: it has a more complex pattern, but deals with fewer canvas calls overall.
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
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/v2/patch-status/codereview.chromium.or...
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/v2/patch-status/codereview.chromium.or...
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/v2/patch-status/codereview.chromium.or...
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
reed@google.com changed reviewers: + robertphillips@google.com
https://codereview.chromium.org/2120333002/diff/360001/src/utils/SkDeferredCa... File src/utils/SkDeferredCanvas.cpp (right): https://codereview.chromium.org/2120333002/diff/360001/src/utils/SkDeferredCa... src/utils/SkDeferredCanvas.cpp:101: this->INHERITED::willSave(); Do we really want to defer the upcall (as opposed to dispatching immediately in SkDeferredCanvas::willSave())? E.g. SkDeferredCanvas c; c.save(); c.getSaveCount() == ?? Ditto for INHERITED::onClipRect/INHERITED::didConcat. https://codereview.chromium.org/2120333002/diff/360001/src/utils/SkDeferredCa... File src/utils/SkDeferredCanvas.h (right): https://codereview.chromium.org/2120333002/diff/360001/src/utils/SkDeferredCa... src/utils/SkDeferredCanvas.h:121: SkASSERT(isconcat); Nit: SkAssertResult?
pre-CL to try to simplify this one https://codereview.chromium.org/2130973002#
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/v2/patch-status/codereview.chromium.or...
PTAL Would like to land this to ease experiments w/ the new class on android. This CL should not affect any current clients, except 3 of our tools, each of which needs an opt-in to trigger.
https://codereview.chromium.org/2120333002/diff/400001/dm/DMSrcSink.cpp File dm/DMSrcSink.cpp (right): https://codereview.chromium.org/2120333002/diff/400001/dm/DMSrcSink.cpp#newco... dm/DMSrcSink.cpp:1236: return src.draw(gUseDeferredCanvas ? &deferred : &canvas); We would normally hook this into DM differently. ViaMatrix is probably the best example to follow. The upshot is, you can put a deferred canvas anywhere in the pipeline you like: --cofnig deferred-8888 is the same as --config 8888 --defer as you've written it, but you will also be able to just run deferred-gpu, deferred-skp, deferred-serialize-deferred-8888, .... whatever pipeline you like.
The CQ bit was unchecked by reed@google.com
On 2016/07/08 12:30:47, mtklein wrote: > https://codereview.chromium.org/2120333002/diff/400001/dm/DMSrcSink.cpp > File dm/DMSrcSink.cpp (right): > > https://codereview.chromium.org/2120333002/diff/400001/dm/DMSrcSink.cpp#newco... > dm/DMSrcSink.cpp:1236: return src.draw(gUseDeferredCanvas ? &deferred : > &canvas); > We would normally hook this into DM differently. ViaMatrix is probably the best > example to follow. > > The upshot is, you can put a deferred canvas anywhere in the pipeline you like: > --cofnig deferred-8888 is the same as --config 8888 --defer as you've written > it, but you will also be able to just run deferred-gpu, deferred-skp, > deferred-serialize-deferred-8888, .... whatever pipeline you like. Thanks. Will try it that way (I didn't like the global either, just didn't take time to see the existing pattern).
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/v2/patch-status/codereview.chromium.or...
ptal -- ViaDefer
lgtm
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/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2120333002/diff/400001/src/utils/SkDeferredCa... File src/utils/SkDeferredCanvas.cpp (right): https://codereview.chromium.org/2120333002/diff/400001/src/utils/SkDeferredCa... src/utils/SkDeferredCanvas.cpp:51: fSaveCount = 0; Nit: fSaveCount(0) https://codereview.chromium.org/2120333002/diff/400001/src/utils/SkDeferredCa... src/utils/SkDeferredCanvas.cpp:101: this->INHERITED::willSave(); So deferring the proxy canvas ops (and not just the target canvas ops) is intentional? (see my earlier comment) https://codereview.chromium.org/2120333002/diff/400001/src/utils/SkDeferredCa... src/utils/SkDeferredCanvas.cpp:189: if (rec.fData.fBounds.contains(*bounds)) { Nit: invert the check to avoid empty branch? https://codereview.chromium.org/2120333002/diff/400001/src/utils/SkDeferredCa... src/utils/SkDeferredCanvas.cpp:538: void SkDeferredCanvas::onFlush() { return fCanvas->flush(); } Should this also flush all deferred ops?
The CQ bit was unchecked by reed@google.com
https://codereview.chromium.org/2120333002/diff/400001/src/utils/SkDeferredCa... File src/utils/SkDeferredCanvas.cpp (right): https://codereview.chromium.org/2120333002/diff/400001/src/utils/SkDeferredCa... src/utils/SkDeferredCanvas.cpp:101: this->INHERITED::willSave(); On 2016/07/08 13:12:02, f(malita) wrote: > So deferring the proxy canvas ops (and not just the target canvas ops) is > intentional? > > (see my earlier comment) It is, just to cut down on the cost of executing clips and concats anywhere (that might not be needed). Not sure if that is critical. https://codereview.chromium.org/2120333002/diff/400001/src/utils/SkDeferredCa... src/utils/SkDeferredCanvas.cpp:189: if (rec.fData.fBounds.contains(*bounds)) { On 2016/07/08 13:12:02, f(malita) wrote: > Nit: invert the check to avoid empty branch? Done. https://codereview.chromium.org/2120333002/diff/400001/src/utils/SkDeferredCa... src/utils/SkDeferredCanvas.cpp:538: void SkDeferredCanvas::onFlush() { return fCanvas->flush(); } On 2016/07/08 13:12:02, f(malita) wrote: > Should this also flush all deferred ops? Good catch (tho calling flush when you're unbalanced is probably not useful).
https://codereview.chromium.org/2120333002/diff/400001/src/utils/SkDeferredCa... File src/utils/SkDeferredCanvas.cpp (right): https://codereview.chromium.org/2120333002/diff/400001/src/utils/SkDeferredCa... src/utils/SkDeferredCanvas.cpp:51: fSaveCount = 0; On 2016/07/08 13:12:02, f(malita) wrote: > Nit: fSaveCount(0) Removed.
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/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by reed@google.com
The CQ bit was checked by reed@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com Link to the patchset: https://codereview.chromium.org/2120333002/#ps420002 (title: "apply comments from #87")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2120333002/diff/400001/src/utils/SkDeferredCa... File src/utils/SkDeferredCanvas.cpp (right): https://codereview.chromium.org/2120333002/diff/400001/src/utils/SkDeferredCa... src/utils/SkDeferredCanvas.cpp:101: this->INHERITED::willSave(); On 2016/07/08 13:32:32, reed1 wrote: > On 2016/07/08 13:12:02, f(malita) wrote: > > So deferring the proxy canvas ops (and not just the target canvas ops) is > > intentional? > > > > (see my earlier comment) > > It is, just to cut down on the cost of executing clips and concats anywhere > (that might not be needed). Not sure if that is critical. SGTM as a targeted experiment. But I think we should verify that there's a benefit to it. Diverging from SkCanvas semantics (lying about savecount, etc) will likely surprise some clients (worried about the legacy saveflags emulation glue in JNI in particular).
On 2016/07/08 13:42:44, f(malita) wrote: > https://codereview.chromium.org/2120333002/diff/400001/src/utils/SkDeferredCa... > File src/utils/SkDeferredCanvas.cpp (right): > > https://codereview.chromium.org/2120333002/diff/400001/src/utils/SkDeferredCa... > src/utils/SkDeferredCanvas.cpp:101: this->INHERITED::willSave(); > On 2016/07/08 13:32:32, reed1 wrote: > > On 2016/07/08 13:12:02, f(malita) wrote: > > > So deferring the proxy canvas ops (and not just the target canvas ops) is > > > intentional? > > > > > > (see my earlier comment) > > > > It is, just to cut down on the cost of executing clips and concats anywhere > > (that might not be needed). Not sure if that is critical. > > SGTM as a targeted experiment. > > But I think we should verify that there's a benefit to it. Diverging from > SkCanvas semantics (lying about savecount, etc) will likely surprise some > clients (worried about the legacy saveflags emulation glue in JNI in > particular). Never mind, the save count is not dependent on SkCanvas::willSave(), so my worries are unfounded :)
The CQ bit was unchecked by reed@google.com
The CQ bit was checked by reed@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com, fmalita@chromium.org Link to the patchset: https://codereview.chromium.org/2120333002/#ps450001 (title: "override drawTextRSXform")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== deferred canvas tries to eliminate unneeded clipRects and concats (and their associated save/restores). BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2120333002 ========== to ========== deferred canvas tries to eliminate unneeded clipRects and concats (and their associated save/restores). BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2120333002 Committed: https://skia.googlesource.com/skia/+/babc3de2ce7c2f7b32b1b6b9f36d2ac4a4d8dee1 ==========
Message was sent while issue was closed.
Committed patchset #24 (id:450001) as https://skia.googlesource.com/skia/+/babc3de2ce7c2f7b32b1b6b9f36d2ac4a4d8dee1 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
