CompositorResizeLock doesn't work |
||||
Issue descriptionThis has been broken for a few years. We noticed it Jan 2015, though. It's disabled on platforms other than chromeos and on chromeos it is enabled but doing nothing. The problem is its trying to call virtual methods from the constructor and destructor, which do not reach the subclass.
,
Mar 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ea89dd544163618531f6892ce94b1b06fd006c14 commit ea89dd544163618531f6892ce94b1b06fd006c14 Author: danakj <danakj@chromium.org> Date: Wed Mar 29 23:30:47 2017 Fix CompositorResizeLock to do something. Currently CompositorResizeLock doesn't work unless deferred since the base class tries to use a virtual method from its constructor which doesn't know about the subclass yet. This fixes that and makes the following changes in the process: I noticed that there are no longer any unit tests that depend on ResizeLock being an interface. So I made CompositorResizeLock the concrete type. I noticed that CompositorResizeLock was in located in files named compositor_resize_lock_aura.{h,cc} but is not named for aura anymore, and is in fact used in non-aura cases for mac. So I renamed the files, and moved the aura pieces out to DelegatedFrameHostClientAura. In order for DelegatedFrameHostClientAura to know when to release events however, it needs to know when the resize lock ends. So I introduced CompositorResizeLockClient, which tells when the lock is over. Importantly, the above is not the same as the CompositorLock ending. That can be done by calling CompositorResizeLock::UnlockCompositor, which does not end the resize lock, but does release the CompositorLock inside it. So this doesn't inform the client at that time until the resize lock actually ends. I've tried to make this more clear with function names and comments. In order to unit test everything I've pulled CompositorLock out of compositor.h and removed the friend relationships between Compositor and CompositorLock. To do so CompositorLock now has 2 interfaces: - CompositorLockClient which it informs about timeouts. This is so that CompositorResizeLock knows when its internal CompositorLock is released by timeout. - CompositorLockDelegate which performs actual unlocking. This is the ui::Compositor outside of tests. Lastly, CompositorResizeLock has a timeout, and CompositorLock has a timeout. These happen to be the same number tho CompositorResizeLock allows you to override it. I've combined these into a single timeout (with the help of CompositorLockClient). And will allow GetCompositorLock to request a specific timeout. With that, Compositor no longer needs SetLocksWillTimeOut which has some action-at-distance properties. This maintains the property that the first to grab a lock chooses the timeout for all other locks grabbed during that time, however Mac code which wants no timeouts now doesn't cause other parts of the system to also get no timeout unless the mac lock is currently active without a timeout. R=piman@chromium.org BUG= 704928 NOTRY=true Review-Url: https://codereview.chromium.org/2773433003 Cr-Commit-Position: refs/heads/master@{#460577} [modify] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/content/browser/BUILD.gn [modify] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/content/browser/renderer_host/browser_compositor_view_mac.h [modify] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/content/browser/renderer_host/browser_compositor_view_mac.mm [add] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/content/browser/renderer_host/compositor_resize_lock.cc [add] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/content/browser/renderer_host/compositor_resize_lock.h [delete] https://crrev.com/824a4fafaea59bede0f34603abfd0f550db4992e/content/browser/renderer_host/compositor_resize_lock_aura.cc [delete] https://crrev.com/824a4fafaea59bede0f34603abfd0f550db4992e/content/browser/renderer_host/compositor_resize_lock_aura.h [add] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/content/browser/renderer_host/compositor_resize_lock_unittest.cc [modify] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/content/browser/renderer_host/delegated_frame_host.cc [modify] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/content/browser/renderer_host/delegated_frame_host.h [modify] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/content/browser/renderer_host/delegated_frame_host_client_aura.cc [modify] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/content/browser/renderer_host/delegated_frame_host_client_aura.h [modify] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/content/browser/renderer_host/render_widget_host_view_aura.h [modify] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc [delete] https://crrev.com/824a4fafaea59bede0f34603abfd0f550db4992e/content/browser/renderer_host/resize_lock.cc [delete] https://crrev.com/824a4fafaea59bede0f34603abfd0f550db4992e/content/browser/renderer_host/resize_lock.h [modify] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/content/test/BUILD.gn [modify] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/ui/compositor/BUILD.gn [modify] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/ui/compositor/compositor.cc [modify] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/ui/compositor/compositor.h [add] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/ui/compositor/compositor_lock.cc [add] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/ui/compositor/compositor_lock.h [modify] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/ui/compositor/compositor_unittest.cc [add] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/ui/compositor/test/fake_compositor_lock.cc [add] https://crrev.com/ea89dd544163618531f6892ce94b1b06fd006c14/ui/compositor/test/fake_compositor_lock.h
,
Mar 30 2017
NO_PENDING_RENDERER_FRAME and NO_PENDING_COMMIT confuse the heck out of me. Here's a few observations. 1) NO_PENDING_COMMIT implies that we have received a frame from the renderer and are waiting for it to be presented (through the UI compositor which is no longer true). If the UI is resized after a frame from the renderer arrives and is submitted to the display compositor, the NO_PENDING_COMMIT says that we should allow the UI to resize with the wrong sized renderer in order to ACK and let the renderer make progress. However if we don't allow the UI to resize, the new renderer frame will *still* be shown in the old UI size via the display compositor. So NO_PENDING_COMMIT does not contribute anything useful by deferring the lock. => I would propose MaybeCreateResizeLock does not check NO_PENDING_COMMIT. 2) Secondly, NO_PENDING_COMMIT is used to prevent skipping renderer frames. In this case it implies we received a renderer frame of size A, resized the UI to size B, and received another renderer frame of size A, before the UI resize finished. It claims that we should accept this second renderer frame so that the renderer can make progress. I don't believe it, the renderer will hear about size B and make progress. => I would propose ShouldSkipFrame does not check NO_PENDING_COMMIT. 3) NO_PENDING_COMMIT allows us to move off of NO_PENDING_RENDERER_FRAME when we get a renderer frame. We could simply go to YES_CAN_LOCK while having a resize_lock_ in this case. => I would propose we set YES_CAN_LOCK if we have a deferred lock and receive a renderer frame. 4) NO_PENDING_RENDERER_FRAME implies actually that we resized but the ui::Compositor's lock timed out before the renderer sent a frame. We would also briefly enter this state if we received a frame of the correct size, but would override it to NO_PENDING_COMMIT at the end of receiving the frame. In MaybeCreateResizeLock we will not try to create a resize lock if we already have one. NO_PENDING_RENDERER_FRAME means we have a lock (that the CompositorLock inside it timed out). Once this happens, we stay in NO_PENDING_RENDERER_FRAME until we receive a renderer frame, but we also don't delete the resize_lock_ until we commit in the UI with a correctly-sized renderer frame, implying that also means we received a renderer frame, making the state redundant. => I would propose we stop checking NO_PENDING_RENDERER_FRAME in MaybeCreateResizeLock. 5) NO_PENDING_RENDERER_FRAME is also used to prevent skipping renderer frames in ShouldSkipFrame. The implication being that if we resized the UI due to timeout, it may prevent the renderer from making progress. However if we resize the UI, but receive a renderer frame of the wrong size, returning it will unblock the renderer and allow it to produce a frame of the correct size. => I would propose we stop checking NO_PENDING_RENDERER_FRAME in ShouldSkipFrame. What this all boils down to, I think, is that I am proposing: A) Always skip renderer frames of the wrong size. I don't believe this will ever prevent the renderer from making a frame of the right size, as returning the frame will unblock it, and the renderer will hear what size it should be making. B) Getting rid of deferred resize locks. If we resize, prevent another resize until the renderer give a frame of the right size. I believe this is in practice what happens now, except that we defer blocking the UI compositor in cases that we don't need to (point 1 above). If the compositor lock times out, just leave it. We'll wait for a renderer frame of the old size still, show it with gutters, then release the resize lock.
,
Mar 30 2017
For A) the ShouldSkipFrame early out comes from https://chromiumcodereview.appspot.com/10690168 when resize locks were first added. Noteable changes from master: - RWHVs can fast ACK the GPU process (not flip in the browser or TextureImageTransportSurface); useful for allowing the renderer to c atch up when it gets too far behind. - RWHVA will insist kicking a renderer frame after the lock times out on resize (wasn't necessary before) This is all tied to renderer frames going through the UI compositor.
,
Mar 30 2017
There is one case where I am worried about: 1. Resize RWHVA 2. Grab resize lock 3. Tell renderer new size B 4. Receive frame of size A from renderer -> skipped 5. Lock times out 6. Resize RWHVA 7. Tell renderer new size C 8. Receive frame of size C from renderer -> skipped I think there's no strong value to accepting size A when the UI is hoping for size C, which NO_PENDING_RENDERER_FRAME would. But it would also accept size C which is important cuz that's what we actually want. The problem is CompositorResizeLock takes a size when it's created and waits for only that size.
,
Apr 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/62a56f6fcfea29c8dfd333156b0e704f0f1ff525 commit 62a56f6fcfea29c8dfd333156b0e704f0f1ff525 Author: danakj <danakj@chromium.org> Date: Tue Apr 04 15:53:00 2017 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} [modify] https://crrev.com/62a56f6fcfea29c8dfd333156b0e704f0f1ff525/content/browser/renderer_host/browser_compositor_view_mac.h [modify] https://crrev.com/62a56f6fcfea29c8dfd333156b0e704f0f1ff525/content/browser/renderer_host/browser_compositor_view_mac.mm [modify] https://crrev.com/62a56f6fcfea29c8dfd333156b0e704f0f1ff525/content/browser/renderer_host/compositor_resize_lock.cc [modify] https://crrev.com/62a56f6fcfea29c8dfd333156b0e704f0f1ff525/content/browser/renderer_host/compositor_resize_lock.h [modify] https://crrev.com/62a56f6fcfea29c8dfd333156b0e704f0f1ff525/content/browser/renderer_host/compositor_resize_lock_unittest.cc [modify] https://crrev.com/62a56f6fcfea29c8dfd333156b0e704f0f1ff525/content/browser/renderer_host/delegated_frame_host.cc [modify] https://crrev.com/62a56f6fcfea29c8dfd333156b0e704f0f1ff525/content/browser/renderer_host/delegated_frame_host.h [modify] https://crrev.com/62a56f6fcfea29c8dfd333156b0e704f0f1ff525/content/browser/renderer_host/delegated_frame_host_client_aura.cc [modify] https://crrev.com/62a56f6fcfea29c8dfd333156b0e704f0f1ff525/content/browser/renderer_host/delegated_frame_host_client_aura.h [modify] https://crrev.com/62a56f6fcfea29c8dfd333156b0e704f0f1ff525/content/browser/renderer_host/render_widget_host_view_aura.h [modify] https://crrev.com/62a56f6fcfea29c8dfd333156b0e704f0f1ff525/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
,
Apr 4 2017
Ok things should work consistently now afaik.
,
Apr 7 2017
I still see wrong size rendering at the ChromeOS "Add person"/welcome UI in my screen rotation animation implementation (https://codereview.chromium.org/2790583004/). This is only happen to device kevin. Increasing the kCompositorLockTimeoutMs to 133ms seems improved it. +xiyuan@, I heard this UI page is Polymer framework. Do you know any thing will make it slow? |
||||
►
Sign in to add a comment |
||||
Comment 1 by bugdroid1@chromium.org
, Mar 27 2017