Do not delete asynchronously decoded frames for large images if their clients are...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 16 May 2017 05:14:50 +0000 (05:14 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 16 May 2017 05:14:50 +0000 (05:14 +0000)
https://bugs.webkit.org/show_bug.cgi?id=170640

Patch by Said Abou-Hallawa <sabouhallawa@apple.com> on 2017-05-15
Reviewed by Simon Fraser.

Source/WebCore:

The image flickering problem happens when a large image is visible in the
view port and for some reason, the decoded frame gets destroyed. When this
image is repainted, BitmapImage::draw() does not find a valid decoded frame
for that image. It then requests an async decoding for the image and just
draws nothing in the image rectangle. Drawing no content between two drawing
phases in which the image is drawn causes the unwanted flickering.

To fix this issue we need to protect the decoded frames of all the images
in the view port from being destroyed. When BitmapImage::destroyDecodedData()
is called, it is going to check, through the ImageObserver, whether any
of its clients is visible. And if so, the current decoded frame won't be
destroyed.

Tests: Modifying existing tests.

* loader/cache/CachedImage.cpp:
(WebCore::CachedImage::CachedImageObserver::decodedSizeChanged):
(WebCore::CachedImage::CachedImageObserver::didDraw):
(WebCore::CachedImage::CachedImageObserver::canDestroyDecodedData):
(WebCore::CachedImage::CachedImageObserver::imageFrameAvailable):
(WebCore::CachedImage::CachedImageObserver::changedInRect):
(WebCore::CachedImage::decodedSizeChanged):
(WebCore::CachedImage::didDraw):
(WebCore::CachedImage::canDestroyDecodedData): Finds out whether it's okay
to discard the image decoded data or not.
(WebCore::CachedImage::imageFrameAvailable):
(WebCore::CachedImage::changedInRect):
* loader/cache/CachedImage.h:
* loader/cache/CachedImageClient.h:
(WebCore::CachedImageClient::canDestroyDecodedData):
* loader/cache/MemoryCache.cpp:
(WebCore::MemoryCache::destroyDecodedDataForAllImages): This function is
currently not used. Use in the internal destroyDecodedDataForAllImages()
but unlike what CachedImage::destroyDecodedData() does, make it destroy
the decoded frames without deleting the image itself.
* loader/cache/MemoryCache.h:
* platform/graphics/BitmapImage.cpp:
(WebCore::BitmapImage::destroyDecodedData):
(WebCore::BitmapImage::draw):
(WebCore::BitmapImage::canDestroyCurrentFrameDecodedData):
(WebCore::BitmapImage::advanceAnimation):
(WebCore::BitmapImage::internalAdvanceAnimation):
(WebCore::BitmapImage::imageFrameAvailableAtIndex):
* platform/graphics/BitmapImage.h:
* platform/graphics/GraphicsContext3D.cpp:
(WebCore::GraphicsContext3D::packImageData):
* platform/graphics/ImageFrameCache.cpp:
(WebCore::ImageFrameCache::decodedSizeChanged):
(ImageFrameCache::cacheAsyncFrameNativeImageAtIndex): The assertion in this
function is wrong. frameIsCompleteAtIndex() can be false when the an image
decoding is requested but can be true when the decoding finishes.
* platform/graphics/ImageObserver.h:
* platform/graphics/cairo/ImageCairo.cpp:
(WebCore::Image::drawPattern):
* platform/graphics/cg/ImageCG.cpp:
(WebCore::Image::drawPattern):
* platform/graphics/cg/ImageDecoderCG.cpp:
(WebCore::ImageDecoder::frameIsCompleteAtIndex):
* platform/graphics/cg/PDFDocumentImage.cpp:
(WebCore::PDFDocumentImage::decodedSizeChanged):
(WebCore::PDFDocumentImage::draw):
* platform/graphics/texmap/TextureMapperTiledBackingStore.cpp:
(WebCore::TextureMapperTiledBackingStore::updateContentsFromImageIfNeeded):
* platform/graphics/win/ImageDirect2D.cpp:
(WebCore::Image::drawPattern):
* rendering/RenderElement.cpp:
(WebCore::RenderElement::isVisibleInDocumentRect):
(WebCore::RenderElement::isVisibleInViewport):
(WebCore::RenderElement::imageFrameAvailable):
(WebCore::RenderElement::repaintForPausedImageAnimationsIfNeeded):
(WebCore::RenderElement::shouldRepaintInVisibleRect): Deleted. Function
is renamed to isVisibleInViewport() for better readability.
* rendering/RenderElement.h:
* svg/graphics/SVGImage.cpp:
(WebCore::SVGImage::draw):
* svg/graphics/SVGImageClients.h:
* testing/Internals.cpp:
(WebCore::Internals::destroyDecodedDataForAllImages):
* testing/Internals.h:
* testing/Internals.idl:

Source/WebKit/mac:

Enable the async decoding for large images.

* WebView/WebView.mm:
(-[WebView _preferencesChanged:]):

Source/WebKit2:

Enable the async decoding for large images.

* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::updatePreferences):

LayoutTests:

* fast/images/async-image-background-image-repeated.html:
* fast/images/async-image-background-image.html:
* fast/images/sprite-sheet-image-draw.html:

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

