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

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

 /*
  * Copyright (C) 2006 Apple Computer, 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
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 109 matching lines @@
 
     ImageFrame& frame = m_frameBufferCache[index];
     if (frame.status() != ImageFrame::FrameComplete) {
         PlatformInstrumentation::willDecodeImage("GIF");
         decode(index + 1, GIFFullQuery);
         PlatformInstrumentation::didDecodeImage();
     }
     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();
     return ImageDecoder::setFailed();
 }
 
 void GIFImageDecoder::clearFrameBufferCache(size_t clearBeforeFrame)
 {
     // In some cases, like if the decoder was destroyed while animating, we
     // can be asked to clear more frames than we currently have.
@@ skipping 38 matching lines @@
     // Now |i| holds the last frame we need to preserve; clear prior frames.
     for (Vector<ImageFrame>::iterator j(m_frameBufferCache.begin()); j != i; ++j) {
         ASSERT(j->status() != ImageFrame::FramePartial);
         if (j->status() != ImageFrame::FrameEmpty)
             j->clearPixelData();
     }
 }
 
 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
     // that width == (size().width() - frameContext->xOffset), so
     // we must ensure we don't run off the end of either the source data or the
     // row's X-coordinates.
     int xBegin = upperBoundScaledX(frameContext->xOffset);
     int yBegin = upperBoundScaledY(frameContext->yOffset + rowNumber);
     int xEnd = lowerBoundScaledX(std::min(static_cast<int>(frameContext->xOffset + width), size().width()) - 1, xBegin + 1) + 1;
     int yEnd = lowerBoundScaledY(std::min(static_cast<int>(frameContext->yOffset + rowNumber + repeatCount), size().height()) - 1, yBegin + 1) + 1;
@@ skipping 91 matching lines @@
     }
 
     return true;
 }
 
 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)
 {
     if (failed())
         return;
 
     if (!m_reader) {
         m_reader = adoptPtr(new GIFImageReader(this));
         m_reader->setData(m_data);
@@ skipping 16 matching lines @@
         m_frameBufferCache[i].setPremultiplyAlpha(m_premultiplyAlpha);
 
     if (query == GIFFrameCountQuery)
         return;
 
     if (!m_reader->decode(GIFFullQuery, haltAtFrame)) {
         setFailed();
         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.
     if (frameRect.maxX() > size().width())
         frameRect.setWidth(size().width() - frameContext->xOffset);
     if (frameRect.maxY() > size().height())
         frameRect.setHeight(size().height() - frameContext->yOffset);
 
     ImageFrame* const buffer = &m_frameBufferCache[frameIndex];
     int left = upperBoundScaledX(frameRect.x());
@@ skipping 53 matching lines @@
 
     // Update our status to be partially complete.
     buffer->setStatus(ImageFrame::FramePartial);
 
     // Reset the alpha pixel tracker for this frame.
     m_currentBufferSawAlpha = false;
     return true;
 }
 
 } // namespace WebCore