cc: Clear checker-image tracking on navigation and visibility changes.

cc: Clear checker-image tracking on navigation and visibility changes.

Currently the tracker tracks decode policy for images across
navigations, which can significantly increase the size of the map
tracking this state. This change ensures that we clear this state on
navigations since the same image id should not be re-used. Also, it
ensures that we drop any cached decodes after navigations and a
memory policy update which evicts all resources.

BUG=693228, 691080
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2869513002
Cr-Commit-Position: refs/heads/master@{#471927}
Committed: https://chromium.googlesource.com/chromium/src/+/f9c42e188b9b4d4333b25ca75a7666c28695fce4

[Khushal, 2017-05-06 01:36:44]

Description was changed from

==========
cc: Clear checker-image tracking on navigation and visibility changes.

Currently the tracker tracks decode policy for images across
navigations, which can significantly increase the size of the map
tracking this state. This change ensures that we clear this state on
navigations since the same image id should not be re-used. Also, it
ensures that we drop any cached decodes after navigations and a
memory policy update which evicts all resources.

BUG=693228
==========

to

==========
cc: Clear checker-image tracking on navigation and visibility changes.

Currently the tracker tracks decode policy for images across
navigations, which can significantly increase the size of the map
tracking this state. This change ensures that we clear this state on
navigations since the same image id should not be re-used. Also, it
ensures that we drop any cached decodes after navigations and a
memory policy update which evicts all resources.

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

[Khushal, 2017-05-06 01:40:51]

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

[Khushal, 2017-05-06 03:53:34]

Description was changed from

==========
cc: Clear checker-image tracking on navigation and visibility changes.

Currently the tracker tracks decode policy for images across
navigations, which can significantly increase the size of the map
tracking this state. This change ensures that we clear this state on
navigations since the same image id should not be re-used. Also, it
ensures that we drop any cached decodes after navigations and a
memory policy update which evicts all resources.

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

to

==========
cc: Clear checker-image tracking on navigation and visibility changes.

Currently the tracker tracks decode policy for images across
navigations, which can significantly increase the size of the map
tracking this state. This change ensures that we clear this state on
navigations since the same image id should not be re-used. Also, it
ensures that we drop any cached decodes after navigations and a
memory policy update which evicts all resources.

BUG=693228, 691080
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Khushal, 2017-05-09 04:42:17]

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

[commit-bot: I haz the power, 2017-05-09 04:42:52]

Dry run: 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-05-09 06:35:19]

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

[commit-bot: I haz the power, 2017-05-09 06:35:20]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2017-05-09 22:06:21]

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

https://codereview.chromium.org/2869513002/diff/20001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.h:71: class ScopedDecodeHolder {
ScopedDecodeUnlocker maybe? Can you also drop a class comment here to explain
what it does

https://codereview.chromium.org/2869513002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2869513002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1287:
tile_manager_.ClearCheckerImageTracking();
Can you verify what happens in a resourceless software draw for a webview? I
think we might set the memory limit to 0 there as well or is that not true?

[Khushal, 2017-05-10 00:53:17]

https://codereview.chromium.org/2869513002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2869513002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1287:
tile_manager_.ClearCheckerImageTracking();
On 2017/05/09 22:06:21, vmpstr wrote:
> Can you verify what happens in a resourceless software draw for a webview? I
> think we might set the memory limit to 0 there as well or is that not true?

Do you think this is okay for the general case? In particular I'm considering
how this will interact with partial raster. If we are able to acquire any
resources on becoming visible, we might trigger re-raster only for some tiles,
on which images will be checkered. And when that decode finishes, it will cause
an invalidation for all tiles with that image on the next pending tree.

[vmpstr, 2017-05-10 18:20:27]

