Blink image-decoders: (lazy) deferred image decoding support for ICO

Blink image-decoders: (lazy) deferred image decoding support for ICO

BUG=513306
Committed: https://crrev.com/de72effc185c6093879a0bc3379e38cb19b5b02b
Cr-Commit-Position: refs/heads/master@{#399855}

[aleksandar.stojiljkovic, 2016-06-07 12:32:52]

aleksandar.stojiljkovic@intel.com changed reviewers:
+ fmalita@chromium.org, noel@chromium.org, pkasting@chromium.org,
scroggo@chromium.org

[aleksandar.stojiljkovic, 2016-06-07 12:32:53]

Please suggest additional tests to add.
There are ICO related tests spread around and added byte by byte decoding here.

Related to the behavior expected in
DeferredImageDecoder::prepareLazyDecodedFrames that all the frames before
partially available frame are expected to be fully received:
this behavior is one of the reasons that frame count is implemented here similar
to WebPImageDecoder [1] - it shouldn't return frame count from header while all
the frames are not received. Instead, returned frame count is the size of list
of completely received frames where the last frame could be partially received.
I could modify this to be the same as in WebP case - that the frameCount
includes only fully received frames - might make more sense.

[1]
https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou...

[scroggo_chromium, 2016-06-07 17:19:28]

On 2016/06/07 12:32:53, aleksandar.stojiljkovic wrote:
> Please suggest additional tests to add.
> There are ICO related tests spread around and added byte by byte decoding
here.
> 
> Related to the behavior expected in
> DeferredImageDecoder::prepareLazyDecodedFrames that all the frames before
> partially available frame are expected to be fully received:
> this behavior is one of the reasons that frame count is implemented here
similar
> to WebPImageDecoder [1] - it shouldn't return frame count from header while
all
> the frames are not received. Instead, returned frame count is the size of list
> of completely received frames where the last frame could be partially
received.
> I could modify this to be the same as in WebP case - that the frameCount
> includes only fully received frames - might make more sense.

I don't have a strong preference between matching GIF versus matching WebP.
Ideally, all three should match.

> 
> [1]
>
https://codereview.chromium.org/1962563002/diff/140001/third_party/WebKit/Sou...

https://codereview.chromium.org/2044093002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
(right):

https://codereview.chromium.org/2044093002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:187:
ASSERT(m_frameBufferCache.size() == decodeFrameCount());
I don't think you should call a non-const method in an ASSERT.

[f(malita), 2016-06-07 17:57:53]

https://codereview.chromium.org/2044093002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp
(right):

https://codereview.chromium.org/2044093002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp:29:
*/
Please use the current license header
(https://www.chromium.org/developers/coding-style#TOC-File-headers) for new
files.

[aleksandar.stojiljkovic, 2016-06-07 18:01:26]

On 2016/06/07 17:19:28, scroggo_chromium wrote:
> On 2016/06/07 12:32:53, aleksandar.stojiljkovic wrote:
> > Please suggest additional tests to add.
> > There are ICO related tests spread around and added byte by byte decoding
> here.
> > 
> > Related to the behavior expected in
> > DeferredImageDecoder::prepareLazyDecodedFrames that all the frames before
> > partially available frame are expected to be fully received:
> > this behavior is one of the reasons that frame count is implemented here
> similar
> > to WebPImageDecoder [1] - it shouldn't return frame count from header while
> all
> > the frames are not received. Instead, returned frame count is the size of
list
> > of completely received frames where the last frame could be partially
> received.
> > I could modify this to be the same as in WebP case - that the frameCount
> > includes only fully received frames - might make more sense.
> 
> I don't have a strong preference between matching GIF versus matching WebP.
> Ideally, all three should match.
Thanks.
Displaying partially decoded frames in an interesting feature for large size GIF
animations but it consumes extra power.
I did it this way because there is unit test
DeferredImageDecoderTestWoPlatform.mixImagesIco testing partially decoded ICO
that would crash (frame count is 0 until the file is fully received) and wanted
to keep the test to verify implementation.
After some thought, don't think that partially decoded icon is as good of an
idea as partially decoded large size gif frame or JPEG.
Let me check what the test is testing and to remove it and return only
completely available frames in frameCount.

https://codereview.chromium.org/2044093002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
(right):

https://codereview.chromium.org/2044093002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:187:
ASSERT(m_frameBufferCache.size() == decodeFrameCount());
On 2016/06/07 17:19:28, scroggo_chromium wrote:
> I don't think you should call a non-const method in an ASSERT.

Thanks, removed it. The assert wasn't that useful as decodeFrameCount gets
called before this from ImageDecoder::frameBufferAtIndex.

[aleksandar.stojiljkovic, 2016-06-07 19:12:24]

On 2016/06/07 18:01:26, aleksandar.stojiljkovic wrote:
> On 2016/06/07 17:19:28, scroggo_chromium wrote:
> > On 2016/06/07 12:32:53, aleksandar.stojiljkovic wrote:
> > > Please suggest additional tests to add.
> > > There are ICO related tests spread around and added byte by byte decoding
> > here.
> > > 
> > > Related to the behavior expected in
> > > DeferredImageDecoder::prepareLazyDecodedFrames that all the frames before
> > > partially available frame are expected to be fully received:
> > > this behavior is one of the reasons that frame count is implemented here
> > similar
> > > to WebPImageDecoder [1] - it shouldn't return frame count from header
while
> > all
> > > the frames are not received. Instead, returned frame count is the size of
> list
> > > of completely received frames where the last frame could be partially
> > received.
> > > I could modify this to be the same as in WebP case - that the frameCount
> > > includes only fully received frames - might make more sense.
> > 
> > I don't have a strong preference between matching GIF versus matching WebP.
> > Ideally, all three should match.
> Thanks.
> Displaying partially decoded frames in an interesting feature for large size
GIF
> animations but it consumes extra power.
> I did it this way because there is unit test
> DeferredImageDecoderTestWoPlatform.mixImagesIco testing partially decoded ICO
> that would crash (frame count is 0 until the file is fully received) and
wanted
> to keep the test to verify implementation.
> After some thought, don't think that partially decoded icon is as good of an
> idea as partially decoded large size gif frame or JPEG.
> Let me check what the test is testing and to remove it and return only
> completely available frames in frameCount.

Done in patchset 3.

https://codereview.chromium.org/2044093002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp
(right):

https://codereview.chromium.org/2044093002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp:29:
*/
On 2016/06/07 17:57:53, f(malita) wrote:
> Please use the current license header
> (https://www.chromium.org/developers/coding-style#TOC-File-headers) for new
> files.

Done.

https://codereview.chromium.org/2044093002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTestWoPlatform.cpp
(right):

https://codereview.chromium.org/2044093002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTestWoPlatform.cpp:79:
mixImages("/LayoutTests/fast/images/resources/wrong-frame-dimensions.ico",
1376u, 1u);
This file corresponds to mixImages test description - decodes first frame 1 and
then frame 0.

[scroggo_chromium, 2016-06-07 19:22:37]

https://codereview.chromium.org/2044093002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTestWoPlatform.cpp
(right):

https://codereview.chromium.org/2044093002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTestWoPlatform.cpp:79:
mixImages("/LayoutTests/fast/images/resources/wrong-frame-dimensions.ico",
1376u, 1u);
On 2016/06/07 19:12:24, aleksandar.stojiljkovic wrote:
> This file corresponds to mixImages test description - decodes first frame 1
and
> then frame 0.

That's not what the mixImages test is about. Notice that it is used for other
formats, where the frames have to come in order.

wrong-frame-dimensions.ico might have frames out of order, but the test is
actually about the order the client attempts to decode.

[aleksandar.stojiljkovic, 2016-06-07 19:35:54]

On 2016/06/07 19:22:37, scroggo_chromium wrote:
>
https://codereview.chromium.org/2044093002/diff/40001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTestWoPlatform.cpp
> (right):
> 
>
https://codereview.chromium.org/2044093002/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTestWoPlatform.cpp:79:
> mixImages("/LayoutTests/fast/images/resources/wrong-frame-dimensions.ico",
> 1376u, 1u);
> On 2016/06/07 19:12:24, aleksandar.stojiljkovic wrote:
> > This file corresponds to mixImages test description - decodes first frame 1
> and
> > then frame 0.
> 
> That's not what the mixImages test is about. Notice that it is used for other
> formats, where the frames have to come in order.
> 
> wrong-frame-dimensions.ico might have frames out of order, but the test is
> actually about the order the client attempts to decode.

No, wrong-frame-dimensions.ico has frames in required order for this test -
frame 0 starts at byte offset 70 and is 1306 bytes long. Frame 1 starts at 1376
and is 1778 bytes long.

[scroggo_chromium, 2016-06-07 19:57:03]

On 2016/06/07 19:35:54, aleksandar.stojiljkovic wrote:
> On 2016/06/07 19:22:37, scroggo_chromium wrote:
> >
>
https://codereview.chromium.org/2044093002/diff/40001/third_party/WebKit/Sour...
> > File
> >
>
third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTestWoPlatform.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2044093002/diff/40001/third_party/WebKit/Sour...
> >
>
third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTestWoPlatform.cpp:79:
> > mixImages("/LayoutTests/fast/images/resources/wrong-frame-dimensions.ico",
> > 1376u, 1u);
> > On 2016/06/07 19:12:24, aleksandar.stojiljkovic wrote:
> > > This file corresponds to mixImages test description - decodes first frame
1
> > and
> > > then frame 0.
> > 
> > That's not what the mixImages test is about. Notice that it is used for
other
> > formats, where the frames have to come in order.
> > 
> > wrong-frame-dimensions.ico might have frames out of order, but the test is
> > actually about the order the client attempts to decode.
> 
> No, wrong-frame-dimensions.ico has frames in required order for this test -
> frame 0 starts at byte offset 70 and is 1306 bytes long. Frame 1 starts at
1376
> and is 1778 bytes long.

Ah, sorry, I misunderstood your previous comment. lgtm

[aleksandar.stojiljkovic, 2016-06-07 20:34:28]

On 2016/06/07 17:19:28, scroggo_chromium wrote:
> I don't have a strong preference between matching GIF versus matching WebP.
> Ideally, all three should match.

Thanks. I'll continue with this by sending "Intent to remove displaying
partially received frames while rendering partially received GIF animation" to
blink-dev. Or something else with shorter title.

[Peter Kasting, 2016-06-07 21:25:39]

On 2016/06/07 20:34:28, aleksandar.stojiljkovic wrote:
> On 2016/06/07 17:19:28, scroggo_chromium wrote:
> > I don't have a strong preference between matching GIF versus matching WebP.
> > Ideally, all three should match.
> 
> Thanks. I'll continue with this by sending "Intent to remove displaying
> partially received frames while rendering partially received GIF animation" to
> blink-dev. Or something else with shorter title.

Are you sure we do such a thing?  There used to be explicit code not to advance
animations to incomplete frames.  I've never seen us advance an animation to an
incomplete frame.

[aleksandar.stojiljkovic, 2016-06-08 10:16:50]

On 2016/06/07 21:25:39, Peter Kasting wrote:
> On 2016/06/07 20:34:28, aleksandar.stojiljkovic wrote:
> > On 2016/06/07 17:19:28, scroggo_chromium wrote:
> > > I don't have a strong preference between matching GIF versus matching
WebP.
> > > Ideally, all three should match.
> > 
> > Thanks. I'll continue with this by sending "Intent to remove displaying
> > partially received frames while rendering partially received GIF animation"
to
> > blink-dev. Or something else with shorter title.
> 
> Are you sure we do such a thing?  There used to be explicit code not to
advance
> animations to incomplete frames.  I've never seen us advance an animation to
an
> incomplete frame.

Uploaded the video here [1] to illustrate the problem. The video was taken six
months ago so I need to still verify this is the case with more recent code
before sending the intent to remove message.

[1]
https://bugs.chromium.org/p/chromium/issues/detail?id=490895#c12

[Peter Kasting, 2016-06-08 21:44:28]

On Wed, Jun 8, 2016 at 3:16 AM, <aleksandar.stojiljkovic@intel.com> wrote:

> On 2016/06/07 21:25:39, Peter Kasting wrote:
> > On 2016/06/07 20:34:28, aleksandar.stojiljkovic wrote:
> > > On 2016/06/07 17:19:28, scroggo_chromium wrote:
> > > > I don't have a strong preference between matching GIF versus matching
> WebP.
> > > > Ideally, all three should match.
> > >
> > > Thanks. I'll continue with this by sending "Intent to remove displaying
> > > partially received frames while rendering partially received GIF
> animation"
> to
> > > blink-dev. Or something else with shorter title.
> >
> > Are you sure we do such a thing? There used to be explicit code not to
> advance
> > animations to incomplete frames. I've never seen us advance an animation
> to
> an
> > incomplete frame.
>
> Uploaded the video here [1] to illustrate the problem. The video was taken
> six
> months ago so I need to still verify this is the case with more recent code
> before sending the intent to remove message.
>

As noted there, this looks like an issue with displaying an incomplete
first frame of the image, which is a different question than the issue of
advancing the animation to an incomplete frame.  The current behavior --
where we draw the incomplete first frame, but we don't replace it with
another frame until the next frame is complete -- seems more correct to me
than not drawing anything at all until we have the full first frame.  If
you're going to unify behavior, I'd unify on that.

PK

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2016-06-08 21:44:29]

On Wed, Jun 8, 2016 at 3:16 AM, <aleksandar.stojiljkovic@intel.com> wrote:

> On 2016/06/07 21:25:39, Peter Kasting wrote:
> > On 2016/06/07 20:34:28, aleksandar.stojiljkovic wrote:
> > > On 2016/06/07 17:19:28, scroggo_chromium wrote:
> > > > I don't have a strong preference between matching GIF versus matching
> WebP.
> > > > Ideally, all three should match.
> > >
> > > Thanks. I'll continue with this by sending "Intent to remove displaying
> > > partially received frames while rendering partially received GIF
> animation"
> to
> > > blink-dev. Or something else with shorter title.
> >
> > Are you sure we do such a thing? There used to be explicit code not to
> advance
> > animations to incomplete frames. I've never seen us advance an animation
> to
> an
> > incomplete frame.
>
> Uploaded the video here [1] to illustrate the problem. The video was taken
> six
> months ago so I need to still verify this is the case with more recent code
> before sending the intent to remove message.
>

As noted there, this looks like an issue with displaying an incomplete
first frame of the image, which is a different question than the issue of
advancing the animation to an incomplete frame.  The current behavior --
where we draw the incomplete first frame, but we don't replace it with
another frame until the next frame is complete -- seems more correct to me
than not drawing anything at all until we have the full first frame.  If
you're going to unify behavior, I'd unify on that.

PK

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2016-06-08 23:16:14]

LGTM

https://codereview.chromium.org/2044093002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTestWoPlatform.cpp
(right):

https://codereview.chromium.org/2044093002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTestWoPlatform.cpp:79:
mixImages("/LayoutTests/fast/images/resources/wrong-frame-dimensions.ico",
1376u, 1u);
On 2016/06/07 19:22:37, scroggo_chromium wrote:
> On 2016/06/07 19:12:24, aleksandar.stojiljkovic wrote:
> > This file corresponds to mixImages test description - decodes first frame 1
> and
> > then frame 0.
> 
> That's not what the mixImages test is about. Notice that it is used for other
> formats, where the frames have to come in order.
> 
> wrong-frame-dimensions.ico might have frames out of order, but the test is
> actually about the order the client attempts to decode.

Yeah, I think the change to this file should be reverted.

https://codereview.chromium.org/2044093002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
(left):

https://codereview.chromium.org/2044093002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:110: 
Nit: Don't remove this

https://codereview.chromium.org/2044093002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp
(right):

https://codereview.chromium.org/2044093002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp:19:
} // anonymous namespace
Nit: I'd just remove this comment since the namespace is so short

