Implement NVPR on GLES

Implement NV_path_rendering on OpenGL ES

Implement support for NV_path_rendering on OpenGL ES. Use
glProgramPathFragmentInputGenNV function call instead of glPathTexGenNV to
communicate transforms to fragment shader.

The intention is that the NVPR paths will be drawn with the same shader program
as non-NVPR geometry. For NVPR calls, the GPU will skip the vertex shader and
just run the fragment shader.

After program is linked, query the locations of the fragment shader inputs with
glGetResourceLocation. The location will be used to set the transforms with
glProgramPathFragmentInputGenNV.

The functions and their workings are documented in:

glProgramPathFragmentInputGenNV
https://www.opengl.org/registry/specs/NV/path_rendering.txt
(note: addition as of API version 1.3)

glGetResourceLocation
https://www.opengl.org/registry/specs/ARB/program_interface_query.txt
http://www.opengl.org/registry/doc/glspec44.core.pdf
(function is in core Open GL 4.4)

Note: glProgramPathFragmentInputGenNV could be used also for OpenGL. However,
using seems to trigger a bug in the driver. Disable this feature on OpenGL at
least until the driver is fixed and released. The bug manifests in shadertext
test, where the lower-left text pair is missing. Valgrind catches a bad read
for the test and causes the context to OOM reproducibly.

Committed: https://skia.googlesource.com/skia/+/ec56e4545477e30d4f165ca55ed99f90525c6c38

[Kimmo Kinnunen, 2014-07-01 13:01:21]

Brian, I'm trying to implement the NVPR on GL ES with the augmented NVPR API we
have bothered you about.

There's a design problem with the code structure, could you perhaps give insight
how it should be solved?

The problem is that currently programs are parametrized with "uniform manager"
instance as the "context" how the parametrization code gets a reference to the
parameters. My feeling is that this doesn't fit that well with parametrizing the
program with things that are not uniforms.

One option is to simply say that the feeling is probably wrong, and just rename
the "uniform manager" to "shader resource manager", and be done with it.

Other option that I can think of is to expose the "link result" structure
instead of "uniform manager" structure to the parametrization. The handles to
various variables (uniforms/fragment program inputs(/texcoords)) would be the
references to "link result"...

The explanation of the problem is not that good, maybe the problem itself isn't
a problem either. Maybe you could just take a look at the code and make a
suggestion?

https://codereview.chromium.org/367643004/diff/1/src/gpu/gl/GrGLProgramEffect...
File src/gpu/gl/GrGLProgramEffects.cpp (right):

