-webkit-text-decoration-skip: ink is slow
Created attachment 217017 [details] Patch
If a process exits without releasing all of its IOSurfaces, is that video memory gone forever?
(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.
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.
Created attachment 217074 [details] Patch
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.
Created attachment 217077 [details] Patch
Created attachment 217083 [details] Patch
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.
*** This bug has been marked as a duplicate of bug 125718 ***