Issue 7322060: New SVG turbulence in Skia (Closed)
|
Created:
11 years, 10 months ago by sugoi Modified:
11 years, 8 months ago CC:
skia-review_googlegroups.com, JimVV Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionNew SVG turbulence in Skia
This cl contains the code for both CPU and GPU generation of noise. Both codepaths yield equivalent results.
TEST:Added 'perlinnoise' gm
Committed: https://code.google.com/p/skia/source/detail?r=8313
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 35
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 5
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #
Total comments: 15
Patch Set 9 : #
Total comments: 4
Patch Set 10 : #
Total comments: 1
Patch Set 11 : #
Total comments: 1
Patch Set 12 : #Patch Set 13 : #Patch Set 14 : #Patch Set 15 : #Patch Set 16 : #
Total comments: 2
Patch Set 17 : #
Total comments: 2
Patch Set 18 : #Patch Set 19 : #
MessagesTotal messages: 63
Do we see the same seed value repeatedly? If so I really think we will want to cache the textures (if not the raster's SkBitmap as well). https://codereview.appspot.com/7322060/diff/1/include/core/SkTurbulenceShader.h File include/core/SkTurbulenceShader.h (right): https://codereview.appspot.com/7322060/diff/1/include/core/SkTurbulenceShader... include/core/SkTurbulenceShader.h:16: class SkTurbulenceShader : public SkShader { Brief descriptive comment? https://codereview.appspot.com/7322060/diff/1/include/core/SkTurbulenceShader... include/core/SkTurbulenceShader.h:26: // Note that, if stitchTiles is true, the size must be set before attempting to process the Can you write this as a doxygen style comment /** Note, that ... ... (for using asNewEffect()). */ I think some more explanation of what stitching is and what the size params means is warranted here. size of what? We can change the params to asNewEffect if it will make this cleaner. Since we don't call setContext() it seems reasonable that asNewEffect should communicate anything that the raster device would through setContext(). https://codereview.appspot.com/7322060/diff/1/src/core/SkTurbulenceShader.cpp File src/core/SkTurbulenceShader.cpp (right): https://codereview.appspot.com/7322060/diff/1/src/core/SkTurbulenceShader.cpp... src/core/SkTurbulenceShader.cpp:23: static const int gBlockSize = 256; nit: static const ints are styled kMyConstant not gMyConstant. https://codereview.appspot.com/7322060/diff/1/src/core/SkTurbulenceShader.cpp... src/core/SkTurbulenceShader.cpp:34: // If the noise value would bring us out of bounds of the current noise array while we are stiching nit: line wraps in this comment exceed 100 col.
Sign in to reply to this message.
New version with Brian's comments fixed.
Sign in to reply to this message.
note -- moving to a factory might eliminate the need for the enum altogether. https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... File include/core/SkTurbulenceShader.h (right): https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:12: Both of these forward-declares can be private, inside the class. https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:23: public: Nit: can be shorted to Type imho, since this enum is always scoped by SkTurbulenceShader. https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:24: enum TurbulenceType { What is the purpose of kUnknown? https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:36: */ Are there combinations of parameters that do not make sense? Consider if we should use a factory instead of public constructor here. SkGradientShader has lots of internal specializations, and so does not expose its actual class, but just provides factories. This also lets it return NULL (or emptyshader, etc.) in response to non-sensical or degenerate input parameters. https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:41: Ick. We are trying very hard to make all effects immutable. 1. what does setSize do? 2. can we instead move it to a parameter for our factory/constructor? https://codereview.appspot.com/7322060/diff/6001/src/core/SkTurbulenceShader.cpp File src/core/SkTurbulenceShader.cpp (right): https://codereview.appspot.com/7322060/diff/6001/src/core/SkTurbulenceShader.... src/core/SkTurbulenceShader.cpp:52: can you use SkScalarInterp from SkScalar.h ? https://codereview.appspot.com/7322060/diff/6001/src/core/SkTurbulenceShader.... src/core/SkTurbulenceShader.cpp:66: How is this different from the default == operator the compiler would generate?
Sign in to reply to this message.
also, .h and .cpp should be move out of core and into effects
Sign in to reply to this message.
On 2013/02/21 15:21:12, reed1 wrote: > also, .h and .cpp should be move out of core and into effects belay my last, I didn't see the move to effects
Sign in to reply to this message.
Perhaps we can separate out the revisions to rectshaderimagefilter and the new CL for turbulence?
Sign in to reply to this message.
https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... File include/core/SkTurbulenceShader.h (right): https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:26: kFractalnoise_TurbulenceType, Fractalnoise -> FractalNoise ? https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:32: * Note that, if stitchTiles is true, the size must be set before attempting to process the This still isn't clear to me. Can the comment explain what stitching means, what space the rect is in and what the meaning of the rect is? https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:42: bool setSize(const SkISize&); Do we need this function? I think we want to move towards immutable effect objects. https://codereview.appspot.com/7322060/diff/6001/src/core/SkTurbulenceShader.cpp File src/core/SkTurbulenceShader.cpp (right): https://codereview.appspot.com/7322060/diff/6001/src/core/SkTurbulenceShader.... src/core/SkTurbulenceShader.cpp:601: PaintingData paintingData(random->nextU(), paintingSize); It might be simpler to create an SkTurbulenceShader with random input and then just call asNewEffect() rather than duplicating some of the code from that function here. https://codereview.appspot.com/7322060/diff/6001/src/core/SkTurbulenceShader.... src/core/SkTurbulenceShader.cpp:863: key += 0x4; // Flip the 3rd bit if tile stitching is on tiny detail, but |= seems more standard than += https://codereview.appspot.com/7322060/diff/6001/src/core/SkTurbulenceShader.... src/core/SkTurbulenceShader.cpp:866: return key; If you're using GrGLEffectMatrix then it needs to participate in generating the key as different matrices may produce different code. https://codereview.appspot.com/7322060/diff/6001/src/core/SkTurbulenceShader.... src/core/SkTurbulenceShader.cpp:901: GrTexture* permutationsTexture = GrLockAndRefCachedBitmapTexture( Using the cache in this manner we will get reuse if the same SkShader (with same setSize() setting) is used to draw multiple times but not if different equivalent SkShaders are used. It is possible to create a custom cache key where you would could use the size, octaves, whatever as in the key. If you think that is worthwhile I can explain how to do it.
Sign in to reply to this message.
Ok, I addressed most comments, but I didn't change SkRect to SKISize in SkRectShaderImageFilter. I can do that too, if everyone agrees. https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... File include/core/SkTurbulenceShader.h (right): https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:12: They can't be private, but I put them inside the class https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:23: public: On 2013/02/21 15:20:55, reed1 wrote: > Nit: can be shorted to Type imho, since this enum is always scoped by > SkTurbulenceShader. Done. https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:24: enum TurbulenceType { Compatibility with WebKit https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:26: kFractalnoise_TurbulenceType, On 2013/02/21 15:31:32, bsalomon wrote: > Fractalnoise -> FractalNoise ? Done. https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:32: * Note that, if stitchTiles is true, the size must be set before attempting to process the If I understand this correctly, this simply means : do you want to produce "tilable" noise or do you not care about having discontinuities at the border of the produced noise. https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:36: */ - baseFrequency[XY] should usually in the ]0,1[ range to produce good noise - numOctaves shouldn't be too large, as computation time goes up with it. There's no actual hard limit, though. - All other parameters can have any value. https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:41: The reason for this is that the shader could be created before knowing what tile size it will use. Presumably, this could only be known after the shader is constructed. Although, now that the rect is a parameter of SkRectShaderImageFilter, in that case, we can know the size of the noise we want to generate at creation time, so I could remove this function for now. I changed the function so that it's private, since I don't need it to be public in this cl. I'll also remove the NULL default argument on the size parameter in the constructor and change it to a reference, so that it's mandatory. https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:42: bool setSize(const SkISize&); It's now private (see other comment) https://codereview.appspot.com/7322060/diff/6001/src/core/SkTurbulenceShader.cpp File src/core/SkTurbulenceShader.cpp (right): https://codereview.appspot.com/7322060/diff/6001/src/core/SkTurbulenceShader.... src/core/SkTurbulenceShader.cpp:52: On 2013/02/21 15:20:55, reed1 wrote: > can you use SkScalarInterp from SkScalar.h ? Done. https://codereview.appspot.com/7322060/diff/6001/src/core/SkTurbulenceShader.... src/core/SkTurbulenceShader.cpp:66: C++ provides a default operator=, but not a default operator==, which is why it's defined here. https://codereview.appspot.com/7322060/diff/6001/src/core/SkTurbulenceShader.... src/core/SkTurbulenceShader.cpp:601: PaintingData paintingData(random->nextU(), paintingSize); On 2013/02/21 15:31:32, bsalomon wrote: > It might be simpler to create an SkTurbulenceShader with random input and then > just call asNewEffect() rather than duplicating some of the code from that > function here. Done. https://codereview.appspot.com/7322060/diff/6001/src/core/SkTurbulenceShader.... src/core/SkTurbulenceShader.cpp:863: key += 0x4; // Flip the 3rd bit if tile stitching is on On 2013/02/21 15:31:32, bsalomon wrote: > tiny detail, but |= seems more standard than += Done. https://codereview.appspot.com/7322060/diff/6001/src/core/SkTurbulenceShader.... src/core/SkTurbulenceShader.cpp:866: return key; On 2013/02/21 15:31:32, bsalomon wrote: > If you're using GrGLEffectMatrix then it needs to participate in generating the > key as different matrices may produce different code. Done. https://codereview.appspot.com/7322060/diff/6001/src/core/SkTurbulenceShader.... src/core/SkTurbulenceShader.cpp:901: GrTexture* permutationsTexture = GrLockAndRefCachedBitmapTexture( Maybe not in this cl, but I would add it as an improvement after this is submitted. You can send me the howto in an e-mail, whenever you have time.
Sign in to reply to this message.
:( can we please separate out a CL that handles the changes to rectshader?
Sign in to reply to this message.
On 2013/02/21 19:39:47, reed1 wrote: > :( can we please separate out a CL that handles the changes to rectshader? Oh, right, give me a few minutes.
Sign in to reply to this message.
I think "turbulence" is a rare enough topic that we will require a real live comment block describe each of the parameters being passed to the constructor.
Sign in to reply to this message.
Done
Sign in to reply to this message.
On 2013/02/21 19:47:01, sugoi wrote: > Done (This refers to splitting the cl into 2 cls, not to Mike's comment. I will add the parameter description).
Sign in to reply to this message.
https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... File include/core/SkTurbulenceShader.h (right): https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:12: On 2013/02/21 19:02:24, sugoi wrote: > They can't be private, but I put them inside the class Why can they not be private? We use that pattern in SkChunkAlloc.h https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:24: enum TurbulenceType { On 2013/02/21 19:02:24, sugoi wrote: > Compatibility with WebKit > My question was intended to have this enum be documented in the header. https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:41: On 2013/02/21 19:02:24, sugoi wrote: > The reason for this is that the shader could be created before knowing what tile > size it will use. Presumably, this could only be known after the shader is > constructed. Although, now that the rect is a parameter of > SkRectShaderImageFilter, in that case, we can know the size of the noise we want > to generate at creation time, so I could remove this function for now. > I changed the function so that it's private, since I don't need it to be public > in this cl. I'll also remove the NULL default argument on the size parameter in > the constructor and change it to a reference, so that it's mandatory. It can be private, assuming it is only ever called from the constructor (or in the scope of a constructor, if we move to a factory model for creating these). Is that the case?
Sign in to reply to this message.
https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... File include/core/SkTurbulenceShader.h (right): https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:32: * Note that, if stitchTiles is true, the size must be set before attempting to process the On 2013/02/21 19:02:24, sugoi wrote: > If I understand this correctly, this simply means : do you want to produce > "tilable" noise or do you not care about having discontinuities at the border of > the produced noise. I just don't have any context for understanding what is being tiled and why as a user of this class I'd care about tiling. I guess its not obvious from reading the header up to here that there are tiles at all. Are the tiles in the local space or dst space? Is tiling a concern for a single draw using the shader (drawing something larger than the tile size)? Are the tiles a repeating pattern or is each tile's noise values distinct? If I pass false to stitchTiles then I expect to see discontinuities but won't that be at some frequency in X and Y? If so, then why is the size parameter ignored? Maybe I'm missing something basic.
Sign in to reply to this message.
1 ) The shader produces tiles of the provided size. The tiles can be repeatable or not, depending on the stitchTiles parameter provided, which determines if sems are visible between the tiles. You could produce only 1 tile (if the size parameter is exactly the same as the size of the region being drawn) and then tile it across the screen (I assume this is possible in Skia). If the drawing size is larger than the tile size, then it should produce multiple tiles. 2 ) You're right, I'll change the CPU so that it uses the size parameter too. The current code always produces a single tile in CPU.
Sign in to reply to this message.
https://codereview.appspot.com/7322060/diff/21001/include/effects/SkTurbulenc... File include/effects/SkTurbulenceShader.h (right): https://codereview.appspot.com/7322060/diff/21001/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:39: const SkISize& size); As discussed, let's rename size -> tileSize, if that makes sense. https://codereview.appspot.com/7322060/diff/21001/src/effects/SkTurbulenceSha... File src/effects/SkTurbulenceShader.cpp (right): https://codereview.appspot.com/7322060/diff/21001/src/effects/SkTurbulenceSha... src/effects/SkTurbulenceShader.cpp:110: long result = gRandAmplitude * (fSeed % gRandQ) - gRandR * (fSeed / gRandQ); Do we really need this to be a long? kRandMaximum seems suspiciously tied to 32 bit int; perhaps result (and this function) should be int32_t? https://codereview.appspot.com/7322060/diff/21001/src/effects/SkTurbulenceSha... src/effects/SkTurbulenceShader.cpp:674: code->appendf("\t\tfor (int octave = 0; octave < %s; ++octave) {", numOctavesUni); As discussed, we'll probably have to limit the octaves of noise here to prevent running out of loop iterations on older cards. This function as-written probably won't even compile on SM2 cards, but we can deal with that later. https://codereview.appspot.com/7322060/diff/21001/src/effects/SkTurbulenceSha... src/effects/SkTurbulenceShader.cpp:868: GrEffectRef* SkTurbulenceShader::asNewEffect(GrContext* context, const SkPaint& paint) const { I've just realized that the SkRectShaderImageFilter invokes this via the shader asNewEffect() called indirectly from SkRectShaderImageFilter::onFilterImage(). This probably means that SkRectShaderImageFilter won't get the drawSprite() fast path on the GPU. This may simply be a matter of plumbing through from the shader's own asNewEffect() to SkRectShaderImageFilter::asNewEffect(). (This will also allow SkShaders to participate in future optimizations to collapse GrEffect chains produced by SkImageFilters into a single GPU pass.) This can wait for a future patch, though. https://codereview.appspot.com/7322060/diff/21001/src/effects/SkTurbulenceSha... src/effects/SkTurbulenceShader.cpp:875: GrTexture* permutationsTexture = GrLockAndRefCachedBitmapTexture( As discussed, we might be able to share these textures between effects when the parameters are the same. Otherwise I would suggest having the GrTurbulenceEffect create and own its textures itself. Either one can wait for a future patch.
Sign in to reply to this message.
https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... File include/core/SkTurbulenceShader.h (right): https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:12: I can put PaintingData private, but StitchData is used outside of the SkTurbulenceShader class. I could make some changes to use the values rather than the object, but that seems less efficient... https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:24: enum TurbulenceType { On 2013/02/21 19:51:32, reed1 wrote: > On 2013/02/21 19:02:24, sugoi wrote: > > Compatibility with WebKit > > > > My question was intended to have this enum be documented in the header. Done. https://codereview.appspot.com/7322060/diff/6001/include/core/SkTurbulenceSha... include/core/SkTurbulenceShader.h:41: Yes, only constructors will call it.
Sign in to reply to this message.
On 2013/02/21 20:06:24, sugoi wrote: > 1 ) The shader produces tiles of the provided size. The tiles can be repeatable > or not, depending on the stitchTiles parameter provided, which determines if > sems are visible between the tiles. You could produce only 1 tile (if the size > parameter is exactly the same as the size of the region being drawn) and then > tile it across the screen (I assume this is possible in Skia). If the drawing > size is larger than the tile size, then it should produce multiple tiles. > 2 ) You're right, I'll change the CPU so that it uses the size parameter too. > The current code always produces a single tile in CPU. Can you add a comment about the tiled nature of this shader to the header (in the class description)? This is a fairly important distinction for this shader and I think it should be well documented.
Sign in to reply to this message.
On 2013/02/21 21:14:13, bsalomon wrote: > Can you add a comment about the tiled nature of this shader to the header (in > the class description)? This is a fairly important distinction for this shader > and I think it should be well documented. Ok, I tried to write a description of the tilable nature of the noise, you can tell me if it's clear to you or not.
Sign in to reply to this message.
https://codereview.appspot.com/7322060/diff/34001/include/effects/SkTurbulenc... File include/effects/SkTurbulenceShader.h (right): https://codereview.appspot.com/7322060/diff/34001/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:17: It can produce tilable noise if asked to stitch tiles and provided a tile size. In that case, Nit: s/tilable/tileable/ https://codereview.appspot.com/7322060/diff/34001/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:22: tiles within the result will be tilable, but the result itself will not be tilable. Hmm. This description seems to be confusing me more than it helps me. :) How about something a little more prescriptive, like: "In order to fill a large area with repeating noise, set the stitchTiles flag to true, and render exactly a single tile of noise. Without this flag, the result will contain visible seams between tiles." https://codereview.appspot.com/7322060/diff/34001/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:34: * kUnknown_Type is invalid, but will behave like kTurbulence_Type by default, Do we really need the unknown type here? Could we keep just fractal and turbulence at the Skia level, and handle the unknown type at the WebKit level? https://codereview.appspot.com/7322060/diff/34001/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:45: * This will construct a Perlin noise of the given type (Fractal or Turbulence). s/a Perlin noise/Perlin noise/, I think? https://codereview.appspot.com/7322060/diff/34001/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:47: * Both base frequencies (X and Y) produce better noise in the ]0,1[ range. Better how? https://codereview.appspot.com/7322060/diff/34001/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:49: * The number of octaves provided should be fairly small, although no limit is explicitely Nit: explicitely -> explicitly Actually, I would just say "no limit is enforced" (although perhaps we should enforce a limit for the GPU path? Or at least drop back to software in that case?) https://codereview.appspot.com/7322060/diff/34001/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:50: * provided. Each octave doubles the frequency, so 10 octaves would produce a noise from s/a noise/noise/ https://codereview.appspot.com/7322060/diff/34001/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:55: * that the noise will be tilable for the given tile size (if stitchTiles is false, the s/tilable/tileable/ I would remove the parens on the last clause, and make it into a new sentence.
Sign in to reply to this message.
https://codereview.appspot.com/7322060/diff/34001/include/effects/SkTurbulenc... File include/effects/SkTurbulenceShader.h (right): https://codereview.appspot.com/7322060/diff/34001/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:34: * kUnknown_Type is invalid, but will behave like kTurbulence_Type by default, On 2013/02/21 21:47:05, Stephen White wrote: > Do we really need the unknown type here? Could we keep just fractal and > turbulence at the Skia level, and handle the unknown type at the WebKit level? Done. https://codereview.appspot.com/7322060/diff/34001/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:45: * This will construct a Perlin noise of the given type (Fractal or Turbulence). On 2013/02/21 21:47:05, Stephen White wrote: > s/a Perlin noise/Perlin noise/, I think? Done. https://codereview.appspot.com/7322060/diff/34001/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:47: * Both base frequencies (X and Y) produce better noise in the ]0,1[ range. On 2013/02/21 21:47:05, Stephen White wrote: > Better how? Done. https://codereview.appspot.com/7322060/diff/34001/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:49: * The number of octaves provided should be fairly small, although no limit is explicitely On 2013/02/21 21:47:05, Stephen White wrote: > Nit: explicitely -> explicitly > > Actually, I would just say "no limit is enforced" (although perhaps we should > enforce a limit for the GPU path? Or at least drop back to software in that > case?) Yeah. I'll need to do some tests when this cl is merged in to see what values are acceptable on which platforms. https://codereview.appspot.com/7322060/diff/34001/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:50: * provided. Each octave doubles the frequency, so 10 octaves would produce a noise from On 2013/02/21 21:47:05, Stephen White wrote: > s/a noise/noise/ Done. https://codereview.appspot.com/7322060/diff/34001/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:55: * that the noise will be tilable for the given tile size (if stitchTiles is false, the On 2013/02/21 21:47:05, Stephen White wrote: > s/tilable/tileable/ > > I would remove the parens on the last clause, and make it into a new sentence. Done.
Sign in to reply to this message.
https://codereview.appspot.com/7322060/diff/34001/include/effects/SkTurbulenc... File include/effects/SkTurbulenceShader.h (right): https://codereview.appspot.com/7322060/diff/34001/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:22: tiles within the result will be tilable, but the result itself will not be tilable. On 2013/02/21 21:47:05, Stephen White wrote: > Hmm. This description seems to be confusing me more than it helps me. :) > Agreed. I think this should clearly explain the tiled nature of the shader here and then explain how the params control it in the constructor comment. Explaining tiling via the params is confusing to me.
Sign in to reply to this message.
I need to install this locally and play around. At the moment I still don't understand the tiles/stitch parameters. https://codereview.appspot.com/7322060/diff/36001/include/effects/SkTurbulenc... File include/effects/SkTurbulenceShader.h (right): https://codereview.appspot.com/7322060/diff/36001/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:38: * What happens if the frequencies are outside of this range?
Sign in to reply to this message.
Is PerlinShader a better name?
class SkPerlinShader ... {
public:
// no need for an enum (at least not a public one)
static SkShader* CreateNoise(...);
static SkShader* CreateTubulence(...);
};
Sign in to reply to this message.
https://codereview.appspot.com/7322060/diff/36001/include/effects/SkTurbulenc... File include/effects/SkTurbulenceShader.h (right): https://codereview.appspot.com/7322060/diff/36001/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:39: * Both base frequencies (X and Y) have a usual range of ]0,1[. Lets use (0..1) for open intervals, as we use that convention (and not backwards ][) in other parts of the base.
Sign in to reply to this message.
On 2013/02/22 14:13:49, reed1 wrote:
> Is PerlinShader a better name?
>
> class SkPerlinShader ... {
> public:
> // no need for an enum (at least not a public one)
>
> static SkShader* CreateNoise(...);
> static SkShader* CreateTubulence(...);
> };
Not that I dislike the name, but I'm a big fan of keeping the same name from one
end of the pipeline to the other. Since it's called feTurbulence in the spec and
FETurbulence in WebKit, I would argue that keeping the name Turbulence here
makes things clearer.
Sign in to reply to this message.
I suggested Perlin mostly by just looking at your header: - you mention Perlin noise and Perlin turbulence in it - you have an enum distinguishing turbulence and noise To me, naming both after one of the "distinctions" seems confusing. I'm not trying to be different from the SVG spec, but neither to I want to hold to it to the point of adding a documentation/clarity burden on our code.. which lives and breaths outside of the scope of SVG and webkit. All that said, I'm mostly just asking that we all think about the name (for all of our classes).
Sign in to reply to this message.
On 2013/02/22 14:48:35, reed1 wrote: > I suggested Perlin mostly by just looking at your header: > - you mention Perlin noise and Perlin turbulence in it > - you have an enum distinguishing turbulence and noise > > To me, naming both after one of the "distinctions" seems confusing. > > I'm not trying to be different from the SVG spec, but neither to I want to hold > to it to the point of adding a documentation/clarity burden on our code.. which > lives and breaths outside of the scope of SVG and webkit. > > All that said, I'm mostly just asking that we all think about the name (for all > of our classes). My 2c: I don't think that Skia should be constrained by SVG naming or conventions (some of the latter are downright weird). For a graphics guy coming to the Skia API for the first time, encountering a Perlin shader probably conveys more meaning than Turbulence.
Sign in to reply to this message.
Just a precision about the noise types : the difference between the 2 is just minor tweaks to the algorithm, they're not 2 entirely different noises. The output looks different, but once the noise is generated in the [1, -1] range, the output is brought back in the [0, 1] range by doing : Turbulence : abs(noise) Fractal : noise * 0.5 + 0.5 Very little differences between the 2 types, although you can tell the difference visually.
Sign in to reply to this message.
On 2013/02/22 15:02:16, sugoi wrote: > Just a precision about the noise types : the difference between the 2 is just > minor tweaks to the algorithm, they're not 2 entirely different noises. The > output looks different, but once the noise is generated in the [1, -1] range, > the output is brought back in the [0, 1] range by doing : > > Turbulence : abs(noise) > Fractal : noise * 0.5 + 0.5 > > Very little differences between the 2 types, although you can tell the > difference visually. Ah, that helps a lot. Perfect to add as a comment somewhere. Might still want to have a factory pattern, since it will allow you to specialize your impl based on that (or any other things you find down the road), though I don't know the details of the impl to know what optimizations are possible.
Sign in to reply to this message.
I did the Factory related changes. I haven't renamed it to SkPerlinNoiseShader yet to make reviewing these changes easier, but I will do it in the next upload. https://codereview.appspot.com/7322060/diff/36001/include/effects/SkTurbulenc... File include/effects/SkTurbulenceShader.h (right): https://codereview.appspot.com/7322060/diff/36001/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:38: * On 2013/02/22 14:12:07, reed1 wrote: > What happens if the frequencies are outside of this range? In the negative range, it sort of looks like a mirror image of the noise. If one of the 2 frequencies is over 1, it looks like smeared noise, if both are over 1, it looks like unstructured noise and 0 produces a constant color, but these are kind of "side effects" of the algorithm. Users are more likely to want the nicer Perlin noise in the (0..1) range. https://codereview.appspot.com/7322060/diff/36001/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:39: * Both base frequencies (X and Y) have a usual range of ]0,1[. On 2013/02/22 14:15:22, reed1 wrote: > Lets use (0..1) for open intervals, as we use that convention (and not backwards > ][) in other parts of the base. Done. https://codereview.appspot.com/7322060/diff/32006/include/effects/SkTurbulenc... File include/effects/SkTurbulenceShader.h (right): https://codereview.appspot.com/7322060/diff/32006/include/effects/SkTurbulenc... include/effects/SkTurbulenceShader.h:65: virtual ~SkTurbulenceShader(); Oops, I had forgotten to move the constructor and destructor in the private section, but I did it locally.
Sign in to reply to this message.
This new change only contains the name change from Turbulence to PerlinNoise
Sign in to reply to this message.
Thanks for the additional documentation and factories for creating these. I have run the patch locally, and you have some (~7) warnings that are treated as errors on the mac. More significantly, 1. The shader doesn't respect the matrix (or localmatrix), which is required for normal scrolling and zooming 2. The shader doesn't respect the paint's alpha (it should modulate its per-pixel alpha by the paint's More questions: 1. Is the noise defined to always also affect alpha? The current impl appears to do that. That's a shame (from a practical perspective) if one cannot get an "opaque" noise out of it. 2. I see that your constructor doesn't take SkShader::TileMode. Perhaps that is correct, perhaps not. Can you support Skia's notion of tiling? (clamp, repeat, mirror)? https://codereview.appspot.com/7322060/diff/36003/include/effects/SkPerlinNoi... File include/effects/SkPerlinNoiseShader.h (right): https://codereview.appspot.com/7322060/diff/36003/include/effects/SkPerlinNoi... include/effects/SkPerlinNoiseShader.h:64: Once you use Factories, you can move *everything* else into the .cpp - forward declares - virtual overrides - etc.
Sign in to reply to this message.
On 2013/02/22 19:01:46, reed1 wrote: > I have run the patch locally, and you have some (~7) warnings that are treated as errors on the mac. This is annoying. Is there a currently available way to get more warnings on Linux ? > More significantly, > 1. The shader doesn't respect the matrix (or localmatrix), which is required for normal scrolling and zooming What is that matrix provided by ? Other examples usually link the matrix to a texture, but since this is a generator, I have no input texture. > 2. The shader doesn't respect the paint's alpha (it should modulate its per-pixel alpha by the paint's Will do. Any examples of code already doing this ? > More questions: > 1. Is the noise defined to always also affect alpha? The current impl appears to do that. That's a shame (from a practical perspective) if one cannot get an "opaque" noise out of it. Yes, it always affects alpha, since it writes to RGBA. But setting blend modes using something like glBlendFunc(GL_ONE, GL_ZERO) get rid of the alpha. > 2. I see that your constructor doesn't take SkShader::TileMode. Perhaps that is correct, perhaps not. Can you support Skia's notion of tiling? (clamp, repeat, mirror)? I'm not sure what this is exactly. Since this is a generator, it is usually used to generate the entire area and there's no input texture to apply these modes on.
Sign in to reply to this message.
The matrix and paint is provided in setContext(), which you need to override. See any of the gradient shaders for examples. Regarding opaqueness, I see that you do affect the alpha channel of each pixel, I guess my question is: is that required. Can you have a mode where you keep all of the pixels opaque, or does that violate something important in the spec? I'm fine if supporting skia's tilemodes doesn't make sense for this (our SweepGradient also doesn't support these). Just wanted to document (at least for myself) that what you call "tiles" is different from how Skia/SkShader use that term.
Sign in to reply to this message.
On 2013/02/22 19:43:43, reed1 wrote: > Regarding opaqueness, I see that you do affect the alpha channel of each pixel, I guess my question is: is that required. Can you have a mode where you keep all of the pixels opaque, or does that violate something important in the spec? We could add an opaque mode for Skia's own internal purposes, but the spec is to produce RGBA turbulence, so we don't need an opaque mode for this spec. This generates pre-multiplied noise, so the alpha would already be "baked" into the RGB components if we use the algorithm as it is today. We can add an option to produce opaque noise without any alpha, but this would never be used by the SVG turbulence filter.
Sign in to reply to this message.
Added support for paint alpha and local matrix transformation.
Sign in to reply to this message.
Just a ping to remind you that I'd like this cl to be reviewed. Thanks.
Sign in to reply to this message.
I apologize. I have not yet installed the patch to see how it behaves with matrices, etc. Can this wait until early next week (I have to leave town for the next 2 days)?
Sign in to reply to this message.
Sure, no problem, as long as you don't simply forget about it, I'm fine with you taking your time :)
Sign in to reply to this message.
I had to update the code because of the changes to GrGLShaderBuilder.
Sign in to reply to this message.
This shader does not respect the CTM, which means when I zoom (or any other xform) the resulting colors are not affected. This our model of what a shader is, and will be very apparent on high-res monitors and thumbnails.
Sign in to reply to this message.
On 2013/03/08 16:06:22, reed1 wrote: > This shader does not respect the CTM, which means when I zoom (or any other xform) the resulting colors are not affected. This our model of what a shader is, and will be very apparent on high-res monitors and thumbnails. All right, is it possible to write a gm for that ?
Sign in to reply to this message.
For local testing, just hit up-arrow (or down-arrow) to scale the canvas in SampleApp. For a gm, just scale the canvas and then draw.
Sign in to reply to this message.
another local test is to just click-draw in SampleApp. This translate the canvas, and you'll see that the noise is locked to the pixels, not the shape/canvas.
Sign in to reply to this message.
All right, I'm not absolutely certain that this is the proper way to do this, but the ctm matrix seems to be applied properly now. https://codereview.appspot.com/7322060/diff/54001/src/effects/SkRectShaderIma... File src/effects/SkRectShaderImageFilter.cpp (right): https://codereview.appspot.com/7322060/diff/54001/src/effects/SkRectShaderIma... src/effects/SkRectShaderImageFilter.cpp:53: fShader->setLocalMatrix(ctm); Since there are no inputs to this shader, I had to pass the ctm matrix down to the shader as the local matrix to be able to use it properly.
Sign in to reply to this message.
Your shader (noise) will want to override setContext(). That call gives you the current paint (whose alpha you need to modulate with) and the CTM. https://codereview.appspot.com/7322060/diff/54001/src/effects/SkRectShaderIma... File src/effects/SkRectShaderImageFilter.cpp (right): https://codereview.appspot.com/7322060/diff/54001/src/effects/SkRectShaderIma... src/effects/SkRectShaderImageFilter.cpp:53: fShader->setLocalMatrix(ctm); On 2013/03/08 20:08:51, sugoi wrote: > Since there are no inputs to this shader, I had to pass the ctm matrix down to > the shader as the local matrix to be able to use it properly. This should not be necessary (and is wrong if the shader already has a localmatrix). Shaders see the CTM in their setContext() virtual, which is called before they are invoked.
Sign in to reply to this message.
On 2013/03/08 20:26:53, reed1 wrote: > Your shader (noise) will want to override setContext(). That call gives you the > current paint (whose alpha you need to modulate with) and the CTM. > > https://codereview.appspot.com/7322060/diff/54001/src/effects/SkRectShaderIma... > File src/effects/SkRectShaderImageFilter.cpp (right): > > https://codereview.appspot.com/7322060/diff/54001/src/effects/SkRectShaderIma... > src/effects/SkRectShaderImageFilter.cpp:53: fShader->setLocalMatrix(ctm); > On 2013/03/08 20:08:51, sugoi wrote: > > Since there are no inputs to this shader, I had to pass the ctm matrix down to > > the shader as the local matrix to be able to use it properly. > > This should not be necessary (and is wrong if the shader already has a > localmatrix). Shaders see the CTM in their setContext() virtual, which is called > before they are invoked. 1 ) I thought setContext was only used for the CPU implementation, and not the GPU implementation (I could be wrong about this) 2 ) I tried overriding setContext and I was always getting an Identity matrix, which is not the same as the ctm. Is there anything other than implementing setContext that I would need to do in order to get the proper matrix ?
Sign in to reply to this message.
On 2013/03/08 20:32:02, sugoi wrote: > On 2013/03/08 20:26:53, reed1 wrote: > > Your shader (noise) will want to override setContext(). That call gives you > the > > current paint (whose alpha you need to modulate with) and the CTM. > > > > > https://codereview.appspot.com/7322060/diff/54001/src/effects/SkRectShaderIma... > > File src/effects/SkRectShaderImageFilter.cpp (right): > > > > > https://codereview.appspot.com/7322060/diff/54001/src/effects/SkRectShaderIma... > > src/effects/SkRectShaderImageFilter.cpp:53: fShader->setLocalMatrix(ctm); > > On 2013/03/08 20:08:51, sugoi wrote: > > > Since there are no inputs to this shader, I had to pass the ctm matrix down > to > > > the shader as the local matrix to be able to use it properly. > > > > This should not be necessary (and is wrong if the shader already has a > > localmatrix). Shaders see the CTM in their setContext() virtual, which is > called > > before they are invoked. > > 1 ) I thought setContext was only used for the CPU implementation, and not the > GPU implementation (I could be wrong about this) Correct, the GPU side will have a different mechanism for you to be told what the CTM is. > 2 ) I tried overriding setContext and I was always getting an Identity matrix, > which is not the same as the ctm. Is there anything other than implementing > setContext that I would need to do in order to get the proper matrix ? Not sure what your test was when you tried that. My local test was to install your shader in a paint (outside of any imagefilters) and then draw stuff (rects, ovals, text). Then if you translate or scale the canvas (ala play with it in SampleApp), you should see interesting CTMs.
Sign in to reply to this message.
Brian or Jim can comment on getting CTM info for your gpu-shader
Sign in to reply to this message.
On 2013/03/08 20:36:26, reed1 wrote: > Brian or Jim can comment on getting CTM info for your gpu-shader You access the the CTM directly in your shader. The exact mechanism is about to change, but currently you get "inCoords" in your effect's emitCode() function. It will be the pre-CTM position in the VS. There is a wrinkle: Sometimes the view matix gets set to indentity*after* your effect is installed and so the input positions are no longer in local coords. Your effect is informed of this via the coord change matrix owned by the GrEffectStage. Your effect as written is already accounting for this here in setData: fEffectMatrix.setData(uman, m, stage.getCoordChangeMatrix(), NULL); and here in emitCode: fEffectMatrix.emitCodeMakeFSCoords2D(builder, key, vertexCoords, &vCoords); vCoords will be in local coords.
Sign in to reply to this message.
On 2013/03/08 20:44:20, bsalomon wrote: > On 2013/03/08 20:36:26, reed1 wrote: > > Brian or Jim can comment on getting CTM info for your gpu-shader > > You access the the CTM directly in your shader. The exact mechanism is about to > change, but currently you get "inCoords" in your effect's emitCode() function. > It will be the pre-CTM position in the VS. > > There is a wrinkle: Sometimes the view matix gets set to indentity*after* your > effect is installed and so the input positions are no longer in local coords. > Your effect is informed of this via the coord change matrix owned by the > GrEffectStage. Your effect as written is already accounting for this here in > setData: > > fEffectMatrix.setData(uman, m, stage.getCoordChangeMatrix(), NULL); > and here in emitCode: > fEffectMatrix.emitCodeMakeFSCoords2D(builder, key, vertexCoords, &vCoords); > > vCoords will be in local coords. I should say vCoords will be the local coords multiplied by the other matrix you're specifying to the effect matrix, "m" in your setData().
Sign in to reply to this message.
I adapted the code to support the ctm matrix and I changed the gm to use the shader directly instead of using SkRectShaderImageFilter. Note : The perlin noise algorithm needs the size of the noise as an input argument, so applying the ctm matrix to the perlin noise algorithm essentially produces crappy noise. For example, if you double the width, you'll probably get pairs of identical columns of pixels in the result. The algorithm was never intended to support transformations of any kind. The ctm matrix support that is in the code right now will produce, at best, a crude approximation of what the noise would have been if it have been produces properly without any transformations applied on it and then transformed using linear or anisotropic interpolation (of course, we can't do that either, as some transformations could lead to the creation of gigantic noise textures if we wanted to apply the transformation after the texture generation and still fill the given screen area). The noise should REALLY be generated without any transformations applied on it if we want the result to look good.
Sign in to reply to this message.
Thanks. I will install and test in the morning.
Sign in to reply to this message.
Does the CTM work in the GPU backend, too? https://codereview.appspot.com/7322060/diff/62001/src/effects/SkPerlinNoiseSh... File src/effects/SkPerlinNoiseShader.cpp (right): https://codereview.appspot.com/7322060/diff/62001/src/effects/SkPerlinNoiseSh... src/effects/SkPerlinNoiseShader.cpp:940: getLocalMatrix(), paint.getAlpha()) : this->
Sign in to reply to this message.
On 2013/03/15 15:43:18, bsalomon wrote: > Does the CTM work in the GPU backend, too? > > https://codereview.appspot.com/7322060/diff/62001/src/effects/SkPerlinNoiseSh... > File src/effects/SkPerlinNoiseShader.cpp (right): > > https://codereview.appspot.com/7322060/diff/62001/src/effects/SkPerlinNoiseSh... > src/effects/SkPerlinNoiseShader.cpp:940: getLocalMatrix(), paint.getAlpha()) : > this-> My scaling test gets a very similar result in both CPU and GPU, so yes, it seems like the local matrix contains what I need to apply that matrix (according to my gms). Without that logic, the noise looks clamped instead of scaled, so the effect of the ctm matrix is fairly obvious.
Sign in to reply to this message.
Updated the cl for the recent GrDrawEffect related changes. https://codereview.appspot.com/7322060/diff/62001/src/effects/SkPerlinNoiseSh... File src/effects/SkPerlinNoiseShader.cpp (right): https://codereview.appspot.com/7322060/diff/62001/src/effects/SkPerlinNoiseSh... src/effects/SkPerlinNoiseShader.cpp:940: getLocalMatrix(), paint.getAlpha()) : Done locally
Sign in to reply to this message.
On 2013/03/21 14:46:44, sugoi wrote: > Updated the cl for the recent GrDrawEffect related changes. > > https://codereview.appspot.com/7322060/diff/62001/src/effects/SkPerlinNoiseSh... > File src/effects/SkPerlinNoiseShader.cpp (right): > > https://codereview.appspot.com/7322060/diff/62001/src/effects/SkPerlinNoiseSh... > src/effects/SkPerlinNoiseShader.cpp:940: getLocalMatrix(), paint.getAlpha()) : > Done locally lgtm, not sure if Mike has anything else to say.
Sign in to reply to this message.
Hope this fixes the mac warnings...
Sign in to reply to this message.
lgtm
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
