Changeset 223754 in webkit

Timestamp:

Oct 20, 2017 4:08:12 AM (7 years ago)

Author:

magomez@igalia.com

Message:

[GTK][WPE] Fix review comments on WEBPImageDecoder
​https://bugs.webkit.org/show_bug.cgi?id=178080

Reviewed by Said Abou-Hallawa.

Source/WebCore:

Properly free the demuxer in case of error, improve the code to detect the first
required frame to decode, fix the usage of the DecodingStatus and some styling
changes.

Covered by existent tests.

• platform/image-decoders/webp/WEBPImageDecoder.cpp:

(WebCore::webpFrameAtIndex):
(WebCore::WEBPImageDecoder::findFirstRequiredFrameToDecode):
(WebCore::WEBPImageDecoder::decode):
(WebCore::WEBPImageDecoder::decodeFrame):
(WebCore::WEBPImageDecoder::initFrameBuffer):
(WebCore::WEBPImageDecoder::clearFrameBufferCache):

LayoutTests:

Adjusted test duration.

• fast/images/animated-webp.html:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/images/animated-webp.html (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp (modified) (11 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2017-10-20  Miguel Gomez  <magomez@igalia.com>
+
+        [GTK][WPE] Fix review comments on WEBPImageDecoder
+        https://bugs.webkit.org/show_bug.cgi?id=178080
+
+        Reviewed by Said Abou-Hallawa.
+
+        Adjusted test duration.
+
+        * fast/images/animated-webp.html:
+
 2017-10-20  Zan Dobersek  <zdobersek@igalia.com>
 

trunk/LayoutTests/fast/images/animated-webp.html

@@ -13 +13 @@
             if (window.testRunner)
                 testRunner.notifyDone();
-        }, 500);
+        }, 300);
     }
 </script>

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2017-10-20  Miguel Gomez  <magomez@igalia.com>
+
+        [GTK][WPE] Fix review comments on WEBPImageDecoder
+        https://bugs.webkit.org/show_bug.cgi?id=178080
+
+        Reviewed by Said Abou-Hallawa.
+
+        Properly free the demuxer in case of error, improve the code to detect the first
+        required frame to decode, fix the usage of the DecodingStatus and some styling
+        changes.
+
+        Covered by existent tests.
+
+        * platform/image-decoders/webp/WEBPImageDecoder.cpp:
+        (WebCore::webpFrameAtIndex):
+        (WebCore::WEBPImageDecoder::findFirstRequiredFrameToDecode):
+        (WebCore::WEBPImageDecoder::decode):
+        (WebCore::WEBPImageDecoder::decodeFrame):
+        (WebCore::WEBPImageDecoder::initFrameBuffer):
+        (WebCore::WEBPImageDecoder::clearFrameBufferCache):
+
 2017-10-20  Basuke Suzuki  <Basuke.Suzuki@sony.com>
 

trunk/Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp

@@ -34 +34 @@
 namespace WebCore {
 
+// Convenience function to improve code readability, as WebPDemuxGetFrame is +1 based.
+bool webpFrameAtIndex(WebPDemuxer* demuxer, size_t index, WebPIterator* webpFrame)
+{
+    return WebPDemuxGetFrame(demuxer, index + 1, webpFrame);
+}
+
 WEBPImageDecoder::WEBPImageDecoder(AlphaOption alphaOption, GammaAndColorProfileOption gammaAndColorProfileOption)
     : ScalableImageDecoder(alphaOption, gammaAndColorProfileOption)
@@ -86 +92 @@
         return 0;
 
-    // Check the most probable scenario first: the previous frame is complete, so we can decode the requested one.
-    if (m_frameBufferCache[frameIndex - 1].isComplete())
-        return frameIndex;
-
-    // Check if the requested frame can be rendered without dependencies. This happens if the frame
-    // fills the whole area and doesn't have alpha.
-    WebPIterator webpFrame;
-    if (WebPDemuxGetFrame(demuxer, frameIndex + 1, &webpFrame)) {
+    // Go backwards and find the first complete frame.
+    size_t firstIncompleteFrame = frameIndex;
+    for (; firstIncompleteFrame; --firstIncompleteFrame) {
+        if (m_frameBufferCache[firstIncompleteFrame - 1].isComplete())
+            break;
+    }
+
+    // Check if there are any independent frames between firstIncompleteFrame and frameIndex.
+    for (size_t firstIndependentFrame = frameIndex; firstIndependentFrame > firstIncompleteFrame ; --firstIndependentFrame) {
+        WebPIterator webpFrame;
+        if (!webpFrameAtIndex(demuxer, firstIndependentFrame, &webpFrame))
+            continue;
+
         IntRect frameRect(webpFrame.x_offset, webpFrame.y_offset, webpFrame.width, webpFrame.height);
-        if (frameRect.contains(IntRect(IntPoint(), size())) && !webpFrame.has_alpha)
-            return frameIndex;
-    }
-
-    // Go backwards in the list of frames, until we find the first complete frame or a frame that
-    // doesn't depend on previous frames.
-    for (size_t i = frameIndex - 1; i > 0; i--) {
-        // This frame is complete, so we can start the decoding from the next one.
-        if (m_frameBufferCache[i].isComplete())
-            return i + 1;
-
-        if (WebPDemuxGetFrame(demuxer, i + 1, &webpFrame)) {
-            IntRect frameRect(webpFrame.x_offset, webpFrame.y_offset, webpFrame.width, webpFrame.height);
-            // This frame is not complete, but it fills the whole size and its disposal method is
-            // RestoreToBackground. This means that we will draw the next frame on an initially transparent
-            // buffer, so there's no dependency. We can start decoding from the next frame.
-            if (frameRect.contains(IntRect(IntPoint(), size())) && (m_frameBufferCache[i].disposalMethod() == ImageFrame::DisposalMethod::RestoreToBackground))
-                return i + 1;
-
-            // This frame is not complete, but it fills the whole size and doesn't have alpha,
-            // so it doesn't depend on former frames. We can start decoding from here.
-            if (frameRect.contains(IntRect(IntPoint(), size())) && !webpFrame.has_alpha)
-                return i;
-        }
-    }
-    return 0;
+        if (!frameRect.contains({ { }, size() }))
+            continue;
+
+        // This frame covers the whole area and doesn't have alpha, so it can be rendered without
+        // dependencies.
+        if (!webpFrame.has_alpha)
+            return firstIndependentFrame;
+
+        // This frame covers the whole area and its disposalMethod is RestoreToBackground, which means
+        // that the next frame will be rendered on top of a transparent background, and can be decoded
+        // without dependencies. This can only be checked for frames prior to frameIndex.
+        if (firstIndependentFrame < frameIndex && m_frameBufferCache[firstIndependentFrame].disposalMethod() == ImageFrame::DisposalMethod::RestoreToBackground)
+            return firstIndependentFrame + 1;
+    }
+
+    return firstIncompleteFrame;
 }
 
