Animated WebP: Optimize decoding in case of seeking

Animated WebP: Optimize decoding in case of seeking

In case of WebP, it can be inferred whether an animation frame contains
alpha without decoding the frame.
WebP also has a mode where a frame overwrites the corresponding
rectangle on the previous frame.
These two were added to Chromium here:
https://codereview.chromium.org/22802020/

So, we optimize findRequiredPreviousFrame() for cases when a frame is
known to be opaque or it overwrites pixels on the previous frame.

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

[Peter Kasting, 2013-08-22 00:08:36]

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

https://codereview.chromium.org/23068027/diff/3001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:72: };
I don't think I really understand what's going on with this enum.

Is this for cases where you only want to update a subframe of the image (and so
you want DisposeKeep for the previous frame, to preserve the space outside that
subframe), but within the subframe, you need to overwrite at least some of the
previous frame's pixels with new ones that have alpha != 255 (for which you'd
otherwise often have to use DisposeOverwriteBgcolor)?

If so, I suggest:
(1) Putting that motivation in a comment somewhere to give the reader context.
(2) Calling the two states "BlendAtopPreviousFrame" and "BlendAtopBgcolor"
(3) Possibly calling the enum AlphaBlendSourceForSubframe or some other name
that tries to be more descriptive about (a) this only happening for non-opaque
pixels and (b) this only happening within the current subframe (and being
unnecessary when the subframe is the full frame, since you'd just use an
appropriate disposal method in that case).

Otherwise, please tell me where I got things wrong.

[urvang (Google), 2013-08-22 01:49:35]

Thanks Peter!
Pls check if the comments make more sense now.

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

https://codereview.chromium.org/23068027/diff/3001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:72: };
On 2013/08/22 00:08:37, Peter Kasting wrote:
> I don't think I really understand what's going on with this enum.
> 
> Is this for cases where you only want to update a subframe of the image (and
so
> you want DisposeKeep for the previous frame, to preserve the space outside
that
> subframe), but within the subframe, you need to overwrite at least some of the
> previous frame's pixels with new ones that have alpha != 255 (for which you'd
> otherwise often have to use DisposeOverwriteBgcolor)?

That's the idea, more of less.

To be more precise:  BlendOverwrite is useful for cases where one wants to
transform a few opaque pixels of previous frame into non-opaque pixels in the
current frame.

> 
> If so, I suggest:
> (1) Putting that motivation in a comment somewhere to give the reader context.

Done.

> (2) Calling the two states "BlendAtopPreviousFrame" and "BlendAtopBgcolor"

Done.

> (3) Possibly calling the enum AlphaBlendSourceForSubframe or some other name

Done.

> that tries to be more descriptive about (a) this only happening for non-opaque
> pixels and 


> (b) this only happening within the current subframe (and being
> unnecessary when the subframe is the full frame, since you'd just use an
> appropriate disposal method in that case).
> 

This isn't accurate actually.

Note that 'dispose method' specifies how a frame is treated after it is
displayed, while the 'blending method' specifies how the current frame is
blended with previous frame before display.

For example: consider a case where frame 2 is not a full frame, but frame 3 is.
Now, if frame 3 wanted to overwrite all pixels, none of the disposal method
would work (in the absence of BlendOverwrite).

[This is because frame disposal works on the frame rectangle of previous frame
only, and not the full previous frame].

See also:
https://gerrit.chromium.org/gerrit/#/c/65379/4/doc/webp-container-spec.txt

