Decode GIF image frames on demand.

Decode GIF image frames on demand.

Previously GIFImageDecoder::frameBufferAtIndex() decodes all frames from
the last decoded frame to the requested frame. This causes delays when
skipping to specific frames.

With this change, GIFImageDecoder decodes only necessary frames based on
previous frames' disposal methods. Added ImageDecoder::findRequiredPreviousFrame()
to find the required previous frame based on the disposal methods.

Also changed the behavior of GIFImageDecoder::clearFrameBufferCache() (moved to
ImageDecoder because it can be shared between multi-frame decoders) because
the original semantics no longer apply with on-demand frame decoding.
Instead of clearing before the given frame, now clear from the whole frame buffer
cache, preserving the frames that are likely to be used soon.

BUG=238242
TEST=ImageDecoderTest.findRequiredPreviousFrame, ImageDecoderTest.clearFrameBufferCache, GIFImageDecoderTest.randomFrameDecode, GIFImageDecoderTest.decodeAfterClearFrameBufferCache
R=abarth@chromium.org, pkasting@chromium.org

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

[Xianzhu, 2013-05-17 23:17:56]

Untested. For preview.
Will add/update unit tests.

[Alpha Left Google, 2013-05-18 00:39:35]

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

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:192:
ASSERT(j->status() != ImageFrame::FramePartial);
This condition can fail now with this change.

Because now you can decode:
0 1 2 3'  where 3' is partially decoded

And then skip ahead to decode:
5 6 7 8