32 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/images/async-image-background-image-repeated.html
LayoutTests/fast/images/async-image-background-image.html
LayoutTests/fast/images/sprite-sheet-image-draw.html
Source/WebCore/ChangeLog
Source/WebCore/loader/cache/CachedImage.cpp
Source/WebCore/loader/cache/CachedImage.h
Source/WebCore/loader/cache/CachedImageClient.h
Source/WebCore/loader/cache/MemoryCache.cpp
Source/WebCore/loader/cache/MemoryCache.h
Source/WebCore/platform/graphics/BitmapImage.cpp
Source/WebCore/platform/graphics/BitmapImage.h
Source/WebCore/platform/graphics/GraphicsContext3D.cpp
Source/WebCore/platform/graphics/ImageFrameCache.cpp
Source/WebCore/platform/graphics/ImageObserver.h
Source/WebCore/platform/graphics/cairo/ImageCairo.cpp
Source/WebCore/platform/graphics/cg/ImageCG.cpp
Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp
Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp
Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.cpp
Source/WebCore/platform/graphics/win/ImageDirect2D.cpp
Source/WebCore/rendering/RenderElement.cpp
Source/WebCore/rendering/RenderElement.h
Source/WebCore/svg/graphics/SVGImage.cpp
Source/WebCore/svg/graphics/SVGImageClients.h
Source/WebCore/testing/Internals.cpp
Source/WebCore/testing/Internals.h
Source/WebCore/testing/Internals.idl
Source/WebKit/mac/ChangeLog
Source/WebKit/mac/WebView/WebView.mm
Source/WebKit2/ChangeLog
Source/WebKit2/WebProcess/WebPage/WebPage.cpp

index aa459a5..edd05f7 100644 (file)
@@ -1,3 +1,14 @@
+2017-05-15  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Do not delete asynchronously decoded frames for large images if their clients are in the viewport
+        https://bugs.webkit.org/show_bug.cgi?id=170640
+
+        Reviewed by Simon Fraser.
+
+        * fast/images/async-image-background-image-repeated.html:
+        * fast/images/async-image-background-image.html:
+        * fast/images/sprite-sheet-image-draw.html:
+
 2017-05-15  Youenn Fablet  <youenn@apple.com>
 
         WebRTC outgoing muted video sources should send black frames
index b2eb63a..6f069c5 100644 (file)
@@ -57,6 +57,9 @@
                         promises.push(setElementImageBackground(elements[index], image));
  
                     Promise.all(promises).then(() => {
+                        // Ensure internals.destroyDecodedDataForAllImages() will not destroy
+                        // the images' decoded data because it is inside the viewport.
+                        internals.destroyDecodedDataForAllImages();
                         testRunner.notifyDone();
                     });
                 } else {
index 9f58fbd..1246b30 100644 (file)
@@ -44,6 +44,9 @@
  
                     // Wait for the image frame to finish decoding before finishing the test.
                     element.addEventListener("webkitImageFrameReady", function() {
+                        // Ensure internals.destroyDecodedDataForAllImages() will not destroy
+                        // the image's decoded data because it is inside the viewport.
+                        internals.destroyDecodedDataForAllImages();
                         testRunner.notifyDone();
                     }, false);
                 }
index 5bc9c8b..bda44fa 100644 (file)
@@ -37,6 +37,9 @@
  
                     // Wait for the image frame to finish decoding before finishing the test.
                     element.addEventListener("webkitImageFrameReady", function() {
+                        // Ensure internals.destroyDecodedDataForAllImages() will not destroy
+                        // the image's decoded data because it is inside the viewport.
+                        internals.destroyDecodedDataForAllImages();
                         testRunner.notifyDone();
                     }, false);
                 }
