paint: Introduce PaintImage that wraps SkImage in paint calls.

paint: Introduce PaintImage that wraps SkImage in paint calls.

This patch introduces a PaintImage object that wraps an SkImage for the
purposes of paint. It also introduces two states:
AnimationType - indicates whether the image is animated (ie this is a
                single frame of a larger stack of images)
CompletionState - indicates whether this image is fully loaded.

R=enne@chromium.org, khushalsagar@chromium.org
BUG=702386
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2812763002
Cr-Commit-Position: refs/heads/master@{#464643}
Committed: https://chromium.googlesource.com/chromium/src/+/94cfa88dd9922d14e7b08544b3b9710fc53ea94c

[vmpstr, 2017-04-10 22:36:31]

Description was changed from

==========
paint: Introduce PaintImage that wraps SkImage in paint calls.

This patch introduces a PaintImage object that wraps an SkImage for the
purposes of paint. It also introduces two states:
AnimationType - indicates whether the image is animated (ie this is a
                single frame of a larger stack of images)
CompletionState - indicates whether this image is fully loaded.

R=enne@chromium.org, khushalsagar@chromium.org
BUG=702386
==========

to

==========
paint: Introduce PaintImage that wraps SkImage in paint calls.

This patch introduces a PaintImage object that wraps an SkImage for the
purposes of paint. It also introduces two states:
AnimationType - indicates whether the image is animated (ie this is a
                single frame of a larger stack of images)
CompletionState - indicates whether this image is fully loaded.

R=enne@chromium.org, khushalsagar@chromium.org
BUG=702386
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[vmpstr, 2017-04-10 22:36:32]

Description was changed from

==========
paint: Introduce PaintImage that wraps SkImage in paint calls.

This patch introduces a PaintImage object that wraps an SkImage for the
purposes of paint. It also introduces two states:
AnimationType - indicates whether the image is animated (ie this is a
                single frame of a larger stack of images)
CompletionState - indicates whether this image is fully loaded.

R=enne@chromium.org, khushalsagar@chromium.org
BUG=702386
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
paint: Introduce PaintImage that wraps SkImage in paint calls.

This patch introduces a PaintImage object that wraps an SkImage for the
purposes of paint. It also introduces two states:
AnimationType - indicates whether the image is animated (ie this is a
                single frame of a larger stack of images)
CompletionState - indicates whether this image is fully loaded.

R=enne@chromium.org, khushalsagar@chromium.org
BUG=702386
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[vmpstr, 2017-04-10 22:36:36]

The CQ bit was checked by vmpstr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 22:37:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vmpstr, 2017-04-10 22:38:10]

Please take a look.

[enne (OOO), 2017-04-11 00:39:19]

lgtm % khushalsagar

[commit-bot: I haz the power, 2017-04-11 00:55:33]

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

[commit-bot: I haz the power, 2017-04-11 00:55:34]

Dry run: This issue passed the CQ dry run.

[Khushal, 2017-04-11 02:19:46]

https://codereview.chromium.org/2812763002/diff/1/cc/paint/paint_image.h
File cc/paint/paint_image.h (right):

