Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
169973002:
Begin making SkPerlinNoiseShader const. (Closed)
|
Created:
6 years, 10 months ago by scroggo Modified:
6 years, 9 months ago CC:
skia-review_googlegroups.com, mtklein Base URL:
https://skia.googlesource.com/skia.git@master Visibility:
Public. |
DescriptionBegin making SkPerlinNoiseShader const.
The overall goal is to make SkShader itself immutable (BUG=skia:1976
).
The fields cannot yet be made constant, due to the constructor which
takes an SkReadBuffer. Other than that constructor, the fields are now
unchanged.
Remove setTileSize and initPaint. Merge initPaint with the constructor
of PaintingData, since it is only ever used on a new PaintingData.
Merge setTileSize with the SkPerlinNoiseShader constructor, its only
call site.
BUG=skia:1976
Committed: http://code.google.com/p/skia/source/detail?r=13682
Patch Set 1 #
Total comments: 5
Patch Set 2 : Remove PaintingData #Patch Set 3 : Make methods const and simplify const comments. #
Total comments: 10
Patch Set 4 : Flatten the original fSeed value. #Patch Set 5 : Go back to patch set 1. PaintingData has been restored. #Patch Set 6 : const methods, no default option on private constructor, mutex #Patch Set 7 : const methods, no default option on private constructor, mutex #Patch Set 8 : const methods, no default option on private constructor, mutex #
Total comments: 1
Patch Set 9 : Don't lazily initialize bitmaps. Use setPixels #
Messages
Total messages: 32 (0 generated)
Adding senorblanco@ to the discussion to get his input on this: From what I remember, the "setTileSize" function was there because it may be possible to create a filter dag without knowing what inputs will be used and, therefore, what input sizes will be use. The tile size could be dependent on the input size, so making the tile size immutable may make it difficult for a single filter dag to be used by different or unknown sized inputs. This may not be an issue today and, I hope, is not an issue for the work we are doing to replace the blink filter dag by a pure skia filter dag (currently, blink creates that shader at render time, so making it immutable is not an issue). I think tile stitching should work when using FETurbulence::applySkia(), but is currently disabled when using FETurbulence::createImageFilter(). I just want to make sure that tile stitching will not be broken or that we have a way of not breaking tile stitching somehow.
https://codereview.chromium.org/169973002/diff/1/src/effects/SkPerlinNoiseSha... File src/effects/SkPerlinNoiseShader.cpp (right): https://codereview.chromium.org/169973002/diff/1/src/effects/SkPerlinNoiseSha... src/effects/SkPerlinNoiseShader.cpp:84: PaintingData(const SkISize& tileSize, SkScalar seed, Not new to this patch, but I think the PaintingData approach is something we inherited from the WebKit implementation. Now that it is created at shader construction time from SkPerlinNoiseShader's member variables, could we simply remove it and access the member variables directly?
https://codereview.chromium.org/169973002/diff/1/include/effects/SkPerlinNois... File include/effects/SkPerlinNoiseShader.h (right): https://codereview.chromium.org/169973002/diff/1/include/effects/SkPerlinNois... include/effects/SkPerlinNoiseShader.h:95: // be made constant. Is it always feasible to make all fields const? If so, that would be great. I haven't really thought about that yet, but it feels like the right thing to do when making effects immutable. I'm just worried that there might be some initialization code in a constructor that you can't easily place into an initializer list. But maybe there isn't. All effects currently have constructors that take an SkReadBuffer as parameter. If we wanted to get rid of them (which we'd have to as you said above), maybe we can introduce a macro similar to SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS but using the factory methods instead. Would be a separate patch I suppose :)
On 2014/02/18 13:24:48, sugoi wrote: > Adding senorblanco@ to the discussion to get his input on this: > > From what I remember, the "setTileSize" function was there because it may be > possible to create a filter dag without knowing what inputs will be used and, > therefore, what input sizes will be use. The tile size could be dependent on the > input size, so making the tile size immutable may make it difficult for a single > filter dag to be used by different or unknown sized inputs. We do know at filter DAG construction time what the input is (ie., if its SkImageFilter* input is non-NULL). However, if it is NULL, this means that we use the original primitive as input. It is possible in Skia to re-use the same image filter (inside the same paint) on multiple input primitives, e.g., draw a circle and a bitmap with the same filter, with different dimensions. I have no idea what effect this has on tile size inside the noise shader. Although Blink doesn't do this currently, it will probably have to start doing this for SVG-on-SVG filters, where pinch zoom will replay the same filter with an input primitive redrawn at a new size (think image filter inside SkPicture). But perhaps we can simply compute the tile size from the input size at render time? Would this have adverse effects? If tile size is absolutely dependent on input primitive size and can't be determined at render time from the shader, then we could create an actual SkPerlinNoiseImageFilter which explicitly constructs an SkPerlinNoiseShader at render time, rather than the generic SkRectShaderImageFilter we have now. We would lose the generic nature of SkRectShaderImageFilter, though, and have to duplicate all the perlin noise parameters already in the shader into the filter. WDYT? > This may not be an > issue today and, I hope, is not an issue for the work we are doing to replace > the blink filter dag by a pure skia filter dag (currently, blink creates that > shader at render time, so making it immutable is not an issue). I think tile > stitching should work when using FETurbulence::applySkia(), but is currently > disabled when using FETurbulence::createImageFilter(). I just want to make sure > that tile stitching will not be broken or that we have a way of not breaking > tile stitching somehow.
On 2014/02/18 15:10:24, Stephen White wrote: > If tile size is absolutely dependent on input primitive size and can't be > determined at render time from the shader, then we could create an actual SkPerlinNoiseImageFilter > which explicitly constructs an SkPerlinNoiseShader at render time, rather than the generic > SkRectShaderImageFilter we have now. We would lose the generic nature of > SkRectShaderImageFilter, though, and have to duplicate all the perlin noise parameters already > in the shader into the filter. WDYT? That's a good idea. If we ever find out that this is an issue, adding a dedicated image filter for that shader will probably be an acceptable workaround, especially if it allows this shader to be immutable.
On 2014/02/18 15:30:50, sugoi wrote: > On 2014/02/18 15:10:24, Stephen White wrote: > > If tile size is absolutely dependent on input primitive size and can't be > > determined at render time from the shader, then we could create an actual > SkPerlinNoiseImageFilter > > which explicitly constructs an SkPerlinNoiseShader at render time, rather than > the generic > > SkRectShaderImageFilter we have now. We would lose the generic nature of > > SkRectShaderImageFilter, though, and have to duplicate all the perlin noise > parameters already > > in the shader into the filter. WDYT? > > That's a good idea. If we ever find out that this is an issue, adding a > dedicated image filter for that shader will probably be an acceptable > workaround, especially if it allows this shader to be immutable. This sounds fine to me, with the caveat that my knowledge of image filters is basically what Stephen told us in his presentation. If we go that route, does this seem like a reasonable CL? (I can also merge PaintingData into the shader if you want me to go ahead and make that change.) https://codereview.chromium.org/169973002/diff/1/include/effects/SkPerlinNois... File include/effects/SkPerlinNoiseShader.h (right): https://codereview.chromium.org/169973002/diff/1/include/effects/SkPerlinNois... include/effects/SkPerlinNoiseShader.h:95: // be made constant. On 2014/02/18 15:00:33, Dominik Grewe wrote: > Is it always feasible to make all fields const? If so, that would be great. I > haven't really thought about that yet, but it feels like the right thing to do > when making effects immutable. I don't know of any other way to make them immutable. > I'm just worried that there might be some initialization code in a constructor > that you can't easily place into an initializer list. But maybe there isn't. That may be so. I tend to think that's a bridge we can cross when we get there. > > All effects currently have constructors that take an SkReadBuffer as parameter. > If we wanted to get rid of them (which we'd have to as you said above), maybe we > can introduce a macro similar to > SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS but using the factory > methods instead. Would be a separate patch I suppose :) https://codereview.chromium.org/169973002/diff/1/src/effects/SkPerlinNoiseSha... File src/effects/SkPerlinNoiseShader.cpp (right): https://codereview.chromium.org/169973002/diff/1/src/effects/SkPerlinNoiseSha... src/effects/SkPerlinNoiseShader.cpp:84: PaintingData(const SkISize& tileSize, SkScalar seed, On 2014/02/18 14:58:56, Stephen White wrote: > Not new to this patch, but I think the PaintingData approach is something we > inherited from the WebKit implementation. Now that it is created at shader > construction time from SkPerlinNoiseShader's member variables, could we simply > remove it and access the member variables directly? I believe so.
On 2014/02/18 16:42:57, scroggo wrote: > If we go that route, does this seem like a reasonable CL? (I can also merge > PaintingData into the shader if you want me to go ahead and make that change.) Or should we investigate its repercussions on the future use of perlin noise first?
Ping! We'd like to constify the shaders this quarter. Is there work I can help pick up here?
On 2014/03/03 14:41:57, tomhudson wrote: > Ping! We'd like to constify the shaders this quarter. Is there work I can help > pick up here? Stephen and Alexis, what are your thoughts on this CL? Does it mesh with how you imagine the shader working in the future? For the present, I haven't actually changed the behavior, AFAICT (I removed only private functions that were never called). From my perspective, patch set 1 is ready to go (just needs an LGTM). Patch set 2 follows Stephen's suggestion of storing the PaintingData directly on the SkPerlinNoiseShader. Patch set 2/3 (3 is small changes) is not fully tested on the GPU side, because my laptop's version of mesa doesn't support the features the GM uses. PTAL https://codereview.chromium.org/169973002/diff/1/src/effects/SkPerlinNoiseSha... File src/effects/SkPerlinNoiseShader.cpp (right): https://codereview.chromium.org/169973002/diff/1/src/effects/SkPerlinNoiseSha... src/effects/SkPerlinNoiseShader.cpp:84: PaintingData(const SkISize& tileSize, SkScalar seed, On 2014/02/18 16:42:57, scroggo wrote: > On 2014/02/18 14:58:56, Stephen White wrote: > > Not new to this patch, but I think the PaintingData approach is something we > > inherited from the WebKit implementation. Now that it is created at shader > > construction time from SkPerlinNoiseShader's member variables, could we simply > > remove it and access the member variables directly? > > I believe so. Done. This is a much more pervasive change, and reitveld did not handle the diffs as well as I would have hoped, but I believe I haven't changed any behavior meaningfully. https://codereview.chromium.org/169973002/diff/140001/include/effects/SkPerli... File include/effects/SkPerlinNoiseShader.h (right): https://codereview.chromium.org/169973002/diff/140001/include/effects/SkPerli... include/effects/SkPerlinNoiseShader.h:71: struct StitchData { StitchData is now defined in the class declaration because PaintingData.fStitchDataInit has moved into SkPerlinNoiseShader. https://codereview.chromium.org/169973002/diff/140001/include/effects/SkPerli... include/effects/SkPerlinNoiseShader.h:116: mutable int fSeed; fSeed is now an int, like PaintingData::fSeed was. https://codereview.chromium.org/169973002/diff/140001/include/effects/SkPerli... include/effects/SkPerlinNoiseShader.h:120: static const int kBlockSize = 256; Now defined here for members referencing it that used to be on PaintingData. https://codereview.chromium.org/169973002/diff/140001/include/effects/SkPerli... include/effects/SkPerlinNoiseShader.h:131: mutable SkBitmap fPermutationsBitmap; I changed these from pointers on fPaintingData to full members to avoid the new/delete. https://codereview.chromium.org/169973002/diff/140001/include/effects/SkPerli... include/effects/SkPerlinNoiseShader.h:138: void init(); This should now be the only function that modifies any members (besides the ones marked mutable). It is only called by the constructors. https://codereview.chromium.org/169973002/diff/140001/src/effects/SkPerlinNoi... File src/effects/SkPerlinNoiseShader.cpp (right): https://codereview.chromium.org/169973002/diff/140001/src/effects/SkPerlinNoi... src/effects/SkPerlinNoiseShader.cpp:144: if (fTileSize.isEmpty()) { stitch() is now part of init.
https://codereview.chromium.org/169973002/diff/140001/src/effects/SkPerlinNoi... File src/effects/SkPerlinNoiseShader.cpp (right): https://codereview.chromium.org/169973002/diff/140001/src/effects/SkPerlinNoi... src/effects/SkPerlinNoiseShader.cpp:246: fSeed = buffer.readInt(); Hmmm. Isn't this going to require an SkPicture version bump? In this new backward-compatibility serialization world, I think we'll have to support the old format too (check w/Brian). I also find it strange that we have a value that's both serialized and mutable (changed on every call to random()). This doesn't seem right to me, but perhaps I'm misunderstanding. Maybe Alexis can tell me that my suggestion of getting rid of PaintingData was a bad one. :) Another option might be to backtrack a little, keep the mutable data in PaintingData, and leave the original parameters in the shader (everything that's const-able).
https://codereview.chromium.org/169973002/diff/140001/src/effects/SkPerlinNoi... File src/effects/SkPerlinNoiseShader.cpp (right): https://codereview.chromium.org/169973002/diff/140001/src/effects/SkPerlinNoi... src/effects/SkPerlinNoiseShader.cpp:246: fSeed = buffer.readInt(); On 2014/03/05 14:32:58, Stephen White wrote: > Hmmm. Isn't this going to require an SkPicture version bump? In this new > backward-compatibility serialization world, I think we'll have to support the > old format too (check w/Brian). > > I also find it strange that we have a value that's both serialized and mutable > (changed on every call to random()). This doesn't seem right to me, but perhaps > I'm misunderstanding. > > Maybe Alexis can tell me that my suggestion of getting rid of PaintingData was a > bad one. :) Another option might be to backtrack a little, keep the mutable data > in PaintingData, and leave the original parameters in the shader (everything > that's const-able). Hmmm... In this const shader world, is there no way to cleanly compute and store some values that will be needed later on during processing? As we discussed earlier this week, currently, we could compute these values every time we render since the shader is recreated every time we render it (although that may not be the case forever). For now, my opinion is that we should simplify this code as much as possible for the current case and worry about other considerations when we get there. I don't want to delay this effort while not being entirely sure of what will be needed later on.
On 2014/03/05 16:50:32, sugoi wrote: > https://codereview.chromium.org/169973002/diff/140001/src/effects/SkPerlinNoi... > File src/effects/SkPerlinNoiseShader.cpp (right): > > https://codereview.chromium.org/169973002/diff/140001/src/effects/SkPerlinNoi... > src/effects/SkPerlinNoiseShader.cpp:246: fSeed = buffer.readInt(); > On 2014/03/05 14:32:58, Stephen White wrote: > > Hmmm. Isn't this going to require an SkPicture version bump? In this new > > backward-compatibility serialization world, I think we'll have to support the > > old format too (check w/Brian). > > > > I also find it strange that we have a value that's both serialized and mutable > > (changed on every call to random()). This doesn't seem right to me, but > perhaps > > I'm misunderstanding. > > > > Maybe Alexis can tell me that my suggestion of getting rid of PaintingData was > a > > bad one. :) Another option might be to backtrack a little, keep the mutable > data > > in PaintingData, and leave the original parameters in the shader (everything > > that's const-able). > > Hmmm... In this const shader world, is there no way to cleanly compute and store > some values that will be needed later on during processing? As we discussed > earlier this week, currently, we could compute these values every time we render > since the shader is recreated every time we render it (although that may not be > the case forever). It's my understanding that all member variables will be essentially immutable after construction, so the only way to precompute values would be to do it in the constructor. Perhaps I'm wrong about that, though. I don't think it will be possible to cache values computed during processing, and cache them for a later call to processing. All caches will have to be external to the class (this is what I'm planning to do for image filters). > For now, my opinion is that we should simplify this code as much as possible for > the current case and worry about other considerations when we get there. I don't > want to delay this effort while not being entirely sure of what will be needed > later on. I'm not sure what you're saying. Do you prefer this version or the previous version of this patch?
1. We are absolutely on the road to reentrant-safe shaders. 2. This does not strictly require strict immutability of fields, but if they are mutated, it must be done in a thread-reentrant-safe manner 3. Gradients also have a deferred cache (their table of colors) that will need to be reentrant-safe-retrofited. 4. I can imagine at least 3 different ways to do this, but there are likely many more... - mutex lock around the code that "stores" the computed cache - some tricky SkOnce thing that is somehow cheaper than a mutex - us an external cache (which will have to have a mutex) adding mike and leon, who are thinking about these issues for other shaders.
https://codereview.chromium.org/169973002/diff/140001/src/effects/SkPerlinNoi... File src/effects/SkPerlinNoiseShader.cpp (right): https://codereview.chromium.org/169973002/diff/140001/src/effects/SkPerlinNoi... src/effects/SkPerlinNoiseShader.cpp:246: fSeed = buffer.readInt(); On 2014/03/05 16:50:33, sugoi wrote: > On 2014/03/05 14:32:58, Stephen White wrote: > > Hmmm. Isn't this going to require an SkPicture version bump? In this new > > backward-compatibility serialization world, I think we'll have to support the > > old format too (check w/Brian). > > > > I also find it strange that we have a value that's both serialized and mutable > > (changed on every call to random()). This doesn't seem right to me, but > perhaps > > I'm misunderstanding. > > > > Maybe Alexis can tell me that my suggestion of getting rid of PaintingData was > a > > bad one. :) Another option might be to backtrack a little, keep the mutable > data > > in PaintingData, and leave the original parameters in the shader (everything > > that's const-able). > > Hmmm... In this const shader world, is there no way to cleanly compute and store > some values that will be needed later on during processing? What do you mean by "later on during processing?" In the const shader world, the only way to store data for the future is in SkShader::setContext. setContext is called once before each draw, and this is where data gets cached that will be used for that draw. Currently, that data lives on the shader, but in the const shader world, it will be eliminated after the draw (https://codereview.chromium.org/160103002/ shows an early, rough look at what that looks like). > As we discussed > earlier this week, currently, we could compute these values every time we render > since the shader is recreated every time we render it (although that may not be > the case forever). I think I didn't quite follow this part of the discussion, which seemed to be largely about Blink and image filters. Right now we do compute all the values at shader creation time (with or without my changes - without my changes there were private functions that theoretically could be called to recompute all the values, but these functions were never called). If you create this shader at render time (do you mean before each draw?), this CL should not impact that use. If, on the other hand, you want to be able to reuse the same shader with different inputs at different times, I'm not sure how to provide that functionality without sacrificing immutable shaders (which we think would be useful for faster record times and cheaper multithreaded drawing). I think that's where Stephen's suggestion comes in, where some other class creates a new shader each time we need new values. In that case, I still think this CL is the right approach, and we can figure out how to make that work in a future CL. > > For now, my opinion is that we should simplify this code as much as possible for > the current case and worry about other considerations when we get there. I don't > want to delay this effort while not being entirely sure of what will be needed > later on. That sounds good to me. Are there specific ways you think this code should be simplified? https://codereview.chromium.org/169973002/diff/140001/src/effects/SkPerlinNoi... src/effects/SkPerlinNoiseShader.cpp:246: fSeed = buffer.readInt(); On 2014/03/05 14:32:58, Stephen White wrote: > Hmmm. Isn't this going to require an SkPicture version bump? In this new > backward-compatibility serialization world, I think we'll have to support the > old format too (check w/Brian). Yes, this would require a picture version bump. In my mind, it makes more sense to store an int than a scalar, since we always truncate it (it seems odd that the input is a scalar - is this part of a webkit restriction?). On the other hand, I think the cost of storing a scalar is low, so I'm fine with leaving it a scalar and leaving the picture version unchanged. > > I also find it strange that we have a value that's both serialized and mutable > (changed on every call to random()). This doesn't seem right to me, but perhaps > I'm misunderstanding. No, you are correct. I overlooked that. I was thinking we never again need the original value of SkPerlinNoiseShader::fSeed which is *almost* true. The one time we do need it is for flattening (although truly we only need the truncated version). random() is only called in init (which in turn is only called by the constructor), and come to think of it, there's no good reason to modify the original seed value. I've updated the code to just modify a local variable. > > Maybe Alexis can tell me that my suggestion of getting rid of PaintingData was a > bad one. :) Another option might be to backtrack a little, keep the mutable data > in PaintingData, and leave the original parameters in the shader (everything > that's const-able). Alexis, what do you think? Now that I've fixed fSeed, the only things that are modified outside of init (and therefore should be mutable) will be the bitmaps. I think removing the PaintingData is a bit simpler.
On 2014/03/05 17:41:46, Stephen White wrote: > On 2014/03/05 16:50:32, sugoi wrote: > > > https://codereview.chromium.org/169973002/diff/140001/src/effects/SkPerlinNoi... > > File src/effects/SkPerlinNoiseShader.cpp (right): > > > > > https://codereview.chromium.org/169973002/diff/140001/src/effects/SkPerlinNoi... > > src/effects/SkPerlinNoiseShader.cpp:246: fSeed = buffer.readInt(); > > On 2014/03/05 14:32:58, Stephen White wrote: > > > Hmmm. Isn't this going to require an SkPicture version bump? In this new > > > backward-compatibility serialization world, I think we'll have to support > the > > > old format too (check w/Brian). > > > > > > I also find it strange that we have a value that's both serialized and > mutable > > > (changed on every call to random()). This doesn't seem right to me, but > > perhaps > > > I'm misunderstanding. > > > > > > Maybe Alexis can tell me that my suggestion of getting rid of PaintingData > was > > a > > > bad one. :) Another option might be to backtrack a little, keep the mutable > > data > > > in PaintingData, and leave the original parameters in the shader (everything > > > that's const-able). > > > > Hmmm... In this const shader world, is there no way to cleanly compute and > store > > some values that will be needed later on during processing? As we discussed > > earlier this week, currently, we could compute these values every time we > render > > since the shader is recreated every time we render it (although that may not > be > > the case forever). > > It's my understanding that all member variables will be essentially immutable > after construction, so the only way to precompute values would be to do it in > the constructor. Perhaps I'm wrong about that, though. > > I don't think it will be possible to cache values computed during processing, > and cache them for a later call to processing. All caches will have to be > external to the class (this is what I'm planning to do for image filters). > > > For now, my opinion is that we should simplify this code as much as possible for > > the current case and worry about other considerations when we get there. I don't > > want to delay this effort while not being entirely sure of what will be needed > > later on. > > I'm not sure what you're saying. Do you prefer this version or the previous > version of this patch? The new version adds a lot of code in the header file. If constness can be achieved without this, I'd rather not have all this code in there. I think the previous version was simpler, but if there's a reason why the new version is necessary to achieve constness, then so be it. All I'm saying is that, for now, the goal should be to get the simplest one that works and we'll adapt our code later on if we need to. I don't really mind which solution is the simplest, as long as there's a valid reasoning behind why it's simpler. I wasn't explicitly saying that the new solution was more complicated (although it looks more complicated). If we can do a minimal number of changes and achieve constness, we should do that. I'm simply trying to avoid adding complexity by taking into account requirements that we may never have for this class.
On 2014/03/05 18:24:05, scroggo wrote: > What do you mean by "later on during processing?" In the const shader world, the > only way to store data for the future is in SkShader::setContext. setContext is > called once before each draw, and this is where data gets cached that will be > used for that draw. Currently, that data lives on the shader, but in the const > shader world, it will be eliminated after the draw > (https://codereview.chromium.org/160103002/ shows an early, rough look at what > that looks like). I meant caching bitmaps, since these could be affected by the context when tiling is on. But we'll have a workaround of recreating the shader each time we render if the shader constness doesn't allow this (note that we already recreated the DAG at each render, so we aren't losing much right now). > I think I didn't quite follow this part of the discussion, which seemed to be > largely about Blink and image filters. Right now we do compute all the values at > shader creation time (with or without my changes - without my changes there were > private functions that theoretically could be called to recompute all the > values, but these functions were never called). If you create this shader at > render time (do you mean before each draw?), this CL should not impact that use. > If, on the other hand, you want to be able to reuse the same shader with > different inputs at different times, I'm not sure how to provide that > functionality without sacrificing immutable shaders (which we think would be > useful for faster record times and cheaper multithreaded drawing). I think > that's where Stephen's suggestion comes in, where some other class creates a new > shader each time we need new values. In that case, I still think this CL is the > right approach, and we can figure out how to make that work in a future CL. The solution would be to have an image filter that creates a shader on every draw. If we hit that issue, we'll just do that for now. As I said, this is already the case, so this is just preventing some future hypothetical optimization that may not happen in the short/mid term. > That sounds good to me. Are there specific ways you think this code should be > simplified? I think the code is ok, but there's a lot of new stuff in the header file. I'm not going to oppose the change if there's really no way around it, though. > Yes, this would require a picture version bump. In my mind, it makes more sense > to store an int than a scalar, since we always truncate it (it seems odd that > the input is a scalar - is this part of a webkit restriction?). On the other > hand, I think the cost of storing a scalar is low, so I'm fine with leaving it a > scalar and leaving the picture version unchanged. Yes, that's probably better for now. > Alexis, what do you think? Now that I've fixed fSeed, the only things that are > modified outside of init (and therefore should be mutable) will be the bitmaps. > I think removing the PaintingData is a bit simpler. I have no particular issue with removing PaintingData.
reed wrote: > 1. We are absolutely on the road to reentrant-safe shaders. > 2. This does not strictly require strict immutability of fields, but if they are > mutated, it must be done in a thread-reentrant-safe manner > 3. Gradients also have a deferred cache (their table of colors) that will need > to be reentrant-safe-retrofited. Don't the tables get computed once in setContext? My plan is to do all that caching in a new object, created in setContext (see https://docs.google.com/a/google.com/document/d/13HHTH6O9QL_gTBNBRZCBzCP8JwsG... for my writeup). I don't think gradients need to do anything special (though it's possible I've overlooked something). > 4. I can imagine at least 3 different ways to do this, but there are likely many > more... > > - mutex lock around the code that "stores" the computed cache > - some tricky SkOnce thing that is somehow cheaper than a mutex > - us an external cache (which will have to have a mutex) If I understand the image filter goal correctly, a mutex doesn't solve the problem. If each successive draw should have values that depend on the computed values in the last draw, different threads grabbing the mutex at different times will have different results. > > adding mike and leon, who are thinking about these issues for other shaders. Leon's already here, this is my change ;) sugoi wrote: > The new version adds a lot of code in the header file. If constness can be > achieved without this, I'd rather not have all this code in there. > I think the previous version was simpler, but if there's a reason why the new > version is necessary to achieve constness, then so be it. All I'm saying is > that, for now, the goal should be to get the simplest one that works and we'll > adapt our code later on if we need to. I don't really mind which solution is the > simplest, as long as there's a valid reasoning behind why it's simpler. I wasn't > explicitly saying that the new solution was more complicated (although it looks > more complicated). If we can do a minimal number of changes and achieve > constness, we should do that. I'm simply trying to avoid adding complexity by > taking into account requirements that we may never have for this class. sgtm. Most of the changes, including moving code into the header file, were to remove PaintingData, not to enforce constness. If a big priority is to keep things out of the header file and to make the smallest necessary change (which I agree are both good goals), patch set 1 is the way to go. This discussion has made me realize that the calls to get the bitmaps aren't threadsafe. I'll put up a CL that is mostly patch set 1 soon.
would a quick VC help discuss some of these issues?
On 2014/03/05 18:50:50, reed1 wrote: > would a quick VC help discuss some of these issues? On my end, I think I understand the desires now. If patch set 1 is close to making Alexis and Stephen happy for now, I don't see the need for a VC. Alexis and Stephen, what do you think of patch set 1?
api lgtm
On 2014/03/05 19:02:35, scroggo wrote: > On 2014/03/05 18:50:50, reed1 wrote: > > would a quick VC help discuss some of these issues? > > On my end, I think I understand the desires now. If patch set 1 is close to > making Alexis and Stephen happy for now, I don't see the need for a VC. > > Alexis and Stephen, what do you think of patch set 1? On my side, I'm fine with patch set 1.
On 2014/03/05 19:41:14, sugoi1 wrote: > On 2014/03/05 19:02:35, scroggo wrote: > > On 2014/03/05 18:50:50, reed1 wrote: > > > would a quick VC help discuss some of these issues? > > > > On my end, I think I understand the desires now. If patch set 1 is close to > > making Alexis and Stephen happy for now, I don't see the need for a VC. > > > > Alexis and Stephen, what do you think of patch set 1? > > On my side, I'm fine with patch set 1. Patch set 8 is up and ready for review. PTAL. (5 just reverts back to 1, and 8 makes some small changes I think are important. 6 and 7 are failed partial uploads.)
LGTM, but please consider my comment about the bitmaps. https://codereview.chromium.org/169973002/diff/240001/src/effects/SkPerlinNoi... File src/effects/SkPerlinNoiseShader.cpp (right): https://codereview.chromium.org/169973002/diff/240001/src/effects/SkPerlinNoi... src/effects/SkPerlinNoiseShader.cpp:251: SkAutoMutexAcquire autoMutex(fPermutationsMutex); Maybe I'm being dense, but can these bitmaps just be created in the constructor, obviating the need for a mutex? Do they have any parameters which must be computed at processing time?
On 2014/03/05 21:37:49, Stephen White wrote: > LGTM, but please consider my comment about the bitmaps. > > https://codereview.chromium.org/169973002/diff/240001/src/effects/SkPerlinNoi... > File src/effects/SkPerlinNoiseShader.cpp (right): > > https://codereview.chromium.org/169973002/diff/240001/src/effects/SkPerlinNoi... > src/effects/SkPerlinNoiseShader.cpp:251: SkAutoMutexAcquire > autoMutex(fPermutationsMutex); > Maybe I'm being dense, but can these bitmaps just be created in the constructor, > obviating the need for a mutex? Do they have any parameters which must be > computed at processing time? Ahh, I see now they're only used on the GPU path. So it would incur some unnecessary cost for the raster path, since the bitmaps memcpy() in their data. I think a better fix would be to assign the pixel data directly via setPixels(), avoiding the memcpy(). Then we could just throw the SkBitmaps on the stack and not keep them around, since the cost of building them would be really low. Would that work?
On 2014/03/05 21:46:24, Stephen White wrote: > On 2014/03/05 21:37:49, Stephen White wrote: > > LGTM, but please consider my comment about the bitmaps. > > > > > https://codereview.chromium.org/169973002/diff/240001/src/effects/SkPerlinNoi... > > File src/effects/SkPerlinNoiseShader.cpp (right): > > > > > https://codereview.chromium.org/169973002/diff/240001/src/effects/SkPerlinNoi... > > src/effects/SkPerlinNoiseShader.cpp:251: SkAutoMutexAcquire > > autoMutex(fPermutationsMutex); > > Maybe I'm being dense, but can these bitmaps just be created in the > constructor, > > obviating the need for a mutex? Do they have any parameters which must be > > computed at processing time? > > Ahh, I see now they're only used on the GPU path. So it would incur some > unnecessary cost for the raster path, since the bitmaps memcpy() in their data. > > I think a better fix would be to assign the pixel data directly via setPixels(), > avoiding the memcpy(). Then we could just throw the SkBitmaps on the stack and > not keep them around, since the cost of building them would be really low. Would > that work? Exactly. Yes, I think that's the best solution. New patch isn't ready yet but I'll post soon.
On 2014/03/05 21:46:24, Stephen White wrote: > On 2014/03/05 21:37:49, Stephen White wrote: > > LGTM, but please consider my comment about the bitmaps. > > > > > https://codereview.chromium.org/169973002/diff/240001/src/effects/SkPerlinNoi... > > File src/effects/SkPerlinNoiseShader.cpp (right): > > > > > https://codereview.chromium.org/169973002/diff/240001/src/effects/SkPerlinNoi... > > src/effects/SkPerlinNoiseShader.cpp:251: SkAutoMutexAcquire > > autoMutex(fPermutationsMutex); > > Maybe I'm being dense, but can these bitmaps just be created in the > constructor, > > obviating the need for a mutex? Do they have any parameters which must be > > computed at processing time? > > Ahh, I see now they're only used on the GPU path. So it would incur some > unnecessary cost for the raster path, since the bitmaps memcpy() in their data. > > I think a better fix would be to assign the pixel data directly via setPixels(), > avoiding the memcpy(). Then we could just throw the SkBitmaps on the stack and > not keep them around, since the cost of building them would be really low. Would > that work? Just to answer my own question: it'd work, but we'd then be uploading all the texture data on each draw, since a new SkBitmap would defeat Ganesh's cache. Probably not a huge deal (it's only 4.25K of data, and we're already re-creating the SkShader on every draw in Blink currently). I guess if mutexes are acceptable for this type of cache, the current approach is fine. Race conditions make me nervous, though...
On 2014/03/05 22:09:53, Stephen White wrote: > On 2014/03/05 21:46:24, Stephen White wrote: > > On 2014/03/05 21:37:49, Stephen White wrote: > > > LGTM, but please consider my comment about the bitmaps. > > > > > > > > > https://codereview.chromium.org/169973002/diff/240001/src/effects/SkPerlinNoi... > > > File src/effects/SkPerlinNoiseShader.cpp (right): > > > > > > > > > https://codereview.chromium.org/169973002/diff/240001/src/effects/SkPerlinNoi... > > > src/effects/SkPerlinNoiseShader.cpp:251: SkAutoMutexAcquire > > > autoMutex(fPermutationsMutex); > > > Maybe I'm being dense, but can these bitmaps just be created in the > > constructor, > > > obviating the need for a mutex? Do they have any parameters which must be > > > computed at processing time? > > > > Ahh, I see now they're only used on the GPU path. So it would incur some > > unnecessary cost for the raster path, since the bitmaps memcpy() in their > data. > > > > I think a better fix would be to assign the pixel data directly via > setPixels(), > > avoiding the memcpy(). Then we could just throw the SkBitmaps on the stack and > > not keep them around, since the cost of building them would be really low. > Would > > that work? > > Just to answer my own question: it'd work, but we'd then be uploading all the > texture data on each draw, since a new SkBitmap would defeat Ganesh's cache. > Probably not a huge deal (it's only 4.25K of data, and we're already re-creating > the SkShader on every draw in Blink currently). > > I guess if mutexes are acceptable for this type of cache, the current approach > is fine. Race conditions make me nervous, though... Race condition removed. I've followed your suggestion and initialized the bitmaps in the constructor. The bitmaps also only exist now if SK_SUPPORT_GPU is set and SK_USE_SIMPLEX_NOISE is not defined, as otherwise they are not needed. PTAL.
Great! I think this'll get around my worry about defeating Ganesh cacheing, too, since the bitmaps are persistent. LGTM
The CQ bit was checked by scroggo@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/scroggo@google.com/169973002/260001
Message was sent while issue was closed.
Change committed as 13682 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
