Checking if frame is complete and access duration doesn't need a decode

Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp

Index: Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp
diff --git a/Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp b/Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp
index 2aa6c66986d3b55d3d3877d507cbb921a3b02cbf..e8b8fbc4754b11a13ecf65ccd70abd1658b4f097 100644
--- a/Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp
+++ b/Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp
@@ -127,6 +127,26 @@ ImageFrame* GIFImageDecoder::frameBufferAtIndex(size_t index)
     return &frame;
 }
 
+bool GIFImageDecoder::frameIsCompleteAtIndex(size_t index) const
+{
+    if (!m_reader)
+        return false;
+    if (index >= m_reader->imagesCount())
+        return false;
+    return m_reader->frameContext(index)->isComplete();

[Peter Kasting, 2013/04/23 21:43:20]

Nit: Simpler:

    return m_reader && (index < m_reader->imagesCount()) &&
m_reader->frameContext(index)->isComplete();

[Alpha Left Google, 2013/04/25 23:02:44]

Done.

+}
+
+float GIFImageDecoder::frameDurationAtIndex(size_t index) const
+{
+    if (!m_reader)
+        return 0;
+    if (index >= m_reader->imagesCount())
+        return 0;
+    if (m_reader->frameContext(index)->isHeaderDefined())
+        return m_reader->frameContext(index)->delayTime;
+    return 0;

[Peter Kasting, 2013/04/23 21:43:20]

Nit: Simpler:

  return (m_reader && (index < m_reader->imagesCount()) &&
m_reader->frameContext(index)->isHeaderDefined()) ?
m_reader->frameContext(index)->delayTime : 0;

[Alpha Left Google, 2013/04/25 23:02:44]

Done.

+}
+
 bool GIFImageDecoder::setFailed()
 {
     m_reader.clear();
@@ -185,7 +205,7 @@ void GIFImageDecoder::clearFrameBufferCache(size_t clearBeforeFrame)
 
 bool GIFImageDecoder::haveDecodedRow(unsigned frameIndex, const Vector<unsigned char>& rowBuffer, size_t width, size_t rowNumber, unsigned repeatCount, bool writeTransparentPixels)
 {
-    const GIFFrameContext* frameContext = m_reader->frameContext();
+    const GIFFrameContext* frameContext = m_reader->frameContext(frameIndex);
     // The pixel data and coordinates supplied to us are relative to the frame's
     // origin within the entire image size, i.e.
     // (frameContext->xOffset, frameContext->yOffset). There is no guarantee
@@ -297,8 +317,6 @@ void GIFImageDecoder::gifComplete()
     // Cache the repetition count, which is now as authoritative as it's ever
     // going to be.
     repetitionCount();
-
-    m_reader.clear();
 }
 
 void GIFImageDecoder::decode(unsigned haltAtFrame, GIFQuery query)
@@ -335,16 +353,16 @@ void GIFImageDecoder::decode(unsigned haltAtFrame, GIFQuery query)
         return;
     }
 
-    // It is also a fatal error if all data is received but we failed to decode
-    // all frames completely.
-    if (isAllDataReceived() && haltAtFrame >= m_frameBufferCache.size() && m_reader)
+    // It is also a fatal error if all data is received and we have decoded all
+    // frames available but the file is truncated.

[Peter Kasting, 2013/04/23 21:43:20]

The old comment seemed better, honestly, but this also relates to the
implementation issue below.

+    if (isAllDataReceived() && haltAtFrame >= m_frameBufferCache.size() && !m_reader->parseCompleted())

[Peter Kasting, 2013/04/23 21:43:20]

Doesn't this check now only test parsing the GIF, but not actually decoding? 
Seems like we could parse cleanly but encounter a decoding error and we would
never wind up calling setFailed() anymore.

[Alpha Left Google, 2013/04/25 23:02:44]

If it is parse only we'd return at line 349. The if statement is reached only
during decoding. So this statement means that we have not parsed cleanly to the
end, but we allow decoding to the last available frame before reporting an
error.

Note that decoding error is already reported at line 352. This is additional to
stop decoding / animation after decoding the last available frame and file is
truncated. In practice this is the same outcome as the code before.

[Peter Kasting, 2013/04/25 23:22:32]

I understand what is going on now.  My main problem was in misunderstanding the
meaning of "m_parseCompleted", which doesn't actually mean "we succeeded in
parsing", it means "we succeeded in decoding and reached GIFDone state".

I think this would be slightly clearer if, instead of adding a parseCompleted()
accessor on GIFImageReader, you added a private bool m_gifComplete to
GIFImageDecoder, and set it true in gifComplete(), then checked it here.  This
would function the same, but read a little more clearly and allow the
gifComplete() callback to perform all necessary state changes on reaching
GIFDone (as it used to do) rather than requiring us to read back into the
GIFImageReader.

         setFailed();
 }
 
 bool GIFImageDecoder::initFrameBuffer(unsigned frameIndex)
 {
     // Initialize the frame rect in our buffer.
-    const GIFFrameContext* frameContext = m_reader->frameContext();
+    const GIFFrameContext* frameContext = m_reader->frameContext(frameIndex);
     IntRect frameRect(frameContext->xOffset, frameContext->yOffset, frameContext->width, frameContext->height);
 
     // Make sure the frameRect doesn't extend outside the buffer.