Merge r244372 - ScalableImageDecoder: don't forcefully decode image d…

…ata when querying frame completeness, duration

https://bugs.webkit.org/show_bug.cgi?id=191354
<rdar://problem/46123406>

Reviewed by Michael Catanzaro.

ScalableImageDecoder::frameIsCompleteAtIndex() should only check the
index validity and, if the index is valid, check for completeness of the
corresponding frame. ScalableImageDecoder::frameDurationAtIndex() should
also only retrieve duration for already-complete frames, or expand the
default 0-second value according to the flashing-protection rule when
the target frame is not yet complete.

Both methods avoid calling ScalableImageDecoder::frameBufferAtIndex()
as that method goes on and decodes image data to determine specific
information. The ImageSource class that's querying this information
doesn't anticipate this, and doesn't handle the increased memory
consumption of the decoded data, leaving MemoryCache in the blind about
the image resource's actual amount of consumed memory. ImageSource can
instead gracefully handle any incomplete frame by marking the decoding
status for this frame as only partial.

* platform/image-decoders/ScalableImageDecoder.cpp:
(WebCore::ScalableImageDecoder::frameIsCompleteAtIndex const):
(WebCore::ScalableImageDecoder::frameHasAlphaAtIndex const):
(WebCore::ScalableImageDecoder::frameDurationAtIndex const):

Source/WebCore/ChangeLog

@@ -1,3 +1,32 @@
+2019-04-16  Zan Dobersek  <zdobersek@igalia.com>
+
+        ScalableImageDecoder: don't forcefully decode image data when querying frame completeness, duration
+        https://bugs.webkit.org/show_bug.cgi?id=191354
+        <rdar://problem/46123406>
+
+        Reviewed by Michael Catanzaro.
+
+        ScalableImageDecoder::frameIsCompleteAtIndex() should only check the
+        index validity and, if the index is valid, check for completeness of the
+        corresponding frame. ScalableImageDecoder::frameDurationAtIndex() should
+        also only retrieve duration for already-complete frames, or expand the
+        default 0-second value according to the flashing-protection rule when
+        the target frame is not yet complete.
+
+        Both methods avoid calling ScalableImageDecoder::frameBufferAtIndex()
+        as that method goes on and decodes image data to determine specific
+        information. The ImageSource class that's querying this information
+        doesn't anticipate this, and doesn't handle the increased memory
+        consumption of the decoded data, leaving MemoryCache in the blind about
+        the image resource's actual amount of consumed memory. ImageSource can
+        instead gracefully handle any incomplete frame by marking the decoding
+        status for this frame as only partial.
+
+        * platform/image-decoders/ScalableImageDecoder.cpp:
+        (WebCore::ScalableImageDecoder::frameIsCompleteAtIndex const):
+        (WebCore::ScalableImageDecoder::frameHasAlphaAtIndex const):
+        (WebCore::ScalableImageDecoder::frameDurationAtIndex const):
+
 2019-06-20  Ryosuke Niwa  <rniwa@webkit.org>
 
         REGRESSION(r245912): Crash in TextIterator::range via visiblePositionForIndexUsingCharacterIterator

Source/WebCore/platform/image-decoders/ScalableImageDecoder.cpp

@@ -196,21 +196,23 @@ template <MatchType type> int getScaledValue(const Vector<int>& scaledValues, in
 bool ScalableImageDecoder::frameIsCompleteAtIndex(size_t index) const
 {
     LockHolder lockHolder(m_mutex);
-    // FIXME(176089): asking whether enough data has been appended for a decode
-    // operation to succeed should not require decoding the entire frame.
-    // This function should be implementable in a way that allows const.
-    auto* buffer = const_cast<ScalableImageDecoder*>(this)->frameBufferAtIndex(index);
-    return buffer && buffer->isComplete();
+    if (index >= m_frameBufferCache.size())
+        return false;
+
+    auto& frame = m_frameBufferCache[index];
+    return frame.isComplete();
 }
 
 bool ScalableImageDecoder::frameHasAlphaAtIndex(size_t index) const
 {
     LockHolder lockHolder(m_mutex);
     if (m_frameBufferCache.size() <= index)
         return true;
-    if (m_frameBufferCache[index].isComplete())
-        return m_frameBufferCache[index].hasAlpha();
-    return true;
+
+    auto& frame = m_frameBufferCache[index];
+    if (!frame.isComplete())
+        return true;
+    return frame.hasAlpha();
 }
 
 unsigned ScalableImageDecoder::frameBytesAtIndex(size_t index, SubsamplingLevel) const
@@ -225,20 +227,24 @@ unsigned ScalableImageDecoder::frameBytesAtIndex(size_t index, SubsamplingLevel)
 Seconds ScalableImageDecoder::frameDurationAtIndex(size_t index) const
 {
     LockHolder lockHolder(m_mutex);
-    // FIXME(176089): asking for the duration of a sub-image should not require decoding
-    // the entire frame. This function should be implementable in a way that
-    // allows const.
-    auto* buffer = const_cast<ScalableImageDecoder*>(this)->frameBufferAtIndex(index);
-    if (!buffer || buffer->isInvalid())
+    if (index >= m_frameBufferCache.size())
         return 0_s;
-
+
+    // Returning 0_s in case of an incomplete frame can break display of animated image formats.
+    // We pick up the decoded duration if it's available, otherwise the default 0_s value is
+    // adjusted below.
+    Seconds duration = 0_s;
+    auto& frame = m_frameBufferCache[index];
+    if (frame.isComplete())
+        duration = frame.duration();
+
     // Many annoying ads specify a 0 duration to make an image flash as quickly as possible.
     // We follow Firefox's behavior and use a duration of 100 ms for any frames that specify
     // a duration of <= 10 ms. See <rdar://problem/7689300> and <http://webkit.org/b/36082>
     // for more information.
-    if (buffer->duration() < 11_ms)
+    if (duration < 11_ms)
         return 100_ms;
-    return buffer->duration();
+    return duration;
 }
 
 NativeImagePtr ScalableImageDecoder::createFrameImageAtIndex(size_t index, SubsamplingLevel, const DecodingOptions&)