Bug 54484 – [chromium] Clean up shader code from LayerChromium classes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Adrienne Walker]

[chromium] Clean up shader code from LayerChromium classes

[Adrienne Walker]

There's a lot of copy and paste shader code among all the Layer Chromium classes.  Additionally, a lot of the SharedValue classes are more or less identical.  It'd be nice to be able to share more shader and binding code.

[Adrienne Walker]

Created attachment 82500 [details]
Patch

[Adrienne Walker]

If anybody has time, I'd love an initial review to catch issues with the general approach, naming, or style.

Marked as no review because this patch needs more cross-platform testing to make sure that I didn't break anything when moving things around.

[Adrienne Walker]

Created attachment 82503 [details]
Patch

[Adrienne Walker]

(In reply to comment #4)
> Created an attachment (id=82503) [details]
> Patch

Remembered to add new files into the patch.  Oops.

[James Robinson]

Comment on attachment 82503 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/ProgramBinding.h:60
> +    bool init(const char* vertexShader, const char* fragmentShader);

please use String for strings (GraphicsContext3D takes Strings, so we have to pay the conversion cost anyway).

> Source/WebCore/platform/graphics/chromium/ProgramBinding.h:71
> +template<class VertexShader, class FragmentShader>

why templates instead of inheritance here? inheritance is more common in WebKit and I don't think the runtime overhead is significant here.

> Source/WebCore/platform/graphics/chromium/ShaderChromium.cpp:36
> +#define SHADER0(Src) #Src
> +#define SHADER(Src) SHADER0(Src)

this is cute but IMHO makes things a lot harder to read as well as putting everything on one line in the final shader. I'd prefer making the string literals be explicit in the code.

> Source/WebCore/platform/graphics/chromium/ShaderChromium.h:40
> +    const char* getShaderString() const;

String here, please (and in the rest of the patch)

[Adrienne Walker]

(In reply to comment #6)

> > Source/WebCore/platform/graphics/chromium/ProgramBinding.h:71
> > +template<class VertexShader, class FragmentShader>
> 
> why templates instead of inheritance here? inheritance is more common in WebKit and I don't think the runtime overhead is significant here.

What sort of inheritance are you suggesting?

I could make all VertexShader classes derive from some common VertexShader base class, but at each call site you'd have to dynamically downcast to the derived class to get to all of the fooLocation() members.  Or, you'd have to push those members all up to a bloated base class, which is also not great.

I could also make ProgramBinding have multiple inheritance from two concrete parent shader classes, which I don't like to do.  And, even if I did that, I would probably also use a template so that I didn't have to declare every vertex/fragment shader as a separate class.

This approach seemed like the one with the smallest code smell to me.

> > Source/WebCore/platform/graphics/chromium/ShaderChromium.cpp:36
> > +#define SHADER0(Src) #Src
> > +#define SHADER(Src) SHADER0(Src)
> 
> this is cute but IMHO makes things a lot harder to read as well as putting everything on one line in the final shader. I'd prefer making the string literals be explicit in the code.

Hmm.  I found it much easier to read than having these giant blocks of literals.  The other advantage is that you get some syntax highlighting from your editor, rather than having everything just be a giant string.

Vangelis was planning on changing the shaders to use this format, so I figured I'd just go ahead and do it in this patch while I was touching everything.

> Use String.

Will do.

[James Robinson]

(In reply to comment #7)
> (In reply to comment #6)
> 
> > > Source/WebCore/platform/graphics/chromium/ProgramBinding.h:71
> > > +template<class VertexShader, class FragmentShader>
> > 
> > why templates instead of inheritance here? inheritance is more common in WebKit and I don't think the runtime overhead is significant here.
> 
> What sort of inheritance are you suggesting?
> 
> I could make all VertexShader classes derive from some common VertexShader base class, but at each call site you'd have to dynamically downcast to the derived class to get to all of the fooLocation() members.  Or, you'd have to push those members all up to a bloated base class, which is also not great.
> 

Ahh, I see what it's doing.  That's quite nice, actually :)

> > > Source/WebCore/platform/graphics/chromium/ShaderChromium.cpp:36
> > > +#define SHADER0(Src) #Src
> > > +#define SHADER(Src) SHADER0(Src)
> > 
> > this is cute but IMHO makes things a lot harder to read as well as putting everything on one line in the final shader. I'd prefer making the string literals be explicit in the code.
> 
> Hmm.  I found it much easier to read than having these giant blocks of literals.  The other advantage is that you get some syntax highlighting from your editor, rather than having everything just be a giant string.

Yeah, but it's syntax highlighting for the wrong language.
> 
> Vangelis was planning on changing the shaders to use this format, so I figured I'd just go ahead and do it in this patch while I was touching everything.
>

[Nat Duca]

+1 for Enne's approach. There's clear win here when editing shader code.

> Yeah, but it's syntax highlighting for the wrong language.
> > 
> > Vangelis was planning on changing the shaders to use this format, so I figured I'd just go ahead and do it in this patch while I was touching everything.
> >

[Vangelis Kokkevis]

Comment on attachment 82503 [details]
Patch

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

really like the cleanup!

> Source/WebCore/platform/graphics/chromium/ProgramBinding.cpp:44
> +GeometryBinding::GeometryBinding(GraphicsContext3D* context)

Shouldn't GeometryBinding be in its own file (.cpp, .h) just like every other non-trivial class in WebKit?

> Source/WebCore/platform/graphics/chromium/ProgramBinding.cpp:86
> +static unsigned loadShader(GraphicsContext3D* context, unsigned type, const char* shaderSource)

May be cleaner to make these functions members of ProgramBindingBase and remove the context parameter passed to them.

[Adrienne Walker]

Re: shader formatting.  Here are the options, as I see them, with some long-winded commentary.

1) Stringified, with vertically aligned quotes:

"void main()                  \n"
"{                            \n"
"  gl_Position = matrix * a_position; \n"
"  v_texCoord = a_texCoord;   \n"
"}                            \n";

Maybe it's a personal pet peeve, but I hate vertically aligning source code.  If I add a long line, do I have to modify every other line in that block to have the correct amount of spaces? If I do that, then source control merge and blame operations become more tricky.  If I don't modify the line (as in the above and in many other examples), then it's not consistent, but then why do it in the first place?

2) Stringified, no vertical alignment

"void main()\n"
"{\n"
"  gl_Position = matrix * a_position;\n"
"  v_texCoord = a_texCoord;\n"
"}\n";

If you're not vertically aligning the trailing \n", then maybe we could just ditch all the whitespace on the right.  This approach, like the above, still has the issue that you can't use your editor to indent the shader code like you'd indent the surrounding C++ code.  Granted, our compositor shaders are short and are unlikely to have conditional branches, but it's still a mark against editability for me.

Mostly, I find this really hard to read because the characters are too dense.  It looks more like a regular expression than source code.

3) Using #define