[aleksandar.stojiljkovic, 2016-06-09 20:34:07]

Patchset 4. Nits. Thanks pkasting@.

https://codereview.chromium.org/2044093002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTestWoPlatform.cpp
(right):

https://codereview.chromium.org/2044093002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTestWoPlatform.cpp:79:
mixImages("/LayoutTests/fast/images/resources/wrong-frame-dimensions.ico",
1376u, 1u);
On 2016/06/08 23:16:13, Peter Kasting wrote:
> On 2016/06/07 19:22:37, scroggo_chromium wrote:
> > On 2016/06/07 19:12:24, aleksandar.stojiljkovic wrote:
> > > This file corresponds to mixImages test description - decodes first frame
1
> > and
> > > then frame 0.
> > 
> > That's not what the mixImages test is about. Notice that it is used for
other
> > formats, where the frames have to come in order.
> > 
> > wrong-frame-dimensions.ico might have frames out of order, but the test is
> > actually about the order the client attempts to decode.
> 
> Yeah, I think the change to this file should be reverted.

Sorry, I didn't put original response to Leon [1] inline.
Verified that the other file (wrong-frame-dimensions.ico) is better for testing
this as it has multiple frames and they are in order.
wrong-frame-dimensions.ico has frames in required order for this test -
frame 0 starts at byte offset 70 and is 1306 bytes long. Frame 1 starts at 1376
and is 1778 bytes long.
One the other side 1bit.ico is not useful for this test as it has only one
frame.
[1]
https://codereview.chromium.org/2044093002/#msg8

