Bug 49780

Summary: [chromium] Implement alpha masking for composited layers
Product: WebKit Reporter: Vangelis Kokkevis <vangelis>
Component: WebCore Misc.Assignee: Vangelis Kokkevis <vangelis>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dglazkov, kbr, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
proposed patch
none
Patch addressing review comments
none
Removed ANGLE workaround which is no longer necessary
kbr: review-
Proposed patch - Removed unecessary changelog and ASSERT
none
Patch
none
Patch kbr: review+, vangelis: commit-queue-

Description Vangelis Kokkevis 2010-11-18 19:36:26 PST
Add support for -webkit-mask properties in the accelerated compositing in chromium's accelerated compositing path.
Comment 1 Vangelis Kokkevis 2010-11-18 19:41:38 PST
Created attachment 74346 [details]
proposed patch
Comment 2 Kenneth Russell 2010-11-19 12:08:58 PST
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?
Comment 3 Vangelis Kokkevis 2010-11-19 15:15:24 PST
Created attachment 74429 [details]
Patch addressing review comments
Comment 4 Vangelis Kokkevis 2010-11-19 15:16:39 PST
(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.
Comment 5 Vangelis Kokkevis 2010-11-19 15:42:00 PST
Created attachment 74435 [details]
Removed ANGLE workaround which is no longer necessary
Comment 6 Kenneth Russell 2010-11-22 15:52:17 PST
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
> +2010-11-18  Vangelis Kokkevis  <vangelis@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Need a short description and bug URL (OOPS!)
> +
> +        * platform/chromium-gpu/test_expectations.txt:
> +

This file needs to be reverted.
Comment 7 Vangelis Kokkevis 2010-11-22 16:02:34 PST
Created attachment 74609 [details]
Proposed patch - Removed unecessary changelog and ASSERT
Comment 8 Kenneth Russell 2010-11-22 16:32:09 PST
Comment on attachment 74609 [details]
Proposed patch - Removed unecessary changelog and ASSERT

Looks good.
Comment 9 WebKit Commit Bot 2010-12-14 03:31:16 PST
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
Comment 10 Vangelis Kokkevis 2011-01-21 16:28:32 PST
Created attachment 79808 [details]
Patch
Comment 11 Vangelis Kokkevis 2011-01-21 16:30:31 PST
(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!
Comment 12 WebKit Review Bot 2011-01-21 16:33:08 PST
Attachment 79808 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7503288
Comment 13 Vangelis Kokkevis 2011-01-21 17:54:33 PST
Created attachment 79815 [details]
Patch
Comment 14 Vangelis Kokkevis 2011-01-21 17:55:11 PST
(In reply to comment #13)
> Created an attachment (id=79815) [details]
> Patch

Fixed Linux compilation error
Comment 15 Kenneth Russell 2011-01-21 18:11:04 PST
Comment on attachment 79815 [details]
Patch

Looks fine. (I wish we could see the differences between patches with the review tool...)
Comment 16 Vangelis Kokkevis 2011-01-23 17:54:14 PST
Committed r76475: <http://trac.webkit.org/changeset/76475>