index ba3e76f..1182ea2 100644 (file)
@@ -1,3 +1,91 @@
+2017-05-15  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Do not delete asynchronously decoded frames for large images if their clients are in the viewport
+        https://bugs.webkit.org/show_bug.cgi?id=170640
+
+        Reviewed by Simon Fraser.
+
+        The image flickering problem happens when a large image is visible in the
+        view port and for some reason, the decoded frame gets destroyed. When this
+        image is repainted, BitmapImage::draw() does not find a valid decoded frame
+        for that image. It then requests an async decoding for the image and just
+        draws nothing in the image rectangle. Drawing no content between two drawing
+        phases in which the image is drawn causes the unwanted flickering.
+
+        To fix this issue we need to protect the decoded frames of all the images
+        in the view port from being destroyed. When BitmapImage::destroyDecodedData()
+        is called, it is going to check, through the ImageObserver, whether any
+        of its clients is visible. And if so, the current decoded frame won't be
+        destroyed.
+
+        Tests: Modifying existing tests.
+
+        * loader/cache/CachedImage.cpp:
+        (WebCore::CachedImage::CachedImageObserver::decodedSizeChanged):
+        (WebCore::CachedImage::CachedImageObserver::didDraw):
+        (WebCore::CachedImage::CachedImageObserver::canDestroyDecodedData):
+        (WebCore::CachedImage::CachedImageObserver::imageFrameAvailable):
+        (WebCore::CachedImage::CachedImageObserver::changedInRect):
+        (WebCore::CachedImage::decodedSizeChanged):
+        (WebCore::CachedImage::didDraw):
+        (WebCore::CachedImage::canDestroyDecodedData): Finds out whether it's okay
+        to discard the image decoded data or not.
+        (WebCore::CachedImage::imageFrameAvailable):
+        (WebCore::CachedImage::changedInRect):
+        * loader/cache/CachedImage.h:
+        * loader/cache/CachedImageClient.h:
+        (WebCore::CachedImageClient::canDestroyDecodedData):
+        * loader/cache/MemoryCache.cpp:
+        (WebCore::MemoryCache::destroyDecodedDataForAllImages): This function is
+        currently not used. Use in the internal destroyDecodedDataForAllImages()
+        but unlike what CachedImage::destroyDecodedData() does, make it destroy
+        the decoded frames without deleting the image itself.
+        * loader/cache/MemoryCache.h:
+        * platform/graphics/BitmapImage.cpp:
+        (WebCore::BitmapImage::destroyDecodedData):
+        (WebCore::BitmapImage::draw):
+        (WebCore::BitmapImage::canDestroyCurrentFrameDecodedData): 
+        (WebCore::BitmapImage::advanceAnimation):
+        (WebCore::BitmapImage::internalAdvanceAnimation):
+        (WebCore::BitmapImage::imageFrameAvailableAtIndex):
+        * platform/graphics/BitmapImage.h:
+        * platform/graphics/GraphicsContext3D.cpp:
+        (WebCore::GraphicsContext3D::packImageData):
+        * platform/graphics/ImageFrameCache.cpp:
+        (WebCore::ImageFrameCache::decodedSizeChanged):
+        (ImageFrameCache::cacheAsyncFrameNativeImageAtIndex): The assertion in this
+        function is wrong. frameIsCompleteAtIndex() can be false when the an image
+        decoding is requested but can be true when the decoding finishes.
+        * platform/graphics/ImageObserver.h:
+        * platform/graphics/cairo/ImageCairo.cpp:
+        (WebCore::Image::drawPattern):
+        * platform/graphics/cg/ImageCG.cpp:
+        (WebCore::Image::drawPattern):
+        * platform/graphics/cg/ImageDecoderCG.cpp:
+        (WebCore::ImageDecoder::frameIsCompleteAtIndex):
+        * platform/graphics/cg/PDFDocumentImage.cpp:
+        (WebCore::PDFDocumentImage::decodedSizeChanged):
+        (WebCore::PDFDocumentImage::draw):
+        * platform/graphics/texmap/TextureMapperTiledBackingStore.cpp:
+        (WebCore::TextureMapperTiledBackingStore::updateContentsFromImageIfNeeded):
+        * platform/graphics/win/ImageDirect2D.cpp:
+        (WebCore::Image::drawPattern):
+        * rendering/RenderElement.cpp:
+        (WebCore::RenderElement::isVisibleInDocumentRect):
+        (WebCore::RenderElement::isVisibleInViewport):
+        (WebCore::RenderElement::imageFrameAvailable):
+        (WebCore::RenderElement::repaintForPausedImageAnimationsIfNeeded):
+        (WebCore::RenderElement::shouldRepaintInVisibleRect): Deleted. Function
+        is renamed to isVisibleInViewport() for better readability.
+        * rendering/RenderElement.h:
+        * svg/graphics/SVGImage.cpp:
+        (WebCore::SVGImage::draw):
+        * svg/graphics/SVGImageClients.h:
+        * testing/Internals.cpp:
+        (WebCore::Internals::destroyDecodedDataForAllImages):
+        * testing/Internals.h:
+        * testing/Internals.idl:
+
 2017-05-15  Youenn Fablet  <youenn@apple.com>
 
         Simplify RealtimeMediaSource data production and state
index 79b8086..e860d14 100644 (file)
@@ -330,25 +330,36 @@ CachedImage::CachedImageObserver::CachedImageObserver(CachedImage& image)
     m_cachedImages.append(&image);
 }
 
-void CachedImage::CachedImageObserver::decodedSizeChanged(const Image* image, long long delta)
+void CachedImage::CachedImageObserver::decodedSizeChanged(const Image& image, long long delta)
 {
     for (auto cachedImage : m_cachedImages)
         cachedImage->decodedSizeChanged(image, delta);
 }
 
-void CachedImage::CachedImageObserver::didDraw(const Image* image)
+void CachedImage::CachedImageObserver::didDraw(const Image& image)
 {
     for (auto cachedImage : m_cachedImages)
         cachedImage->didDraw(image);
 }
 
-void CachedImage::CachedImageObserver::imageFrameAvailable(const Image* image, ImageAnimatingState animatingState, const IntRect* changeRect)
+bool CachedImage::CachedImageObserver::canDestroyDecodedData(const Image& image)
+{
+    for (auto cachedImage : m_cachedImages) {
+        if (&image != cachedImage->image())
+            continue;
+        if (!cachedImage->canDestroyDecodedData(image))
+            return false;
+    }
+    return true;
+}
+
+void CachedImage::CachedImageObserver::imageFrameAvailable(const Image& image, ImageAnimatingState animatingState, const IntRect* changeRect)
 {
     for (auto cachedImage : m_cachedImages)
         cachedImage->imageFrameAvailable(image, animatingState, changeRect);
 }
 
