Bug 62997 - [Qt] Add shadowblur support
Summary: [Qt] Add shadowblur support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Igor Trindade Oliveira
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-06-20 10:04 PDT by Igor Trindade Oliveira
Modified: 2011-07-05 14:30 PDT (History)
10 users (show)

See Also:


Attachments
WIP patch (37.42 KB, patch)
2011-06-20 10:05 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
Patch WIP (38.04 KB, patch)
2011-06-22 14:34 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
Patch (39.07 KB, patch)
2011-06-26 07:49 PDT, Igor Trindade Oliveira
gustavo.noronha: commit-queue-
Details | Formatted Diff | Diff
Patch (41.91 KB, patch)
2011-06-26 15:32 PDT, Igor Trindade Oliveira
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
Patch (42.35 KB, patch)
2011-06-27 05:39 PDT, Igor Trindade Oliveira
simon.fraser: review-
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
Patch (42.37 KB, patch)
2011-06-27 12:07 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
Patch (42.77 KB, patch)
2011-06-27 14:30 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
Patch (42.77 KB, patch)
2011-06-28 02:58 PDT, Igor Trindade Oliveira
igor.oliveira: commit-queue-
Details | Formatted Diff | Diff
Patch (45.01 KB, patch)
2011-06-28 07:36 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
Patch. (45.30 KB, patch)
2011-06-28 11:23 PDT, Igor Trindade Oliveira
kling: review+
kling: commit-queue-
Details | Formatted Diff | Diff
Patch. (45.25 KB, patch)
2011-07-05 12:23 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Igor Trindade Oliveira 2011-06-20 10:04:13 PDT
We should update the shadow code to use ShadowBlur which has platform independent code.
Comment 1 Igor Trindade Oliveira 2011-06-20 10:05:04 PDT
Created attachment 97817 [details]
WIP patch

This patch is a working in progress.
Comment 2 Igor Trindade Oliveira 2011-06-22 14:34:46 PDT
Created attachment 98244 [details]
Patch WIP

Working in progress patch v2.
Comment 3 Igor Trindade Oliveira 2011-06-26 07:49:50 PDT
Created attachment 98622 [details]
Patch

Proposed patch.
Comment 4 Collabora GTK+ EWS bot 2011-06-26 08:08:49 PDT
Comment on attachment 98622 [details]
Patch

Attachment 98622 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8946077
Comment 5 Igor Trindade Oliveira 2011-06-26 15:32:23 PDT
Created attachment 98640 [details]
Patch

Proposed patch. fix gtk build.
Comment 6 Simon Fraser (smfr) 2011-06-26 22:47:36 PDT
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.
Comment 7 Igor Trindade Oliveira 2011-06-27 05:39:49 PDT
Created attachment 98709 [details]
Patch

Proposed patch. Update ChangeLog and constify mustUseShadowBlur method.
Comment 8 Simon Fraser (smfr) 2011-06-27 08:45:18 PDT
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.
Comment 9 Igor Trindade Oliveira 2011-06-27 08:54:49 PDT
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.
Comment 10 Martin Robinson 2011-06-27 09:14:29 PDT
(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.
Comment 11 Igor Trindade Oliveira 2011-06-27 10:34:28 PDT
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.
Comment 12 Simon Fraser (smfr) 2011-06-27 10:38:58 PDT
(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.
Comment 13 Igor Trindade Oliveira 2011-06-27 10:52:56 PDT
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.
Comment 14 Igor Trindade Oliveira 2011-06-27 12:07:55 PDT
Created attachment 98761 [details]
Patch

Proposed patch. Remove mustUseShadowBlur method from ShadowBlur class.
Comment 15 Simon Fraser (smfr) 2011-06-27 12:26:17 PDT
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.
Comment 16 Igor Trindade Oliveira 2011-06-27 12:50:37 PDT
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.
Comment 17 Simon Fraser (smfr) 2011-06-27 13:17:02 PDT
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.
Comment 18 Igor Trindade Oliveira 2011-06-27 14:30:02 PDT
Created attachment 98786 [details]
Patch

Proposed patch. Implement copy ctor that asserts that m_layerImage is null.
Comment 19 Igor Trindade Oliveira 2011-06-27 14:39:15 PDT
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.
Comment 20 Igor Trindade Oliveira 2011-06-28 02:58:14 PDT
Created attachment 98890 [details]
Patch

Proposed patch. Fix assert.
Comment 21 Igor Trindade Oliveira 2011-06-28 06:20:13 PDT
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.
Comment 22 Igor Trindade Oliveira 2011-06-28 07:36:39 PDT
Created attachment 98918 [details]
Patch

Proposed patch. Use the GC state saved to restore the shadow state.
Comment 23 Simon Fraser (smfr) 2011-06-28 08:17:54 PDT
Comment on attachment 98918 [details]
Patch

The ShadowBlur changes look OK to me in this one. I did not review the rest.
Comment 24 Igor Trindade Oliveira 2011-06-28 11:23:40 PDT
Created attachment 98949 [details]
Patch.

Proposed patch. Fix crash when using inspector.
Comment 25 Ariya Hidayat 2011-07-02 22:16:32 PDT
Does it take into account that endianness? See also https://bugs.webkit.org/show_bug.cgi?id=55180
Comment 26 Igor Trindade Oliveira 2011-07-03 04:49:59 PDT
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
Comment 27 Igor Trindade Oliveira 2011-07-04 02:54:24 PDT
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
Comment 28 Ariya Hidayat 2011-07-04 08:52:15 PDT
> 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.
Comment 29 Igor Trindade Oliveira 2011-07-04 09:54:25 PDT
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.
Comment 30 Ariya Hidayat 2011-07-04 18:25:08 PDT
One way to avoid performance regression: force ImageBuffer to use QImage as the back-end when used with ScratchBuffer.
Comment 31 Benjamin Poulain 2011-07-05 04:35:28 PDT
(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.
Comment 32 Simon Fraser (smfr) 2011-07-05 08:02:16 PDT
(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.
Comment 33 Andreas Kling 2011-07-05 11:26:01 PDT
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.
Comment 34 Igor Trindade Oliveira 2011-07-05 12:23:16 PDT
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.
Comment 35 WebKit Review Bot 2011-07-05 14:29:55 PDT
Comment on attachment 99735 [details]
Patch.

Clearing flags on attachment: 99735

Committed r90406: <http://trac.webkit.org/changeset/90406>
Comment 36 WebKit Review Bot 2011-07-05 14:30:03 PDT
All reviewed patches have been landed.  Closing bug.