cc: Add scaling for checkered images.

cc: Add scaling for checkered images.

Currently checkered images are decoded at original size. Since the GPU
image decode cache assumes that any decodes it has seen will also be
used with raster, it uploads and keeps this decode both in discardable
and in GPU memory, even if each use of this image at raster is
downscaled, thereby increasing memory usage.

This change has checker-imaging use the max raster scale and quality
seen for an image before starting the decode, which has the max
possibility of the image being used by the GPU cache.

BUG=728796
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2928433003
Cr-Commit-Position: refs/heads/master@{#477891}
Committed: https://chromium.googlesource.com/chromium/src/+/98125cbd86d41a4f9b7915ac2b67197f20b130b2

[Khushal, 2017-06-06 03:46:43]

Description was changed from

==========
cc: Scale checkered images.

BUG=
==========

to

==========
cc: Scale checkered images.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Khushal, 2017-06-06 19:03:31]

Description was changed from

==========
cc: Scale checkered images.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Add scaling for checkered images.

Currently checkered images are decoded at original size. Since the GPU
image decode cache assumes that any decodes it has seen will also be
used with raster, it uploads and keeps this decode both in discardable
and in GPU memory, even if each use of this image at raster is
downscaled and thereby increasing memory usage.

This change has checker-imaging use the max raster scale and quality
seen for an image before starting the decode, which has the max
possibility of the image being used by the GPU cache. In the software
case, its possible to lose the original decode from discardable since
only the scaled decode is locked.

BUG=728796
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Khushal, 2017-06-06 19:06:19]

khushalsagar@chromium.org changed reviewers:
+ ericrk@chromium.org, vmpstr@chromium.org

[Khushal, 2017-06-06 19:06:19]

PTAL.

[Khushal, 2017-06-06 19:27:45]

Description was changed from

==========
cc: Add scaling for checkered images.

Currently checkered images are decoded at original size. Since the GPU
image decode cache assumes that any decodes it has seen will also be
used with raster, it uploads and keeps this decode both in discardable
and in GPU memory, even if each use of this image at raster is
downscaled and thereby increasing memory usage.

This change has checker-imaging use the max raster scale and quality
seen for an image before starting the decode, which has the max
possibility of the image being used by the GPU cache. In the software
case, its possible to lose the original decode from discardable since
only the scaled decode is locked.

BUG=728796
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Add scaling for checkered images.

Currently checkered images are decoded at original size. Since the GPU
image decode cache assumes that any decodes it has seen will also be
used with raster, it uploads and keeps this decode both in discardable
and in GPU memory, even if each use of this image at raster is
downscaled and thereby increasing memory usage.

This change has checker-imaging use the max raster scale and quality
seen for an image before starting the decode, which has the max
possibility of the image being used by the GPU cache.

BUG=728796
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[ericrk, 2017-06-06 22:39:51]

https://codereview.chromium.org/2928433003/diff/20001/cc/tiles/checker_image_...
File cc/tiles/checker_image_tracker.cc (right):

https://codereview.chromium.org/2928433003/diff/20001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.cc:166: it->second.policy = (too_small ||
too_large)
The policy already defaults to SYNC_PERMANENT, and you rely on this in the other
branch. Maybe cleaner to just say:

if (!too_small && !too_large)
  it->second.policy = DecodePolicy::ASYNC

Or might be cleaner to just pull these out and have a single if statement? WDYT?

https://codereview.chromium.org/2928433003/diff/20001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.cc:175: "static", static_image);
nit: The naming of these trace values seems a bit odd - doesn't too_small /
too_large also mean we can't checker? Why is that tracked separately from
CanNotChecker?

https://codereview.chromium.org/2928433003/diff/20001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.cc:205: decode_state->scale.fWidth = std::min(
Do we really not want to scale up at this point? This has no impact on GPU
(which will never scale over 1, even if requested), but seems like it could be
nice for SW, getting a potentially expensive scale out of the way? Or does the
memory usage outweigh the potential perf gains?

