Bug 160617 - REGRESSION (r203378): [iOS] The PDF image is rendered stretched if a sub image of it is cached first
Summary: REGRESSION (r203378): [iOS] The PDF image is rendered stretched if a sub imag...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2016-08-05 16:12 PDT by Said Abou-Hallawa
Modified: 2016-08-25 16:44 PDT (History)
7 users (show)

See Also:


Attachments
Patch (21.85 KB, patch)
2016-08-05 16:45 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (21.64 KB, patch)
2016-08-05 17:04 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
The fix without the test case (3.73 KB, patch)
2016-08-05 18:28 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (21.52 KB, patch)
2016-08-10 12:08 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
The fix without the test case 2 (3.63 KB, patch)
2016-08-10 12:10 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (22.03 KB, patch)
2016-08-11 18:08 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (22.43 KB, patch)
2016-08-23 10:54 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (22.35 KB, patch)
2016-08-25 10:55 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (22.35 KB, patch)
2016-08-25 16:43 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2016-08-05 16:12:29 PDT
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.
Comment 1 Said Abou-Hallawa 2016-08-05 16:45:16 PDT
Created attachment 285462 [details]
Patch
Comment 2 Said Abou-Hallawa 2016-08-05 16:47:10 PDT
<rdar://problem/27627628>
Comment 3 Said Abou-Hallawa 2016-08-05 17:04:21 PDT
Created attachment 285466 [details]
Patch
Comment 4 Said Abou-Hallawa 2016-08-05 18:28:20 PDT
Created attachment 285472 [details]
The fix without the test case
Comment 5 Simon Fraser (smfr) 2016-08-10 09:47:08 PDT
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?
Comment 6 Said Abou-Hallawa 2016-08-10 12:08:02 PDT
Created attachment 285742 [details]
Patch
Comment 7 Said Abou-Hallawa 2016-08-10 12:10:28 PDT
Created attachment 285743 [details]
The fix without the test case 2
Comment 8 Said Abou-Hallawa 2016-08-10 12:11:29 PDT
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.
Comment 9 Jon Lee 2016-08-11 16:53:04 PDT
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?
Comment 10 Said Abou-Hallawa 2016-08-11 18:08:49 PDT
Created attachment 285878 [details]
Patch
Comment 11 Said Abou-Hallawa 2016-08-11 18:16:25 PDT
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;
Comment 12 Tim Horton 2016-08-22 10:41:15 PDT
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?
Comment 13 Tim Horton 2016-08-22 10:41:45 PDT
(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!
Comment 14 Said Abou-Hallawa 2016-08-23 10:54:22 PDT
Created attachment 286734 [details]
Patch
Comment 15 Said Abou-Hallawa 2016-08-23 11:12:37 PDT
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.
Comment 16 Tim Horton 2016-08-24 15:53:30 PDT
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
Comment 17 Said Abou-Hallawa 2016-08-25 10:55:25 PDT
Created attachment 286983 [details]
Patch
Comment 18 WebKit Commit Bot 2016-08-25 12:20:15 PDT
Comment on attachment 286983 [details]
Patch

Clearing flags on attachment: 286983

Committed r204983: <http://trac.webkit.org/changeset/204983>
Comment 19 WebKit Commit Bot 2016-08-25 12:20:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Said Abou-Hallawa 2016-08-25 16:43:35 PDT
Reopening to attach new patch.
Comment 21 Said Abou-Hallawa 2016-08-25 16:43:37 PDT
Created attachment 287045 [details]
Patch
Comment 22 Said Abou-Hallawa 2016-08-25 16:44:42 PDT
(In reply to comment #21)
> Created attachment 287045 [details]
> Patch

I uploaded this patch by mistake.