Bug 105786 – [TexMap] Use a premuliplied color in TextureMapperGL.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dongseong Hwang]

TextureMapperGL always uses a premultiplied color, so we must convert
an unmultiplied color to a premultiplied color before setting the uniform value of
colorLocation.

[Dongseong Hwang]

Created attachment 180780 [details]
Patch

[Dongseong Hwang]

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.

[Darin Adler]

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.

[Dongseong Hwang]

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

[Dongseong Hwang]

Created attachment 180849 [details]
Patch

[Noam Rosenthal]

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 ;)

[Dongseong Hwang]

(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 :)

[Dongseong Hwang]

Created attachment 181131 [details]
Patch

[Dongseong Hwang]

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

[Noam Rosenthal]

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

[Dongseong Hwang]

Created attachment 182103 [details]
Patch

[Dongseong Hwang]

Created attachment 182232 [details]
Patch

[Dongseong Hwang]

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.

[Noam Rosenthal]

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?

[Dongseong Hwang]

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;
}

[Dongseong Hwang]

Created attachment 182505 [details]
Patch

[WebKit Review Bot]

Comment on attachment 182505 [details]
Patch

Clearing flags on attachment: 182505

Committed r139674: <http://trac.webkit.org/changeset/139674>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.