Bug 120476 – [CSS Masking] Implement luminance masking

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Andrei Parvu]

In order to comply to the CSS Masking specification we need to support luminance masks as well. In order to apply a luminance mask, we need to transform its RGB values into an alpha value, using luminance-to-alpha coefficients.

[Andrei Parvu]

Created attachment 210071 [details]
Patch

[Darin Adler]

Comment on attachment 210071 [details]
Patch

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

> Source/WebCore/platform/graphics/Image.h:191
> +    bool isLuminanceMask() const { return m_isLuminanceMask; }
> +    void setIsLuminanceMask() { m_isLuminanceMask = true; }

This seems like a strange layer to add this concept at. All images have such a flag? The point of this class is an abstraction for an image. We shouldn’t be building in magic images into the platform layer. The platform layer can provide a new drawing function, but the actual logic for triggering it needs to be up at the filter/SVG level of the call.

[Andrei Parvu]

(In reply to comment #2)
> (From update of attachment 210071 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210071&action=review
> 
> > Source/WebCore/platform/graphics/Image.h:191
> > +    bool isLuminanceMask() const { return m_isLuminanceMask; }
> > +    void setIsLuminanceMask() { m_isLuminanceMask = true; }
> 
> This seems like a strange layer to add this concept at. All images have such a flag? The point of this class is an abstraction for an image. We shouldn’t be building in magic images into the platform layer. The platform layer can provide a new drawing function, but the actual logic for triggering it needs to be up at the filter/SVG level of the call.

I don't quite understand what you mean by filter/SVG level of the call. As I see it, another way to deal with this would be to add another parameter to the drawTiledImage / drawPattern functions, which would be defaulted to false. Is this what you wanted to suggest?

[Andrei Parvu]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 210071 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=210071&action=review
> > 
> > > Source/WebCore/platform/graphics/Image.h:191
> > > +    bool isLuminanceMask() const { return m_isLuminanceMask; }
> > > +    void setIsLuminanceMask() { m_isLuminanceMask = true; }
> > 
> > This seems like a strange layer to add this concept at. All images have such a flag? The point of this class is an abstraction for an image. We shouldn’t be building in magic images into the platform layer. The platform layer can provide a new drawing function, but the actual logic for triggering it needs to be up at the filter/SVG level of the call.
> 
> I don't quite understand what you mean by filter/SVG level of the call. As I see it, another way to deal with this would be to add another parameter to the drawTiledImage / drawPattern functions, which would be defaulted to false. Is this what you wanted to suggest?

Looking through the code, it seems that we could either move the luminanceMask flag from the Image class to the GraphicsContext class, and then use it from there in the drawPattern functions, or, as I said above, add another parameter to the drawPattern function.  Darin, Dirk, any suggestions regarding one of these two approaches?

[Andrei Parvu]

Created attachment 211014 [details]
Patch

[Dirk Schulze]

Comment on attachment 211014 [details]
Patch

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

Some nits.

> Source/WebCore/ChangeLog:19
> +        * platform/graphics/GeneratorGeneratedImage.cpp: Converted the ImageBuffer to luminance, if necessary.

s/Converted/Convert/

> Source/WebCore/ChangeLog:28
> +        * svg/graphics/SVGImage.cpp: Converted the ImageBuffer to luminance, if necessary.

Ditto.

> Source/WebCore/ChangeLog:30
> +        * svg/graphics/SVGImageForContainer.cpp: Passed the luminance property to the SVG image.

s/Passed/Pass/

> Source/WebCore/platform/graphics/BitmapImage.cpp:508
> +    if (!ctxt->drawLuminanceMask())
> +        Image::drawPattern(ctxt, tileRect, transform, phase, styleColorSpace, op, destRect, blendMode);
> +    else {

please use early return here"

if (!ctxt->drawLuminanceMask()) {
    Image::drawPattern(ctxt, tileRect, transform, phase, styleColorSpace, op, destRect, blendMode);
    return;
}

> Source/WebCore/platform/graphics/BitmapImage.cpp:510
> +            OwnPtr<ImageBuffer> buffer = ImageBuffer::create(expandedIntSize(tileRect.size()), 1);

Why do you expend every image buffer size by 1? Doesn't that cause problems with the offset?

> Source/WebCore/platform/graphics/BitmapImage.cpp:512
> +            if (!buffer)
> +                return;

Can the tileRect be zero size or negative? If not, you may want an assertion here instead. Even though, better to check for tileRect size isEmpty() at the beginning. and return earlier in this case.

> Source/WebCore/platform/graphics/BitmapImage.cpp:520
> +            draw(buffer->context(), tileRect, tileRect, styleColorSpace, op, blendMode);

Not you do not respect the 1px offset because of your expand earlier.

> Source/WebCore/platform/graphics/BitmapImage.h:137
> +        const FloatPoint& phase, ColorSpace styleColorSpace, CompositeOperator, const FloatRect& destRect, BlendMode = BlendModeNormal);

I think you miss a makro OVERRIDE at the end of the definition.

[Andrei Parvu]

Created attachment 212073 [details]
Patch

[Andrei Parvu]

> > Source/WebCore/platform/graphics/BitmapImage.cpp:510
> > +            OwnPtr<ImageBuffer> buffer = ImageBuffer::create(expandedIntSize(tileRect.size()), 1);
> 
> Why do you expend every image buffer size by 1? Doesn't that cause problems with the offset?
> 
That was not size expanding, it was a resolutionScale parameter, which is anyway defaulted to 1.

[Andrei Parvu]

Created attachment 212441 [details]
Patch

[Andrei Parvu]

Created attachment 212488 [details]
Patch

[Dirk Schulze]

Comment on attachment 212488 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 212488 [details]
Patch

Clearing flags on attachment: 212488

Committed r156391: <http://trac.webkit.org/changeset/156391>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.