Bug 160617 – REGRESSION (r203378): [iOS] The PDF image is rendered stretched if a sub image of it is cached first

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

r203378 made it possible to cache only a sub-image of the PDF first and then draw this sub-image to the destination context. What was left is fixing the source and destination rectangles when drawing from the PDF to the cached image and when drawing from the cached image to the destination context. Mostly the old code, which was using the whole image rectangle for both srcRect and dstRect, were left as it was before r203378.

[Said Abou-Hallawa]

Created attachment 285462 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/27627628>

[Said Abou-Hallawa]

Created attachment 285466 [details]
Patch

[Said Abou-Hallawa]

Created attachment 285472 [details]
The fix without the test case

[Simon Fraser (smfr)]

Comment on attachment 285472 [details]
The fix without the test case

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

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:170
> +    FloatRect rect = context.clipBounds();

This could use a better name; maybe "dirtyRect".

It's not clear why you don't intersect with dstRect up front. Why shift the edges to the edge of the clip bounds, when the clip bounds may be far outside dstRect?

[Said Abou-Hallawa]

Created attachment 285742 [details]
Patch

[Said Abou-Hallawa]

Created attachment 285743 [details]
The fix without the test case 2

[Said Abou-Hallawa]

Comment on attachment 285472 [details]
The fix without the test case

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

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:170
>> +    FloatRect rect = context.clipBounds();
> 
> This could use a better name; maybe "dirtyRect".
> 
> It's not clear why you don't intersect with dstRect up front. Why shift the edges to the edge of the clip bounds, when the clip bounds may be far outside dstRect?

I changed the logic in this function. Please see the new uploaded patches.

[Jon Lee]

Comment on attachment 285742 [details]
Patch

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

> Source/WebCore/ChangeLog:23
> +        rename it cachingPDFImage. Allow "limited" option to be averrable for other

Did you mean "available"?

> Source/WebCore/page/Settings.h:80
> +enum class CachingPDFImage {

I'm not sure about the name here. CachingPDFImagePolicy? PDFImageCachingPolicy? PDFImageCachingMode?

It would probably be worth a comment about expected behavior. Limited and Clipped don't explain enough. And maybe Clipped should be renamed ClippedForTestingOnly.

> Source/WebCore/page/Settings.h:376
> +    CachingPDFImage m_cachingPDFImage { CachingPDFImage::Enabled };

Please rename. The variable sounds like it's an image, not a policy or strategy.

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:105
> +    m_cachingPDFImage = caching;

Is it correct to always destroy the data when this is called, even if m_cachingPDFImage == caching?

[Said Abou-Hallawa]

Created attachment 285878 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 285742 [details]
Patch

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

>> Source/WebCore/ChangeLog:23
>> +        rename it cachingPDFImage. Allow "limited" option to be averrable for other
> 
> Did you mean "available"?

Yes I meant "available" but autocorrect changed it to "averrable".

>> Source/WebCore/page/Settings.h:80
>> +enum class CachingPDFImage {
> 
> I'm not sure about the name here. CachingPDFImagePolicy? PDFImageCachingPolicy? PDFImageCachingMode?
> 
> It would probably be worth a comment about expected behavior. Limited and Clipped don't explain enough. And maybe Clipped should be renamed ClippedForTestingOnly.

Done. I changed it to PDFImageCachingPolicy.

>> Source/WebCore/page/Settings.h:376
>> +    CachingPDFImage m_cachingPDFImage { CachingPDFImage::Enabled };
> 
> Please rename. The variable sounds like it's an image, not a policy or strategy.

Done. But I am not sure about the coding style with a variable whose type starts with capital letters as PDFImageCachingPolicy. I renamed this member to be 

PDFImageCachingPolicy m_pdfImageCachingPolicy { PDFImageCachingPolicy::Enabled };

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:105
>> +    m_cachingPDFImage = caching;
> 
> Is it correct to always destroy the data when this is called, even if m_cachingPDFImage == caching?

Done. I added the following if-statement at the beginning of this function:

if (m_pdfImageCachingPolicy == pdfImageCachingPolicy)
    return;

[Tim Horton]

Comment on attachment 285878 [details]
Patch

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

> Source/WebCore/page/Settings.h:373
> +#if PLATFORM(IOS)

I'm not sure that this should be platform-specific. Or at least not here? You should ask around.

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:251
> +    // We need to transform the coordinates system such that top-left of m_cachedImageRect will be mapped to the

s/coordinates/coordinate/

> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:252
> +    // top-left of dstRect. Although only m_cachedImageRect.size() of the image  copied, the sizes of srcRect

You have two spaces between image and copied.

> Source/WebCore/testing/InternalSettings.cpp:491
> +    if (equalLettersIgnoringASCIICase(policy, "disabled")) {

Not an enum?

[Tim Horton]

(In reply to comment #12)
> Comment on attachment 285878 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285878&action=review
> 
> > Source/WebCore/page/Settings.h:373
> > +#if PLATFORM(IOS)
> 
> I'm not sure that this should be platform-specific. Or at least not here?
> You should ask around.

I do like that you've made the various different modes testable on all platforms, though, that's a big improvement!

[Said Abou-Hallawa]

Created attachment 286734 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 285878 [details]
Patch

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

>>> Source/WebCore/page/Settings.h:373
>>> +#if PLATFORM(IOS)
>> 
>> I'm not sure that this should be platform-specific. Or at least not here? You should ask around.
> 
> I do like that you've made the various different modes testable on all platforms, though, that's a big improvement!

I asked around if we need to apply this fix on other platforms and the response I got was "No we do not need to". The reasons are
1. We are fixing a memory jetsam limitation on iOS which does not exist on other platforms. So why do we apply the same fix for platforms which we have not encountered any problem with in this scenario?
2. We do not have layout tests or benchmarks for rendering large PDF images to know the impact of this fix on other platforms. All the tests and the verifications of the fixes are done manually.
3. By enabling limiting caching the PDF to a certain size and rectangle on other platforms. we may encounter other bugs which do not happen now and hard to reproduce on iOS. So why do we take the risk for that?

I also moved the definition of this member to Settings.in. I added an enum value which is named PDFImageCachingDefault. Its value can be either PDFImageCachingEnabled or PDFImageCachingBelowMemoryLimit. I initialized Setting::m_pdfImageCachingPolicy to PDFImageCachingDefault.

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:251
>> +    // We need to transform the coordinates system such that top-left of m_cachedImageRect will be mapped to the
> 
> s/coordinates/coordinate/

Fixed.

>> Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp:252
>> +    // top-left of dstRect. Although only m_cachedImageRect.size() of the image  copied, the sizes of srcRect
> 
> You have two spaces between image and copied.

Fixed.

[Tim Horton]

Comment on attachment 286734 [details]
Patch

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

> Source/WebCore/testing/InternalSettings.idl:71
> +    [RaisesException] void setPdfImageCachingPolicy(DOMString policy);

This really should be PDF

[Said Abou-Hallawa]

Created attachment 286983 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 286983 [details]
Patch

Clearing flags on attachment: 286983

Committed r204983: <http://trac.webkit.org/changeset/204983>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

Reopening to attach new patch.

[Said Abou-Hallawa]

Created attachment 287045 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #21)
> Created attachment 287045 [details]
> Patch

I uploaded this patch by mistake.