Bug 124394 – -webkit-text-decoration-skip: ink is slow

• Component: New Bugs
• Status: RESOLVED DUPLICATE

[Myles C. Maxfield]

-webkit-text-decoration-skip: ink is slow

[Myles C. Maxfield]

Created attachment 217017 [details]
Patch

[Myles C. Maxfield]

If a process exits without releasing all of its IOSurfaces, is that video memory gone forever?

[Tim Horton]

(In reply to comment #2)
> If a process exits without releasing all of its IOSurfaces, is that video memory gone forever?

My reading of IOSurfaceAPI.h says that the kernel will reclaim said memory.

[Simon Fraser (smfr)]

Comment on attachment 217017 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext.cpp:813
> +IntSize GraphicsContext::compatibleBufferPixelSize(const IntSize& size) const
>  {
>      // Make the buffer larger if the context's transform is scaling it so we need a higher
>      // resolution than one pixel per unit. Also set up a corresponding scale factor on the
>      // graphics context.
>  
>      AffineTransform transform = getCTM(DefinitelyIncludeDeviceScale);
> -    IntSize scaledSize(static_cast<int>(ceil(size.width() * transform.xScale())), static_cast<int>(ceil(size.height() * transform.yScale())));
> +    return IntSize(static_cast<int>(ceil(size.width() * transform.xScale())), static_cast<int>(ceil(size.height() * transform.yScale())));

It's possibly confusing that this looks at GraphicsContext state to determine buffer size. I think it would be better as a static function, passing in the transform.

> Source/WebCore/platform/graphics/GraphicsContext.cpp:834
> +    IntSize scaledSize = compatibleBufferPixelSize(size);
> +    buffer.resetBuffer();
> +    buffer.context()->scale(FloatSize(static_cast<float>(scaledSize.width()) / size.width(),
> +        static_cast<float>(scaledSize.height()) / size.height()));

Not related to this patch, but this scaling strategy means that sometimes the scale in the buffer won't quite match the scale in the target context.

> Source/WebCore/rendering/InlineTextBox.cpp:1067
> +            DEFINE_STATIC_LOCAL(OwnPtr<ImageBuffer>, imageBuffer, ());
> +            DEFINE_STATIC_LOCAL(IntSize, imageBufferSize, ());

This means that a single rendering of a skip-ink underline is going to keep hold of an ImageBuffer for the rest of the life of the process. I don't think we want that. In some places (e.g. ShadowBlur) we use a timer to clear caches like this; I think we should centralize this logic into one buffer cache somewhere.

[Myles C. Maxfield]

Created attachment 217074 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 217017 [details]
Patch

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

>> Source/WebCore/platform/graphics/GraphicsContext.cpp:813
>> +    return IntSize(static_cast<int>(ceil(size.width() * transform.xScale())), static_cast<int>(ceil(size.height() * transform.yScale())));
> 
> It's possibly confusing that this looks at GraphicsContext state to determine buffer size. I think it would be better as a static function, passing in the transform.

Done.

>> Source/WebCore/platform/graphics/GraphicsContext.cpp:834
>> +        static_cast<float>(scaledSize.height()) / size.height()));
> 
> Not related to this patch, but this scaling strategy means that sometimes the scale in the buffer won't quite match the scale in the target context.

https://bugs.webkit.org/show_bug.cgi?id=124432

>> Source/WebCore/rendering/InlineTextBox.cpp:1067
>> +            DEFINE_STATIC_LOCAL(IntSize, imageBufferSize, ());
> 
> This means that a single rendering of a skip-ink underline is going to keep hold of an ImageBuffer for the rest of the life of the process. I don't think we want that. In some places (e.g. ShadowBlur) we use a timer to clear caches like this; I think we should centralize this logic into one buffer cache somewhere.

Done.

[Myles C. Maxfield]

Created attachment 217077 [details]
Patch

[Myles C. Maxfield]

Created attachment 217083 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 217083 [details]
Patch

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

> Source/WebCore/platform/graphics/ScratchBuffer.h:107
> +    bool setCachedShadowValues(const FloatSize& radius, const Color& color, ColorSpace colorSpace, const FloatRect& shadowRect, const RoundedRect::Radii& radii, const FloatSize& layerSize)
> +    {
> +        if (!m_lastWasInset && m_lastRadius == radius && m_lastColor == color && m_lastColorSpace == colorSpace && m_lastShadowRect == shadowRect &&  m_lastRadii == radii && m_lastLayerSize == layerSize)
> +            return false;
> +
> +        m_lastWasInset = false;
> +        m_lastRadius = radius;
> +        m_lastColor = color;
> +        m_lastColorSpace = colorSpace;
> +        m_lastShadowRect = shadowRect;
> +        m_lastRadii = radii;
> +        m_lastLayerSize = layerSize;
> +
> +        return true;
> +    }
> +
> +    bool setCachedInsetShadowValues(const FloatSize& radius, const Color& color, ColorSpace colorSpace, const FloatRect& bounds, const FloatRect& shadowRect, const RoundedRect::Radii& radii)
> +    {
> +        if (m_lastWasInset && m_lastRadius == radius && m_lastColor == color && m_lastColorSpace == colorSpace && m_lastInsetBounds == bounds && shadowRect == m_lastShadowRect && radii == m_lastRadii)
> +            return false;
> +
> +        m_lastWasInset = true;
> +        m_lastInsetBounds = bounds;
> +        m_lastRadius = radius;
> +        m_lastColor = color;
> +        m_lastColorSpace = colorSpace;
> +        m_lastShadowRect = shadowRect;
> +        m_lastRadii = radii;
> +
> +        return true;
> +    }

This class shouldn't have shadow-related stuff in it.

> Source/WebCore/rendering/InlineTextBox.cpp:1081
> +                FloatRect imageBufferRectInUserSpace = FloatRect(enclosingUnderlineRect.location(),
> +                    FloatSize(enclosingUnderlineRect.size().width() * xMagnification,
> +                        enclosingUnderlineRect.size().height() * yMagnification));

Would be nice to push this complexity into some buffer class that under the hood can use larger buffers.

[Myles C. Maxfield]

*** This bug has been marked as a duplicate of bug 125718 ***