[Peter Kasting, 2013-08-22 20:04:36]

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

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.cpp:165: &&
currBuffer->originalFrameRect().contains(IntRect(IntPoint(), size())))
Nit: Since we clamp the frame rect to not extend outside the image, should this
just be an equality check instead of contains()?  Or do not all decoders enforce
that?  (Same question later on)

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

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:71: // * WebP also gives
'BlendAtopBgcolor' option. This is useful for cases
Nit: So you never stated clearly in the private email thread whether WebP has an
actual bgcolor concept, or whether "clear to bgcolor" is always equivalent to
"clear to #00000000" or something.  If it's the latter, then you were probably
right that using "bgcolor" here can be misleading, and we should do something
like "Overwrite" or "DoNotBlend" instead.

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:74: enum
AlphaBlendSourceForSubframe {
Nit: Since I was mistaken about thinking this was only necessary for cases where
you had a smaller subframe, and since the enums above both start "Frame", maybe
this should be "FrameCompositionMethod", or  "FrameCompositionSource", or
"FrameAlphaBlendSource".

Bonus points for optionally nuking the word "Frame" out of all three enums,
since they're already in "ImageFrame" and thus "Frame" is implied.

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:75: BlendAtopPreviousFrame,
// Compose the rectangle of current frame by blending it with corresponding
pixels in the previous (disposed) framebuffer.
Nit: Put these comments above the enum values (and then wrap to 80 chars since
the rest of the comments here do so):

    enum X {
        // Blah blah
        X1,

        // Note blank line above for easier scanning
        X2,
    }

Also, maybe the comments should say "...with the corresponding pixels in the
initial buffer state (i.e. any previous frame buffer after having been properly
disposed)." or something (the current comment is a little confusing).

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:408: // and
'frameRectIsOpaque', where 'frameRectIsOpaque' signifies whether
Nit: Use || instead of '' to denote variable names

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
File Source/core/platform/image-decoders/ImageDecoderTest.cpp (right):

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoderTest.cpp:143: // A full frame
with 'blending method == BlendOverwrite' doesn't depend on any prior frames.
Might also want to test that non-full frames with BlendOverwrite or that are
opaque do still depend on prior frames.

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoderTest.cpp:148: Vector<ImageFrame,
1>& decoderFrameBufferCache = decoder->frameBufferCache();
Tiny nit: Honestly, this temp only saves a few characters... bonus points for
fixing all the tests in this file to either not bother to use it, or to have a
much shorter name (I don't have any good ideas).

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoderTest.cpp:172:
decoder->resetRequiredPreviousFrames(true); // Opaque.
Nit: I'd nuke the comment

[urvang (Google), 2013-08-22 23:25:13]

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

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.cpp:165: &&
currBuffer->originalFrameRect().contains(IntRect(IntPoint(), size())))
On 2013/08/22 20:04:36, Peter Kasting wrote:
> Nit: Since we clamp the frame rect to not extend outside the image, should
this
> just be an equality check instead of contains()?  Or do not all decoders
enforce
> that?  (Same question later on)

Good question! I have wondered the same because all the code under
image-decoders/ seems to be using contains (this file itself at line#192,
GIFDecoder.cpp and WEBPImageDecoder.cpp).

Maybe this should all be changed to equality check in a separate patch? I just
feel that it would be good to check that there were no specific reasons for
doing this.

e.g. I see that u first used it in GIF decoder in this patch:
https://chromium.googlesource.com/chromium/blink/+/7263ed7d5fee2d0bf46e921840...
I know that this was long ago, but do you remember if there was a reason not to
use equality at the time?

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

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:71: // * WebP also gives
'BlendAtopBgcolor' option. This is useful for cases
On 2013/08/22 20:04:36, Peter Kasting wrote:
> Nit: So you never stated clearly in the private email thread whether WebP has
an
> actual bgcolor concept, or whether "clear to bgcolor" is always equivalent to
> "clear to #00000000" or something.  If it's the latter, then you were probably
> right that using "bgcolor" here can be misleading, and we should do something
> like "Overwrite" or "DoNotBlend" instead.

Short answer: WebP does have a background color concept:
https://gerrit.chromium.org/gerrit/gitweb?p=webm/libwebp.git;a=blob;f=doc/web...

Long answer: As noted there, renders like Chrome are allowed to ignore it and
use their own background color, which is 0x00000000.
GIF has a similar background color field (see the "background color index" here:
http://www.w3.org/Graphics/GIF/spec-gif89a.txt) and it is ignored by Chrome as
well.

Given this, which name sounds better?

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:74: enum
AlphaBlendSourceForSubframe {
On 2013/08/22 20:04:36, Peter Kasting wrote:
> Nit: Since I was mistaken about thinking this was only necessary for cases
where
> you had a smaller subframe, and since the enums above both start "Frame",
maybe
> this should be "FrameCompositionMethod", or  "FrameCompositionSource", or
> "FrameAlphaBlendSource".

Using the name 'AlphaBlendSource'.

> 
> Bonus points for optionally nuking the word "Frame" out of all three enums,
> since they're already in "ImageFrame" and thus "Frame" is implied.

Done.

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:75: BlendAtopPreviousFrame,
// Compose the rectangle of current frame by blending it with corresponding
pixels in the previous (disposed) framebuffer.
On 2013/08/22 20:04:36, Peter Kasting wrote:
> Nit: Put these comments above the enum values (and then wrap to 80 chars since
> the rest of the comments here do so):
> 
>     enum X {
>         // Blah blah
>         X1,
> 
>         // Note blank line above for easier scanning
>         X2,
>     }
> 
> Also, maybe the comments should say "...with the corresponding pixels in the
> initial buffer state (i.e. any previous frame buffer after having been
properly
> disposed)." or something (the current comment is a little confusing).

Done.

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:408: // and
'frameRectIsOpaque', where 'frameRectIsOpaque' signifies whether
On 2013/08/22 20:04:36, Peter Kasting wrote:
> Nit: Use || instead of '' to denote variable names

Done.

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
File Source/core/platform/image-decoders/ImageDecoderTest.cpp (right):

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoderTest.cpp:143: // A full frame
with 'blending method == BlendOverwrite' doesn't depend on any prior frames.
On 2013/08/22 20:04:36, Peter Kasting wrote:
> Might also want to test that non-full frames with BlendOverwrite or that are
> opaque do still depend on prior frames.

Done.
Using 3 frames instead of 2 to correctly test the 'disposeToPrevios' case.

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoderTest.cpp:148: Vector<ImageFrame,
1>& decoderFrameBufferCache = decoder->frameBufferCache();
On 2013/08/22 20:04:36, Peter Kasting wrote:
> Tiny nit: Honestly, this temp only saves a few characters... bonus points for
> fixing all the tests in this file to either not bother to use it, or to have a
> much shorter name (I don't have any good ideas).

I agree.
Renamed it to 'frameBuffers' to be curt.

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoderTest.cpp:172:
decoder->resetRequiredPreviousFrames(true); // Opaque.
On 2013/08/22 20:04:36, Peter Kasting wrote:
> Nit: I'd nuke the comment

Done.

[Peter Kasting, 2013-08-22 23:39:31]

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

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.cpp:165: &&
currBuffer->originalFrameRect().contains(IntRect(IntPoint(), size())))
On 2013/08/22 23:25:14, urvang wrote:
> On 2013/08/22 20:04:36, Peter Kasting wrote:
> > Nit: Since we clamp the frame rect to not extend outside the image, should
> this
> > just be an equality check instead of contains()?  Or do not all decoders
> enforce
> > that?  (Same question later on)
> 
> Good question! I have wondered the same because all the code under
> image-decoders/ seems to be using contains (this file itself at line#192,
> GIFDecoder.cpp and WEBPImageDecoder.cpp).
> 
> Maybe this should all be changed to equality check in a separate patch? I just
> feel that it would be good to check that there were no specific reasons for
> doing this.
> 
> e.g. I see that u first used it in GIF decoder in this patch:
>
https://chromium.googlesource.com/chromium/blink/+/7263ed7d5fee2d0bf46e921840...
> I know that this was long ago, but do you remember if there was a reason not
to
> use equality at the time?

I probably wasn't sure (as I'm still not sure, offhand) whether the GIF frames
were clamped to the image size.

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

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:71: // * WebP also gives
'BlendAtopBgcolor' option. This is useful for cases
On 2013/08/22 23:25:14, urvang wrote:
> On 2013/08/22 20:04:36, Peter Kasting wrote:
> > Nit: So you never stated clearly in the private email thread whether WebP
has
> an
> > actual bgcolor concept, or whether "clear to bgcolor" is always equivalent
to
> > "clear to #00000000" or something.  If it's the latter, then you were
probably
> > right that using "bgcolor" here can be misleading, and we should do
something
> > like "Overwrite" or "DoNotBlend" instead.
> 
> Short answer: WebP does have a background color concept:
>
https://gerrit.chromium.org/gerrit/gitweb?p=webm/libwebp.git;a=blob;f=doc/web...
> 
> Long answer: As noted there, renders like Chrome are allowed to ignore it and
> use their own background color, which is 0x00000000.
> GIF has a similar background color field (see the "background color index"
here:
> http://www.w3.org/Graphics/GIF/spec-gif89a.txt) and it is ignored by Chrome as
> well.
> 
> Given this, which name sounds better?

Uggghhhh, I can't believe you guys put in something whose interpretation is
optional like this.  It should have either been mandated or dropped entirely. 
Its current state just leads to confusion; who is actually going to respect this
field?  What is the encoder going to assume the decode process will do?

I guess in this case you could say "BlendAtopBgcolor" and document it as "by
blending against transparent, i.e. simply overwriting the corresponding pixels"
or some such.  ("Blending against transparent" is chosen because you stuck with
AlphaBlendSource and for parallel structure with the comment above on
DisposeOverwriteBgcolor.)

[urvang (Google), 2013-08-23 00:13:35]

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

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.cpp:165: &&
currBuffer->originalFrameRect().contains(IntRect(IntPoint(), size())))

> I probably wasn't sure (as I'm still not sure, offhand) whether the GIF frames
> were clamped to the image size.

Yes, the frames are clamed to image size for GIF and WebP:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

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

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:71: // * WebP also gives
'BlendAtopBgcolor' option. This is useful for cases
On 2013/08/22 23:39:31, Peter Kasting wrote:
> Uggghhhh, I can't believe you guys put in something whose interpretation is
> optional like this.  It should have either been mandated or dropped entirely. 
> Its current state just leads to confusion; who is actually going to respect
this
> field?  What is the encoder going to assume the decode process will do?

I completely understand your point. We would have liked to make it fixed either
way as well. But then the scenario is exactly same for GIF as well (even though
the spec doesn't mention it): the browsers always chose transparent background,
some image viewers choose transparent backgrounds, while a few viewers show the
background  color GIF specified. And we wanted to cover that particular use case
of GIF in WebP.

> I guess in this case you could say "BlendAtopBgcolor" and document it as "by
> blending against transparent, i.e. simply overwriting the corresponding
pixels"
> or some such.  ("Blending against transparent" is chosen because you stuck
with
> AlphaBlendSource and for parallel structure with the comment above on
> DisposeOverwriteBgcolor.)

Updated comment.

[Peter Kasting, 2013-08-23 00:17:55]

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

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:71: // * WebP also gives
'BlendAtopBgcolor' option. This is useful for cases
On 2013/08/23 00:13:35, urvang wrote:
> On 2013/08/22 23:39:31, Peter Kasting wrote:
> > Uggghhhh, I can't believe you guys put in something whose interpretation is
> > optional like this.  It should have either been mandated or dropped
entirely. 
> > Its current state just leads to confusion; who is actually going to respect
> this
> > field?  What is the encoder going to assume the decode process will do?
> 
> I completely understand your point. We would have liked to make it fixed
either
> way as well. But then the scenario is exactly same for GIF as well (even
though
> the spec doesn't mention it): the browsers always chose transparent
background,
> some image viewers choose transparent backgrounds, while a few viewers show
the
> background  color GIF specified. And we wanted to cover that particular use
case
> of GIF in WebP.

"Behave inconsistently" is a bug, not a use case.  There's no use case for
people to make images intentionally behave differently on different viewers. 
And certainly anyone transcoding GIF to WebP can afford for WebP to lock this
sort of inconsistency into one behavior or the other.  After all, you didn't
preserve other parts of GIF people don't care about like the pixel aspect ratio
field or GIF Text Mode, did you?

Just nuke it from the WebP spec.  If the spec is locked, then document it as
deprecated, say that all viewers MUST display transparent, and note that its
value will never be set.  Seriously, I'm not kidding.  This is a really bad
thing to have put in the spec.

[urvang (Google), 2013-08-23 00:21:31]

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

https://codereview.chromium.org/23068027/diff/8001/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:71: // * WebP also gives
'BlendAtopBgcolor' option. This is useful for cases
On 2013/08/23 00:17:55, Peter Kasting wrote:
> On 2013/08/23 00:13:35, urvang wrote:
> > On 2013/08/22 23:39:31, Peter Kasting wrote:
> > > Uggghhhh, I can't believe you guys put in something whose interpretation
is
> > > optional like this.  It should have either been mandated or dropped
> entirely. 
> > > Its current state just leads to confusion; who is actually going to
respect
> > this
> > > field?  What is the encoder going to assume the decode process will do?
> > 
> > I completely understand your point. We would have liked to make it fixed
> either
> > way as well. But then the scenario is exactly same for GIF as well (even
> though
> > the spec doesn't mention it): the browsers always chose transparent
> background,
> > some image viewers choose transparent backgrounds, while a few viewers show
> the
> > background  color GIF specified. And we wanted to cover that particular use
> case
> > of GIF in WebP.
> 
> "Behave inconsistently" is a bug, not a use case.  There's no use case for
> people to make images intentionally behave differently on different viewers. 
> And certainly anyone transcoding GIF to WebP can afford for WebP to lock this
> sort of inconsistency into one behavior or the other.  After all, you didn't
> preserve other parts of GIF people don't care about like the pixel aspect
ratio
> field or GIF Text Mode, did you?
> 
> Just nuke it from the WebP spec.  If the spec is locked, then document it as
> deprecated, say that all viewers MUST display transparent, and note that its
> value will never be set.  Seriously, I'm not kidding.  This is a really bad
> thing to have put in the spec.

Point taken.

The spec is, in fact, locked in terms of 'ANIM' chunk (which 'bgcolor' is a part
of). So, I'll discuss with team and possibly mark it as deprecated.

Thanks for the suggestion.

[Peter Kasting, 2013-08-23 01:14:02]

LGTM

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/graph...
File Source/core/platform/graphics/chromium/DeferredImageDecoderTest.cpp
(right):

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/graph...
Source/core/platform/graphics/chromium/DeferredImageDecoderTest.cpp:135:
ImageFrame::Status m_frameStatus;
Nit: Name this m_status (same everywhere else in this patch)

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/graph...
File Source/core/platform/graphics/chromium/ImageDecodingStoreTest.cpp (right):

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/graph...
Source/core/platform/graphics/chromium/ImageDecodingStoreTest.cpp:66: virtual
ImageFrame::Status frameStatus()
Nit: Name this status() (same everywhere else in this patch)

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/graph...
File Source/core/platform/graphics/chromium/ImageFrameGeneratorTest.cpp (right):

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/graph...
Source/core/platform/graphics/chromium/ImageFrameGeneratorTest.cpp:127:
ImageFrame::Status m_nextFrameStatus;
(This can stay as m_nextFrameStatus)

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

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.cpp:163: const ImageFrame*
const currBuffer = &m_frameBufferCache[frameIndex];
Nit: I might avoid this temp just because you only use it twice, in the same
statement...

If you do want to keep it, either remove the second const or make prevBuffer
const.

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

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:63: DisposeOverwriteBgcolor, 
// Clear frame to transparent
Nit: transparent -> fully transparent

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:68: // blended with those of
the previous frame buffer.
Nit: How about:

Indicates how non-opaque pixels in the current frame are blended with those in
the previous frame.

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:71: // * WebP also gives
'BlendAtopBgcolor' option. This is useful for cases
Nit: gives -> uses the

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:72: //   where one wants to
transform a few opaque pixels of previous frame
Nit: previous -> the previous

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:75: // Compose the rectangle
of current frame by blending it with
Nit: Change this first line to "Blend non-opaque pixels atop the"

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:81: // transparent, i.e.
simply overwriting the corresponding pixels.
Nit: How about:

Blend non-opaque pixels against fully transparent (i.e. simply overwrite the
corresponding pixels).

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

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/image...
Source/core/platform/image-decoders/gif/GIFImageReader.h:232: void
setDisposalMethod(WebCore::ImageFrame::DisposalMethod method) { m_disposalMethod
= method; }
Tiny nit: While here, make the argument |disposalMethod|

[urvang (Google), 2013-08-23 02:34:31]

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/graph...
File Source/core/platform/graphics/chromium/DeferredImageDecoderTest.cpp
(right):

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/graph...
Source/core/platform/graphics/chromium/DeferredImageDecoderTest.cpp:135:
ImageFrame::Status m_frameStatus;
On 2013/08/23 01:14:02, Peter Kasting wrote:
> Nit: Name this m_status (same everywhere else in this patch)

Done.

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/graph...
File Source/core/platform/graphics/chromium/ImageDecodingStoreTest.cpp (right):

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/graph...
Source/core/platform/graphics/chromium/ImageDecodingStoreTest.cpp:66: virtual
ImageFrame::Status frameStatus()
On 2013/08/23 01:14:02, Peter Kasting wrote:
> Nit: Name this status() (same everywhere else in this patch)

Done.

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/graph...
File Source/core/platform/graphics/chromium/ImageFrameGeneratorTest.cpp (right):

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/graph...
Source/core/platform/graphics/chromium/ImageFrameGeneratorTest.cpp:127:
ImageFrame::Status m_nextFrameStatus;
On 2013/08/23 01:14:02, Peter Kasting wrote:
> (This can stay as m_nextFrameStatus)

Ack.

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

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.cpp:163: const ImageFrame*
const currBuffer = &m_frameBufferCache[frameIndex];
On 2013/08/23 01:14:02, Peter Kasting wrote:
> Nit: I might avoid this temp just because you only use it twice, in the same
> statement...
> 
> If you do want to keep it, either remove the second const or make prevBuffer
> const.

I'd keep it as it makes the next line more readable. Made it "const ImageFrame*
currBuffer" to be consistent with 'prevBuffer'.

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

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:63: DisposeOverwriteBgcolor, 
// Clear frame to transparent
On 2013/08/23 01:14:02, Peter Kasting wrote:
> Nit: transparent -> fully transparent

Done.

Note: check-webkit-style now complains "single space before end of line
comment". So, I'm forced to move these comments one space after enum values.

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:68: // blended with those of
the previous frame buffer.
On 2013/08/23 01:14:02, Peter Kasting wrote:
> Nit: How about:
> 
> Indicates how non-opaque pixels in the current frame are blended with those in
> the previous frame.

Done. But I'm retaining the term 'frame rectangle' to emphasize that this only
applies to the 'originalframerect' of this frame.

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:71: // * WebP also gives
'BlendAtopBgcolor' option. This is useful for cases
On 2013/08/23 01:14:02, Peter Kasting wrote:
> Nit: gives -> uses the

Done.

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:72: //   where one wants to
transform a few opaque pixels of previous frame
On 2013/08/23 01:14:02, Peter Kasting wrote:
> Nit: previous -> the previous

Done.

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:75: // Compose the rectangle
of current frame by blending it with
On 2013/08/23 01:14:02, Peter Kasting wrote:
> Nit: Change this first line to "Blend non-opaque pixels atop the"

Done.

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/image...
Source/core/platform/image-decoders/ImageDecoder.h:81: // transparent, i.e.
simply overwriting the corresponding pixels.
On 2013/08/23 01:14:02, Peter Kasting wrote:
> Nit: How about:
> 
> Blend non-opaque pixels against fully transparent (i.e. simply overwrite the
> corresponding pixels).

Done.

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

https://codereview.chromium.org/23068027/diff/1009/Source/core/platform/image...
Source/core/platform/image-decoders/gif/GIFImageReader.h:232: void
setDisposalMethod(WebCore::ImageFrame::DisposalMethod method) { m_disposalMethod
= method; }
On 2013/08/23 01:14:02, Peter Kasting wrote:
> Tiny nit: While here, make the argument |disposalMethod|

Done (here and elsewhere in patch).

[urvang (Google), 2013-08-23 18:44:50]

+Darin for OWNERS review on the following files:
Source/core/platform/graphics/chromium/DeferredImageDecoderTest.cpp
    Source/core/platform/graphics/chromium/ImageDecodingStoreTest.cpp
    Source/core/platform/graphics/chromium/ImageFrameGeneratorTest.cpp
    Source/core/platform/graphics/chromium/test/MockImageDecoder.h

@Darin:
These are just some renames for consistency that were suggested during the
review.

[darin (slow to review), 2013-08-23 20:47:32]

LGTM (deferring to pkasting)

[commit-bot: I haz the power, 2013-08-24 00:21:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/urvang@google.com/23068027/44001

[commit-bot: I haz the power, 2013-08-26 18:16:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/urvang@google.com/23068027/54001

[commit-bot: I haz the power, 2013-08-26 20:11:29]

Change committed as 156723