Charts on sixcolors.com flicker when zooming in

https://bugs.webkit.org/show_bug.cgi?id=256620 rdar://108930635 Reviewed by Simon Fraser. Rearrange the logic of RenderBoxModelObject::decodingModeForImageDraw() such that we call isVisibleInViewport() at the end of this function. But there is only one exception to this. If the image has the attribute decoding="async" specified, then we have to make sure the image will not flicker. And to check that we have to call isVisibleInViewport(). Fix a subtle one-time-flickering we should not rely on the layer repaint count only because new layers can be created when pinch zoom the image. In addition to the repaint count, we can rely on a new flag called hasEverPaintedImages which can be stored in the NodeRareData. It is initialized to false and it is set to true when the image is drawn by its RenderObject. An image is allowed to be asynchronously decoded if layer repaint count is zero and the element's flag hasEverPaintedImages is false. The internal setting setLargeImageAsyncDecodingEnabledForTesting() will be renamed to setAsyncDecodingEnabledForTesting(). Its use will change to enable async image decoding for any image regardless of its size. * LayoutTests/fast/images/async-image-background-change.html: * LayoutTests/fast/images/async-image-background-image-repeated.html: * LayoutTests/fast/images/async-image-background-image.html: * LayoutTests/fast/images/async-image-body-background-image.html: * LayoutTests/fast/images/async-image-multiple-clients-repaint.html: * LayoutTests/fast/images/async-image-src-change.html: * LayoutTests/fast/images/decode-render-static-image.html: * LayoutTests/fast/images/decoding-attribute-async-small-image.html: * LayoutTests/fast/images/decoding-attribute-dynamic-async-small-image.html: * LayoutTests/fast/images/sprite-sheet-image-draw.html: * LayoutTests/http/tests/images/render-partial-image-load.html: * Source/WebCore/dom/Node.cpp: (WebCore::Node::hasEverPaintedImages const): (WebCore::Node::setHasEverPaintedImages): * Source/WebCore/dom/Node.h: * Source/WebCore/dom/NodeRareData.h: (WebCore::NodeRareData::hasEverPaintedImages const): (WebCore::NodeRareData::setHasEverPaintedImages): * Source/WebCore/platform/graphics/BitmapImage.h: * Source/WebCore/rendering/BackgroundPainter.cpp: (WebCore::BackgroundPainter::paintFillLayer): * Source/WebCore/rendering/RenderBoxModelObject.cpp: (WebCore::RenderBoxModelObject::decodingModeForImageDraw const): * Source/WebCore/rendering/RenderImage.cpp: (WebCore::RenderImage::paintIntoRect): * Source/WebCore/rendering/RenderObject.h: * Source/WebCore/testing/Internals.cpp: (WebCore::Internals::setAsyncDecodingEnabledForTesting): (WebCore::Internals::setLargeImageAsyncDecodingEnabledForTesting): Deleted. * Source/WebCore/testing/Internals.h: * Source/WebCore/testing/Internals.idl: Canonical link: https://commits.webkit.org/265328@main
WebKit · Jun 20, 2023 · 8f042c7 · 8f042c7
1 parent f77a168
commit 8f042c7
Show file tree

Hide file tree

