Towards NV_path_rendering support

[bsalomon, 2012-06-08 13:09:40]

I'd like to land this partial implementation as a checkpoint. This adds the
class GrPath, subclass GrGLPath, and (uncalled) member to create a GrPath on a
GrGpu and a stub entry to draw it to the stencil buffer via GrDrawTarget.

[robertphillips, 2012-06-08 16:16:55]

LGTM + nits

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrDrawTarget.h#newcod...
src/gpu/GrDrawTarget.h:474: */
Everyone else has params?

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrDrawTarget.h#newcod...
src/gpu/GrDrawTarget.h:1049: int vertexCount) = 0;
Well ... except for this guy.

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrGpu.h#newcode350
src/gpu/GrGpu.h:350: protected:
Move this comment back to setupClipAndFlushState?

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrGpu.h#newcode462
src/gpu/GrGpu.h:462: uint32_t numVertices) = 0;
param names?

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrPath.h
File src/gpu/GrPath.h (right):

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrPath.h#newcode30
src/gpu/GrPath.h:30: /*
Why two?

http://codereview.appspot.com/6302049/diff/1023/src/gpu/gl/GrGLPath.h
File src/gpu/gl/GrGLPath.h (right):

http://codereview.appspot.com/6302049/diff/1023/src/gpu/gl/GrGLPath.h#newcode13
src/gpu/gl/GrGLPath.h:13: #include "gl/GrGLInterface.h"
Do we need the interface header here?

http://codereview.appspot.com/6302049/diff/1023/src/gpu/gl/GrGLPath.h#newcode14
src/gpu/gl/GrGLPath.h:14: #include "SkPath.h"
Can't we just forward declare SkPath here?

http://codereview.appspot.com/6302049/diff/1023/src/gpu/gl/GrGLPath.h#newcode29
src/gpu/gl/GrGLPath.h:29: size_t sizeInBytes() const { return 100; }
100?

[bsalomon, 2012-06-08 18:43:34]

Updated based on Rob's comments (even the param names which pained me to add).

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrDrawTarget.h#newcod...
src/gpu/GrDrawTarget.h:474: */
On 2012/06/08 16:16:55, robertphillips wrote:
> Everyone else has params?

Done.

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrDrawTarget.h#newcod...
src/gpu/GrDrawTarget.h:1049: int vertexCount) = 0;
On 2012/06/08 16:16:55, robertphillips wrote:
> Well ... except for this guy.

Done.

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrGpu.h#newcode350
src/gpu/GrGpu.h:350: protected:
On 2012/06/08 16:16:55, robertphillips wrote:
> Move this comment back to setupClipAndFlushState?

Done.

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrGpu.h#newcode462
src/gpu/GrGpu.h:462: uint32_t numVertices) = 0;
On 2012/06/08 16:16:55, robertphillips wrote:
> param names?

