Issue 6104043: Initial version of AA clip A8/R8 mask creation (Closed)
|
Created:
12 years, 6 months ago by robertphillips Modified:
12 years, 6 months ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionThis version simply generates the A8/R8 map. I will need to work with Tom to create the shaders that use them.
There is also plenty more optimization that could be done.
Patch Set 1 #
Total comments: 33
Patch Set 2 : Addressed code review issues #Patch Set 3 : Removed unnecessary param from drawClipShape #
MessagesTotal messages: 7
http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp File src/gpu/GrClipMaskManager.cpp (right): http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:553: // TODO: shouldn't the "getOp" above use the "op" determined below? Brian? http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:640: // TODO: just use GrContext's now that non-AA limit has been removed opinion?
Sign in to reply to this message.
Just for scheduling - my current plan is to try to get the Convolution code ready on Friday, since I'm busy with the conference through Thursday. We can look at the A8/R8 shaders next week. I'm looking forward to it, but I'll be distracted by an M20 bug that's been assigned to me, where I think I'll need to make several large WebKit changes and get those signed off on by way too many reviewers.
Sign in to reply to this message.
I actually don't think we need modifications to the existing shader generator to support drawing with the generated mask. We might need to bump kNumStages by one to support all the following at once: an SkShader a glyph mask a mask filter a soft clip But the soft clip should be no different than drawing with a glyph or mask filter. http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp File src/gpu/GrClipMaskManager.cpp (right): http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:263: return drawPath(gpu, temp, kEvenOdd_PathFill, clipIn.getDoAA(index)); this-> http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:268: return drawPath(gpu, this-> http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:277: GrDrawState* drawState, is it necessary to pass drawState or can we just use gpu->drawState()? http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:299: void clear(GrGpu* gpu, GrDrawState* drawState, ditto http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:303: // zap entire target to specified color There is a GrDrawTarget::clear() call that boils down to glClear() http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:340: bounds = rtRect; yikes... I guess we'd have to rebuild for a different rt. If we know that all the control points of all the clip elements are in some reasonable finite bounds then we could use that as the clip bounds and set the texture to clamp mode. http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:347: bounds.outset(SkIntToScalar(1), SkIntToScalar(1)); You can say SK_Scalar1, ever so slightly more straight-forward. http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:378: drawState->setColor(0xffffffff); redundant with the asr reset http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:380: GrDrawState::AutoViewMatrixRestore avmr; I don't think you need this... ~asr will restore the previous matrix. http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:411: // for this path - refactor the stencil path so it can handle I think if we did any real work before a replace op then we've done something wrong. I think we should always start from the last replace (which effectively is the same as treating the replace as an intersect since the clip starts open) http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:448: drawTexture(gpu, drawState, accum, newRTBounds, temp); this-> http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:553: // TODO: shouldn't the "getOp" above use the "op" determined below? On 2012/04/20 22:01:01, robertphillips wrote: > Brian? It seems like you have to be right about that. Let's change it and cross our fingers that GM would catch a regression. http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:640: // TODO: just use GrContext's now that non-AA limit has been removed On 2012/04/20 22:01:01, robertphillips wrote: > opinion? It may make sense but we'd have to reconstruct a GrPaint from the GrDrawState which would be somewhat awkward. Perhaps some refactoring of the GrContext drawPath would help. It'd be nice not to have two pr chains. Alternatively, some planned refactoring of GrPaint to be a public-facing wrapper of a GrDrawState may make calling the existing drawPath more natural.
Sign in to reply to this message.
http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp File src/gpu/GrClipMaskManager.cpp (right): http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:314: // Create a 8-bit clip mask in R8/A8 Super minor but the R8/A8 fuzziness is a GLism. In this code I think we can just say an "a8" or an "alpha mask" with the implicit understanding that api-specific code will decide how to map that to a api-level format. http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:429: clear(gpu, drawState, temp, newRTBounds, 0x00000000); Here if we started with a RGBA target I think we could draw the temp into RGB and then blend them together. At some point it might be worth timing (esp on SGX where target switching is killer). We'd prolly want to walk the clip stack ahead of time to check out what the ops are to know if we should create an alpha or rgba target.
Sign in to reply to this message.
Also added compile-time #define to turn off AA clipping http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp File src/gpu/GrClipMaskManager.cpp (right): http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:263: return drawPath(gpu, temp, kEvenOdd_PathFill, clipIn.getDoAA(index)); On 2012/04/24 00:29:07, bsalomon wrote: > this-> Done. http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:268: return drawPath(gpu, On 2012/04/24 00:29:07, bsalomon wrote: > this-> Done. http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:277: GrDrawState* drawState, On 2012/04/24 00:29:07, bsalomon wrote: > is it necessary to pass drawState or can we just use gpu->drawState()? Done. http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:299: void clear(GrGpu* gpu, GrDrawState* drawState, On 2012/04/24 00:29:07, bsalomon wrote: > ditto Done. http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:303: // zap entire target to specified color On 2012/04/24 00:29:07, bsalomon wrote: > There is a GrDrawTarget::clear() call that boils down to glClear() Done - although I called the GrGpu's clear. http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:314: // Create a 8-bit clip mask in R8/A8 On 2012/04/24 00:46:50, bsalomon wrote: > Super minor but the R8/A8 fuzziness is a GLism. In this code I think we can just > say an "a8" or an "alpha mask" with the implicit understanding that api-specific > code will decide how to map that to a api-level format. Done. http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:340: bounds = rtRect; I have noted this down as a possible future optimization http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:347: bounds.outset(SkIntToScalar(1), SkIntToScalar(1)); I would like to leave this as it is b.c. it is useful when debugging to bump the outset to ~10 to see what is going on outside of the drawing area. http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:378: drawState->setColor(0xffffffff); On 2012/04/24 00:29:07, bsalomon wrote: > redundant with the asr reset Done. http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:380: GrDrawState::AutoViewMatrixRestore avmr; On 2012/04/24 00:29:07, bsalomon wrote: > I don't think you need this... ~asr will restore the previous matrix. Done. http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:411: // for this path - refactor the stencil path so it can handle I just meant that replace only requires one texture for this path while intersect requires two. I definitely want to keep the optimization where we ignore everything before the last replace. http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:429: clear(gpu, drawState, temp, newRTBounds, 0x00000000); Noted http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:448: drawTexture(gpu, drawState, accum, newRTBounds, temp); On 2012/04/24 00:29:07, bsalomon wrote: > this-> Done. http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:553: // TODO: shouldn't the "getOp" above use the "op" determined below? On 2012/04/24 00:29:07, bsalomon wrote: > On 2012/04/20 22:01:01, robertphillips wrote: > > Brian? > It seems like you have to be right about that. Let's change it and cross our > fingers that GM would catch a regression. Done. http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:640: // TODO: just use GrContext's now that non-AA limit has been removed I'll leave it be for now.
Sign in to reply to this message.
LGTM http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp File src/gpu/GrClipMaskManager.cpp (right): http://codereview.appspot.com/6104043/diff/1/src/gpu/GrClipMaskManager.cpp#ne... src/gpu/GrClipMaskManager.cpp:411: // for this path - refactor the stencil path so it can handle On 2012/04/27 13:47:08, robertphillips wrote: > I just meant that replace only requires one texture for this path while > intersect requires two. I definitely want to keep the optimization where we > ignore everything before the last replace. got it
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