Suppose clearFrameBufferCache(7) is called then this assert will fail.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:307: void
GIFImageDecoder::gifComplete()
I don't think this method is needed any more. Now a frame count query will scan
the file until the end and hence repetition count will be correct regardless of
whether the entire file is decoded or not.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:342: BitVector
frames(frameIndex);
Instead of a BitVector what about a vector of frame indices? This way you can
call m_reader->decode(...) with those indices in order.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:345: if
(!m_reader->decode(frames)) {
I'd prefer to call m_reader->decode(...) multiple times to decode all the
necessary frames.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:353: setFailed();
The reason for calling setFailed() here is to stop animation such that for
truncated GIF files we only animate once.

I think this will look much simpler if we intead do a condition in
repetitionCount() that:
if (isAllDataReceived() && m_reader && !m_reader->parseCompleted())
  return cAnimationLoopOnce;
It's not necessary to do this change now though.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:366: size_t
GIFImageDecoder::getPreviousRequiredFrame(size_t frameIndex)
I see this method will be called multiple times for the same |frameIndex|. Can
you make an optimization here by caching the |previousRequiredFrame|? In fact
after each frame count query you can go over the new frames and compute each
|previousRequiredFrame| quickly.

The current algorithm is to scan backward for each frame and in the worst case
this is O(n^2).

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
File Source/core/platform/image-decoders/gif/GIFImageDecoder.h (right):

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageDecoder.h:65: void
gifComplete();
I suggest removing this method. This used to mean that the entire GIF image is
decoded until the end without error. This change allows GIF frames to be decoded
out of order and this method doesn't apply.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageDecoder.h:69: // data. If
parsing fails but there is no more data coming, sets the
If parsing fails we report an error anyway, regardless of whether there's new
incoming data.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
File Source/core/platform/image-decoders/gif/GIFImageReader.cpp (right):

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageReader.cpp:359: for (size_t i =
0; i < frames.size(); ++i) {
I would rather let the user of this API to call multiple times for each frame it
wants to decode.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageReader.cpp:376:
m_decodedFramesCount++;
See my comments below, I don't think we need to do this any more.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageReader.cpp:380: if
(m_decodedFramesCount == m_frames.size() && m_parseCompleted)
This check will not be correct because we now allow the GIF image to be decoded
at random index, so a frame can be decoded more than once.

My intention was to remove the call to m_client->gifComplete() because it
doesn't apply any more if we're not decoding serially.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
File Source/core/platform/image-decoders/gif/GIFImageReader.h (right):

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageReader.h:253: bool decode(const
BitVector& frames);
Why not just give the index? User of this API (GIFImageDecoder) can call this
multiple times.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageReader.h:325: size_t
m_decodedFramesCount;
I don't think we need this any more. See my comments in GIFImageReader.cpp.

[Xianzhu, 2013-05-20 04:30:59]

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

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:192:
ASSERT(j->status() != ImageFrame::FramePartial);
On 2013/05/18 00:39:35, Alpha wrote:
> This condition can fail now with this change.
> 
> Because now you can decode:
> 0 1 2 3'  where 3' is partially decoded
> 
> And then skip ahead to decode:
> 5 6 7 8
> 
> Suppose clearFrameBufferCache(7) is called then this assert will fail.

Done.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:307: void
GIFImageDecoder::gifComplete()
On 2013/05/18 00:39:35, Alpha wrote:
> I don't think this method is needed any more. Now a frame count query will
scan
> the file until the end and hence repetition count will be correct regardless
of
> whether the entire file is decoded or not.

Done.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:342: BitVector
frames(frameIndex);
On 2013/05/18 00:39:35, Alpha wrote:
> Instead of a BitVector what about a vector of frame indices? This way you can
> call m_reader->decode(...) with those indices in order.

Done.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:345: if
(!m_reader->decode(frames)) {
On 2013/05/18 00:39:35, Alpha wrote:
> I'd prefer to call m_reader->decode(...) multiple times to decode all the
> necessary frames.

Done.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:366: size_t
GIFImageDecoder::getPreviousRequiredFrame(size_t frameIndex)
On 2013/05/18 00:39:35, Alpha wrote:
> I see this method will be called multiple times for the same |frameIndex|. Can
> you make an optimization here by caching the |previousRequiredFrame|? In fact
> after each frame count query you can go over the new frames and compute each
> |previousRequiredFrame| quickly.
> 
> The current algorithm is to scan backward for each frame and in the worst case
> this is O(n^2).

Done.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
File Source/core/platform/image-decoders/gif/GIFImageDecoder.h (right):

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageDecoder.h:65: void
gifComplete();
On 2013/05/18 00:39:35, Alpha wrote:
> I suggest removing this method. This used to mean that the entire GIF image is
> decoded until the end without error. This change allows GIF frames to be
decoded
> out of order and this method doesn't apply.

Done.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageDecoder.h:69: // data. If
parsing fails but there is no more data coming, sets the
On 2013/05/18 00:39:35, Alpha wrote:
> If parsing fails we report an error anyway, regardless of whether there's new
> incoming data.

Done.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
File Source/core/platform/image-decoders/gif/GIFImageReader.cpp (right):

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageReader.cpp:359: for (size_t i =
0; i < frames.size(); ++i) {
On 2013/05/18 00:39:35, Alpha wrote:
> I would rather let the user of this API to call multiple times for each frame
it
> wants to decode.

Done.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageReader.cpp:376:
m_decodedFramesCount++;
On 2013/05/18 00:39:35, Alpha wrote:
> See my comments below, I don't think we need to do this any more.

Done.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageReader.cpp:380: if
(m_decodedFramesCount == m_frames.size() && m_parseCompleted)
On 2013/05/18 00:39:35, Alpha wrote:
> This check will not be correct because we now allow the GIF image to be
decoded
> at random index, so a frame can be decoded more than once.
> 
> My intention was to remove the call to m_client->gifComplete() because it
> doesn't apply any more if we're not decoding serially.

Done.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
File Source/core/platform/image-decoders/gif/GIFImageReader.h (right):

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageReader.h:253: bool decode(const
BitVector& frames);
On 2013/05/18 00:39:35, Alpha wrote:
> Why not just give the index? User of this API (GIFImageDecoder) can call this
> multiple times.

Done.

https://codereview.chromium.org/15350006/diff/1/Source/core/platform/image-de...
Source/core/platform/image-decoders/gif/GIFImageReader.h:325: size_t
m_decodedFramesCount;
On 2013/05/18 00:39:35, Alpha wrote:
> I don't think we need this any more. See my comments in GIFImageReader.cpp.

Done.

[Xianzhu, 2013-05-20 17:06:39]

There is some bug in the changed code, causing the animated gifs drawn blank.
Now debugging.

[Xianzhu, 2013-05-20 17:27:11]

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

https://codereview.chromium.org/15350006/diff/13001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:357: for (size_t i =
0; i < framesToDecode.size(); ++i) {
This is wrong. Should inverse the order. Fixing.

[Xianzhu, 2013-05-20 19:45:06]

PTAL. Patch Set 4 is tested.

[Peter Kasting, 2013-05-20 23:20:07]

Some general questions.

Can we instrument Chrome somehow to measure the memory impact of keeping more
frames around, as well as the frame decode time improvements?  It would be nice
to get real-world data on the tradeoffs of this patch so we can tune further.

Is it still necessary to have BitmapImage ask the decoders to destroy decoded
data?  Should the GIF decoder just do all this internally without any prompting?

[Xianzhu, 2013-05-21 00:26:25]

On 2013/05/20 23:20:07, Peter Kasting wrote:
> Some general questions.
> 
> Can we instrument Chrome somehow to measure the memory impact of keeping more
> frames around, as well as the frame decode time improvements?  It would be
nice
> to get real-world data on the tradeoffs of this patch so we can tune further.
> 

Will test on Android with animated GIFs converted from videos because they have
the biggest performance issue. Just thought of that these GIFs have some special
features (e.g. full-size fully-opaque frames) that we can optimize accordingly.
These optimizations may be more effective than the latest Patch Set here.

We can also make the trade-off easier by not introducing memory impact (i.e. not
keeping more frames around) in this change.

> Is it still necessary to have BitmapImage ask the decoders to destroy decoded
> data?  Should the GIF decoder just do all this internally without any
prompting?

For now BitmapImage references frame pixel data and reports memory usages, so it
should know which frames are cleared. We can let decoder to notify BitmapImage
about
cleared frames or do even further refactors, but I prefer doing them in separate
changes.

[Alpha Left Google, 2013-05-21 00:40:17]

> > Is it still necessary to have BitmapImage ask the decoders to destroy
decoded
> > data?  Should the GIF decoder just do all this internally without any
> prompting?
> 
> For now BitmapImage references frame pixel data and reports memory usages, so
it
> should know which frames are cleared. We can let decoder to notify BitmapImage
> about
> cleared frames or do even further refactors, but I prefer doing them in
separate
> changes.

I also suggest moving the logic of which frame to clear into ImageDecoder. This
means a method like ImageDecoder::clear(); which the decoder will try its best
to reclaim memory given known states like loop count. It is still necessary to
do this in BitmapImage for now since it caches each frame and clear them
independently. The whole logic around clearing frames in BitmapImage needs some
refactoring given that ImageDecoder can do smart caching internally, it seems
wise to do it in a separate change.

[Xianzhu, 2013-05-21 16:32:03]

PTAL. I removed the code to preserve frames at intervals, so that this change
doesn't have impact on memory. I'd like to add it back after we have data about
the benefit.

[Xianzhu, 2013-05-21 16:35:36]

On 2013/05/21 16:32:03, Xianzhu wrote:
> PTAL. I removed the code to preserve frames at intervals, so that this change
> doesn't have impact on memory. I'd like to add it back after we have data
about
> the benefit.

There is still one change about preserving policy: now the frame that will be
required when decoding the clearExceptFrame will be preserved. However I think
this doesn't have extra memory impact because it will be used immediately.

[Xianzhu, 2013-05-21 17:47:58]

Sorry for the multiple versions. Deleted intermediate versions, keeping only the
last one for review.

[Alpha Left Google, 2013-05-22 20:18:09]

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
File Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp (right):

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp:37: #include "wtf/OwnPtr.h"
nit: I'm not sure whether we are moving to use "wtf/..." instead of <wtf/...>.
Other core files seem to remaing using <wtf/...>.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp:251: // Compute hashes when
the frames are requested sequentially.
This is not really random isn't it?

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp:254:
decoder->frameBufferAtIndex(0); // To trigger parsing.
frameCount() will trigger parsing already.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp:292:
decoder->frameBufferAtIndex(0); // To trigger parsing.
frameCount() will trigger parsing so this is not necessary.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp:311: 
You also need to test this corner case:
1. Frame 1 is partially decoded.
2. Skip to decode other frames and then clear.
3. Come back to resume decoding frame 1.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
File Source/WebKit/chromium/tests/ImageDecoderTest.cpp (right):

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageDecoderTest.cpp:99:
decoderFrameBufferCache[1].setDisposalMethod(ImageFrame::DisposeOverwriteBgcolor);
I suggest this be a separate test case.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageDecoderTest.cpp:102: //
DisposeOverwritePrevious of the first frame is treated as
DisposeOverwriteBgcolor.
I suggest this be a separate test case.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageDecoderTest.cpp:108: // Partially converting
DisposeOverriteBgcolor previous frame is treated DisposeKeep.
nit: I can't parse this sentence please rewrite it.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageDecoderTest.cpp:109:
decoderFrameBufferCache[1].setOriginalFrameRect(IntRect(50, 50, 50, 50));
I suggest this be a separate test case.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageDecoderTest.cpp:111: 
Please add a condition that a DisposeKeep or DisposeUnspecified and doesn't have
alpha will also reset the starting state.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageDecoderTest.cpp:126: // This should no
nothing.
nit: do

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageDecoderTest.cpp:143:
EXPECT_EQ(ImageFrame::FramePartial, decoderFrameBufferCache[9].status());
The test only clear (7) which doesn't cover 9 anyway. I'd suggest moving this to
a separate test case.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageDecoderTest.cpp:147:
EXPECT_EQ(ImageFrame::FrameEmpty, decoderFrameBufferCache[i].status());
nit: If this expect fails it's not clear from the log which frame fails, I'd
suggest you check that [7] and [9] are non empty and when check every other is
empty.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageDecoderTest.cpp:150: // Repeated
clearFrameBufferCache should not change the state.
This should be a separate test case.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageDecoderTest.cpp:168:
decoder->clearFrameBufferCache(notFound);
This should be a separate test case.

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

https://codereview.chromium.org/15350006/diff/32001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:230: void
ImageDecoder::clearFrameBufferCache(size_t clearExceptFrame)
I'm not sure you should move this to the base class given that this is relavent
to only animated GIFs. Animated WebP has their internal methods for clearing.

https://codereview.chromium.org/15350006/diff/32001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:258: if (frame.status() ==
ImageFrame::FrameComplete) // 3.
Using this condition you'll hit a corner case where a couple of frames are
partially decoded. This will leave multiple frames we don't need and also not
cleared. Clearing these frames will be more involved because you need to clear
the states for individual frames in the reader as well.

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

https://codereview.chromium.org/15350006/diff/32001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:227:
buffer.setRequiredPreviousFrameIndex(notFound);
Knowing this frame doesn't depend on any previous frame we should update the
dependency of subsequent frames.

https://codereview.chromium.org/15350006/diff/32001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:285:
buffer.setDuration(frameContext->delayTime);
This information should always be there for each new frame. Because we increment
the image count only when a control extension or image block is scanned. Control
extensions contains duration and disposal method and appears before the image
block in the stream.

[Xianzhu, 2013-05-22 23:43:29]

Thanks for review! Will upload a new patch set after we determine where to put
ImageDecoder::clearImageFrameCache(). Could it be reused by animated WebP
decoder?

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
File Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp (right):

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp:37: #include "wtf/OwnPtr.h"
On 2013/05/22 20:18:09, Alpha wrote:
> nit: I'm not sure whether we are moving to use "wtf/..." instead of <wtf/...>.
> Other core files seem to remaing using <wtf/...>.

Yes, this is the new style. The presubmit script reports errors for <wtf/...>.
For now it seems only changed files need to update this.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp:251: // Compute hashes when
the frames are requested sequentially.
On 2013/05/22 20:18:09, Alpha wrote:
> This is not really random isn't it?

This is to get a baseline for testing the random decoding case. Moved it to a
utility function.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp:254:
decoder->frameBufferAtIndex(0); // To trigger parsing.
On 2013/05/22 20:18:09, Alpha wrote:
> frameCount() will trigger parsing already.

Done.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp:292:
decoder->frameBufferAtIndex(0); // To trigger parsing.
On 2013/05/22 20:18:09, Alpha wrote:
> frameCount() will trigger parsing so this is not necessary.

Done.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp:311: 
On 2013/05/22 20:18:09, Alpha wrote:
> You also need to test this corner case:
> 1. Frame 1 is partially decoded.
> 2. Skip to decode other frames and then clear.
> 3. Come back to resume decoding frame 1.

Done.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
File Source/WebKit/chromium/tests/ImageDecoderTest.cpp (right):

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageDecoderTest.cpp:99:
decoderFrameBufferCache[1].setDisposalMethod(ImageFrame::DisposeOverwriteBgcolor);
On 2013/05/22 20:18:09, Alpha wrote:
> I suggest this be a separate test case.
Done.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageDecoderTest.cpp:102: //
DisposeOverwritePrevious of the first frame is treated as
DisposeOverwriteBgcolor.
On 2013/05/22 20:18:09, Alpha wrote:
> I suggest this be a separate test case.

Done.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageDecoderTest.cpp:108: // Partially converting
DisposeOverriteBgcolor previous frame is treated DisposeKeep.
On 2013/05/22 20:18:09, Alpha wrote:
> nit: I can't parse this sentence please rewrite it.

Done.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageDecoderTest.cpp:109:
decoderFrameBufferCache[1].setOriginalFrameRect(IntRect(50, 50, 50, 50));
On 2013/05/22 20:18:09, Alpha wrote:
> I suggest this be a separate test case.

Done.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageDecoderTest.cpp:111: 
On 2013/05/22 20:18:09, Alpha wrote:
> Please add a condition that a DisposeKeep or DisposeUnspecified and doesn't
have
> alpha will also reset the starting state.

This will be tested in GIFImageDecoderTest because the status is updated after
decoding.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageDecoderTest.cpp:126: // This should no
nothing.
On 2013/05/22 20:18:09, Alpha wrote:
> nit: do

Done.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageDecoderTest.cpp:143:
EXPECT_EQ(ImageFrame::FramePartial, decoderFrameBufferCache[9].status());
On 2013/05/22 20:18:09, Alpha wrote:
> The test only clear (7) which doesn't cover 9 anyway. I'd suggest moving this
to
> a separate test case.

Done.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageDecoderTest.cpp:147:
EXPECT_EQ(ImageFrame::FrameEmpty, decoderFrameBufferCache[i].status());
On 2013/05/22 20:18:09, Alpha wrote:
> nit: If this expect fails it's not clear from the log which frame fails, I'd
> suggest you check that [7] and [9] are non empty and when check every other is
> empty.

Done.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageDecoderTest.cpp:150: // Repeated
clearFrameBufferCache should not change the state.
On 2013/05/22 20:18:09, Alpha wrote:
> This should be a separate test case.

Done.

https://codereview.chromium.org/15350006/diff/32001/Source/WebKit/chromium/te...
Source/WebKit/chromium/tests/ImageDecoderTest.cpp:168:
decoder->clearFrameBufferCache(notFound);
On 2013/05/22 20:18:09, Alpha wrote:
> This should be a separate test case.

Done.

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

https://codereview.chromium.org/15350006/diff/32001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:230: void
ImageDecoder::clearFrameBufferCache(size_t clearExceptFrame)
On 2013/05/22 20:18:09, Alpha wrote:
> I'm not sure you should move this to the base class given that this is
relavent
> to only animated GIFs. Animated WebP has their internal methods for clearing.

I thought as the disposal methods are defined in common ImageDecoder.h and the
rules seems common so I moved it here and expected that WebP could reuse it.
WebP has less disposal methods but they should be covered. Can it reuse?

https://codereview.chromium.org/15350006/diff/32001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:247: } while
(frameToBeRequired != notFound && m_frameBufferCache[frameToBeRequired].status()
!= ImageFrame::FrameComplete);
Changed the above condition to ...== ImageFrame::FrameEmpty because a partial
frame can also be the frame to be required.

Also changed GIFImageDecoder::decode() so that we don't go back beyond a partial
frame whose initFrameBuffer() has already been called so doesn't require another
previous frame.

https://codereview.chromium.org/15350006/diff/32001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:258: if (frame.status() ==
ImageFrame::FrameComplete) // 3.
On 2013/05/22 20:18:09, Alpha wrote:
> Using this condition you'll hit a corner case where a couple of frames are
> partially decoded. This will leave multiple frames we don't need and also not
> cleared. Clearing these frames will be more involved because you need to clear
> the states for individual frames in the reader as well.

Changed to clear partial frames. However, I'm not sure what states to clear in
the reader. Can the states be used the next time when the frame is decoded
again?

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

https://codereview.chromium.org/15350006/diff/32001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:227:
buffer.setRequiredPreviousFrameIndex(notFound);
On 2013/05/22 20:18:09, Alpha wrote:
> Knowing this frame doesn't depend on any previous frame we should update the
> dependency of subsequent frames.

That seems unnecessary. Whether this frame resets starting state of itself
should not affect the starting state assumptions of subsequent frames which were
based on the disposal methods and geometry of their previous frames.

To be more specific, a subsequent frame's requiredPreviousFrameIndexd may have
the following values:
1. notFound or >frameIndex: the subsequent frame is independent to this frame;
2. <frameIndex: the frameIndex frame must be DisposeOverwritePrevious so its
contents don't affect subsequent frames' dependencies)
4. ==frameIndex: the subsequent frame should still depend on this frame.

For all the cases, the dependency seems not to need updating.

https://codereview.chromium.org/15350006/diff/32001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:285:
buffer.setDuration(frameContext->delayTime);
On 2013/05/22 20:18:09, Alpha wrote:
> This information should always be there for each new frame. Because we
increment
> the image count only when a control extension or image block is scanned.
Control
> extensions contains duration and disposal method and appears before the image
> block in the stream.
> 

Done.

https://codereview.chromium.org/15350006/diff/32001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:308: }
Just noticed I missed the case of not enough data.
Added bool* frameDecoded parameter to GIFImageReader::decode() to check it here.

[Xianzhu, 2013-05-23 00:33:49]

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

https://codereview.chromium.org/15350006/diff/32001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:258: if (frame.status() ==
ImageFrame::FrameComplete) // 3.
On 2013/05/22 23:43:29, Xianzhu wrote:
> Changed to clear partial frames. However, I'm not sure what states to clear in
> the reader. Can the states be used the next time when the frame is decoded
> again?

