Bug 84197 - [Chromium] Solid color layers should respect opacity value.
Summary: [Chromium] Solid color layers should respect opacity value.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Reveman
URL:
Keywords:
Depends on: 84215
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-17 14:56 PDT by David Reveman
Modified: 2012-04-18 16:33 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.29 KB, patch)
2012-04-17 14:58 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (4.30 KB, patch)
2012-04-17 16:08 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (4.23 KB, patch)
2012-04-18 15:30 PDT, David Reveman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Reveman 2012-04-17 14:56:18 PDT
Fold opacity into shader color for solid color layers.
Comment 1 David Reveman 2012-04-17 14:58:11 PDT
Created attachment 137611 [details]
Patch
Comment 2 Antoine Labour 2012-04-17 15:05:51 PDT
I would do the folding into LayerRendererChromium::drawSolidColorQuad instead. It's intrinsically linked with the shader - e.g. if we were to change the shader to explicitly take the opacity as a param, that's where we'd do it.
Comment 3 James Robinson 2012-04-17 15:19:33 PDT
Comment on attachment 137611 [details]
Patch

I agree with Antoine.
Comment 4 David Reveman 2012-04-17 16:07:14 PDT
(In reply to comment #2)
> I would do the folding into LayerRendererChromium::drawSolidColorQuad instead. It's intrinsically linked with the shader - e.g. if we were to change the shader to explicitly take the opacity as a param, that's where we'd do it.

Good point. That also gets rid of the ugly rounding code I have in the current patch.
Comment 5 David Reveman 2012-04-17 16:08:07 PDT
Created attachment 137627 [details]
Patch
Comment 6 Antoine Labour 2012-04-17 16:13:35 PDT
Comment on attachment 137627 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:623
> +    GLC(context(), context()->uniform4f(solidColorProgram->fragmentShader().colorLocation(), color.red() / 255.0, color.green() / 255.0, color.blue() / 255.0, (color.alpha() / 255.0) * opacity));

Don't you need to apply opacity to all components, since we're doing premultiplied alpha? All other shaders multiply all components by the opacity.
Comment 7 David Reveman 2012-04-17 16:57:58 PDT
(In reply to comment #6)
> (From update of attachment 137627 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137627&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:623
> > +    GLC(context(), context()->uniform4f(solidColorProgram->fragmentShader().colorLocation(), color.red() / 255.0, color.green() / 255.0, color.blue() / 255.0, (color.alpha() / 255.0) * opacity));
> 
> Don't you need to apply opacity to all components, since we're doing premultiplied alpha? All other shaders multiply all components by the opacity.

The solid color shader doesn't expect premultiplied alpha. We expect textures to have premultiplied alpha but I'm not sure what the case is for LayerChromium::setBackgroundColor(). If it needs to be fixed, lets do that in a separate patch.
Comment 8 Antoine Labour 2012-04-17 17:04:26 PDT
OK, NVM, the shader already multiplies RGB by A.

So this LGTM.
Comment 9 Adrienne Walker 2012-04-18 11:26:31 PDT
Comment on attachment 137627 [details]
Patch

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

Oops.  Thanks for fixing this.  R=me.

> Source/WebKit/chromium/tests/CCSolidColorLayerImplTest.cpp:104
> +    EXPECT_EQ(opacity, quadCuller.quadList()[0]->toSolidColorDrawQuad()->opacity());

This test would have passed before, but I guess there's no reason not to add it.
Comment 10 David Reveman 2012-04-18 11:33:42 PDT
Will rebase and land this patch after: https://bugs.webkit.org/show_bug.cgi?id=84215
Comment 11 David Reveman 2012-04-18 15:30:54 PDT
Created attachment 137786 [details]
Patch

Rebase
Comment 12 WebKit Review Bot 2012-04-18 16:33:00 PDT
Comment on attachment 137786 [details]
Patch

Clearing flags on attachment: 137786

Committed r114580: <http://trac.webkit.org/changeset/114580>
Comment 13 WebKit Review Bot 2012-04-18 16:33:05 PDT
All reviewed patches have been landed.  Closing bug.