Bug 73317 – [CSS Shaders] Add FECustomFilter that renders custom filters

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alexandru Chiculita]

Add a FECustomFilter that can render the shader using GPU, but read back the result to be used with other software shaders.

[Alexandru Chiculita]

Created attachment 116960 [details]
Work in progress

Just a dump of the current progress.

[Dean Jackson]

Comment on attachment 116960 [details]
Work in progress

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

Looking good. We're going to be hooking up the hardware compositing for the other effects this week. That might allow you to skip the readback from the GPU, although I'm not sure if you'll get the compositing for free in RenderLayerBacking on ports other than OS X. I haven't looked to see how Chrome and others do this.

> Source/WebCore/platform/graphics/filters/CustomFilterShader.cpp:149
> +    if (m_vertexShader)
> +        m_context->deleteShader(m_vertexShader);
> +    if (m_fragmentShader)
> +        m_context->deleteShader(m_fragmentShader);
> +}

Can't we delete the shaders as soon as we've linked?

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:69
> +    const float farValue = 10000;
> +    const float nearValue = -10000;

Do we describe why we pick these values anywhere?

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:95
> +static void toGLMatrix(float* flattened, const TransformationMatrix& m)
> +{
> +    flattened[0] = m.m11();
> +    flattened[1] = m.m12();
> +    flattened[2] = m.m13();
> +    flattened[3] = m.m14();
> +    flattened[4] = m.m21();
> +    flattened[5] = m.m22();
> +    flattened[6] = m.m23();
> +    flattened[7] = m.m24();
> +    flattened[8] = m.m31();
> +    flattened[9] = m.m32();
> +    flattened[10] = m.m33();
> +    flattened[11] = m.m34();
> +    flattened[12] = m.m41();
> +    flattened[13] = m.m42();
> +    flattened[14] = m.m43();
> +    flattened[15] = m.m44();
> +}

Aside: we really need to improve matrices in WebKit. This type of thing should be supported directly on TransformationMatrix.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:175
> +    unsigned vertexSize = (m_meshType == CustomFilterOperation::ATTACHED ? 8 : 11) * sizeof(float);