Never mind this question. I know the answer now :) We need to clear the reader
status to reflect the cleared frame data

[Xianzhu, 2013-05-23 01:07:38]

PTAL. Still keep ImageDecoder::clearImageFrameBuffer() but I'd move it back to
GIFImageDecoder if it could not be reused.

[Peter Kasting, 2013-05-24 03:15:22]

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/grap...
File Source/core/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/grap...
Source/core/platform/graphics/BitmapImage.cpp:503:
destroyDecodedDataIfNecessary(true);
Since we're resetting m_currentFrame to 0 here, and we're going to want to
display frame 0, we could just as correctly (if not more correctly) pass "false"
to destroyDecodedDataIfNecessary().

The advantage of doing this is that now the only two callers would both pass
"false".  That means we can nuke the argument entirely.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/grap...
File Source/core/platform/graphics/ImageSource.h (right):

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/grap...
Source/core/platform/graphics/ImageSource.h:90: // Implementations that delete
less than what's specified above waste
Nit: Change "waste" to "may waste", since ultimately we're going to want to
preserve more frames than are requested here, at least for GIF.

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

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:240: //  3. We don't clear
empty frames.
Why not clear empty frames?  Shouldn't that just be a no-op?  It seems like we
don't need to special-case those.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:242: size_t
frameToBeRequired;
Nit: Init this to notFound; this allows eliminating the "else" below and is
safer in case someone later changes this code and accidentally adds an
uninitialized-variable codepath.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:247: } while
(frameToBeRequired != notFound && m_frameBufferCache[frameToBeRequired].status()
== ImageFrame::FrameEmpty);
This loop tries to preserve the frame we'll need to start from if we later
decode frames >= clearExceptFrame.  But if we later ask for a frame <
clearExceptFrame, we may need to redecode the whole image to that point.

