Bug 62997 – [Qt] Add shadowblur support

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Igor Trindade Oliveira]

We should update the shadow code to use ShadowBlur which has platform independent code.

[Igor Trindade Oliveira]

Created attachment 97817 [details]
WIP patch

This patch is a working in progress.

[Igor Trindade Oliveira]

Created attachment 98244 [details]
Patch WIP

Working in progress patch v2.

[Igor Trindade Oliveira]

Created attachment 98622 [details]
Patch

Proposed patch.

[Collabora GTK+ EWS bot]

Comment on attachment 98622 [details]
Patch

Attachment 98622 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8946077

[Igor Trindade Oliveira]

Created attachment 98640 [details]
Patch

Proposed patch. fix gtk build.

[Simon Fraser (smfr)]

Comment on attachment 98640 [details]
Patch

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

> Source/WebCore/ChangeLog:39
> +        Use ShadowBlur instead of ContextShadow to handle canvas and css shadows. ShadowBlur is
> +        a newer and platform independent shadow implementation.
> +
> +        * WebCore.pro:
> +        * platform/graphics/GraphicsContext.cpp:
> +        * platform/graphics/GraphicsContext.h:
> +        * platform/graphics/ShadowBlur.cpp:
> +        (WebCore::ShadowBlur::ShadowBlur):
> +        (WebCore::ShadowBlur::mustUseShadowBlur):
> +        (WebCore::ShadowBlur::clear):
> +        (WebCore::ShadowBlur::blurAndColorShadowBuffer):
> +        (WebCore::ShadowBlur::beginShadowLayer):
> +        (WebCore::ShadowBlur::endShadowLayer):
> +        * platform/graphics/ShadowBlur.h:
> +        (WebCore::ShadowBlur::type):
> +        * platform/graphics/qt/ContextShadowQt.cpp: Removed.
> +        * platform/graphics/qt/FontQt.cpp:
> +        (WebCore::drawTextCommon):
> +        (WebCore::Font::drawGlyphs):
> +        * platform/graphics/qt/GraphicsContextQt.cpp:
> +        (WebCore::GraphicsContext::restorePlatformState):
> +        (WebCore::GraphicsContext::fillPath):
> +        (WebCore::GraphicsContext::strokePath):
> +        (WebCore::GraphicsContext::fillRect):
> +        (WebCore::GraphicsContext::fillRoundedRect):
> +        (WebCore::GraphicsContext::shadowBlur):
> +        (WebCore::GraphicsContext::clipBounds):
> +        (WebCore::GraphicsContext::setPlatformShadow):
> +        * platform/graphics/qt/ImageQt.cpp:
> +        (WebCore::BitmapImage::draw):
> +        * platform/graphics/qt/StillImageQt.cpp:
> +        (WebCore::StillImage::draw):

This should say more about what you changed. Why do you have to touch ShadowBlur code at all?

> Source/WebCore/platform/graphics/ShadowBlur.h:66
> +    bool mustUseShadowBlur(GraphicsContext*);

This should be const.

[Igor Trindade Oliveira]

Created attachment 98709 [details]
Patch

Proposed patch. Update ChangeLog and constify mustUseShadowBlur method.

[Simon Fraser (smfr)]

Comment on attachment 98709 [details]
Patch

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

> Source/WebCore/platform/graphics/ShadowBlur.cpp:262
> +bool ShadowBlur::mustUseShadowBlur(GraphicsContext* context) const
> +{
> +    // We can't avoid ShadowBlur, since the shadow has blur.
> +    if (m_type == ShadowBlur::BlurShadow)
> +        return true;
> +    // We can avoid ShadowBlur and optimize, since we're not drawing on a
> +    // canvas and box shadows are affected by the transformation matrix.
> +    if (!shadowsIgnoreTransforms())
> +        return false;
> +    // We can avoid ShadowBlur, since there are no transformations to apply to the canvas.
> +    if (context->getCTM().isIdentity())
> +        return false;
> +    // Otherwise, no chance avoiding ShadowBlur.
> +    return true;
> +}

This code doesn't belong here. It's asking Qt-specific questions. I see no reason why some other class can't ask the same questions.

[Igor Trindade Oliveira]

GraphicsContextCG.cpp has a static function called hasBlurredShadow() and it has the same behavior of mustUseShadowBlur my idea was put this function on ShadowBlur so Qt and CG can use it.

