Bug 157857 – [iOS] PDFDocumentImage should not create a cached image larger than 4M pixels

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

This has been causing a memory jetsam on iOS devices when opening a large single page PDF file and zooming. When zooming on this PDF, PDFDocumentImage::draw() tries to update its cached image first. The purpose of this caching is to make redrawing the same part of the PDF document faster. The problem is the image we cache for a single tile has to be scaled with the current zooming factor. At the maximum zooming level, the image size is a little bit larger than (4000x4000) pixels. This requires 4000x4000x4=64MB.

This is a huge memory to be allocated for a single tile especially on low-memory machine or devices. Luckily PDFDocumentImage::draw() can fallback peacefully if the cached image can't be created for any reason. All it does is transformContextForPainting() and then  drawPDFPage() which will draw exactly the same thing the cached image would have drawn if it was created. 

The fix it to prevent creating the cached image for the PDFDocumentImage if its size is more than 4B pixels. The only drawback for this fix is the render will be a little bit slower when zooming on a PDFDocumentImage. But with lower zooming factors, the caching will still happen and the rendering/scrolling perf will not be affected.

[Said Abou-Hallawa]

Created attachment 279273 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/25876032>

[Said Abou-Hallawa]

Created attachment 279294 [details]
Patch

[Tim Horton]

Comment on attachment 279294 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:152
> +    if (!shouldCreateCachedImage(context, dstRect))

If you already have a cached image and then change scale, we should throw the old one away here, otherwise we'll keep reusing it.

[Simon Fraser (smfr)]

Comment on attachment 279294 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:123
> +    AffineTransform transform = context.getCTM(GraphicsContext::DefinitelyIncludeDeviceScale);
> +    FloatSize scaledSize(dstRect.width() * transform.xScale(), dstRect.height() * transform.yScale());

It's not clear that this copies logic that createCompatibleBuffer has. Maybe share the code?

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:124
> +    return scaledSize.area() < FloatSize(2000, 2000).area();

I don't think we have enough confidence that this is the right threshold, without regressing the bug that added the cache (bug 121207 and its associated radar).

[Said Abou-Hallawa]

Created attachment 279456 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 279294 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:123
>> +    FloatSize scaledSize(dstRect.width() * transform.xScale(), dstRect.height() * transform.yScale());
> 
> It's not clear that this copies logic that createCompatibleBuffer has. Maybe share the code?

Done. createCompatibleBuffer() was moved from GraphicsContext to ImageBuffer. A new function called ImageBuffer::canCreateCompatibleBuffer() is added to check the size limit. A shared function called ImageBuffer::compatibleBufferSize() is added to the size scaling and is used by ImageBuffer::createCompatibleBuffer() and ImageBuffer::canCreateCompatibleBuffer().

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:124
>> +    return scaledSize.area() < FloatSize(2000, 2000).area();
> 
> I don't think we have enough confidence that this is the right threshold, without regressing the bug that added the cache (bug 121207 and its associated radar).

I made the (2048x2048) limit be used for iOS or non-accelated drawing. Otherwise, we use the IOSurface::maximumSize() as the limit for the cached image.

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:152
>> +    if (!shouldCreateCachedImage(context, dstRect))
> 
> If you already have a cached image and then change scale, we should throw the old one away here, otherwise we'll keep reusing it.

Done. m_cachedImageBuffer is now set to nullptr before returning.

[Darin Adler]

Comment on attachment 279456 [details]
Patch

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

Looks OK. I have quite a few doubts and questions, though.

> Source/WebCore/ChangeLog:9
> +        the 4M pixels limit, do not create it. PDFDocumentImage::draw() fallbacks

Should be "falls back" rather than "fallbacks".

> Source/WebCore/ChangeLog:14
> +        through the cached image. This means the whole PDF will be drawn multiple
> +        times; one time for each tile. I think this is okay for zooming in a
> +        large PDFDocumentImage.

Are you sure this is OK. Couldn’t this be terribly slow with a complex PDF?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2451
> +        std::unique_ptr<ImageBuffer> maskImage = ImageBuffer::createCompatibleBuffer(maskRect.size(), *c);

Would be nice to use auto instead of writing out std::unique_ptr.

