Checking if frame is complete and access duration doesn't need a decode

Checking if frame is complete and access duration doesn't need a decode

The goal of this change is to minimize image decoding on the main thread.

This change is to avoid image decoding for these two operations:
1. frameIsCompleteAtIndex
2. frameDurationAtIndex
These two operations are moved to ImageDecoder interface and are now const
to prevent future regression.

We are now able to check if a frame is complete by parsing the entire GIF file
without decoding. This also provides information like frame duration such that
controller the animation doesn't require any decoding.

This change is critical to moving animated image decoding off the main thread.

BUG=169377, 181321
TEST=(1) Unit test in GIFImageDecoderTest to test change in decoder behavior.
     (2) Image animation cannot be tested reliable via layout test and is
         tested manually with wikigifs.org.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=149883

[Alpha Left Google, 2013-04-20 01:17:24]

senorblanco: Please look at platform/graphics/*
pkasting: Please look at GIF* including decoder and tests.

[Peter Kasting, 2013-04-23 21:43:20]

https://codereview.chromium.org/14317005/diff/3001/Source/WebKit/chromium/tes...
File Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp (right):

https://codereview.chromium.org/14317005/diff/3001/Source/WebKit/chromium/tes...
Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp:251: RefPtr<SharedBuffer>
tempData = SharedBuffer::create(data->data(), data->size() - 10);
Maybe we should ASSERT_GE(data->size(), 10U) before this.  (2 places)

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/graph...
File Source/core/platform/graphics/ImageSource.cpp (right):

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/graph...
Source/core/platform/graphics/ImageSource.cpp:181: if (!m_decoder)
Nit: Shorter:

    return !m_decoder || m_decoder->frameHasAlphaAtIndex(index);

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/graph...
Source/core/platform/graphics/ImageSource.cpp:188: if (!m_decoder)
Nit: Shorter:

    return m_decoder && m_decoder->frameIsCompleteAtIndex(index);

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/graph...
File Source/core/platform/graphics/ImageSource.h (right):

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/graph...
Source/core/platform/graphics/ImageSource.h:132: bool
frameIsCompleteAtIndex(size_t) const; // Whether or not the frame is completely
decoded or fully received.
Nit: Shouldn't this just be "...is fully received"?  We don't care about the
decode state anymore.

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/graph...
File Source/core/platform/graphics/chromium/DeferredImageDecoder.cpp (right):

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/graph...
Source/core/platform/graphics/chromium/DeferredImageDecoder.cpp:191: return
m_actualDecoder ? m_actualDecoder->frameIsCompleteAtIndex(index) : false;
Nit: Simpler:

    return m_actualDecoder && m_actualDecoder->frameIsCompleteAtIndex(index);

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
File Source/core/platform/image-decoders/ImageDecoder.h (right):

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:297: // This method is used
by animated images only.
I'm not terribly comfortable with this comment.  It may be currently true that
we only wind up calling this during animation (I'm not actually sure), but I
think the API of this class should support calling this for any image, meaning
we should provide a real implementation in the .cpp file.

Doing so should be easy and will greatly simplify the frameHasAlphaAtIndex()
implementation which basically implements the "frame complete?" check inline as
most of the function.

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
File Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp (right):

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:136: return
m_reader->frameContext(index)->isComplete();
Nit: Simpler:

    return m_reader && (index < m_reader->imagesCount()) &&
m_reader->frameContext(index)->isComplete();

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:147: return 0;
Nit: Simpler:

  return (m_reader && (index < m_reader->imagesCount()) &&
m_reader->frameContext(index)->isHeaderDefined()) ?
m_reader->frameContext(index)->delayTime : 0;

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:357: // frames
available but the file is truncated.
The old comment seemed better, honestly, but this also relates to the
implementation issue below.

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:358: if
(isAllDataReceived() && haltAtFrame >= m_frameBufferCache.size() &&
!m_reader->parseCompleted())
Doesn't this check now only test parsing the GIF, but not actually decoding? 
Seems like we could parse cleanly but encounter a decoding error and we would
never wind up calling setFailed() anymore.

[Alpha Left Google, 2013-04-25 23:02:44]

https://codereview.chromium.org/14317005/diff/3001/Source/WebKit/chromium/tes...
File Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp (right):

https://codereview.chromium.org/14317005/diff/3001/Source/WebKit/chromium/tes...
Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp:251: RefPtr<SharedBuffer>
tempData = SharedBuffer::create(data->data(), data->size() - 10);
On 2013/04/23 21:43:20, Peter Kasting wrote:
> Maybe we should ASSERT_GE(data->size(), 10U) before this.  (2 places)

Done.

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/graph...
File Source/core/platform/graphics/ImageSource.cpp (right):

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/graph...
Source/core/platform/graphics/ImageSource.cpp:181: if (!m_decoder)
On 2013/04/23 21:43:20, Peter Kasting wrote:
> Nit: Shorter:
> 
>     return !m_decoder || m_decoder->frameHasAlphaAtIndex(index);

Done.

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/graph...
Source/core/platform/graphics/ImageSource.cpp:188: if (!m_decoder)
On 2013/04/23 21:43:20, Peter Kasting wrote:
> Nit: Shorter:
> 
>     return m_decoder && m_decoder->frameIsCompleteAtIndex(index);

Done.

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/graph...
File Source/core/platform/graphics/ImageSource.h (right):

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/graph...
Source/core/platform/graphics/ImageSource.h:132: bool
frameIsCompleteAtIndex(size_t) const; // Whether or not the frame is completely
decoded or fully received.
On 2013/04/23 21:43:20, Peter Kasting wrote:
> Nit: Shouldn't this just be "...is fully received"?  We don't care about the
> decode state anymore.

Done.

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/graph...
File Source/core/platform/graphics/chromium/DeferredImageDecoder.cpp (right):

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/graph...
Source/core/platform/graphics/chromium/DeferredImageDecoder.cpp:191: return
m_actualDecoder ? m_actualDecoder->frameIsCompleteAtIndex(index) : false;
On 2013/04/23 21:43:20, Peter Kasting wrote:
> Nit: Simpler:
> 
>     return m_actualDecoder && m_actualDecoder->frameIsCompleteAtIndex(index);

Done.

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
File Source/core/platform/image-decoders/ImageDecoder.h (right):

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:297: // This method is used
by animated images only.
I'm pretty sure this API is used only for animated images.

I agree with you that this API shouldn't be restricted to animated images. I'll
leave this as a TODO as simplifying frameHasAlphaAtIndex() and having a default
implementation for non-animated images is not a goal for this CL.

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
File Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp (right):

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:136: return
m_reader->frameContext(index)->isComplete();
On 2013/04/23 21:43:20, Peter Kasting wrote:
> Nit: Simpler:
> 
>     return m_reader && (index < m_reader->imagesCount()) &&
> m_reader->frameContext(index)->isComplete();

Done.

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:147: return 0;
On 2013/04/23 21:43:20, Peter Kasting wrote:
> Nit: Simpler:
> 
>   return (m_reader && (index < m_reader->imagesCount()) &&
> m_reader->frameContext(index)->isHeaderDefined()) ?
> m_reader->frameContext(index)->delayTime : 0;

Done.

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:358: if
(isAllDataReceived() && haltAtFrame >= m_frameBufferCache.size() &&
!m_reader->parseCompleted())
On 2013/04/23 21:43:20, Peter Kasting wrote:
> Seems like we could parse cleanly but encounter a decoding error and we would
> never wind up calling setFailed() anymore.

If it is parse only we'd return at line 349. The if statement is reached only
during decoding. So this statement means that we have not parsed cleanly to the
end, but we allow decoding to the last available frame before reporting an
error.

Note that decoding error is already reported at line 352. This is additional to
stop decoding / animation after decoding the last available frame and file is
truncated. In practice this is the same outcome as the code before.

[Peter Kasting, 2013-04-25 23:22:32]

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
File Source/core/platform/image-decoders/ImageDecoder.h (right):

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:297: // This method is used
by animated images only.
On 2013/04/25 23:02:44, Alpha wrote:
> I'm pretty sure this API is used only for animated images.
> 
> I agree with you that this API shouldn't be restricted to animated images.
I'll
> leave this as a TODO as simplifying frameHasAlphaAtIndex() and having a
default
> implementation for non-animated images is not a goal for this CL.

But you're adding the function in this CL, and the implementation that works for
both image types is like 2 lines long.  Why not do it now?  It's trivial.

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
File Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp (right):

https://codereview.chromium.org/14317005/diff/3001/Source/core/platform/image...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:358: if
(isAllDataReceived() && haltAtFrame >= m_frameBufferCache.size() &&
!m_reader->parseCompleted())
On 2013/04/25 23:02:44, Alpha wrote:
> On 2013/04/23 21:43:20, Peter Kasting wrote:
> > Seems like we could parse cleanly but encounter a decoding error and we
would
> > never wind up calling setFailed() anymore.
> 
> If it is parse only we'd return at line 349. The if statement is reached only
> during decoding. So this statement means that we have not parsed cleanly to
the
> end, but we allow decoding to the last available frame before reporting an
> error.
> 
> Note that decoding error is already reported at line 352. This is additional
to
> stop decoding / animation after decoding the last available frame and file is
> truncated. In practice this is the same outcome as the code before.

I understand what is going on now.  My main problem was in misunderstanding the
meaning of "m_parseCompleted", which doesn't actually mean "we succeeded in
parsing", it means "we succeeded in decoding and reached GIFDone state".

I think this would be slightly clearer if, instead of adding a parseCompleted()
accessor on GIFImageReader, you added a private bool m_gifComplete to
GIFImageDecoder, and set it true in gifComplete(), then checked it here.  This
would function the same, but read a little more clearly and allow the
gifComplete() callback to perform all necessary state changes on reaching
GIFDone (as it used to do) rather than requiring us to read back into the
GIFImageReader.

https://codereview.chromium.org/14317005/diff/11001/Source/core/platform/imag...
File Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp (left):

https://codereview.chromium.org/14317005/diff/11001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:301:
m_reader.clear();
We need to keep the reader around now for the frame context vector, but it seems
like we ought to have the reader clear m_data, shouldn't we?  Otherwise doesn't
this patch mean we'll no longer free the un-decoded data stream after decode
finishes?

[Stephen White, 2013-04-26 17:11:32]

BitmapImage changes look good; I defer to Peter for the decoder changes.

[Alpha Left Google, 2013-04-26 20:43:08]

> I understand what is going on now.  My main problem was in misunderstanding
the
> meaning of "m_parseCompleted", which doesn't actually mean "we succeeded in
> parsing", it means "we succeeded in decoding and reached GIFDone state".
> 
> I think this would be slightly clearer if, instead of adding a
parseCompleted()
> accessor on GIFImageReader, you added a private bool m_gifComplete to
> GIFImageDecoder, and set it true in gifComplete(), then checked it here.  This
> would function the same, but read a little more clearly and allow the
> gifComplete() callback to perform all necessary state changes on reaching
> GIFDone (as it used to do) rather than requiring us to read back into the
> GIFImageReader.
> 

I thought of the approach you suggested here. 

One consideration for not doing it is that we're moving away from decoding GIF
as a stream. Instead GIFImageDecoder will be the driver and choose which frame
to decode in the future. There won't be a gifComplete() event in the future so
I'm moving towards having more getters in GIFImageReader.

[Alpha Left Google, 2013-04-26 20:43:36]

https://codereview.chromium.org/14317005/diff/11001/Source/core/platform/imag...
File Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp (left):

https://codereview.chromium.org/14317005/diff/11001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:301:
m_reader.clear();
It is true that GIFImageReader is holding a reference to SharedBuffer but so as
ImageDecoder. See code here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

If the client of ImageDecoder doesn't want the decoder to hold a reference it
will call ImageDecoder::setData(NULL) which will make GIFImageReader to release
the reference as well.

So I don't think the lifetime of SharedBuffer exceeds what the client of
ImageDecoder expects.

https://codereview.chromium.org/14317005/diff/11001/Source/core/platform/imag...
File Source/core/platform/image-decoders/gif/GIFImageDecoder.h (right):

https://codereview.chromium.org/14317005/diff/11001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.h:53: virtual float
frameDurationAtIndex(size_t) const;
Done. Now it has a default implementation.

[Peter Kasting, 2013-04-27 00:51:00]

>  There won't be a gifComplete() event in the future so I'm moving towards
having more getters in GIFImageReader.

Won't that mean there won't be an m_parseCompleted flag either?  ...since that's
only set on hitting GIFDone, which we'd only hit in cases where it's actually
unexpected and an error?

...By which I mean to say, it doesn't seem like the solution in this patch is
actually more future-proof.

https://codereview.chromium.org/14317005/diff/11001/Source/core/platform/imag...
File Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp (left):

https://codereview.chromium.org/14317005/diff/11001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:301:
m_reader.clear();
On 2013/04/26 20:43:36, Alpha wrote:
> It is true that GIFImageReader is holding a reference to SharedBuffer but so
as
> ImageDecoder. See code here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> If the client of ImageDecoder doesn't want the decoder to hold a reference it
> will call ImageDecoder::setData(NULL) which will make GIFImageReader to
release
> the reference as well.
> 
> So I don't think the lifetime of SharedBuffer exceeds what the client of
> ImageDecoder expects.

Actually, in this case I think the decoder should just do setData(NULL).  This
will drop the references on both the reader and the decoder (base class).  This
way the reader can trigger freeing the data immediately even if the layer that
passes data to ImageDecoder doesn't know it can ask for the ref to be dropped
yet.

https://codereview.chromium.org/14317005/diff/23001/Source/core/platform/imag...
File Source/core/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/14317005/diff/23001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:132: return
isAllDataReceived();
It seems like this should be:

    return ((index < m_frameBufferCache.size()) &&
(m_frameBufferCache[index].status() == ImageFrame::FrameComplete);

This will work better for e.g. ICO or other formats besides GIF that have
multiple frames.

Not including a trailing "|| isAllDataReceived()" part also lets us go ahead and
simplify frameHasAlphaAtIndex(), and is exactly equivalent to what the code did
before your patch, so it won't regress anything.  It will be slightly more
pessimistic in cases where we've gotten the data but hadn't decoded yet, but I
think it's actually safer.

The reason for this is that BitmapImage::cacheFrame() used to force us to decode
as much as possible, so the information we got was as accurate as possible, and
the "frame complete" flag would tell us if the information was reliable.  Now,
however, with anything other than GIF, cacheFrame() won't trigger any decoding,
and we'll store whatever the ImageDecoder base functions return for the various
values.  This can lead to us claiming a frame is complete when the other
metadata is inaccurate, which could cause problems.  I'm not sure whether it
_will_ cause problems as I haven't sufficiently audited all the codepaths, but
it definitely feels safer.

Long-term, if we want to support e.g. animated WebP, we'll want to do something
like you did for GIF and override these functions to be able to return accurate
answers while having only parsed the data, not decoded the framebuffer.  We also
may want to audit how the metadata is used more thoroughly, and perhaps plumb
the idea of "data available" versus "data parsed" versus "frame decoded" more
clearly to the ImageSource.  But I'd hope that can be a separate patch.

[Alpha Left Google, 2013-05-02 23:02:43]

On 2013/04/27 00:51:00, Peter Kasting wrote:
> >  There won't be a gifComplete() event in the future so I'm moving towards
> having more getters in GIFImageReader.
> 
> Won't that mean there won't be an m_parseCompleted flag either?  ...since
that's
> only set on hitting GIFDone, which we'd only hit in cases where it's actually
> unexpected and an error?
> 
> ...By which I mean to say, it doesn't seem like the solution in this patch is
> actually more future-proof.

I think m_parseCompleted will stay and in the form to detect that file is
truncated. I added a new method in GIFImageDecoder::truncated(). We actually
want to keep this flag because we want to detect if the file is correctly
formed.

> Actually, in this case I think the decoder should just do setData(NULL).  This
> will drop the references on both the reader and the decoder (base class). 
This
> way the reader can trigger freeing the data immediately even if the layer that
> passes data to ImageDecoder doesn't know it can ask for the ref to be dropped
> yet.

Do you mean doing this in gifComplete()? This doesn't seem right as the user of
GIFImageDecoder will have to call setData() before calling frameBufferAtIndex()
which is not the case now.

> 
>
https://codereview.chromium.org/14317005/diff/23001/Source/core/platform/imag...
> File Source/core/platform/image-decoders/ImageDecoder.cpp (right):
> 
>
https://codereview.chromium.org/14317005/diff/23001/Source/core/platform/imag...
> Source/core/platform/image-decoders/ImageDecoder.cpp:132: return
> isAllDataReceived();
> It seems like this should be:
> 
>     return ((index < m_frameBufferCache.size()) &&
> (m_frameBufferCache[index].status() == ImageFrame::FrameComplete);
> 
> This will work better for e.g. ICO or other formats besides GIF that have
> multiple frames.
> 
> Not including a trailing "|| isAllDataReceived()" part also lets us go ahead
and
> simplify frameHasAlphaAtIndex(), and is exactly equivalent to what the code
did
> before your patch, so it won't regress anything.  It will be slightly more
> pessimistic in cases where we've gotten the data but hadn't decoded yet, but I
> think it's actually safer.
> 
> The reason for this is that BitmapImage::cacheFrame() used to force us to
decode
> as much as possible, so the information we got was as accurate as possible,
and
> the "frame complete" flag would tell us if the information was reliable.  Now,
> however, with anything other than GIF, cacheFrame() won't trigger any
decoding,
> and we'll store whatever the ImageDecoder base functions return for the
various
> values.  This can lead to us claiming a frame is complete when the other
> metadata is inaccurate, which could cause problems.  I'm not sure whether it
> _will_ cause problems as I haven't sufficiently audited all the codepaths, but
> it definitely feels safer.
> 
> Long-term, if we want to support e.g. animated WebP, we'll want to do
something
> like you did for GIF and override these functions to be able to return
accurate
> answers while having only parsed the data, not decoded the framebuffer.  We
also
> may want to audit how the metadata is used more thoroughly, and perhaps plumb
> the idea of "data available" versus "data parsed" versus "frame decoded" more
> clearly to the ImageSource.  But I'd hope that can be a separate patch.

Yeah I like this idea. In general this is how I intend these API to be, they are
pessimistic guess of alpha and completeness unless the metadata is known.

[Peter Kasting, 2013-05-02 23:15:25]

On 2013/05/02 23:02:43, Alpha wrote:
> On 2013/04/27 00:51:00, Peter Kasting wrote:
> > Actually, in this case I think the decoder should just do setData(NULL). 
This
> > will drop the references on both the reader and the decoder (base class). 
> This
> > way the reader can trigger freeing the data immediately even if the layer
that
> > passes data to ImageDecoder doesn't know it can ask for the ref to be
dropped
> > yet.
> 
> Do you mean doing this in gifComplete()? This doesn't seem right as the user
of
> GIFImageDecoder will have to call setData() before calling
frameBufferAtIndex()
> which is not the case now.

Thinking out loud.  Tell me if I ended up at the right spot.

Well, we only get gifComplete() after we decode the last frame, not just after
parsing the GIF file, right?  ...Although I guess the problem is we're moving
away from a world in which we decode all the frames in order.  If you can just
ask for the last frame alone, then later some other frame, this would probably
be bad... what if we only call this once all frames have been decoded?  Or does
that not work either, because we can discard the decoded framebuffers and ask to
redecode... the old code would recreate the reader in this case and grab the
data from the decoder, so we can't throw that data away... hmm, maybe there
isn't a good way to do this.  But is requiring a setData() call before decoding
a frame so bad?  I don't know.  Maybe?

[Alpha Left Google, 2013-05-03 00:12:24]

> Well, we only get gifComplete() after we decode the last frame, not just after
> parsing the GIF file, right?

Yes.

> Although I guess the problem is we're moving
> away from a world in which we decode all the frames in order.  If you can just
> ask for the last frame alone, then later some other frame, this would probably
> be bad... what if we only call this once all frames have been decoded?  Or
does
> that not work either, because we can discard the decoded framebuffers and ask
to
> redecode... the old code would recreate the reader in this case and grab the
> data from the decoder, so we can't throw that data away... hmm, maybe there
> isn't a good way to do this. 

You're exactly right. Because we can discard frames and be asked to redecode.
And
if we don't have m_data then decode won't work.

> But is requiring a setData() call before decoding
> a frame so bad?  I don't know.  Maybe?

It's not bad at all but this requires a change in BitmapImage.cpp which can
affect
all other decoders.

The reason this has to be in BitmapImage.cpp is that ImageSource doesn't keep
a reference to m_data. See code here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I actually like the idea of setData(data), decode() and setData(NULL), maybe we
can
do this in ImageSource? But I think this should belong to another change.

[Alpha Left Google, 2013-05-03 20:54:53]

$ ping -friendly

[Alpha Left Google, 2013-05-03 21:00:42]

On 2013/05/03 20:54:53, Alpha wrote:
> $ ping -friendly

i mean --friendly

[Peter Kasting, 2013-05-03 22:03:19]

LGTM with question.

https://codereview.chromium.org/14317005/diff/29001/Source/core/platform/imag...
File Source/core/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/14317005/diff/29001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:123: if
(m_frameBufferCache.size() <= index)
Nit: This can now be simplified to:

  return !frameIsCompleteAtIndex(index) || m_frameBufferCache[index].hasAlpha();

https://codereview.chromium.org/14317005/diff/29001/Source/core/platform/imag...
File Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp (right):

https://codereview.chromium.org/14317005/diff/29001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:432: return
isAllDataReceived() && m_reader && !m_reader->parseCompleted();
This is only going to be accurate if we've asked to decode the whole file,
though, right?  Wouldn't it be true that if we got all the data and then only
asked for an early frame, then someone could call this function and it would
erroneously return true?

Maybe for that reason, we should keep this inlined into decode() where it was
before.

[Alpha Left Google, 2013-05-06 17:43:24]

https://codereview.chromium.org/14317005/diff/29001/Source/core/platform/imag...
File Source/core/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/14317005/diff/29001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:123: if
(m_frameBufferCache.size() <= index)
On 2013/05/03 22:03:19, Peter Kasting wrote:
> Nit: This can now be simplified to:
> 
>   return !frameIsCompleteAtIndex(index) ||
m_frameBufferCache[index].hasAlpha();

Done.

https://codereview.chromium.org/14317005/diff/29001/Source/core/platform/imag...
File Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp (right):

https://codereview.chromium.org/14317005/diff/29001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:432: return
isAllDataReceived() && m_reader && !m_reader->parseCompleted();
On 2013/05/03 22:03:19, Peter Kasting wrote:
> This is only going to be accurate if we've asked to decode the whole file,
> though, right?  Wouldn't it be true that if we got all the data and then only
> asked for an early frame, then someone could call this function and it would
> erroneously return true?
> 
> Maybe for that reason, we should keep this inlined into decode() where it was
> before.

Yes you're right. Changed it back to before so we don't make mistake in the
future.

[commit-bot: I haz the power, 2013-05-06 17:44:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/14317005/38001

[commit-bot: I haz the power, 2013-05-06 17:53:36]

Retried try job too often on blink_bare_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_bare...

[Alpha Left Google, 2013-05-06 17:57:15]

Ping senorblanco@ for owner approval.

jamesr@ for approval of changes in GIFImageDecoderTest.cpp.

[jamesr, 2013-05-06 17:59:03]

lgtm on test

[Alpha Left Google, 2013-05-07 17:14:12]

ping senorblanco.

[Stephen White, 2013-05-07 17:34:20]

On 2013/05/07 17:14:12, Alpha wrote:
> ping senorblanco.

LGTM for platform/graphics.

[commit-bot: I haz the power, 2013-05-07 17:36:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/14317005/38001

[commit-bot: I haz the power, 2013-05-07 18:38:21]

Change committed as 149883

[Noel Gordon, 2013-05-16 14:35:24]

https://codereview.chromium.org/14317005/diff/29001/Source/core/platform/imag...
File Source/core/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/14317005/diff/29001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:123: if
(m_frameBufferCache.size() <= index)
On 2013/05/03 22:03:19, Peter Kasting wrote:
> Nit: This can now be simplified to:
> 
>   return !frameIsCompleteAtIndex(index) ||
m_frameBufferCache[index].hasAlpha();

I get nervous when we change the default implementation of frameHasAlphaAtIndex.
 When we have exposed frame alpha before the frame is decoded (FrameComplete) in
the past, it often ended in tears.

The reason was documented in ImageSource once upon a time
http://trac.webkit.org/changeset/125154/trunk/Source/WebCore/platform/graphic...

and is still valid today: the undecoded regions of an image frame are
transparent while the image frame is loading, even for image formats that have
no alpha (JPEG).  Indeed, the JPEG decoder explicitly marks the image buffer as
having alpha during decoding

http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/image-decoder...

and then clears the buffer alpha just before it marks the buffer FrameComplete
(decoded).

http://src.chromium.org/viewvc/blink/trunk/Source/core/platform/image-decoder...

Seems weird, but when we did't attend to that detail, it caused bugs:

https://code.google.com/p/chromium/issues/detail?id=111758
https://bugs.webkit.org/show_bug.cgi?id=78239

The GIF decoder overrides frameIsCompleteAtIndex to mean frame received so
frameHasAlphaAtIndex could now expose a GIF image frame alpha before it fully
decoded (FrameComplete).  Might that cause the same bugs we've seen for JPEG in
some cases?

If the the GIF decoder wants to override frameIsCompleteAtIndex, that's ok, but
I don't think frameHasAlphaAtIndex needed to be changed to depend on that
override to make this patch work?

[Peter Kasting, 2013-05-16 21:37:53]

https://codereview.chromium.org/14317005/diff/29001/Source/core/platform/imag...
File Source/core/platform/image-decoders/ImageDecoder.cpp (right):

https://codereview.chromium.org/14317005/diff/29001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:123: if
(m_frameBufferCache.size() <= index)
On 2013/05/16 14:35:24, Noel Gordon (Google) wrote:
> The GIF decoder overrides frameIsCompleteAtIndex to mean frame received so
> frameHasAlphaAtIndex could now expose a GIF image frame alpha before it fully
> decoded (FrameComplete).  Might that cause the same bugs we've seen for JPEG
in
> some cases?

I think this concern is valid, but I think it isn't about whether we've changed
the implementation of frameHasAlphaAtIndex(), but rather about the whole point
of this patch: the GIF decoder at least is now reporting that a frame is
"complete" when in fact the frame isn't complete in the same sense that we used
to mean that.  In theory, a caller might ask for any arbitrary information about
a frame at that point, and expect it to be accurate and authoritative.  We need
to ensure that either the answers are correct, or that callers don't ask.

So not just for alpha, but for anything else the caller might ask about this
frame, are we ensuring that if the GIF decoder wants to report "complete"
earlier, that it will correctly answer all other such queries?  I suspect that
addressing this probably requires overriding frameHasAlphaAtIndex() in the GIF
decoder to actually decode all the pixels in the "frame complete, but haven't
decoded" case so it can properly check for alpha.

Incidentally, your comment makes me note another issue: if
frameHasAlphaAtIndex() assumes that frameIsCompleteAtIndex() implies the
existence of m_frameBufferCache[index] -- as we've updated it to do here -- then
it's problematic if a subclass overrides frameIsCompleteAtIndex() to violate
that guarantee, and does not also override frameHasAlphaAtIndex() to not rely on
it.  We could address this by adding comments to this effect, or by adding an
ASSERT and comments, or by recoding the functions to not rely on each other and
then adding comments about why they can't rely on each other.  All these
solutions have problems, and while the third sounds initially more appealing,
the fact that practically speaking anyone who overrides frameIsCompleteAtIndex()
must override frameHasAlphaAtIndex() in order to avoid problems like the one
initially discussed above makes me think that we might as well leave things as
they are but add comments to the header noting that subclasses who override the
former function should almost certainly override the latter, and why.