Summary: | [Chromium] Solid color layers should respect opacity value. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Reveman <reveman> | ||||||||
Component: | WebCore Misc. | Assignee: | David Reveman <reveman> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cc-bugs, jamesr, piman, webkit.review.bot, wjmaclean | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 84215 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
David Reveman
2012-04-17 14:56:18 PDT
Created attachment 137611 [details]
Patch
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 on attachment 137611 [details]
Patch
I agree with Antoine.
(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. Created attachment 137627 [details]
Patch
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. (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. OK, NVM, the shader already multiplies RGB by A. So this LGTM. 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. Will rebase and land this patch after: https://bugs.webkit.org/show_bug.cgi?id=84215 Created attachment 137786 [details]
Patch
Rebase
Comment on attachment 137786 [details] Patch Clearing flags on attachment: 137786 Committed r114580: <http://trac.webkit.org/changeset/114580> All reviewed patches have been landed. Closing bug. |