> Source/WebCore/platform/graphics/BitmapImage.cpp:605
> +        std::unique_ptr<ImageBuffer> buffer = ImageBuffer::createCompatibleBuffer(expandedIntSize(tileRect.size()), ctxt);

Would be nice to use auto instead of writing out std::unique_ptr.

> Source/WebCore/platform/graphics/FloatSize.h:92
> +    FloatSize scaledBy(const FloatSize& other) const
> +    {
> +        return FloatSize(m_width * other.m_width, m_height * other.m_height);
> +    }

Maybe this should just be operator* instead of a named function?

> Source/WebCore/platform/graphics/GraphicsContext.cpp:1036
> +    return FloatSize(transform.xScale(), transform.yScale());

Seems unfortunate for this to convert double to float every time. I am vexed by the way we mix double and float.

> Source/WebCore/platform/graphics/GraphicsContext.h:485
> +    FloatSize getCTMScale() const;

WebKit coding style prohibits the word "get" for functions like this one. I’m not super happy with the term “CTM” here. I know it’s a term of art for some graphics experts, but not for me.

> Source/WebCore/platform/graphics/ImageBuffer.cpp:171
> +    return expandedIntSize(size.scaledBy(context.getCTMScale()));

Seems inefficient to do this by converting to int and then back to float. Also will corrupt values that are outside the range of int.

> Source/WebCore/platform/graphics/ImageBuffer.cpp:178
> +    std::unique_ptr<ImageBuffer> buffer = ImageBuffer::createCompatibleBuffer(scaledSize, 1, ColorSpaceSRGB, context, hasAlpha);

Would be nicer to use auto here instead of writing out the type explicitly.

> Source/WebCore/platform/graphics/ImageBuffer.cpp:183
> +    buffer->context().scale(FloatSize(scaledSize.width() / size.width(), scaledSize.height() / size.height()));

Are we sure we won’t have any division by zero? Could use { } syntax instead of naming the type FloatSize here.

> Source/WebCore/platform/graphics/ImageBuffer.cpp:198
> +}
> +    
> +
> +bool ImageBuffer::isCompatibleWithContext(const GraphicsContext& context) const

Extra unwanted blank line here.

> Source/WebCore/platform/graphics/ImageBuffer.h:83
> +    // Create an image buffer compatible with the context, with suitable resolution
> +    // for drawing into the buffer and then into this context.

Should be all on one line.

> Source/WebCore/platform/graphics/ImageBuffer.h:87
> +    static bool canCreateCompatibleBuffer(const FloatSize&, const GraphicsContext&, const FloatSize& maxSize);

Seems peculiar to pass a maximum size when it’s actually a maximum area.

> Source/WebCore/platform/graphics/NamedImageGeneratedImage.cpp:67
> +    std::unique_ptr<ImageBuffer> imageBuffer = ImageBuffer::createCompatibleBuffer(size(), context, true);

Would be nice to use auto.

[Said Abou-Hallawa]

Created attachment 280014 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 279456 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        the 4M pixels limit, do not create it. PDFDocumentImage::draw() fallbacks
> 
> Should be "falls back" rather than "fallbacks".

Fixed.

>> Source/WebCore/ChangeLog:14
>> +        large PDFDocumentImage.
> 
> Are you sure this is OK. Couldn’t this be terribly slow with a complex PDF?

The cachedImage size limitation will be applied only on iOS and only when the systemPhysicalMemory <= 1GB.

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2451
>> +        std::unique_ptr<ImageBuffer> maskImage = ImageBuffer::createCompatibleBuffer(maskRect.size(), *c);
> 
> Would be nice to use auto instead of writing out std::unique_ptr.

Fixed.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:605
>> +        std::unique_ptr<ImageBuffer> buffer = ImageBuffer::createCompatibleBuffer(expandedIntSize(tileRect.size()), ctxt);
> 
> Would be nice to use auto instead of writing out std::unique_ptr.

Fixed

>> Source/WebCore/platform/graphics/FloatSize.h:92
>> +    }
> 
> Maybe this should just be operator* instead of a named function?

Operator* is added instead of a named function.

>> Source/WebCore/platform/graphics/GraphicsContext.h:485
>> +    FloatSize getCTMScale() const;
> 
> WebKit coding style prohibits the word "get" for functions like this one. I’m not super happy with the term “CTM” here. I know it’s a term of art for some graphics experts, but not for me.

