Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
239163002:
WIP -- separate out Parts from SkPaint (Closed)
|
Created:
6 years, 8 months ago by reed1 Modified:
4 years, 8 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
Descriptionseparate out Parts from SkPaint
BUG=skia:
Patch Set 1 #
Total comments: 26
Patch Set 2 : close to linking... #Patch Set 3 : links and runs #
Messages
Total messages: 11 (0 generated)
https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h File include/core/SkPaintParts.h (right): https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h#... include/core/SkPaintParts.h:41: sk_bzero(this, sizeof(*this)); Tsk tsk. We know this bzero/memcpy stuff is bad news now... https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h#... include/core/SkPaintParts.h:44: SkPaintParts(const SkPaintParts& other) { Maybe we should just write these two as // Intentionally using the default copy constructor and operator=. // SkPaintParts(const SkPaintParts&) = default; // SkPaintParts& operator=(const SkPaintParts&) = default; ? Those defaults are actually what we'd want here. https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h#... include/core/SkPaintParts.h:61: kAntiAlias_Flag = 0x01, //!< mask to enable antialiasing Can you note if 0x02 available for future use or somehow verboten? https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h#... include/core/SkPaintParts.h:74: kDistanceFieldTextTEMP_Flag = 0x4000, //!< TEMPORARY mask to enable distance fields Can't hurt to align everyone to the widest =? https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h#... include/core/SkPaintParts.h:88: kNormal_Hinting = 2, //!< this is the default Doesn't bzero make this comment wrong? If the idea is to make SkPaint enforce all the semantics and have this just store the data dumbly, could be the default constructor (that is, leave everything uninitialized) is what we want. https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h#... include/core/SkPaintParts.h:93: return static_cast<Hinting>(fHinting); Might take the opportunity to make some things consistent in the file: - Hinting is explicitly enumerated, the rest of the enums aren't - Hinting is static_cast<Hinting>, the rest are (Class) https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h#... include/core/SkPaintParts.h:165: SkTypeface* fTypeface; Shall we go whole-hog here and make everything just a public field (and tag the thing as a struct)? You ought to be able to write the fields as their proper enum types too: struct SkPaintParts { SkTypeface* fTypeface; ... SkAnnotation* fAnnotation; SkScalar fTextSize; ... SkScalar fMiterLimit; union { struct { uint32_t fFlags : 16; Align fTextAlign : 2; Cap fCapType : 2; ... // unsigned fFreeBits : 4; }; uint32_t fAllBitfields; }; }; bool operator==(const SkPaintParts&, const SkPaintParts&); bool operator!=(const SkPaintParts&, const SkPaintParts&) { return !(a==b); } https://codereview.chromium.org/239163002/diff/1/src/core/SkPaint.cpp File src/core/SkPaint.cpp (right): https://codereview.chromium.org/239163002/diff/1/src/core/SkPaint.cpp#newcode38 src/core/SkPaint.cpp:38: bool SkPaintParts::operator==(const SkPaintParts& other) const { Give him his own .cpp? https://codereview.chromium.org/239163002/diff/1/src/core/SkPaint.cpp#newcode211 src/core/SkPaint.cpp:211: if (fPaintOptionsAndroid != b.fPaintOptionsAndroid) { It makes me very happy that these are not moving into SkPaintParts.
https://codereview.chromium.org/239163002/diff/1/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/239163002/diff/1/include/core/SkPaint.h#newco... include/core/SkPaint.h:456: Join getStrokeJoin() const { return (Join)fParts.getStrokeJoin(); } Wait, are you casting from an SkPaintParts::Join to an SkPaint::Join? Why the duplicate enums? https://codereview.chromium.org/239163002/diff/1/include/core/SkPaint.h#newco... include/core/SkPaint.h:673: Align getTextAlign() const { return (Align)fParts.getTextAlign(); } If we remove the duplication, this cast is unnecessary. https://codereview.chromium.org/239163002/diff/1/include/core/SkPaint.h#newco... include/core/SkPaint.h:726: TextEncoding getTextEncoding() const { return (TextEncoding)fParts.getTextEncoding(); } Same casting issue. https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h File include/core/SkPaintParts.h (right): https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h#... include/core/SkPaintParts.h:38: class SkPaintParts { Comments would be nice. If I understand correctly, a goal of this class is to hold on to these parts without reffing and unreffing them. Further, a client could use an SkPaintParts instead of an SkPaint, to get all of the fields. (Are there other reasons for this class?) https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h#... include/core/SkPaintParts.h:60: enum Flags { I'm pretty opposed to having all these enums duplicated here. It seems like it would be very easy to get the two classes out of sync - especially for Flags; I expect the others won't change very often, but I don't see the advantage to the duplication. https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h#... include/core/SkPaintParts.h:93: return static_cast<Hinting>(fHinting); On 2014/04/15 14:32:44, mtklein wrote: > Might take the opportunity to make some things consistent in the file: > - Hinting is explicitly enumerated, the rest of the enums aren't > - Hinting is static_cast<Hinting>, the rest are (Class) I vote for C++ style casts.
https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h File include/core/SkPaintParts.h (right): https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h#... include/core/SkPaintParts.h:38: class SkPaintParts { ...and if this is somehow avoiding touching the refcount, it isn't obvious how. https://codereview.chromium.org/239163002/diff/1/src/core/SkPaint.cpp File src/core/SkPaint.cpp (right): https://codereview.chromium.org/239163002/diff/1/src/core/SkPaint.cpp#newcode123 src/core/SkPaint.cpp:123: operator= asserts that the argument isn't *(NULL). Is that worth doing here?
https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h File include/core/SkPaintParts.h (right): https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h#... include/core/SkPaintParts.h:61: kAntiAlias_Flag = 0x01, //!< mask to enable antialiasing On 2014/04/15 14:32:44, mtklein wrote: > Can you note if 0x02 available for future use or somehow verboten? 0x02 used to be kFilterBitmap_Flag, and Android treats 0x02 as their analogous FILTER_BITMAP_FLAG (in Paint.java). That said, we already have to special case that (at least, with recent revs to Skia - you can see this by the CTS failures in Android's master-skia branch), so if we put a new flag there, Android will just need to continue special casing it.
https://codereview.chromium.org/239163002/diff/1/include/core/SkPaint.h File include/core/SkPaint.h (right): https://codereview.chromium.org/239163002/diff/1/include/core/SkPaint.h#newco... include/core/SkPaint.h:456: Join getStrokeJoin() const { return (Join)fParts.getStrokeJoin(); } On 2014/04/15 17:21:07, scroggo wrote: > Wait, are you casting from an SkPaintParts::Join to an SkPaint::Join? > > Why the duplicate enums? Temporary, to simplify all 1000+ callsites of SkPaint. Also, want to make SkPaintParts be stand-alone, so I don't want it to import SkPaint.h for its types. https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h File include/core/SkPaintParts.h (right): https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h#... include/core/SkPaintParts.h:38: class SkPaintParts { On 2014/04/15 17:21:07, scroggo wrote: > Comments would be nice. If I understand correctly, a goal of this class is to > hold on to these parts without reffing and unreffing them. Further, a client > could use an SkPaintParts instead of an SkPaint, to get all of the fields. (Are > there other reasons for this class?) Absolutely this will need to be documented. (but maybe not until it is more fleshed out). The end-goal (in my mind) is to make SkPaint purely an optional helper library, and move it (logically) out of core. https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h#... include/core/SkPaintParts.h:41: sk_bzero(this, sizeof(*this)); On 2014/04/15 14:32:44, mtklein wrote: > Tsk tsk. We know this bzero/memcpy stuff is bad news now... Tsk tsk... premature optimization until the fields of this class settle down :) https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h#... include/core/SkPaintParts.h:44: SkPaintParts(const SkPaintParts& other) { On 2014/04/15 14:32:44, mtklein wrote: > Maybe we should just write these two as > // Intentionally using the default copy constructor and operator=. > // SkPaintParts(const SkPaintParts&) = default; > // SkPaintParts& operator=(const SkPaintParts&) = default; > ? Those defaults are actually what we'd want here. Agreed, will remove these. https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h#... include/core/SkPaintParts.h:61: kAntiAlias_Flag = 0x01, //!< mask to enable antialiasing On 2014/04/15 14:32:44, mtklein wrote: > Can you note if 0x02 available for future use or somehow verboten? Will do that also in a diff CL for the current SkPaint. https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h#... include/core/SkPaintParts.h:93: return static_cast<Hinting>(fHinting); On 2014/04/15 17:21:07, scroggo wrote: > On 2014/04/15 14:32:44, mtklein wrote: > > Might take the opportunity to make some things consistent in the file: > > - Hinting is explicitly enumerated, the rest of the enums aren't > > - Hinting is static_cast<Hinting>, the rest are (Class) > > I vote for C++ style casts. Will do (before committing) https://codereview.chromium.org/239163002/diff/1/include/core/SkPaintParts.h#... include/core/SkPaintParts.h:165: SkTypeface* fTypeface; On 2014/04/15 14:32:44, mtklein wrote: > Shall we go whole-hog here and make everything just a public field (and tag the > thing as a struct)? You ought to be able to write the fields as their proper > enum types too: > > struct SkPaintParts { > SkTypeface* fTypeface; > ... > SkAnnotation* fAnnotation; > > SkScalar fTextSize; > ... > SkScalar fMiterLimit; > > union { > struct { > uint32_t fFlags : 16; > Align fTextAlign : 2; > Cap fCapType : 2; > ... > // unsigned fFreeBits : 4; > }; > uint32_t fAllBitfields; > }; > }; > > bool operator==(const SkPaintParts&, const SkPaintParts&); > bool operator!=(const SkPaintParts&, const SkPaintParts&) { > return !(a==b); > } This is meant to be our new PUBLIC api. History has amply shown us that we evolve how/where we store values in Paint. It is convenient perhaps to just expose everything, but makes future refactoring much harder. https://codereview.chromium.org/239163002/diff/1/src/core/SkPaint.cpp File src/core/SkPaint.cpp (right): https://codereview.chromium.org/239163002/diff/1/src/core/SkPaint.cpp#newcode38 src/core/SkPaint.cpp:38: bool SkPaintParts::operator==(const SkPaintParts& other) const { On 2014/04/15 14:32:44, mtklein wrote: > Give him his own .cpp? Eventually yes.
as of #3, the code runs and passes tests. 1. Does this technique (embedded class) seem reasonable? 2. If so, what are next possible steps? - land as is, and iterate - go whole-hog and change all of /core to use Parts instead of Paint? - other? I think the real payoff will be when SkCanvas virtuals take Parts, allowing internal code (including picdture record) to use them and never need Paint.
On 2014/04/18 15:28:47, reed1 wrote: > as of #3, the code runs and passes tests. > > 1. Does this technique (embedded class) seem reasonable? Yes. > 2. If so, what are next possible steps? > - land as is, and iterate That seems fine to me, with comments and TODOs. I'd like for the duplication to not last for too long, since the longer we have the duplication, the more likely we'll get things out of sync. (I realize that one bottleneck is Android; we'll need to update the call sites to use the SkPaintParts enums.) > - go whole-hog and change all of /core to use Parts instead of Paint? I'm fine with that so long as we use a flag for the Android transition. > - other? > > I think the real payoff will be when SkCanvas virtuals take Parts, allowing > internal code (including picdture record) to use them and never need Paint.
On 2014/04/18 15:53:17, scroggo wrote: > On 2014/04/18 15:28:47, reed1 wrote: > > as of #3, the code runs and passes tests. > > > > 1. Does this technique (embedded class) seem reasonable? > > Yes. > > > 2. If so, what are next possible steps? > > - land as is, and iterate > > That seems fine to me, with comments and TODOs. I'd like for the duplication > to not last for too long, since the longer we have the duplication, the more > likely we'll get things out of sync. (I realize that one bottleneck is Android; > we'll need to update the call sites to use the SkPaintParts enums.) > > > - go whole-hog and change all of /core to use Parts instead of Paint? > > I'm fine with that so long as we use a flag for the Android transition. > > > - other? > > > > I think the real payoff will be when SkCanvas virtuals take Parts, allowing > > internal code (including picdture record) to use them and never need Paint. I hope that no changes is "required" for Android -- in that SkPaint can live as a helper semi-indefinitely (modulo duplicate enums).
Is this dead now? Or just indefinitely backburnered? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
