Bug 84197 – [Chromium] Solid color layers should respect opacity value.

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

[David Reveman]

Fold opacity into shader color for solid color layers.

[David Reveman]

Created attachment 137611 [details]
Patch

[Antoine Labour]

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.

[James Robinson]

Comment on attachment 137611 [details]
Patch

I agree with Antoine.

[David Reveman]

(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.

[David Reveman]

Created attachment 137627 [details]
Patch

[Antoine Labour]

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.

[David Reveman]

(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.

[Antoine Labour]

OK, NVM, the shader already multiplies RGB by A.

So this LGTM.

[Adrienne Walker]

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.

[David Reveman]

Will rebase and land this patch after: https://bugs.webkit.org/show_bug.cgi?id=84215

[David Reveman]

Created attachment 137786 [details]
Patch

Rebase

[WebKit Review Bot]

Comment on attachment 137786 [details]
Patch

Clearing flags on attachment: 137786

Committed r114580: <http://trac.webkit.org/changeset/114580>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.