The function name was changed to be scaleFactor().

>> Source/WebCore/platform/graphics/ImageBuffer.cpp:171
>> +    return expandedIntSize(size.scaledBy(context.getCTMScale()));
> 
> Seems inefficient to do this by converting to int and then back to float. Also will corrupt values that are outside the range of int.

ceilf() is used instead of expandedIntSize().

>> Source/WebCore/platform/graphics/ImageBuffer.cpp:178
>> +    std::unique_ptr<ImageBuffer> buffer = ImageBuffer::createCompatibleBuffer(scaledSize, 1, ColorSpaceSRGB, context, hasAlpha);
> 
> Would be nicer to use auto here instead of writing out the type explicitly.

Fixed.

>> Source/WebCore/platform/graphics/ImageBuffer.cpp:183
>> +    buffer->context().scale(FloatSize(scaledSize.width() / size.width(), scaledSize.height() / size.height()));
> 
> Are we sure we won’t have any division by zero? Could use { } syntax instead of naming the type FloatSize here.

Checking size.isEmpty() is added at the beginning of this function. Also I do not think we need to do this calculation because scaledSize / size is equal to context.scaleFactor() unless ceilf() in compatibleBufferSize() adds extra pixels. But do we need to scale by the ratio of the expanded scaledSize to size? I do not think so. isCompatibleWithContext() checks whether the scaleFactor of the drawing context is equal to the scaleFactor of ImageBuffer context to reuse the same cachedImage. If they are different because of this pixel expansion, we are going to recreate the cache every time the PDFDocumentImage is drawn. So the buffer->context should be scaled by context.scaleFactor().

>> Source/WebCore/platform/graphics/ImageBuffer.cpp:198
>> +bool ImageBuffer::isCompatibleWithContext(const GraphicsContext& context) const
> 
> Extra unwanted blank line here.

Removed.

>> Source/WebCore/platform/graphics/ImageBuffer.h:83
>> +    // for drawing into the buffer and then into this context.
> 
> Should be all on one line.

Done.

>> Source/WebCore/platform/graphics/ImageBuffer.h:87
>> +    static bool canCreateCompatibleBuffer(const FloatSize&, const GraphicsContext&, const FloatSize& maxSize);
> 
> Seems peculiar to pass a maximum size when it’s actually a maximum area.

This function is removed. Instead the caller calls ImageBuffer::compatibleBufferSize() and compare its area() with the maxArea.

>> Source/WebCore/platform/graphics/NamedImageGeneratedImage.cpp:67
>> +    std::unique_ptr<ImageBuffer> imageBuffer = ImageBuffer::createCompatibleBuffer(size(), context, true);
> 
> Would be nice to use auto.

Done.

[Said Abou-Hallawa]

Can someone in the CC list do a final review for this patch?

[Darin Adler]

Comment on attachment 280014 [details]
Patch

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

I’m going to say review- because we should not add a new physical memory function since we already have one.

> Source/WebCore/platform/cocoa/SystemInfo.h:28
> +WEBCORE_EXPORT unsigned long long systemPhysicalMemory();

Please do not add this. Instead use the ramSize() function from <wtf/RAMSize.h>.

> Source/WebCore/platform/graphics/ImageBuffer.cpp:193
> +    return { ceilf(scaledSize.width()), ceilf(scaledSize.height()) };

Should be using std::ceil rather than ceilf. But why aren’t we using expandedIntSize here? Further, I also don’t understand exactly why rounding to an integer is the correct thing to do, and why it is done here instead of in the createCompatibleBuffer function.

> Source/WebCore/platform/graphics/ImageBuffer.cpp:198
> +    return context.scaleFactor() == this->context().scaleFactor();

Given that these are computed with floating point math, is it safe to actually check == here? Could we get false negatives?

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:147
> +    // On iOS, if the physical memory is less than 1GB, do not allocate more than 16MB for the PDF cachedImage.

I don’t understand this rule; why no limit at all when the physical memory is greater than 1 GB?

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:148
> +    const float memoryThreshold = 1 << 30;

Why float for this? systemPhysicalMemory returns unsigned long long; I don’t understand the rationale for involving floating point. I would write this:

    const unsigned memoryThreshold = 1024 * MB;

