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 128 matching lines @@
         decode(index);
         PlatformInstrumentation::didDecodeImage();
     }
 
     frame->notifyBitmapIfPixelsChanged();
     return frame;
 }
 
 bool ImageDecoder::frameHasAlphaAtIndex(size_t index) const
 {
-    return !frameIsCompleteAtIndex(index) || m_frameBufferCache[index].hasAlpha();
+    return index >= m_frameBufferCache.size() || m_frameBufferCache[index].hasAlpha();

[Peter Kasting, 2016/05/10 00:00:13]

Will hasAlpha() return true on partially-decoded frames that don't have alpha in
the decoded part?  If not seems like this is less correct than before.

[scroggo_chromium, 2016/05/10 14:14:37]

I think that is the intent. When we create a new ImageFrame, we set m_hasAlpha
(which this method returns) to true [1]. Some of our decoders (BMP, PNG)
setHasAlpha(false) immediately after that [2][3]. BMP has comments that it will
set it back to true if it sees a transparent pixel [4], but it is not obvious to
me that either one sets it back to true in the case of an incomplete image.

My understanding is that JPEG does the right thing - it sets hasAlpha to true
initially [5] (unnecessarily, though, since setSize already did it) and then
sets it to false on completion [6]. GIF [7] and WEBP [8] also seem to do the
right thing.

I think PNG and BMP actually have bugs here, which get exposed by this change.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
[2] BMP:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
[3] PNG:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
[4]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
[5]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
[6]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
[7]https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp&l=195&rcl=1462864689
/
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
[8]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[aleksandar.stojiljkovic, 2016/05/10 21:59:31]

I disliked that we are handling "decoding partial received content has
transparent background" feature here.
Anyway, it doesn't look OK to use neither of new methods;
frameIsFullyReceivedAtIndex nor frameIsCompleteAtIndex.
frameIsFullyReceivedAtIndex has problem with PNG when partially downloaded -
(PNG sets hasAlpha = false).
frameIsCompleteAtIndex doesnt let cached hasAlpha from m_frameBufferCache for
"cleared pixeldata" frames.

So, need to check PNG & BMP decoders.

[Peter Kasting, 2016/05/11 00:33:02]

Me too, frankly.  I don't think that's really something the image decoder itself
should deal with.  To me, whether the image has alpha should reflect whether the
decoded image data has any partly-transparent pixels; separately, this code, or
some other code that's not specific to each individual decoder, could check if
the frame status is not yet FrameComplete, and if it's not, assume that the
drawn frame will always have alpha (in the non-decoded portions).

[scroggo_chromium, 2016/05/11 15:17:34]

Is there an existing caller that needs to know whether the decoded portion has
alpha (as opposed to the whole bitmap)? It seems like the client needs to know
whether there is alpha anywhere in the bitmap, since that affects
blending/elements behind it showing etc.

FWIW, I actually proposed that the image decoder report the alpha in the decoded
portion in crbug.com/593430. In order for that to work properly, the client
needs to only draw the portion of the bitmap that has been decoded. The deferred
decoding pipeline is not set up to do that.

Which reminds me - the use of this method is already pretty broken. It is only
called by DeferredImageDecoder, which queries m_actualDecoder, which will never
return false (since m_actualDecoder is not used for decoding).
(DeferredImageDecoder's version can return false once all data is received and
we throw away m_actualDecoder, so it will check with m_frameGenerator for a
multi frame image.)

In principle, I'm fine with making the ImageDecoder keep track of whether it's
seen alpha*, although I would argue for a more specific name (instead of
hasAlpha(), something like "hasSeenAlpha()" or "decodedPortionHasAlpha()"). We
also need to add tests that verify that we actually get that behavior and state
clearly that it's what we want. As aleksandar points out, we *used to* behave
that way, but some decoders switched.

* Assuming there's a way for the caller to determine whether the frame is
complete. DeferredImageDecoder, for example, does not know. It could guess based
on how much data was received, but that's no guarantee.

 }
 
-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;
 }
 
 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