https://codereview.chromium.org/2044093002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp
(left):

https://codereview.chromium.org/2044093002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.cpp:110: 
On 2016/06/08 23:16:13, Peter Kasting wrote:
> Nit: Don't remove this

Done.

https://codereview.chromium.org/2044093002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp
(right):

https://codereview.chromium.org/2044093002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoderTest.cpp:19:
} // anonymous namespace
On 2016/06/08 23:16:13, Peter Kasting wrote:
> Nit: I'd just remove this comment since the namespace is so short

Done.

[aleksandar.stojiljkovic, 2016-06-09 20:37:23]

fmalita@, the patch would also need your approval, especially for the changes in
platform/graphics. Thanks.

[f(malita), 2016-06-09 20:44:54]

LGTM

[aleksandar.stojiljkovic, 2016-06-12 13:11:43]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com

[aleksandar.stojiljkovic, 2016-06-12 13:11:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
scroggo@chromium.org, pkasting@chromium.org, fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/2044093002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-12 13:11:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2044093002/80001

[commit-bot: I haz the power, 2016-06-12 14:26:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-12 14:26:12]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aleksandar.stojiljkovic, 2016-06-12 21:25:27]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-12 21:25:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2044093002/100001

[commit-bot: I haz the power, 2016-06-12 22:37:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-12 22:37:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aleksandar.stojiljkovic, 2016-06-13 10:00:19]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-13 10:00:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2044093002/120001