Done.

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrPath.h
File src/gpu/GrPath.h (right):

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrPath.h#newcode30
src/gpu/GrPath.h:30: /*
On 2012/06/08 16:16:55, robertphillips wrote:
> Why two?

Woah, I think I applied my own patch to itself.

http://codereview.appspot.com/6302049/diff/1023/src/gpu/gl/GrGLPath.h
File src/gpu/gl/GrGLPath.h (right):

http://codereview.appspot.com/6302049/diff/1023/src/gpu/gl/GrGLPath.h#newcode13
src/gpu/gl/GrGLPath.h:13: #include "gl/GrGLInterface.h"
On 2012/06/08 16:16:55, robertphillips wrote:
> Do we need the interface header here?

We need GrGLFunctions.h for GrGLuint. I changed it to include that header
directly.

http://codereview.appspot.com/6302049/diff/1023/src/gpu/gl/GrGLPath.h#newcode14
src/gpu/gl/GrGLPath.h:14: #include "SkPath.h"
On 2012/06/08 16:16:55, robertphillips wrote:
> Can't we just forward declare SkPath here?

Done.

http://codereview.appspot.com/6302049/diff/1023/src/gpu/gl/GrGLPath.h#newcode29
src/gpu/gl/GrGLPath.h:29: size_t sizeInBytes() const { return 100; }
On 2012/06/08 16:16:55, robertphillips wrote:
> 100?

I added TODO a comment. I need to figure out what should be returned here. (also
made it virtual/override in the updated patch).

[TomH, 2012-06-08 21:38:25]

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrDrawTarget.h#newcode24
src/gpu/GrDrawTarget.h:24: class GrTexture;
Nit: We both #include the header and forward declare the class - are both
necessary? We don't seem to refer to GrTexture anywhere else in the file - is
either necessary?

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrGpu.cpp#newcode375
src/gpu/GrGpu.cpp:375: // Must flush the scissor after graphics state
Nit: comment unclear

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrGpu.h#newcode356
src/gpu/GrGpu.h:356: kIsPathStencil_FlushFlag  = 0x8,
1. not yet used
2. you've set these up as flags, but are returning them like enums - how's that
going to work?

http://codereview.appspot.com/6302049/diff/8001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

http://codereview.appspot.com/6302049/diff/8001/src/gpu/GrGpu.h#newcode555
src/gpu/GrGpu.h:555: virtual void onStencilPath(const GrPath&, GrPathFill)
SK_OVERRIDE;
You added param names above but forgot them here?

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGLPath.cpp
File src/gpu/gl/GrGLPath.cpp (right):

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGLPath.cpp#newco...
src/gpu/gl/GrGLPath.cpp:12: #define GPUGL static_cast<GrGpuGL*>(getGpu())
this->getGpu()?

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGLPath.cpp#newco...
src/gpu/gl/GrGLPath.cpp:60: GR_STATIC_ASSERT(SK_SCALAR_IS_FLOAT); // assuming
SkPoint is floats
Hmm, does Ganesh not get built at all when we have a Fixed config?

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGLPath.cpp#newco...
src/gpu/gl/GrGLPath.cpp:78: GrAssert(numPts = pathPoints.count());
==, not = ??

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGLPath.cpp#newco...
src/gpu/gl/GrGLPath.cpp:82: 2 * numPts, GR_GL_FLOAT, &pathPoints[0]));
numPts is DEBUGCODE - do you mean pointCnt?

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGpuGL.cpp#newcod...
src/gpu/gl/GrGpuGL.cpp:1334: return path;
Fallback implemented at higher level? Maybe I missed something...

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGpuGL.cpp#newcod...
src/gpu/gl/GrGpuGL.cpp:1731: GrCrash("Should not get here.");
"Not implemented yet; should not get here." ?? Or will this always be true?

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGpuGL.h
File src/gpu/gl/GrGpuGL.h (right):

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGpuGL.h#newcode70
src/gpu/gl/GrGpuGL.h:70: virtual GrPath* onCreatePath(const SkPath&)
SK_OVERRIDE;
Robert wants you to name parameters.

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGpuGL.h#newcode110
src/gpu/gl/GrGpuGL.h:110: virtual void onGpuStencilPath(const GrPath&,
GrPathFill) SK_OVERRIDE;
Again.

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGpuGL.h#newcode117
src/gpu/gl/GrGpuGL.h:117: virtual bool flushGraphicsState(FlushFlags)
SK_OVERRIDE;
And again.

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGpuGL.h#newcode140
src/gpu/gl/GrGpuGL.h:140: void flushBlend(bool         isLines,
Nit: we don't usually inflict this kind of indentation on parameter lists...

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGpuGL_program.cpp
File src/gpu/gl/GrGpuGL_program.cpp (right):

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGpuGL_program.cp...
src/gpu/gl/GrGpuGL_program.cpp:381:
this->flushAAState(SkToBool(kIsLines_FlushFlag & flags));
This expression appears 3 times; I can't quite decide whether or not we should
store it in a local variable.

[bsalomon, 2012-06-08 22:04:54]

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrDrawTarget.h#newcode24
src/gpu/GrDrawTarget.h:24: class GrTexture;
On 2012/06/08 21:38:25, TomH wrote:
> Nit: We both #include the header and forward declare the class - are both
> necessary? We don't seem to refer to GrTexture anywhere else in the file - is
> either necessary?

Neither is necessary. It turns out we were missing GrTexture.h in GrDrawState.h
where we do use it. This was masking that problem.

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrGpu.cpp
File src/gpu/GrGpu.cpp (right):

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrGpu.cpp#newcode375
src/gpu/GrGpu.cpp:375: // Must flush the scissor after graphics state
On 2012/06/08 21:38:25, TomH wrote:
> Nit: comment unclear

I don't know that it is true anymore or why it was once true. I think it is
orthogonal to this change.

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

http://codereview.appspot.com/6302049/diff/1023/src/gpu/GrGpu.h#newcode356
src/gpu/GrGpu.h:356: kIsPathStencil_FlushFlag  = 0x8,
On 2012/06/08 21:38:25, TomH wrote:
> 1. not yet used
> 2. you've set these up as flags, but are returning them like enums - how's
that
> going to work?

I agree it is a little silly. I was thinking there could be other flags
orthogonal to the the type of primitive. I can't think of a great name for it as
a type though. The first three are primitive types, the last isn't really. 
Maybe DrawType (?):

DrawPoints_DrawType
DrawLines_DrawType
DrawTriangles_DrawType
StencilPath_DrawType
FillPath_DrawType // future

http://codereview.appspot.com/6302049/diff/8001/src/gpu/GrGpu.h
File src/gpu/GrGpu.h (right):

http://codereview.appspot.com/6302049/diff/8001/src/gpu/GrGpu.h#newcode555
src/gpu/GrGpu.h:555: virtual void onStencilPath(const GrPath&, GrPathFill)
SK_OVERRIDE;
On 2012/06/08 21:38:25, TomH wrote:
> You added param names above but forgot them here?

Done. You guys and your param names. Personally, I'd prefer to leave them out
when the names just repeat the type name since it is more typing reading and
adds no value. I just consulted with Mike and he says they are optional, so
there!

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGLPath.cpp
File src/gpu/gl/GrGLPath.cpp (right):

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGLPath.cpp#newco...
src/gpu/gl/GrGLPath.cpp:12: #define GPUGL static_cast<GrGpuGL*>(getGpu())
On 2012/06/08 21:38:25, TomH wrote:
> this->getGpu()?

Done.

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGLPath.cpp#newco...
src/gpu/gl/GrGLPath.cpp:78: GrAssert(numPts = pathPoints.count());
On 2012/06/08 21:38:25, TomH wrote:
> ==, not = ??

AHhhh, yes. Done and yoda-ized.

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGLPath.cpp#newco...
src/gpu/gl/GrGLPath.cpp:82: 2 * numPts, GR_GL_FLOAT, &pathPoints[0]));
On 2012/06/08 21:38:25, TomH wrote:
> numPts is DEBUGCODE - do you mean pointCnt?

Done.

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGpuGL.cpp#newcod...
src/gpu/gl/GrGpuGL.cpp:1334: return path;
On 2012/06/08 21:38:25, TomH wrote:
> Fallback implemented at higher level? Maybe I missed something...

I'll turn the check into an assert, we shouldn't get here if the caps isn't set.

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGpuGL.cpp#newcod...
src/gpu/gl/GrGpuGL.cpp:1731: GrCrash("Should not get here.");
On 2012/06/08 21:38:25, TomH wrote:
> "Not implemented yet; should not get here." ?? Or will this always be true?

The former, done.

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGpuGL.h
File src/gpu/gl/GrGpuGL.h (right):

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGpuGL.h#newcode70
src/gpu/gl/GrGpuGL.h:70: virtual GrPath* onCreatePath(const SkPath&)
SK_OVERRIDE;
On 2012/06/08 21:38:25, TomH wrote:
> Robert wants you to name parameters.

That does it. I refuse.

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGpuGL.h#newcode110
src/gpu/gl/GrGpuGL.h:110: virtual void onGpuStencilPath(const GrPath&,
GrPathFill) SK_OVERRIDE;
On 2012/06/08 21:38:25, TomH wrote:
> Again.

Not Done.

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGpuGL.h#newcode117
src/gpu/gl/GrGpuGL.h:117: virtual bool flushGraphicsState(FlushFlags)
SK_OVERRIDE;
On 2012/06/08 21:38:25, TomH wrote:
> And again.

Not Done.

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGpuGL.h#newcode140
src/gpu/gl/GrGpuGL.h:140: void flushBlend(bool         isLines,
On 2012/06/08 21:38:25, TomH wrote:
> Nit: we don't usually inflict this kind of indentation on parameter lists...

Done.

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGpuGL_program.cpp
File src/gpu/gl/GrGpuGL_program.cpp (right):

http://codereview.appspot.com/6302049/diff/8001/src/gpu/gl/GrGpuGL_program.cp...
src/gpu/gl/GrGpuGL_program.cpp:381:
this->flushAAState(SkToBool(kIsLines_FlushFlag & flags));
On 2012/06/08 21:38:25, TomH wrote:
> This expression appears 3 times; I can't quite decide whether or not we should
> store it in a local variable.

It appears twice, one is IsPoints.

I'll try resubmitting with this as a real enum instead of a flags bitfield.

[TomH, 2012-06-09 13:02:06]

Lgtmish

[bsalomon, 2012-06-11 12:34:16]

Tom, Can you take one last look? I changed the FlushFlags bitfield into a
DrawType enum.

[TomH, 2012-06-11 14:05:15]

Delta 5->6 LGTM. Although if it's an enum, you can go back to 1,2,3,4 instead of
1,2,4,8?

[bsalomon, 2012-06-11 14:10:33]

On 2012/06/11 14:05:15, TomH wrote:
> Delta 5->6 LGTM. Although if it's an enum, you can go back to 1,2,3,4 instead
of
> 1,2,4,8?

True, will do before checkin