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

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

Index: third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp b/third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp
index 3c51d4d721e9ce215e416a24110b7697c9c91a37..01d484322822cb2ad66ca4e6f5d44af4264df82d 100644
--- a/third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/ImageDecoderTestHelpers.cpp
@@ -86,7 +86,15 @@ void testByteByByteDecode(DecoderCreator createDecoder, const char* file, size_t
         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;
     }