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

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

 /*
  * Copyright (C) 2010 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
@@ skipping 148 matching lines @@
 void WEBPImageDecoder::onSetData(SegmentReader*)
 {
     m_haveAlreadyParsedThisData = false;
 }
 
 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
 {
     return index < m_frameBufferCache.size() ? m_frameBufferCache[index].duration() : 0;
 }
 
 bool WEBPImageDecoder::updateDemuxer()
 {
     if (failed())
@@ skipping 306 matching lines @@
     m_decoderBuffer.u.RGBA.rgba = reinterpret_cast<uint8_t*>(buffer.getAddr(frameRect.x(), frameRect.y()));
 
     switch (WebPIUpdate(m_decoder, dataBytes, dataSize)) {
     case VP8_STATUS_OK:
         applyPostProcessing(frameIndex);
         buffer.setHasAlpha((m_formatFlags & ALPHA_FLAG) || m_frameBackgroundHasAlpha);
         buffer.setStatus(ImageFrame::FrameComplete);
         clearDecoder();
         return true;
     case VP8_STATUS_SUSPENDED:
-        if (!isAllDataReceived() && !frameIsCompleteAtIndex(frameIndex)) {
+        if (!isAllDataReceived() && !frameIsFullyReceivedAtIndex(frameIndex)) {
             applyPostProcessing(frameIndex);
             return false;
         }
         // FALLTHROUGH
     default:
         clear();
         return setFailed();
     }
 }
 
 } // namespace blink