Showing 22 changed files with 93 additions and 40 deletions.
diff --git a/LayoutTests/fast/images/async-image-background-change.html b/LayoutTests/fast/images/async-image-background-change.html
@@ -14,10 +14,10 @@
                 image.onload = (() => {
                     if (window.internals && window.testRunner && forceAsyncImageDrawing) {
                         // Force async image decoding for this image.
-                        internals.setLargeImageAsyncDecodingEnabledForTesting(image, true);
+                        internals.setAsyncDecodingEnabledForTesting(image, true);
 
                         image.addEventListener("webkitImageFrameReady", function() {
-                            internals.setLargeImageAsyncDecodingEnabledForTesting(image, false);
+                            internals.setAsyncDecodingEnabledForTesting(image, false);
                             setTimeout(function() {
                                 // Force redraw to get the red image drawn.
                                 testRunner.display();

diff --git a/LayoutTests/fast/images/async-image-background-image-repeated.html b/LayoutTests/fast/images/async-image-background-image-repeated.html
@@ -51,7 +51,7 @@
             image.onload = function() {
                  // Force async image decoding for this image.
                 if (window.internals)
-                    internals.setLargeImageAsyncDecodingEnabledForTesting(image, true);
+                    internals.setAsyncDecodingEnabledForTesting(image, true);
 
                 // Change the background of the elements.
                 var elements = document.getElementsByClassName("image-background");

diff --git a/LayoutTests/fast/images/async-image-background-image.html b/LayoutTests/fast/images/async-image-background-image.html
@@ -34,7 +34,7 @@
             image.onload = function() {
                 // Force async image decoding for this image.
                 if (window.internals)
-                    internals.setLargeImageAsyncDecodingEnabledForTesting(image, true);
+                    internals.setAsyncDecodingEnabledForTesting(image, true);
 
                 // Change the background of the element.                 
                 var element = document.getElementsByClassName("image-background")[0];

diff --git a/LayoutTests/fast/images/async-image-body-background-image.html b/LayoutTests/fast/images/async-image-body-background-image.html
@@ -37,7 +37,7 @@
             image.onload = function() {
                 // Force async image decoding for this image.
                 if (window.internals)
-                    internals.setLargeImageAsyncDecodingEnabledForTesting(image, true);
+                    internals.setAsyncDecodingEnabledForTesting(image, true);
 
                 var iframeDocument = document.querySelector('iframe').contentWindow.document;
 

diff --git a/LayoutTests/fast/images/async-image-multiple-clients-repaint.html b/LayoutTests/fast/images/async-image-multiple-clients-repaint.html
@@ -67,7 +67,7 @@
             image.onload = function() {
                 // Force async image decoding for this image.
                 if (window.internals)
-                    internals.setLargeImageAsyncDecodingEnabledForTesting(image, true);
+                    internals.setAsyncDecodingEnabledForTesting(image, true);
 
                 if (window.internals && window.testRunner) {
                     setElementImageBackground(document.querySelector(".small-box"), image).then(() => {

diff --git a/LayoutTests/fast/images/async-image-src-change.html b/LayoutTests/fast/images/async-image-src-change.html
@@ -7,15 +7,15 @@
                 image.onload = (() => {
                     if (window.internals && window.testRunner && forceAsyncImageDrawing) {
                         // Force async image decoding for this image.
-                        internals.setLargeImageAsyncDecodingEnabledForTesting(image, true);
+                        internals.setAsyncDecodingEnabledForTesting(image, true);
 
                         // Force layout and display so the image gets drawn.
                         document.body.offsetHeight;
                         if (window.testRunner)
                             testRunner.display();
 
                         image.addEventListener("webkitImageFrameReady", function() {
-                            internals.setLargeImageAsyncDecodingEnabledForTesting(image, false);
+                            internals.setAsyncDecodingEnabledForTesting(image, false);
                             setTimeout(function() {
                                 // Force redraw to get the red image drawn.
                                 testRunner.display();

diff --git a/LayoutTests/fast/images/decode-render-static-image.html b/LayoutTests/fast/images/decode-render-static-image.html
@@ -26,7 +26,7 @@
         image.onload = (() => {
             if (window.internals && window.testRunner) {
                 // Force async image decoding for this image.
-                internals.setLargeImageAsyncDecodingEnabledForTesting(image, true);
+                internals.setAsyncDecodingEnabledForTesting(image, true);
 
                 // Force layout and display so the image gets drawn.
                 document.body.offsetHeight;

diff --git a/LayoutTests/fast/images/decoding-attribute-async-small-image.html b/LayoutTests/fast/images/decoding-attribute-async-small-image.html
@@ -6,6 +6,9 @@
             return new Promise((resolve) => {
                 image.onload = (() => {
                     if (window.internals && window.testRunner) {
+                        // Force async image decoding for this image.
+                        internals.setAsyncDecodingEnabledForTesting(image, true);
+
                         // Force layout and display so the image gets drawn.
                         document.body.offsetHeight;
                         testRunner.display();

diff --git a/LayoutTests/fast/images/decoding-attribute-dynamic-async-small-image.html b/LayoutTests/fast/images/decoding-attribute-dynamic-async-small-image.html
@@ -6,6 +6,9 @@
             return new Promise((resolve) => {
                 image.onload = (() => {
                     if (window.internals && window.testRunner) {
+                        // Force async image decoding for this image.
+                        internals.setAsyncDecodingEnabledForTesting(image, true);
+
                         // Force layout and display so the image gets drawn.
                         document.body.offsetHeight;
                         testRunner.display();

diff --git a/LayoutTests/fast/images/sprite-sheet-image-draw.html b/LayoutTests/fast/images/sprite-sheet-image-draw.html
@@ -28,7 +28,7 @@
             image.onload = function() {
                 // Force async image decoding for this image.
                 if (window.internals)
-                    internals.setLargeImageAsyncDecodingEnabledForTesting(image, true);
+                    internals.setAsyncDecodingEnabledForTesting(image, true);
 
                 // Change the background of the element.
                 var element = document.getElementsByClassName("image-background")[0];

diff --git a/LayoutTests/http/tests/images/render-partial-image-load.html b/LayoutTests/http/tests/images/render-partial-image-load.html
@@ -20,7 +20,7 @@
 
             // Force async image decoding for this image.
             if (window.internals)
-                internals.setLargeImageAsyncDecodingEnabledForTesting(image, true);
+                internals.setAsyncDecodingEnabledForTesting(image, true);
 
             image.onload = (() => {
                 if (window.testRunner)

diff --git a/Source/WebCore/dom/Node.cpp b/Source/WebCore/dom/Node.cpp
@@ -1273,6 +1273,16 @@ void Node::setManuallyAssignedSlot(HTMLSlotElement* slotElement)
     ensureRareData().setManuallyAssignedSlot(slotElement);
 }
 
+bool Node::hasEverPaintedImages() const
+{
+    return hasRareData() && rareData()->hasEverPaintedImages();
+}
+
+void Node::setHasEverPaintedImages(bool hasEverPaintedImages)
+{
+    ensureRareData().setHasEverPaintedImages(hasEverPaintedImages);
+}
+
 ContainerNode* Node::parentInComposedTree() const
 {
     ASSERT(isMainThreadOrGCThread());

diff --git a/Source/WebCore/dom/Node.h b/Source/WebCore/dom/Node.h
@@ -250,6 +250,9 @@ class Node : public EventTarget {
     HTMLSlotElement* manuallyAssignedSlot() const;
     void setManuallyAssignedSlot(HTMLSlotElement*);
 
+    bool hasEverPaintedImages() const;
+    void setHasEverPaintedImages(bool);
+
     bool isUncustomizedCustomElement() const { return customElementState() == CustomElementState::Uncustomized; }
     bool isCustomElementUpgradeCandidate() const { return customElementState() == CustomElementState::Undefined; }
     bool isDefinedCustomElement() const { return customElementState() == CustomElementState::Custom; }

diff --git a/Source/WebCore/dom/NodeRareData.h b/Source/WebCore/dom/NodeRareData.h
@@ -286,6 +286,9 @@ class NodeRareData {
     HTMLSlotElement* manuallyAssignedSlot() { return m_manuallyAssignedSlot.get(); }
     void setManuallyAssignedSlot(HTMLSlotElement* slot) { m_manuallyAssignedSlot = slot; }
 
+    bool hasEverPaintedImages() const { return m_hasEverPaintedImages; }
+    void setHasEverPaintedImages(bool hasEverPaintedImages) { m_hasEverPaintedImages = hasEverPaintedImages; }
+
 #if DUMP_NODE_STATISTICS
     OptionSet<UseType> useTypes() const
     {
@@ -308,7 +311,8 @@ class NodeRareData {
     std::unique_ptr<NodeListsNodeData> m_nodeLists;
     std::unique_ptr<NodeMutationObserverData> m_mutationObserverData;
     WeakPtr<HTMLSlotElement, WeakPtrImplWithEventTargetData> m_manuallyAssignedSlot;
-    bool m_isElementRareData; // Keep last for better bit packing with ElementRareData.
+    bool m_isElementRareData;
+    bool m_hasEverPaintedImages { false }; // Keep last for better bit packing with ElementRareData.
 };
 
 template<> struct NodeListTypeIdentifier<NameNodeList> {

diff --git a/Source/WebCore/platform/graphics/BitmapImage.h b/Source/WebCore/platform/graphics/BitmapImage.h
@@ -117,8 +117,8 @@ class BitmapImage final : public Image {
     bool canUseAsyncDecodingForLargeImages() const;
     bool shouldUseAsyncDecodingForAnimatedImages() const;
     void setClearDecoderAfterAsyncFrameRequestForTesting(bool value) { m_clearDecoderAfterAsyncFrameRequestForTesting = value; }
-    void setLargeImageAsyncDecodingEnabledForTesting(bool enabled) { m_largeImageAsyncDecodingEnabledForTesting = enabled; }
-    bool isLargeImageAsyncDecodingEnabledForTesting() const { return m_largeImageAsyncDecodingEnabledForTesting; }
+    void setAsyncDecodingEnabledForTesting(bool enabled) { m_asyncDecodingEnabledForTesting = enabled; }
+    bool isAsyncDecodingEnabledForTesting() const { return m_asyncDecodingEnabledForTesting; }
     void stopAsyncDecodingQueue() { m_source->stopAsyncDecodingQueue(); }
 
     DestinationColorSpace colorSpace() final;
@@ -248,7 +248,7 @@ class BitmapImage final : public Image {
     bool m_showDebugBackground { false };
 
     bool m_clearDecoderAfterAsyncFrameRequestForTesting { false };
-    bool m_largeImageAsyncDecodingEnabledForTesting { false };
+    bool m_asyncDecodingEnabledForTesting { false };
 
 #if ASSERT_ENABLED || !LOG_DISABLED
     size_t m_lateFrameCount { 0 };

diff --git a/Source/WebCore/rendering/BackgroundPainter.cpp b/Source/WebCore/rendering/BackgroundPainter.cpp
@@ -416,6 +416,9 @@ void BackgroundPainter::paintFillLayer(const Color& color, const FillLayer& bgLa
                 ASSERT(bgImage->hasCachedImage());
                 bgImage->cachedImage()->addClientWaitingForAsyncDecoding(m_renderer);
             }
+
+            if (m_renderer.element() && !context.paintingDisabled())
+                m_renderer.element()->setHasEverPaintedImages(true);
         }
     }
 

diff --git a/Source/WebCore/rendering/RenderBoxModelObject.cpp b/Source/WebCore/rendering/RenderBoxModelObject.cpp
@@ -310,36 +310,60 @@ DecodingMode RenderBoxModelObject::decodingModeForImageDraw(const Image& image,
         return DecodingMode::Synchronous;
     }
 
-    // Large image case.
+    // Some document types force synchronous decoding.
 #if PLATFORM(IOS_FAMILY)
     if (IOSApplication::isIBooksStorytime())
         return DecodingMode::Synchronous;
 #endif
-    if (paintInfo.paintBehavior.contains(PaintBehavior::Snapshotting))
+    if (document().isImageDocument())
         return DecodingMode::Synchronous;
-    if (paintInfo.paintBehavior.contains(PaintBehavior::ForceSynchronousImageDecode))
+
+    // A PaintBehavior may force synchronous decoding.
+    if (paintInfo.paintBehavior.contains(PaintBehavior::Snapshotting))
         return DecodingMode::Synchronous;
+
+    auto defaultDecodingMode = [&]() -> DecodingMode {
+        if (paintInfo.paintBehavior.contains(PaintBehavior::ForceSynchronousImageDecode))
+            return DecodingMode::Synchronous;
+
+        // First tile paint.
+        if (paintInfo.paintBehavior.contains(PaintBehavior::DefaultAsynchronousImageDecode)) {
+            // And the images has not been painted in this element yet.
+            if (element() && !element()->hasEverPaintedImages())
+                return DecodingMode::Asynchronous;
+        }
+
+        // FIXME: Calling isVisibleInViewport() is not cheap. Find a way to make this faster.
+        return isVisibleInViewport() ? DecodingMode::Synchronous : DecodingMode::Asynchronous;
+    };
+
     if (is<HTMLImageElement>(element())) {
-        auto decodingMode = downcast<HTMLImageElement>(*element()).decodingMode();
-        if (decodingMode == DecodingMode::Asynchronous)
-            return DecodingMode::Asynchronous;
-        if (decodingMode == DecodingMode::Synchronous)
+        // <img decoding="sync"> forces synchronous decoding.
+        if (downcast<HTMLImageElement>(*element()).decodingMode() == DecodingMode::Synchronous)
             return DecodingMode::Synchronous;
+
+        // <img decoding="async"> forces asynchronous decoding but make sure this
+        // will not cause flickering.
+        if (downcast<HTMLImageElement>(*element()).decodingMode() == DecodingMode::Asynchronous) {
+            // isAsyncDecodingEnabledForTesting() forces async image decoding regardless whether it is in the viewport or not.
+            if (bitmapImage.isAsyncDecodingEnabledForTesting())
+                return DecodingMode::Asynchronous;
+
+            // Choose a decodingMode such that the image does not flicker.
+            return defaultDecodingMode();
+        }
     }
-    if (bitmapImage.isLargeImageAsyncDecodingEnabledForTesting())
+
+    // isAsyncDecodingEnabledForTesting() forces async image decoding regardless of the size.
+    if (bitmapImage.isAsyncDecodingEnabledForTesting())
         return DecodingMode::Asynchronous;
-    if (document().isImageDocument())
-        return DecodingMode::Synchronous;
-    if (!settings().largeImageAsyncDecodingEnabled())
-        return DecodingMode::Synchronous;
-    if (!bitmapImage.canUseAsyncDecodingForLargeImages())
+
+    // Large image case.
+    if (!(bitmapImage.canUseAsyncDecodingForLargeImages() && settings().largeImageAsyncDecodingEnabled()))
         return DecodingMode::Synchronous;
-    if (paintInfo.paintBehavior.contains(PaintBehavior::DefaultAsynchronousImageDecode))
-        return DecodingMode::Asynchronous;
-    // FIXME: isVisibleInViewport() is not cheap. Find a way to make this condition faster.
-    if (!isVisibleInViewport())
-        return DecodingMode::Asynchronous;
-    return DecodingMode::Synchronous;
+
+    // Choose a decodingMode such that the image does not flicker.
+    return defaultDecodingMode();
 }
 
 LayoutSize RenderBoxModelObject::relativePositionOffset() const

diff --git a/Source/WebCore/rendering/RenderImage.cpp b/Source/WebCore/rendering/RenderImage.cpp
@@ -716,6 +716,9 @@ ImageDrawResult RenderImage::paintIntoRect(PaintInfo& paintInfo, const FloatRect
         theme().paintSystemPreviewBadge(*img, paintInfo, rect);
 #endif
 
+    if (element() && !paintInfo.context().paintingDisabled())
+        element()->setHasEverPaintedImages(true);
+
     return drawResult;
 }
 

diff --git a/Source/WebCore/rendering/RenderObject.h b/Source/WebCore/rendering/RenderObject.h
@@ -963,8 +963,8 @@ class RenderObject : public CachedImageClient {
 
     private:
         unsigned m_positionedState : 2; // PositionedState
-        unsigned m_selectionState : 3; // SelectionState
-        unsigned m_fragmentedFlowState : 2; // FragmentedFlowState
+        unsigned m_selectionState : 3; // HighlightState
+        unsigned m_fragmentedFlowState : 1; // FragmentedFlowState
         unsigned m_boxDecorationState : 2; // BoxDecorationState
 
     public:

diff --git a/Source/WebCore/testing/Internals.cpp b/Source/WebCore/testing/Internals.cpp
@@ -1165,10 +1165,10 @@ unsigned Internals::remoteImagesCountForTesting() const
     return document->page()->chrome().client().remoteImagesCountForTesting();
 }
 
-void Internals::setLargeImageAsyncDecodingEnabledForTesting(HTMLImageElement& element, bool enabled)
+void Internals::setAsyncDecodingEnabledForTesting(HTMLImageElement& element, bool enabled)
 {
     if (auto* bitmapImage = bitmapImageFromImageElement(element))
-        bitmapImage->setLargeImageAsyncDecodingEnabledForTesting(enabled);
+        bitmapImage->setAsyncDecodingEnabledForTesting(enabled);
 }
 
 void Internals::setForceUpdateImageDataEnabledForTesting(HTMLImageElement& element, bool enabled)

diff --git a/Source/WebCore/testing/Internals.h b/Source/WebCore/testing/Internals.h
@@ -242,7 +242,7 @@ class Internals final : public RefCounted<Internals>, private ContextDestruction
     unsigned imageDecodeCount(HTMLImageElement&);
     unsigned imageCachedSubimageCreateCount(HTMLImageElement&);
     unsigned remoteImagesCountForTesting() const;
-    void setLargeImageAsyncDecodingEnabledForTesting(HTMLImageElement&, bool enabled);
+    void setAsyncDecodingEnabledForTesting(HTMLImageElement&, bool enabled);
     void setForceUpdateImageDataEnabledForTesting(HTMLImageElement&, bool enabled);
 
     void setGridMaxTracksLimit(unsigned);

diff --git a/Source/WebCore/testing/Internals.idl b/Source/WebCore/testing/Internals.idl
@@ -583,7 +583,7 @@ typedef (FetchRequest or FetchResponse) FetchObject;
     unsigned long imageDecodeCount(HTMLImageElement element);
     unsigned long imageCachedSubimageCreateCount(HTMLImageElement element);
     unsigned long remoteImagesCountForTesting();
-    undefined setLargeImageAsyncDecodingEnabledForTesting(HTMLImageElement element, boolean enabled);
+    undefined setAsyncDecodingEnabledForTesting(HTMLImageElement element, boolean enabled);
     undefined setForceUpdateImageDataEnabledForTesting(HTMLImageElement element, boolean enabled);
 
     undefined setGridMaxTracksLimit(unsigned long maxTracksLimit);