[webkit-reviews] review granted: [Bug 93869] [CSS Shaders] Implement normal blend mode and source-atop compositing mode : [Attachment 160277] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Max Vujovic <mvujovic at adobe.com>'s
request for review:
Bug 93869: [CSS Shaders] Implement normal blend mode and source-atop
compositing mode
https://bugs.webkit.org/show_bug.cgi?id=93869

Attachment 160277: Patch
https://bugs.webkit.org/attachment.cgi?id=160277&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=160277&action=review


> LayoutTests/ChangeLog:10
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Add a test for normal blend mode mode and source-atop compositing
mode using css_MixColor in
> +	   the shader.
> +	   Add a test using css_ColorMatrix in the shader.

I think we should add a single line here explaining that we've added these two
new attribs/uniforms to the shaders. The description "normal blend mode and
source-atop compositing" is probably not enough to someone just looking at this
changelog (and not following the bug link).

> LayoutTests/css3/filters/custom/custom-filter-composite-source-atop.html:35
> +	   /* The fragment shader writes a blue square to css_MixColor in the
lower right corner of the destination div. */
> +	   /* It writes transparent elsewhere. */
> +	   -webkit-filter: custom(url('../resources/pass-tex-coord.vs')
mix(url('../resources/composite.fs') normal source-atop));
> +    }
> +    /* Draws a yellow square in the upper left corner of the destination
div. */
> +    /* The rest of the destination div is transparent. */

This is a cool test!

> Source/WebCore/ChangeLog:10
> +	   Instead of allowing direct texture access in an author's shader via
u_texture, CSS
> +	   Shaders blends special symbols in the author's shader (css_MixColor
and
> +	   css_ColorMatrix) with the DOM element texture.

It's a shame that this makes testing outside the Web more complicated, but
that's a spec issue rather than this patch.

> Source/WebCore/ChangeLog:21
> +	   This patch introduces a new class, CustomFilterValidatedProgram. 
> +	   CustomFilterValidatedProgram validates the shader using ANGLE. If
the shader uses

" ... a new class, CustomFilterValidatedProgram, which validates the shader..."


> Source/WebCore/ChangeLog:31
> +	   After validation, CustomFilterValidatedProgram adds blending,
compositing, and
> +	   texture access shader code to the author's original shaders. The
definitions for
> +	   css_MixColor and css_ColorMatrix are added before the author's
fragment shader
> +	   code so that the author code can access them. The blending,
compositing, and

I assume it is still ok if the author includes them in their shader (as long as
they are defined the same way)? I'm thinking of a non-Web test system for just
the standalone shaders.

> Source/WebCore/ChangeLog:41
> +

This is a great ChangeLog. Thankyou!

> Source/WebCore/WebCore.gyp/WebCore.gyp:-1618
> -	   ['exclude', 'platform/graphics/ANGLEWebKitBridge\\.(cpp|h)$'],

I assume this won't cause any issues in Chromium?

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.h:34
> -#elif !PLATFORM(GTK) && !PLATFORM(EFL) && !PLATFORM(BLACKBERRY)
> +#elif !PLATFORM(GTK) && !PLATFORM(EFL) && !PLATFORM(BLACKBERRY) &&
!PLATFORM(CHROMIUM)

This probably answers my question above :)

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.h:56
> -    bool validateShaderSource(const char* shaderSource, ANGLEShaderType
shaderType, String& translatedShaderSource, String& shaderValidationLog, int
extraCompileOptions);
> +    bool validateShaderSource(const char* shaderSource, ANGLEShaderType,
String& translatedShaderSource, String& shaderValidationLog, int
extraCompileOptions = 0);

Thanks. I probably should have done this in my original patch.

> Source/WebCore/platform/graphics/filters/CustomFilterCompiledProgram.cpp:132
> +    m_internalTexAttribLocation = m_context->getAttribLocation(m_program,
"css_a_texCoord");

m_internalTexCoordAttribLocation?

> Source/WebCore/platform/graphics/filters/CustomFilterCompiledProgram.h:63
> +    int internalTexAttribLocation() const { return
m_internalTexAttribLocation; }

Similarly, internalTexCoordAttribLocation() ?

> Source/WebCore/platform/graphics/filters/CustomFilterGlobalContext.cpp:62
> +ANGLEWebKitBridge* CustomFilterGlobalContext::webglShaderValidator()
> +{
> +    if (!m_webglShaderValidator)
> +	   m_webglShaderValidator = createShaderValidator(SH_WEBGL_SPEC);
> +    return m_webglShaderValidator.get();
> +}
> +
> +ANGLEWebKitBridge* CustomFilterGlobalContext::mixShaderValidator()
> +{
> +    if (!m_mixShaderValidator)
> +	   m_mixShaderValidator = createShaderValidator(SH_CSS_SHADERS_SPEC);
> +    return m_mixShaderValidator.get();
> +}

Is there any reason why we wouldn't always want to use the SH_CSS_SHADERS_SPEC
validator, even if the shader doesn't use mix or the generated
attribs/uniforms?

> Source/WebCore/platform/graphics/filters/CustomFilterGlobalContext.h:62
> +    // CSS shaders referenced from the CSS mix function should be validated
slightly differently than WebGL shaders.
> +    // This ANGLE validator uses the SH_CSS_SHADERS_SPEC flag.
> +    // Under this flag, most notably:
> +    // - The "gl_FragColor" built-in is not available.
> +    // - Instead, the "css_MixColor" and "css_ColorMatrix" built-ins are
available.

Not really an issue for this patch, but this answers my question above about
always using the CSS shader validation. It would be really nice if gl_FragColor
was auto-swappable with css_MixColor. Oh well.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.cpp:91
> +    if (!vertexShaderValid || !fragmentShaderValid)
> +	   // FIXME: Report the validation errors.
> +	   // https://bugs.webkit.org/show_bug.cgi?id=74416
> +	   return;

Please add {} since you have multiline comments.

> Source/WebCore/platform/graphics/filters/CustomFilterValidatedProgram.h:77
> +    // 'detachGlobalContext' is called when the CustomFilterGlobalContext is
deleted
> +    // and there's no need for the callback anymore. 

You mean 'detachFromGlobalContext'