REGRESSION(r217236): [iOS] PDFDocumentImage does not update its cached ImageBuffer...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 26 Jan 2018 03:42:41 +0000 (03:42 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 26 Jan 2018 03:42:41 +0000 (03:42 +0000)
https://bugs.webkit.org/show_bug.cgi?id=182083

Patch by Said Abou-Hallawa <sabouhallawa@apple.com> on 2018-01-25
Reviewed by Simon Fraser.

Source/WebCore:

Test: fast/images/pdf-as-image-dest-rect-change.html

Revert the change r217236 back. Fix the issue of throwing out the cached
ImageBuffer of the PDF document image when moving its rectangle.

* platform/graphics/cg/PDFDocumentImage.cpp:
(WebCore::PDFDocumentImage::cacheParametersMatch): Return the if-statement
which was deleted in r217236 back but intersect it with dstRect. The context
clipping rectangle can be more than the dstRect.
(WebCore::PDFDocumentImage::updateCachedImageIfNeeded):
-- Remove a wrong optimization which used to work for Mac only if the context
   interpolation quality is not set to low or none quality. This optimization
   does not consider the case when srcRect or destRect change after caching
   the ImageBuffer. Or even if m_cachedImageRect does not include the
   whole clipping rectangle.
-- Move back the call to cacheParametersMatch() before changing the
   m_cachedImageRect.
-- Always intersect the clipping rectangle with the dstRect to ensure we
   only look at the dirty rectangle inside the image boundary.
-- If cacheParametersMatch() returns true, set m_cachedDestinationRect to
   dstRect and move m_cachedImageRect by the difference between the new
   and the old dstRects since no re-caching will happen.
* platform/graphics/cg/PDFDocumentImage.h:
* testing/Internals.cpp:
(WebCore::pdfDocumentImageFromImageElement):
(WebCore::Internals::pdfDocumentCachingCount):
* testing/Internals.h:
* testing/Internals.idl:
Add an internal API which returns the number of drawing the PDF into an
ImageBuffer.

LayoutTests:

PDFDocumentImage renders only on CG platforms. Enable the new test for
iOS only.

* TestExpectations:
* fast/images/pdf-as-image-dest-rect-change-expected.txt: Added.
* fast/images/pdf-as-image-dest-rect-change.html: Added.
* platform/ios/TestExpectations:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@227651 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/TestExpectations
LayoutTests/fast/images/pdf-as-image-dest-rect-change-expected.txt [new file with mode: 0644]
LayoutTests/fast/images/pdf-as-image-dest-rect-change.html [new file with mode: 0644]
LayoutTests/platform/ios/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp
Source/WebCore/platform/graphics/cg/PDFDocumentImage.h
Source/WebCore/testing/Internals.cpp
Source/WebCore/testing/Internals.h
Source/WebCore/testing/Internals.idl

index 94b6949..222a0a0 100644 (file)
@@ -1,3 +1,18 @@
+2018-01-25  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        REGRESSION(r217236): [iOS] PDFDocumentImage does not update its cached ImageBuffer if it has a sub-rectangle of the image
+        https://bugs.webkit.org/show_bug.cgi?id=182083
+
+        Reviewed by Simon Fraser.
+
+        PDFDocumentImage renders only on CG platforms. Enable the new test for
+        iOS only.
+
+        * TestExpectations:
+        * fast/images/pdf-as-image-dest-rect-change-expected.txt: Added.
+        * fast/images/pdf-as-image-dest-rect-change.html: Added.
+        * platform/ios/TestExpectations:
+
 2018-01-25  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/errorhandling.html crashes
index bfe0d30..9ef47a8 100644 (file)
@@ -111,6 +111,7 @@ fast/attachment/attachment-borderless.html [ Skip ]
 editing/selection/character-granularity-selected-range-after-dismissing-selection.html [ Skip ]
 editing/selection/character-granularity-select-text-with-click-handler.html [ Skip ]
 editing/selection/caret-after-tap-in-editable-selection.html [ Skip ]
+fast/images/pdf-as-image-dest-rect-change.html [ Skip ]
 
 # Only iOS has selection UI drawn by UIKit
 editing/selection/character-granularity-rect.html [ Skip ]
diff --git a/LayoutTests/fast/images/pdf-as-image-dest-rect-change-expected.txt b/LayoutTests/fast/images/pdf-as-image-dest-rect-change-expected.txt
new file mode 100644 (file)
index 0000000..0790dd4
--- /dev/null
@@ -0,0 +1,10 @@
+Test the cached ImageBuffer of a PDF docoument image won't be thrown away if the image moves.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS internals.pdfDocumentCachingCount(window.image) is 1
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/images/pdf-as-image-dest-rect-change.html b/LayoutTests/fast/images/pdf-as-image-dest-rect-change.html
new file mode 100644 (file)
index 0000000..682647b
--- /dev/null
@@ -0,0 +1,38 @@
+<head>
+    <style>
+        img {
+            width: 100px;
+            height: 100px;
+            position: absolute;
+            top: 200px;
+            }
+    </style>
+    <script src="../../resources/js-test-pre.js"></script>
+</head>
+<body>
+    <img src="resources/annotation.pdf">
+    <script>
+        description("Test the cached ImageBuffer of a PDF docoument image won't be thrown away if the image moves.");
+        jsTestIsAsync = true;
+
+        function moveImageLoop(image, stepDistance, stepCount) {
+            var step = 0;
+            function loop() {
+                image.style.left = step * stepDistance + 'px';
+                if (++step < stepCount) {
+                    requestAnimationFrame(loop);
+                    return;
+                }
+
+                if (window.internals) {
+                    window.image = image;
+                    shouldBe("internals.pdfDocumentCachingCount(window.image)", "1");
+                }
+                finishJSTest();
+            }
+            loop();
+        }
+        moveImageLoop(document.querySelector("img"), 50, 10);
+    </script>
+    <script src="../../resources/js-test-post.js"></script>
+</body>
index 38fe29e..db62a2c 100644 (file)
@@ -2799,6 +2799,7 @@ media/video-seek-to-current-time.html [ Failure ]
 
 fast/attachment/attachment-wrapping-action.html [ Pass ]
 fast/attachment/attachment-borderless.html [ Pass ]
+fast/images/pdf-as-image-dest-rect-change.html [ Pass ]
 
 fast/events/page-visibility-iframe-move-test.html [ Skip ]
 
index 0a470df..f8872b2 100644 (file)
@@ -1,3 +1,41 @@
+2018-01-25  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        REGRESSION(r217236): [iOS] PDFDocumentImage does not update its cached ImageBuffer if it has a sub-rectangle of the image
+        https://bugs.webkit.org/show_bug.cgi?id=182083
+
+        Reviewed by Simon Fraser.
+
+        Test: fast/images/pdf-as-image-dest-rect-change.html
+
+        Revert the change r217236 back. Fix the issue of throwing out the cached
+        ImageBuffer of the PDF document image when moving its rectangle.
+
+        * platform/graphics/cg/PDFDocumentImage.cpp:
+        (WebCore::PDFDocumentImage::cacheParametersMatch): Return the if-statement
+        which was deleted in r217236 back but intersect it with dstRect. The context
+        clipping rectangle can be more than the dstRect.
+        (WebCore::PDFDocumentImage::updateCachedImageIfNeeded):
+        -- Remove a wrong optimization which used to work for Mac only if the context
+           interpolation quality is not set to low or none quality. This optimization
+           does not consider the case when srcRect or destRect change after caching
+           the ImageBuffer. Or even if m_cachedImageRect does not include the
+           whole clipping rectangle.
+        -- Move back the call to cacheParametersMatch() before changing the
+           m_cachedImageRect.
+        -- Always intersect the clipping rectangle with the dstRect to ensure we
+           only look at the dirty rectangle inside the image boundary.
+        -- If cacheParametersMatch() returns true, set m_cachedDestinationRect to
+           dstRect and move m_cachedImageRect by the difference between the new
+           and the old dstRects since no re-caching will happen.
+        * platform/graphics/cg/PDFDocumentImage.h:
+        * testing/Internals.cpp:
+        (WebCore::pdfDocumentImageFromImageElement):
+        (WebCore::Internals::pdfDocumentCachingCount):
+        * testing/Internals.h:
+        * testing/Internals.idl:
+        Add an internal API which returns the number of drawing the PDF into an
+        ImageBuffer.
+
 2018-01-25  Joseph Pecoraro  <pecoraro@apple.com>
 
         Web Inspector: Remove unnecessary developerExtrasEnabled checks
index ac33971..7e14a2f 100644 (file)
@@ -107,12 +107,24 @@ void PDFDocumentImage::setPdfImageCachingPolicy(PDFImageCachingPolicy pdfImageCa
 
 bool PDFDocumentImage::cacheParametersMatch(GraphicsContext& context, const FloatRect& dstRect, const FloatRect& srcRect) const
 {
-    if (dstRect.size() != m_cachedDestinationSize)
+    // Old and new source rectangles have to match.
+    if (srcRect != m_cachedSourceRect)
         return false;
 
-    if (srcRect != m_cachedSourceRect)
+    // Old and new scaling factors "dest / src" have to match.
+    if (dstRect.size() != m_cachedDestinationRect.size())
         return false;
 
+    // m_cachedImageRect can be moved if srcRect and dstRect.size() did not change.
+    FloatRect movedCachedImageRect = m_cachedImageRect;
+    movedCachedImageRect.move(FloatSize(dstRect.location() - m_cachedDestinationRect.location()));
+
+    // movedCachedImageRect has to contain the whole dirty rectangle.
+    FloatRect dirtyRect = intersection(context.clipBounds(), dstRect);
+    if (!movedCachedImageRect.contains(dirtyRect))
+        return false;
+
+    // Old and new context scaling factors have to match as well.
     AffineTransform::DecomposedType decomposedTransform;
     context.getCTM(GraphicsContext::DefinitelyIncludeDeviceScale).decompose(decomposedTransform);
 
@@ -187,19 +199,14 @@ void PDFDocumentImage::decodedSizeChanged(size_t newCachedBytes)
 
 void PDFDocumentImage::updateCachedImageIfNeeded(GraphicsContext& context, const FloatRect& dstRect, const FloatRect& srcRect)
 {
-#if PLATFORM(IOS)
-    // On iOS, some clients use low-quality image interpolation always, which throws off this optimization,
-    // as we never get the subsequent high-quality paint. Since live resize is rare on iOS, disable the optimization.
-    // FIXME (136593): It's also possible to do the wrong thing here if CSS specifies low-quality interpolation via the "image-rendering"
-    // property, on all platforms. We should only do this optimization if we're actually in a ImageQualityController live resize,
-    // and are guaranteed to do a high-quality paint later.
-    bool repaintIfNecessary = true;
-#else
-    // If we have an existing image, reuse it if we're doing a low-quality paint, even if cache parameters don't match;
-    // we'll rerender when we do the subsequent high-quality paint.
-    InterpolationQuality interpolationQuality = context.imageInterpolationQuality();
-    bool repaintIfNecessary = interpolationQuality != InterpolationNone && interpolationQuality != InterpolationLow;
-#endif
+    // Clipped option is for testing only. Force re-caching the PDF with each draw.
+    bool forceUpdateCachedImage = m_pdfImageCachingPolicy == PDFImageCachingClipBoundsOnly || !m_cachedImageBuffer;
+    if (!forceUpdateCachedImage && cacheParametersMatch(context, dstRect, srcRect)) {
+        // Adjust the view-port rectangles if no re-caching will happen.
+        m_cachedImageRect.move(FloatSize(dstRect.location() - m_cachedDestinationRect.location()));
+        m_cachedDestinationRect = dstRect;
+        return;
+    }
 
     switch (m_pdfImageCachingPolicy) {
     case PDFImageCachingDisabled:
@@ -210,19 +217,13 @@ void PDFDocumentImage::updateCachedImageIfNeeded(GraphicsContext& context, const
         m_cachedImageRect = cachedImageRect(context, dstRect);
         break;
     case PDFImageCachingClipBoundsOnly:
-        m_cachedImageRect = context.clipBounds();
+        m_cachedImageRect = intersection(context.clipBounds(), dstRect);
         break;
     case PDFImageCachingEnabled:
         m_cachedImageRect = dstRect;
         break;
     }
 
-    // Clipped option is for testing only. Force recaching the PDF with each draw.
-    if (m_pdfImageCachingPolicy != PDFImageCachingClipBoundsOnly) {
-        if (m_cachedImageBuffer && (!repaintIfNecessary || cacheParametersMatch(context, dstRect, srcRect)))
-            return;
-    }
-
     FloatSize cachedImageSize = FloatRect(enclosingIntRect(m_cachedImageRect)).size();
 
     // Cache the PDF image only if the size of the new image won't exceed the cache threshold.
@@ -248,8 +249,9 @@ void PDFDocumentImage::updateCachedImageIfNeeded(GraphicsContext& context, const
     drawPDFPage(bufferContext);
 
     m_cachedTransform = context.getCTM(GraphicsContext::DefinitelyIncludeDeviceScale);
-    m_cachedDestinationSize = dstRect.size();
+    m_cachedDestinationRect = dstRect;
     m_cachedSourceRect = srcRect;
+    ++m_cachingCountForTesting;
 
     IntSize internalSize = m_cachedImageBuffer->internalSize();
     decodedSizeChanged(internalSize.unclampedArea() * 4);
index 155bcd7..90d7069 100644 (file)
@@ -59,6 +59,8 @@ public:
     WEBCORE_EXPORT static RetainPtr<CFMutableDataRef> convertPostScriptDataToPDF(RetainPtr<CFDataRef>&& postScriptData);
 #endif
 
+    unsigned cachingCountForTesting() const { return m_cachingCountForTesting; }
+
 private:
     PDFDocumentImage(ImageObserver*);
     virtual ~PDFDocumentImage();
@@ -103,9 +105,10 @@ private:
     std::unique_ptr<ImageBuffer> m_cachedImageBuffer;
     FloatRect m_cachedImageRect;
     AffineTransform m_cachedTransform;
-    FloatSize m_cachedDestinationSize;
+    FloatRect m_cachedDestinationRect;
     FloatRect m_cachedSourceRect;
     size_t m_cachedBytes { 0 };
+    unsigned m_cachingCountForTesting { 0 };
 
     FloatRect m_cropBox;
     int m_rotationDegrees { 0 }; // Can only be 0, 90, 180, or 270 degrees.
index 6dc5355..899c92f 100644 (file)
 #include "MockLibWebRTCPeerConnection.h"
 #include "MockPageOverlay.h"
 #include "MockPageOverlayClient.h"
+#if USE(CG)
+#include "PDFDocumentImage.h"
+#endif
 #include "Page.h"
 #include "PageCache.h"
 #include "PageOverlay.h"
@@ -773,6 +776,14 @@ static BitmapImage* bitmapImageFromImageElement(HTMLImageElement& element)
     return image && is<BitmapImage>(image) ? &downcast<BitmapImage>(*image) : nullptr;
 }
 
+#if USE(CG)
+static PDFDocumentImage* pdfDocumentImageFromImageElement(HTMLImageElement& element)
+{
+    auto* image = imageFromImageElement(element);
+    return image && is<PDFDocumentImage>(image) ? &downcast<PDFDocumentImage>(*image) : nullptr;
+}
+#endif
+
 unsigned Internals::imageFrameIndex(HTMLImageElement& element)
 {
     auto* bitmapImage = bitmapImageFromImageElement(element);
@@ -809,6 +820,17 @@ unsigned Internals::imageDecodeCount(HTMLImageElement& element)
     return bitmapImage ? bitmapImage->decodeCountForTesting() : 0;
 }
 
+unsigned Internals::pdfDocumentCachingCount(HTMLImageElement& element)
+{
+#if USE(CG)
+    auto* pdfDocumentImage = pdfDocumentImageFromImageElement(element);
+    return pdfDocumentImage ? pdfDocumentImage->cachingCountForTesting() : 0;
+#else
+    UNUSED_PARAM(element);
+    return 0;
+#endif
+}
+
 void Internals::setLargeImageAsyncDecodingEnabledForTesting(HTMLImageElement& element, bool enabled)
 {
     if (auto* bitmapImage = bitmapImageFromImageElement(element))
index c909c50..ce29958 100644 (file)
@@ -139,6 +139,7 @@ public:
     bool isImageAnimating(HTMLImageElement&);
     void setClearDecoderAfterAsyncFrameRequestForTesting(HTMLImageElement&, bool enabled);
     unsigned imageDecodeCount(HTMLImageElement&);
+    unsigned pdfDocumentCachingCount(HTMLImageElement&);
     void setLargeImageAsyncDecodingEnabledForTesting(HTMLImageElement&, bool enabled);
 
     void setGridMaxTracksLimit(unsigned);
index 33519c4..1aeafb1 100644 (file)
@@ -262,6 +262,7 @@ enum EventThrottlingBehavior {
     boolean isImageAnimating(HTMLImageElement element);
     void setClearDecoderAfterAsyncFrameRequestForTesting(HTMLImageElement element, boolean enabled);
     unsigned long imageDecodeCount(HTMLImageElement element);
+    unsigned long pdfDocumentCachingCount(HTMLImageElement element);
     void setLargeImageAsyncDecodingEnabledForTesting(HTMLImageElement element, boolean enabled);
 
     void setGridMaxTracksLimit(unsigned long maxTracksLimit);