cc: Add image decode control in the compositor.

cc: Add image decode control in the compositor.

This patch reworks ImageDecodeController and the surrounding
system to give control over image decodes to the compositor.

Perf note:

This is disabled on android, and should only affect desktop.
That being said, this patch changes the behavior with
respect to image decodes, so some perf changes are expected.

BUG=516751
R=enne, reed1
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/9dd810a16253140466189823394288ed60dfbb30
Cr-Commit-Position: refs/heads/master@{#370188}

[vmpstr, 2015-10-20 00:05:55]

Description was changed from

==========
(not for commit) cc: Image decodes early review patch

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

patch from issue 1385733002 at patchset 80001
(http://crrev.com/1385733002#ps80001)
==========

to

==========
(not for commit) cc: Image decodes early review patch
==========

[vmpstr, 2015-10-26 22:15:23]

Description was changed from

==========
(not for commit) cc: Image decodes early review patch
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
(not for commit) cc: Image decodes early review patch
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[vmpstr, 2015-10-26 22:15:23]

vmpstr@chromium.org changed reviewers:
+ enne@chromium.org, reed@google.com

[vmpstr, 2015-10-26 22:17:00]

Hey, can you peeps take a look at this? This isn't landable, because we need a
better scaling method that doesn't cache the images in skia, but other than that
(and lack of tests) I think this is doing the right thing.

reed@, can you take a detailed look at the skia uses, the nway canvas in
particular. Also, the scaling logic is probably going to change, but as an FYI
that's what I have now..

enne@, can you take a look at everything else?

Thanks.

[reed1, 2015-10-27 17:35:07]

https://codereview.chromium.org/1418573002/diff/80001/cc/playback/display_lis...
File cc/playback/display_list_raster_source.cc (right):

https://codereview.chromium.org/1418573002/diff/80001/cc/playback/display_lis...
cc/playback/display_list_raster_source.cc:65: SkNWayCanvas::scale(1.f /
(decoded_image.scale_adjustment().width()),
Is scale_adjustment = scaled_version.width / original_version ?

If so, then this makes sense (though it might be nicer to avoid the invert here
by storing the inverse division).

[enne (OOO), 2015-10-29 22:59:46]

Can you leave some more comments in general about how this works in the code?

Is this accurate?  When encountering tiles with tasks, create decode image tasks
(if they fit in the budget), decrement the budget for that task.  Make those
decode tasks a dependency on the actual raster.  Cached images get unrefed when
the raster that uses them is done.  When the cached image is no longer ref'ed,
it gets removed from the cache and can be freed.  However, the image was decoded
into discardable memory, so theoretically the next decode could be really fast,
but from the tile manager's perspective, this isn't known.

If the budget is breached, then individual tasks are just handled at raster time
rather than being predecoded.  Individual tasks themselves can go over the
budget temporarily, but no more than the budget will be locked?


How is one raster task using the same image multiple times handled?

Does this patch show improvements on the benchmark you landed earlier?

https://codereview.chromium.org/1418573002/diff/100001/cc/playback/display_li...
File cc/playback/display_list_raster_source.cc (right):

https://codereview.chromium.org/1418573002/diff/100001/cc/playback/display_li...
cc/playback/display_list_raster_source.cc:75: SkNWayCanvas::save();
Is it worth conditionalizing this save and restore if scale is 1?

https://codereview.chromium.org/1418573002/diff/100001/cc/playback/display_li...
File cc/playback/display_list_raster_source.h (right):

https://codereview.chromium.org/1418573002/diff/100001/cc/playback/display_li...
cc/playback/display_list_raster_source.h:110: void
SetImageDecodeController(ImageDecodeController* image_decode_controller);
I think it would be worth commenting about lifetime issues here, in that this
needs to be preserved past the raster source's lifetime.  Also maybe worth
hoisting the comment that this can only be set once to the header.

https://codereview.chromium.org/1418573002/diff/100001/cc/tiles/image_decode_...
File cc/tiles/image_decode_controller.cc (right):

https://codereview.chromium.org/1418573002/diff/100001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:99: return nullptr;
I kind of wish there was a way to differentiate this so that we could UMA stat /
trace when this case of budget overflow is happening vs the image already exists
and is cached?

https://codereview.chromium.org/1418573002/diff/100001/cc/tiles/image_decode_...
File cc/tiles/image_decode_controller.h (right):

https://codereview.chromium.org/1418573002/diff/100001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.h:83: void DrawWithImageFinished(const
DrawImage& image);
What's the Finished part here mean? Could this function and
GetDecodedImageAndRef get some block comments too?

https://codereview.chromium.org/1418573002/diff/100001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.h:86: // Note that this function has to remain
thread safe.
Should other functions check that they're called on the right thread?

[vmpstr, 2015-12-01 00:17:46]

Patchset #18 (id:340001) has been deleted

[vmpstr, 2015-12-01 00:18:00]

Patchset #17 (id:320001) has been deleted

[vmpstr, 2015-12-01 18:25:51]

I've updated this quite a bit since the last review. Please take a look. This is
the patch I'm using to do some telemetry tests. Also, there's a few layout tests
that fail with this, so I'm taking a look at those as well.

[enne (OOO), 2015-12-02 23:33:25]

https://codereview.chromium.org/1418573002/diff/420001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1418573002/diff/420001/cc/layers/picture_laye...
cc/layers/picture_layer_impl.cc:541: // Only set the image decode controller
when we're committing.
...for efficiency's sake? It'd still be set later, right?

Maybe PictureLayer::PushPropertiesTo should just do this always after creating
the raster source during commit, rather than conditionally doing it here?

https://codereview.chromium.org/1418573002/diff/420001/cc/playback/display_li...
File cc/playback/display_list_raster_source.cc (right):

https://codereview.chromium.org/1418573002/diff/420001/cc/playback/display_li...
cc/playback/display_list_raster_source.cc:28: bool ExtractScale(const SkMatrix&
matrix, SkSize* scale) {
Put this somewhere common and don't copy and paste it?

https://codereview.chromium.org/1418573002/diff/420001/cc/playback/display_li...
cc/playback/display_list_raster_source.cc:522: DCHECK(!image_decode_controller_
||
rip DCHECK_IMPLIES

https://codereview.chromium.org/1418573002/diff/420001/cc/playback/display_li...
File cc/playback/display_list_raster_source.h (right):

https://codereview.chromium.org/1418573002/diff/420001/cc/playback/display_li...
cc/playback/display_list_raster_source.h:110: // Image decode controller should
be set once. It's lifetime has to exceed
s/'//

https://codereview.chromium.org/1418573002/diff/420001/cc/playback/draw_image.h
File cc/playback/draw_image.h (right):

https://codereview.chromium.org/1418573002/diff/420001/cc/playback/draw_image...
cc/playback/draw_image.h:48: class DecodedDrawImage {
Different classes, different files?

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
File cc/tiles/image_decode_controller.cc (right):

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:97: // If the image already exist or if
we're going to create a task for it, then
exists

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:107: // image the first time we see it. This
doesn't need to account for memory.
Should we preroll once per frame per image to fix the "first time seen"
nonsense? If it's still in discardable memory, shouldn't it be fast to preroll?
Or, would that be a mistake?

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:162: // Actually create the image. RefImage
will account for memory on the first
create the...task?

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:460: //       2b2. ... its ref count is not
0: keep our image locked and replace
Anything that gets into 2b2 outline form scares me a tiny bit.  Is there any way
to unit test all the unlocking scenarios?

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:549: DCHECK_EQ(0u,
decoded_images_ref_counts_.size());
This is a lot of surprising checks in a setter function.  Can you summarize this
in the header about when this function is safe to call?

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
File cc/tiles/image_decode_controller.h (right):

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.h:30: bool operator==(const
ImageDecodeControllerKey& other) const {
Provide operator != too for symmetry?

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.h:41: base::CheckedNumeric<size_t> result = 4;
// TODO(vmpstr): handle formats other than RGBA.

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.h:121: // Unrefs an image. This should be
called for every GetDecodedImageAndRef when
GetDecodedImageAndRef == GetDecodedImageForDraw?

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.h:202: // The the members can only be accessed
if the lock is held to ensure that
The the

[vmpstr, 2015-12-03 21:20:23]

ptal

https://codereview.chromium.org/1418573002/diff/420001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1418573002/diff/420001/cc/layers/picture_laye...
cc/layers/picture_layer_impl.cc:541: // Only set the image decode controller
when we're committing.
On 2015/12/02 23:33:25, enne wrote:
> ...for efficiency's sake? It'd still be set later, right?
> 
> Maybe PictureLayer::PushPropertiesTo should just do this always after creating
> the raster source during commit, rather than conditionally doing it here?

This effectively does just this. PictureLayer sets the raster source via this
function during PushPropertiesTo (with pending_set == nullptr)

I can do:

layer_impl->layer_tree_impl()->tile_manager()->GetImageDecodeController()

in PictureLayer::PushPropertiesTobut it just feels a bit yucky to reach into
layer_tree_impl from picture layer :P

Wdyt?

https://codereview.chromium.org/1418573002/diff/420001/cc/playback/display_li...
File cc/playback/display_list_raster_source.cc (right):

https://codereview.chromium.org/1418573002/diff/420001/cc/playback/display_li...
cc/playback/display_list_raster_source.cc:28: bool ExtractScale(const SkMatrix&
matrix, SkSize* scale) {
On 2015/12/02 23:33:25, enne wrote:
> Put this somewhere common and don't copy and paste it?

Put up https://codereview.chromium.org/1497543004/ to move it, but it doesn't
seem like ui/gfx/skia_utils.h is a good place for it (according to the
reviewer). 

I don't really want to put this cc/base/math_util.h because they aren't math
utils, but more of skia_utils which we don't really have. 

I'm gonna keep one copy and define them in discardable_image_map.h. Let me know
if this reads OK. The alternative I guess is cc/base/skia_utils.h which would be
a new file.

Let me know which one you prefer.

https://codereview.chromium.org/1418573002/diff/420001/cc/playback/display_li...
cc/playback/display_list_raster_source.cc:522: DCHECK(!image_decode_controller_
||
On 2015/12/02 23:33:25, enne wrote:
> rip DCHECK_IMPLIES

heh yup!

https://codereview.chromium.org/1418573002/diff/420001/cc/playback/display_li...
File cc/playback/display_list_raster_source.h (right):

https://codereview.chromium.org/1418573002/diff/420001/cc/playback/display_li...
cc/playback/display_list_raster_source.h:110: // Image decode controller should
be set once. It's lifetime has to exceed
On 2015/12/02 23:33:25, enne wrote:
> s/'//

Done.

https://codereview.chromium.org/1418573002/diff/420001/cc/playback/draw_image.h
File cc/playback/draw_image.h (right):

https://codereview.chromium.org/1418573002/diff/420001/cc/playback/draw_image...
cc/playback/draw_image.h:48: class DecodedDrawImage {
On 2015/12/02 23:33:25, enne wrote:
> Different classes, different files?

Done.

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
File cc/tiles/image_decode_controller.cc (right):

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:97: // If the image already exist or if
we're going to create a task for it, then
On 2015/12/02 23:33:25, enne wrote:
> exists

Done.

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:107: // image the first time we see it. This
doesn't need to account for memory.
On 2015/12/02 23:33:25, enne wrote:
> Should we preroll once per frame per image to fix the "first time seen"
> nonsense? If it's still in discardable memory, shouldn't it be fast to
preroll?
> Or, would that be a mistake?

I don't think that would work for GPU raster, because the image could be in the
uploaded cache (yep, there's more caches to consider!)

The more I think about it, the more it seems difficult to unify software and gpu
paths when it comes to the image decode controller. I think once this has to
manage the uploaded cache too, it will have a nice split into two separate
classes... That's kind of as an aside though.

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:162: // Actually create the image. RefImage
will account for memory on the first
On 2015/12/02 23:33:25, enne wrote:
> create the...task?

Yes. Thanks! Done.

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:460: //       2b2. ... its ref count is not
0: keep our image locked and replace
On 2015/12/02 23:33:25, enne wrote:
> Anything that gets into 2b2 outline form scares me a tiny bit.  Is there any
way
> to unit test all the unlocking scenarios?

I've added a bunch of tests that I think cover all of the points. And yes, I
think 2b2 isn't possible, I've changed the comment and added a DCHECK instead of
the if.

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:549: DCHECK_EQ(0u,
decoded_images_ref_counts_.size());
On 2015/12/02 23:33:25, enne wrote:
> This is a lot of surprising checks in a setter function.  Can you summarize
this
> in the header about when this function is safe to call?

Done.

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
File cc/tiles/image_decode_controller.h (right):

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.h:30: bool operator==(const
ImageDecodeControllerKey& other) const {
On 2015/12/02 23:33:25, enne wrote:
> Provide operator != too for symmetry?

Done.

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.h:41: base::CheckedNumeric<size_t> result = 4;
On 2015/12/02 23:33:25, enne wrote:
> // TODO(vmpstr): handle formats other than RGBA.

Done.

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.h:121: // Unrefs an image. This should be
called for every GetDecodedImageAndRef when
On 2015/12/02 23:33:25, enne wrote:
> GetDecodedImageAndRef == GetDecodedImageForDraw?

Err ya, renames gone awry! Done. Thanks.

https://codereview.chromium.org/1418573002/diff/420001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.h:202: // The the members can only be accessed
if the lock is held to ensure that
On 2015/12/02 23:33:25, enne wrote:
> The the

Done. Done.

[ericrk, 2015-12-04 00:50:45]

ericrk@chromium.org changed reviewers:
+ ericrk@chromium.org

[ericrk, 2015-12-04 00:50:46]

https://codereview.chromium.org/1418573002/diff/480001/cc/playback/decoded_dr...
File cc/playback/decoded_draw_image.h (right):

https://codereview.chromium.org/1418573002/diff/480001/cc/playback/decoded_dr...
cc/playback/decoded_draw_image.h:41: SkSize scale_adjustment_;
const? same below.

https://codereview.chromium.org/1418573002/diff/480001/cc/playback/display_li...
File cc/playback/display_list_raster_source.cc (right):

https://codereview.chromium.org/1418573002/diff/480001/cc/playback/display_li...
cc/playback/display_list_raster_source.cc:65: SkMatrix ctm =
this->getTotalMatrix();
why this? skia leaking in?

https://codereview.chromium.org/1418573002/diff/480001/cc/playback/display_li...
File cc/playback/display_list_raster_source.h (right):

https://codereview.chromium.org/1418573002/diff/480001/cc/playback/display_li...
cc/playback/display_list_raster_source.h:140: ImageDecodeController*
image_decode_controller_;
Given all the warnings above re threading, can you add a comment here indicating
why this is threadsafe (I'm assuming it is only set once before any threaded
work begins?)

https://codereview.chromium.org/1418573002/diff/480001/cc/tiles/image_decode_...
File cc/tiles/image_decode_controller.cc (right):

https://codereview.chromium.org/1418573002/diff/480001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:100: // If the image already exists or if
we're going to create a task for it, then
this line is a bit unclear - you say "if we're going to create a task for it,
then we need to ref...", but just below we create a ImageDecodeTaskImpl to
preroll, but don't ref... I get the preroll part, but maybe this comment needs a
few more details?

https://codereview.chromium.org/1418573002/diff/480001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:591:
std::abs(SkScalarRoundToInt(image.image()->width() * scale.width())),
maybe this is fine, but SkScalarRoundToInt is defined as:

#define SkScalarRoundToInt(x)       (int)floor((x) + 0.5)

this means that an image with width 1.0f and scale 0.5f will end up with a size
of 1.0f, while a scale of -0.5f would result in a size of 0.0f.

Would be consistent if you took the abs of the multiplication, before rounding.
- not sure what behavior is desired.

https://codereview.chromium.org/1418573002/diff/480001/cc/tiles/image_decode_...
File cc/tiles/image_decode_controller.h (right):

https://codereview.chromium.org/1418573002/diff/480001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.h:73:
static_cast<uint64>(base::HashPair(key.target_size().width(),
nit: prefer uint64_t

https://codereview.chromium.org/1418573002/diff/480001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.h:176: class MemoryBudget {
Not as part of this CL :D, but can you add a TODO to add a memory infra logger
for this pool so we can see it's memory usage in dumps.

https://codereview.chromium.org/1418573002/diff/480001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.h:221: base::hash_set<uint32_t>
prerolled_images_;
does this need to have the lock held? unclear whether the blank line separates
it from the lock-required members above.

https://codereview.chromium.org/1418573002/diff/480001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1418573002/diff/480001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:648: image_decode_controller_.ReduceCacheUsage();
should we also ReduceCacheUsage in CheckIfMoreTilesNeedToBePrepared (the other
usage of ReduceResourceUsage) - this will ensure we reduce after all tasks
complete, not just when starting a new set of tasks.

[vmpstr, 2015-12-04 01:05:54]

https://codereview.chromium.org/1418573002/diff/480001/cc/playback/display_li...
File cc/playback/display_list_raster_source.cc (right):

https://codereview.chromium.org/1418573002/diff/480001/cc/playback/display_li...
cc/playback/display_list_raster_source.cc:26: return false;
This is debug code :D

[vmpstr, 2015-12-08 19:45:35]

Description was changed from

==========
(not for commit) cc: Image decodes early review patch
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Add image decode control in the compositor.

This patch reworks ImageDecodeController and the surrounding
system to give control over image decodes to the compositor.

Perf note:

This is disabled on android, and should only affect desktop.
That being said, this patch changes the behavior with
respect to image decodes, so some perf changes are expected.

R=enne, reed1
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[vmpstr, 2015-12-08 21:11:22]

Please take another look. I've updated this and I think I've addressed most
comments.

[ericrk, 2015-12-08 21:34:26]

https://codereview.chromium.org/1418573002/diff/620001/cc/playback/discardabl...
File cc/playback/discardable_image_map.h (right):

https://codereview.chromium.org/1418573002/diff/620001/cc/playback/discardabl...
cc/playback/discardable_image_map.h:33: bool ComputeNewRectFromPaintBounds(
nit: name is a bit weird, as we are producing paint_bounds, but the funciton is
called ....FromPaintBounds - which makes it feel like this would be an in
parameter.

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
File cc/tiles/image_decode_controller.cc (right):

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:310: // see if transporting it in the key is
reasonable.
in what cases is the target_size not equal to the value we compute here? is this
just to handle differences in rounding?

[reed1, 2015-12-08 22:01:18]

lgtm w/ a comment/suggestion

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
File cc/tiles/image_decode_controller.cc (right):

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:290:
ImageDecodeController::DecodeImageInternal(const ImageKey& key,
I think this code is doing the following (but I may be wrong)

1. alloc orig pixels at #298
2. decode/read into them at #300
3. wrap that memory into a tmp image at #305
4. allocate scaled pixels at #322
5. scale orig into scaled at #330

I think you can skip steps 1-3, and replace (5) with just:

4. allocate scaled pixels at #322
5. image->scalePixels(scaled_pixmap, kHigh..., kDisallow...);

I think there's no reason to alloc/readPixels if you're only use for them is to
then call scale, as SkImage::scalePixels() does that for you. Just a suggestion.

[vmpstr, 2015-12-08 22:04:49]

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
File cc/tiles/image_decode_controller.cc (right):

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:290:
ImageDecodeController::DecodeImageInternal(const ImageKey& key,
On 2015/12/08 22:01:18, reed1 wrote:
> I think this code is doing the following (but I may be wrong)
> 
> 1. alloc orig pixels at #298
> 2. decode/read into them at #300
> 3. wrap that memory into a tmp image at #305
> 4. allocate scaled pixels at #322
> 5. scale orig into scaled at #330
> 
> I think you can skip steps 1-3, and replace (5) with just:
> 
> 4. allocate scaled pixels at #322
> 5. image->scalePixels(scaled_pixmap, kHigh..., kDisallow...);
> 
> I think there's no reason to alloc/readPixels if you're only use for them is
to
> then call scale, as SkImage::scalePixels() does that for you. Just a
suggestion.

Yeah you're right that this is what the code is doing. The difference is that
readPixels calls passes SkImage::kAllow_CachingHint. That is, I do want the
original sized decode to be cached. When scaling pixels, I disable caching
though. 

The steps you're recommending (removing extra allocs) should happen when this
code will change to cache original sized images (then we can just use that cache
to scale pixels).

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:310: // see if transporting it in the key is
reasonable.
On 2015/12/08 21:34:26, ericrk wrote:
> in what cases is the target_size not equal to the value we compute here? is
this
> just to handle differences in rounding?

The size is different, because target_size refers to the scaled original image.
Here, however, we're using src_rect to extract a rect. I think maybe target_size
can become the src_rect scaled version. I'll see if that makes sense.

This was a patch on top of a patch to fix some layout tests, so if they are
fixed, I'll take a look at cleaning this up. Do you think this is landable as is
with the TODO?

[ericrk, 2015-12-08 22:12:23]

lgtm

lgtm

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
File cc/tiles/image_decode_controller.cc (right):

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:310: // see if transporting it in the key is
reasonable.
On 2015/12/08 22:04:49, vmpstr wrote:
> On 2015/12/08 21:34:26, ericrk wrote:
> > in what cases is the target_size not equal to the value we compute here? is
> this
> > just to handle differences in rounding?
> 
> The size is different, because target_size refers to the scaled original
image.
> Here, however, we're using src_rect to extract a rect. I think maybe
target_size
> can become the src_rect scaled version. I'll see if that makes sense.
> 
> This was a patch on top of a patch to fix some layout tests, so if they are
> fixed, I'll take a look at cleaning this up. Do you think this is landable as
is
> with the TODO?

yup - seems fine, just potentially less efficient

[enne (OOO), 2015-12-08 23:27:24]

https://codereview.chromium.org/1418573002/diff/620001/cc/playback/discardabl...
File cc/playback/discardable_image_map.h (right):

https://codereview.chromium.org/1418573002/diff/620001/cc/playback/discardabl...
cc/playback/discardable_image_map.h:35: const SkPaint* paint,
Maybe calling this arg "current_paint" would also help with clarity.

https://codereview.chromium.org/1418573002/diff/620001/cc/playback/display_li...
File cc/playback/display_list_raster_source.cc (right):

https://codereview.chromium.org/1418573002/diff/620001/cc/playback/display_li...
cc/playback/display_list_raster_source.cc:63: return;
I'm a little confused why this check and paint inspection needs to happen at
raster time.  Doesn't the rtree already know which image ops fit in the right
place?

What use case are you trying to optimize for here?

https://codereview.chromium.org/1418573002/diff/620001/cc/playback/display_li...
cc/playback/display_list_raster_source.cc:140: // Does Blink use this?
grep suggests no.  If we don't have any testing that this code is right, I'd
rather just NOTREACHED it?

https://codereview.chromium.org/1418573002/diff/620001/cc/playback/display_li...
cc/playback/display_list_raster_source.cc:309: // discardable images. This way
we would only pay for the hijack canvas if the
Do you have any sense of what the expense is of the hijack canvas?

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
File cc/tiles/image_decode_controller.cc (right):

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:569: size_t
ImageDecodeController::SanityCheckState(int line, bool lock_acquired) {
Why does this return something?

https://codereview.chromium.org/1418573002/diff/620001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1418573002/diff/620001/content/renderer/rende...
content/renderer/render_thread_impl.cc:761: are_image_decode_tasks_enabled_ =
true;
Should this also be num_raster_threads > 1?

[aelias_OOO_until_Jul13, 2015-12-09 00:09:47]

aelias@chromium.org changed reviewers:
+ aelias@chromium.org

[aelias_OOO_until_Jul13, 2015-12-09 00:09:48]

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
File cc/tiles/image_decode_controller.h (right):

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.h:224: bool ShouldDecodeAndScaleImage(const
ImageKey& key, const DrawImage& image);
CanHandleImage();

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.h:237: base::hash_set<uint32_t>
prerolled_images_;
It took me some time to realize this is only used on the SkImage fallback path. 
I'd suggest bundling it together in the header with all the related stuff and
renaming things like this:

// TODO(vmpstr): We have a temporary fallback path using SkImage preroll for
images ImageDecodeController
// doesn't know how to natively handle yet.  This can go away when
http://crbug.com/558077 and http://crbug.com/558072
// are both fixed.
bool CanHandleFilterQuality(SkFilterQuality filter_quality);
bool CanHandleDecodeAndScaleForImage(const ImageKey& key, const DrawImage&
image);
bool is_using_gpu_rasterization_;
base::hash_set<uint32_t> unhandled_fallback_images_;

[reed1, 2015-12-09 16:03:04]

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
File cc/tiles/image_decode_controller.cc (right):

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:290:
ImageDecodeController::DecodeImageInternal(const ImageKey& key,
On 2015/12/08 22:04:49, vmpstr wrote:
> On 2015/12/08 22:01:18, reed1 wrote:
> > I think this code is doing the following (but I may be wrong)
> > 
> > 1. alloc orig pixels at #298
> > 2. decode/read into them at #300
> > 3. wrap that memory into a tmp image at #305
> > 4. allocate scaled pixels at #322
> > 5. scale orig into scaled at #330
> > 
> > I think you can skip steps 1-3, and replace (5) with just:
> > 
> > 4. allocate scaled pixels at #322
> > 5. image->scalePixels(scaled_pixmap, kHigh..., kDisallow...);
> > 
> > I think there's no reason to alloc/readPixels if you're only use for them is
> to
> > then call scale, as SkImage::scalePixels() does that for you. Just a
> suggestion.
> 
> Yeah you're right that this is what the code is doing. The difference is that
> readPixels calls passes SkImage::kAllow_CachingHint. That is, I do want the
> original sized decode to be cached. When scaling pixels, I disable caching
> though. 
> 
> The steps you're recommending (removing extra allocs) should happen when this
> code will change to cache original sized images (then we can just use that
cache
> to scale pixels).

Got it, thanks.

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:305: skia::RefPtr<SkImage> decoded_image =
skia::AdoptRef(SkImage::NewFromRaster(
Do you need decoded_image? If the only use is so you can call scalePixels, you
could instead call.

SkPixmap::scalePixmap(dst);

Just replace #305 with building a decoded_pixmap. This guys is free (no calls
into new/delete) and is what will get invoked anyway when you call
decoded_image->scalePixels(dst)

[vmpstr, 2015-12-09 23:53:41]

Addressed most comments. Let me know if I missed something.

https://codereview.chromium.org/1418573002/diff/620001/cc/playback/discardabl...
File cc/playback/discardable_image_map.h (right):

https://codereview.chromium.org/1418573002/diff/620001/cc/playback/discardabl...
cc/playback/discardable_image_map.h:33: bool ComputeNewRectFromPaintBounds(
On 2015/12/08 21:34:26, ericrk wrote:
> nit: name is a bit weird, as we are producing paint_bounds, but the funciton
is
> called ....FromPaintBounds - which makes it feel like this would be an in
> parameter.

Renamed.

https://codereview.chromium.org/1418573002/diff/620001/cc/playback/discardabl...
cc/playback/discardable_image_map.h:35: const SkPaint* paint,
On 2015/12/08 23:27:24, enne wrote:
> Maybe calling this arg "current_paint" would also help with clarity.

Done.

https://codereview.chromium.org/1418573002/diff/620001/cc/playback/display_li...
File cc/playback/display_list_raster_source.cc (right):

https://codereview.chromium.org/1418573002/diff/620001/cc/playback/display_li...
cc/playback/display_list_raster_source.cc:63: return;
On 2015/12/08 23:27:24, enne wrote:
> I'm a little confused why this check and paint inspection needs to happen at
> raster time.  Doesn't the rtree already know which image ops fit in the right
> place?
> 
> What use case are you trying to optimize for here?

I guess this is more of a safety check, since we could be using our own rtree,
or skia's rtree or no rtree at all, and the consequences of us not doing exactly
what the discardable image map does is that we'll decode things in place.

For example the drop shadow case I had would have not been caught if it wasn't
for this type of situation and instead would've been happily decoded in place
(causing bad performance instead of a correctness issue).

I can remove this if you prefer, but it feels somewhat safer to keep it.

https://codereview.chromium.org/1418573002/diff/620001/cc/playback/display_li...
cc/playback/display_list_raster_source.cc:140: // Does Blink use this?
On 2015/12/08 23:27:24, enne wrote:
> grep suggests no.  If we don't have any testing that this code is right, I'd
> rather just NOTREACHED it?

Done.

https://codereview.chromium.org/1418573002/diff/620001/cc/playback/display_li...
cc/playback/display_list_raster_source.cc:309: // discardable images. This way
we would only pay for the hijack canvas if the
On 2015/12/08 23:27:24, enne wrote:
> Do you have any sense of what the expense is of the hijack canvas?

I don't really know how expensive it is if there are no images. The problem is
that if we aren't allowing out of raster image decodes (ie they are disabled by
a flag), then we must not have a hijack canvas, because it will cause decodes in
place for everything (since we never populated the discardable image map).

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
File cc/tiles/image_decode_controller.cc (right):

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:305: skia::RefPtr<SkImage> decoded_image =
skia::AdoptRef(SkImage::NewFromRaster(
On 2015/12/09 16:03:04, reed1 wrote:
> Do you need decoded_image? If the only use is so you can call scalePixels, you
> could instead call.
> 
> SkPixmap::scalePixmap(dst);
> 
> Just replace #305 with building a decoded_pixmap. This guys is free (no calls
> into new/delete) and is what will get invoked anyway when you call
> decoded_image->scalePixels(dst)

Cool, thanks! Done.

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.cc:569: size_t
ImageDecodeController::SanityCheckState(int line, bool lock_acquired) {
On 2015/12/08 23:27:24, enne wrote:
> Why does this return something?

Removed the return.

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
File cc/tiles/image_decode_controller.h (right):

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.h:224: bool ShouldDecodeAndScaleImage(const
ImageKey& key, const DrawImage& image);
On 2015/12/09 00:09:48, aelias wrote:
> CanHandleImage();

Done.

https://codereview.chromium.org/1418573002/diff/620001/cc/tiles/image_decode_...
cc/tiles/image_decode_controller.h:237: base::hash_set<uint32_t>
prerolled_images_;
On 2015/12/09 00:09:48, aelias wrote:
> It took me some time to realize this is only used on the SkImage fallback
path. 
> I'd suggest bundling it together in the header with all the related stuff and
> renaming things like this:
> 
> // TODO(vmpstr): We have a temporary fallback path using SkImage preroll for
> images ImageDecodeController
> // doesn't know how to natively handle yet.  This can go away when
> http://crbug.com/558077 and http://crbug.com/558072
> // are both fixed.
> bool CanHandleFilterQuality(SkFilterQuality filter_quality);
> bool CanHandleDecodeAndScaleForImage(const ImageKey& key, const DrawImage&
> image);
> bool is_using_gpu_rasterization_;
> base::hash_set<uint32_t> unhandled_fallback_images_;

I moved these around a bit and added a few comments. Let me know if it looks
better.

https://codereview.chromium.org/1418573002/diff/620001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1418573002/diff/620001/content/renderer/rende...
content/renderer/render_thread_impl.cc:761: are_image_decode_tasks_enabled_ =
true;
On 2015/12/08 23:27:24, enne wrote:
> Should this also be num_raster_threads > 1?

I don't think so... Even in cases of a single raster thread, if we have a high
quality image we'll decode, scale, and pin it for all of the rasters that were
scheduled. That's an improvement over what Skia does now (which is not to pin
anything).

[enne (OOO), 2015-12-10 00:07:16]

https://codereview.chromium.org/1418573002/diff/620001/cc/playback/display_li...
File cc/playback/display_list_raster_source.cc (right):

https://codereview.chromium.org/1418573002/diff/620001/cc/playback/display_li...
cc/playback/display_list_raster_source.cc:63: return;
On 2015/12/09 at 23:53:41, vmpstr wrote:
> On 2015/12/08 23:27:24, enne wrote:
> > I'm a little confused why this check and paint inspection needs to happen at
> > raster time.  Doesn't the rtree already know which image ops fit in the
right
> > place?
> > 
> > What use case are you trying to optimize for here?
> 
> I guess this is more of a safety check, since we could be using our own rtree,
or skia's rtree or no rtree at all, and the consequences of us not doing exactly
what the discardable image map does is that we'll decode things in place.
> 
> For example the drop shadow case I had would have not been caught if it wasn't
for this type of situation and instead would've been happily decoded in place
(causing bad performance instead of a correctness issue).
> 
> I can remove this if you prefer, but it feels somewhat safer to keep it.

Safer, but also a needless cost per image draw.

I think if you analyze a picture and the analysis says these are the pictures in
this rect, then you go to draw that rect, you don't need to also double check
later to make sure the pictures are there.  

Or, if you are going to, it should be a dcheck and not an early out, and should
be cheap in release.

https://codereview.chromium.org/1418573002/diff/620001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1418573002/diff/620001/content/renderer/rende...
content/renderer/render_thread_impl.cc:761: are_image_decode_tasks_enabled_ =
true;
On 2015/12/09 at 23:53:41, vmpstr wrote:
> On 2015/12/08 23:27:24, enne wrote:
> > Should this also be num_raster_threads > 1?
> 
> I don't think so... Even in cases of a single raster thread, if we have a high
quality image we'll decode, scale, and pin it for all of the rasters that were
scheduled. That's an improvement over what Skia does now (which is not to pin
anything).

Oh, ok.  Makes sense!

https://codereview.chromium.org/1418573002/diff/660001/cc/playback/discardabl...
File cc/playback/discardable_image_map.cc (right):

https://codereview.chromium.org/1418573002/diff/660001/cc/playback/discardabl...
cc/playback/discardable_image_map.cc:113: // Blink doesn't issue image nine
calls.
Blink doesn't issue => no cc embedder issues

...here and elsewhere.

[vmpstr, 2015-12-10 00:47:03]

PTAL

https://codereview.chromium.org/1418573002/diff/620001/cc/playback/display_li...
File cc/playback/display_list_raster_source.cc (right):

https://codereview.chromium.org/1418573002/diff/620001/cc/playback/display_li...
cc/playback/display_list_raster_source.cc:63: return;
On 2015/12/10 00:07:16, enne wrote:
> On 2015/12/09 at 23:53:41, vmpstr wrote:
> > On 2015/12/08 23:27:24, enne wrote:
> > > I'm a little confused why this check and paint inspection needs to happen
at
> > > raster time.  Doesn't the rtree already know which image ops fit in the
> right
> > > place?
> > > 
> > > What use case are you trying to optimize for here?
> > 
> > I guess this is more of a safety check, since we could be using our own
rtree,
> or skia's rtree or no rtree at all, and the consequences of us not doing
exactly
> what the discardable image map does is that we'll decode things in place.
> > 
> > For example the drop shadow case I had would have not been caught if it
wasn't
> for this type of situation and instead would've been happily decoded in place
> (causing bad performance instead of a correctness issue).
> > 
> > I can remove this if you prefer, but it feels somewhat safer to keep it.
> 
> Safer, but also a needless cost per image draw.
> 
> I think if you analyze a picture and the analysis says these are the pictures
in
> this rect, then you go to draw that rect, you don't need to also double check
> later to make sure the pictures are there.  
> 
> Or, if you are going to, it should be a dcheck and not an early out, and
should
> be cheap in release.

Sounds good. I've removed this, since it was adding a lot of code and it's hard
to make it DCHECK only (canvas overrides for saving the paints). We can re-add
that if we start finding that we're leaking images that weren't predecoded.

https://codereview.chromium.org/1418573002/diff/660001/cc/playback/discardabl...
File cc/playback/discardable_image_map.cc (right):

https://codereview.chromium.org/1418573002/diff/660001/cc/playback/discardabl...
cc/playback/discardable_image_map.cc:113: // Blink doesn't issue image nine
calls.
On 2015/12/10 00:07:16, enne wrote:
> Blink doesn't issue => no cc embedder issues
> 
> ...here and elsewhere.

Done.

[enne (OOO), 2015-12-10 01:38:06]

Are all the layout test passing now too?

[vmpstr, 2015-12-10 23:31:52]

On 2015/12/10 01:38:06, enne wrote:
> Are all the layout test passing now too?

The majority of layout failures are due to the fact that we're falling down a
different branch in Skia now. Before, we would treat the matrix as a scale +
translate. However, now since I do the scale, it's treating it as a translate
which yields different results.

Note however, that I think that might be a bug in Skia, since for one of the
cases I was looking at, the image I scale to was 50x61 and the dst rect was
50x61.122, which isn't a translate only. However, skia went down the translate
only path anyway:

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

I suspect that this TODO probably has something to do with it:

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

Or maybe this line should be roundOut instead of round?

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

I'm deferring to reed@ on this.

[vmpstr, 2016-01-11 22:13:01]

I think this is ready for another review. After several iterations with this
patch and skia rolls, a lot of the layout test failures were resolved (please
take a look at the latest results).

[enne (OOO), 2016-01-12 19:19:27]

lgtm

[vmpstr, 2016-01-13 19:34:45]

Description was changed from

==========
cc: Add image decode control in the compositor.

This patch reworks ImageDecodeController and the surrounding
system to give control over image decodes to the compositor.

Perf note:

This is disabled on android, and should only affect desktop.
That being said, this patch changes the behavior with
respect to image decodes, so some perf changes are expected.

R=enne, reed1
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Add image decode control in the compositor.

This patch reworks ImageDecodeController and the surrounding
system to give control over image decodes to the compositor.

Perf note:

This is disabled on android, and should only affect desktop.
That being said, this patch changes the behavior with
respect to image decodes, so some perf changes are expected.

BUG=516751
R=enne, reed1
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[vmpstr, 2016-01-13 19:45:39]

vmpstr@chromium.org changed reviewers:
+ piman@chromium.org

[vmpstr, 2016-01-13 19:45:40]

+piman for content/

[piman, 2016-01-13 21:12:58]

LGTM

[vmpstr, 2016-01-13 21:52:03]

(i will land this after the branch)

[vmpstr, 2016-01-19 19:59:20]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2016-01-19 19:59:20]

The patchset sent to the CQ was uploaded after l-g-t-m from ericrk@chromium.org,
reed@google.com, enne@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1418573002/#ps900001
(title: "rebase")

[commit-bot: I haz the power, 2016-01-19 20:00:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1418573002/900001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1418573002/900001

[commit-bot: I haz the power, 2016-01-19 21:16:17]

Description was changed from

==========
cc: Add image decode control in the compositor.

This patch reworks ImageDecodeController and the surrounding
system to give control over image decodes to the compositor.

Perf note:

This is disabled on android, and should only affect desktop.
That being said, this patch changes the behavior with
respect to image decodes, so some perf changes are expected.

BUG=516751
R=enne, reed1
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Add image decode control in the compositor.

This patch reworks ImageDecodeController and the surrounding
system to give control over image decodes to the compositor.

Perf note:

This is disabled on android, and should only affect desktop.
That being said, this patch changes the behavior with
respect to image decodes, so some perf changes are expected.

BUG=516751
R=enne, reed1
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-01-19 21:16:18]

Committed patchset #44 (id:900001)

[commit-bot: I haz the power, 2016-01-19 21:17:31]

Description was changed from

==========
cc: Add image decode control in the compositor.

This patch reworks ImageDecodeController and the surrounding
system to give control over image decodes to the compositor.

Perf note:

This is disabled on android, and should only affect desktop.
That being said, this patch changes the behavior with
respect to image decodes, so some perf changes are expected.

BUG=516751
R=enne, reed1
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Add image decode control in the compositor.

This patch reworks ImageDecodeController and the surrounding
system to give control over image decodes to the compositor.

Perf note:

This is disabled on android, and should only affect desktop.
That being said, this patch changes the behavior with
respect to image decodes, so some perf changes are expected.

BUG=516751
R=enne, reed1
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/9dd810a16253140466189823394288ed60dfbb30
Cr-Commit-Position: refs/heads/master@{#370188}
==========

[commit-bot: I haz the power, 2016-01-19 21:17:32]

Patchset 44 (id:??) landed as
https://crrev.com/9dd810a16253140466189823394288ed60dfbb30
Cr-Commit-Position: refs/heads/master@{#370188}