-void CachedImage::CachedImageObserver::changedInRect(const Image* image, const IntRect* rect)
+void CachedImage::CachedImageObserver::changedInRect(const Image& image, const IntRect* rect)
 {
     for (auto cachedImage : m_cachedImages)
         cachedImage->changedInRect(image, rect);
@@ -476,18 +487,18 @@ void CachedImage::destroyDecodedData()
         m_image->destroyDecodedData();
 }
 
-void CachedImage::decodedSizeChanged(const Image* image, long long delta)
+void CachedImage::decodedSizeChanged(const Image& image, long long delta)
 {
-    if (!image || image != m_image)
+    if (&image != m_image)
         return;
 
     ASSERT(delta >= 0 || decodedSize() + delta >= 0);
     setDecodedSize(static_cast<unsigned>(decodedSize() + delta));
 }
 
-void CachedImage::didDraw(const Image* image)
+void CachedImage::didDraw(const Image& image)
 {
-    if (!image || image != m_image)
+    if (&image != m_image)
         return;
     
     double timeStamp = FrameView::currentPaintTimeStamp();
@@ -497,9 +508,23 @@ void CachedImage::didDraw(const Image* image)
     CachedResource::didAccessDecodedData(timeStamp);
 }
 
-void CachedImage::imageFrameAvailable(const Image* image, ImageAnimatingState animatingState, const IntRect* changeRect)
+bool CachedImage::canDestroyDecodedData(const Image& image)
+{
+    if (&image != m_image)
+        return false;
+
+    CachedResourceClientWalker<CachedImageClient> clientWalker(m_clients);
+    while (CachedImageClient* client = clientWalker.next()) {
+        if (!client->canDestroyDecodedData())
+            return false;
+    }
+
+    return true;
+}
+
+void CachedImage::imageFrameAvailable(const Image& image, ImageAnimatingState animatingState, const IntRect* changeRect)
 {
-    if (!image || image != m_image)
+    if (&image != m_image)
         return;
 
     CachedResourceClientWalker<CachedImageClient> clientWalker(m_clients);
@@ -514,9 +539,9 @@ void CachedImage::imageFrameAvailable(const Image* image, ImageAnimatingState an
         m_image->stopAnimation();
 }
 
-void CachedImage::changedInRect(const Image* image, const IntRect* rect)
+void CachedImage::changedInRect(const Image& image, const IntRect* rect)
 {
-    if (!image || image != m_image)
+    if (&image != m_image)
         return;
     notifyObservers(rect);
 }
index 4ba9373..e0c6f60 100644 (file)
@@ -127,20 +127,22 @@ private:
         explicit CachedImageObserver(CachedImage&);
 
         // ImageObserver API
-        URL sourceUrl() const override { return m_cachedImages[0]->url(); }
-        void decodedSizeChanged(const Image*, long long delta) final;
-        void didDraw(const Image*) final;
+        URL sourceUrl() const override { return !m_cachedImages.isEmpty() ? m_cachedImages[0]->url() : URL(); }
+        void decodedSizeChanged(const Image&, long long delta) final;
+        void didDraw(const Image&) final;
 
-        void imageFrameAvailable(const Image*, ImageAnimatingState, const IntRect* changeRect = nullptr) final;
-        void changedInRect(const Image*, const IntRect*) final;
+        bool canDestroyDecodedData(const Image&) final;
+        void imageFrameAvailable(const Image&, ImageAnimatingState, const IntRect* changeRect = nullptr) final;
+        void changedInRect(const Image&, const IntRect*) final;
 
         Vector<CachedImage*> m_cachedImages;
     };
 
-    void decodedSizeChanged(const Image*, long long delta);
-    void didDraw(const Image*);
-    void imageFrameAvailable(const Image*, ImageAnimatingState, const IntRect* changeRect = nullptr);
-    void changedInRect(const Image*, const IntRect*);
+    void decodedSizeChanged(const Image&, long long delta);
+    void didDraw(const Image&);
+    bool canDestroyDecodedData(const Image&);
+    void imageFrameAvailable(const Image&, ImageAnimatingState, const IntRect* changeRect = nullptr);
+    void changedInRect(const Image&, const IntRect*);
 
     void addIncrementalDataBuffer(SharedBuffer&);
 
index 955ac22..4be8ee4 100644 (file)
@@ -42,7 +42,9 @@ public:
     // If not null, the IntRect is the changed rect of the image.
     virtual void imageChanged(CachedImage*, const IntRect* = nullptr) { }
 
-    // Called when GIF animation progresses.
+    virtual bool canDestroyDecodedData() { return true; }
+
+    // Called when a new decoded frame for a large image is available or when an animated image is ready to advance to the next frame.
     virtual VisibleInViewportState imageFrameAvailable(CachedImage& image, ImageAnimatingState, const IntRect* changeRect) { imageChanged(&image, changeRect); return VisibleInViewportState::No; }
 
     virtual void didRemoveCachedImageClient(CachedImage&) { }
