This change should result in no pixel changes, but does reduce line count, most
of which is removing some confusing usage and replacing it with more
straight-forward code. It also means that SkScalerContext::getImage is no longer
making decisions about the mask; the logic is moved to Rec creation in
SkPaint.cpp where it belongs.
This should now work as intended and clean up some problems. By removing
SkScalerContext::getImage's deciding to use the mask or not, this also paves the
way for https://codereview.appspot.com/6499101/ to be split into its two logical
changes.
Is it important that we pass the preblend parameter to generateImage, instead of
to the constructor, where it could be stashed away? Seems like we're suggesting
that it might be different for different glyphs within the same context. Can it
ever be?
If we only ever received A8 and LCD32, would you pass it in at all, or would you
just have ScalerContext apply it as an afterthought?
>Is it important that we pass the preblend parameter to generateImage, instead
of to the constructor?
No, the selection is based on whether there is a filter or not. Moved to
constructor.
>just have ScalerContext apply it as an afterthought?
It would be possible to have generateImage always return linear masks and have
getImage make another pass over the mask to convert it. Having generateImage
bake it in when possible avoids the extra pass in the majority of cases.
https://codereview.appspot.com/6749061/diff/16024/src/core/SkMaskGamma.h
File src/core/SkMaskGamma.h (right):
https://codereview.appspot.com/6749061/diff/16024/src/core/SkMaskGamma.h#newc...
src/core/SkMaskGamma.h:97:
On 2012/10/25 19:09:40, reed1 wrote:
> Static Methods Are Capitalized.
And iffy: removed.
This should address most of the issues. https://codereview.appspot.com/6749061/diff/31001/src/core/SkMaskGamma.h File src/core/SkMaskGamma.h (right): https://codereview.appspot.com/6749061/diff/31001/src/core/SkMaskGamma.h#newcode121 src/core/SkMaskGamma.h:121: /** Given ...
This should address most of the issues.
https://codereview.appspot.com/6749061/diff/31001/src/core/SkMaskGamma.h
File src/core/SkMaskGamma.h (right):
https://codereview.appspot.com/6749061/diff/31001/src/core/SkMaskGamma.h#newc...
src/core/SkMaskGamma.h:121: /** Given a color, returns the closest cannonical
color. */
On 2012/10/26 13:18:28, reed1 wrote:
> 'C'annonicalColor
Wasn't part of this CL, but sure, why not? Done.
https://codereview.appspot.com/6749061/diff/31001/src/core/SkScalerContext.cpp
File src/core/SkScalerContext.cpp (right):
https://codereview.appspot.com/6749061/diff/31001/src/core/SkScalerContext.cp...
src/core/SkScalerContext.cpp:77:
On 2012/10/26 13:18:28, reed1 wrote:
> At some point (maybe we're already there) trying to do so much work as member
> initializers becomes to hard to read. I think just writing assignment code
after
> the { is ok too :)
I really want the PreBlend to be a constant embedded object. The way to
initialize such a thing is to put it in the initializer list. This means that
the pre-requisites (fRec, fMaskFilter) need to also be in the initializer list.
Also, I found the code much more difficult to understand when it was in the
constructor body as no order was imposed and it was non-declarative. Having as
much as possible in the initializer list also makes me feel more comfortable. In
other words, I don't understand the 'hard to read' argument. Is it just
formatting? It's already essentially what would end up in the body of the
constructor if it were moved there. I'll add some white space and we can discuss
it.
https://codereview.appspot.com/6749061/diff/31001/src/core/SkScalerContext.cp...
src/core/SkScalerContext.cpp:86: , fNextContext(NULL)
On 2012/10/26 13:18:28, reed1 wrote:
> Is fPreBlend a copy or deepcopy? i.e. are there any life-cycle issues related
to
> the pointers inside it?
PreBlends are tear-offs of a MaskGamma. The PreBlend consists of three pointers
into a MaskGamma and a reference to that MaksGamma (to keep it alive while it
has pointers into it). A default constructed PreBlend is created will all fields
NULL.
There does exist a copy constructor on PreBlend which safely handles any
life-cycle issues, but with RTO it should never be called and always be
constructed in-place.
https://codereview.appspot.com/6749061/diff/31001/src/core/SkScalerContext.cp...
src/core/SkScalerContext.cpp:87: , fPreBlend(fMaskFilter ?
SkMaskGamma::PreBlend() : SkScalerContext::GetMaskPreBlend(fRec))
On 2012/10/26 13:18:28, reed1 wrote:
> Need a comment here, and/or in the header, as to why we have to, and why only
> one is visible to our subclasses.
Made comment in header.
lgtm
I don't see anything important about the order things are initialized for
SkScalerContext.h.
You have conditionals in several of your initializers, w/o a natural way to add
comments within given the initializer structure. As a review who did not
originally write the code, I have found this hard to read, esp. given skia's
history of *allowing* code and logic in the constructor.
I think a const-preblend field is a good goal. That is also accommodated w/ a
const getter for the subclasses.
Lets check this in and move on.
Issue 6749061: Clean up usage of mask gamma.
(Closed)
Created 12 years ago by bungeman
Modified 12 years ago
Reviewers: reed1
Base URL: http://skia.googlecode.com/svn/trunk/
Comments: 10