It's probably worth updating the comments on ImageSource::clear() to say that
the intent of the function is to throw away as much data as possible given an
expectation that you're next going to ask for a frame on/after the provided
frame index, and that if callers violate this by clearing and then requesting a
frame before the provided frame number, the decoder may need to redecode the
image from the beginning.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:264: size_t
ImageDecoder::findRequiredPreviousFrame(size_t frameIndex)
By the way, the algorithm in this function can be simpler and faster if we know
that all frames before frameIndex have already had m_requiredPreviousFrameIndex
set -- because we just need to look at the previous frame's disposal method and
required previous frame, we don't have to loop through other frames.

You may want to ensure that invariant holds and then change to the simpler
algorithm.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:280: const ImageFrame*
prevBuffer = &m_frameBufferCache[prevFrame];
If we're going to do this we should either check or ASSERT that frameIndex() is
<= m_frameBufferCache.size().

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:288: // prevFrame will be
used as the starting state for this frame.
Nit: Might want to add a note or FIXME about being even smarter by checking the
frame sizes and/or alpha-containing regions.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:294: //
DisposeOverritePrevious will be reset to background despite its size.
Nit: Comment could be clearer; how about:

This frame's disposal method is DisposeOverwriteBgColor or
DisposeOverwritePrevious.  If this is the first frame, the starting state for
the next frame is guaranteed to be just an empty frame.

(I would also place this comment above the conditional instead of inside it.)

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:300: const IntSize&
bufferSize = scaledSize();
I think scaledSize() got nuked (or is getting nuked) in a separate CL.