@@ -144 +145 @@
     // It is a fatal error if all data is received and we have decoded all frames available but the file is truncated.
     if (frameIndex >= m_frameBufferCache.size() - 1 && allDataReceived && demuxer && demuxerState != WEBP_DEMUX_DONE) {
-        setFailed();
-        return;
-    }
-
-    size_t startFrame = findFirstRequiredFrameToDecode(frameIndex, demuxer);
-    for (size_t i = startFrame; i <= frameIndex; i++)
+        WebPDemuxDelete(demuxer);
+        setFailed();
+        return;
+    }
+
+    for (size_t i = findFirstRequiredFrameToDecode(frameIndex, demuxer); i <= frameIndex; i++)
         decodeFrame(i, demuxer);
 
@@ -161 +162 @@
 
     WebPIterator webpFrame;
-    if (!WebPDemuxGetFrame(demuxer, frameIndex + 1, &webpFrame))
+    if (!webpFrameAtIndex(demuxer, frameIndex, &webpFrame))
         return;
 
@@ -185 +186 @@
     decoderBuffer.u.RGBA.size = decoderBuffer.u.RGBA.stride * webpFrame.height;
     decoderBuffer.is_external_memory = 1;
-    decoderBuffer.u.RGBA.rgba = reinterpret_cast<uint8_t*>(fastMalloc(decoderBuffer.u.RGBA.size));
+    std::unique_ptr<unsigned char[]> p(new uint8_t[decoderBuffer.u.RGBA.size]());
+    decoderBuffer.u.RGBA.rgba = p.get();
     if (!decoderBuffer.u.RGBA.rgba) {
         setFailed();
@@ -193 +195 @@
     WebPIDecoder* decoder = WebPINewDecoder(&decoderBuffer);
     if (!decoder) {
-        fastFree(decoderBuffer.u.RGBA.rgba);
         setFailed();
         return;
@@ -206 +207 @@
         if (!isAllDataReceived()) {
             applyPostProcessing(frameIndex, decoder, decoderBuffer, blend);
+            buffer.setDecodingStatus(DecodingStatus::Partial);
             break;
         }
@@ -214 +216 @@
 
     WebPIDelete(decoder);
-    fastFree(decoderBuffer.u.RGBA.rgba);
 }
 
@@ -228 +229 @@
 
     // Make sure the frameRect doesn't extend outside the buffer.
-    if (frameRect.maxX() > size().width())
-        frameRect.setWidth(size().width() - webpFrame->x_offset);
-    if (frameRect.maxY() > size().height())
-        frameRect.setHeight(size().height() - webpFrame->y_offset);
+    frameRect.intersect({ { }, size() });
 
     if (!frameIndex || !m_frameBufferCache[frameIndex - 1].backingStore()) {
@@ -255 +253 @@
     buffer.setHasAlpha(webpFrame->has_alpha);
     buffer.backingStore()->setFrameRect(frameRect);
-    buffer.setDecodingStatus(DecodingStatus::Partial);
 
     return true;
@@ -353 +350 @@
     //
     // In WEBP every frame depends on the previous one or none. That means that frames after clearBeforeFrame
-    // won't need any frame before them to render, so we can clear them all. If we find a buffer that is partial,
-    // don't delete it as it's being decoded.
+    // won't need any frame before them to render, so we can clear them all.
     for (int i = clearBeforeFrame - 1; i >= 0; i--) {
         ImageFrame& buffer = m_frameBufferCache[i];
-        if (buffer.isComplete() || buffer.isInvalid())
+        if (!buffer.isInvalid())
             buffer.clear();
     }