Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1971023006:
Add begin frame paused to android (Closed)
|
Created:
4 years, 7 months ago by boliu Modified:
4 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nona+watch_chromium.org, piman+watch_chromium.org, shuchen+watch_chromium.org, James Su, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd begin frame paused to android
The pause signal is added to fix crbug.com/539373 which was
a deadlock issue in webview, but could theoretically happen
in chrome as well. It's basically the visibility signal but
sent directly to the compositor thread without going
through the blink main thread.
Add this to Android's BeginFrameSource as well, so that
webview can share the same BeginFrameSource implementation.
BUG=609977
Committed: https://crrev.com/875afa8e5cc793f01c981472d257b33be860ecc6
Cr-Commit-Position: refs/heads/master@{#397304}
Patch Set 1 #
Total comments: 2
Patch Set 2 : ipc filter + nit #
Total comments: 2
Patch Set 3 : rebase #
Total comments: 4
Patch Set 4 : sievers review #
Messages
Total messages: 37 (13 generated)
Description was changed from ========== Add begin frame paused to android BUG=609977 ========== to ========== Add begin frame paused to android The pause signal is added to fix crbug.com/539373 which was a deadlock issue in webview, but could theoretically happen in chrome as well. It's basically the visibility signal but sent directly to the compositor thread without going through the blink main thread. Add this to Android's BeginFrameSource as well, so that webview can share the same BeginFrameSource implementation. BUG=609977 ==========
boliu@chromium.org changed reviewers: + sunnyps@chromium.org
thoughts? fwiw the big CL to merge BFS is here: https://codereview.chromium.org/1969263004/ This is one of the dependencies
LGTM with nits I'm not an OWNER for this code though. https://codereview.chromium.org/1971023006/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1971023006/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_android.cc:1436: !observing_root_window_)); nit: Change !observing_root_window_ to false. https://codereview.chromium.org/1971023006/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_android.cc:1461: !observing_root_window_)); nit: Change !observing_root_window_ to true.
nits addressed On 2016/05/14 00:56:02, sunnyps wrote: > LGTM with nits > > I'm not an OWNER for this code though. But you are the only one who knows this "begin frame paused" thing :) > > https://codereview.chromium.org/1971023006/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > https://codereview.chromium.org/1971023006/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/render_widget_host_view_android.cc:1436: > !observing_root_window_)); > nit: Change !observing_root_window_ to false. > > https://codereview.chromium.org/1971023006/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/render_widget_host_view_android.cc:1461: > !observing_root_window_)); > nit: Change !observing_root_window_ to true.
boliu@chromium.org changed reviewers: + sievers@chromium.org
+sievers for owners review, adding one more post holiday review for ya cc enne: with unified begin frames for desktop as well, this signal probably needs to be added on desktop as well
enne@chromium.org changed reviewers: + enne@chromium.org
Yeah, if the browser is no longer sending begin frame messages, it definitely needs to signal that it's paused to avoid deadlock. I'll see if we need this on desktop. Maybe we need to do it on something like RenderWidgetHostViewAura::Hide. https://codereview.chromium.org/1971023006/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1971023006/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1458: host_->Send(new ViewMsg_SetBeginFramePaused(host_->GetRoutingID(), true)); Does StopObservingRootWindow (and this set begin frame paused) happen during destruction? Just curious about shutdown-related deadlock if the browser goes away first.
https://codereview.chromium.org/1971023006/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1971023006/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1458: host_->Send(new ViewMsg_SetBeginFramePaused(host_->GetRoutingID(), true)); On 2016/05/16 17:22:30, enne wrote: > Does StopObservingRootWindow (and this set begin frame paused) happen during > destruction? Just curious about shutdown-related deadlock if the browser goes > away first. Good question. Looks yes: ~RenderWidgetHostViewAndroid -> SetContentViewCore(NULL) -> calls StopObservingRootWindow and skips StartObservingRootWindow
Thanks for the explanation. Here's a second non-owner lgtm. ;)
if we do this what about SetNeedsBeginFrames() happening when we are not attached to the window?
On 2016/05/17 21:47:25, sievers wrote: > if we do this what about SetNeedsBeginFrames() happening when we are not > attached to the window? Same as before, renderer will not receive begin frames until rwhva attached to window
On 2016/05/17 21:51:10, boliu wrote: > On 2016/05/17 21:47:25, sievers wrote: > > if we do this what about SetNeedsBeginFrames() happening when we are not > > attached to the window? > > Same as before, renderer will not receive begin frames until rwhva attached to > window Actually... I do see a problem wrt latency in this case. If BeginFramePaused is true, then scheduler will SetNeedsBeginFrame to false. So in StartObservingRootWindow, it's very likely that RequestVSyncUpdate won't do anything, and the next BeginFrame will need to wait until renderer SetNeedsBeginFrame to true again. Solution is to send a proactive begin frame just like the input case. But in practice, do you think anyone will notice latecy issue here with attach/detach from window or switching between tabs? I'd default to no until proven otherwise..
sievers: ping! I still want to land this
lgtm w/comments https://codereview.chromium.org/1971023006/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1971023006/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1419: return; remove this https://codereview.chromium.org/1971023006/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1881: if (!host_ || host_->is_hidden()) remove "| host_->is_hidden()'
https://codereview.chromium.org/1971023006/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/1971023006/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1419: return; On 2016/06/01 20:12:53, sievers wrote: > remove this Done. https://codereview.chromium.org/1971023006/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1881: if (!host_ || host_->is_hidden()) On 2016/06/01 20:12:53, sievers wrote: > remove "| host_->is_hidden()' Done.
boliu@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng for IPC this is the same as the begin_frame_source_paused bool in synchronous compositor messages
On 2016/06/01 at 21:10:22, boliu wrote: > +dcheng for IPC > > this is the same as the begin_frame_source_paused bool in synchronous compositor messages Does that mean we eventually won't need begin_frame_source_paused?
On 2016/06/01 21:14:17, dcheng wrote: > On 2016/06/01 at 21:10:22, boliu wrote: > > +dcheng for IPC > > > > this is the same as the begin_frame_source_paused bool in synchronous > compositor messages > > Does that mean we eventually won't need begin_frame_source_paused? That is precisely the plan! https://codereview.chromium.org/1969263004/
LGTM!
The CQ bit was checked by boliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sunnyps@chromium.org, enne@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1971023006/#ps60001 (title: "sievers review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1971023006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1971023006/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1971023006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1971023006/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1971023006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1971023006/60001
Message was sent while issue was closed.
Description was changed from ========== Add begin frame paused to android The pause signal is added to fix crbug.com/539373 which was a deadlock issue in webview, but could theoretically happen in chrome as well. It's basically the visibility signal but sent directly to the compositor thread without going through the blink main thread. Add this to Android's BeginFrameSource as well, so that webview can share the same BeginFrameSource implementation. BUG=609977 ========== to ========== Add begin frame paused to android The pause signal is added to fix crbug.com/539373 which was a deadlock issue in webview, but could theoretically happen in chrome as well. It's basically the visibility signal but sent directly to the compositor thread without going through the blink main thread. Add this to Android's BeginFrameSource as well, so that webview can share the same BeginFrameSource implementation. BUG=609977 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add begin frame paused to android The pause signal is added to fix crbug.com/539373 which was a deadlock issue in webview, but could theoretically happen in chrome as well. It's basically the visibility signal but sent directly to the compositor thread without going through the blink main thread. Add this to Android's BeginFrameSource as well, so that webview can share the same BeginFrameSource implementation. BUG=609977 ========== to ========== Add begin frame paused to android The pause signal is added to fix crbug.com/539373 which was a deadlock issue in webview, but could theoretically happen in chrome as well. It's basically the visibility signal but sent directly to the compositor thread without going through the blink main thread. Add this to Android's BeginFrameSource as well, so that webview can share the same BeginFrameSource implementation. BUG=609977 Committed: https://crrev.com/875afa8e5cc793f01c981472d257b33be860ecc6 Cr-Commit-Position: refs/heads/master@{#397304} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/875afa8e5cc793f01c981472d257b33be860ecc6 Cr-Commit-Position: refs/heads/master@{#397304} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