On 2017/05/10 00:53:17, Khushal wrote:
>
https://codereview.chromium.org/2869513002/diff/20001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2869513002/diff/20001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_impl.cc:1287:
> tile_manager_.ClearCheckerImageTracking();
> On 2017/05/09 22:06:21, vmpstr wrote:
> > Can you verify what happens in a resourceless software draw for a webview? I
> > think we might set the memory limit to 0 there as well or is that not true?
> 
> Do you think this is okay for the general case? In particular I'm considering
> how this will interact with partial raster. If we are able to acquire any
> resources on becoming visible, we might trigger re-raster only for some tiles,
> on which images will be checkered. And when that decode finishes, it will
cause
> an invalidation for all tiles with that image on the next pending tree.

The intent of this is to make it so that when a tab becomes visible again, then
we are allowed to checker anyway right? Or are you saying that some of the
resource that we acquire _won't_ be checkered because they weren't before?

That's a bit of a concern. The browser will hold on to some resources and
display them if available when the tab goes from invis to vis, and then if the
next frame we see is blank images then that's bad. Perhaps we should only clear
this on navigation as we do with the image caches?

[Khushal, 2017-05-10 22:47:10]

Yeah, I was running with the assumption that on becoming invisible, all
resources will be evicted. So if any tiles are rasterized after that, they will
be checker-imaged. But the resources can be re-used after becoming visible if
they are being held by the browser. And we could end up checkering these images
after they have been displayed.

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

https://codereview.chromium.org/2869513002/diff/20001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.h:71: class ScopedDecodeHolder {
On 2017/05/09 22:06:21, vmpstr wrote:
> ScopedDecodeUnlocker maybe? Can you also drop a class comment here to explain
> what it does

Done.

[vmpstr, 2017-05-12 16:38:40]

lgtm

[Khushal, 2017-05-13 03:24:43]

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

https://codereview.chromium.org/2869513002/diff/60001/cc/tiles/checker_image_...
cc/tiles/checker_image_tracker.cc:88: for (auto image_id :
images_pending_invalidation_) {
Realized I was missing this case. I've added a test for this as well. Do you
mind taking another look?

[vmpstr, 2017-05-15 15:19:48]

lgtm

[Khushal, 2017-05-15 17:21:01]

The CQ bit was checked by khushalsagar@chromium.org

[commit-bot: I haz the power, 2017-05-15 17:21:37]

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-05-15 18:34:25]

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

[commit-bot: I haz the power, 2017-05-15 18:34:27]

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

[Khushal, 2017-05-15 21:20:15]

The CQ bit was checked by khushalsagar@chromium.org

[commit-bot: I haz the power, 2017-05-15 21:21:31]

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-05-15 22:30:52]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494883215003570,
"parent_rev": "4ad81c082997550261445c5fc7c88b9a1a64b4f8", "commit_rev":
"f9c42e188b9b4d4333b25ca75a7666c28695fce4"}

[commit-bot: I haz the power, 2017-05-15 22:31:03]

Description was changed from

==========
cc: Clear checker-image tracking on navigation and visibility changes.

Currently the tracker tracks decode policy for images across
navigations, which can significantly increase the size of the map
tracking this state. This change ensures that we clear this state on
navigations since the same image id should not be re-used. Also, it
ensures that we drop any cached decodes after navigations and a
memory policy update which evicts all resources.

BUG=693228, 691080
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Clear checker-image tracking on navigation and visibility changes.

Currently the tracker tracks decode policy for images across
navigations, which can significantly increase the size of the map
tracking this state. This change ensures that we clear this state on
navigations since the same image id should not be re-used. Also, it
ensures that we drop any cached decodes after navigations and a
memory policy update which evicts all resources.

BUG=693228, 691080
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2869513002
Cr-Commit-Position: refs/heads/master@{#471927}
Committed:
https://chromium.googlesource.com/chromium/src/+/f9c42e188b9b4d4333b25ca75a76...
==========

[commit-bot: I haz the power, 2017-05-15 22:31:05]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/f9c42e188b9b4d4333b25ca75a76...