Bug 49780 – [chromium] Implement alpha masking for composited layers

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

[Vangelis Kokkevis]

Add support for -webkit-mask properties in the accelerated compositing in chromium's accelerated compositing path.

[Vangelis Kokkevis]

Created attachment 74346 [details]
proposed patch

[Kenneth Russell]

Comment on attachment 74346 [details]
proposed patch

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

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:853
> +        "  vec4 maskColor = texture2D(s_maskTexture, v_texCoord); \n"
> +        "  gl_FragColor = vec4(texColor.x, texColor.y, texColor.z, texColor.w) * alpha * maskColor.w; \n"

Are you sure it's a good idea to impose the extra texture fetch and multiply in all cases? Masking doesn't seem to be a very common operation (the compositor seems to have mostly been working fine up to this point) and I'm concerned about making the default shader more expensive since it touches every pixel in the browser window. It seems to me it would be better to have two shaders, one for when masking is enabled and one for when it isn't. Would this impose too much code complexity? It seems it would be pretty self-contained in LayerRendererChromium::drawLayer.

> LayoutTests/platform/chromium-gpu/test_expectations.txt:56
> -BUGNONE WIN LINUX : fast/canvas = PASS
> -BUGNONE WIN LINUX : canvas/philip = PASS
> +//BUGNONE WIN LINUX : fast/canvas = PASS
> +//BUGNONE WIN LINUX : canvas/philip = PASS

Why are these being commented out?

[Vangelis Kokkevis]

Created attachment 74429 [details]
Patch addressing review comments

[Vangelis Kokkevis]

(In reply to comment #2)
> (From update of attachment 74346 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74346&action=review
> 
> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:853
> > +        "  vec4 maskColor = texture2D(s_maskTexture, v_texCoord); \n"
> > +        "  gl_FragColor = vec4(texColor.x, texColor.y, texColor.z, texColor.w) * alpha * maskColor.w; \n"
> 
> Are you sure it's a good idea to impose the extra texture fetch and multiply in all cases? Masking doesn't seem to be a very common operation (the compositor seems to have mostly been working fine up to this point) and I'm concerned about making the default shader more expensive since it touches every pixel in the browser window. It seems to me it would be better to have two shaders, one for when masking is enabled and one for when it isn't. Would this impose too much code complexity? It seems it would be pretty self-contained in LayerRendererChromium::drawLayer.

Good point.  I created a different shader to be used by RenderSurfaces that do masking.

> 
> > LayoutTests/platform/chromium-gpu/test_expectations.txt:56
> > -BUGNONE WIN LINUX : fast/canvas = PASS
> > -BUGNONE WIN LINUX : canvas/philip = PASS
> > +//BUGNONE WIN LINUX : fast/canvas = PASS
> > +//BUGNONE WIN LINUX : canvas/philip = PASS
> 
> Why are these being commented out?

Ooops... They shouldn't be.  I reverted the file.

[Vangelis Kokkevis]

Created attachment 74435 [details]
Removed ANGLE workaround which is no longer necessary

[Kenneth Russell]

Comment on attachment 74435 [details]
Removed ANGLE workaround which is no longer necessary

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

Looks good. r- because of the needed revert to ChangeLog. Also please consider something different than the ASSERT_NOT_REACHED + working code in LayerChromium::bindContentsTexture().

> WebCore/platform/graphics/chromium/LayerChromium.cpp:523
> +    // We don't really expect to call this method for the base layer type.
> +    ASSERT_NOT_REACHED();

That's pretty gross.

> LayoutTests/ChangeLog:8
> [phone]  Vangelis Kokkevis  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Need a short description and bug URL (OOPS!)
> +
> +        * platform/chromium-gpu/test_expectations.txt:
> +

This file needs to be reverted.

[Vangelis Kokkevis]

Created attachment 74609 [details]
Proposed patch - Removed unecessary changelog and ASSERT

[Kenneth Russell]

Comment on attachment 74609 [details]
Proposed patch - Removed unecessary changelog and ASSERT

Looks good.

[WebKit Commit Bot]

Comment on attachment 74609 [details]
Proposed patch - Removed unecessary changelog and ASSERT

Rejecting attachment 74609 [details] from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--non-interactive', 74609]" exit_code: 2
Last 500 characters of output:
s FAILED -- saving rejects to file WebCore/platform/graphics/chromium/LayerRendererChromium.cpp.rej
patching file WebCore/platform/graphics/chromium/LayerRendererChromium.h
Hunk #1 succeeded at 143 with fuzz 2 (offset -4 lines).
patching file WebCore/platform/graphics/chromium/RenderSurfaceChromium.h
Hunk #1 succeeded at 82 with fuzz 2 (offset 27 lines).

Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Kenneth Russell', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7000112

[Vangelis Kokkevis]

Created attachment 79808 [details]
Patch

[Vangelis Kokkevis]

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

Had to rework the previous patch to account for code changes that happened since the original upload, including the introduction of the TextureManager and moving much of the RenderSurface drawing logic into the RenderSurface class.

Can you please have another quick look? 
Thanks!

[WebKit Review Bot]

Attachment 79808 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7503288

[Vangelis Kokkevis]

Created attachment 79815 [details]
Patch

[Vangelis Kokkevis]

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

Fixed Linux compilation error

[Kenneth Russell]

Comment on attachment 79815 [details]
Patch

Looks fine. (I wish we could see the differences between patches with the review tool...)

[Vangelis Kokkevis]

Committed r76475: <http://trac.webkit.org/changeset/76475>