Summary: | [TexMap] Use a premuliplied color in TextureMapperGL. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dongseong Hwang <dongseong.hwang> | ||||||||||||||
Component: | Layout and Rendering | Assignee: | Dongseong Hwang <dongseong.hwang> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | noam, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Dongseong Hwang
2012-12-27 02:02:28 PST
Created attachment 180780 [details]
Patch
I thought I can make layout test using background color like compositing/background-color/background-color-alpha.html, but alpha value of backgound color is always 1.0f. I think it is because RenderStyle::visitedDependentColor has some bug. Color RenderStyle::visitedDependentColor(int colorProperty) const { ... // FIXME: Technically someone could explicitly specify the color transparent, but for now we'll just // assume that if the background color is transparent that it wasn't set. Note that it's weird that // we're returning unvisited info for a visited link, but given our restriction that the alpha values // have to match, it makes more sense to return the unvisited background color if specified than it // does to return black. This behavior matches what Firefox 4 does as well. ... } I thought I can make layout test using drop shadow filter like css3/filters/effect-drop-shadow-hw.html, but our drop shadow seems not to be correct because there is big difference with the result of css3/filters/effect-drop-shadow.html So I tested manually using debug border color. If webkit supports background fractional color, we can make a pixel test case easily. Comment on attachment 180780 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180780&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:312 > + RGBA32 premultipliedARGB = premultipliedARGBFromColor(color); > + Color premultipliedColor(premultipliedARGB); > + premultipliedColor.getRGBA(r, g, b, a); This would read better as a one-liner: Color(premultipliedARGBFromColor(color)).getRGBA(r, g, b, a); The two local variables don’t add any clarity. (In reply to comment #3) > (From update of attachment 180780 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180780&action=review > > > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:312 > > + RGBA32 premultipliedARGB = premultipliedARGBFromColor(color); > > + Color premultipliedColor(premultipliedARGB); > > + premultipliedColor.getRGBA(r, g, b, a); > > This would read better as a one-liner: > > Color(premultipliedARGBFromColor(color)).getRGBA(r, g, b, a); > > The two local variables don’t add any clarity. Good suggestion! Created attachment 180849 [details]
Patch
Comment on attachment 180849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180849&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:435 > + Color(premultipliedARGBFromColor(color)).getRGBA(r, g, b, a); Look at TextureMapperLayer, we do premultiply the color, though maybe not in the correct way. if this patch tries to correct this it has to finish the job ;) (In reply to comment #6) > (From update of attachment 180849 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180849&action=review > > > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:435 > > + Color(premultipliedARGBFromColor(color)).getRGBA(r, g, b, a); > > Look at TextureMapperLayer, we do premultiply the color, though maybe not in the correct way. if this patch tries to correct this it has to finish the job ;) As I mentioned, I only tested debug border color. I'm not sure whether this patch fixes or messes up background color and drop shadow color. I assumed both are fine because this patch fixes debug border color although unresolved Bug 105787 is needed to test it. I'll survey more, especially TextureMapperLayer :) Created attachment 181131 [details]
Patch
(In reply to comment #6) > (From update of attachment 180849 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180849&action=review > > > Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:435 > > + Color(premultipliedARGBFromColor(color)).getRGBA(r, g, b, a); > > Look at TextureMapperLayer, we do premultiply the color, though maybe not in the correct way. if this patch tries to correct this it has to finish the job ;) TextureMapperLayer only converts background color, not drop shadow color and debug border color. And TextureMapperLayer must not convert a premultiplied color because TextureMapperImageBuffer needs an unmultiplied color. void TextureMapperImageBuffer::drawSolidColor(const FloatRect& rect, const TransformationMatrix& matrix, const Color& color) { ... context->fillRect(rect, color, ColorSpaceDeviceRGB); <- GraphicsContext expects an unmultiplied color. ... } It is natural for TextureMapperGL to convert color because converting premultiplied color is open gl source over blend implementation detail. The third patch removed color converting in TextureMapperLayer. Comment on attachment 181131 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181131&action=review > Source/WebCore/ChangeLog:12 > + Covered by existing pixel/ref tests when run on Qt/EFL/GTK. I think it needs a new ref test for directly composited solid color with opacity. > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:125 > + options.textureMapper->drawSolidColor(m_state.contentsRect, transform, m_state.solidColor); This still needs to multiply the solid color's alpha with the opacity value Created attachment 182103 [details]
Patch
Created attachment 182232 [details]
Patch
Comment on attachment 182232 [details] Patch (In reply to comment #10) > (From update of attachment 181131 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=181131&action=review > > I think it needs a new ref test for directly composited solid color with opacity. Yes, I added, but there are some issue. see below. > > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:125 > > + options.textureMapper->drawSolidColor(m_state.contentsRect, transform, m_state.solidColor); > > This still needs to multiply the solid color's alpha with the opacity value I had misunderstood. I removed only converting premultiplied color part. View in context: https://bugs.webkit.org/attachment.cgi?id=182232&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:122 > if (!m_state.solidColor.alpha()) Could you explain why this 'if' is needed? Currently, RenderStyle::visitedDependentColor get rid of alpha channel of background color as I mentioned #c2. But I'm not sure the behavior is correct. > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:-128 > - color = Color(r * opacity, g * opacity, b * opacity, a * opacity); Current code converts solidColor to premultiplied color using only opacity, without the alpha of solidColor. But TextureMapperImageBuffer expects unmultiplied color. > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:129 > + Color color(colorWithOverrideAlpha(rgba, effectiveAlpha)); I create new unmultiplied color applied by opacity and send it to TextureMapper. > LayoutTests/compositing/background-color/background-color-alpha-with-opacity.html:7 > + background-color: rgba(0, 128, 0, 128); RenderStyle::visitedDependentColor converts rgba(0, 128, 0, 128) to rgba(0, 128, 0, 255), so we can not test actual premultiplied alpha mixed with opacity and the alpha. But after RenderStyle::visitedDependentColor is fixed, this test can test correctly. Comment on attachment 182232 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182232&action=review >> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:122 >> if (!m_state.solidColor.alpha()) > > Could you explain why this 'if' is needed? > > Currently, RenderStyle::visitedDependentColor get rid of alpha channel of background color as I mentioned #c2. But I'm not sure the behavior is correct. I think this should be removed, you're right. > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:127 > + float alpha = alphaChannel(rgba) / 255.; Shouldn't this be 256? Comment on attachment 182232 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182232&action=review >>> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:122 >>> if (!m_state.solidColor.alpha()) >> >> Could you explain why this 'if' is needed? >> >> Currently, RenderStyle::visitedDependentColor get rid of alpha channel of background color as I mentioned #c2. But I'm not sure the behavior is correct. > > I think this should be removed, you're right. I'll remove > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:126 > + // See Color::getRGBA() to know how to extract alpha from color. As I commented, I copied from Color::getRGBA() void Color::getRGBA(float& r, float& g, float& b, float& a) const { r = red() / 255.0f; g = green() / 255.0f; b = blue() / 255.0f; a = alpha() / 255.0f; } Created attachment 182505 [details]
Patch
Comment on attachment 182505 [details] Patch Clearing flags on attachment: 182505 Committed r139674: <http://trac.webkit.org/changeset/139674> All reviewed patches have been landed. Closing bug. |