https://codereview.chromium.org/2928433003/diff/20001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.cc:207: decode_state->scale.fHeight = std::min(
Is this ever called with a non-default DecodeState? do you need to consider the
decode_state scale value?

https://codereview.chromium.org/2928433003/diff/20001/cc/tiles/checker_image_...
File cc/tiles/checker_image_tracker.h (right):

https://codereview.chromium.org/2928433003/diff/20001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.h:79: SkSize scale;
SkSize has no default constructor, I think you are getting uninitialized floats
here.... maybe specifically set this to (0,0) or (1,1)?

[Khushal, 2017-06-07 00:43:32]

https://codereview.chromium.org/2928433003/diff/20001/cc/tiles/checker_image_...
File cc/tiles/checker_image_tracker.cc (right):

https://codereview.chromium.org/2928433003/diff/20001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.cc:166: it->second.policy = (too_small ||
too_large)
On 2017/06/06 22:39:51, ericrk wrote:
> The policy already defaults to SYNC_PERMANENT, and you rely on this in the
other
> branch. Maybe cleaner to just say:
> 
> if (!too_small && !too_large)
>   it->second.policy = DecodePolicy::ASYNC
> 
> Or might be cleaner to just pull these out and have a single if statement?
WDYT?

How does it look now? I broke the size part seperately because I wanted to
capture what causes the veto in the trace. Looks cleaner too now.

https://codereview.chromium.org/2928433003/diff/20001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.cc:175: "static", static_image);
On 2017/06/06 22:39:51, ericrk wrote:
> nit: The naming of these trace values seems a bit odd - doesn't too_small /
> too_large also mean we can't checker? Why is that tracked separately from
> CanNotChecker?

You're right. Its because I went to look for TRACE_EVENT3. :P Now I see there is
a better way.

https://codereview.chromium.org/2928433003/diff/20001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.cc:205: decode_state->scale.fWidth = std::min(
On 2017/06/06 22:39:51, ericrk wrote:
> Do we really not want to scale up at this point? This has no impact on GPU
> (which will never scale over 1, even if requested), but seems like it could be
> nice for SW, getting a potentially expensive scale out of the way? Or does the
> memory usage outweigh the potential perf gains?

I don't think the memory impact would be much. Now that I think about it, it
might be better since right now we keep the original decode locked in addition
to the scaled version. There is a possibility that if we see the same image with
different scales and high filter quality then the software cache wouldn't use
it, because it takes the exact scale value (could happen during pinch-zoom). But
I guess we can consider that case rare enough to not avoid avoid the performance
benefit of scaling.

https://codereview.chromium.org/2928433003/diff/20001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.cc:207: decode_state->scale.fHeight = std::min(
On 2017/06/06 22:39:51, ericrk wrote:
> Is this ever called with a non-default DecodeState? do you need to consider
the
> decode_state scale value? 

It will be called with a non-default state if we see 2 calls for the same
PaintImage. That's why its keeping the max of what we have seen so far.

https://codereview.chromium.org/2928433003/diff/20001/cc/tiles/checker_image_...
File cc/tiles/checker_image_tracker.h (right):

https://codereview.chromium.org/2928433003/diff/20001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.h:79: SkSize scale;
On 2017/06/06 22:39:51, ericrk wrote:
> SkSize has no default constructor, I think you are getting uninitialized
floats
> here.... maybe specifically set this to (0,0) or (1,1)?

(0,0) it is. Since I want it to use the smallest scale possible.

[Khushal, 2017-06-07 03:47:31]

Description was changed from

==========
cc: Add scaling for checkered images.

Currently checkered images are decoded at original size. Since the GPU
image decode cache assumes that any decodes it has seen will also be
used with raster, it uploads and keeps this decode both in discardable
and in GPU memory, even if each use of this image at raster is
downscaled and thereby increasing memory usage.

This change has checker-imaging use the max raster scale and quality
seen for an image before starting the decode, which has the max
possibility of the image being used by the GPU cache.