https://codereview.chromium.org/2812763002/diff/1/cc/paint/paint_image.h#newc...
cc/paint/paint_image.h:14: class CC_PAINT_EXPORT PaintImage {
Add a TODO for a persistent id for PaintImage, while the SkImage can still be
updated?

https://codereview.chromium.org/2812763002/diff/1/cc/paint/paint_image.h#newc...
cc/paint/paint_image.h:17: enum class AnimationType { UNKNOWN, ANIMATED, STATIC
};
Do you think we actually need the animation and completion values versus just a
bool, like, can_async_decode_? Blink can set this value based on whether the
image is animated. Similarly the completion state, not sure if we need it right
now.

[vmpstr, 2017-04-11 18:58:34]

https://codereview.chromium.org/2812763002/diff/1/cc/paint/paint_image.h
File cc/paint/paint_image.h (right):

https://codereview.chromium.org/2812763002/diff/1/cc/paint/paint_image.h#newc...
cc/paint/paint_image.h:14: class CC_PAINT_EXPORT PaintImage {
On 2017/04/11 02:19:46, Khushal wrote:
> Add a TODO for a persistent id for PaintImage, while the SkImage can still be
> updated?

Done.

https://codereview.chromium.org/2812763002/diff/1/cc/paint/paint_image.h#newc...
cc/paint/paint_image.h:17: enum class AnimationType { UNKNOWN, ANIMATED, STATIC
};
On 2017/04/11 02:19:46, Khushal wrote:
> Do you think we actually need the animation and completion values versus just
a
> bool, like, can_async_decode_? Blink can set this value based on whether the
> image is animated. Similarly the completion state, not sure if we need it
right
> now.

I'd rather keep the logic for deciding that in cc. Additionally, this is useful
to know for any future animated images in the compositor, whereas
can_async_decode is somewhat limited in the information it provides. I'd also
like to keep a notion of unknown, since it is again up to cc to decide what that
means as opposed to relying on logic in blink to decide whether or not it's ok
to async decode the image.

[Khushal, 2017-04-11 20:55:01]

lgtm.

https://codereview.chromium.org/2812763002/diff/1/cc/paint/paint_image.h
File cc/paint/paint_image.h (right):

https://codereview.chromium.org/2812763002/diff/1/cc/paint/paint_image.h#newc...
cc/paint/paint_image.h:17: enum class AnimationType { UNKNOWN, ANIMATED, STATIC
};
On 2017/04/11 18:58:33, vmpstr wrote:
> On 2017/04/11 02:19:46, Khushal wrote:
> > Do you think we actually need the animation and completion values versus
just
> a
> > bool, like, can_async_decode_? Blink can set this value based on whether the
> > image is animated. Similarly the completion state, not sure if we need it
> right
> > now.
> 
> I'd rather keep the logic for deciding that in cc. Additionally, this is
useful
> to know for any future animated images in the compositor, 

We are using ANIMATED here for the frames in skcanvas_video_renderer, which I
don't think will be the kind of animation that cc will want to care about. Use
something else for this?

My feeling was that it might be easier to just say that based on the info the
caller has, it gets to tell cc whether something is suitable for async updates
or not. Like later for the case of an async attribute for images or the
CrossFadeImageGenerator. If you feel it would be better to have the info for
making that decision plumbed through so it happens later, that should work too.

> can_async_decode is somewhat limited in the information it provides. I'd also
> like to keep a notion of unknown, since it is again up to cc to decide what
that
> means as opposed to relying on logic in blink to decide whether or not it's ok
> to async decode the image.

[vmpstr, 2017-04-11 21:43:52]

vmpstr@chromium.org changed reviewers:
+ dalecurtis@chromium.org, pdr@chromium.org

[vmpstr, 2017-04-11 21:43:52]

+pdr for webkit
+dalecurtis for media

Please take a look.

[pdr., 2017-04-12 17:37:05]

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

https://codereview.chromium.org/2812763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/Image.cpp:252:
PaintImage(std::move(image), animation_type, completion_state), 0, 0,
It looks like PaintImage is used as a single-frame image but isn't your
long-term plan to fully wrap animated gifs so that PaintImage holds more than a
single frame?

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

https://codereview.chromium.org/2812763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:39: auto
animation_type = MaybeAnimated() ? PaintImage::AnimationType::ANIMATED
WDYT of just changing the return type of MaybeAnimated and
CurrentFrameIsComplete to be your new enums?

[DaleCurtis, 2017-04-12 18:27:45]

media/ lgtm

[vmpstr, 2017-04-12 18:58:44]

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

https://codereview.chromium.org/2812763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/Image.cpp:252:
PaintImage(std::move(image), animation_type, completion_state), 0, 0,
On 2017/04/12 17:37:05, pdr. wrote:
> It looks like PaintImage is used as a single-frame image but isn't your
> long-term plan to fully wrap animated gifs so that PaintImage holds more than
a
> single frame?

Yeah, that's the plan. I'm just trying to stage this in pieces, so for now I
just want to introduce a PaintImage which we can work with in cc. When we're
properly plumbing all of the cc stuff, then we'll go back and see how to pack
the full gif into it.

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

https://codereview.chromium.org/2812763002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:39: auto
animation_type = MaybeAnimated() ? PaintImage::AnimationType::ANIMATED
On 2017/04/12 17:37:05, pdr. wrote:
> WDYT of just changing the return type of MaybeAnimated and
> CurrentFrameIsComplete to be your new enums?

Hmm, there are callers outside of graphics that use this (like LayoutBox) and I
think the PaintImage guts should remain limited to graphics.. So I would prefer
to leave that as is. WDYT?

[pdr., 2017-04-12 21:29:38]

On 2017/04/12 at 18:58:44, vmpstr wrote:
>
https://codereview.chromium.org/2812763002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/Image.cpp (right):
> 
>
https://codereview.chromium.org/2812763002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/Image.cpp:252:
PaintImage(std::move(image), animation_type, completion_state), 0, 0,
> On 2017/04/12 17:37:05, pdr. wrote:
> > It looks like PaintImage is used as a single-frame image but isn't your
> > long-term plan to fully wrap animated gifs so that PaintImage holds more
than a
> > single frame?
> 
> Yeah, that's the plan. I'm just trying to stage this in pieces, so for now I
just want to introduce a PaintImage which we can work with in cc. When we're
properly plumbing all of the cc stuff, then we'll go back and see how to pack
the full gif into it.
> 
>
https://codereview.chromium.org/2812763002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp
(right):
> 
>
https://codereview.chromium.org/2812763002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp:39: auto
animation_type = MaybeAnimated() ? PaintImage::AnimationType::ANIMATED
> On 2017/04/12 17:37:05, pdr. wrote:
> > WDYT of just changing the return type of MaybeAnimated and
> > CurrentFrameIsComplete to be your new enums?
> 
> Hmm, there are callers outside of graphics that use this (like LayoutBox) and
I think the PaintImage guts should remain limited to graphics.. So I would
prefer to leave that as is. WDYT?

OK LGTM

[vmpstr, 2017-04-12 22:22:28]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2017-04-12 22:22:29]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
khushalsagar@chromium.org
Link to the patchset: https://codereview.chromium.org/2812763002/#ps40001
(title: "update")

[commit-bot: I haz the power, 2017-04-12 22:23:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 00:07:59]

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

[commit-bot: I haz the power, 2017-04-13 00:08:01]

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

[vmpstr, 2017-04-13 23:02:21]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2017-04-13 23:03:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 01:19:44]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492124541508740,
"parent_rev": "c407e95a371a94bfd714e25eab788c9405de6975", "commit_rev":
"94cfa88dd9922d14e7b08544b3b9710fc53ea94c"}

[commit-bot: I haz the power, 2017-04-14 01:20:33]

Description was changed from

==========
paint: Introduce PaintImage that wraps SkImage in paint calls.

This patch introduces a PaintImage object that wraps an SkImage for the
purposes of paint. It also introduces two states:
AnimationType - indicates whether the image is animated (ie this is a
                single frame of a larger stack of images)
CompletionState - indicates whether this image is fully loaded.

R=enne@chromium.org, khushalsagar@chromium.org
BUG=702386
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
paint: Introduce PaintImage that wraps SkImage in paint calls.

This patch introduces a PaintImage object that wraps an SkImage for the
purposes of paint. It also introduces two states:
AnimationType - indicates whether the image is animated (ie this is a
                single frame of a larger stack of images)
CompletionState - indicates whether this image is fully loaded.

R=enne@chromium.org, khushalsagar@chromium.org
BUG=702386
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2812763002
Cr-Commit-Position: refs/heads/master@{#464643}
Committed:
https://chromium.googlesource.com/chromium/src/+/94cfa88dd9922d14e7b08544b3b9...
==========

[commit-bot: I haz the power, 2017-04-14 01:20:35]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/94cfa88dd9922d14e7b08544b3b9...