I think we need to declare what these magic values mean in CustomFilterOperation and use the consts here.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:196
> +    if (m_meshType == CustomFilterOperation::DETACHED) {
> +        offset += 2 * sizeof(float);

I guess to be consistent the offset line should be outside the if.

[Alexandru Chiculita]

(In reply to comment #2)

Thanks for the review! All the comments make sense, so I will try to fix them tomorrow and also add some tests.

[Alexandru Chiculita]

Created attachment 119321 [details]
Patch that generates results

[Alexandru Chiculita]

Created attachment 119450 [details]
Patch V1

[WebKit Review Bot]

Attachment 119450 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1

Source/WebCore/WebCore.vcproj/WebCore.vcproj:43832:  mismatched tag  [xml/syntax] [5]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexandru Chiculita]

Created attachment 119458 [details]
Patch V2

It looks like main windows build is broken currently (there's a missing </File>) 

I've fixed that in this patch, just to make sure EWS is passing all the platforms.

[Alexandru Chiculita]

The FECustomFilter is only enabled on platforms that have WebGL and will only be created if WebGL is enabled in the Settings.

[WebKit Review Bot]

Comment on attachment 119458 [details]
Patch V2

Attachment 119458 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10904340

[Alexandru Chiculita]

Created attachment 119487 [details]
Fixing build issue

[Chris Marrin]

Comment on attachment 119450 [details]
Patch V1

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

Please make another pass, trying to simplify the mesh manipulation logic

> LayoutTests/css3/filters/resources/fragment.shader:1
> +precision mediump float;

I don't like the .shader suffix much. It would be more descriptive if you used .vs and .fs, which are fairly standard shader suffixes.

> LayoutTests/platform/wk2/Skipped:192
> +css3/filters/custom-filter.html

Please mention why you had to turn off this test in wk2 up in your ChangeLog.

> Source/WebCore/platform/graphics/filters/CustomFilterMesh.cpp:98
> +                vertices.append(hSize * i - 0.5f + meshBoxX);

Factor out common code between this and addPoint()

> Source/WebCore/platform/graphics/filters/CustomFilterMesh.cpp:109
> +        #define ADD_POINT(x, y) \

This is confusingly called ADD_POINT, when it doesn't use addPoint(), and has the same name as the ADD_POINT below, which does. Seems like all this vertex list manipulation logic could be coalesced into a small set of functions.

> Source/WebCore/platform/graphics/filters/CustomFilterMesh.h:51
> +    unsigned verticesBuffer() const { return m_verticesBuffer; }

I can't tell what verticesBuffer is from the name or return type. I know it is a GC3D buffer object, so it's type should be Platform3DObject. And maybe it could be called verticesBufferObject.

> Source/WebCore/platform/graphics/filters/CustomFilterMesh.h:54
> +    unsigned elementsBuffer() const { return m_elementsBuffer; }

Ditto

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1201
> +void TransformationMatrix::toColumnMajorFloatArray(float* result) const

I think we're actually storing the matrix in column major order (if not, we should be). So it seems like this could be a memcpy()

> Source/WebCore/rendering/FilterEffectRenderer.cpp:242
> +#if ENABLE(WEBGL)

Shouldn't this be ENABLE(WEBGL) && ENABLE(CSS_SHADERS). There's no need to have this code compiled in if CSS_SHADERS is off, is there?

[Alexandru Chiculita]

(In reply to comment #11)
> (From update of attachment 119450 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119450&action=review
> 
> Please make another pass, trying to simplify the mesh manipulation logic
Yes. I'm working on that. I will send an updated patch tomorrow.

> 
> > LayoutTests/css3/filters/resources/fragment.shader:1
> > +precision mediump float;
> 
> I don't like the .shader suffix much. It would be more descriptive if you used .vs and .fs, which are fairly standard shader suffixes.
Ok.

> 
> > LayoutTests/platform/wk2/Skipped:192
> > +css3/filters/custom-filter.html
> 
> Please mention why you had to turn off this test in wk2 up in your ChangeLog.
I had a comment in the ChangeLog: 
* platform/wk2/Skipped: WebKit2 has no option to force enable WebGL yet.
Should I add more than that? The tracking bug number for that issue is inside the Skipped file.

> 
> > Source/WebCore/platform/graphics/filters/CustomFilterMesh.cpp:98
> > +                vertices.append(hSize * i - 0.5f + meshBoxX);
> 
> Factor out common code between this and addPoint()
> 
> > Source/WebCore/platform/graphics/filters/CustomFilterMesh.cpp:109
> > +        #define ADD_POINT(x, y) \
> 
> This is confusingly called ADD_POINT, when it doesn't use addPoint(), and has the same name as the ADD_POINT below, which does. Seems like all this vertex list manipulation logic could be coalesced into a small set of functions.
> 
Ok.

> > Source/WebCore/platform/graphics/filters/CustomFilterMesh.h:51
> > +    unsigned verticesBuffer() const { return m_verticesBuffer; }
> 
> I can't tell what verticesBuffer is from the name or return type. I know it is a GC3D buffer object, so it's type should be Platform3DObject. And maybe it could be called verticesBufferObject.
> 
> > Source/WebCore/platform/graphics/filters/CustomFilterMesh.h:54
> > +    unsigned elementsBuffer() const { return m_elementsBuffer; }
> 
Ok.

> Ditto
> 
> > Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1201
> > +void TransformationMatrix::toColumnMajorFloatArray(float* result) const
> 
> I think we're actually storing the matrix in column major order (if not, we should be). So it seems like this could be a memcpy()
Ok. I thought the compiler will optimize that out and the code looked more portable.

> 
> > Source/WebCore/rendering/FilterEffectRenderer.cpp:242
> > +#if ENABLE(WEBGL)
> 
> Shouldn't this be ENABLE(WEBGL) && ENABLE(CSS_SHADERS). There's no need to have this code compiled in if CSS_SHADERS is off, is there?

There's a ENABLE(CSS_SHADER) two lines before that. I needed to have the case, so that all the values in the enum are handled.

[Alexandru Chiculita]

> > > Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:1201
> > > +void TransformationMatrix::toColumnMajorFloatArray(float* result) const
> > 
> > I think we're actually storing the matrix in column major order (if not, we should be). So it seems like this could be a memcpy()
> Ok. I thought the compiler will optimize that out and the code looked more portable.

I take that back. The Matrix4 uses doubles and I actually need floats.

[Alexandru Chiculita]

Created attachment 119668 [details]
Patch V3

Thanks for the review. I've updated the patch.

[WebKit Review Bot]

Attachment 119668 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1

Source/WebCore/platform/graphics/filters/CustomFilterMesh.cpp:95:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexandru Chiculita]

Created attachment 119672 [details]
Patch V4

Fixed style.

[Chris Marrin]

Mac is failing ews. Looks like MeshGenerator::DetachedMeshVertexSize() is missing?

[Alexandru Chiculita]

Created attachment 119729 [details]
Patch V5

Fixing mac-gcc build.

[Alexandru Chiculita]

Created attachment 119732 [details]
Patch V6

Rebased the patch.

[Chris Marrin]

Comment on attachment 119732 [details]
Patch V6

We have a winner!

[WebKit Review Bot]

Comment on attachment 119732 [details]
Patch V6

Clearing flags on attachment: 119732

Committed r103170: <http://trac.webkit.org/changeset/103170>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nikolas Zimmermann]

Comment on attachment 119732 [details]
Patch V6

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

Great that you've worked on this. Though, I'm really sad the patch went in as-is.
It does need better ChangeLogs, and commenting on what has changed. A patch, as big as this, with such an impact, needs this.

As this is already in trunk, would you want to create a follow-up patch, that fixes the issues I mention. Or shall we go as far, and roll this out? I would have given r- to the patch as-is.

> LayoutTests/ChangeLog:10
> +

I would have expected a much more detailed ChangeLog, for new functionality like that!
It's not obvious to anyone what's the difference between fragment/vertex shader .fs and .vs files, and what this feature is about.

> LayoutTests/css3/filters/effect-custom-expected.txt:1
> +    

This looks awkward. You should at least include some text in the test file explaining why that the text result doesn't matter and only the pixel test.

> LayoutTests/css3/filters/resources/color-offset.fs:6
> +    gl_FragColor = texture2D(s_texture, v_texCoord) + 0.3;

This should be commented IMHO, to make it easier to understand what's happening here.

> LayoutTests/css3/filters/resources/vertex-explode-detached.vs:12
> +    gl_Position = u_projectionMatrix * (a_position + vec4(a_triangleCoord.x / 4.0, a_triangleCoord.y / 4.0, 0.0, 0.0));

Ditto.

> LayoutTests/css3/filters/resources/vertex-offset.vs:11
> +    gl_Position = u_projectionMatrix * (a_position + vec4(0.1, 0.0, 0.0, 0.0));

Ditto.

> Source/WebCore/ChangeLog:7
> +        Using a GraphicsContext3D to render the shaders in GPU, read the
> +        result back and use it in the software filters pipeline.

This ChangeLog contains waaaaay too few information. Sad, that this got in as-is.

> Source/WebCore/ChangeLog:19
> +        * loader/cache/CachedShader.cpp:

Inline comments for all following files would have helped. We usually use extensive ChangeLogs for new features like this.

> Source/WebCore/loader/cache/CachedShader.cpp:52
> +    if (!m_shaderString && m_data) {

if (!m_shaderString)? What's operator bool supposed to return for a String?
I'd rather use m_shaderString.isEmpty() here.

> Source/WebCore/loader/cache/CachedShader.cpp:54
> +        m_shaderString = m_decoder->decode(m_data->data(), m_data->size());
> +        m_shaderString += m_decoder->flush();

Ouch, operator+= should be avoided. Please use StringBuilder here.

> Source/WebCore/loader/cache/CachedShader.cpp:63
> +    if (allDataReceived)
> +        m_data = data;

There's no need to enter CachedResource::data() if allDataReceived is true. It would only early exit then.

> Source/WebCore/platform/graphics/filters/CustomFilterMesh.cpp:33
> +

Newline unncessary.

> Source/WebCore/platform/graphics/filters/CustomFilterMesh.cpp:35
> +

Ditto.

> Source/WebCore/platform/graphics/filters/CustomFilterMesh.cpp:38
> +#define DEBUG_CUSTOM_FILTER_MESH 0

This is ugly, you should rather always compile your debug function #ifndef NDEBUG, to avoid bit-rot. Your approach is likely to break.

> Source/WebCore/platform/graphics/filters/CustomFilterMesh.cpp:47
> +        , m_points(cols + 2, rows + 2)
> +        , m_tiles(cols + 1, rows + 1)

This needs to be commented, so the "magic" numbers are understandable, w/o having to inspect all off the code.

> Source/WebCore/platform/graphics/filters/CustomFilterMesh.cpp:52
> +        m_vertices.reserveCapacity(vertexCount() * floatsPerVertex());

It's not obvious why vertexCount() * floatsPerVertex() is needed here...

> Source/WebCore/platform/graphics/filters/CustomFilterMesh.cpp:53
> +        m_indices.reserveCapacity(indicesCount());

... but not for m_indices.

Whenever you read code, that's non-obvious like this, a comment is needed.

> Source/WebCore/platform/graphics/filters/CustomFilterMesh.cpp:120
> +        for (int j = 0; j < m_points.height(); ++j) 

We usually use an enclosing brace for the outer loop in cases like this.

> Source/WebCore/platform/graphics/filters/CustomFilterMesh.cpp:124
> +        for (int j = 0; j < m_tiles.height(); ++j)

Ditto.

> Source/WebCore/platform/graphics/filters/CustomFilterMesh.cpp:137
> +        for (int j = 0; j < m_tiles.height(); ++j)

Ditto.

> Source/WebCore/platform/graphics/filters/CustomFilterMesh.h:47
> +                                               CustomFilterOperation::MeshType meshType) {

Hm, this look awkward, you could have kept one long line.

> Source/WebCore/platform/graphics/filters/CustomFilterShader.cpp:35
> +
> +#define SHADER0(Src) #Src
> +#define SHADER(Src) SHADER0(Src)

Hm, what's that? :-) This should really be reworked, and or commented.

> Source/WebCore/platform/graphics/filters/CustomFilterShader.cpp:43
> +String CustomFilterShader::defaultVertexShaderString()

How about statically allocating the string inside CustomFilterShader?

> Source/WebCore/platform/graphics/filters/CustomFilterShader.cpp:61
> +    return SHADER(

Ditto.

> Source/WebCore/platform/graphics/filters/CustomFilterShader.cpp:93
> +    Platform3DObject vertexShader = m_context->createShader(GraphicsContext3D::VERTEX_SHADER);
> +    Platform3DObject fragmentShader = m_context->createShader(GraphicsContext3D::FRAGMENT_SHADER);
> +
> +    m_context->shaderSource(vertexShader, m_vertexShaderString);

Endless constructor following, I'd prefer this to be splitted up in more inline helper functions. Anything that splits up the beast is better.

> Source/WebCore/platform/graphics/filters/CustomFilterShader.cpp:151
> +

Newline too much.

> Source/WebCore/platform/graphics/filters/CustomFilterShader.cpp:156
> +
> +

Ditto.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:54
> +static TransformationMatrix orthoMatrix(float left, float right, float bottom, float top)

Abbrevations should be avoided, this needs a new name.
Copying TrafoMatrix by value is a bad idea! Pass in TrafoMatrix& by reference, to avoid the copies!

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:69
> +    const float farValue = 10000;
> +    const float nearValue = -10000;

I don't like it, but well..

[Alexandru Chiculita]

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

Thanks for the review. I've posted a new follow-up bug to fix the issues
https://bugs.webkit.org/show_bug.cgi?id=74840

[Alexandru Chiculita]

*** Bug 71405 has been marked as a duplicate of this bug. ***

[Tim Nguyen (:ntim)]

rdar://10386016