Issue 6354062: Added GPU implementation of 2-point conical gradient (Closed)
|
Created:
12 years, 5 months ago by rileya Modified:
12 years, 5 months ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionAdded GPU implementation of 2-point conical gradient.
Committed: https://code.google.com/p/skia/source/detail?r=4442
Patch Set 1 #
Total comments: 20
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #
Total comments: 1
MessagesTotal messages: 16
Nice work, mostly looks good. Do our existing test cases cover both the same center and same radius cases? I believe the degenerate two pt test is testing cases around where the quadratic collapses to a linear eq. IIRC the old two pt gradient only checks for the exact pt of collapse and draws horribly when it is nearly linear. (Future improvement task for our illustrious intern?) Let's add a row to the gradients slide that uses the new code. And let's make 2 variants of it. One where the view matrix has perspective and one where the shader's local matrix has perspective. https://codereview.appspot.com/6354062/diff/1/src/effects/SkGradientShader.cpp File src/effects/SkGradientShader.cpp (right): https://codereview.appspot.com/6354062/diff/1/src/effects/SkGradientShader.cp... src/effects/SkGradientShader.cpp:2321: diffL = SkScalarSqrt(SkScalarSquare(diff.fX) + I'm pretty sure SkPoint has a length function of some sort. https://codereview.appspot.com/6354062/diff/1/src/effects/SkGradientShader.cp... src/effects/SkGradientShader.cpp:2326: SkScalar invDiffL = SkScalarInvert(diffL); One-line comment that you're rotating to align the line between the centers with the x-axis?
Sign in to reply to this message.
LGTM - nice work for your first patch! For me, the important question was - does this feel at all natural to you? Did you understand the code you were modifying? Can you think anything that'd make it easier to write this kind of shader? https://codereview.appspot.com/6354062/diff/1/src/effects/SkGradientShader.cpp File src/effects/SkGradientShader.cpp (right): https://codereview.appspot.com/6354062/diff/1/src/effects/SkGradientShader.cp... src/effects/SkGradientShader.cpp:2307: virtual BitmapType asABitmap(SkBitmap* bitmap, const? https://codereview.appspot.com/6354062/diff/1/src/effects/SkGradientShader.cp... src/effects/SkGradientShader.cpp:2321: diffL = SkScalarSqrt(SkScalarSquare(diff.fX) + On 2012/07/02 13:18:47, bsalomon wrote: > I'm pretty sure SkPoint has a length function of some sort. http://skia-autogen.googlecode.com/svn/docs/html/structSkPoint.html#a9c908f8b... https://codereview.appspot.com/6354062/diff/1/src/gpu/SkGpuDevice.cpp File src/gpu/SkGpuDevice.cpp (right): https://codereview.appspot.com/6354062/diff/1/src/gpu/SkGpuDevice.cpp#newcode554 src/gpu/SkGpuDevice.cpp:554: twoPointParams[2]))->unref(); Nit: missing 1 space on each of the 2 above lines
Sign in to reply to this message.
Good job - LGTM modulo nits & some questions. https://codereview.appspot.com/6354062/diff/1/src/effects/SkGradientShader.cpp File src/effects/SkGradientShader.cpp (right): https://codereview.appspot.com/6354062/diff/1/src/effects/SkGradientShader.cp... src/effects/SkGradientShader.cpp:2318: } diffLen maybe? https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... File src/gpu/effects/GrGradientEffects.cpp (right): https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.cpp:330: GrGLConical2Gradient(const GrProgramStageFactory& factory, nit: space below to line up https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.cpp:420: void GrGLConical2Gradient::emitVS(GrGLShaderBuilder* state, indentation on following line https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.cpp:440: void GrGLConical2Gradient::emitFS(GrGLShaderBuilder* state, indent? https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.cpp:542: void GrGLConical2Gradient::setData(const GrGLInterface* gl, line up? https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.cpp:587: GrConical2Gradient::GrConical2Gradient(GrScalar center, same here https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.cpp:606: const GrConical2Gradient& s = static_cast<const GrConical2Gradient&>(sBase); Is this really the correct equality check? https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... File src/gpu/effects/GrGradientEffects.h (right): https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.h:115: Does isPosRoot need to be part of the public interface?
Sign in to reply to this message.
Update: I changed the shader to work more like the raster implementation; this resulted in a somewhat more complex shader (more branching), but it now handles the near-degenerate case smoothly without the ugly numerical instability issues. It's still not identical to raster in the animated DegenerateTwoPtRadials test, but this seems to be because the raster version does a float->fixed conversion that overflows and rejects pixels when t > 2^15. I also believe I've fixed most of the style nits. >For me, the important question was - does this feel at all natural to you? Did >you understand the code you were modifying? Can you think anything that'd make >it easier to write this kind of shader? It was reasonably intuitive, although the code generation seemed a little tedious to work with in places. I tried to follow the style of the old radial shader and found that just about every append I made to the shader code was a line starting with a tab and ending with a newline, so some sort of function to add a line and automatically handle adding tabs and newlines might make things a little cleaner, in particular, conditionals were a little awkward (see the nested conditional in the newly modified shader, lots of \t's...). >Do our existing test cases cover both the same center and same radius cases? Yeah, the "twopointconical" gm test covers these. >Let's add a row to the gradients slide that uses the new code. And let's make 2 >variants of it. One where the view matrix has perspective and one where the >shader's local matrix has perspective. I'll do this next.
Sign in to reply to this message.
https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... File src/gpu/effects/GrGradientEffects.cpp (right): https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.cpp:606: const GrConical2Gradient& s = static_cast<const GrConical2Gradient&>(sBase); On 2012/07/02 18:12:05, robertphillips wrote: > Is this really the correct equality check? I believe so, I think this is supposed to mean equality in terms of the generated code, and being degenerate is the only thing that results in different shader code being generated, all the other properties are uniforms and can be changed. https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... File src/gpu/effects/GrGradientEffects.h (right): https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.h:115: On 2012/07/02 18:12:05, robertphillips wrote: > Does isPosRoot need to be part of the public interface? I changed the shader to work more like the raster implementation, and PosRoot was actually no longer needed, so I've now removed it entirely.
Sign in to reply to this message.
https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... File src/gpu/effects/GrGradientEffects.h (right): https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.h:119: SkScalarAbs(fRadius0) < SkScalarAbs(fCenterX1); } So you not only removed it from the public interface, but also removed it from the code completely? WIN! https://codereview.appspot.com/6354062/diff/9/src/gpu/effects/GrGradientEffec... File src/gpu/effects/GrGradientEffects.cpp (right): https://codereview.appspot.com/6354062/diff/9/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.cpp:512: // try the larger one first Nit: see line 520 https://codereview.appspot.com/6354062/diff/9/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.cpp:518: p5.c_str(), p3.c_str()); Higher-level comment (at last!): once we have a benchmark, I'm curious about the performance of this shader. We do: max if (r(max) <= 0) min if (r(min) <= 0) (4 conditionals? depends on microcode, I guess) Couldn't we just do one more-complex if? if (r(r0) <= 0 && r(r1) <= 0) Or, also, is there a reason to think min-before-max might short-circuit more often? https://codereview.appspot.com/6354062/diff/9/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.cpp:520: // smaller of the two roots Nit: this comment is not quite as bad as line 529 (qv), but doesn't tell me *why* we want the smaller of the two roots; since line 524 does, it's unnecessary noise. https://codereview.appspot.com/6354062/diff/9/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.cpp:529: // end branch Nit: this comment is evil. It doesn't tell me *anything* that isn't obvious from reading the next line, does it? Most of the comments above do a good job of explaining a rather opaque algorithm that's made more opaque by all the text substitutions, but this is evil. </soapbox>
Sign in to reply to this message.
https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... File src/gpu/effects/GrGradientEffects.cpp (right): https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.cpp:606: const GrConical2Gradient& s = static_cast<const GrConical2Gradient&>(sBase); That would make a great comment somewhere
Sign in to reply to this message.
https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... File src/gpu/effects/GrGradientEffects.cpp (right): https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.cpp:606: const GrConical2Gradient& s = static_cast<const GrConical2Gradient&>(sBase); On 2012/07/02 20:38:32, rileya wrote: > On 2012/07/02 18:12:05, robertphillips wrote: > > Is this really the correct equality check? > I believe so, I think this is supposed to mean equality in terms of the > generated code, and being degenerate is the only thing that results in different > shader code being generated, all the other properties are uniforms and can be > changed. 2 GrGLConical2Gradient objects are equal if they have equal code. However, 2 GrConical2Gradient objects are equal only if they have equal parameters. Or, so I *think*, and I wrote the API, and I even did try to document it. But then again GrRadial2Gradient and GrSweepGradient do the same thing as Riely did here, and it seems to work - but there's probably a bug hiding in there somewhere.
Sign in to reply to this message.
https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... File src/gpu/effects/GrGradientEffects.cpp (right): https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.cpp:606: const GrConical2Gradient& s = static_cast<const GrConical2Gradient&>(sBase); On 2012/07/02 20:38:32, rileya wrote: > On 2012/07/02 18:12:05, robertphillips wrote: > > Is this really the correct equality check? > I believe so, I think this is supposed to mean equality in terms of the > generated code, and being degenerate is the only thing that results in different > shader code being generated, all the other properties are uniforms and can be > changed. I don't think that's right. I believe the stage key determines code equivalence and this means "draws the same pixels". Tom is that right? Do we need to clarify the comments on isEqual() in the base class. https://codereview.appspot.com/6354062/diff/9/src/gpu/effects/GrGradientEffec... File src/gpu/effects/GrGradientEffects.cpp (right): https://codereview.appspot.com/6354062/diff/9/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.cpp:518: p5.c_str(), p3.c_str()); On 2012/07/02 20:38:56, TomH wrote: > Higher-level comment (at last!): once we have a benchmark, I'm curious about the > performance of this shader. We do: > max > if (r(max) <= 0) > min > if (r(min) <= 0) > (4 conditionals? depends on microcode, I guess) > > Couldn't we just do one more-complex if? > if (r(r0) <= 0 && r(r1) <= 0) > > Or, also, is there a reason to think min-before-max might short-circuit more > often? If there is a large per hit for the extra computation and branching would it be easy to detect the cases where the old two point grad renders the same as the new one? https://codereview.appspot.com/6354062/diff/9/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.cpp:520: // smaller of the two roots On 2012/07/02 20:38:56, TomH wrote: > Nit: this comment is not quite as bad as line 529 (qv), but doesn't tell me > *why* we want the smaller of the two roots; since line 524 does, it's > unnecessary noise. I think it's just spec'ed that way. https://codereview.appspot.com/6354062/diff/9/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.cpp:537: // discard fragment if r(t) is <= 0 Let's change the comment since this isn't really a discard anymore.
Sign in to reply to this message.
https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... File src/gpu/effects/GrGradientEffects.cpp (right): https://codereview.appspot.com/6354062/diff/1/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.cpp:606: const GrConical2Gradient& s = static_cast<const GrConical2Gradient&>(sBase); In this case it looks like the stage key is the same as this test. If this method is intended to only return true if the two stages draw the same pixels, what about calling it something like drawsSamePixels?
Sign in to reply to this message.
https://codereview.appspot.com/6354062/diff/9/src/gpu/effects/GrGradientEffec... File src/gpu/effects/GrGradientEffects.cpp (right): https://codereview.appspot.com/6354062/diff/9/src/gpu/effects/GrGradientEffec... src/gpu/effects/GrGradientEffects.cpp:520: // smaller of the two roots On 2012/07/02 20:47:48, bsalomon wrote: > On 2012/07/02 20:38:56, TomH wrote: > > Nit: this comment is not quite as bad as line 529 (qv), but doesn't tell me > > *why* we want the smaller of the two roots; since line 524 does, it's > > unnecessary noise. > > I think it's just spec'ed that way. A meta comment could read: "If there are two roots that both generate radius(t)>0, the spec says choose the larger t."
Sign in to reply to this message.
On 2012/07/02 21:06:15, reed1 wrote: > https://codereview.appspot.com/6354062/diff/9/src/gpu/effects/GrGradientEffec... > File src/gpu/effects/GrGradientEffects.cpp (right): > > https://codereview.appspot.com/6354062/diff/9/src/gpu/effects/GrGradientEffec... > src/gpu/effects/GrGradientEffects.cpp:520: // smaller of the two roots > On 2012/07/02 20:47:48, bsalomon wrote: > > On 2012/07/02 20:38:56, TomH wrote: > > > Nit: this comment is not quite as bad as line 529 (qv), but doesn't tell me > > > *why* we want the smaller of the two roots; since line 524 does, it's > > > unnecessary noise. > > > > I think it's just spec'ed that way. > > A meta comment could read: "If there are two roots that both generate > radius(t)>0, the spec says choose the larger t." And that also explains why my suggestions of combining the if()s or reversing the order of the tests don't work.
Sign in to reply to this message.
I've tweaked the comments, including a note about the spec wanting the larger root.
Sign in to reply to this message.
LGTM, the updated tests can be a separate CL if you like. https://codereview.appspot.com/6354062/diff/9003/src/gpu/effects/GrGradientEf... File src/gpu/effects/GrGradientEffects.cpp (right): https://codereview.appspot.com/6354062/diff/9003/src/gpu/effects/GrGradientEf... src/gpu/effects/GrGradientEffects.cpp:619: bool GrConical2Gradient::isEqual(const GrCustomStage& sBase) const { We should fix this to check for "gradient is the same" rather than "code is the same" for this and other gradients. I think it is OK to check this is as is since it is consistent with the others (and the consequence is only perf not correctness). I'll file a bug to fix them all.
Sign in to reply to this message.
On 2012/07/03 12:26:42, bsalomon wrote: > LGTM, the updated tests can be a separate CL if you like. > > https://codereview.appspot.com/6354062/diff/9003/src/gpu/effects/GrGradientEf... > File src/gpu/effects/GrGradientEffects.cpp (right): > > https://codereview.appspot.com/6354062/diff/9003/src/gpu/effects/GrGradientEf... > src/gpu/effects/GrGradientEffects.cpp:619: bool > GrConical2Gradient::isEqual(const GrCustomStage& sBase) const { > We should fix this to check for "gradient is the same" rather than "code is the > same" for this and other gradients. I think it is OK to check this is as is > since it is consistent with the others (and the consequence is only perf not > correctness). > > I'll file a bug to fix them all. I have a CL to fix them all, but this comment box is too small to contain it. (Actually, the problem is password management - for some reason I can't get to valentine over VPN? I think uberproxy or one of the other hacks is interfering with it.)
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