return SHADER(
    void main()
    {
        gl_Position = matrix * a_position;
        v_texCoord = a_texCoord;
    }
);

I really do find this the clearest to read, but that may be a personal preference.  The argument that syntax highlighting is for the wrong language is very true, but it's not so different as to be entirely unusable.  It's an improvement over string literals.

There's also the argument that this puts everything on one line in the shader, but that doesn't actually add any value.  At least in my tests, when a compositor shader failed to compile, you just get a runtime error message printed from the createShader function that the shader failed and no line numbers.  We could probably make this better, but given the small number of compositor shaders, it's likely not worth the time.

4) Separate GLSL files

return
    #include "vertexshadertexpos.glsl"
    ;

This is another option.  The original source shader files could have correct syntax highlighting applied to them because they are pure GLSL, but then we have a whole bunch of stray shader files to handle.  You'd also need a compiler step to stringify these files because you can't do an #include in the middle of the SHADER macro expansion.  Although, this if it's part of the build we could be extra fancy and have a shader compilation step as a part of the build to let grossly incorrect shaders fail at compile-time instead of at run-time.  Sadly, all of this is a lot of extra work.

[Kenneth Russell]

I think this should wait for 54559 to land, as that one might need to be merged to M10 and after this restructuring lands it will be impossible to backport compositor fixes automatically.

[Adrienne Walker]

(In reply to comment #12)
> I think this should wait for 54559 to land, as that one might need to be merged to M10 and after this restructuring lands it will be impossible to backport compositor fixes automatically.

Thanks for pointing that out.  I don't want to make it tough to merge for anybody else.

Also, re: shader macro.  I talked to jamesr in person and reached an agreement that the SHADER() macro is the least worst option to include shader code in the compositor.  So, I'll keep that in for the final patch.

[James Robinson]

(In reply to comment #12)
> I think this should wait for 54559 to land, as that one might need to be merged to M10 and after this restructuring lands it will be impossible to backport compositor fixes automatically.

54559 has landed now: http://trac.webkit.org/changeset/78787

is this patch ready to go?

[Adrienne Walker]

(In reply to comment #14)
> (In reply to comment #12)
> > I think this should wait for 54559 to land, as that one might need to be merged to M10 and after this restructuring lands it will be impossible to backport compositor fixes automatically.
> 
> 54559 has landed now: http://trac.webkit.org/changeset/78787
> 
> is this patch ready to go?

I'm working on rebaselining it against vrk's changes and making sure I didn't break anything.  Most likely tomorrow?

[James Robinson]

Cool beans :)

[Adrienne Walker]

Created attachment 83020 [details]
Patch

[Adrienne Walker]

(In reply to comment #17)
> Created an attachment (id=83020) [details]
> Patch

My only reservation here is that I couldn't figure out how to trigger either the PluginLayerChromium drawing or the RGBA path for VideoLayerChromium.  (Are we even using the RGBA path anymore now that we have the YUV path?)

I ran all the media/ and compositing/ tests on Windows though, so I feel confident that I'm not breaking any existing tests.

Also, jamesr tells me I need to rebaseline this patch.

[Adrienne Walker]

Created attachment 83026 [details]
Patch

[Adrienne Walker]

Committed r79043: <http://trac.webkit.org/changeset/79043>