[commit-bot: I haz the power, 2016-06-13 13:13:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-13 13:13:34]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[aleksandar.stojiljkovic, 2016-06-13 15:26:26]

Failing layout tests fixes:
Patch Set 6 : copy greenbox-3frames.cur to decoder unittest resources instead of
adding the file to .isolate.

Patch Set 7 : hotSpot implementation to support deferred decoding.

This seems to be all causing the failed tests - win_chromium_rel_ng fails and
timeouts without patches today. I'll wait for it to stabilize before verify
dry-run on in_chromium_rel_ng bot again.

[aleksandar.stojiljkovic, 2016-06-14 07:51:04]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-14 07:51:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2044093002/120001

[commit-bot: I haz the power, 2016-06-14 10:34:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-14 10:34:33]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[aleksandar.stojiljkovic, 2016-06-14 13:47:03]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-14 13:47:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2044093002/120001

[commit-bot: I haz the power, 2016-06-14 15:17:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-14 15:17:14]

Dry run: This issue passed the CQ dry run.

[aleksandar.stojiljkovic, 2016-06-14 15:21:31]

On 2016/06/14 15:17:14, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Passed now. Patchset 6 and 7 (that fixed layout tests) are not reviewed. I will
wait for somebody to review them before committing. Thanks.

[Peter Kasting, 2016-06-14 17:55:25]

