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

third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp

Index: third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp b/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
index bff97c205e1780f9f60392caac16f80acff9d86d..9fc0f002136d7935b2213bdeb5cf2ce09d20781f 100644
--- a/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
@@ -163,6 +163,7 @@ private:
 PNGImageDecoder::PNGImageDecoder(AlphaOption alphaOption, GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes, size_t offset)
     : ImageDecoder(alphaOption, colorOptions, maxDecodedBytes)
     , m_offset(offset)
+    , m_hasPixelsWithAlpha(false)
 {
 }
 
@@ -314,7 +315,6 @@ void PNGImageDecoder::rowAvailable(unsigned char* rowBuffer, unsigned rowIndex,
         }
 #endif
         buffer.setStatus(ImageFrame::FramePartial);
-        buffer.setHasAlpha(false);

[aleksandar.stojiljkovic, 2016/05/11 11:05:16]

The code was like this in all of the decoders [1] but meanwhile partially
decoded got alpha apparently and it was fixed in other decoders except PNG and
BMP. PNG should be OK now.

[1] @pkasting's patch at 2009
https://chromium.googlesource.com/chromium/src/+/f09863db44eed54d690e52815adb...

 
         // For PNGs, the frame always fills the entire image.
         buffer.setOriginalFrameRect(IntRect(IntPoint(), size()));
@@ -401,8 +401,8 @@ void PNGImageDecoder::rowAvailable(unsigned char* rowBuffer, unsigned rowIndex,
         }
     }
 
-    if (alphaMask != 255 && !buffer.hasAlpha())
-        buffer.setHasAlpha(true);
+    if (alphaMask != 255 && !m_hasPixelsWithAlpha)
+        m_hasPixelsWithAlpha = true;

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

So now it will have alpha until fully decoded (when it may change to opaque)? I
thought you and pkasting@ agreed that was the wrong behavior? And why did you
leave BMP alone but "fix" PNG?

[aleksandar.stojiljkovic, 2016/05/16 13:09:49]

Seemed like a bug, that the state in ImageFrame is not consistent for partially
decoded content - so attempting to fix it here.
About handling alpha state, I'll answer to original pkasting@'s comment.
I just didn't do BMP part then yet - fixed in this patch.

 
     buffer.setPixelsChanged(true);
 }
@@ -412,21 +412,19 @@ void PNGImageDecoder::complete()
     if (m_frameBufferCache.isEmpty())
         return;
 
+    m_frameBufferCache[0].setHasAlpha(m_reader->hasAlpha() && m_hasPixelsWithAlpha);

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

I think it's redundant here to check m_reader->hasAlpha()

[aleksandar.stojiljkovic, 2016/05/16 13:09:49]

Done.

     m_frameBufferCache[0].setStatus(ImageFrame::FrameComplete);
 }
 
-inline bool isComplete(const PNGImageDecoder* decoder)
-{
-    return decoder->frameIsCompleteAtIndex(0);
-}
-
 void PNGImageDecoder::decode(bool onlySize)
 {
     if (failed())
         return;
 
-    if (!m_reader)
+    if (!m_reader) {
         m_reader = adoptPtr(new PNGImageReader(this, m_offset));
+        m_hasPixelsWithAlpha = false;

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

Why did you need to set this to false? Isn't it set to false in the constructor?

[aleksandar.stojiljkovic, 2016/05/16 13:09:49]

I thought it was safer to reset it for every new reader (in case it was reused
for different data) but now saw it is not reused in ICO decoder either. Thanks.
Removed.

+    }
 
     // If we couldn't decode the image but have received all the data, decoding
     // has failed.
@@ -434,7 +432,7 @@ void PNGImageDecoder::decode(bool onlySize)
         setFailed();
 
     // If decoding is done or failed, we don't need the PNGImageReader anymore.
-    if (isComplete(this) || failed())
+    if (frameIsCompleteAtIndex(0) || failed())
         m_reader.clear();
 }