index bc5c575..2d407b7 100644 (file)
@@ -289,8 +289,11 @@ void MemoryCache::forEachSessionResource(SessionID sessionID, const std::functio
 void MemoryCache::destroyDecodedDataForAllImages()
 {
     MemoryCache::singleton().forEachResource([](CachedResource& resource) {
-        if (resource.isImage())
-            resource.destroyDecodedData();
+        if (!resource.isImage())
+            return;
+
+        if (auto image = downcast<CachedImage>(resource).image())
+            image->destroyDecodedData();
     });
 }
 
index 7c2e8c7..fbc9881 100644 (file)
@@ -104,7 +104,7 @@ public:
 
     void forEachResource(const std::function<void(CachedResource&)>&);
     void forEachSessionResource(SessionID, const std::function<void(CachedResource&)>&);
-    void destroyDecodedDataForAllImages();
+    WEBCORE_EXPORT void destroyDecodedDataForAllImages();
 
     // Sets the cache's memory capacities, in bytes. These will hold only approximately,
     // since the decoded cost of resources like scripts and stylesheets is not known.
index 3d4a521..40dc438 100644 (file)
@@ -79,16 +79,17 @@ void BitmapImage::destroyDecodedData(bool destroyAll)
 
     if (!destroyAll)
         m_source.destroyDecodedDataBeforeFrame(m_currentFrame);
-    else if (m_source.hasAsyncDecodingQueue())
+    else if (!canDestroyDecodedData()) {
         m_source.destroyAllDecodedDataExcludeFrame(m_currentFrame);
-    else {
+        destroyAll = false;
+    } else {
         m_source.destroyAllDecodedData();
         m_currentFrameDecodingStatus = ImageFrame::DecodingStatus::Invalid;
     }
 
     // There's no need to throw away the decoder unless we're explicitly asked
     // to destroy all of the frames.
-    if (!destroyAll || m_source.hasAsyncDecodingQueue())
+    if (!destroyAll)
         m_source.clearFrameBufferCache(m_currentFrame);
     else
         m_source.clear(data());
@@ -246,7 +247,7 @@ void BitmapImage::draw(GraphicsContext& context, const FloatRect& destRect, cons
     m_currentFrameDecodingStatus = frameDecodingStatusAtIndex(m_currentFrame);
 
     if (imageObserver())
-        imageObserver()->didDraw(this);
+        imageObserver()->didDraw(*this);
 }
 
 void BitmapImage::drawPattern(GraphicsContext& ctxt, const FloatRect& destRect, const FloatRect& tileRect, const AffineTransform& transform, const FloatPoint& phase, const FloatSize& spacing, CompositeOperator op, BlendMode blendMode)
@@ -315,6 +316,19 @@ void BitmapImage::startTimer(Seconds delay)
     m_frameTimer->startOneShot(delay);
 }
 
+bool BitmapImage::canDestroyDecodedData()
+{
+    // Animated images should preserve the current frame till the next one finishes decoding.
+    if (m_source.hasAsyncDecodingQueue())
+        return false;
+
+    // Small image should be decoded synchronously. Deleting its decoded frame is fine.
+    if (!shouldUseAsyncDecodingForLargeImages())
+        return true;
+
+    return !imageObserver() || imageObserver()->canDestroyDecodedData(*this);
+}
+
 BitmapImage::StartAnimationStatus BitmapImage::internalStartAnimation()
 {
     if (!canAnimate())
@@ -408,7 +422,7 @@ void BitmapImage::advanceAnimation()
     else {
         // Force repaint if showDebugBackground() is on.
         if (m_showDebugBackground)
-            imageObserver()->changedInRect(this);
+            imageObserver()->changedInRect(*this);
         LOG(Images, "BitmapImage::%s - %p - url: %s [lateFrameCount = %ld nextFrame = %ld]", __FUNCTION__, this, sourceURL().string().utf8().data(), ++m_lateFrameCount, nextFrame);
     }
 }
@@ -423,7 +437,7 @@ void BitmapImage::internalAdvanceAnimation()
     if (m_currentFrameDecodingStatus == ImageFrame::DecodingStatus::Decoding)
         m_currentFrameDecodingStatus = frameDecodingStatusAtIndex(m_currentFrame);
     if (imageObserver())
-        imageObserver()->imageFrameAvailable(this, ImageAnimatingState::Yes);
+        imageObserver()->imageFrameAvailable(*this, ImageAnimatingState::Yes);
 
     LOG(Images, "BitmapImage::%s - %p - url: %s [m_currentFrame = %ld]", __FUNCTION__, this, sourceURL().string().utf8().data(), m_currentFrame);
 }
@@ -474,7 +488,7 @@ void BitmapImage::imageFrameAvailableAtIndex(size_t index)
         if (m_currentFrameDecodingStatus == ImageFrame::DecodingStatus::Decoding)
             m_currentFrameDecodingStatus = frameDecodingStatusAtIndex(m_currentFrame);
         if (imageObserver())
-            imageObserver()->imageFrameAvailable(this, ImageAnimatingState::No);
+            imageObserver()->imageFrameAvailable(*this, ImageAnimatingState::No);
     }
 }
 
index 5ee6970..7030173 100644 (file)
@@ -191,6 +191,7 @@ protected:
 private:
     void clearTimer();
     void startTimer(Seconds delay);
+    bool canDestroyDecodedData();
     bool isBitmapImage() const override { return true; }
     void dump(TextStream&) const override;
 