LGTM

https://codereview.chromium.org/2044093002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.h
(right):

https://codereview.chromium.org/2044093002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/image-decoders/ico/ICOImageDecoder.h:54: 
Nit: No blank

[aleksandar.stojiljkovic, 2016-06-14 18:05:29]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com

[aleksandar.stojiljkovic, 2016-06-14 18:05:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
scroggo@chromium.org, fmalita@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2044093002/#ps140001
(title: "Nits")

[commit-bot: I haz the power, 2016-06-14 18:05:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2044093002/140001

[commit-bot: I haz the power, 2016-06-14 21:39:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-14 21:39:55]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[aleksandar.stojiljkovic, 2016-06-15 05:28:46]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com

[commit-bot: I haz the power, 2016-06-15 05:28:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2044093002/140001

[commit-bot: I haz the power, 2016-06-15 06:51:29]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-15 06:56:42]

Description was changed from

==========
Blink image-decoders: (lazy) deferred image decoding support for ICO

BUG=513306
==========

to

==========
Blink image-decoders: (lazy) deferred image decoding support for ICO

BUG=513306

Committed: https://crrev.com/de72effc185c6093879a0bc3379e38cb19b5b02b
Cr-Commit-Position: refs/heads/master@{#399855}
==========

[commit-bot: I haz the power, 2016-06-15 06:56:44]

Patchset 8 (id:??) landed as
https://crrev.com/de72effc185c6093879a0bc3379e38cb19b5b02b
Cr-Commit-Position: refs/heads/master@{#399855}

[tommycli, 2016-06-15 22:35:13]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2070723002/ by tommycli@chromium.org.

The reason for reverting is: I think this broke a Dr Memory bot:

First breaking build:
https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%2...

ICOImageDecoderTests.parseAndDecodeByteByByte:
c:\b\build\slave\drm-cr\build\src\third_party\webkit\source\platform\image-decoders\imagedecodertesthelpers.cpp(83):
error: Expected: (frameCount) <= (decoder->frameCount()), actual: 2 vs 0
c:\b\build\slave\drm-cr\build\src\third_party\webkit\source\platform\image-decoders\imagedecodertesthelpers.cpp(83):
error: Expected: (frameCount) <= (decoder->frameCount()), actual: 3 vs 0
c:\b\build\slave\drm-cr\build\src\third_party\webkit\source\platform\image-decoders\imagedecodertesthelpers.cpp(83):
error: Expected: (frameCount) <= (decoder->frameCount()), actual: 1 vs 0
c:\b\build\slave\drm-cr\build\src\third_party\webkit\source\platform\image-decoders\imagedecodertesthelpers.cpp(83):
error: Expected: (frameCount) <= (decoder->frameCount()), actual: 1 vs 0.

[aleksandar.stojiljkovic, 2016-06-16 07:19:22]

Description was changed from

==========
Blink image-decoders: (lazy) deferred image decoding support for ICO

BUG=513306

Committed: https://crrev.com/de72effc185c6093879a0bc3379e38cb19b5b02b
Cr-Commit-Position: refs/heads/master@{#399855}
==========

to

==========
Blink image-decoders: (lazy) deferred image decoding support for ICO

BUG=513306

Committed: https://crrev.com/de72effc185c6093879a0bc3379e38cb19b5b02b
Cr-Commit-Position: refs/heads/master@{#399855}
==========

[aleksandar.stojiljkovic, 2016-06-16 10:30:51]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-16 10:31:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2044093002/180001

[aleksandar.stojiljkovic, 2016-06-16 10:48:51]

The CQ bit was unchecked by aleksandar.stojiljkovic@intel.com

[aleksandar.stojiljkovic, 2016-06-16 11:16:37]

On 2016/06/15 22:35:13, tommycli wrote:
> A revert of this CL (patchset #8 id:140001) has been created in
> https://codereview.chromium.org/2070723002/ by mailto:tommycli@chromium.org.
> 
> The reason for reverting is: I think this broke a Dr Memory bot:
> 
> First breaking build:
>
https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%2...
> 

Sorry for this.
Patch Set 10 should fix it. Copied the approach from WebP and GIF decoders'
decodeFrameCount. In case of failure, decodeFrameCount returns existing number
of frames.

I cannot access chromium.memory.fyi server from here to try Windows Unit
(DrMemory) bot [1} that was failing.

I tried with master.tryserver.webrtc bots but it was not a good idea as patch
doesn't apply (.gitignore ignores .cur file).

After review of patch set 10, I plan to submit it again - unless there is a way
to try Windows Unit (DrMemory) before doing it.

[1]
https://build.chromium.org/p/chromium.memory.fyi/waterfall?show=Windows%20Uni...

[Peter Kasting, 2016-06-16 11:23:07]

Please don't upload new patch sets for a CL after it lands.  This really muddies
the waters as to what go landed when.

Instead, delete the new patch sets, and create a new CL, where the first patch
set uploaded is whatever landed on the old CL, and then your next patch set is
the new version (so reviewers can look at the diff between 1 and 2).

[aleksandar.stojiljkovic, 2016-06-16 11:47:29]

Patchset #9 (id:160001) has been deleted

[aleksandar.stojiljkovic, 2016-06-16 11:47:49]

Patchset #9 (id:180001) has been deleted

[aleksandar.stojiljkovic, 2016-06-16 12:01:03]

On 2016/06/16 11:23:07, Peter Kasting wrote:
> Please don't upload new patch sets for a CL after it lands.  This really
muddies
> the waters as to what go landed when.
> 
> Instead, delete the new patch sets, and create a new CL, where the first patch
> set uploaded is whatever landed on the old CL, and then your next patch set is
> the new version (so reviewers can look at the diff between 1 and 2).

Thanks. Did it that way. New patch:
https://codereview.chromium.org/2065423003