Bug 182083 – REGRESSION(r217236): [iOS] PDFDocumentImage does not update its cached ImageBuffer if it has a sub-rectangle of the image

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

The PDFKit currently does not respect the context clipping rectangle. That means to draw a sub-recatnge of the PDF document image, the whole PDF document has to be rendered. This why the PDFDocumentImage draws the PDF document in an ImageBuffer first and then draws this ImageBuffer to the context. The ImageBuffer is cached and reused as long as the scaling factor does not change.


In <http://trac.webkit.org/changeset/203378>, the PDFDocumentImage cached ImageBuffer was bounded to a certain limit (64MB) to avoid a jetsam from the system. The check for keeping the cached ImageBuffer was extended to include the fact that the cached ImageBuffer includes only a sub-rectangle from the PDF document and not all of it. In PDFDocumentImage::cacheParametersMatch(), the condition below was added:

boo PDFDocumentImage::cacheParametersMatch()
{
>>    if (!m_cachedImageRect.contains(context.clipBounds()))
>>        return false;
}

m_cachedImageRect is in context coordinates and we check if the whole clipping rectangle is inside the drawing of the cached ImageBuffer. This condition is valid if the page is a standalone PDF image document. But if the image is inside an HTML page, this condition will be too restricted if the clipping rectangle includes more than the image rectangle even if the whole image drawing is cached in the ImageBuffer.

In <https://trac.webkit.org/changeset/224719>, this condition was removed to fix the issue of throwing the cached ImageBuffer needlessly because of moving the image. But removing this condition causes not updating the cached ImageBuffer when it has a sub-rectangle of the image and the new clipping rectangle is not the same as the one the ImageBuffer was cached for.

[Said Abou-Hallawa]

Created attachment 332220 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/35761369>

[Said Abou-Hallawa]

Created attachment 332221 [details]
Patch

[EWS Watchlist]

Comment on attachment 332221 [details]
Patch

Attachment 332221 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6203135

New failing tests:
fast/images/pdf-as-image-dest-rect-change.html

[EWS Watchlist]

Created attachment 332227 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332221 [details]
Patch

Attachment 332221 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6203319

New failing tests:
fast/images/pdf-as-image-dest-rect-change.html

[EWS Watchlist]

Created attachment 332229 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332221 [details]
Patch

Attachment 332221 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6203531

New failing tests:
fast/images/pdf-as-image-dest-rect-change.html

[EWS Watchlist]

Created attachment 332230 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Said Abou-Hallawa]

Created attachment 332276 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 332276 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:116
> +    if (dstRect != m_cachedImageRect && !m_cachedImageRect.contains(context.clipBounds()))

Are m_cachedImageRect and context.clipBounds() really in the same coordinate system?

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:207
> +    // Clipped option is for testing only. Force recaching the PDF with each draw.

Maybe re-caching.

If the testing-only comment is accurate, the option should have "ForTesting" in the name.

[Said Abou-Hallawa]

Created attachment 332331 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 332276 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:116
>> +    if (dstRect != m_cachedImageRect && !m_cachedImageRect.contains(context.clipBounds()))
> 
> Are m_cachedImageRect and context.clipBounds() really in the same coordinate system?

Yes m_cachedImageRect and context.clipBounds() are in viewport coordinates.

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:207
>> +    // Clipped option is for testing only. Force recaching the PDF with each draw.
> 
> Maybe re-caching.
> 
> If the testing-only comment is accurate, the option should have "ForTesting" in the name.

Yes it is for testing only. But PDFImageCachingDisabled is also for testing. Should both be prefixed with "ForTesting"?

[Simon Fraser (smfr)]

Comment on attachment 332331 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Revert the change r217236 back and fix the issue of throwing the cached

"throwing"? Do you mean throwing out?

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:122
> +    // movedCachedImageRect has to inlcude the dirty rectangle.

inlcude -> include

[Said Abou-Hallawa]

Created attachment 332343 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 332343 [details]:

http/tests/xmlhttprequest/redirect-cross-origin-tripmine.html bug 179840 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 332343 [details]
Patch

Clearing flags on attachment: 332343

Committed r227651: <https://trac.webkit.org/changeset/227651>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.