Or you could even add GB to StdLibExtras.h alongside MB.

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:149
> +    const float maxArea = 1 << 22;

There is nothing in the expression 1 << 22 that says 16 MB to me. The comment doesn’t make it clear why this is the correct value for this constant. I assume this involves a factor of 4 because of the "4 bytes per pixel" assumption. If I wanted to make it possible to see the 16 MB I’d write something more like this:

    const float maxArea = 16 * MB / 4; // 16 MB maximum size, divided by a rough cost of 4 bytes per pixel of area.

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:175
> +    GraphicsContext& bufferContext = m_cachedImageBuffer->context();

I’d use auto& here.

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:186
> +    m_cachedBytes = internalSize.width() * internalSize.height() * 4;

What guarantees this calculation doesn’t overflow a 32-bit integer? While m_cachedBytes is a size_t, the calculation will be done as an int, since all three things we are multiplying are int. The safeCast below doesn’t do us any good because the overflow could happen here.

[Said Abou-Hallawa]

Created attachment 280375 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 280014 [details]
Patch

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

>> Source/WebCore/platform/cocoa/SystemInfo.h:28
>> +WEBCORE_EXPORT unsigned long long systemPhysicalMemory();
> 
> Please do not add this. Instead use the ramSize() function from <wtf/RAMSize.h>.

Done. I was not aware of ramSize() existence.

>> Source/WebCore/platform/graphics/ImageBuffer.cpp:193
>> +    return { ceilf(scaledSize.width()), ceilf(scaledSize.height()) };
> 
> Should be using std::ceil rather than ceilf. But why aren’t we using expandedIntSize here? Further, I also don’t understand exactly why rounding to an integer is the correct thing to do, and why it is done here instead of in the createCompatibleBuffer function.

Done. compatibleBufferSize() returns the scaled size without expansion. And createCompatibleBuffer() expandedIntSize(scaledSize) when creating the ImageBuffer.

>> Source/WebCore/platform/graphics/ImageBuffer.cpp:198
>> +    return context.scaleFactor() == this->context().scaleFactor();
> 
> Given that these are computed with floating point math, is it safe to actually check == here? Could we get false negatives?

Done. areEssentiallyEqual() is used instead of operator==().

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:147
>> +    // On iOS, if the physical memory is less than 1GB, do not allocate more than 16MB for the PDF cachedImage.
> 
> I don’t understand this rule; why no limit at all when the physical memory is greater than 1 GB?

Because I do not know the right heuristic to be applied it here. I am fixing a bug which happens only on low end devices. If there is a rule which covers all cases, I will be happy to change the code accordingly.

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:148
>> +    const float memoryThreshold = 1 << 30;
> 
> Why float for this? systemPhysicalMemory returns unsigned long long; I don’t understand the rationale for involving floating point. I would write this:
> 
>     const unsigned memoryThreshold = 1024 * MB;
> 
> Or you could even add GB to StdLibExtras.h alongside MB.

Done. A constant value for GB is added to StdLibExtras.h.

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:149
>> +    const float maxArea = 1 << 22;
> 
> There is nothing in the expression 1 << 22 that says 16 MB to me. The comment doesn’t make it clear why this is the correct value for this constant. I assume this involves a factor of 4 because of the "4 bytes per pixel" assumption. If I wanted to make it possible to see the 16 MB I’d write something more like this:
> 
>     const float maxArea = 16 * MB / 4; // 16 MB maximum size, divided by a rough cost of 4 bytes per pixel of area.

Done. The initialization of the maxArea is rewritten and a comment is added as well.

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:175
>> +    GraphicsContext& bufferContext = m_cachedImageBuffer->context();
> 
> I’d use auto& here.

Done.

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:186
>> +    m_cachedBytes = internalSize.width() * internalSize.height() * 4;
> 
> What guarantees this calculation doesn’t overflow a 32-bit integer? While m_cachedBytes is a size_t, the calculation will be done as an int, since all three things we are multiplying are int. The safeCast below doesn’t do us any good because the overflow could happen here.

Done. internalSize.width() was casted to size_t to force size_t multiplication.

[WebKit Commit Bot]

Comment on attachment 280375 [details]
Patch

Clearing flags on attachment: 280375

Committed r201629: <http://trac.webkit.org/changeset/201629>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.