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

third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp

Index: third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp b/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
index 70de78249db332997998e3edd75e1cd5195529e1..c8383a0fa48e37c0d07b2644c18460b9280b4bb9 100644
--- a/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
@@ -114,6 +114,9 @@ PassOwnPtr<ImageDecoder> ImageDecoder::create(const SegmentReader& data, AlphaOp
 
 size_t ImageDecoder::frameCount()
 {
+    if (m_haveUpdatedFrameCount)
+        return m_frameBufferCache.size();
+
     const size_t oldSize = m_frameBufferCache.size();
     const size_t newSize = decodeFrameCount();
     if (oldSize != newSize) {
@@ -123,6 +126,7 @@ size_t ImageDecoder::frameCount()
             initializeNewFrame(i);
         }
     }
+    m_haveUpdatedFrameCount = true;
     return newSize;
 }
 
@@ -142,15 +146,11 @@ ImageFrame* ImageDecoder::frameBufferAtIndex(size_t index)
     return frame;
 }
 
-bool ImageDecoder::frameHasAlphaAtIndex(size_t index) const
-{
-    return !frameIsCompleteAtIndex(index) || m_frameBufferCache[index].hasAlpha();
-}
-
-bool ImageDecoder::frameIsCompleteAtIndex(size_t index) const
+bool ImageDecoder::frameIsFullyReceivedAtIndex(size_t index) const
 {
-    return (index < m_frameBufferCache.size()) &&
-        (m_frameBufferCache[index].getStatus() == ImageFrame::FrameComplete);
+    ASSERT(m_haveUpdatedFrameCount);
+    return (index < m_frameBufferCache.size())
+        && (m_isAllDataReceived || failed() || m_frameBufferCache[index].getStatus() == ImageFrame::FrameComplete);

[Peter Kasting, 2016/06/10 00:10:02]

If we know all decoders report only fully-received frames in the frame count, we
could simplify this function down to just checking if the frame buffer cache was
large enough.

If we don't know that, then this implementation basically works, but risks false
negatives if we've decoded enough to get an updated frame count, but haven't
actually decoded the data of some of those new frames.  There's no way around
this unless you want to call frameBufferAtIndex() to force a decode.  It's not
obvious to me which one callers would want.  We should strive to make sure these
sorts of caveats are very clear in the API documentation.

[aleksandar.stojiljkovic, 2016/06/10 21:51:08]

For GIF and single frame (e.g. JPEG) we return partial (first frame of
animation) in frame count. 
False negatives are expected and "|| m_frameBufferCache[index].getStatus() ==
ImageFrame::FrameComplete" is just a best effort in case decoding happened. On
the other hand, id the frame was cleared from cache after this, it is possible
that for the same index we return both true and false. So, to keep it consistant
it is better to remove "getStatus() == ImageFrame::FrameComplete" for multiframe
decoders - single frame decoders don't remove frames from cache.

[Peter Kasting, 2016/06/10 22:25:03]

I know, but it sounded on another CL like there was a possibility of unifying
all the behaviors here.

(I think returning partial frames in the frame count is probably better, though,
so that would likely be what I'd unify on.)
APIs that are expected to be wrong make me worried.  These lead to confusing,
hard-to-understand code later because the code doesn't actually mean what it
naively reads as meaning.

This is why I've been looking so hard for ways to make these bulletproof, and
add assertions around all the edges.

If we can, I'd prefer to make the implementation "just work"; where that isn't
the right thing to do, we have to make it extremely clear to callers what the
caveats are.
Do we actually delete frames from the cache entirely?  It seems like that's
overkill; we could just drop the ref to the pixel data and keep the metadata. 
That would avoid this issue.
Moving in the direction of even-less-accurate results seems like the wrong way
to go...

 }
 
 size_t ImageDecoder::frameBytesAtIndex(size_t index) const
@@ -171,6 +171,14 @@ size_t ImageDecoder::frameBytesAtIndex(size_t index) const
     return ImageSize(frameSizeAtIndex(index)).area * sizeof(ImageFrame::PixelData);
 }
 
+bool ImageDecoder::frameHasAlphaAtIndex(size_t index) const
+{
+    ASSERT(m_haveUpdatedFrameCount);
+    if (m_frameBufferCache.size() == 1)
+        return !frameIsCompleteAtIndex(index) || m_frameBufferCache[index].hasAlpha();
+    return !frameIsFullyReceivedAtIndex(index) || m_frameBufferCache[index].hasAlpha();

[Peter Kasting, 2016/06/10 00:10:02]

This implementation confuses me.  I don't understand why we could ever rely on
frameIsFullyReceivedAtIndex() instead of FrameIsCompleteAtIndex().  For example,
if the last frame is corrupt, "fully received" will return true due to the
failed() check, but the frame is incomplete, and we should always report it as
having alpha even if the decoding code didn't see alpha up to that point.

So it seems like the right thing here is

  return (index < m_frameBufferCache.size()) &&
         (!frameIsCompleteAtIndex(index) ||
          m_frameBufferCache[index].hasAlpha());

(The first line could be omitted if we were OK with returning true for frames
past the end, but it seems like the convention is to return false from all these
oracles in this case.)

[aleksandar.stojiljkovic, 2016/06/10 21:51:08]

I think that adding failed() to fullyReceived is causing the inconsistency here.
If it is fullyreceived (but not failed) then method is OK. Maybe we remove
failed from frameIsFullyReceivedAtIndex and client calls both
frameIsFullyReceivedAtIndex and failed() if needed to know that there is no more
changes pending to frame encoded data.

[Peter Kasting, 2016/06/10 22:25:03]

I don't understand why we would ever want to toggle between Complete and
FullyReceived, regardless of whether either one includes failed() in its check.

What's wrong with my proposed implementation?
Once again, this makes the API weirder and harder to use and demands that
clients "do it right".  In general we should be doing our utmost to not do that.

+}
+
 bool ImageDecoder::deferredImageDecodingEnabled()
 {
     return DeferredImageDecoder::enabled();