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

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "platform/image-decoders/ImageDecoderTestHelpers.h"
 
 #include "platform/SharedBuffer.h"
 #include "platform/image-decoders/ImageDecoder.h"
 #include "platform/image-decoders/ImageFrame.h"
 #include "platform/testing/UnitTestHelpers.h"
@@ skipping 68 matching lines @@
         sourceData[1]->append(source++, 1u);
         // Alternate the buffers to cover the JPEGImageDecoder::onSetData restart code.
         decoder->setData(sourceData[length & 1].get(), length == data->size());
 
         EXPECT_LE(frameCount, decoder->frameCount());
         frameCount = decoder->frameCount();
 
         if (!decoder->isSizeAvailable())
             continue;
 
+        bool frameIsFullyReceived = decoder->frameIsFullyReceivedAtIndex(frameCount - 1);
         ImageFrame* frame = decoder->frameBufferAtIndex(frameCount - 1);
+
+        // FIXME: libwebp parsing of crbug.364830.webp doesn't mark the frame

[scroggo_chromium, 2016/05/24 19:34:11]

We know the file name. Should we special case further to the file?

[aleksandar.stojiljkovic, 2016/06/06 14:26:38]

Written the check differently.

+        // as complete two bytes before the end of the file. However, decoding
+        // it to the same point produces completely decoded frame.
+        if (decoder->filenameExtension() != "webp" || frameIsFullyReceived)
+            EXPECT_EQ(frameIsFullyReceived, decoder->frameIsCompleteAtIndex(frameCount - 1) || decoder->failed());

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

Isn't this test fundamentally incorrect?  It's checking that
frameIsCompleteAtIndex() changes value at the same time as
frameIsFullyReceivedAtIndex().  But it's possible for a frame to be complete
before it's fully received, if there's garbage data after the frame that the
decoder doesn't know in advance isn't needed to decode the frame.

The only way we could actually do this check, I think, is for
frameIsFullyReceivedAtIndex() to try to decode the frame and return true if the
status was FrameComplete.  Which I don't think you want to do, so I'd remove
this.

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

This test checks that frameIsFullyReceivedAtIndex before calling decode frame
(i.e. decoder->frameBufferAtIndex) returns the same value as
frameIsCompleteAtIndex() after decode.
It was missing before and it is important as it is verifying that clients get
valid results for (frameCount + frameIsFullyReceivedAtIndex) before decoding
frames. 

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

If you want to check that, isn't the correct test simply:

  if (frameIsFullyReceived)
    EXPECT_TRUE(decoder->frameIsCompleteAtIndex(frameCount - 1) ||
decoder->failed());

The remainder of the check is incorrect in the case I mentioned, or if decoding
were to fail before the frame is fully received.

[aleksandar.stojiljkovic, 2016/06/01 22:47:35]

For other than WebP it is important to check that also false means that there is
not enough data to produce complete frame on decoding.

 

[aleksandar.stojiljkovic, 2016/06/01 22:47:35]

Decoder get to know this in setData() which is called before decode. And after
setData, frameCount is called to parse.
I don't know how it is possible that the frame is complete before it is fully
received (except if there was a failure).

[aleksandar.stojiljkovic, 2016/06/06 14:26:38]

 You have the point here. Written the the check like this - my argument how this
was useful for GIF was wrong since GIFImageDecoderTest is having separate
byteByByteDecode implementation.

+
         if (frame && frame->getStatus() == ImageFrame::FrameComplete && framesDecoded < frameCount)
             ++framesDecoded;
     }
 
     EXPECT_FALSE(decoder->failed());
     EXPECT_EQ(expectedFrameCount, decoder->frameCount());
     EXPECT_EQ(expectedFrameCount, framesDecoded);
     EXPECT_EQ(expectedRepetitionCount, decoder->repetitionCount());
 
     ASSERT_EQ(expectedFrameCount, baselineHashes.size());
@@ skipping 32 matching lines @@
     // data in a segment, its pointer would no longer be valid.
     segmentedData->data();
 
     ImageFrame* frame = decoder->frameBufferAtIndex(0);
     ASSERT_FALSE(decoder->failed());
     EXPECT_EQ(frame->getStatus(), ImageFrame::FrameComplete);
     EXPECT_EQ(hashBitmap(frame->bitmap()), hash);
 }
 
 } // namespace blink