(In reply to comment #8)
> (From update of attachment 98709 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98709&action=review
> 
> > Source/WebCore/platform/graphics/ShadowBlur.cpp:262
> > +bool ShadowBlur::mustUseShadowBlur(GraphicsContext* context) const
> > +{
> > +    // We can't avoid ShadowBlur, since the shadow has blur.
> > +    if (m_type == ShadowBlur::BlurShadow)
> > +        return true;
> > +    // We can avoid ShadowBlur and optimize, since we're not drawing on a
> > +    // canvas and box shadows are affected by the transformation matrix.
> > +    if (!shadowsIgnoreTransforms())
> > +        return false;
> > +    // We can avoid ShadowBlur, since there are no transformations to apply to the canvas.
> > +    if (context->getCTM().isIdentity())
> > +        return false;
> > +    // Otherwise, no chance avoiding ShadowBlur.
> > +    return true;
> > +}
> 
> This code doesn't belong here. It's asking Qt-specific questions. I see no reason why some other class can't ask the same questions.

[Martin Robinson]

(In reply to comment #9)
> GraphicsContextCG.cpp has a static function called hasBlurredShadow() and it has the same behavior of mustUseShadowBlur my idea was put this function on ShadowBlur so Qt and CG can use it.

Cairo will also need this logic when it switches to ShadowBlur. Even if it should go in some other class, that class should be shared.

[Igor Trindade Oliveira]

I can rename the mustUseShadowBlur to hasBlurredShadow() because it is a better name and it can be reused by Qt, Cairo and CG. Simon, Martin what do you think?

(In reply to comment #10)
> (In reply to comment #9)
> > GraphicsContextCG.cpp has a static function called hasBlurredShadow() and it has the same behavior of mustUseShadowBlur my idea was put this function on ShadowBlur so Qt and CG can use it.
> 
> Cairo will also need this logic when it switches to ShadowBlur. Even if it should go in some other class, that class should be shared.

[Simon Fraser (smfr)]

(In reply to comment #11)
> I can rename the mustUseShadowBlur to hasBlurredShadow() because it is a better name and it can be reused by Qt, Cairo and CG. Simon, Martin what do you think?

That doesn't cover the shadowsIgnoreTransforms() or getCTM().isIdentity() tests.

[Igor Trindade Oliveira]

Yeah, you are right. Updating the patch.
 
(In reply to comment #12)
> (In reply to comment #11)
> > I can rename the mustUseShadowBlur to hasBlurredShadow() because it is a better name and it can be reused by Qt, Cairo and CG. Simon, Martin what do you think?
> 
> That doesn't cover the shadowsIgnoreTransforms() or getCTM().isIdentity() tests.

[Igor Trindade Oliveira]

Created attachment 98761 [details]
Patch

Proposed patch. Remove mustUseShadowBlur method from ShadowBlur class.

[Simon Fraser (smfr)]

Comment on attachment 98761 [details]
Patch

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

> Source/WebCore/platform/graphics/ShadowBlur.h:-45
>  class ShadowBlur {
> -    WTF_MAKE_NONCOPYABLE(ShadowBlur);

Why does it have to be copyable? This worries me, because it could result in two objects trashing the same m_layerImage.

[Igor Trindade Oliveira]

Qt uses ShadowBlur for CSS and Canvas shadows. For canvas when GraphicsContext::save is called we push the current ShadowBlur(it is an instance object) to a list and in GraphicsContext::restore we pop the object(GraphicsContextQt.cpp:330)so we need to be able to copy the object. This behavior was inherited from ContextShadow implementation.


(In reply to comment #15)
> (From update of attachment 98761 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98761&action=review
> 
> > Source/WebCore/platform/graphics/ShadowBlur.h:-45
> >  class ShadowBlur {
> > -    WTF_MAKE_NONCOPYABLE(ShadowBlur);
> 
> Why does it have to be copyable? This worries me, because it could result in two objects trashing the same m_layerImage.

[Simon Fraser (smfr)]

Is ShadowBlur just being push/popped to store the shadow state (blur radius etc), or will ShadowBlurs on the state stack also be holding onto live buffers?

I think you should implement a copy ctor that asserts that m_bufferImage is null then. Otherwise, make a helper class that you can push/pop to store the shadow state.

[Igor Trindade Oliveira]

Created attachment 98786 [details]
Patch

Proposed patch. Implement copy ctor that asserts that m_layerImage is null.

[Igor Trindade Oliveira]

We have a little bit complicated use case. Specially because we need ShadowBlur also to draw shadow on fonts. What i think could be done is to create setters for some ShadowBlur attributes and QtWebKit will have just one ShadowBlur object changing these attributes when the context is restored.

(In reply to comment #17)
> Is ShadowBlur just being push/popped to store the shadow state (blur radius etc), or will ShadowBlurs on the state stack also be holding onto live buffers?
> 
> I think you should implement a copy ctor that asserts that m_bufferImage is null then. Otherwise, make a helper class that you can push/pop to store the shadow state.

[Igor Trindade Oliveira]

Created attachment 98890 [details]
Patch

Proposed patch. Fix assert.

[Igor Trindade Oliveira]

Comment on attachment 98890 [details]
Patch

After look the bug https://bugs.webkit.org/show_bug.cgi?id=49917 we shouldn't be creating ShadowBlur when unnecessary.

[Igor Trindade Oliveira]

Created attachment 98918 [details]
Patch

Proposed patch. Use the GC state saved to restore the shadow state.

[Simon Fraser (smfr)]

Comment on attachment 98918 [details]
Patch

The ShadowBlur changes look OK to me in this one. I did not review the rest.

[Igor Trindade Oliveira]

Created attachment 98949 [details]
Patch.

Proposed patch. Fix crash when using inspector.

[Ariya Hidayat]

Does it take into account that endianness? See also https://bugs.webkit.org/show_bug.cgi?id=55180

[Igor Trindade Oliveira]

Good catch.
(In reply to comment #25)
> Does it take into account that endianness? See also https://bugs.webkit.org/show_bug.cgi?id=55180

[Igor Trindade Oliveira]

We are not blurring the ImageBuffer directly, so probably the fix done by bug https://bugs.webkit.org/show_bug.cgi?id=55180 is enough.

(In reply to comment #25)
> Does it take into account that endianness? See also https://bugs.webkit.org/show_bug.cgi?id=55180

[Ariya Hidayat]

> We are not blurring the ImageBuffer directly, so probably the fix done by bug https://bugs.webkit.org/show_bug.cgi?id=55180 is enough.

Which means it is rather scary. Consider this, on X11 we'll use QPixmap (via ImageBuffer, and thus ScratchBuffer). Before blurring, we get the pixel data by converting to QImage. Looks like a lot of round-trip and defeats the purpose of having the scratch buffer at the first place.

[Igor Trindade Oliveira]

Yeah I agree. ShadowBlur with raster paint engine will be a problem. But the advantages of moving to ShadowBlur also is satisfactory. We can have just one shadow implementation and all bug fixes and future optimizations done for a platform is shared between all platforms.
IMHO we could move to ShadowBlur and open a bug about ShadowBlur optimization.

(In reply to comment #28)
> > We are not blurring the ImageBuffer directly, so probably the fix done by bug https://bugs.webkit.org/show_bug.cgi?id=55180 is enough.
> 
> Which means it is rather scary. Consider this, on X11 we'll use QPixmap (via ImageBuffer, and thus ScratchBuffer). Before blurring, we get the pixel data by converting to QImage. Looks like a lot of round-trip and defeats the purpose of having the scratch buffer at the first place.

[Ariya Hidayat]

One way to avoid performance regression: force ImageBuffer to use QImage as the back-end when used with ScratchBuffer.

[Benjamin Poulain]

(In reply to comment #28)
> > We are not blurring the ImageBuffer directly, so probably the fix done by bug https://bugs.webkit.org/show_bug.cgi?id=55180 is enough.
> 
> Which means it is rather scary. Consider this, on X11 we'll use QPixmap (via ImageBuffer, and thus ScratchBuffer). Before blurring, we get the pixel data by converting to QImage. Looks like a lot of round-trip and defeats the purpose of having the scratch buffer at the first place.

Raster is now the default on X11.
We basically say raster graphic system is the only way to get correct rendering with QtWebKit.

We prefer QPixmap over QImage to enjoy some optimizations on ARM at the moment (like 16 bits color depth in some case, faster image conversion, etc.)

I would say it is ok to use QPixmap here for consistency with what is in WebCore. We plan to move to QImage explicitely for Qt 5.

[Simon Fraser (smfr)]

(In reply to comment #31)
> (In reply to comment #28)
> > > We are not blurring the ImageBuffer directly, so probably the fix done by bug https://bugs.webkit.org/show_bug.cgi?id=55180 is enough.
> > 
> > Which means it is rather scary. Consider this, on X11 we'll use QPixmap (via ImageBuffer, and thus ScratchBuffer). Before blurring, we get the pixel data by converting to QImage. Looks like a lot of round-trip and defeats the purpose of having the scratch buffer at the first place.

We don't blur the ImageBuffer directly.  We get the data into a ByteArray and blur that, then put it back.

[Andreas Kling]

Comment on attachment 98949 [details]
Patch.

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

r=me, but please fix GraphicsContext::clipBounds() before landing.

Also, are there really no changes to the pixel results of canvas and CSS shadow tests?

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:818
> +    return IntRect(clipRect.x(), clipRect.y(), clipRect.width(), clipRect.height());

This should be return enclosingIntRect(clipRect); or we may get 1px rounding errors.

[Igor Trindade Oliveira]

Created attachment 99735 [details]
Patch.

Proposed patch. Update clipBounds to use enclosingIntRect.
The ShadowBlur and ContextShadow share the same algorithm with minor modifications so the LayoutTests has the same results.

[WebKit Review Bot]

Comment on attachment 99735 [details]
Patch.

Clearing flags on attachment: 99735

Committed r90406: <http://trac.webkit.org/changeset/90406>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.