Summary: | [CSS Masking] Implement luminance masking | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrei Parvu <parvu> | ||||||||||||
Component: | CSS | Assignee: | 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
Andrei Parvu
2013-08-29 09:34:53 PDT
Created attachment 210071 [details]
Patch
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. (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? (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? Created attachment 211014 [details]
Patch
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. Created attachment 212073 [details]
Patch
> > 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.
Created attachment 212441 [details]
Patch
Created attachment 212488 [details]
Patch
Comment on attachment 212488 [details]
Patch
r=me
Comment on attachment 212488 [details] Patch Clearing flags on attachment: 212488 Committed r156391: <http://trac.webkit.org/changeset/156391> All reviewed patches have been landed. Closing bug. |