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

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

Index: third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp b/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
index 663f0d5d1ede08172707f54097054bba8349cca9..84b75ef8b211fa647ae06f0230926f2522dfe25a 100644
--- a/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
@@ -146,13 +146,14 @@ ImageFrame* ImageDecoder::frameBufferAtIndex(size_t index)
 
 bool ImageDecoder::frameHasAlphaAtIndex(size_t index) const
 {
-    return !frameIsCompleteAtIndex(index) || m_frameBufferCache[index].hasAlpha();
+    return index >= m_frameBufferCache.size()
+        || m_frameBufferCache[index].getStatus() == ImageFrame::FramePartial

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

As I still didn't fix BMPImageDecoder alpha handling leaving
m_frameBufferCache[index].getStatus() == ImageFrame::FramePartial here - to
cover previous functionality.

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

FWIW, you didn't fix PNGImageDecoder (or, you fixed it the other direction), and
you left the others unchanged. I think BMPImageDecoder matches the behavior you
and pkasting@ proposed.

I also think it's awkward here that frameHasAlphaAtIndex(i) may return a
different value from another way a client could attempt to find this info:
frameBufferAtIndex(i).hasAlpha()

[Peter Kasting, 2016/05/11 18:29:34]

I think you want "!= ImageFrame::FrameComplete", because the frame has alpha
when empty as well.
Meaning instead of doing this either here or in the decoders, we should do it in
ImageFrame::hasAlpha()?  I'd be OK with that.  As long as the individual
decoders aren't accounting for this, any solution is OK by me.

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

Now the patch fixed the former PNG and BMP decoder behavior (to match others):
on start, all frames are
transparent. When the frame decoding is complete, mark
ImageFrame::setHasAlpha(seenPixelsWithAlpha) (in every decoder separately for
now).
Complete/Partial is now removed from this check. Still the decoders are
calculating "has seen alpha" and then keeping ImageFrame::hasAlpha up to date.
If I understand correctly, you'd prefer to get rid of
ImageDecoder::frameHasAlphaAtIndex and always get this from ImageFrame? If so, I
could do this as part of another patch, maybe [1] bellow.

After scroggo@ pointed that frameHasAlphaAtIndex has been returning true we
should check this:

1) What benefit is the "has seen alpha" approach, where decoder checks every
pixel, bringing now?
2) On a related note, I have started approach here [1] to early return alpha
(before decoding).

Maybe alternative approach to one we have so far would be to remove all "has
seen alpha" calculation and return only the calculation from [1] always.


[1] https://crrev.com/1972683002: Early reporting of opaqueness by decoders.

[scroggo_chromium, 2016/05/16 20:32:08]

My preference is that *if* we have two separate methods, they both return the
same answer. That can be accomplished by having the impl for
frameHasAlphaAtIndex look like this:

return index >= m_frameBufferCache.size() ||
m_frameBufferCache[index].hasAlpha();
Well, the general system (for getting the alpha state back to the client) is
broken, so I would guess none. My real question is whether switching to what you
and pkasting@ thought made sense (i.e. decoder does not worry about whether the
frame is complete) would give us benefit. I think we'd need to update more code
before we can see a benefit.

[Peter Kasting, 2016/05/16 20:35:53]

What I want is for the following two things to be considered completely
distinct:

(1) Whether the decoded pixels in the image have alpha
(2) Whether there is undecoded region within the image

Keeping these separate means the decoders can consider only (1), making them
simpler to understand and less error-prone.  But the drawing subsystem that
wants to know if the bitmap is fully opaque has to know about both.

Perhaps the way to achieve this while still respecting Leon's concerns is to
name the functions which provide these functions differently, e.g.:

(1) ImageDecoder::DecodedPixelsHaveAlpha()
(2) ImageFrame::IsFullyOpaque()

Or something to that effect.

[scroggo_chromium, 2016/05/16 20:40:56]

Yes, that sounds good to me.

+        || 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;
 }
 
 size_t ImageDecoder::frameBytesAtIndex(size_t index) const