Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
698643002:
Expose FillBounds to allow GrPictureUtils::CollectLayers to be layered on top of it (Closed)
|
Created:
6 years, 1 month ago by robertphillips Modified:
6 years, 1 month ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Project:
skia Visibility:
Public. |
DescriptionExpose FillBounds to allow GrPictureUtils::CollectLayers to be layered on top of it
Patch Set 1 #Patch Set 2 : Expose SkRecods::FillBounds in SkRecordDraw.h #Patch Set 3 : Remove dead comment #
Total comments: 2
Patch Set 4 : Layer CollectLayers on top of FillBounds #
Total comments: 11
Patch Set 5 : Address code review comments #Patch Set 6 : Possible layering fix #
Total comments: 8
Patch Set 7 : Update to ToT #Patch Set 8 : Compose rather than inherit #
Total comments: 2
Patch Set 9 : Remove bogus INHERITED #Patch Set 10 : Whitespace change #Patch Set 11 : Fix unit test #Patch Set 12 : Move most everything to the header #Patch Set 13 : Fix Windows conversion complaints #Patch Set 14 : Fix Windows conversion complaints #
Messages
Total messages: 37 (11 generated)
robertphillips@google.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/698643002/diff/40001/src/core/SkRecordDraw.h File src/core/SkRecordDraw.h (right): https://codereview.chromium.org/698643002/diff/40001/src/core/SkRecordDraw.h#... src/core/SkRecordDraw.h:94: FillBounds(const SkRecord& record, SkBBoxHierarchy* bbh); Given that we do all the work in the constructor in one go and all the state is private, what value do we get out of exposing this beyond what SkRecordFillBounds gives us?
https://codereview.chromium.org/698643002/diff/40001/src/core/SkRecordDraw.h File src/core/SkRecordDraw.h (right): https://codereview.chromium.org/698643002/diff/40001/src/core/SkRecordDraw.h#... src/core/SkRecordDraw.h:94: FillBounds(const SkRecord& record, SkBBoxHierarchy* bbh); I guess we could move CollectLayers from GrPictureUtils.cpp to SkRecordDraw.cpp and then add a "SkRecordCollectLayersAndFillBounds" to SkRecordDraw.h.
PTAL. This isn't quite the end game but it shows the code duplication this change removes. The complete end game is: Add flag to SkPictureRecorder::beginRecording to enable computation of layer information Alter ctor to use CollectLayers instead of FillBounds when a bbh is present and layer information has been requested
Also, if/when https://codereview.chromium.org/696763002/ lands, CollectLayers::SaveLayerInfo will no longer require the fClipBound member.
https://codereview.chromium.org/698643002/diff/60001/src/core/SkRecordDraw.h File src/core/SkRecordDraw.h (right): https://codereview.chromium.org/698643002/diff/60001/src/core/SkRecordDraw.h#... src/core/SkRecordDraw.h:110: Bounds adjustAndMap(SkRect rect, const SkPaint* paint) const; getBound is only called when processing a Restore and 'index' points to its matching saveLayer op.
Thanks, that helped. https://codereview.chromium.org/698643002/diff/60001/src/core/SkRecordDraw.cpp File src/core/SkRecordDraw.cpp (right): https://codereview.chromium.org/698643002/diff/60001/src/core/SkRecordDraw.cp... src/core/SkRecordDraw.cpp:150: if (bbh) { Is this an indication that we really want this to be FillBounds(const SkRecord&, SkTDArray<SkRect>*), or something like that? https://codereview.chromium.org/698643002/diff/60001/src/core/SkRecordDraw.h File src/core/SkRecordDraw.h (right): https://codereview.chromium.org/698643002/diff/60001/src/core/SkRecordDraw.h#... src/core/SkRecordDraw.h:110: Bounds adjustAndMap(SkRect rect, const SkPaint* paint) const; On 2014/11/03 15:38:57, robertphillips wrote: > getBound is only called when processing a Restore and 'index' points to its > matching saveLayer op. Let's call it getBounds() for consistency with all the other names? https://codereview.chromium.org/698643002/diff/60001/src/core/SkRecordDraw.h#... src/core/SkRecordDraw.h:128: static bool PaintMayAffectTransparentBlack(const SkPaint* paint); Let's make this guy a static function in the .cpp? https://codereview.chromium.org/698643002/diff/60001/src/core/SkRecordDraw.h#... src/core/SkRecordDraw.h:134: static void AdjustTextForFontMetrics(SkRect* rect, const SkPaint& paint); These two too? https://codereview.chromium.org/698643002/diff/60001/src/gpu/GrPictureUtils.cpp File src/gpu/GrPictureUtils.cpp (right): https://codereview.chromium.org/698643002/diff/60001/src/gpu/GrPictureUtils.c... src/gpu/GrPictureUtils.cpp:40: this->trackSaveLayers(op); Are you sure trackSaveLayers() is called when it's run this way? Seems operator() is overridden non-virtually, so don't we need at least CRTP to make sure to call the right operator() from INHERITED(*pict->fRecord, bbh) above?
https://codereview.chromium.org/698643002/diff/60001/src/core/SkRecordDraw.cpp File src/core/SkRecordDraw.cpp (right): https://codereview.chromium.org/698643002/diff/60001/src/core/SkRecordDraw.cp... src/core/SkRecordDraw.cpp:150: if (bbh) { On 2014/11/03 15:56:46, mtklein wrote: > Is this an indication that we really want this to be FillBounds(const SkRecord&, > SkTDArray<SkRect>*), or something like that? Ultimately, I think that whenever we see the collect-layer-info flag we should just create a BBH no matter what. Right now the GPUOptimize method isn't fully integrated into the SkPicture creation pipeline but once it is we should be able to go back to always requiring a BBH. Note that the CollectLayers code does require some "in-the-moment" information (e.g., nesting layers for adjustAndMap) so just returning the final set of operation bounds won't be sufficient. https://codereview.chromium.org/698643002/diff/60001/src/core/SkRecordDraw.h File src/core/SkRecordDraw.h (right): https://codereview.chromium.org/698643002/diff/60001/src/core/SkRecordDraw.h#... src/core/SkRecordDraw.h:110: Bounds adjustAndMap(SkRect rect, const SkPaint* paint) const; On 2014/11/03 15:56:46, mtklein wrote: > On 2014/11/03 15:38:57, robertphillips wrote: > > getBound is only called when processing a Restore and 'index' points to its > > matching saveLayer op. > > Let's call it getBounds() for consistency with all the other names? Done. https://codereview.chromium.org/698643002/diff/60001/src/core/SkRecordDraw.h#... src/core/SkRecordDraw.h:128: static bool PaintMayAffectTransparentBlack(const SkPaint* paint); On 2014/11/03 15:56:46, mtklein wrote: > Let's make this guy a static function in the .cpp? Done. https://codereview.chromium.org/698643002/diff/60001/src/core/SkRecordDraw.h#... src/core/SkRecordDraw.h:134: static void AdjustTextForFontMetrics(SkRect* rect, const SkPaint& paint); On 2014/11/03 15:56:46, mtklein wrote: > These two too? Done. https://codereview.chromium.org/698643002/diff/60001/src/gpu/GrPictureUtils.cpp File src/gpu/GrPictureUtils.cpp (right): https://codereview.chromium.org/698643002/diff/60001/src/gpu/GrPictureUtils.c... src/gpu/GrPictureUtils.cpp:40: this->trackSaveLayers(op); On 2014/11/03 15:56:46, mtklein wrote: > Are you sure trackSaveLayers() is called when it's run this way? Seems > operator() is overridden non-virtually, so don't we need at least CRTP to make > sure to call the right operator() from INHERITED(*pict->fRecord, bbh) above? You're right (That last patch was rushed to show you what I had in mind). Let me poke around a bit. CRTP seems like it would ugly up FillBounds a bit.
PTAL. CRTP didn't seem precisely correct since we want FillRecords to do everything it usually does but plus a little piece. This CL splits the visitor apart from the visiting loop and allows derivation from the visitor.
https://codereview.chromium.org/698643002/diff/100001/include/core/SkPicture.h File include/core/SkPicture.h (right): https://codereview.chromium.org/698643002/diff/100001/include/core/SkPicture.... include/core/SkPicture.h:284: friend class SkRecords::CollectLayers; // access to fRecord No need for this friend anymore? https://codereview.chromium.org/698643002/diff/100001/src/core/SkMultiPicture... File src/core/SkMultiPictureDraw.cpp (right): https://codereview.chromium.org/698643002/diff/100001/src/core/SkMultiPicture... src/core/SkMultiPictureDraw.cpp:74: #define SK_IGNORE_GPU_LAYER_HOISTING 1 This change was intentional? https://codereview.chromium.org/698643002/diff/100001/src/core/SkRecordDraw.cpp File src/core/SkRecordDraw.cpp (right): https://codereview.chromium.org/698643002/diff/100001/src/core/SkRecordDraw.c... src/core/SkRecordDraw.cpp:512: SkRecords::VisitMaster<SkRecords::FillBounds>(record, bbh); Can we drop VisitMaster and just have this write the loop out explicitly, like we did in the other file? https://codereview.chromium.org/698643002/diff/100001/src/gpu/GrPictureUtils.cpp File src/gpu/GrPictureUtils.cpp (right): https://codereview.chromium.org/698643002/diff/100001/src/gpu/GrPictureUtils.... src/gpu/GrPictureUtils.cpp:25: class CollectLayers : public FillBounds { Now that we've separated out the loop from the constructor, can't we just do this by composition instead of inheritance?
Patchset #7 (id:120001) has been deleted
https://codereview.chromium.org/698643002/diff/100001/include/core/SkPicture.h File include/core/SkPicture.h (right): https://codereview.chromium.org/698643002/diff/100001/include/core/SkPicture.... include/core/SkPicture.h:284: friend class SkRecords::CollectLayers; // access to fRecord On 2014/11/03 19:46:02, mtklein wrote: > No need for this friend anymore? SkPicturePlayback still exists so I think that cleanup would need its own CL. https://codereview.chromium.org/698643002/diff/100001/src/core/SkMultiPicture... File src/core/SkMultiPictureDraw.cpp (right): https://codereview.chromium.org/698643002/diff/100001/src/core/SkMultiPicture... src/core/SkMultiPictureDraw.cpp:74: #define SK_IGNORE_GPU_LAYER_HOISTING 1 On 2014/11/03 19:46:02, mtklein wrote: > This change was intentional? Done. Gone now - always on inside Skia. https://codereview.chromium.org/698643002/diff/100001/src/core/SkRecordDraw.cpp File src/core/SkRecordDraw.cpp (right): https://codereview.chromium.org/698643002/diff/100001/src/core/SkRecordDraw.c... src/core/SkRecordDraw.cpp:512: SkRecords::VisitMaster<SkRecords::FillBounds>(record, bbh); On 2014/11/03 19:46:02, mtklein wrote: > Can we drop VisitMaster and just have this write the loop out explicitly, like > we did in the other file? Done. https://codereview.chromium.org/698643002/diff/100001/src/gpu/GrPictureUtils.cpp File src/gpu/GrPictureUtils.cpp (right): https://codereview.chromium.org/698643002/diff/100001/src/gpu/GrPictureUtils.... src/gpu/GrPictureUtils.cpp:25: class CollectLayers : public FillBounds { On 2014/11/03 19:46:02, mtklein wrote: > Now that we've separated out the loop from the constructor, can't we just do > this by composition instead of inheritance? Certainly doable - not sure it is any better. PTAL PS#8.
lgtm https://codereview.chromium.org/698643002/diff/160001/src/gpu/GrPictureUtils.cpp File src/gpu/GrPictureUtils.cpp (right): https://codereview.chromium.org/698643002/diff/160001/src/gpu/GrPictureUtils.... src/gpu/GrPictureUtils.cpp:174: typedef FillBounds INHERITED; Remove?
https://codereview.chromium.org/698643002/diff/160001/src/gpu/GrPictureUtils.cpp File src/gpu/GrPictureUtils.cpp (right): https://codereview.chromium.org/698643002/diff/160001/src/gpu/GrPictureUtils.... src/gpu/GrPictureUtils.cpp:174: typedef FillBounds INHERITED; On 2014/11/06 17:50:29, mtklein wrote: > Remove? Done.
The CQ bit was checked by robertphillips@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/698643002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 698643002-220001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Since the CL is editing public API, you must have an LGTM from one of: ('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org', 'bsalomon@google.com', 'djsollen@chromium.org', 'djsollen@google.com')
robertphillips@google.com changed reviewers: + bsalomon@google.com, reed@google.com
Adding Mike & Brian for API review
On 2014/11/07 13:11:03, robertphillips wrote: > Adding Mike & Brian for API review (There is no API change here...)
On 2014/11/07 13:17:24, mtklein wrote: > On 2014/11/07 13:11:03, robertphillips wrote: > > Adding Mike & Brian for API review > > (There is no API change here...) rubberstamp lgtm
The CQ bit was checked by robertphillips@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/698643002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on client.skia.compile (http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-GC...)
The CQ bit was checked by robertphillips@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/698643002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile (http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)
The CQ bit was checked by robertphillips@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/698643002/280001
The CQ bit was unchecked by robertphillips@google.com
Hey Mike - could you PTAL? Specifically, can you think of anything better than moving almost everything to the header?
On 2014/11/07 18:23:15, robertphillips wrote: > Hey Mike - could you PTAL? Specifically, can you think of anything better than > moving almost everything to the header? Well, it's gross, but perhaps not as gross as moving everything to the header. Templates generally are not exported outside their compilation unit unless concretely declared. I think things will work if you explicitly declare each instantiation of those methods that aren't being found. The SK_RECORD_TYPES macro can probably cut the bulk of this down to a couple lines. But, this does make me wonder if there is another way to arrange this to keep all this encapsulated. Perhaps invert it by passing a callback or two to the FillBounds object (via SkRecordFillBounds)? In that case, all the code will stay in a single translation unit, and can remain inlined together. This code is all written as a zillion tiny methods assumed to inline completely, so I wouldn't be surprised if the declare-all-the-things method above lead to lousy performance and a big jump in code size.
Message was sent while issue was closed.
This has been abandoned in favor of: Move SkRecordComputeLayers and CollectLayers into SkRecordDraw.cpp - https://codereview.chromium.org/716913003/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
