Bug 51567 – Enhance ShadowBlur to tile inset box shadows

• Component: Platform
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

RenderBoxModelObject::paintBoxShadow paints inset box shadows by creating a path which is a rect with a hole in the middle, and filling with an event-odd fill rule.

We should enhance GraphicsContext and ContextShadow so that we can render this with a tiled shadow.

[Simon Fraser (smfr)]

Created attachment 77424 [details]
Part 1: new method on GraphicsContext.

[Ariya Hidayat]

Comment on attachment 77424 [details]
Part 1: new method on GraphicsContext.

LGTM. I'm not superhappy with the name fillRectWithRoundedHole but I also can't come up with a clearer one. I think this one will do just fine.

[Eric Seidel (no email)]

Did this land?

[Simon Fraser (smfr)]

Not yet.

[Simon Fraser (smfr)]

Comment on attachment 77424 [details]
Part 1: new method on GraphicsContext.

Landed in http://trac.webkit.org/changeset/77106

[Simon Fraser (smfr)]

Created attachment 80619 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 80619 [details]
Patch

http://trac.webkit.org/changeset/77110

Keep bug open for tiling optimization.

[Simon Fraser (smfr)]

In drawInsetShadow(), rect can also be very large. We could be smarter about limiting the size of the layer, but we need to take the shadow offsets into account too.

[Simon Fraser (smfr)]

Another enhancement (for all shadows) would be to remember the parameters for the last shadow rendered into the scratch buffer, and just re-use the buffer if the current parameters are the same. That would pay off on pages with consecutive shadowed boxes of the same size.

[Simon Fraser (smfr)]

Created attachment 81673 [details]
Patch

[Dirk Schulze]

Comment on attachment 81673 [details]
Patch

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

r=me but with comments.

> Source/WebCore/platform/graphics/ShadowBlur.cpp:410
> +    leftSlice= twiceRadius + max(radii.topLeft().width(), radii.bottomLeft().width()); 

missing space after leftSlice

> Source/WebCore/platform/graphics/ShadowBlur.cpp:-505
> -        1 | 2 | 3
> -       -----------
> -        4 |   | 5
> -       -----------
> -        6 | 7 | 8
> -
> -     The corners are directly copied from the template rectangle to the
> -     real one and the side tiles are 1 pixel width, we use them as
> -
> -     tiles to cover the destination side. The corner tiles are bigger
> -     than just the side of the rounded corner, we need to increase it
> -     because the modifications caused by the corner over the blur
> -     effect. We fill the central part with solid color to complete the
> -     shadow.

I think this comment is useful. I wouldn't remove it.

> Source/WebCore/platform/graphics/ShadowBlur.cpp:539
> +    boundingRect.move(m_offset.width(), m_offset.height());

boundingRect.move(m_offset);

> Source/WebCore/platform/graphics/ShadowBlur.cpp:542
> +    destHoleRect.move(m_offset.width(), m_offset.height());

ditto

> Source/WebCore/platform/graphics/ShadowBlur.cpp:607
> +    const int templateSideLength = 1;

You are using this at least twice. Can you add a static const at the beginning of the document?

> Source/WebCore/platform/graphics/ShadowBlur.cpp:686
> +    {
> +        IntRect blurRect(IntPoint(), templateSize);
> +        RefPtr<ByteArray> layerData = m_layerImage->getUnmultipliedImageData(blurRect);
> +        blurLayerImage(layerData->data(), blurRect.size(), blurRect.width() * 4);
> +        m_layerImage->putUnmultipliedImageData(layerData.get(), blurRect.size(), blurRect, IntPoint());
> +    }

The braces seem useless to me.

> Source/WebCore/platform/graphics/ShadowBlur.cpp:692
> +    shadowContext->fillRect(FloatRect(0, 0, templateSize.width(), templateSize.height()));

argh. Why don't we have a ctor FloatRect(IntPoint, IntSize), you're using this multiple times :-)

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/78062