https://codereview.chromium.org/367643004/diff/1/src/gpu/gl/GrGLProgramEffect...
src/gpu/gl/GrGLProgramEffects.cpp:357: const GrEffectStage* effectStages[]) {
So here we would need a context which would point to "all things related to the
linked shader program that is currently in use".

Currently this context is the uniformManager, which is a subset or derivation of
"result of shader program link information".

What I'm trying to say is that, to my understanding, this sets the parameters of
a specific shader program. However, the function does not have explicit access
to the shader program, it only has access to the parameter instances. Maybe this
in turn happens to look ok because the global nature of the uniform set
functions. It turns uglier with the equivalent ProgramPathFragmentInputGen,
because this function takes in the explicit program parameter..

https://codereview.chromium.org/367643004/diff/1/src/gpu/gl/GrGLShaderBuilder.h
File src/gpu/gl/GrGLShaderBuilder.h (right):

https://codereview.chromium.org/367643004/diff/1/src/gpu/gl/GrGLShaderBuilder...
src/gpu/gl/GrGLShaderBuilder.h:63: struct GenProgramOutput {
I think this is the correct concept to represent the information gotten out of
the process of linking a shader program..

However, this class is not at the moment part of the bigger program structure..
Or, what I mean is that this is not passed to anything that would want to
configure the program in use. (eg. to use the information that (should be)
stored here)

https://codereview.chromium.org/367643004/diff/1/src/gpu/gl/GrGLUniformManager.h
File src/gpu/gl/GrGLUniformManager.h (right):

https://codereview.chromium.org/367643004/diff/1/src/gpu/gl/GrGLUniformManage...
src/gpu/gl/GrGLUniformManager.h:20: /** Manages a program's uniforms and other
shader resources.
I think this change sums up well the problem of the patch:
What does it mean to "manage" shader resources? 
IMHO this is a bit redundant, and doesn't represent the concepts of the domain,
at least clearly.

https://codereview.chromium.org/367643004/diff/1/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

https://codereview.chromium.org/367643004/diff/1/src/gpu/gl/GrGpuGL.cpp#newco...
src/gpu/gl/GrGpuGL.cpp:2487: GL_CALL(ProgramPathFragmentInputGen(fHWProgramID,
If we consider the role of GrGpuGL to contain functions that draw and functions
that affect the global GL state, then this function is very much in a wrong
place.

However, to me it seems that getting robust access to the program id is not
possible without refactoring

[bsalomon, 2014-07-01 20:04:18]

Kimmo, can you send a link to the spec for glProgramPathFragmentInputGenNV? I
have a vague idea of the problem you're describing but I'm not sure I completely
grok it. In general, I think we can consider the current GrGLUniformManager to
really be something like GrGLProgramDataManager and it can manager thing other
than GL uniforms.

[Kimmo Kinnunen, 2014-07-02 11:09:45]

https://chromiumcodereview.appspot.com/365853002/ is a possible rename

[Kimmo Kinnunen, 2014-08-01 11:25:01]

This is now updated and ready for review.

This depends on following issue:
https://codereview.chromium.org/426553011/

[bsalomon, 2014-08-05 14:10:50]

Hey Kimmo, I'm assuming this needs some updating after Chris's recent change.
Let me know if I'm mistaken.

[Kimmo Kinnunen, 2014-08-07 10:51:19]

On 2014/08/05 14:10:50, bsalomon wrote:
> Hey Kimmo, I'm assuming this needs some updating after Chris's recent change.
> Let me know if I'm mistaken.

Done.

[bsalomon, 2014-08-07 21:02:52]

Adding Chris because some of my comments are really about where the division
between GrGpuGL and GrGLPathRendering lies. I think a lot of the PR code in
GpuGL would be better moved to the GrGLPR, esp since this change is adding more
knowledge to that code about the particulars of the PR extension that is in use.

https://codereview.chromium.org/367643004/diff/140001/src/gpu/gl/GrGLProgram.cpp
File src/gpu/gl/GrGLProgram.cpp (right):

https://codereview.chromium.org/367643004/diff/140001/src/gpu/gl/GrGLProgram....
src/gpu/gl/GrGLProgram.cpp:29: if (!desc.getHeader().fHasVertexCode &&
I think we should probably rename this field from fHasVertexCode to
fRequiresVertexShader or something similar since it can be false but we may
still create a VS for it.

https://codereview.chromium.org/367643004/diff/140001/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

https://codereview.chromium.org/367643004/diff/140001/src/gpu/gl/GrGpuGL.cpp#...
src/gpu/gl/GrGpuGL.cpp:145:
!this->pathRendering()->caps().fragmentInputGenSupport) {
Maybe we should move the hw path tex gen settings stuff into GrGLPathRendering
and move the cap bits that specify particulars of the nvpr extension in play
there as well.. I'm not crazy about having this public fragmentinputGenSupport
on caps and it feels like GrGLPathRendering would better encapsulate path
rendering.

https://codereview.chromium.org/367643004/diff/140001/src/gpu/gl/GrGpuGL.cpp#...
src/gpu/gl/GrGpuGL.cpp:335: // we don't use the model view matrix.
Again, I think we should move this state tracking stuff to the PR class.

https://codereview.chromium.org/367643004/diff/140001/src/gpu/gl/GrGpuGL.cpp#...
src/gpu/gl/GrGpuGL.cpp:1957: SkASSERT(this->caps()->pathRenderingSupport());
Move whole impl to path rendering class?

https://codereview.chromium.org/367643004/diff/140001/src/gpu/gl/GrGpuGL.cpp#...
src/gpu/gl/GrGpuGL.cpp:2396: GrGLenum matrixMode =
Move to PR class?

[Chris Dalton, 2014-08-07 23:02:55]

https://codereview.chromium.org/367643004/diff/140001/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

https://codereview.chromium.org/367643004/diff/140001/src/gpu/gl/GrGpuGL.cpp#...
src/gpu/gl/GrGpuGL.cpp:145:
!this->pathRendering()->caps().fragmentInputGenSupport) {
On 2014/08/07 21:02:52, bsalomon wrote:
> Maybe we should move the hw path tex gen settings stuff into GrGLPathRendering
> and move the cap bits that specify particulars of the nvpr extension in play
> there as well.. I'm not crazy about having this public fragmentinputGenSupport
> on caps and it feels like GrGLPathRendering would better encapsulate path
> rendering.

The original idea for GrGLPathRendering was to mirror the NVPR api exactly,
which does require a check for fragmentinputGenSupport (since it is impossible
to provide a backup implementation).

But we could alternatively invent our own non-nvpr method on GrGLPathRendering
that uses ProgramPathFragmentInputGen and falls back on PathTexGen. It would be
tricky though, since GrGLPathRendering would then have to somehow influence the
shader builders and coord transforms.

[Kimmo Kinnunen, 2014-08-08 12:49:25]

On 2014/08/07 21:02:52, bsalomon wrote:
> Adding Chris because some of my comments are really about where the division
> between GrGpuGL and GrGLPathRendering lies. I think a lot of the PR code in
> GpuGL would be better moved to the GrGLPR, esp since this change is adding
more
> knowledge to that code about the particulars of the PR extension that is in
use.

Done.

Depends:
https://codereview.chromium.org/444223002/ (reland of GrGLPathRendering)
https://codereview.chromium.org/452823002/ (Separate GL path rendering state
from GrGpuGL to GrGLPathRendering)
https://chromiumcodereview.appspot.com/450283002/ (Simplify GrGLPathRendering
interface)

[Kimmo Kinnunen, 2014-08-22 12:55:39]

The depends are now merged. Should you have sudden craving for path rendering,
this is ready for review.

[Kimmo Kinnunen, 2014-08-22 12:55:39]

The depends are now merged. Should you have sudden craving for path rendering,
this is ready for review.

[bsalomon, 2014-08-22 13:31:13]

Hey Kimmo, I think you need to rebase again :( The shader builder code was just
refactored here: 
https://skia.googlesource.com/skia.git/+/30ba436f04e61d4505fb854d5fc56079636e...

https://codereview.chromium.org/367643004/diff/160001/src/gpu/gl/GrGLProgramE...
File src/gpu/gl/GrGLProgramEffects.cpp (right):

https://codereview.chromium.org/367643004/diff/160001/src/gpu/gl/GrGLProgramE...
src/gpu/gl/GrGLProgramEffects.cpp:429: if
(transforms[t].fCurrentValue.cheapEqualTo(transform))
minor nit: we always use {} for if

https://codereview.chromium.org/367643004/diff/160001/src/gpu/gl/GrGLProgramE...
src/gpu/gl/GrGLProgramEffects.cpp:433: case kVec2f_GrSLType: {
optional: {} only required if a var is declared in the case.

https://codereview.chromium.org/367643004/diff/160001/src/gpu/gl/GrGLProgramE...
src/gpu/gl/GrGLProgramEffects.cpp:442: SkFAIL("Unexpected matrixs type.");
typo, s on matrix

[Kimmo Kinnunen, 2014-08-25 12:43:09]

On 2014/08/22 13:31:13, bsalomon wrote:
> Hey Kimmo, I think you need to rebase again :( The shader builder code was
just
> refactored here: 
>
https://skia.googlesource.com/skia.git/+/30ba436f04e61d4505fb854d5fc56079636e...

Done.

[Kimmo Kinnunen, 2014-08-25 12:43:28]

https://codereview.chromium.org/367643004/diff/160001/src/gpu/gl/GrGLProgramE...
File src/gpu/gl/GrGLProgramEffects.cpp (right):

https://codereview.chromium.org/367643004/diff/160001/src/gpu/gl/GrGLProgramE...
src/gpu/gl/GrGLProgramEffects.cpp:429: if
(transforms[t].fCurrentValue.cheapEqualTo(transform))
On 2014/08/22 13:31:13, bsalomon wrote:
> minor nit: we always use {} for if

Done.

https://codereview.chromium.org/367643004/diff/160001/src/gpu/gl/GrGLProgramE...
src/gpu/gl/GrGLProgramEffects.cpp:433: case kVec2f_GrSLType: {
On 2014/08/22 13:31:12, bsalomon wrote:
> optional: {} only required if a var is declared in the case.

Done.

https://codereview.chromium.org/367643004/diff/160001/src/gpu/gl/GrGLProgramE...
src/gpu/gl/GrGLProgramEffects.cpp:442: SkFAIL("Unexpected matrixs type.");
On 2014/08/22 13:31:12, bsalomon wrote:
> typo, s on matrix

Done.

[bsalomon, 2014-08-25 14:49:28]

lgtm, but since Joshua is actively working in this area I'd like him to approve
it, too.

[joshua.litt, 2014-08-25 17:00:05]

Just to clarify, with this enabled, a user created vertex will be ignored
completely?

https://codereview.chromium.org/367643004/diff/180001/src/gpu/gl/builders/GrG...
File src/gpu/gl/builders/GrGLProgramBuilder.h (right):

https://codereview.chromium.org/367643004/diff/180001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLProgramBuilder.h:295: 
Since this only requires a fragment shader, does it make more sense to move this
to the base class?

[Chris Dalton, 2014-08-25 18:04:23]

On 2014/08/25 17:00:05, joshua.litt wrote:
> Just to clarify, with this enabled, a user created vertex will be ignored
> completely?
> 
>
https://codereview.chromium.org/367643004/diff/180001/src/gpu/gl/builders/GrG...
> File src/gpu/gl/builders/GrGLProgramBuilder.h (right):
> 
>
https://codereview.chromium.org/367643004/diff/180001/src/gpu/gl/builders/GrG...
> src/gpu/gl/builders/GrGLProgramBuilder.h:295: 
> Since this only requires a fragment shader, does it make more sense to move
this
> to the base class?

I'll go ahead and answer for Kimmo as he is out for the day.

Yes, NVpr draw calls ignore any user-created vertex shader (but the standard GL
draw calls like glDrawArrays of course still use them). The idea is the
following:

glDrawArrays et al. -- Fragment varying inputs programmed by vertex shader
glCoverFillPathNV et al. (NVpr) -- Fragment varying inputs programmed by
glProgramPathFragmentInputGenNV

Unlike triangle meshes, paths don't have vertices. They are just squiggly lines,
so a vertex shader that crunches on custom per-vertex attributes wouldn't make
sense (i.e. where are your vertices? What do the custom values even mean? etc.
etc. etc.).

[Chris Dalton, 2014-08-25 18:04:50]

https://codereview.chromium.org/367643004/diff/180001/src/gpu/gl/builders/GrG...
File src/gpu/gl/builders/GrGLProgramBuilder.h (right):

https://codereview.chromium.org/367643004/diff/180001/src/gpu/gl/builders/GrG...
src/gpu/gl/builders/GrGLProgramBuilder.h:295: 
On 2014/08/25 17:00:05, joshua.litt wrote:
> Since this only requires a fragment shader, does it make more sense to move
this
> to the base class?

It actually does require a vertex shader. Not for NVpr calls (those use
glProgramPathFragmentInputGenNV instead), but when drawing with standard GL
calls the separable varying value needs to be set by the vertex shader.

The base class is for making inputs that wouldn't require a vertex shader even
with the standard GL calls (i.e. by using the fixed-function texture coordinates
and glTexGen, which is not the case for separable varyings).

[joshua.litt, 2014-08-25 18:36:20]

I see what you are saying.  In the long term this change should hopefully allow
us to migrate to a single program builder anyways.

LGTM

On 2014/08/25 18:04:50, Chris Dalton wrote:
>
https://codereview.chromium.org/367643004/diff/180001/src/gpu/gl/builders/GrG...
> File src/gpu/gl/builders/GrGLProgramBuilder.h (right):
> 
>
https://codereview.chromium.org/367643004/diff/180001/src/gpu/gl/builders/GrG...
> src/gpu/gl/builders/GrGLProgramBuilder.h:295: 
> On 2014/08/25 17:00:05, joshua.litt wrote:
> > Since this only requires a fragment shader, does it make more sense to move
> this
> > to the base class?
> 
> It actually does require a vertex shader. Not for NVpr calls (those use
> glProgramPathFragmentInputGenNV instead), but when drawing with standard GL
> calls the separable varying value needs to be set by the vertex shader.
> 
> The base class is for making inputs that wouldn't require a vertex shader even
> with the standard GL calls (i.e. by using the fixed-function texture
coordinates
> and glTexGen, which is not the case for separable varyings).

[Kimmo Kinnunen, 2014-08-26 05:12:48]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2014-08-26 05:13:54]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/367643004/180001

[commit-bot: I haz the power, 2014-08-26 05:21:23]

Committed patchset #10 (180001) as ec56e4545477e30d4f165ca55ed99f90525c6c38