index a850b85..4e757a0 100644 (file)
@@ -368,7 +368,7 @@ GraphicsContext3D::ImageExtractor::ImageExtractor(Image* image, ImageHtmlDomSour
 
 bool GraphicsContext3D::packImageData(Image* image, const void* pixels, GC3Denum format, GC3Denum type, bool flipY, AlphaOp alphaOp, DataFormat sourceFormat, unsigned width, unsigned height, unsigned sourceUnpackAlignment, Vector<uint8_t>& data)
 {
-    if (!pixels)
+    if (!image || !pixels)
         return false;
 
     unsigned packedSize;
@@ -380,7 +380,7 @@ bool GraphicsContext3D::packImageData(Image* image, const void* pixels, GC3Denum
     if (!packPixels(reinterpret_cast<const uint8_t*>(pixels), sourceFormat, width, height, sourceUnpackAlignment, format, type, alphaOp, data.data(), flipY))
         return false;
     if (ImageObserver* observer = image->imageObserver())
-        observer->didDraw(image);
+        observer->didDraw(*image);
     return true;
 }
 
index cd8a4b4..be24ba8 100644 (file)
@@ -124,7 +124,7 @@ void ImageFrameCache::decodedSizeChanged(long long decodedSize)
     if (!decodedSize || !m_image || !m_image->imageObserver())
         return;
     
-    m_image->imageObserver()->decodedSizeChanged(m_image, decodedSize);
+    m_image->imageObserver()->decodedSizeChanged(*m_image, decodedSize);
 }
 
 void ImageFrameCache::decodedSizeIncreased(unsigned decodedSize)
index 476a9ab..80062ca 100644 (file)
@@ -41,12 +41,13 @@ protected:
     virtual ~ImageObserver() {}
 public:
     virtual URL sourceUrl() const = 0;
-    virtual void decodedSizeChanged(const Image*, long long delta) = 0;
+    virtual void decodedSizeChanged(const Image&, long long delta) = 0;
 
-    virtual void didDraw(const Image*) = 0;
+    virtual void didDraw(const Image&) = 0;
 
-    virtual void imageFrameAvailable(const Image*, ImageAnimatingState, const IntRect* changeRect = nullptr) = 0;
-    virtual void changedInRect(const Image*, const IntRect* changeRect = nullptr) = 0;
+    virtual bool canDestroyDecodedData(const Image&) = 0;
+    virtual void imageFrameAvailable(const Image&, ImageAnimatingState, const IntRect* changeRect = nullptr) = 0;
+    virtual void changedInRect(const Image&, const IntRect* changeRect = nullptr) = 0;
 };
 
 }
index 4e53659..9733765 100644 (file)
@@ -43,7 +43,7 @@ void Image::drawPattern(GraphicsContext& context, const FloatRect& destRect, con
     context.drawPattern(*this, destRect, tileRect, patternTransform, phase, spacing, op, blendMode);
 
     if (imageObserver())
-        imageObserver()->didDraw(this);
+        imageObserver()->didDraw(*this);
 }
 
 } // namespace WebCore
index e5b3dea..e070c41 100644 (file)
@@ -43,7 +43,7 @@ void Image::drawPattern(GraphicsContext& ctxt, const FloatRect& destRect, const
     ctxt.drawPattern(*this, destRect, tileRect, patternTransform, phase, spacing, op, blendMode);
 
     if (imageObserver())
-        imageObserver()->didDraw(this);
+        imageObserver()->didDraw(*this);
 }
 
 }
index f2be802..b2c3c14 100644 (file)
@@ -307,6 +307,12 @@ IntSize ImageDecoder::frameSizeAtIndex(size_t index, SubsamplingLevel subsamplin
 bool ImageDecoder::frameIsCompleteAtIndex(size_t index) const
 {
     ASSERT(frameCount());
+    // CGImageSourceGetStatusAtIndex() changes the return status value from kCGImageStatusIncomplete
+    // to kCGImageStatusComplete only if (index > 1 && index < frameCount() - 1). To get an accurate
+    // result for the last frame (or the single frame of the static image) use CGImageSourceGetStatus()
+    // instead for this frame.
+    if (index == frameCount() - 1)
+        return CGImageSourceGetStatus(m_nativeDecoder.get()) == kCGImageStatusComplete;
     return CGImageSourceGetStatusAtIndex(m_nativeDecoder.get(), index) == kCGImageStatusComplete;
 }
 
index 0e73e60..96d1800 100644 (file)
@@ -183,7 +183,7 @@ void PDFDocumentImage::decodedSizeChanged(size_t newCachedBytes)
         return;
 
     if (imageObserver())
-        imageObserver()->decodedSizeChanged(this, -static_cast<long long>(m_cachedBytes) + newCachedBytes);
+        imageObserver()->decodedSizeChanged(*this, -static_cast<long long>(m_cachedBytes) + newCachedBytes);
 
     ASSERT(s_allDecodedDataSize >= m_cachedBytes);
     // Update with the difference in two steps to avoid unsigned underflow subtraction.
@@ -288,7 +288,7 @@ void PDFDocumentImage::draw(GraphicsContext& context, const FloatRect& dstRect,
     }
 
     if (imageObserver())
-        imageObserver()->didDraw(this);
+        imageObserver()->didDraw(*this);
 }
 
 void PDFDocumentImage::destroyDecodedData(bool)
index 6833ac6..f93435f 100644 (file)
@@ -37,7 +37,7 @@ void TextureMapperTiledBackingStore::updateContentsFromImageIfNeeded(TextureMapp
     updateContents(textureMapper, m_image.get(), m_image->size(), enclosingIntRect(m_image->rect()), BitmapTexture::UpdateCannotModifyOriginalImageData);
 
     if (m_image->imageObserver())
-        m_image->imageObserver()->didDraw(m_image.get());
+        m_image->imageObserver()->didDraw(*m_image);
     m_image = nullptr;
 }
 
