Bug 45520 – Add shaders for GPU accelerated path rendering

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Kenneth Russell]

The shaders needed to render paths on the GPU need to be generated on the fly and cached, because there are several variables affecting their behavior and not all permutations are needed. Classes are needed to manage the shaders and cache them.

[Kenneth Russell]

Created attachment 67157 [details]
Patch

From the ChangeLog:

Adding generator and cache of the shaders for the GPU accelerated path rendering algorithm from GPU Gems 3. No tests yet; will be tested in conjunction with later code.

[Eric Seidel (no email)]

Comment on attachment 67157 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67157&action=prettypatch

A bunch of nits.  I'm not much of a graphics dude though, so I don't have much of substance to say.

> WebCore/platform/graphics/gpu/LoopBlinnShader.cpp:43
> +long LoopBlinnShader::getUniformLocation(const String& name)
What is a long?  I think we normally use int or unsigned instead in WebCore code.

> WebCore/platform/graphics/gpu/LoopBlinnShader.cpp:48
> +    long location = m_context->getUniformLocation(m_program, name);
We don't use "get" in names in WebCore code.  I guess we do use them for active lookups (like hash lookukps) sometimes though.  Never for accessors.  So I guess this is OK.  We should really clarify our stance in the style guide. :)

> WebCore/platform/graphics/gpu/LoopBlinnShader.cpp:56
> +    if (iter != m_attribLocations.end())
Can this just be if (iter)?

> WebCore/platform/graphics/gpu/LoopBlinnShader.h:29
> +#include "GraphicsContext3D.h"
Not needed, right?  Or is it needed for Platform3DObject?  Should Platform3DObject be in its own header?

> WebCore/platform/graphics/gpu/LoopBlinnShader.h:30
> +#include "PlatformString.h"
Unlikely this is needed, StringHash includes it if nothing else does.

> WebCore/platform/graphics/gpu/LoopBlinnShader.h:44
> +    LoopBlinnShader(GraphicsContext3D* context, Platform3DObject program);
"context" is not needed here.  We only name arguments when they add meaning to the declaration.

