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

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

 /*
  * Copyright (C) Research In Motion Limited 2009-2010. All rights reserved.
  *
  *  This library is free software; you can redistribute it and/or
  *  modify it under the terms of the GNU Library General Public
  *  License as published by the Free Software Foundation; either
  *  version 2 of the License, or (at your option) any later version.
  *
  *  This library is distributed in the hope that it will be useful,
  *  but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 126 matching lines @@
         decode(index);
         PlatformInstrumentation::didDecodeImage();
     }
 
     frame->notifyBitmapIfPixelsChanged();
     return frame;
 }
 
 bool ImageDecoder::frameHasAlphaAtIndex(size_t index) const
 {
-    return !frameIsCompleteAtIndex(index) || m_frameBufferCache[index].hasAlpha();
+    if (m_frameBufferCache.size() == 1)
+        return !frameIsCompleteAtIndex(index) || m_frameBufferCache[index].hasAlpha();
+    return !frameIsFullyReceivedAtIndex(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);
+    return (m_isAllDataReceived || failed()) && index < m_frameBufferCache.size();

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

This implementation worries me.

(1) If we call this without having first called frameCount() to update the frame
buffer cache size, then this might return false if |index| is too large, even
when it should actually return true.
(2) If the image is a multi-frame ICO, you don't override this method in the
subclass decoder, and so I think this can return false even when it should
return true, as long as we haven't received _all_ the data yet.

(2) is probably fixable by implementing an ICO-specific version that checks if
the data length is large enough to cover the desired frame's data region (the
ICO has a directory that should provide this info).  Of course, to know that, we
need to have decoded at least the directory.  This basically reduces to case
(1).

This means that in general this method is probably unsafe to call unless either
it does partial image decoding, or all callers decode before calling it.  That
seems like a critical problem in the API, as I think part of the goal of this
method was to be useful in cases where the caller didn't do, or want, any
decoding.

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

The pattern of calling frameCount() looks like the design choice here - to split
the logic between clients (ImageSource, DeferredImageDecoder) and
ImageDecoder::frameCount().
Parsing for frame completeness happens under frameCount()(which is invoking
virtual decodeFrameCount).
In case of frameBufferAtImdex, frameCount is called on start of API call.
In all other cases, as clients are calling APIs after frameCount() there is no
need to call it from within the ImageDecoder API calls.
As ICOs are opted out [1] of deferred image decoding, it is important to put
back original condition: m_frameBufferCache[index].getStatus() ==
ImageFrame::FrameComplete.
scroggo@, we have discussed about this related to WebP in previous patch - here
the change is for different reason (direct decoding).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
As ICOs are opted out, this is related to direct decoding and restoring the
condition existing in original code should make things right (assuming we don't
want to implement deferred decoding support for ICO).
Not sure if you are referring to ICO case/direct (non-deferred) decoding here or
to all decoders implementation.
Under frameCount -> virtual decodeFrameCount call, WebP and GIF are doing
parsing and there is no need for decoding before
frameIsFullyReceivedAtIndex(size_t index).

[Peter Kasting, 2016/05/31 03:28:26]

I guess what I'm saying is that it's dangerous to implement an API that is
callable in a variety of cases but happens to be silently incorrect in some of
them.  Just because we happen to meet the right criteria today doesn't prevent
us from refactoring later and goofing this up.  The bare minimum would be some
kind of comment about the prerequisites of the API.  Better would be some kind
of set of assertions that would prevent incorrect call order, and better still
might be some system that transparently ensures all prerequisite decoding etc.
has happened, without unnecessarily duplicating effort.
That's bizarre and seems wrong.  Why are ICOs excluded here?  We should fix
that, unless there's some really good reason I'm missing.

[scroggo_chromium, 2016/06/01 15:11:25]

We have a bug filed for the ICO exception: crbug.com/513306. It appears that ICO
is excluded just because no one has added it.

[Peter Kasting, 2016/06/01 15:23:49]

Good find.

It would be nice to get that fixed.  Failing that, though, I would write this
patch assuming that that will eventually be fixed, and so being prepared for the
potential consequences.

[aleksandar.stojiljkovic, 2016/06/01 16:21:33]

I spent some time thinking about this after initial opinion how I wouldn't like
to change it.
Makes sense to do it - we could flag "the change happened" on
ImageDecoder::setData and then call frameCount on start of 
frameIsFullyReceivedAtIndex (early exit if setData wasn't called). This way we
would make WebPImageDecoder::m_haveAlreadyParsedThisData functionality common
for all the decoders.

[aleksandar.stojiljkovic, 2016/06/01 16:21:33]

I'll take the ICO case separately - starting work on it.
Suggest to scope this patch here to refactoring only and add ICO deferred
decoding support including all the tests (e.g. bytebybyte parse and decode) in
separate patch. Tests are missing for ICO - doing any functionality change would
be risky. I understand that we might disagree that this patch actually changes
functionality already but with current set of tests I see that covered. 

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

Done - assertion added.
Calling non const (e.g.frameCount) internally lead to making all of the methods
non const and as an overhead.
On the other side, frameCount is documented as non const that is parsing and
decoding frame count so it looked OK.

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

Changed my mind - an attempt to implement ICO support is in Patch 12# Deferred
decoding for ICO.

 }
 
 size_t ImageDecoder::frameBytesAtIndex(size_t index) const
 {
     if (index >= m_frameBufferCache.size() || m_frameBufferCache[index].getStatus() == ImageFrame::FrameEmpty)
         return 0;
 
     struct ImageSize {
 
         explicit ImageSize(IntSize size)
@@ skipping 192 matching lines @@
 
     qcms_data_type dataFormat = hasAlpha ? QCMS_DATA_RGBA_8 : QCMS_DATA_RGB_8;
 
     // FIXME: Don't force perceptual intent if the image profile contains an intent.
     m_sourceToOutputDeviceColorTransform = adoptPtr(qcms_transform_create(inputProfile.get(), dataFormat, gOutputDeviceProfile, QCMS_DATA_RGBA_8, QCMS_INTENT_PERCEPTUAL));
 }
 
 #endif // USE(QCMSLIB)
 
 } // namespace blink