Fix ImageDecoder::frameIsCompleteAtIndex - fully received instead of decoded.

third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp

Index: third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp b/third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
index fae908a78e567ec303b4b967a767735cffe0861c..f8c047cc089b42d01dbc62190ef456402a89b5c0 100644
--- a/third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
@@ -166,14 +166,11 @@ int WEBPImageDecoder::repetitionCount() const
     return failed() ? cAnimationLoopOnce : m_repetitionCount;
 }
 
-bool WEBPImageDecoder::frameIsCompleteAtIndex(size_t index) const
+bool WEBPImageDecoder::frameIsFullyReceivedAtIndex(size_t index) const
 {
     if (!m_demux || m_demuxState <= WEBP_DEMUX_PARSING_HEADER)
-        return false;
-    if (!(m_formatFlags & ANIMATION_FLAG))
-        return ImageDecoder::frameIsCompleteAtIndex(index);
-    bool frameIsLoadedAtIndex = index < m_frameBufferCache.size();
-    return frameIsLoadedAtIndex;
+        return ImageDecoder::frameIsFullyReceivedAtIndex(index);
+    return ((m_formatFlags & ANIMATION_FLAG) && index < m_frameBufferCache.size()) || ImageDecoder::frameIsFullyReceivedAtIndex(index);

[scroggo_chromium, 2016/05/16 20:32:08]

I brought this up previously, but I don't think you've addressed it. Here is my
previous comment:

"""
As I understand it, GIF's version of frameIsFullyReceivedAtIndex tells us that
we have received all the bytes for a frame (assuming it's valid). So if we
decode that frame (maybe we haven't yet) it will end up as FrameComplete without
providing more data (again, assuming it's valid).

Looking at WEBP's decodeFrameCount, it is not obvious that we have received
enough data to mark the frame FrameComplete after a decode.
"""

Said a different way, if we have enough data to decode part of a frame
(FramePartial), WEBP's implementation looks like it will return true, but GIF's
implementation looks like it will return false.

[aleksandar.stojiljkovic, 2016/05/22 15:41:53]

I refactored it a bit, but it is still based on index <
m_frameBufferCache.size().
For multiframe webp, unless there is complete data for the frame, an entry for
it is not added to m_frameBufferCache.

[scroggo_chromium, 2016/05/23 16:40:42]

That's not obvious to me. An entry is created based on
WebPDemuxGetI(WEBP_FF_FRAME_COUNT), which returns WebPDemuxer.num_frames_ [1].
That value is incremented if AddFrame returns true [2]. The comment above [3]
states

"Store a frame only if the animation flag is set there is *some* data for this
frame is available." (emphasis mine)

The comment for AddFrame [4] says it "ensur[es] the last frame is complete", but
based on the code below, it seems to mean the last one prior to adding.

So as far as I can tell, having a frame in m_frameBufferCache does not mean that
the frame is complete.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb...
[3]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb...
[4]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb...

[aleksandar.stojiljkovic, 2016/05/24 11:08:24]

This confused me too. Notice in WEBPImageDecoder::initializeNewFrame [1] assert
stating that new frame added is always complete. WebPDemuxGetFrame is 1-based
since 0-frame relates to last frame.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
void WEBPImageDecoder::initializeNewFrame(size_t index)
{
    if (!(m_formatFlags & ANIMATION_FLAG)) {
        ASSERT(!index);
        return;
    }
    WebPIterator animatedFrame;
    WebPDemuxGetFrame(m_demux, index + 1, &animatedFrame);
    ASSERT(animatedFrame.complete == 1);

Debugged the test and StoreFrame [2] sets complete only for frames where status
== PARSE_OK.
StoreFrame handling of frame_num ( line with frame->frame_num_ = frame_num; ) is
a bit confusing as frame->frame_num > 0 is checked before call to AddFrame() [3]
  
 
Of course, to make this code more robust, we should probably just call
WebPDemuxGetFrame and return complete but I was considering that the
before-mentioned assert is robust enough check (although only in debug mode).

[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb...
[3]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libweb...

[scroggo_chromium, 2016/05/24 13:55:14]

Very confusing. Can you add a comment? 

[aleksandar.stojiljkovic, 2016/05/24 19:24:34]

Done.

 }
 
 float WEBPImageDecoder::frameDurationAtIndex(size_t index) const
@@ -500,7 +497,7 @@ bool WEBPImageDecoder::decodeSingleFrame(const uint8_t* dataBytes, size_t dataSi
         clearDecoder();
         return true;
     case VP8_STATUS_SUSPENDED:
-        if (!isAllDataReceived() && !frameIsCompleteAtIndex(frameIndex)) {
+        if (!isAllDataReceived() && !frameIsFullyReceivedAtIndex(frameIndex)) {
             applyPostProcessing(frameIndex);
             return false;
         }