BUG=728796
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Add scaling for checkered images.

Currently checkered images are decoded at original size. Since the GPU
image decode cache assumes that any decodes it has seen will also be
used with raster, it uploads and keeps this decode both in discardable
and in GPU memory, even if each use of this image at raster is
downscaled, thereby increasing memory usage.

This change has checker-imaging use the max raster scale and quality
seen for an image before starting the decode, which has the max
possibility of the image being used by the GPU cache.

BUG=728796
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[ericrk, 2017-06-07 21:01:57]

lgtm

https://codereview.chromium.org/2928433003/diff/40001/cc/tiles/checker_image_...
File cc/tiles/checker_image_tracker.h (right):

https://codereview.chromium.org/2928433003/diff/40001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.h:81: SkSize scale;
nit: can you just do:

SkSize scale = SkSize::MakeEmpty();

inline? or does C++ get angry?

[Khushal, 2017-06-07 23:21:33]

Thanks. I think this will conflict with a change vmpstr@ has in CQ, so I'll wait
for that to land.

https://codereview.chromium.org/2928433003/diff/40001/cc/tiles/checker_image_...
File cc/tiles/checker_image_tracker.h (right):

https://codereview.chromium.org/2928433003/diff/40001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.h:81: SkSize scale;
On 2017/06/07 21:01:57, ericrk wrote:
> nit: can you just do:
> 
> SkSize scale = SkSize::MakeEmpty();
> 
> inline? or does C++ get angry?

I thought it would, but I guess not.

[Khushal, 2017-06-08 01:20:08]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2017-06-08 01:20:09]

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

[commit-bot: I haz the power, 2017-06-08 01:20:42]

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-06-08 02:09:55]

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

[commit-bot: I haz the power, 2017-06-08 02:09:56]

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

[Khushal, 2017-06-08 02:20:42]

The CQ bit was checked by khushalsagar@chromium.org

[commit-bot: I haz the power, 2017-06-08 02:20:58]

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-06-08 03:25:28]

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

[commit-bot: I haz the power, 2017-06-08 03:25:29]

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

[Khushal, 2017-06-08 03:37:38]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2017-06-08 03:37:39]

The patchset sent to the CQ was uploaded after l-g-t-m from ericrk@chromium.org
Link to the patchset: https://codereview.chromium.org/2928433003/#ps100001
(title: "..")

[commit-bot: I haz the power, 2017-06-08 03:37:45]

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-06-08 04:45:24]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496893058561230,
"parent_rev": "acde73a7fa15b3877a342ddd8c5d71e97f070aa8", "commit_rev":
"98125cbd86d41a4f9b7915ac2b67197f20b130b2"}

[commit-bot: I haz the power, 2017-06-08 04:45:33]

Description was changed from

==========
cc: Add scaling for checkered images.

Currently checkered images are decoded at original size. Since the GPU
image decode cache assumes that any decodes it has seen will also be
used with raster, it uploads and keeps this decode both in discardable
and in GPU memory, even if each use of this image at raster is
downscaled, thereby increasing memory usage.

This change has checker-imaging use the max raster scale and quality
seen for an image before starting the decode, which has the max
possibility of the image being used by the GPU cache.

BUG=728796
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Add scaling for checkered images.

Currently checkered images are decoded at original size. Since the GPU
image decode cache assumes that any decodes it has seen will also be
used with raster, it uploads and keeps this decode both in discardable
and in GPU memory, even if each use of this image at raster is
downscaled, thereby increasing memory usage.

This change has checker-imaging use the max raster scale and quality
seen for an image before starting the decode, which has the max
possibility of the image being used by the GPU cache.

BUG=728796
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2928433003
Cr-Commit-Position: refs/heads/master@{#477891}
Committed:
https://chromium.googlesource.com/chromium/src/+/98125cbd86d41a4f9b7915ac2b67...
==========

[commit-bot: I haz the power, 2017-06-08 04:45:36]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/98125cbd86d41a4f9b7915ac2b67...