index 2507c1c..9105c9f 100644 (file)
@@ -101,7 +101,7 @@ void Image::drawPattern(GraphicsContext& ctxt, const FloatRect& destRect, const
     ctxt.drawPattern(*this, destRect, tileRect, patternTransform, phase, spacing, op, blendMode);
 
     if (imageObserver())
-        imageObserver()->didDraw(this);
+        imageObserver()->didDraw(*this);
 }
 
 } // namespace WebCore
index 87bd5e1..f0e2c35 100644 (file)
@@ -1432,7 +1432,7 @@ bool RenderElement::mayCauseRepaintInsideViewport(const IntRect* optionalViewpor
     return visibleRect.intersects(enclosingIntRect(absoluteClippedOverflowRect()));
 }
 
-bool RenderElement::shouldRepaintInVisibleRect(const IntRect& visibleRect) const
+bool RenderElement::isVisibleInDocumentRect(const IntRect& documentRect) const
 {
     if (document().activeDOMObjectsAreSuspended())
         return false;
@@ -1456,7 +1456,7 @@ bool RenderElement::shouldRepaintInVisibleRect(const IntRect& visibleRect) const
     }
 
     LayoutRect backgroundPaintingRect = backgroundIsPaintedByRoot ? view().backgroundRect() : absoluteClippedOverflowRect();
-    if (!visibleRect.intersects(enclosingIntRect(backgroundPaintingRect)))
+    if (!documentRect.intersects(enclosingIntRect(backgroundPaintingRect)))
         return false;
 
     return true;
@@ -1493,24 +1493,29 @@ void RenderElement::visibleInViewportStateChanged()
     ASSERT_NOT_REACHED();
 }
 
-VisibleInViewportState RenderElement::imageFrameAvailable(CachedImage& image, ImageAnimatingState animatingState, const IntRect* changeRect)
+bool RenderElement::isVisibleInViewport() const
 {
     auto& frameView = view().frameView();
     auto visibleRect = frameView.windowToContents(frameView.windowClipRect());
-    bool shouldRepaint = shouldRepaintInVisibleRect(visibleRect);
+    return isVisibleInDocumentRect(visibleRect);
+}
+
+VisibleInViewportState RenderElement::imageFrameAvailable(CachedImage& image, ImageAnimatingState animatingState, const IntRect* changeRect)
+{
+    bool isVisible = isVisibleInViewport();
 
-    if (!shouldRepaint && animatingState == ImageAnimatingState::Yes)
+    if (!isVisible && animatingState == ImageAnimatingState::Yes)
         view().addRendererWithPausedImageAnimations(*this, image);
 
     // Static images should repaint even if they are outside the viewport rectangle
     // because they should be inside the TileCoverageRect.
-    if (shouldRepaint || animatingState == ImageAnimatingState::No)
+    if (isVisible || animatingState == ImageAnimatingState::No)
         imageChanged(&image, changeRect);
 
     if (element() && image.image()->isBitmapImage())
         element()->dispatchWebKitImageReadyEventForTesting();
 
-    return shouldRepaint ? VisibleInViewportState::Yes : VisibleInViewportState::No;
+    return isVisible ? VisibleInViewportState::Yes : VisibleInViewportState::No;
 }
 
 void RenderElement::didRemoveCachedImageClient(CachedImage& cachedImage)
@@ -1522,7 +1527,7 @@ void RenderElement::didRemoveCachedImageClient(CachedImage& cachedImage)
 bool RenderElement::repaintForPausedImageAnimationsIfNeeded(const IntRect& visibleRect, CachedImage& cachedImage)
 {
     ASSERT(m_hasPausedImageAnimations);
-    if (!shouldRepaintInVisibleRect(visibleRect))
+    if (!isVisibleInDocumentRect(visibleRect))
         return false;
 
     repaint();
index 2940a46..8067218 100644 (file)
@@ -141,7 +141,7 @@ public:
 
     bool borderImageIsLoadedAndCanBeRendered() const;
     bool mayCauseRepaintInsideViewport(const IntRect* visibleRect = nullptr) const;
-    bool shouldRepaintInVisibleRect(const IntRect& visibleRect) const;
+    bool isVisibleInDocumentRect(const IntRect& documentRect) const;
 
     // Returns true if this renderer requires a new stacking context.
     static bool createsGroupForStyle(const RenderStyle&);
@@ -318,6 +318,8 @@ private:
     std::unique_ptr<RenderStyle> computeFirstLineStyle() const;
     void invalidateCachedFirstLineStyle();
 
+    bool isVisibleInViewport() const;
+    bool canDestroyDecodedData() final { return !isVisibleInViewport(); }
     VisibleInViewportState imageFrameAvailable(CachedImage&, ImageAnimatingState, const IntRect* changeRect) final;
     void didRemoveCachedImageClient(CachedImage&) final;
 
index ca52980..b3897e2 100644 (file)
@@ -321,7 +321,7 @@ void SVGImage::draw(GraphicsContext& context, const FloatRect& dstRect, const Fl
     stateSaver.restore();
 
     if (imageObserver())
-        imageObserver()->didDraw(this);
+        imageObserver()->didDraw(*this);
 }
 
 RenderBox* SVGImage::embeddedContentBox() const
index 8863e05..271c395 100644 (file)
@@ -60,7 +60,7 @@ private:
         if (!imageObserver)
             return;
 
-        imageObserver->imageFrameAvailable(m_image, m_image->isAnimating() ? ImageAnimatingState::Yes : ImageAnimatingState::No, &r);
+        imageObserver->imageFrameAvailable(*m_image, m_image->isAnimating() ? ImageAnimatingState::Yes : ImageAnimatingState::No, &r);
     }
     
     SVGImage* m_image;
