Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2260943002:
Move default begin frame sources out to mus (Closed)
|
Created:
4 years, 4 months ago by enne (OOO) Modified:
4 years, 4 months ago CC:
cc-bugs_chromium.org, chromium-reviews, danakj+watch_chromium.org, darin-cc_chromium.org, jam, jbauman+watch_chromium.org, kalyank, mlamouri+watch-content_chromium.org, piman+watch_chromium.org, rjkroege, sievers+watch_chromium.org, Ian Vollick Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove default begin frame sources out to mus
ui::OutputSurface is the only ui::Compositor output surface type that
doesn't produce its own begin frame source. This allows for further
cleanup to remove the default creation of begin frame sources in
cc proxies.
In the future, the SurfaceManager on the mus side will provide the real
begin frame source that'll be plumbed all the way through FrameGenerator
to ServerWindowSurface to WindowSurface and then into ui::OutputSurface.
R=rjkroege@chromium.org,sadrul@chromium.org
BUG=603600
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/8c223f7679dd639ef2cc25d0aa094406be7af019
Cr-Commit-Position: refs/heads/master@{#413877}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Create BFS on compositor thread #Patch Set 3 : Revert SingleThreadProxy changes #
Dependent Patchsets: Messages
Total messages: 38 (18 generated)
Description was changed from ========== Move default begin frame sources out to mus ui::OutputSurface is the only ui::Compositor output surface type that doesn't produce its own begin frame source. Move the creation of default begin frame sources out from cc::SingleThreadProxy and into ui::OutputSurface. This allows every cc embedder to at least pretend that it's using unified begin frames and allows cleanup of older code paths in the short term. In the future, the SurfaceManager on the mus side will provide the real begin frame source that'll be plumbed all the way through FrameGenerator to ServerWindowSurface to WindowSurface and then into ui::OutputSurface. R=rjkroege@chromium.org,sadrul@chromium.org BUG=603600 ========== to ========== Move default begin frame sources out to mus ui::OutputSurface is the only ui::Compositor output surface type that doesn't produce its own begin frame source. Move the creation of default begin frame sources out from cc::SingleThreadProxy and into ui::OutputSurface. This allows every cc embedder to at least pretend that it's using unified begin frames and allows cleanup of older code paths in the short term. In the future, the SurfaceManager on the mus side will provide the real begin frame source that'll be plumbed all the way through FrameGenerator to ServerWindowSurface to WindowSurface and then into ui::OutputSurface. R=rjkroege@chromium.org,sadrul@chromium.org BUG=603600 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
This reverts: https://codereview.chromium.org/1888093002 https://codereview.chromium.org/2229543003 https://codereview.chromium.org/2260943002/diff/1/services/ui/public/cpp/gpu_... File services/ui/public/cpp/gpu_service.h (right): https://codereview.chromium.org/2260943002/diff/1/services/ui/public/cpp/gpu_... services/ui/public/cpp/gpu_service.h:47: scoped_refptr<base::SingleThreadTaskRunner> main_task_runner() { I have no idea what I'm doing here, but this looked like the most plausible task runner.
lgtm
The CQ bit was checked by enne@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
enne@chromium.org changed reviewers: + erg@chromium.org
sadrul: ui/ OWNERS erg: services/ui/ OWNERS
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
(I don't actually know anything about this gpu code but the change itself looks reasonable and I'll owners stamp it if you have the appropriate review...)
https://codereview.chromium.org/2260943002/diff/1/content/renderer/gpu/render... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/2260943002/diff/1/content/renderer/gpu/render... content/renderer/gpu/render_widget_compositor.cc:516: settings.use_external_begin_frame_source = false; I remembered: you need to use an additional --use-mus-in-renderer to test the renderer. https://codereview.chromium.org/2260943002/diff/1/services/ui/public/cpp/outp... File services/ui/public/cpp/output_surface.cc (right): https://codereview.chromium.org/2260943002/diff/1/services/ui/public/cpp/outp... services/ui/public/cpp/output_surface.cc:35: client->SetBeginFrameSource(begin_frame_source_.get()); Should |begin_frame_source_| be created here? (considering that OutputSurface can be constructed in one thread, and BindToClient() can be called in a different thread)
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
A couple of drive-bys. https://codereview.chromium.org/2260943002/diff/1/services/ui/public/cpp/outp... File services/ui/public/cpp/output_surface.cc (right): https://codereview.chromium.org/2260943002/diff/1/services/ui/public/cpp/outp... services/ui/public/cpp/output_surface.cc:27: gpu_service->main_task_runner().get()))); We shouldn't be giving it the main task runner right? it should be the compositor's task runner? https://codereview.chromium.org/2260943002/diff/1/services/ui/public/cpp/outp... services/ui/public/cpp/output_surface.cc:35: client->SetBeginFrameSource(begin_frame_source_.get()); On 2016/08/22 14:46:40, sadrul wrote: > Should |begin_frame_source_| be created here? (considering that OutputSurface > can be constructed in one thread, and BindToClient() can be called in a > different thread) +1
On 2016/08/22 at 14:46:40, sadrul wrote: > https://codereview.chromium.org/2260943002/diff/1/content/renderer/gpu/render... > File content/renderer/gpu/render_widget_compositor.cc (right): > > https://codereview.chromium.org/2260943002/diff/1/content/renderer/gpu/render... > content/renderer/gpu/render_widget_compositor.cc:516: settings.use_external_begin_frame_source = false; > I remembered: you need to use an additional --use-mus-in-renderer to test the renderer. Ah, ok. The patch that disabled stuff in the renderer didn't mention that. I'll unrevert that and only deal with the browser compositor in this patch. Thanks. :) > https://codereview.chromium.org/2260943002/diff/1/services/ui/public/cpp/outp... > File services/ui/public/cpp/output_surface.cc (right): > > https://codereview.chromium.org/2260943002/diff/1/services/ui/public/cpp/outp... > services/ui/public/cpp/output_surface.cc:35: client->SetBeginFrameSource(begin_frame_source_.get()); > Should |begin_frame_source_| be created here? (considering that OutputSurface can be constructed in one thread, and BindToClient() can be called in a different thread) Seems reasonable! I'll get the task runner in the constructor and then use it to create the BFS during BindToClient. On 2016/08/22 at 15:16:52, fsamuel wrote: > A couple of drive-bys. > > https://codereview.chromium.org/2260943002/diff/1/services/ui/public/cpp/outp... > File services/ui/public/cpp/output_surface.cc (right): > > https://codereview.chromium.org/2260943002/diff/1/services/ui/public/cpp/outp... > services/ui/public/cpp/output_surface.cc:27: gpu_service->main_task_runner().get()))); > We shouldn't be giving it the main task runner right? it should be the compositor's task runner? What do you mean by the compositor task runner in this case? I'm not sure I understand what the thread landscape looks like in mus.
On 2016/08/22 18:55:10, enne wrote: > On 2016/08/22 at 14:46:40, sadrul wrote: > > > https://codereview.chromium.org/2260943002/diff/1/content/renderer/gpu/render... > > File content/renderer/gpu/render_widget_compositor.cc (right): > > > > > https://codereview.chromium.org/2260943002/diff/1/content/renderer/gpu/render... > > content/renderer/gpu/render_widget_compositor.cc:516: > settings.use_external_begin_frame_source = false; > > I remembered: you need to use an additional --use-mus-in-renderer to test the > renderer. > > Ah, ok. The patch that disabled stuff in the renderer didn't mention that. > I'll unrevert that and only deal with the browser compositor in this patch. > Thanks. :) > > > > https://codereview.chromium.org/2260943002/diff/1/services/ui/public/cpp/outp... > > File services/ui/public/cpp/output_surface.cc (right): > > > > > https://codereview.chromium.org/2260943002/diff/1/services/ui/public/cpp/outp... > > services/ui/public/cpp/output_surface.cc:35: > client->SetBeginFrameSource(begin_frame_source_.get()); > > Should |begin_frame_source_| be created here? (considering that OutputSurface > can be constructed in one thread, and BindToClient() can be called in a > different thread) > > Seems reasonable! I'll get the task runner in the constructor and then use it to > create the BFS during BindToClient. We may not be on the same page, so a Q to clarify: should the BFS use the task-runner of the thread BindToClient() is called on? Or should it use the task-runner of a different thread? > > On 2016/08/22 at 15:16:52, fsamuel wrote: > > A couple of drive-bys. > > > > > https://codereview.chromium.org/2260943002/diff/1/services/ui/public/cpp/outp... > > File services/ui/public/cpp/output_surface.cc (right): > > > > > https://codereview.chromium.org/2260943002/diff/1/services/ui/public/cpp/outp... > > services/ui/public/cpp/output_surface.cc:27: > gpu_service->main_task_runner().get()))); > > We shouldn't be giving it the main task runner right? it should be the > compositor's task runner? > > What do you mean by the compositor task runner in this case? I'm not sure I > understand what the thread landscape looks like in mus. When we use mus in the renderer process, the ui::OutputSurface is created in the main-thread, but BindToClient() happens in the compositor thread. So GpuService::main_thread_task_runner() would give you the task-runner of the main-thread. If the BFS should use the main-thread task runner, then yes, the code you have should do the right thing. If not, I believe BindToClient() should use ThreadTaskRunnerHandle::Get() when constructing the BFS. Does that make sense?
On 2016/08/23 at 00:52:27, sadrul wrote: > On 2016/08/22 18:55:10, enne wrote: > > Seems reasonable! I'll get the task runner in the constructor and then use it to > > create the BFS during BindToClient. > > We may not be on the same page, so a Q to clarify: should the BFS use the task-runner of the thread BindToClient() is called on? Or should it use the task-runner of a different thread? Ah, yes, you're quite right. > > On 2016/08/22 at 15:16:52, fsamuel wrote: > > What do you mean by the compositor task runner in this case? I'm not sure I > > understand what the thread landscape looks like in mus. > > When we use mus in the renderer process, the ui::OutputSurface is created in the main-thread, but BindToClient() happens in the compositor thread. So GpuService::main_thread_task_runner() would give you the task-runner of the main-thread. If the BFS should use the main-thread task runner, then yes, the code you have should do the right thing. If not, I believe BindToClient() should use ThreadTaskRunnerHandle::Get() when constructing the BFS. Does that make sense? Yeah, if you're using it in the renderer process, then you want to tick on the compositor thread. This is the same way that CompositorExternalBeginFrameSource works today. It's created on the main thread, filters messages from the browser, and ticks on the compositor thread (which is where the output surface gets bound). I've uploaded another patch that does this.
I like it! Thanks! lgtm
owners stamp lgtm
lgtm++
The CQ bit was checked by enne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rjkroege@chromium.org Link to the patchset: https://codereview.chromium.org/2260943002/#ps20001 (title: "Create BFS on compositor thread")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by enne@chromium.org
Oops, right. I forgot cc unit tests. I'll revert the single thread proxy dchecks and follow up with https://codereview.chromium.org/2083423006.
The CQ bit was checked by enne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rjkroege@chromium.org, sadrul@chromium.org, erg@chromium.org, fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/2260943002/#ps40001 (title: "Revert SingleThreadProxy changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by enne@chromium.org
The CQ bit was checked by enne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Move default begin frame sources out to mus ui::OutputSurface is the only ui::Compositor output surface type that doesn't produce its own begin frame source. Move the creation of default begin frame sources out from cc::SingleThreadProxy and into ui::OutputSurface. This allows every cc embedder to at least pretend that it's using unified begin frames and allows cleanup of older code paths in the short term. In the future, the SurfaceManager on the mus side will provide the real begin frame source that'll be plumbed all the way through FrameGenerator to ServerWindowSurface to WindowSurface and then into ui::OutputSurface. R=rjkroege@chromium.org,sadrul@chromium.org BUG=603600 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Move default begin frame sources out to mus ui::OutputSurface is the only ui::Compositor output surface type that doesn't produce its own begin frame source. This allows for further cleanup to remove the default creation of begin frame sources in cc proxies. In the future, the SurfaceManager on the mus side will provide the real begin frame source that'll be plumbed all the way through FrameGenerator to ServerWindowSurface to WindowSurface and then into ui::OutputSurface. R=rjkroege@chromium.org,sadrul@chromium.org BUG=603600 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by enne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Move default begin frame sources out to mus ui::OutputSurface is the only ui::Compositor output surface type that doesn't produce its own begin frame source. This allows for further cleanup to remove the default creation of begin frame sources in cc proxies. In the future, the SurfaceManager on the mus side will provide the real begin frame source that'll be plumbed all the way through FrameGenerator to ServerWindowSurface to WindowSurface and then into ui::OutputSurface. R=rjkroege@chromium.org,sadrul@chromium.org BUG=603600 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Move default begin frame sources out to mus ui::OutputSurface is the only ui::Compositor output surface type that doesn't produce its own begin frame source. This allows for further cleanup to remove the default creation of begin frame sources in cc proxies. In the future, the SurfaceManager on the mus side will provide the real begin frame source that'll be plumbed all the way through FrameGenerator to ServerWindowSurface to WindowSurface and then into ui::OutputSurface. R=rjkroege@chromium.org,sadrul@chromium.org BUG=603600 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/8c223f7679dd639ef2cc25d0aa094406be7af019 Cr-Commit-Position: refs/heads/master@{#413877} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8c223f7679dd639ef2cc25d0aa094406be7af019 Cr-Commit-Position: refs/heads/master@{#413877} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
