Issue 7300046: New SkShaderImageFilter image filter (Closed)
|
Created:
11 years, 10 months ago by sugoi Modified:
11 years, 9 months ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionNew SkRectShaderImageFilter image filter
This new changelist also introduces a new image filter called SkRectShaderImageFilter which is make to simply apply a shader on a region without using any inputs.
TEST=Added ShaderImageFilter test
Committed: https://code.google.com/p/skia/source/detail?r=7808
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 21
Patch Set 3 : #Patch Set 4 : #
Total comments: 1
Patch Set 5 : #
Total comments: 3
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #
Total comments: 4
MessagesTotal messages: 33
https://codereview.appspot.com/7300046/diff/1/src/core/SkTurbulenceShader.cpp File src/core/SkTurbulenceShader.cpp (right): https://codereview.appspot.com/7300046/diff/1/src/core/SkTurbulenceShader.cpp... src/core/SkTurbulenceShader.cpp:2: * Copyright 2006 The Android Open Source Project Oops, a bit old, changed it to "Copyright 2013 Google Inc."
Sign in to reply to this message.
Lets try to land the SkShaderImageFilter addition separately (first). Also, we could/should add tests for it: - test the asShader() api - test/assert that using this imagefilter or the shader directly give the same output https://codereview.appspot.com/7300046/diff/1/include/core/SkImageFilter.h File include/core/SkImageFilter.h (right): https://codereview.appspot.com/7300046/diff/1/include/core/SkImageFilter.h#ne... include/core/SkImageFilter.h:117: * NULL otherwise. Same need for ownership-comment here. https://codereview.appspot.com/7300046/diff/1/include/core/SkImageFilter.h#ne... include/core/SkImageFilter.h:123: * NULL otherwise. Does the receiver need to call unref() when they are done? I presume that is the right behavior. Please document. https://codereview.appspot.com/7300046/diff/1/include/effects/SkShaderImageFi... File include/effects/SkShaderImageFilter.h (right): https://codereview.appspot.com/7300046/diff/1/include/effects/SkShaderImageFi... include/effects/SkShaderImageFilter.h:17: public: // What does the rect parameter do/mean? // Can the shader be null? // What happens to the refcnt on the shader? (I presume it is incremented) https://codereview.appspot.com/7300046/diff/1/src/effects/SkShaderImageFilter... File src/effects/SkShaderImageFilter.cpp (right): https://codereview.appspot.com/7300046/diff/1/src/effects/SkShaderImageFilter... src/effects/SkShaderImageFilter.cpp:24: SkASSERT(s); Why do we assert AND call saferef?
Sign in to reply to this message.
I really like SkShaderImageFilter. We may to do SkColorFilterImageFilter as well. (general comment, not a review request) Shouldn't there be some SkCanvas code that skips the creation of the src image if asAShader() returns non-NULL? Admittedly, I'm a novice in the dark arts of image filters. Do we need a bench to catch future perf changes? Is there any value to testing use of the new SkShader class directly rather than as a SkImageFilter? What about testing other SkShaders as SkImageFilters? Maybe it'd be good to create these from a factory function in case we ever want to return a different subclass (e.g. if we can tell from the construction params that it really would produce a constant color or some other optimizable special case).
Sign in to reply to this message.
Also... I thought the naming pattern was asAFoo, not asFoo
Sign in to reply to this message.
On 2013/02/05 18:32:09, bsalomon wrote: > I really like SkShaderImageFilter. We may to do SkColorFilterImageFilter as > well. (general comment, not a review request) We already have that one. :) > Shouldn't there be some SkCanvas code that skips the creation of the src image > if asAShader() returns non-NULL? Admittedly, I'm a novice in the dark arts of > image filters. > > Do we need a bench to catch future perf changes? > > Is there any value to testing use of the new SkShader class directly rather than > as a SkImageFilter? What about testing other SkShaders as SkImageFilters? +1 to that. I think we should have a GM for the SkShader standalone, and then another GM (or unit test, checking that it produces the same pixels?) for the SkShaderImageFilter. > Maybe it'd be good to create these from a factory function in case we ever want > to return a different subclass (e.g. if we can tell from the construction params > that it really would produce a constant color or some other optimizable special > case).
Sign in to reply to this message.
About the new update : - I didn't remove the turbulence shader from the cl because I found useful to have it to write the comparison tests for using a SkShader VS SkShaderImageFilter VS SkShaderImageFilter::asFilter. I'm open to suggestions if you really want me to do this in 2 separate cls since I'd need a different 0 input shader to test it if I don't use the turbulence shader. - Also, I kept the name "asShader" (as opposed to "asAShader") since other functions use this naming pattern, like asColorFilter(), asNewEffect(), asRenderTarget(), asTexture(), etc. and I didn't want to change the other ones in this changelist too. https://codereview.appspot.com/7300046/diff/1/src/effects/SkShaderImageFilter... File src/effects/SkShaderImageFilter.cpp (right): https://codereview.appspot.com/7300046/diff/1/src/effects/SkShaderImageFilter... src/effects/SkShaderImageFilter.cpp:24: SkASSERT(s); Because SkASSERT only affects the debug build, I believe, so SkSafeRef handles a NULL pointer gracefully in the release build. Is there a different way of doing this usually ?
Sign in to reply to this message.
On 2013/02/06 16:35:25, sugoi wrote: >https://codereview.appspot.com/7300046/diff/1/src/effects/SkShaderImageFilter.cpp#newcode24 > src/effects/SkShaderImageFilter.cpp:24: SkASSERT(s); > Because SkASSERT only affects the debug build, I believe, so SkSafeRef handles a > NULL pointer gracefully in the release build. Is there a different way of doing > this usually ? IMO a NULL pointer should either: a) be expected and we should handle it correctly without an assert or b) be unexpected and we should assert in the debug build but not attempt to handle it correctly in the release build.
Sign in to reply to this message.
I vote again to have ShaderImageFilter be its own CL, and TurbulanceShader be its own CL. https://codereview.appspot.com/7300046/diff/12/include/core/SkTurbulenceShader.h File include/core/SkTurbulenceShader.h (right): https://codereview.appspot.com/7300046/diff/12/include/core/SkTurbulenceShade... include/core/SkTurbulenceShader.h:18: kTurbulence_TurbulenceType, Can these last two be in a private enum? Does the caller need to ever use these? https://codereview.appspot.com/7300046/diff/12/include/core/SkTurbulenceShade... include/core/SkTurbulenceShader.h:22: // dox to describe parameters https://codereview.appspot.com/7300046/diff/12/include/core/SkTurbulenceShade... include/core/SkTurbulenceShader.h:43: Can this be just forward declared here, and specified in the .cpp? Same question for StetchData. https://codereview.appspot.com/7300046/diff/12/include/core/SkTurbulenceShade... include/core/SkTurbulenceShader.h:49: How many bits do you want for your seed? we almost never use 'long'. https://codereview.appspot.com/7300046/diff/12/include/effects/SkShaderImageF... File include/effects/SkShaderImageFilter.h (right): https://codereview.appspot.com/7300046/diff/12/include/effects/SkShaderImageF... include/effects/SkShaderImageFilter.h:20: * SkShaderImageFilter object returned by this function. It cannot be NULL. nit: Don't need "Also" here. https://codereview.appspot.com/7300046/diff/12/include/effects/SkShaderImageF... include/effects/SkShaderImageFilter.h:21: * Also, "shadedRegion" represents the area of the canvas on which the shader will be applied. - is area in local or device coordinates? - is area a required parameter? seems like it could be optional - what is the use case for area? https://codereview.appspot.com/7300046/diff/12/src/core/SkTurbulenceShader.cpp File src/core/SkTurbulenceShader.cpp (right): https://codereview.appspot.com/7300046/diff/12/src/core/SkTurbulenceShader.cp... src/core/SkTurbulenceShader.cpp:19: var parameters should be *, not & can this function just return the new value? https://codereview.appspot.com/7300046/diff/12/src/core/SkTurbulenceShader.cp... src/core/SkTurbulenceShader.cpp:20: inline void checkNoise(int& noiseValue, int limitValue, int newValue) Nit: { should be at the end of the prev. line, just like "if" blocks https://codereview.appspot.com/7300046/diff/12/src/core/SkTurbulenceShader.cp... src/core/SkTurbulenceShader.cpp:21: { This is a non-obvious set of compares. Can you add a comment explaining them? https://codereview.appspot.com/7300046/diff/12/src/core/SkTurbulenceShader.cp... src/core/SkTurbulenceShader.cpp:35: // returns t * t * (3 - 2 * t) 2 * t is just fine for scalars, don't need to call a macro when you multiply or divide by literal. https://codereview.appspot.com/7300046/diff/12/src/core/SkTurbulenceShader.cp... src/core/SkTurbulenceShader.cpp:275: SkIPoint paintingSize = SkIPoint::Make(device.width(), device.height()); Yikes! Are you allocating a buffer the size of the device? That can easily be much larger than you'll need in practice. What are our options to put a cap on this allocation? tiling? What other info from skia would help make this more optimal?
Sign in to reply to this message.
I agree with Mike about submitting this as 2 separate cls, I just have a chicken and egg issue about using the turbulence filter in the SkShaderImageFilter tests. Any idea ? https://codereview.appspot.com/7300046/diff/12/include/core/SkTurbulenceShader.h File include/core/SkTurbulenceShader.h (right): https://codereview.appspot.com/7300046/diff/12/include/core/SkTurbulenceShade... include/core/SkTurbulenceShader.h:18: kTurbulence_TurbulenceType, I added kFirstType and kLastType for tests to be able to loop over all valid modes in the enum. I didn't find a better way than to have the first and last valid type here in the enum. I have no problem changing it if you have a better suggestion. https://codereview.appspot.com/7300046/diff/12/include/core/SkTurbulenceShade... include/core/SkTurbulenceShader.h:43: Probably, will do. https://codereview.appspot.com/7300046/diff/12/include/core/SkTurbulenceShade... include/core/SkTurbulenceShader.h:49: I doubt that too, that part is a copy of the WebKit implementation : https://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/filter... I can change it to an int. https://codereview.appspot.com/7300046/diff/12/include/effects/SkShaderImageF... File include/effects/SkShaderImageFilter.h (right): https://codereview.appspot.com/7300046/diff/12/include/effects/SkShaderImageF... include/effects/SkShaderImageFilter.h:20: * SkShaderImageFilter object returned by this function. It cannot be NULL. Removed https://codereview.appspot.com/7300046/diff/12/include/effects/SkShaderImageF... include/effects/SkShaderImageFilter.h:21: * Also, "shadedRegion" represents the area of the canvas on which the shader will be applied. Well, what I mean to say is that this will be a parameter to SkCanvas::drawRect(). For an inputless shader like turbulence, there's no other way to specify where the shader should be applied. Usually, I could retrieve that information from the bitmap, but there's no bitmap input here, so I don't know where else I could get this info. https://codereview.appspot.com/7300046/diff/12/src/core/SkTurbulenceShader.cpp File src/core/SkTurbulenceShader.cpp (right): https://codereview.appspot.com/7300046/diff/12/src/core/SkTurbulenceShader.cp... src/core/SkTurbulenceShader.cpp:19: On 2013/02/06 18:25:21, reed1 wrote: > var parameters should be *, not & > > can this function just return the new value? Done. https://codereview.appspot.com/7300046/diff/12/src/core/SkTurbulenceShader.cp... src/core/SkTurbulenceShader.cpp:20: inline void checkNoise(int& noiseValue, int limitValue, int newValue) On 2013/02/06 18:25:21, reed1 wrote: > Nit: { should be at the end of the prev. line, just like "if" blocks Done. https://codereview.appspot.com/7300046/diff/12/src/core/SkTurbulenceShader.cp... src/core/SkTurbulenceShader.cpp:35: // returns t * t * (3 - 2 * t) On 2013/02/06 18:25:21, reed1 wrote: > 2 * t is just fine for scalars, don't need to call a macro when you multiply or > divide by literal. Done. https://codereview.appspot.com/7300046/diff/12/src/core/SkTurbulenceShader.cp... src/core/SkTurbulenceShader.cpp:275: SkIPoint paintingSize = SkIPoint::Make(device.width(), device.height()); I'm not allocating any buffers, I'm just rendering to the device (which is a SkBitmap, not a SkDevice) using the SkShader object. The turbulence shader requires no allocation, it simply write to the output, like a clear operation, with a non constant noisy color. The output itself should have been allocated by the time we get here.
Sign in to reply to this message.
On 2013/02/06 19:45:07, sugoi wrote: > I agree with Mike about submitting this as 2 separate cls, I just have a chicken > and egg issue about using the turbulence filter in the SkShaderImageFilter > tests. Any idea ? > Use another SkShader subclass... e.g. a radial gradient or bitmap shader. I still feel like we're missing some kind of optimization where SkCanvas notices SkImageFilter::asShader() and draws directly to the final device using a paint with the shader rather than ever invoking the filterImage() code path.
Sign in to reply to this message.
https://codereview.appspot.com/7300046/diff/12/include/effects/SkShaderImageF... File include/effects/SkShaderImageFilter.h (right): https://codereview.appspot.com/7300046/diff/12/include/effects/SkShaderImageF... include/effects/SkShaderImageFilter.h:21: * Also, "shadedRegion" represents the area of the canvas on which the shader will be applied. On 2013/02/06 19:45:07, sugoi wrote: > Well, what I mean to say is that this will be a parameter to > SkCanvas::drawRect(). For an inputless shader like turbulence, there's no other > way to specify where the shader should be applied. Usually, I could retrieve > that information from the bitmap, but there's no bitmap input here, so I don't > know where else I could get this info. I think you can still get it from the "src" bitmap in onFilterImage. This should always be supplied by skia, even if your input is NULL, since it's a bitmap representing the drawn primitive to which the filter will be applied. Even though the bitmap will be unused in processing in your case, you should be able to use its size. For SVG, this should be ok for now. If I'm wrong about that, we can revert to a constructor parameter like this.
Sign in to reply to this message.
Ok, I separated the code into 2 cls. The turbulence shader is no longer in this cl.
Sign in to reply to this message.
Meta: what does it really mean to have an imagefilter that doesn't know about the bounds of the input primitive? Seems undefined what the results will be, since it is implementation-dependent on how large an offscreen I decide to allocate an any given time. https://codereview.appspot.com/7300046/diff/7002/include/core/SkImageFilter.h File include/core/SkImageFilter.h (right): https://codereview.appspot.com/7300046/diff/7002/include/core/SkImageFilter.h... include/core/SkImageFilter.h:120: * to keep it alive longer than then SkImageFilter owner. I would vote for a different protocol: the caller is always an owner, and must call unref. My thinking is that to do otherwise is to force the imagefilter to actually keep a slot/cache of the colorfilter. If it happens to already have one, then this is trivial (but so would calling ref() be). If it doesn't, but lazily allocates a colorfilter in response to this call, then it *must* keep track of it. Perhaps, depending on what we think the common use-case will be for the caller, the signature could be ... bool asColorFilter(SkColorFilter**); The bool results tells us if it *can* be a color-filter, and if we pass NULL, then no one allocated anything just for the query. Perhaps we should look at our other as-a's on other effects, to see if there is an existing pattern we should use (or fix)...
Sign in to reply to this message.
Here's a new version with the changes requested by Mike. I'm still not sure what to do about the size of the shader potentially being the size of the device, but I'm not sure this needs to be addressed in this cl.
Sign in to reply to this message.
https://codereview.appspot.com/7300046/diff/14/src/effects/SkColorFilterImage... File src/effects/SkColorFilterImageFilter.cpp (right): https://codereview.appspot.com/7300046/diff/14/src/effects/SkColorFilterImage... src/effects/SkColorFilterImageFilter.cpp:66: && (NULL != inputColorFilter)) { Maybe we should just assert this is non-NULL below? Is there a case where asColorFilter() returns true, but the returned filter is NULL?
Sign in to reply to this message.
BTW, you should probably re-title this issue to reflect that it's just the SkShaderImageFilter now.
Sign in to reply to this message.
Re-titled the issue appropriately. https://codereview.appspot.com/7300046/diff/14/src/effects/SkColorFilterImage... File src/effects/SkColorFilterImageFilter.cpp (right): https://codereview.appspot.com/7300046/diff/14/src/effects/SkColorFilterImage... src/effects/SkColorFilterImageFilter.cpp:66: && (NULL != inputColorFilter)) { Not currently, but the implementation could change and there could be a valid NULL someday (maybe a filter that does nothing could return NULL to be skipped entirely, for example).
Sign in to reply to this message.
On 2013/02/12 21:33:02, sugoi wrote: > Re-titled the issue appropriately. > > https://codereview.appspot.com/7300046/diff/14/src/effects/SkColorFilterImage... > File src/effects/SkColorFilterImageFilter.cpp (right): > > https://codereview.appspot.com/7300046/diff/14/src/effects/SkColorFilterImage... > src/effects/SkColorFilterImageFilter.cpp:66: && (NULL != inputColorFilter)) { > Not currently, but the implementation could change and there could be a valid > NULL someday (maybe a filter that does nothing could return NULL to be skipped > entirely, for example). In that case, it's not an color filter image filter anymore either, this optimization (collapsing matrices) doesn't apply, and asColorFilter() should return false in that case.
Sign in to reply to this message.
https://codereview.appspot.com/7300046/diff/14/src/effects/SkColorFilterImage... File src/effects/SkColorFilterImageFilter.cpp (right): https://codereview.appspot.com/7300046/diff/14/src/effects/SkColorFilterImage... src/effects/SkColorFilterImageFilter.cpp:66: && (NULL != inputColorFilter)) { On 2013/02/12 21:33:02, sugoi wrote: > Not currently, but the implementation could change and there could be a valid > NULL someday (maybe a filter that does nothing could return NULL to be skipped > entirely, for example). Hmm... I agree with Stephen. I think if asColorFilter returns true it should always set the filter to non-NULL (if a NULL ** wasn't passed of course). I don't think returning true with a NULL pointer result from asColorFilter or asShader is how we'd communicate a skippable filter.
Sign in to reply to this message.
concrete description (for discussion purposes)
bool MyImageFilter1::asColorFilter(SkColorFilter** returnRef) const {
if (returnRef) {
*returnRef = new MyImageFilterColorFilter(...);
}
return true;
}
or
bool MyImageFilter2::asColorFilter(SkColorFilter** returnRef) const {
if (returnRef) {
fPrivateColorFilter->ref();
*returnRef = fPrivateColorFilter;
}
return true;
}
#1 does not intrinsically have an instance of said colorfilter, but can cons one
up if the caller wants it.
#2 already had an instance, so it can just ref and return it.
Note: if we can guarantee that colorfilter instances are ALWAYS immutable, we
can safely implement #2 (and that should be a goal). If we cannot, then perhaps
only #1 is safe for now...
Sign in to reply to this message.
Could we have : #1 : bool MyImageFilter1::asColorFilter(SkColorFilter** returnRef) const #2 : bool MyImageFilter2::asColorFilter(const SkColorFilter** returnRef) const so that #2 is explicitly immutable ?
Sign in to reply to this message.
On 2013/02/19 21:41:43, sugoi wrote: > Could we have : > #1 : bool MyImageFilter1::asColorFilter(SkColorFilter** returnRef) const > #2 : bool MyImageFilter2::asColorFilter(const SkColorFilter** returnRef) const > so that #2 is explicitly immutable ? The immutability goes deeper than just what the caller of this function is allowed to do, since to use the returned colorfilter, it may install it in a paint. I think, in fact, that our colorfilters are already immutable (and reentrant-safe, which is harder to ensure), and I don't want to distract this CL on this account. My point was more about possible considerations on the impl side. I am fine to not change the signature to const * for now.
Sign in to reply to this message.
The changes are made. Note that, since the SkRect member is now inside SkShaderImageFilter, that property would be ignored when someone uses the asShader() function and that region would need to be re-specified by the user (which is probably already mandatory when using a shader).
Sign in to reply to this message.
On 2013/02/20 21:53:48, sugoi wrote: > The changes are made. Note that, since the SkRect member is now inside > SkShaderImageFilter, that property would be ignored when someone uses the > asShader() function and that region would need to be re-specified by the user > (which is probably already mandatory when using a shader). If we have no callers then maybe we don't need asShader? Or it should output a rect?
Sign in to reply to this message.
agreed: 1. lets remove asShader 2. rename this to something more specific. e.g. RectShaderImageFilter
Sign in to reply to this message.
Here's SkRectShaderImageFilter.
Sign in to reply to this message.
On 2013/02/21 14:41:45, sugoi wrote: > Here's SkRectShaderImageFilter. lgtm
Sign in to reply to this message.
Message was sent while issue was closed.
These comments are about matching general skia pattern for conditional queries. If we return true, the optional parameter must be set. If we return false, the optional parameter is always ignored. https://codereview.appspot.com/7300046/diff/24011/include/core/SkImageFilter.h File include/core/SkImageFilter.h (right): https://codereview.appspot.com/7300046/diff/24011/include/core/SkImageFilter.... include/core/SkImageFilter.h:119: * refed within this function and the caller will need to call unref() later on. For clarity in comment, perhaps we rename the parameter to filterPtr? // If this returns false, then the filterPtr is unchanged. // If this returns true, then if filterPtr is not null, it must be set to a ref'd colorfitler (i.e. it may not be set to NULL). https://codereview.appspot.com/7300046/diff/24011/include/effects/SkRectShade... File include/effects/SkRectShaderImageFilter.h (right): https://codereview.appspot.com/7300046/diff/24011/include/effects/SkRectShade... include/effects/SkRectShaderImageFilter.h:20: * SkRectShaderImageFilter object returned by this function. It cannot be NULL. region is a loaded term. Please name it rect. https://codereview.appspot.com/7300046/diff/24011/include/effects/SkRectShade... include/effects/SkRectShaderImageFilter.h:22: */ const SkRect& rect, instead of passing the parameter by value.
Sign in to reply to this message.
Message was sent while issue was closed.
Given my prev. comment, perhaps we should add the SK_ attribute that warns if the caller did not look at our return value.
Sign in to reply to this message.
Message was sent while issue was closed.
This code was committed. I will follow up on these comments inside https://codereview.appspot.com/7322060/
Sign in to reply to this message.
Message was sent while issue was closed.
https://codereview.appspot.com/7300046/diff/24011/include/effects/SkRectShade... File include/effects/SkRectShaderImageFilter.h (right): https://codereview.appspot.com/7300046/diff/24011/include/effects/SkRectShade... include/effects/SkRectShaderImageFilter.h:23: static SkRectShaderImageFilter* Create(SkShader* s, SkRect region); Would this not be better as an SkIRect, or better, an SkISize? I don't see how fractional pixels can sensibly be produced from a shader ImageFilter, and I don't think we ever want to use the fTop/fLeft here, since the caller can just request a smaller size (I also don't see any tests which exercise non-zero fLeft/fTop, but I could be wrong).
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