index 1bd5aaf..4c66bf1 100644 (file)
@@ -729,6 +729,11 @@ void Internals::pruneMemoryCacheToSize(unsigned size)
     MemoryCache::singleton().pruneDeadResourcesToSize(size);
     MemoryCache::singleton().pruneLiveResourcesToSize(size, true);
 }
+    
+void Internals::destroyDecodedDataForAllImages()
+{
+    MemoryCache::singleton().destroyDecodedDataForAllImages();
+}
 
 unsigned Internals::memoryCacheSize() const
 {
index d4b53aa..9b0e3d5 100644 (file)
@@ -114,6 +114,7 @@ public:
 
     void clearMemoryCache();
     void pruneMemoryCacheToSize(unsigned size);
+    void destroyDecodedDataForAllImages();
     unsigned memoryCacheSize() const;
 
     unsigned imageFrameIndex(HTMLImageElement&);
index fecce84..214c221 100644 (file)
@@ -103,6 +103,7 @@ enum EventThrottlingBehavior {
     boolean isStyleSheetLoadingSubresources(HTMLLinkElement link);
     void clearMemoryCache();
     void pruneMemoryCacheToSize(long size);
+    void destroyDecodedDataForAllImages();
     long memoryCacheSize();
     void setOverrideCachePolicy(CachePolicy policy);
     void setOverrideResourceLoadPriority(ResourceLoadPriority priority);
index c16b46a..14c3560 100644 (file)
@@ -1,3 +1,15 @@
+2017-05-15  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Do not delete asynchronously decoded frames for large images if their clients are in the viewport
+        https://bugs.webkit.org/show_bug.cgi?id=170640
+
+        Reviewed by Simon Fraser.
+
+        Enable the async decoding for large images.
+
+        * WebView/WebView.mm:
+        (-[WebView _preferencesChanged:]):
+
 2017-05-15  Jer Noble  <jer.noble@apple.com>
 
         Add experimental setting to allow document gesture interaction to fulfill media playback gesture requirement
index b34b621..7c1e057 100644 (file)
@@ -3085,9 +3085,7 @@ static bool needsSelfRetainWhileLoadingQuirk()
 
     settings.setShouldConvertInvalidURLsToBlank(shouldConvertInvalidURLsToBlank());
 
-    // FIXME: enable async image decoding after the flickering bug wk170640 is fixed.
-    // settings.setLargeImageAsyncDecodingEnabled([preferences largeImageAsyncDecodingEnabled]);
-    settings.setLargeImageAsyncDecodingEnabled(false);
+    settings.setLargeImageAsyncDecodingEnabled([preferences largeImageAsyncDecodingEnabled]);
     settings.setAnimatedImageAsyncDecodingEnabled([preferences animatedImageAsyncDecodingEnabled]);
 }
 
index f3a6df4..8207e54 100644 (file)
@@ -1,3 +1,15 @@
+2017-05-15  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Do not delete asynchronously decoded frames for large images if their clients are in the viewport
+        https://bugs.webkit.org/show_bug.cgi?id=170640
+
+        Reviewed by Simon Fraser.
+
+        Enable the async decoding for large images.
+
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::updatePreferences):
+
 2017-05-15  Youenn Fablet  <youenn@apple.com>
 
         Simplify RealtimeMediaSource data production and state
index b002fcd..73528d7 100644 (file)
@@ -3377,9 +3377,7 @@ void WebPage::updatePreferences(const WebPreferencesStore& store)
     m_viewportConfiguration.setCanIgnoreScalingConstraints(m_ignoreViewportScalingConstraints);
     setForceAlwaysUserScalable(m_forceAlwaysUserScalable || store.getBoolValueForKey(WebPreferencesKey::forceAlwaysUserScalableKey()));
 #endif
-    // FIXME: enable async image decoding after the flickering bug wk170640 is fixed.
-    // settings.setLargeImageAsyncDecodingEnabled(store.getBoolValueForKey(WebPreferencesKey::largeImageAsyncDecodingEnabledKey()));
-    settings.setLargeImageAsyncDecodingEnabled(false);
+    settings.setLargeImageAsyncDecodingEnabled(store.getBoolValueForKey(WebPreferencesKey::largeImageAsyncDecodingEnabledKey()));
     settings.setAnimatedImageAsyncDecodingEnabled(store.getBoolValueForKey(WebPreferencesKey::animatedImageAsyncDecodingEnabledKey()));
     settings.setShouldSuppressKeyboardInputDuringProvisionalNavigation(store.getBoolValueForKey(WebPreferencesKey::shouldSuppressKeyboardInputDuringProvisionalNavigationKey()));
 }