Bug 121207 – [mac] Cache rendered image in PDFDocumentImage

• Component: PDF
• Status: RESOLVED FIXED

[Tim Horton]

Apparently, PDFKit/CGPDF have a significant fixed cost per paint, regardless of clipping. Therefore, rendering into multiple tiles via TileController, etc. is much more expensive than it would be if we drew into a bitmap. So, let's do that (up to some size where we think the memory cost of the bitmap is too significant).

This can also provide caching between paints, and a convenient way to hook into the image quality mechanism to make resizing PDFDocumentImage very fast.

<rdar://problem/14930928>

[Tim Horton]

Created attachment 211402 [details]
prelim

[WebKit Commit Bot]

Attachment 211402 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/loader/cache/CachedImage.cpp', u'Source/WebCore/platform/graphics/Image.h', u'Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp', u'Source/WebCore/platform/graphics/cg/PDFDocumentImage.h', u'Source/WebCore/rendering/ImageQualityController.cpp']" exit_code: 1
Source/WebCore/platform/graphics/cg/PDFDocumentImage.h:85:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Does this cache ever need to be reset? Perhaps when system antialiasing settings change?

[Tim Horton]

Created attachment 211457 [details]
patch

[Tim Horton]

(In reply to comment #3)
> Does this cache ever need to be reset? Perhaps when system antialiasing settings change?

Maybe! It is currently reset in a few obvious cases. Color-profile changes and antialiasing changes are also interesting cases.

[Simon Fraser (smfr)]

Comment on attachment 211457 [details]
patch

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

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:132
> +    if (dstRect.size() != m_cachedDestinationSize)
> +        return false;
> +
> +    if (srcRect != m_cachedSourceRect)
> +        return false;

You should do these cheap tests before the expensive decompose-based ones

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:156
> +        bufferContext->scale(FloatSize(hScale, vScale));
> +        bufferContext->scale(FloatSize(1, -1));

You could just do -vScale on the first line.

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:166
> +        int oldCachedBytes = m_cachedBytes;

size_t

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:170
> +            imageObserver()->decodedSizeChanged(this, m_cachedBytes - oldCachedBytes);

But beware of unsigned math here.

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.h:59
> +    virtual bool isPDFDocumentImage() const OVERRIDE { return true; }

FINAL?

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.h:85
> +    bool cacheParametersMatch(GraphicsContext*, const FloatRect& dstRect, const FloatRect& srcRect);

can this be const?

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.h:97
> +    int m_cachedBytes;

size_t?

[Simon Fraser (smfr)]

Comment on attachment 211457 [details]
patch

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

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:150
> +        m_cachedImageBuffer = context->createCompatibleBuffer(imageSize);
> +        GraphicsContext* bufferContext = m_cachedImageBuffer->context();

You should test for a null buffer (it might be giant).

[Tim Horton]

Created attachment 211486 [details]
patch

[Tim Horton]

Created attachment 211487 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 211487 [details]
patch

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

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.h:59
> +    virtual bool isPDFDocumentImage() const OVERRIDE { return true; }

FINAL?

[Tim Horton]

(In reply to comment #10)
> (From update of attachment 211487 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211487&action=review
> 
> > Source/WebCore/platform/graphics/cg/PDFDocumentImage.h:59
> > +    virtual bool isPDFDocumentImage() const OVERRIDE { return true; }
> 
> FINAL?

I don't think there's a reason to do that when the class is FINAL in its entirety, but maybe someone else knows?

[Tim Horton]

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