Add cache for GPU-accelerated path processing results

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Kenneth Russell]

A class is needed to hold the results of processing a path into a triangle mesh suitable for rendering on the GPU. The data structures exposed by this cache need to be low level to be compatible with the GraphicsContext3D rendering API, which matches OpenGL.

[Kenneth Russell]

Created attachment 67156 [details]
Patch

From the ChangeLog:

Adding a cache which holds the results of processing a path into interior and exterior triangle meshes, according to the path rendering algorithm from GPU Gems 3. No tests yet; will be tested in conjunction with later code.

[James Robinson]

Comment on attachment 67156 [details]
Patch

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

> WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:43
> +class LoopBlinnPathCache : public Noncopyable {
> +public:
Please declare a destructor here and give it an (empty) definition in the .cpp.  Otherwise the body of the destructor for vector will get inlined in to every file that includes this header.

> WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:61
> +    // Get the base pointer to the vertex information. There are two
> +    // coordinates per vertex. This pointer is valid until the cache is
> +    // cleared or another vertex is added. Returns 0 if there are no
> +    // vertices in the mesh.
> +    const float* vertices() const
> +    {
> +        if (!numberOfVertices())
> +            return 0;
> +        return m_vertices.data();
> +    }
> +
> +    // Get the base pointer to the texture coordinate information. There
> +    // are three coordinates per vertex. This pointer is valid until the
> +    // cache is cleared or another vertex is added. Returns 0 if
> +    // there are no vertices in the mesh.
> +    const float* texcoords() const
Instead of exposing raw pointers to this class' underlying vector types, this class should expose getters for the length and data at a given offset.  That way users of this class are insulated from the details of the class' guts.  That would also avoid oddness around how to do the correct pointer math for vertices vs texcoords and allow us to change out the underlying implementation if we wanted to.

> WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:80
> +    // Base pointer to the interior vertices; two coordinates per
> +    // vertex, which can be drawn as GL_TRIANGLES. Returns 0 if there
> +    // are no interior vertices in the mesh.
> +    const float* interiorVertices() const
Same here. A better interface would be something like "int interiorVertexCount(); FloatPoint interiorVertex(int i);"

> WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:87
> +    void addInteriorVertex(float x, float y);
Why not use FloatPoint here (and all the other locations that use pairs or triples of floats)?

[Kenneth Russell]

Created attachment 67470 [details]
Revised patch

Addressed above review feedback.

[Kenneth Russell]

(In reply to comment #2)
> (From update of attachment 67156 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67156&action=prettypatch
> 
> > WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:43
> > +class LoopBlinnPathCache : public Noncopyable {
> > +public:
> Please declare a destructor here and give it an (empty) definition in the .cpp.  Otherwise the body of the destructor for vector will get inlined in to every file that includes this header.

Done.

> > WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:61
> > +    // Get the base pointer to the vertex information. There are two
> > +    // coordinates per vertex. This pointer is valid until the cache is
> > +    // cleared or another vertex is added. Returns 0 if there are no
> > +    // vertices in the mesh.
> > +    const float* vertices() const
> > +    {
> > +        if (!numberOfVertices())
> > +            return 0;
> > +        return m_vertices.data();
> > +    }
> > +
> > +    // Get the base pointer to the texture coordinate information. There
> > +    // are three coordinates per vertex. This pointer is valid until the
> > +    // cache is cleared or another vertex is added. Returns 0 if
> > +    // there are no vertices in the mesh.
> > +    const float* texcoords() const
> Instead of exposing raw pointers to this class' underlying vector types, this class should expose getters for the length and data at a given offset.  That way users of this class are insulated from the details of the class' guts.  That would also avoid oddness around how to do the correct pointer math for vertices vs texcoords and allow us to change out the underlying implementation if we wanted to.

This comment doesn't make sense in the context of how the path cache is used. The entire point is to be able to pass the resulting vertices and texture coordinates directly to OpenGL (via GraphicsContext3D). The using code looks approximately like:

GraphicsContext3D* context = ...;
LoopBlinnPathCache pathCache = ...;
// LoopBlinnPathProcessor.process(path, pathCache);
context.bindBuffer(GraphicsContext3D::ARRAY_BUFFER, vertexBuffer);
context.bufferData(GraphicsContext3D::ARRAY_BUFFER, 2 * pathCache.numberOfVertices() * sizeof(float), pathCache.vertices(), GraphicsContext3D::STATIC_DRAW);

The intent is to avoid a data copy when retrieving the vertices and texture coordinates from the path cache; GC3D's bufferData and bufferSubData already copy the incoming data.

> > WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:80
> > +    // Base pointer to the interior vertices; two coordinates per
> > +    // vertex, which can be drawn as GL_TRIANGLES. Returns 0 if there
> > +    // are no interior vertices in the mesh.
> > +    const float* interiorVertices() const
> Same here. A better interface would be something like "int interiorVertexCount(); FloatPoint interiorVertex(int i);"

No, for the reason described above.

> > WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:87
> > +    void addInteriorVertex(float x, float y);
> Why not use FloatPoint here (and all the other locations that use pairs or triples of floats)?

For the reason described above. The data must be returned as a series of floats, not FloatPoint or FloatPoint3D, so it can be uploaded to OpenGL directly.

[James Robinson]

Comment on attachment 67470 [details]
Revised patch

r- for the error handling.  Otherwise, this look all right

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

> WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:52
> +    // cleared or another vertex is added. Returns 0 if there are no
> +    // vertices in the mesh.
> +    const float* vertices() const
Wouldn't calling this function if there are no vertices be an error from the caller? This should be an ASSERT() then, not an if check.

Also, why doesn't this return a const Vector<float>*?  Then the caller could grab the .data() if they want and deal with the empty case in whatever way they please.

> WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:63
> +    // cache is cleared or another vertex is added. Returns 0 if
> +    // there are no vertices in the mesh.
> +    const float* texcoords() const
Same here.

> WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:82
> +    // vertex, which can be drawn as GL_TRIANGLES. Returns 0 if there
> +    // are no interior vertices in the mesh.
> +    const float* interiorVertices() const
Same here

> WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:96
> +    unsigned numberOfInteriorEdgeVertices() const;
Why isn't this inline like the rest?

> WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:100
> +    // no interior edge vertices in the mesh.
> +    const float* interiorEdgeVertices() const;
Same comments as above (don't return NULL, consider exposing a const Vector<float>&).

[Kenneth Russell]

(In reply to comment #5)
> (From update of attachment 67470 [details])
> r- for the error handling.  Otherwise, this look all right
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=67470&action=prettypatch
> 
> > WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:52
> > +    // cleared or another vertex is added. Returns 0 if there are no
> > +    // vertices in the mesh.
> > +    const float* vertices() const
> Wouldn't calling this function if there are no vertices be an error from the caller? This should be an ASSERT() then, not an if check.

It isn't an error. The calling code uses OpenGL functions and semantics, where passing 0 down for the source pointer is legal when 0 vertices are being transferred. Allowing this function to return 0 simplifies the calling code significantly, eliminating the need for several if-tests.

> Also, why doesn't this return a const Vector<float>*?  Then the caller could grab the .data() if they want and deal with the empty case in whatever way they please.

That would be an abstraction violation. The point of this class is to provide a very narrow interface between the code which generates the mesh and the code which consumes it.

> > WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:63
> > +    // cache is cleared or another vertex is added. Returns 0 if
> > +    // there are no vertices in the mesh.
> > +    const float* texcoords() const
> Same here.
> 
> > WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:82
> > +    // vertex, which can be drawn as GL_TRIANGLES. Returns 0 if there
> > +    // are no interior vertices in the mesh.
> > +    const float* interiorVertices() const
> Same here
> 
> > WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:96
> > +    unsigned numberOfInteriorEdgeVertices() const;
> Why isn't this inline like the rest?

This is code for debugging only, so performance is not an issue, and it's cleaner to keep it all together in the .cpp.

> > WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:100
> > +    // no interior edge vertices in the mesh.
> > +    const float* interiorEdgeVertices() const;
> Same comments as above (don't return NULL, consider exposing a const Vector<float>&).

I'll upload a new version of the patch shortly which won't change anything semantically but will do things like update the copyright notice.

[Kenneth Russell]

Created attachment 81725 [details]
Patch

[James Robinson]

Comment on attachment 81725 [details]
Patch

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

R=me

> Source/WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:77
> +    void addVertex(float x, float y,
> +                   float /*k*/, float /*l*/, float /*m*/);

nit: "float k, float l, float m" should be fine here

[Kenneth Russell]

(In reply to comment #8)
> (From update of attachment 81725 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81725&action=review
> 
> R=me

Thanks.

> > Source/WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:77
> > +    void addVertex(float x, float y,
> > +                   float /*k*/, float /*l*/, float /*m*/);
> 
> nit: "float k, float l, float m" should be fine here

The style bot didn't like "float l".

[Kenneth Russell]

Committed r78002: <http://trac.webkit.org/changeset/78002>

[James Robinson]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 81725 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=81725&action=review
> > 
> > R=me
> 
> Thanks.
> 
> > > Source/WebCore/platform/graphics/gpu/LoopBlinnPathCache.h:77
> > > +    void addVertex(float x, float y,
> > > +                   float /*k*/, float /*l*/, float /*m*/);
> > 
> > nit: "float k, float l, float m" should be fine here
> 
> The style bot didn't like "float l".

Come to think of it I added that check :P  we had some code which was in fact confusing a local variable '1' with the literal value '1' which led to a crash.  In the code review font the two glyphs are identical.