ui: Rework CompositorResizeLock interaction with DelegatedFrameHost

Rework UI CompositorResizeLock interaction with DelegatedFrameHost

This removes the CanLockCompositorState enum, and replaces it with
2 bools, in order to disconnect the resize lock from the renderer.
The old code was built on the assumption that renderer frames will
cause a commit in the UI but this is no longer the case as they go
directly to the display compositor (which is now separate from the
UI compositor). Now the bools track  things for the UI compositor.

The first bool |create_resize_lock_after_commit_| allows us to get
rid of the resize lock if it times out. While this bool is true we
do not remake a resize lock in the meantime, and once the UI makes
a single frame and commits, we can re-lock the UI compositor as we
make a new resize lock. The second bool is the only interaction of
the resize lock with the renderer, allowing a renderer frame to be
submitted once the lock has timed out, since we're allowing one UI 
frame through also, so that a slow renderer (causing timeout) will
not remain at its original size indefinitely. This 2nd bool's name
is |allow_one_renderer_frame_during_resize_lock_|. These changes I
made also mean that we no longer entertain the concept of deferred
resize locks. This was used to allow the UI to make a frame before
locking the compositor. Now we just always hold/release the resize
lock directly, along with |create_resize_lock_after_commit_| state
to prevent relocking the UI before producing a frame after unlock.

R=piman@chromium.org
BUG=704928
Review-Url: https://codereview.chromium.org/2790623003
Cr-Commit-Position: refs/heads/master@{#461734}
Committed: https://chromium.googlesource.com/chromium/src/+/62a56f6fcfea29c8dfd333156b0e704f0f1ff525

[danakj, 2017-03-30 23:14:23]

Description was changed from

==========
ui: Stop skipping frames when a compositor lock is deferred.

This was done to ensure that the renderer makes progress, but renderer
progress is no longer related to the UI compositor. If the frame is
the wrong size, acking will allow the renderer to produce a frame of
the right size.

For the case of NO_PENDING_COMMIT, it means a renderer frame arrived,
then a UI resize happened, a resize lock grabbed preventing the UI from
updating. At this point the renderer won't present a frame until it is
ACKed which will happen when the display compositor shows the first
frame. So ShouldSkipFrame won't occur with NO_PENDING_COMMIT.

For the case of NO_PENDING_RENDERER_FRAME, it means that a resize lock
timed out, the UI was allowed to resize and gutter, and we're now
receiving a frame from the renderer of the wrong size still. By
accepting the frame we would cause the display compositor to show the
(still incorrect) renderer frame with gutters. Instead we can save the
CPU/GPU time and spend that having the renderer give a frame of the
correct size.

R=piman@chromium.org
BUG=
==========

to

==========
ui: Rework CompositorResizeLock interaction with DelegatedFrameHost

TODO(danakj): Write something here.

R=piman@chromium.org
BUG=
==========

[danakj, 2017-03-30 23:14:30]

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

[commit-bot: I haz the power, 2017-03-30 23:15:24]

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-03-30 23:42:29]

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

[commit-bot: I haz the power, 2017-03-30 23:42:29]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[piman, 2017-03-31 00:31:55]

https://codereview.chromium.org/2790623003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/delegated_frame_host.cc (right):

https://codereview.chromium.org/2790623003/diff/40001/content/browser/rendere...
content/browser/renderer_host/delegated_frame_host.cc:716: // which will gutter
anyways.
Ok, here I think I know why we wanted the NO_PENDING_RENDERER_FRAME state (with
create_resize_lock_after_commit_ = true being like the NO_PENDING_COMMIT state).
If we take a lock again at the next commit, if it happens before we got a frame
from the renderer, we won't grab the update (because ShouldSkipFrame will be
true) - so basically during the resize of a slow renderer, we may be stuck with
the first frame that we had, and not change until we stop resizing.

Now, I think that when we're in the ShouldSkipFrame case, we should still take
the renderer frame. I don't think there's a reason not to anymore, with
delegated rendering (it was important in the old days). That would fix it. wdyt?

[danakj, 2017-03-31 19:34:42]

https://codereview.chromium.org/2790623003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/delegated_frame_host.cc (right):

https://codereview.chromium.org/2790623003/diff/40001/content/browser/rendere...
content/browser/renderer_host/delegated_frame_host.cc:716: // which will gutter
anyways.
On 2017/03/31 00:31:55, piman wrote:
> Ok, here I think I know why we wanted the NO_PENDING_RENDERER_FRAME state
(with
> create_resize_lock_after_commit_ = true being like the NO_PENDING_COMMIT
state).
> If we take a lock again at the next commit, if it happens before we got a
frame
> from the renderer, we won't grab the update (because ShouldSkipFrame will be
> true) - so basically during the resize of a slow renderer, we may be stuck
with
> the first frame that we had, and not change until we stop resizing.
> 
> Now, I think that when we're in the ShouldSkipFrame case, we should still take
> the renderer frame. I don't think there's a reason not to anymore, with
> delegated rendering (it was important in the old days). That would fix it.
wdyt?

