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);
+
+    // Multi-frame WebP frame gets added to m_frameBufferCache through
+    // initializeNewFrame() only when the frame data is fully received.

[Peter Kasting, 2016/05/25 01:05:23]

Really?  That's not what it looks like to me.  m_frameBufferCache is resized by
ImageDecoder::frameCount(), which calls decodeFrameCount(), which can return a
value larger than the number of frames we have complete data for, AFAICT.

[aleksandar.stojiljkovic, 2016/05/30 16:12:20]

Yes, it is like that. I could work on documenting this further if needed.
You could see it from frameCount [1]:
frameCount() also calls WEBPImageDecoder::initializeNewFrame [2] only for frames
which are completely available and it verifies it by
ASSERT(animatedFrame.complete == 1);

This functionality is specific to multiframe WebP only - that m_frameBufferCache
size is equal to number of completely received frames.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

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

[Peter Kasting, 2016/05/31 03:46:42]

That looks to me like a bug in the webp decoder, honestly.  Doesn't it have the
full projected frame count available near the beginning of the image?  If so I
think we should fix it to return that number as soon as it's available -- as all
the other decoders here do -- and not simply return the number of frames that
have been fully received.  Not only is this more consistent and seems more in
keeping with the spirit of "what is this image's frame count", I would worry
about someone getting a nonzero number from this call and assuming that's the
final number.  I don't know that any code does this today -- I think if we get
new data we may assume all bets are off, including the existing frame count --
but it wouldn't be hard for someone to make that assumption later.

[scroggo_chromium, 2016/06/01 15:11:25]

AFAICT, the GIF decoder does not know its frame count up front either. When it
receives data for another frame it increases its frame count [1].

There is an inconsistency, though - GIF may have partially received frames, but
it appears that WEBP does not.

FWIW, even if the projected frame count *is* at the beginning of the image, what
do we do if it doesn't match the actual number of frames?

- Ignore any extra frames? That seems wrong to me.
- Stop (and loop, if looping is appropriate) if we come up short? (This is what
we do if we do not trust the projected frame count.) Or insert empty frames?

Either way trusting the projected frame count seems wrong to me.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Peter Kasting, 2016/06/01 15:23:49]

Huh.  I was misled by the way there's a separate frame vector for the reader
versus decoder, and things like a "GIF frame count query" enum value, but it
turns out that actually maps to "just decode normally".  I think you're correct.
Yeah.  I'm not sure why that difference exists.  We try to avoid animating to an
incomplete frame so I don't think it should cause any visible effects, but from
a code perspective it would be nice to be consistent.

Perhaps GIF should do what WebP does, and we would be able to remove the
animation code that avoids animating to an incomplete frame?  Admittedly that's
like one line, but whatever.
I agree.

+    return index < m_frameBufferCache.size();
 }
 
 float WEBPImageDecoder::frameDurationAtIndex(size_t index) const
 {
     return index < m_frameBufferCache.size() ? m_frameBufferCache[index].duration() : 0;
 }
 
 bool WEBPImageDecoder::updateDemuxer()
 {
     if (failed())
@@ skipping 318 matching lines @@
             return false;
         }
         // FALLTHROUGH
     default:
         clear();
         return setFailed();
     }
 }
 
 } // namespace blink