> WebCore/platform/graphics/gpu/LoopBlinnShader.h:46
> +    void use();
What does "use" mean here?

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:28
> +#include "config.h"
> +
> +#include "LoopBlinnShaderCache.h"
I don't think folks normally put a new line between config.h and the first header.  Not a big deal.

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:31
> +#include "PlatformString.h"
This was included in the header, no?

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:41
> +    m_shaders.resize(NumShaderTypes * NumRegionTypes * NumAntialiasTypes);
Thanks for using constants!  I'm not sure what the correct style is. I don't think they're capitalized though since that would be class names.

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:67
> +    int index = (shaderIndex * numRegions * numAntialiases +
Um, really?  That's one crazy datastructure!

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:70
> +    if (!m_shaders[index].get())
.get() is not needed here. There is a bool conversion for you.

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:75
> +PassOwnPtr<LoopBlinnShader> LoopBlinnShaderCache::createShader(ShaderType shaderType, RegionType regionType, AntialiasType antialiasType)
This is a really long method... can we make it shorter? Split it up into pieces?

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:154
> +    m_context->getProgramiv(program, GraphicsContext3D::LINK_STATUS, &linkStatus);
what's "Programiv"?  A graphics term?

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:162
> +    m_context->deleteShader(vertexShader);
Should we use some sort of scoping object for this attach/delete pattern?

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:173
> +    m_context->getShaderiv(shader, GraphicsContext3D::COMPILE_STATUS, &compileStatus);
Shaderiv?  Another graphics term?

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.h:65
> +    LoopBlinnShader* getShader(ShaderType shaderType, RegionType regionType, AntialiasType antialiasType);
I wonder if lookupShader would be better than getShader().  Maybe I'm just sensitive to "get" after all these years in WebCore...

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.h:68
> +    PassOwnPtr<LoopBlinnShader> createShader(ShaderType shaderType, RegionType regionType, AntialiasType antialiasType);
None of these names are needed, the types say it all.

[Kenneth Russell]

Created attachment 67222 [details]
Revised patch

Addressed code review feedback above.

[Kenneth Russell]

(In reply to comment #2)
> (From update of attachment 67157 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67157&action=prettypatch
> 
> A bunch of nits.  I'm not much of a graphics dude though, so I don't have much of substance to say.
> 
> > WebCore/platform/graphics/gpu/LoopBlinnShader.cpp:43
> > +long LoopBlinnShader::getUniformLocation(const String& name)
> What is a long?  I think we normally use int or unsigned instead in WebCore code.

This is an artifact of bogus uses of the type "long" in GraphicsContext3D and WebGLRenderingContext dating back to their initial integration. I've just filed https://bugs.webkit.org/show_bug.cgi?id=45557 to fix these up. However, I'd like to not block the integration of this code on that bug.

> > WebCore/platform/graphics/gpu/LoopBlinnShader.cpp:48
> > +    long location = m_context->getUniformLocation(m_program, name);
> We don't use "get" in names in WebCore code.  I guess we do use them for active lookups (like hash lookukps) sometimes though.  Never for accessors.  So I guess this is OK.  We should really clarify our stance in the style guide. :)

This name, like all of the other names in GraphicsContext3D, maps exactly to the OpenGL API with the "gl" prefix removed -- in this case, glGetUniformLocation. There is no reason to use another name.

> > WebCore/platform/graphics/gpu/LoopBlinnShader.cpp:56
> > +    if (iter != m_attribLocations.end())
> Can this just be if (iter)?

No. The semantics of find() are that it returns end() if not found. end() is not null.

> > WebCore/platform/graphics/gpu/LoopBlinnShader.h:29
> > +#include "GraphicsContext3D.h"
> Not needed, right?  Or is it needed for Platform3DObject?  Should Platform3DObject be in its own header?

It was for Platform3DObject. I've changed that to "unsigned" and filed https://bugs.webkit.org/show_bug.cgi?id=45558 to remove Platform3DObject completely, because it's pretty bogus.

> > WebCore/platform/graphics/gpu/LoopBlinnShader.h:30
> > +#include "PlatformString.h"
> Unlikely this is needed, StringHash includes it if nothing else does.

OK, removed.

> > WebCore/platform/graphics/gpu/LoopBlinnShader.h:44
> > +    LoopBlinnShader(GraphicsContext3D* context, Platform3DObject program);
> "context" is not needed here.  We only name arguments when they add meaning to the declaration.

Removed.

> > WebCore/platform/graphics/gpu/LoopBlinnShader.h:46
> > +    void use();
> What does "use" mean here?

It's OpenGL terminology; "glUseProgram". Note that the same name is already used in a couple of other shader-related classes in this directory.

> > WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:28
> > +#include "config.h"
> > +
> > +#include "LoopBlinnShaderCache.h"
> I don't think folks normally put a new line between config.h and the first header.  Not a big deal.

I've been using that style throughout the new code I've been adding. Left unchanged.

> > WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:31
> > +#include "PlatformString.h"
> This was included in the header, no?

Was unnecessary; removed.

> > WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:41
> > +    m_shaders.resize(NumShaderTypes * NumRegionTypes * NumAntialiasTypes);
> Thanks for using constants!  I'm not sure what the correct style is. I don't think they're capitalized though since that would be class names.

This is the correct style.

> > WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:67
> > +    int index = (shaderIndex * numRegions * numAntialiases +
> Um, really?  That's one crazy datastructure!

It's really not that complicated. It simply maintains slots for all permutations of these variables, although not all slots will be filled.

> > WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:70
> > +    if (!m_shaders[index].get())
> .get() is not needed here. There is a bool conversion for you.

OK, removed.

> > WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:75
> > +PassOwnPtr<LoopBlinnShader> LoopBlinnShaderCache::createShader(ShaderType shaderType, RegionType regionType, AntialiasType antialiasType)
> This is a really long method... can we make it shorter? Split it up into pieces?

I thought about this but all it's doing is assembling a vertex and a fragment shader into a program. I don't think that splitting it up will add anything at this point. If it gets more complicated in the future, I will.

> > WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:154
> > +    m_context->getProgramiv(program, GraphicsContext3D::LINK_STATUS, &linkStatus);
> what's "Programiv"?  A graphics term?

OpenGL terminology -- integer pointer.

> > WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:162
> > +    m_context->deleteShader(vertexShader);
> Should we use some sort of scoping object for this attach/delete pattern?

No. This is basically OpenGL code. It is definitely not worth introducing additional complexity for this roughly five lines of actual graphics code in this method.

> > WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:173
> > +    m_context->getShaderiv(shader, GraphicsContext3D::COMPILE_STATUS, &compileStatus);
> Shaderiv?  Another graphics term?

OpenGL terminology. Get integer from shader, passing integer pointer for result.

> > WebCore/platform/graphics/gpu/LoopBlinnShaderCache.h:65
> > +    LoopBlinnShader* getShader(ShaderType shaderType, RegionType regionType, AntialiasType antialiasType);
> I wonder if lookupShader would be better than getShader().  Maybe I'm just sensitive to "get" after all these years in WebCore...

OK, I renamed it to lookupShader.

> > WebCore/platform/graphics/gpu/LoopBlinnShaderCache.h:68
> > +    PassOwnPtr<LoopBlinnShader> createShader(ShaderType shaderType, RegionType regionType, AntialiasType antialiasType);
> None of these names are needed, the types say it all.

Removed.

[James Robinson]

Comment on attachment 67222 [details]
Revised patch

Some minor comments.  I'd really like this type to be merged with the existing Shader base class if possible to in order to share code.

View in context: https://bugs.webkit.org/attachment.cgi?id=67222&action=prettypatch

> WebCore/platform/graphics/gpu/LoopBlinnShader.h:45
> +    LoopBlinnShader(GraphicsContext3D*, unsigned program);
How is this type different from a http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/gpu/Shader.h?
please declare d'tor and define in cpp

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:40
> +unsigned createShaderFromSource(GraphicsContext3D* context, GraphicsContext3D::WebGLEnumType shaderType, const String& source)
We have almost this exact same function in http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/gpu/Shader.cpp#L56.  Is there any way to unify these?

The use of a WebGL type here is unfortunate - should this just be renamed in GraphicsContext3D?

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:94
> +    int index = (shaderIndex * numRegions * numAntialiases +
> +                 regionIndex * numAntialiases +
> +                 antialiasIndex);
> +    if (!m_shaders[index])
> +        m_shaders[index] = createShader(shaderType, regionType, antialiasType);
it seems like m_shaders should be an N-dimensional array instead of a vector

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.h:44
> +    LoopBlinnShaderCache(GraphicsContext3D*);
mark the constructor explicit

please declare a destructor and define in the cpp.  Vector's destructor is large and will be inlined into the header otherwise

[Kenneth Russell]

Created attachment 67468 [details]
Revised patch

Addressed above code review feedback.

[Kenneth Russell]

(In reply to comment #5)
> (From update of attachment 67222 [details])
> Some minor comments.  I'd really like this type to be merged with the existing Shader base class if possible to in order to share code.

Agreed, and there is a FIXME in LoopBlinnShader.h to this effect, but the merge will be intricate and I want to checkpoint the code before doing it to avoid regressions.

> View in context: https://bugs.webkit.org/attachment.cgi?id=67222&action=prettypatch
> 
> > WebCore/platform/graphics/gpu/LoopBlinnShader.h:45
> > +    LoopBlinnShader(GraphicsContext3D*, unsigned program);
> How is this type different from a http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/gpu/Shader.h?
> please declare d'tor and define in cpp

They will be merged after the initial checkpoint. Destructor declared and added to .cpp.

> > WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:40
> > +unsigned createShaderFromSource(GraphicsContext3D* context, GraphicsContext3D::WebGLEnumType shaderType, const String& source)
> We have almost this exact same function in http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/gpu/Shader.cpp#L56.  Is there any way to unify these?

I will do this after the code has been checkpointed.

> The use of a WebGL type here is unfortunate - should this just be renamed in GraphicsContext3D?

Yes, filed as https://bugs.webkit.org/show_bug.cgi?id=45708 .

> > WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:94
> > +    int index = (shaderIndex * numRegions * numAntialiases +
> > +                 regionIndex * numAntialiases +
> > +                 antialiasIndex);
> > +    if (!m_shaders[index])
> > +        m_shaders[index] = createShader(shaderType, regionType, antialiasType);
> it seems like m_shaders should be an N-dimensional array instead of a vector

Yes, this does make the code a lot simpler. Changed.

> > WebCore/platform/graphics/gpu/LoopBlinnShaderCache.h:44
> > +    LoopBlinnShaderCache(GraphicsContext3D*);
> mark the constructor explicit

Done.

> please declare a destructor and define in the cpp.  Vector's destructor is large and will be inlined into the header otherwise

Done.

[James Robinson]

Comment on attachment 67468 [details]
Revised patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67468&action=prettypatch

> WebCore/platform/graphics/gpu/LoopBlinnShader.h:43
> +// FIXME: this class needs to be merged with the shader classes for
> +// the fills for rectangles, etc.
> +class LoopBlinnShader : public Noncopyable {
I would strongly prefer this be merged _before_ landing.  There's no reason to check in code that we know needs to be changed.

It looks like the {SolidFill, Interior} shader is exactly the same as our existing SolidFillShader.  Removing that permutation would make the rest of this code a lot simpler.

> WebCore/platform/graphics/gpu/LoopBlinnShader.h:50
> +    long getUniformLocation(const String& name);
> +    long getAttribLocation(const String& name);
This is a different model from the other shader types.  Why do callers need to grab arbitrary uniform/attribute locations?  This looks fragile since knowing that something is a LoopBlinnShader is not enough to know what uniforms and attributes are available.


r- for the redundancy.  I think merging with the existing Shader class should be very straightforward - the only real change is that Shader::loadShader() / loadProgram() should be changed to take strings as Strings rather than const char*, but that is pretty simple and definitely an improvement anyway.

[Kenneth Russell]

(In reply to comment #8)
> r- for the redundancy.  I think merging with the existing Shader class should be very straightforward - the only real change is that Shader::loadShader() / loadProgram() should be changed to take strings as Strings rather than const char*, but that is pretty simple and definitely an improvement anyway.

We've discussed offline the fact that these shaders optionally require an additional vertex attribute stream, which is a fair bit different than how the existing shaders work. I don't want to risk regressing the current functionality or my shaders by doing the merge before an initial checkpoint. I'm marking this patch r? again.

[Chris Marrin]

Comment on attachment 67468 [details]
Revised patch

But I believe the original point is still valid. Checking in a fix with the intention of "fixing it" in the misty future is bad practice. If the current functionality is deficient and needs to be updated to accommodate your new functionality, you should file a bug for that work, block this one on that, fix that bug and then land this patch.

[Kenneth Russell]

Changing the synopsis to reflect the code restructuring based on review feedback.

[Kenneth Russell]

Created attachment 81335 [details]
Patch

[Kenneth Russell]

The code has been completely restructured to address the previous review feedback. Please re-review.

[James Robinson]

Comment on attachment 81335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81335&action=review

R=me.  Looks great, can't want to see this in action.

> Source/WebCore/platform/graphics/gpu/Shader.cpp:203
> +                "#extension GL_OES_standard_derivatives : enable\n");

Is some higher level code responsible for making sure the appropriate extension is enabled when passing Antialiased down?

[Kenneth Russell]

(In reply to comment #14)
> (From update of attachment 81335 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81335&action=review
> 
> R=me.  Looks great, can't want to see this in action.
> 
> > Source/WebCore/platform/graphics/gpu/Shader.cpp:203
> > +                "#extension GL_OES_standard_derivatives : enable\n");
> 
> Is some higher level code responsible for making sure the appropriate extension is enabled when passing Antialiased down?

Yes. A forthcoming patch to SharedGraphicsContext3D will query for, conditionally enable, and base the Antialiased flag on the availability of this extension.

[Kenneth Russell]

webkit-patch threw an exception while updating this bug.

Committed r77865: <http://trac.webkit.org/changeset/77865>

[WebKit Review Bot]

http://trac.webkit.org/changeset/77865 might have broken Leopard Intel Release (Tests)

[WebKit Review Bot]

http://trac.webkit.org/changeset/77865 might have broken Leopard Intel Release (Tests)