Also, you don't actually use this temp.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:302: // prevFrame covering
the whole image will be reset to background,
Nit: A slightly shorter way of doing this:

    // If prevRect fills the whole image, then the current frame can be decoded
    // alone.  Otherwise, prevFrame contributes to this frame.
    return prevRect.contains(IntRect(IntPoint(), size())) ? notFound :
prevFrame;

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

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:260: // Reset the
states of the partial frame in the reader so that the frame
Nit: states -> state

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:307: } while
(frameToDecode != notFound && m_frameBufferCache[frameToDecode].status() ==
ImageFrame::FrameEmpty);
Doesn't this mean if frame 1 is FramePartial, we ask for frame 2, and frame 2
needs frame 1, we'll only try to decode frame 2, without first finishing
decoding frame 1?

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:309: for (size_t i =
framesToDecode.size(); i; --i) {
Nit: Use a const_reverse_iterator, it's clearer and less error-prone.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:346: size_t
requiredPreviousFrameIndex =
m_frameBufferCache[frameIndex].requiredPreviousFrameIndex();
Nit: Use |buffer|

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:356: if
(!buffer->copyBitmapData(*prevBuffer))
Your code here is simpler than before, but much less efficient in the case where
we have DisposeOverwriteBgcolor and a full frame.  In that case, we should
instead use the setSize() code above to simply clear everything.

Something like:

    if ((requiredPreviousFrameIndex == notFound)
        || ((prevBuffer->disposalMethod() ==
ImageFrame::DisposeOverwriteBgcolor)
            && (prevBuffer->originalFrameRect().contains(IntRect(IntPoint(),
size()))))) {
      // This frame doesn't rely on any previous data.
      ...

...Then I'd put an ASSERT (or at least a comment) here that the prevRect is
smaller than the full image.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
File Source/core/platform/image-decoders/gif/GIFImageReader.cpp (right):

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageReader.cpp:362: if
(!currentFrame->decode(data(0), m_data->size(), m_client, &frameDecoded))
Nit: Simpler:

    return currentFrame->decode(data(0), m_data->size(), m_client,
&frameDecoded)
        && (!frameDecoded || m_client->frameComplete(frameIndex));

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
File Source/core/platform/image-decoders/gif/GIFImageReader.h (right):

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageReader.h:252: bool decode(size_t
frameIndex);
Thumbs up on improving the API here to be noticeably clearer!

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageReader.h:316: int
m_screenBgcolor; // Logical screen background color.
Tangential to your change, but I noticed while looking at how we handle
DisposeOverwriteBgcolor that we don't actually care about this member.  Neither
does Mozilla.  Can you go ahead and nuke this entirely while you're here?

[Xianzhu, 2013-05-28 22:54:21]

Thanks for review!

PTAL.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/grap...
File Source/core/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/grap...
Source/core/platform/graphics/BitmapImage.cpp:503:
destroyDecodedDataIfNecessary(true);
On 2013/05/24 03:15:22, Peter Kasting wrote:
> Since we're resetting m_currentFrame to 0 here, and we're going to want to
> display frame 0, we could just as correctly (if not more correctly) pass
"false"
> to destroyDecodedDataIfNecessary().
> 
> The advantage of doing this is that now the only two callers would both pass
> "false".  That means we can nuke the argument entirely.

Done.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/grap...
File Source/core/platform/graphics/ImageSource.h (right):

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/grap...
Source/core/platform/graphics/ImageSource.h:90: // Implementations that delete
less than what's specified above waste
On 2013/05/24 03:15:22, Peter Kasting wrote:
> Nit: Change "waste" to "may waste", since ultimately we're going to want to
> preserve more frames than are requested here, at least for GIF.

Done.

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

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:240: //  3. We don't clear
empty frames.
On 2013/05/24 03:15:22, Peter Kasting wrote:
> Why not clear empty frames?  Shouldn't that just be a no-op?  It seems like we
> don't need to special-case those.

Done.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:242: size_t
frameToBeRequired;
On 2013/05/24 03:15:22, Peter Kasting wrote:
> Nit: Init this to notFound; this allows eliminating the "else" below and is
> safer in case someone later changes this code and accidentally adds an
> uninitialized-variable codepath.

Done.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:247: } while
(frameToBeRequired != notFound && m_frameBufferCache[frameToBeRequired].status()
== ImageFrame::FrameEmpty);
On 2013/05/24 03:15:22, Peter Kasting wrote:
> This loop tries to preserve the frame we'll need to start from if we later
> decode frames >= clearExceptFrame.  But if we later ask for a frame <
> clearExceptFrame, we may need to redecode the whole image to that point.
> 
> It's probably worth updating the comments on ImageSource::clear() to say that
> the intent of the function is to throw away as much data as possible given an
> expectation that you're next going to ask for a frame on/after the provided
> frame index, and that if callers violate this by clearing and then requesting
a
> frame before the provided frame number, the decoder may need to redecode the
> image from the beginning.

Done.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:280: const ImageFrame*
prevBuffer = &m_frameBufferCache[prevFrame];
On 2013/05/24 03:15:22, Peter Kasting wrote:
> If we're going to do this we should either check or ASSERT that frameIndex()
is
> <= m_frameBufferCache.size().

Done.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:288: // prevFrame will be
used as the starting state for this frame.
On 2013/05/24 03:15:22, Peter Kasting wrote:
> Nit: Might want to add a note or FIXME about being even smarter by checking
the
> frame sizes and/or alpha-containing regions.

Done.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:294: //
DisposeOverritePrevious will be reset to background despite its size.
On 2013/05/24 03:15:22, Peter Kasting wrote:
> Nit: Comment could be clearer; how about:
> 
> This frame's disposal method is DisposeOverwriteBgColor or
> DisposeOverwritePrevious.  If this is the first frame, the starting state for
> the next frame is guaranteed to be just an empty frame.
> 
> (I would also place this comment above the conditional instead of inside it.)

Done.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:300: const IntSize&
bufferSize = scaledSize();
On 2013/05/24 03:15:22, Peter Kasting wrote:
> I think scaledSize() got nuked (or is getting nuked) in a separate CL.
> 
> Also, you don't actually use this temp.

Done.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:302: // prevFrame covering
the whole image will be reset to background,
On 2013/05/24 03:15:22, Peter Kasting wrote:
> Nit: A slightly shorter way of doing this:
> 
>     // If prevRect fills the whole image, then the current frame can be
decoded
>     // alone.  Otherwise, prevFrame contributes to this frame.
>     return prevRect.contains(IntRect(IntPoint(), size())) ? notFound :
> prevFrame;

Done.

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

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:260: // Reset the
states of the partial frame in the reader so that the frame
On 2013/05/24 03:15:22, Peter Kasting wrote:
> Nit: states -> state

Done.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:307: } while
(frameToDecode != notFound && m_frameBufferCache[frameToDecode].status() ==
ImageFrame::FrameEmpty);
On 2013/05/24 03:15:22, Peter Kasting wrote:
> Doesn't this mean if frame 1 is FramePartial, we ask for frame 2, and frame 2
> needs frame 1, we'll only try to decode frame 2, without first finishing
> decoding frame 1?

