Bug 158715 – [iOS] PDFDocumentImage should cache only a sub image of the PDF when caching the whole image is expensive

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

This is a follow up for the bug https://bugs.webkit.org/show_bug.cgi?id=157857.

This bug was partially fixed in http://trac.webkit.org/changeset/201629 for low end devices whose ram sizes are less than 1GB ram. The bug was not addressed for higher end devices which larger ram sizes.

We need to use some heuristic than can work for all ram sizes. We can say, for every 1GB of ram we allow 16MB memory increase in the PDF cachedImage size. Or more formally, the PDF cachedImage will be created only:

    if (ramSize() / PDF_cachedImage_size > 1GB / 16MB)

[Said Abou-Hallawa]

Created attachment 281219 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 281219 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:152
> +    if (ramSize() / ImageBuffer::compatibleBufferSize(dstRect.size(), context).area() < memoryThreshold / maxArea) {

Can compatibleBufferSize return 0?

[Said Abou-Hallawa]

Created attachment 283110 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283201 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #2)
> Comment on attachment 281219 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281219&action=review
> 
> > Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:152
> > +    if (ramSize() / ImageBuffer::compatibleBufferSize(dstRect.size(), context).area() < memoryThreshold / maxArea) {
> 
> Can compatibleBufferSize return 0?

I changed the patch to allocate memory for a rectangle of the image only.

[Said Abou-Hallawa]

Can I get this patch reviewed? Any feedback would be also very helpful.

[Simon Fraser (smfr)]

Comment on attachment 283201 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:146
> +    // Moving coordinates to -srcRect.location() => srcContext(srcRect.x(), srcRect.y()) -> dstContext(0, 0)
> +    // Moving coordinates to dstRect.location() => srcContext(0, 0) -> dstContext(dstRect.x(), dstRect.y())
> +    // Combining both => srcContext(srcRect.x(), srcRect.y()) -> dstContext(dstRect.x(), dstRect.y())
> +    context.translate(dstRect.x() - srcRect.x(), dstRect.y() - srcRect.y());

This seems like a bug fix, so can you make a layout test for it?

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:170
> +    FloatRect rect = context.clipBounds();
> +    rect.setX(std::max(rect.center().x() - maxSize.width() / 2, dstRect.x()));
> +    rect.setY(std::max(rect.center().y() - maxSize.height() / 2, dstRect.y()));
> +
> +    // Cover as much as we could from the dstRect.
> +    rect.setWidth(std::min(maxSize.width(), dstRect.width()));
> +    rect.setHeight(std::min(maxSize.height(), dstRect.height()));

It's hard to follow this logic, and I'm not sure if rect will always cover destRect. Can you rewrite this using shiftXEdgeTo() etc?

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:214
> +    if (s_allDecodedDataSize + m_cachedImageRect.size().area() * 4 - m_cachedBytes > s_maxDecodedDataSize) {
> +        destroyDecodedData();

Should m_cachedImageRect be left as a valid rectangle if we didn't actually cache anything?

[Said Abou-Hallawa]

Created attachment 283654 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/25876032>

[Said Abou-Hallawa]

Comment on attachment 283201 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:146
>> +    context.translate(dstRect.x() - srcRect.x(), dstRect.y() - srcRect.y());
> 
> This seems like a bug fix, so can you make a layout test for it?

Done. But I had to add an internal setting to disable caching the PDF image to expose the bug. The new test fails without this fix.

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:170
>> +    rect.setHeight(std::min(maxSize.height(), dstRect.height()));
> 
> It's hard to follow this logic, and I'm not sure if rect will always cover destRect. Can you rewrite this using shiftXEdgeTo() etc?

The size of dstRect is always the whole image size. Drawing the image relies on clipping the graphics context to the dirty rectangle. Because we scale the cached image rectangle by the scaling factor, this means we have to allocate memory for imageSize * scaleFator which might exceed the memory limit of the device.

Instead we try to cover the dirty rectangle and get as much as we could around it. So I start from the center of the dirty rectangle and get the maximum rectangle that is smaller than the memory limit. This means rect may not cover dstRect.

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:214
>> +        destroyDecodedData();
> 
> Should m_cachedImageRect be left as a valid rectangle if we didn't actually cache anything?

destroyDecodedData() now sets m_cachedImageRect to an empty rectangle.

[Said Abou-Hallawa]

Created attachment 283658 [details]
Patch

[Dean Jackson]

Comment on attachment 283658 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:154
> +    // Moving coordinates to -srcRect.location() => srcContext(srcRect.x(), srcRect.y()) -> dstContext(0, 0)
> +    // Moving coordinates to dstRect.location() => srcContext(0, 0) -> dstContext(dstRect.x(), dstRect.y())
> +    // Combining both => srcContext(srcRect.x(), srcRect.y()) -> dstContext(dstRect.x(), dstRect.y())

I don't understand what these comments mean.

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:163
> +// To avoid the jetsam on iOS, we are going to limit the size of all the PDF cachedImages to be 64MB.
> +static const size_t s_maxCachedImageSide = 4 * KB;
> +static const size_t s_maxCachedImageSize = s_maxCachedImageSide * s_maxCachedImageSide * 4;

Isn't that 64KB, not 64MB?

Also, maybe we should use Edge rather than Side, because I thought it was a typo :)

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:224
> +    // Cache the PDF image only if the size of the new image won't exceed the limit

Nit: missing .

[Said Abou-Hallawa]

Created attachment 283820 [details]
Patch

[Said Abou-Hallawa]

Created attachment 283826 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 283658 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:154
>> +    // Combining both => srcContext(srcRect.x(), srcRect.y()) -> dstContext(dstRect.x(), dstRect.y())
> 
> I don't understand what these comments mean.

I fixed the comment but here is more explanation I used because the original code confused me:

The following translation 

    context.translate(dstRect.x() - srcRect.x(), dstRect.y() - srcRect.y());

means a src point will be drawn at a dst point according to the following transformation:

| dstX |   | 1  0  dstRect.x() - srcRect.x() | | srcX |   | srcX + dstRect.x() - srcRect.x() |
| dstY | = | 0  1  dstRect.y() - srcRect.y() | | srcY | = | srcY + dstRect.y() - srcRect.y() |
|   1  |   | 0  0             1              | |  1   |   |                1                 |

If we want to get which src point will be drawn at the top-left corner of dstRect we can use the following equality

| dstRect.x() | = | srcX + dstRect.x() - srcRect.x() |                                | srcX |   | srcRect.x() |
| dstRect.y() | = | srcY + dstRect.y() - srcRect.y() |, but this can only happen when | srcY | = | srcRect.y() |
|      1      | = |                1                 |                                |   1  |   |      1      |

This means the top-left corner of the srcRect will be drawn at the top-left corner of dstRect, or:

| srcRect.x() |                  | dstRect.x() |
| srcRect.y() | will be drawn at | dstRect.y() |
|      1      |                  |      1      |

The rest of the points will be mapped the same way such that the srcRect will be drawn in dstRect.

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:163
>> +static const size_t s_maxCachedImageSize = s_maxCachedImageSide * s_maxCachedImageSide * 4;
> 
> Isn't that 64KB, not 64MB?
> 
> Also, maybe we should use Edge rather than Side, because I thought it was a typo :)

It was wrong to use the macro KB in this definition. What I meant was:

1. The maximum memory size for the cachedPDFImage = 64 MB
2. This means the maximum area of the cachedPDFImage = 64M / 4 = 16M pixels
3. If we choose cachedPDFImage to be square, then the maximum length of the cachedPDFImage side = sqrt(16M) = 4K pixels.

I changed the naming and the definition to remove the confusion.

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:224
>> +    // Cache the PDF image only if the size of the new image won't exceed the limit
> 
> Nit: missing .

Fixed.

[WebKit Commit Bot]

Comment on attachment 283826 [details]
Patch

Clearing flags on attachment: 283826

Committed r203378: <http://trac.webkit.org/changeset/203378>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.