Bug 120476

Summary: [CSS Masking] Implement luminance masking
Product: WebKit Reporter: Andrei Parvu <parvu>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andrei.prv, commit-queue, d-r, esprehn+autocc, fmalita, glenn, hyatt, kondapallykalyan, krit, pdr, schenney, WebkitBugTracker
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Andrei Parvu 2013-08-29 09:34:53 PDT
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.
Comment 1 Andrei Parvu 2013-08-29 23:47:17 PDT
Created attachment 210071 [details]
Patch
Comment 2 Darin Adler 2013-08-30 09:20:12 PDT
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.
Comment 3 Andrei Parvu 2013-09-02 08:25:51 PDT
(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?
Comment 4 Andrei Parvu 2013-09-06 07:24:50 PDT
(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?
Comment 5 Andrei Parvu 2013-09-09 00:32:41 PDT
Created attachment 211014 [details]
Patch
Comment 6 Dirk Schulze 2013-09-19 07:25:16 PDT
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.
Comment 7 Andrei Parvu 2013-09-19 08:50:04 PDT
Created attachment 212073 [details]
Patch
Comment 8 Andrei Parvu 2013-09-19 08:51:44 PDT
> > 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.
Comment 9 Andrei Parvu 2013-09-24 01:58:30 PDT
Created attachment 212441 [details]
Patch
Comment 10 Andrei Parvu 2013-09-24 11:52:26 PDT
Created attachment 212488 [details]
Patch
Comment 11 Dirk Schulze 2013-09-25 06:28:14 PDT
Comment on attachment 212488 [details]
Patch

r=me
Comment 12 WebKit Commit Bot 2013-09-25 06:51:58 PDT
Comment on attachment 212488 [details]
Patch

Clearing flags on attachment: 212488

Committed r156391: <http://trac.webkit.org/changeset/156391>
Comment 13 WebKit Commit Bot 2013-09-25 06:52:03 PDT
All reviewed patches have been landed.  Closing bug.