Thanks for finding the mistake. Corrected.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:309: for (size_t i =
framesToDecode.size(); i; --i) {
On 2013/05/24 03:15:22, Peter Kasting wrote:
> Nit: Use a const_reverse_iterator, it's clearer and less error-prone.

Done.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:346: size_t
requiredPreviousFrameIndex =
m_frameBufferCache[frameIndex].requiredPreviousFrameIndex();
On 2013/05/24 03:15:22, Peter Kasting wrote:
> Nit: Use |buffer|

Done.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.cpp:356: if
(!buffer->copyBitmapData(*prevBuffer))
On 2013/05/24 03:15:22, Peter Kasting wrote:
> Your code here is simpler than before, but much less efficient in the case
where
> we have DisposeOverwriteBgcolor and a full frame.  In that case, we should
> instead use the setSize() code above to simply clear everything.
> 
> Something like:
> 
>     if ((requiredPreviousFrameIndex == notFound)
>         || ((prevBuffer->disposalMethod() ==
> ImageFrame::DisposeOverwriteBgcolor)
>             && (prevBuffer->originalFrameRect().contains(IntRect(IntPoint(),
> size()))))) {
>       // This frame doesn't rely on any previous data.
>       ...
> 
> ...Then I'd put an ASSERT (or at least a comment) here that the prevRect is
> smaller than the full image.

In the case findRequiredPreviousFrame() should have already set
requiredPreviousFrameIndex to notFound, so we wouldn't need to check it again
here. However, I found a bug that the originalFrameRect is not set when this
method is called. Fixed.

The ASSERT is useful. Added.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
File Source/core/platform/image-decoders/gif/GIFImageReader.cpp (right):

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageReader.cpp:362: if
(!currentFrame->decode(data(0), m_data->size(), m_client, &frameDecoded))
On 2013/05/24 03:15:22, Peter Kasting wrote:
> Nit: Simpler:
> 
>     return currentFrame->decode(data(0), m_data->size(), m_client,
> &frameDecoded)
>         && (!frameDecoded || m_client->frameComplete(frameIndex));

Done.

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
File Source/core/platform/image-decoders/gif/GIFImageReader.h (right):

https://codereview.chromium.org/15350006/diff/42001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageReader.h:316: int
m_screenBgcolor; // Logical screen background color.
On 2013/05/24 03:15:22, Peter Kasting wrote:
> Tangential to your change, but I noticed while looking at how we handle
> DisposeOverwriteBgcolor that we don't actually care about this member. 
Neither
> does Mozilla.  Can you go ahead and nuke this entirely while you're here?

Done.

[Peter Kasting, 2013-05-29 02:02:17]

This is pretty close, just a couple of substantive comments.  Most things below
are just suggestions for improving comments.

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
File Source/core/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
Source/core/platform/graphics/BitmapImage.cpp:124: size_t frameBytesCleared =
m_source.clearCacheExceptFrame(m_currentFrame);
Nit: Or just inline this into the next line.

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
File Source/core/platform/graphics/BitmapImage.h (right):

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
Source/core/platform/graphics/BitmapImage.h:172: virtual void
destroyDecodedData() OVERRIDE;
Nit: I'm a big fan of OVERRIDE, but if we're adding it to one function in this
file, perhaps we should add it to all.  (Applies to other files too)

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
File Source/core/platform/graphics/ImageSource.cpp (right):

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
Source/core/platform/graphics/ImageSource.cpp:52: if (m_decoder)
Nit: Simpler:

    return m_decoder ? m_decoder->clearCacheExceptFrame(clearExceptFrame) : 0;

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
File Source/core/platform/graphics/ImageSource.h (right):

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
Source/core/platform/graphics/ImageSource.h:82: // Tells the ImageSource that
the Image no longer cares about decoded frame
Nit: How about this comment:

Tells the ImageSource that the Image no longer cares about decoded frame data
except for the specified frame.  Callers may pass WTF::notFound to clear all
frames.

In response, the ImageSource should delete cached decoded data for other frames
where possible to keep memory use low.  The expectation is that in the future,
the caller may call createFrameAtIndex() with an index larger than the one
passed to this function, and the implementation may then make use of the
preserved frame data here in decoding that frame.  By contrast, callers who call
this function and then later ask for an earlier frame may require more work to
be done, e.g. redecoding the image from the beginning.

Implementations may elect to preserve more frames than the one requested here if
doing so is likely to save CPU time in the future, but will pay an increased
memory cost to do so.

Returns the number of bytes of frame data actually cleared.

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
Source/core/platform/graphics/ImageSource.h:119: // see comments on clear()
above.
Nit: This comment should be removed.

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
File Source/core/platform/graphics/chromium/DeferredImageDecoder.cpp (right):

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
Source/core/platform/graphics/chromium/DeferredImageDecoder.cpp:178: if
(m_actualDecoder)
Nit: Simpler:

    return m_actualDecoder ?
m_actualDecoder->clearCacheExceptFrame(clearExceptFrame) : 0;

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

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:145: // Don't clear if
there is no or only one frame.
Nit: "...are no frames or only one frame."

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:148: 
In theory, it's technically possible for ICOs to reach here, although that ought
not to happen today because while such images have multiple frames, they aren't
animated.

To be safe and clear, however, you should probably modify the ICO image decoder
to explicitly setRequiredPreviousFrameIndex(notFound) on each frame (since all
frames in ICOs are independent).  In a release build, this will have no effect,
but in a debug build it will result in marking each frame's previous frame index
as valid.

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:152: //     will copy
bitmap data;
Nit: ; -> .

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:154: size_t
frameToBeRequired = notFound;
Nit: You can actually make this even simpler by relying on the fact that
clearing an empty frame is effectively a no-op, and that WTF::notFound will
always evaluate > the framebuffer cache size.  Replace the code from here on
with:

    while ((clearExceptFrame < m_frameBufferCache.size()) &&
(m_frameBufferCache[clearExceptFrame].status() == ImageFrame::FrameEmpty))
        clearExceptFrame =
m_frameBufferCache[clearExceptFrame].requiredPreviousFrameIndex();

    size_t frameBytesCleared = 0;
    for (size_t i = 0; i < m_frameBufferCache.size(); ++i) {
        if (i != clearExceptFrame) {
    ...

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:188: const ImageFrame*
prevBuffer = &m_frameBufferCache[prevFrame];
Here you should also ASSERT(prevBuffer->m_requiredPreviousFrameIndexValid). 
This requires exposing a debug-only accessor for it.

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:204: // However, the first
frame always clears even it partially covers.
We can actually be even smarter here by handling cases where the previous frame
isn't the whole image, but the image's starting state was empty -- basically, a
generalization of the "first frame" case.  Like so:

        // If the previous frame fills the whole image, then the current frame
        // can be decoded alone.  Likewise, if the previous frame could be
        // decoded without reference to any prior frame, the starting state for
        // this frame is a blank frame, so it can again be decoded alone.
        // Otherwise, the previous frame contributes to this frame.
        return (prevBuffer->originalFrameRect().contains(IntRect(IntPoint(),
size()))
            || (prevBuffer->requiredPreviousFrameIndex() == notFound)) ?
notFound : prefFrame;

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

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.h:217: // |notFound| if this
frame doesn't require any previous frame.
Nit: Add this at end:

This is used by ImageDecoder::clearCacheExceptFrame(), and will never be read
for image formats that do not have multiple frames.

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.h:384: // Returns number of
bytes of the cleared frames.
Nit: How about this comment:

Clears decoded pixel data from all frames except the provided frame, unless that
frame has status FrameEmpty, in which case we instead preserve the most recent
frame whose data is required in order to decode this frame.  Callers may pass
WTF::notFound to clear all frames.

Returns the number of bytes of frame data actually cleared.

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.h:409: // cached before calling
this method.
Nit: How about this comment:

Calculates the most recent frame whose image data may be needed in order to
decode frame |frameIndex|, based on frame disposal methods.  If no previous
frame's data is required, returns WTF::notFound.

This function requires that the previous frame's |m_requiredPreviousFrameIndex|
member has been set correctly.  The easiest way to ensure this is for subclasses
to call this method and store the result on the frame via
setRequiredPreviousFrameIndex() as soon as the frame has been created and parsed
sufficiently to determine the disposal method; assuming this happens for all
frames in order, the required invariant will hold.

Image formats which do not use more than one frame do not need to worry about
this; see comments on ImageFrame::m_requiredPreviousFrameIndex.

[Xianzhu, 2013-05-29 18:37:01]

PTAL.

I like your suggestions for the comments :)

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
File Source/core/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
Source/core/platform/graphics/BitmapImage.cpp:124: size_t frameBytesCleared =
m_source.clearCacheExceptFrame(m_currentFrame);
On 2013/05/29 02:02:18, Peter Kasting wrote:
> Nit: Or just inline this into the next line.

Done.

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
File Source/core/platform/graphics/BitmapImage.h (right):

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
Source/core/platform/graphics/BitmapImage.h:172: virtual void
destroyDecodedData() OVERRIDE;
On 2013/05/29 02:02:18, Peter Kasting wrote:
> Nit: I'm a big fan of OVERRIDE, but if we're adding it to one function in this
> file, perhaps we should add it to all.  (Applies to other files too)

Done.

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
File Source/core/platform/graphics/ImageSource.cpp (right):

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
Source/core/platform/graphics/ImageSource.cpp:52: if (m_decoder)
On 2013/05/29 02:02:18, Peter Kasting wrote:
> Nit: Simpler:
> 
>     return m_decoder ? m_decoder->clearCacheExceptFrame(clearExceptFrame) : 0;

Done.

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
File Source/core/platform/graphics/ImageSource.h (right):

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
Source/core/platform/graphics/ImageSource.h:82: // Tells the ImageSource that
the Image no longer cares about decoded frame
On 2013/05/29 02:02:18, Peter Kasting wrote:
> Nit: How about this comment:
> 
> Tells the ImageSource that the Image no longer cares about decoded frame data
> except for the specified frame.  Callers may pass WTF::notFound to clear all
> frames.
> 
> In response, the ImageSource should delete cached decoded data for other
frames
> where possible to keep memory use low.  The expectation is that in the future,
> the caller may call createFrameAtIndex() with an index larger than the one
> passed to this function, and the implementation may then make use of the
> preserved frame data here in decoding that frame.  By contrast, callers who
call
> this function and then later ask for an earlier frame may require more work to
> be done, e.g. redecoding the image from the beginning.
> 
> Implementations may elect to preserve more frames than the one requested here
if
> doing so is likely to save CPU time in the future, but will pay an increased
> memory cost to do so.
> 
> Returns the number of bytes of frame data actually cleared.

Done.

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
Source/core/platform/graphics/ImageSource.h:119: // see comments on clear()
above.
On 2013/05/29 02:02:18, Peter Kasting wrote:
> Nit: This comment should be removed.

Done.

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
File Source/core/platform/graphics/chromium/DeferredImageDecoder.cpp (right):

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/grap...
Source/core/platform/graphics/chromium/DeferredImageDecoder.cpp:178: if
(m_actualDecoder)
On 2013/05/29 02:02:18, Peter Kasting wrote:
> Nit: Simpler:
> 
>     return m_actualDecoder ?
> m_actualDecoder->clearCacheExceptFrame(clearExceptFrame) : 0;

Done.

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

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:145: // Don't clear if
there is no or only one frame.
On 2013/05/29 02:02:18, Peter Kasting wrote:
> Nit: "...are no frames or only one frame."

Done.

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:148: 
On 2013/05/29 02:02:18, Peter Kasting wrote:
> In theory, it's technically possible for ICOs to reach here, although that
ought
> not to happen today because while such images have multiple frames, they
aren't
> animated.
> 
> To be safe and clear, however, you should probably modify the ICO image
decoder
> to explicitly setRequiredPreviousFrameIndex(notFound) on each frame (since all
> frames in ICOs are independent).  In a release build, this will have no
effect,
> but in a debug build it will result in marking each frame's previous frame
index
> as valid.

Done.

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:152: //     will copy
bitmap data;
On 2013/05/29 02:02:18, Peter Kasting wrote:
> Nit: ; -> .

Done.

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:154: size_t
frameToBeRequired = notFound;
On 2013/05/29 02:02:18, Peter Kasting wrote:
> Nit: You can actually make this even simpler by relying on the fact that
> clearing an empty frame is effectively a no-op, and that WTF::notFound will
> always evaluate > the framebuffer cache size.  Replace the code from here on
> with:
> 
>     while ((clearExceptFrame < m_frameBufferCache.size()) &&
> (m_frameBufferCache[clearExceptFrame].status() == ImageFrame::FrameEmpty))
>         clearExceptFrame =
> m_frameBufferCache[clearExceptFrame].requiredPreviousFrameIndex();
> 
>     size_t frameBytesCleared = 0;
>     for (size_t i = 0; i < m_frameBufferCache.size(); ++i) {
>         if (i != clearExceptFrame) {
>     ...

Done.

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:188: const ImageFrame*
prevBuffer = &m_frameBufferCache[prevFrame];
On 2013/05/29 02:02:18, Peter Kasting wrote:
> Here you should also ASSERT(prevBuffer->m_requiredPreviousFrameIndexValid). 
> This requires exposing a debug-only accessor for it.

Done.

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.cpp:204: // However, the first
frame always clears even it partially covers.
On 2013/05/29 02:02:18, Peter Kasting wrote:
> We can actually be even smarter here by handling cases where the previous
frame
> isn't the whole image, but the image's starting state was empty -- basically,
a
> generalization of the "first frame" case.  Like so:
> 
>         // If the previous frame fills the whole image, then the current frame
>         // can be decoded alone.  Likewise, if the previous frame could be
>         // decoded without reference to any prior frame, the starting state
for
>         // this frame is a blank frame, so it can again be decoded alone.
>         // Otherwise, the previous frame contributes to this frame.
>         return (prevBuffer->originalFrameRect().contains(IntRect(IntPoint(),
> size()))
>             || (prevBuffer->requiredPreviousFrameIndex() == notFound)) ?
> notFound : prefFrame;

Done. Thanks for the idea.

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

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.h:217: // |notFound| if this
frame doesn't require any previous frame.
On 2013/05/29 02:02:18, Peter Kasting wrote:
> Nit: Add this at end:
> 
> This is used by ImageDecoder::clearCacheExceptFrame(), and will never be read
> for image formats that do not have multiple frames.

Done.

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.h:384: // Returns number of
bytes of the cleared frames.
On 2013/05/29 02:02:18, Peter Kasting wrote:
> Nit: How about this comment:
> 
> Clears decoded pixel data from all frames except the provided frame, unless
that
> frame has status FrameEmpty, in which case we instead preserve the most recent
> frame whose data is required in order to decode this frame.  Callers may pass
> WTF::notFound to clear all frames.
> 
> Returns the number of bytes of frame data actually cleared.

Done.

https://codereview.chromium.org/15350006/diff/60001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.h:409: // cached before calling
this method.
On 2013/05/29 02:02:18, Peter Kasting wrote:
> Nit: How about this comment:
> 
> Calculates the most recent frame whose image data may be needed in order to
> decode frame |frameIndex|, based on frame disposal methods.  If no previous
> frame's data is required, returns WTF::notFound.
> 
> This function requires that the previous frame's
|m_requiredPreviousFrameIndex|
> member has been set correctly.  The easiest way to ensure this is for
subclasses
> to call this method and store the result on the frame via
> setRequiredPreviousFrameIndex() as soon as the frame has been created and
parsed
> sufficiently to determine the disposal method; assuming this happens for all
> frames in order, the required invariant will hold.
> 
> Image formats which do not use more than one frame do not need to worry about
> this; see comments on ImageFrame::m_requiredPreviousFrameIndex.

Done.

[Alpha Left Google, 2013-05-29 20:47:03]

Functionally this change looks good to me. I'll let Peter to the final approval.

[Peter Kasting, 2013-05-29 21:03:18]

LGTM

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

https://codereview.chromium.org/15350006/diff/77001/Source/core/platform/imag...
Source/core/platform/image-decoders/ImageDecoder.h:412: // This function
requires that the previous frame's |m_requiredPreviousFrameIndex|
Nit: It seems like you're trying to wrap comments at 80 columns, which is fine
with me, but if you're doing this, do it consistently -- 3 lines in this comment
are longer than 80.

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

https://codereview.chromium.org/15350006/diff/77001/Source/core/platform/imag...
Source/core/platform/image-decoders/gif/GIFImageDecoder.h:36: // This class
decodes the GIF image fjormat.
Nit: Inserted misspelling

[Xianzhu, 2013-05-29 21:52:27]

+abarth for approval of Source/WebKit/chromium/tests/,  Source/core/svg/.

[abarth-chromium, 2013-05-29 21:55:02]

Tests LGTM.

Please consider moving the tests next to the classes they test so that changes
can be approved by the same folks.

https://chromiumcodereview.appspot.com/15350006/diff/75002/Source/WebKit/chro...
File Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp (right):

https://chromiumcodereview.appspot.com/15350006/diff/75002/Source/WebKit/chro...
Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp:83: OwnPtr<GIFImageDecoder>
decoder(createDecoder());
We usually use the assignment from of the constructor:

OwnPtr<GIFImageDecoder> decoder = createDecoder();

[Xianzhu, 2013-05-29 23:28:13]

On 2013/05/29 21:55:02, abarth wrote:
> Tests LGTM.
> 
> Please consider moving the tests next to the classes they test so that changes
> can be approved by the same folks.
>

Done (ImageDecoderTest.cpp and GIFImageDecoderTest.cpp).

>
https://chromiumcodereview.appspot.com/15350006/diff/75002/Source/WebKit/chro...
> File Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/15350006/diff/75002/Source/WebKit/chro...
> Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp:83:
OwnPtr<GIFImageDecoder>
> decoder(createDecoder());
> We usually use the assignment from of the constructor:
> 
> OwnPtr<GIFImageDecoder> decoder = createDecoder();

Done.

[commit-bot: I haz the power, 2013-05-29 23:28:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/15350006/98001

[commit-bot: I haz the power, 2013-05-29 23:47:57]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-29 23:52:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/15350006/109001

[commit-bot: I haz the power, 2013-05-30 05:01:11]

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

[commit-bot: I haz the power, 2013-05-30 16:54:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/15350006/109001

[commit-bot: I haz the power, 2013-05-30 19:58:02]

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

[commit-bot: I haz the power, 2013-05-30 22:21:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/15350006/109001

[Alpha Left Google, 2013-05-31 01:13:22]

Failures with win_layout_rel seems to be unrelated. They are not image related
tests. I think you're safe landing this manually.

[commit-bot: I haz the power, 2013-05-31 01:53:19]

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

[Xianzhu, 2013-05-31 16:47:44]

Committed patchset #9 manually as r151560 (presubmit successful).