Ah okay, so maybe allow_one_renderer_frame_ = true. But it's not tied to the UI
compositor importantly, as it used to be.

[danakj, 2017-03-31 19:43:28]

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

[danakj, 2017-03-31 19:43:56]

I also fixed the mac compile (had to delete an empty NOTREACHED method) and
fixed a dcheck. If WasResized() happened after a lock timed out, it would make a
new lock in WasResized() which we don't want, we want it to wait until the
commit. Now WasResized() checks for this state.

[commit-bot: I haz the power, 2017-03-31 19:44:11]

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

[danakj, 2017-03-31 19:47:01]

Description was changed from

==========
ui: Rework CompositorResizeLock interaction with DelegatedFrameHost

TODO(danakj): Write something here.

R=piman@chromium.org
BUG=
==========

to

==========
ui: Rework CompositorResizeLock interaction with DelegatedFrameHost

TODO(danakj): Write something here.

R=piman@chromium.org
BUG=704928
==========

[commit-bot: I haz the power, 2017-03-31 21:19:02]

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

[commit-bot: I haz the power, 2017-03-31 21:19:02]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-04-03 16:15:33]

Description was changed from

==========
ui: Rework CompositorResizeLock interaction with DelegatedFrameHost

TODO(danakj): Write something here.

R=piman@chromium.org
BUG=704928
==========

to

==========
ui: Rework CompositorResizeLock interaction with DelegatedFrameHost

This removes the CanLockCompositorState enum, and replaces it with
2 bools, in order to disconnect the resize lock from the renderer.
The old code was built on the assumption that renderer frames will
cause a commit in the UI but this is no longer the case as they go
directly to the display compositor (which is now separate from the
UI compositor). Now the bools track  things for the UI compositor.

The first bool |create_resize_lock_after_commit_| allows us to get
rid of the resize lock if it times out. While this bool is true we
do not remake a resize lock in the meantime, and once the UI makes
a single frame and commits, we can re-lock the UI compositor as we
make a new resize lock. The second bool is the only interaction of
the resize lock with the renderer, allowing a renderer frame to be
submitted once the lock has timed out, since we're allowing one UI 
frame through also, so that a slow renderer (causing timeout) will
not remain at its original size indefinitely. This 2nd bool's name
is |allow_one_renderer_frame_during_resize_lock_|. These changes I
made also mean that we no longer entertain the concept of deferred
resize locks. This was used to allow the UI to make a frame before
locking the compositor. Now we just always hold/release the resize
lock directly, along with |create_resize_lock_after_commit_| state
to prevent relocking the UI before producing a frame after unlock.

R=piman@chromium.org
BUG=704928
==========

[danakj, 2017-04-03 16:16:23]

Description was changed from

==========
ui: Rework CompositorResizeLock interaction with DelegatedFrameHost

This removes the CanLockCompositorState enum, and replaces it with
2 bools, in order to disconnect the resize lock from the renderer.
The old code was built on the assumption that renderer frames will
cause a commit in the UI but this is no longer the case as they go
directly to the display compositor (which is now separate from the
UI compositor). Now the bools track  things for the UI compositor.

The first bool |create_resize_lock_after_commit_| allows us to get
rid of the resize lock if it times out. While this bool is true we
do not remake a resize lock in the meantime, and once the UI makes
a single frame and commits, we can re-lock the UI compositor as we
make a new resize lock. The second bool is the only interaction of
the resize lock with the renderer, allowing a renderer frame to be
submitted once the lock has timed out, since we're allowing one UI 
frame through also, so that a slow renderer (causing timeout) will
not remain at its original size indefinitely. This 2nd bool's name
is |allow_one_renderer_frame_during_resize_lock_|. These changes I
made also mean that we no longer entertain the concept of deferred
resize locks. This was used to allow the UI to make a frame before
locking the compositor. Now we just always hold/release the resize
lock directly, along with |create_resize_lock_after_commit_| state
to prevent relocking the UI before producing a frame after unlock.

R=piman@chromium.org
BUG=704928
==========

to

==========
Rework UI CompositorResizeLock interaction with DelegatedFrameHost

This removes the CanLockCompositorState enum, and replaces it with
2 bools, in order to disconnect the resize lock from the renderer.
The old code was built on the assumption that renderer frames will
cause a commit in the UI but this is no longer the case as they go
directly to the display compositor (which is now separate from the
UI compositor). Now the bools track  things for the UI compositor.

The first bool |create_resize_lock_after_commit_| allows us to get
rid of the resize lock if it times out. While this bool is true we
do not remake a resize lock in the meantime, and once the UI makes
a single frame and commits, we can re-lock the UI compositor as we
make a new resize lock. The second bool is the only interaction of
the resize lock with the renderer, allowing a renderer frame to be
submitted once the lock has timed out, since we're allowing one UI 
frame through also, so that a slow renderer (causing timeout) will
not remain at its original size indefinitely. This 2nd bool's name
is |allow_one_renderer_frame_during_resize_lock_|. These changes I
made also mean that we no longer entertain the concept of deferred
resize locks. This was used to allow the UI to make a frame before
locking the compositor. Now we just always hold/release the resize
lock directly, along with |create_resize_lock_after_commit_| state
to prevent relocking the UI before producing a frame after unlock.

R=piman@chromium.org
BUG=704928
==========

[piman, 2017-04-04 00:16:25]

LGTM. I really appreciate the work on this.

https://codereview.chromium.org/2790623003/diff/100001/content/browser/render...
File content/browser/renderer_host/delegated_frame_host.cc (right):

https://codereview.chromium.org/2790623003/diff/100001/content/browser/render...
content/browser/renderer_host/delegated_frame_host.cc:242:
allow_one_renderer_frame_during_resize_lock_ = false;
nit: maybe I would set this back to false in SubmitCompositorFrame, in case the
frame arrives before we re-took the lock?

[danakj, 2017-04-04 14:55:32]

The CQ bit was checked by danakj@chromium.org

[danakj, 2017-04-04 14:55:33]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2790623003/#ps120001
(title: "shouldskip: allowskipafterframe")

[commit-bot: I haz the power, 2017-04-04 14:55:50]

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-04 15:53:05]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491317732649540,
"parent_rev": "2c18c898b5e93b53737e884e54e66d2c7f633a33", "commit_rev":
"62a56f6fcfea29c8dfd333156b0e704f0f1ff525"}

[commit-bot: I haz the power, 2017-04-04 15:53:55]

Description was changed from

==========
Rework UI CompositorResizeLock interaction with DelegatedFrameHost

This removes the CanLockCompositorState enum, and replaces it with
2 bools, in order to disconnect the resize lock from the renderer.
The old code was built on the assumption that renderer frames will
cause a commit in the UI but this is no longer the case as they go
directly to the display compositor (which is now separate from the
UI compositor). Now the bools track  things for the UI compositor.

The first bool |create_resize_lock_after_commit_| allows us to get
rid of the resize lock if it times out. While this bool is true we
do not remake a resize lock in the meantime, and once the UI makes
a single frame and commits, we can re-lock the UI compositor as we
make a new resize lock. The second bool is the only interaction of
the resize lock with the renderer, allowing a renderer frame to be
submitted once the lock has timed out, since we're allowing one UI 
frame through also, so that a slow renderer (causing timeout) will
not remain at its original size indefinitely. This 2nd bool's name
is |allow_one_renderer_frame_during_resize_lock_|. These changes I
made also mean that we no longer entertain the concept of deferred
resize locks. This was used to allow the UI to make a frame before
locking the compositor. Now we just always hold/release the resize
lock directly, along with |create_resize_lock_after_commit_| state
to prevent relocking the UI before producing a frame after unlock.

R=piman@chromium.org
BUG=704928
==========

to

==========
Rework UI CompositorResizeLock interaction with DelegatedFrameHost

This removes the CanLockCompositorState enum, and replaces it with
2 bools, in order to disconnect the resize lock from the renderer.
The old code was built on the assumption that renderer frames will
cause a commit in the UI but this is no longer the case as they go
directly to the display compositor (which is now separate from the
UI compositor). Now the bools track  things for the UI compositor.

The first bool |create_resize_lock_after_commit_| allows us to get
rid of the resize lock if it times out. While this bool is true we
do not remake a resize lock in the meantime, and once the UI makes
a single frame and commits, we can re-lock the UI compositor as we
make a new resize lock. The second bool is the only interaction of
the resize lock with the renderer, allowing a renderer frame to be
submitted once the lock has timed out, since we're allowing one UI 
frame through also, so that a slow renderer (causing timeout) will
not remain at its original size indefinitely. This 2nd bool's name
is |allow_one_renderer_frame_during_resize_lock_|. These changes I
made also mean that we no longer entertain the concept of deferred
resize locks. This was used to allow the UI to make a frame before
locking the compositor. Now we just always hold/release the resize
lock directly, along with |create_resize_lock_after_commit_| state
to prevent relocking the UI before producing a frame after unlock.

R=piman@chromium.org
BUG=704928

Review-Url: https://codereview.chromium.org/2790623003
Cr-Commit-Position: refs/heads/master@{#461734}
Committed:
https://chromium.googlesource.com/chromium/src/+/62a56f6fcfea29c8dfd333156b0e...
==========

[commit-bot: I haz the power, 2017-04-04 15:53:56]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/62a56f6fcfea29c8dfd333156b0e...