Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(20)

Issue 2755463002: [cc] Fix CompositorFrameSinkSupport BeginFrameAck interface. (Closed)

Created:
3 years, 9 months ago by Eric Seckler
Modified:
3 years, 9 months ago
CC:
brianderson, cc-bugs_chromium.org, chromium-reviews, enne (OOO), Sami
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[cc] Fix CompositorFrameSinkSupport BeginFrameAck interface. This replaces CFSSupport::DidFinishFrame with BeginFrameDidNotSwap and uses SubmitCompositorFrame for BeginFrameAcks with damage instead. This change also requires that all CompositorFrame submitters actually set the BeginFrameAck on their CompositorFrames, so that we avoid crbug.com/696030 (and so that we don't hit the newly added DLOG/DCHECKs). This is work towards unified BeginFrame acknowledgments, see: Tracking bug: https://crbug.com/697086 Design doc: http://bit.ly/beginframeacks BUG=697086, 646774, 696030 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2755463002 Cr-Commit-Position: refs/heads/master@{#459451} Committed: https://chromium.googlesource.com/chromium/src/+/6a9efe9bb5bce08d48578cda5497045c4d33b6ec

Patch Set 1 #

Patch Set 2 : transfer BeginFrameAck on CompositorFrame across processes. #

Patch Set 3 : rebased, now doesn't transmit ack.has_damage. added todos in offscreencanvas. #

Patch Set 4 : Set BeginFrameAck in all relevant CompositorFrame producers. #

Total comments: 5

Patch Set 5 : add BeginFrameDidNotSwap to MojoCFS. #

Total comments: 8

Patch Set 6 : pass on acks to MojoCFSs in clients. #

Total comments: 8

Patch Set 7 : sync + address comments #

Patch Set 8 : sync #

Unified diffs Side-by-side diffs Delta from patch set Stats (+192 lines, -29 lines) Patch
M android_webview/browser/hardware_renderer.cc View 1 2 3 4 5 6 1 chunk +5 lines, -1 line 0 comments Download
M android_webview/browser/surfaces_instance.cc View 1 2 3 4 5 6 2 chunks +6 lines, -0 lines 0 comments Download
M android_webview/browser/test/rendering_test.cc View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M ash/laser/laser_pointer_view.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M ash/laser/laser_pointer_view.cc View 1 2 3 4 5 6 3 chunks +10 lines, -0 lines 0 comments Download
M cc/ipc/mojo_compositor_frame_sink.mojom View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M cc/output/begin_frame_args.h View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M cc/output/begin_frame_args.cc View 1 2 3 4 5 6 1 chunk +7 lines, -0 lines 0 comments Download
M cc/surfaces/compositor_frame_sink_support.h View 1 chunk +1 line, -1 line 0 comments Download
M cc/surfaces/compositor_frame_sink_support.cc View 1 2 3 4 5 6 2 chunks +29 lines, -1 line 0 comments Download
M cc/surfaces/compositor_frame_sink_support_unittest.cc View 1 2 3 4 5 6 7 14 chunks +26 lines, -14 lines 0 comments Download
M cc/surfaces/direct_compositor_frame_sink.cc View 1 2 3 4 5 6 2 chunks +7 lines, -1 line 0 comments Download
M cc/surfaces/direct_compositor_frame_sink_unittest.cc View 3 chunks +10 lines, -2 lines 0 comments Download
M cc/trees/layer_tree_host_impl_unittest.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -2 lines 0 comments Download
M components/display_compositor/gpu_compositor_frame_sink.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M components/display_compositor/gpu_compositor_frame_sink.cc View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M components/display_compositor/gpu_root_compositor_frame_sink.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M components/display_compositor/gpu_root_compositor_frame_sink.cc View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M components/exo/compositor_frame_sink.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M components/exo/compositor_frame_sink.cc View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M components/exo/compositor_frame_sink_holder.cc View 1 2 3 4 5 1 chunk +3 lines, -1 line 0 comments Download
M components/exo/surface.cc View 1 2 3 4 5 6 1 chunk +8 lines, -1 line 0 comments Download
M content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/frame_host/render_widget_host_view_guest_unittest.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/renderer_host/delegated_frame_host.cc View 1 2 3 4 5 6 7 1 chunk +3 lines, -3 lines 0 comments Download
M content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M content/renderer/android/synchronous_compositor_frame_sink.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M services/ui/public/cpp/client_compositor_frame_sink.cc View 1 2 3 4 5 3 chunks +7 lines, -1 line 0 comments Download
M services/ui/ws/display_client_compositor_frame_sink.cc View 1 2 3 4 5 2 chunks +6 lines, -1 line 0 comments Download
M services/ui/ws/window_tree_client_unittest.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp View 1 2 2 chunks +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp View 1 2 3 4 5 6 2 chunks +16 lines, -0 lines 0 comments Download

Messages

Total messages: 100 (70 generated)
Eric Seckler
+Fady for a first pass. Thanks! :)
3 years, 9 months ago (2017-03-17 11:53:57 UTC) #48
Eric Seckler
+cc some relevant folks
3 years, 9 months ago (2017-03-17 11:55:31 UTC) #49
Fady Samuel
https://codereview.chromium.org/2755463002/diff/190001/cc/surfaces/compositor_frame_sink_support.cc File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2755463002/diff/190001/cc/surfaces/compositor_frame_sink_support.cc#newcode105 cc/surfaces/compositor_frame_sink_support.cc:105: // TODO(eseckler): The CompositorFrame submitted below might not be ...
3 years, 9 months ago (2017-03-17 12:07:19 UTC) #53
Fady Samuel
https://codereview.chromium.org/2755463002/diff/190001/cc/surfaces/compositor_frame_sink_support.h File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2755463002/diff/190001/cc/surfaces/compositor_frame_sink_support.h#newcode52 cc/surfaces/compositor_frame_sink_support.h:52: void BeginFrameDidNotSwap(const BeginFrameAck& ack); Could you please also add ...
3 years, 9 months ago (2017-03-17 12:12:28 UTC) #54
Eric Seckler
Thanks! https://codereview.chromium.org/2755463002/diff/190001/cc/surfaces/compositor_frame_sink_support.cc File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2755463002/diff/190001/cc/surfaces/compositor_frame_sink_support.cc#newcode105 cc/surfaces/compositor_frame_sink_support.cc:105: // TODO(eseckler): The CompositorFrame submitted below might not ...
3 years, 9 months ago (2017-03-17 12:44:25 UTC) #57
Fady Samuel
Non-OWNER LGTM + question. https://codereview.chromium.org/2755463002/diff/190001/cc/surfaces/compositor_frame_sink_support.cc File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2755463002/diff/190001/cc/surfaces/compositor_frame_sink_support.cc#newcode105 cc/surfaces/compositor_frame_sink_support.cc:105: // TODO(eseckler): The CompositorFrame submitted ...
3 years, 9 months ago (2017-03-17 13:16:08 UTC) #58
Eric Seckler
Thanks! https://codereview.chromium.org/2755463002/diff/210001/cc/ipc/mojo_compositor_frame_sink.mojom File cc/ipc/mojo_compositor_frame_sink.mojom (right): https://codereview.chromium.org/2755463002/diff/210001/cc/ipc/mojo_compositor_frame_sink.mojom#newcode40 cc/ipc/mojo_compositor_frame_sink.mojom:40: BeginFrameDidNotSwap(cc.mojom.BeginFrameAck ack); On 2017/03/17 13:16:07, Fady Samuel wrote: ...
3 years, 9 months ago (2017-03-17 13:40:22 UTC) #59
Eric Seckler
adding reviewers: piman@ for cc/ components/display_compositor/ content/ reveman@ for components/exo/ sky@ for ash/ services/ui/ junov@ ...
3 years, 9 months ago (2017-03-17 13:42:02 UTC) #61
Eric Seckler
ah, and I forgot: +boliu for android_webview/ :)
3 years, 9 months ago (2017-03-17 13:42:57 UTC) #63
Fady Samuel
Please update the TODOs I highlighted. Thanks! Still LGTM after that. https://codereview.chromium.org/2755463002/diff/210001/cc/ipc/mojo_compositor_frame_sink.mojom File cc/ipc/mojo_compositor_frame_sink.mojom (right): ...
3 years, 9 months ago (2017-03-17 13:45:25 UTC) #64
Eric Seckler
https://codereview.chromium.org/2755463002/diff/210001/cc/ipc/mojo_compositor_frame_sink.mojom File cc/ipc/mojo_compositor_frame_sink.mojom (right): https://codereview.chromium.org/2755463002/diff/210001/cc/ipc/mojo_compositor_frame_sink.mojom#newcode40 cc/ipc/mojo_compositor_frame_sink.mojom:40: BeginFrameDidNotSwap(cc.mojom.BeginFrameAck ack); On 2017/03/17 13:45:25, Fady Samuel wrote: > ...
3 years, 9 months ago (2017-03-17 14:01:11 UTC) #66
sky
I'm not at all familiar with this code. +fsamuel for services/ui changes. Is there someone ...
3 years, 9 months ago (2017-03-17 15:40:31 UTC) #68
Fady Samuel
On 2017/03/17 15:40:31, sky wrote: > I'm not at all familiar with this code. +fsamuel ...
3 years, 9 months ago (2017-03-17 15:42:40 UTC) #69
Fady Samuel
On 2017/03/17 15:40:31, sky wrote: > I'm not at all familiar with this code. +fsamuel ...
3 years, 9 months ago (2017-03-17 15:42:41 UTC) #70
sky
History seems to indicate sammiequon wrote the laser code, but reveman has done some changes ...
3 years, 9 months ago (2017-03-17 15:50:50 UTC) #72
Tom Sepez
mojom LGTM
3 years, 9 months ago (2017-03-17 17:02:33 UTC) #73
piman
LGTM + a few nits. https://codereview.chromium.org/2755463002/diff/230001/android_webview/browser/hardware_renderer.cc File android_webview/browser/hardware_renderer.cc (right): https://codereview.chromium.org/2755463002/diff/230001/android_webview/browser/hardware_renderer.cc#newcode175 android_webview/browser/hardware_renderer.cc:175: cc::BeginFrameArgs::kStartingFrameNumber, 0, true); nit: ...
3 years, 9 months ago (2017-03-17 18:12:01 UTC) #76
boliu
android_webview rs lgtm
3 years, 9 months ago (2017-03-17 18:16:10 UTC) #77
Fady Samuel
https://codereview.chromium.org/2755463002/diff/230001/services/ui/ws/frame_generator.cc File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2755463002/diff/230001/services/ui/ws/frame_generator.cc#newcode96 services/ui/ws/frame_generator.cc:96: // TODO(eseckler): Acknowledge BeginFrame if we don't submit CompositorFrame. ...
3 years, 9 months ago (2017-03-18 13:03:11 UTC) #78
Eric Seckler
https://codereview.chromium.org/2755463002/diff/230001/android_webview/browser/hardware_renderer.cc File android_webview/browser/hardware_renderer.cc (right): https://codereview.chromium.org/2755463002/diff/230001/android_webview/browser/hardware_renderer.cc#newcode175 android_webview/browser/hardware_renderer.cc:175: cc::BeginFrameArgs::kStartingFrameNumber, 0, true); On 2017/03/17 18:12:01, piman wrote: > ...
3 years, 9 months ago (2017-03-20 11:54:03 UTC) #79
reveman
components/exo lgtm
3 years, 9 months ago (2017-03-21 04:58:13 UTC) #80
Eric Seckler
Justin, Sammie, could you take a look at OffscreenCanvas respectively laser_pointer_view changes please? Thank you! ...
3 years, 9 months ago (2017-03-22 14:19:27 UTC) #81
Justin Novosad
lgtm
3 years, 9 months ago (2017-03-22 14:32:54 UTC) #82
sammiequon
lgtm
3 years, 9 months ago (2017-03-22 16:16:18 UTC) #83
Eric Seckler
Thanks! Back to sky@ for rs of ash/ and services/ui :)
3 years, 9 months ago (2017-03-22 16:18:34 UTC) #84
sky
LGTM
3 years, 9 months ago (2017-03-22 17:35:23 UTC) #85
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2755463002/270001
3 years, 9 months ago (2017-03-24 15:45:53 UTC) #92
commit-bot: I haz the power
Prior attempt to commit was detected, but we were not able to check whether the ...
3 years, 9 months ago (2017-03-24 16:15:59 UTC) #95
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2755463002/270001
3 years, 9 months ago (2017-03-24 16:23:49 UTC) #97
commit-bot: I haz the power
3 years, 9 months ago (2017-03-24 16:30:48 UTC) #100
Message was sent while issue was closed.
Committed patchset #8 (id:270001) as
https://chromium.googlesource.com/chromium/src/+/6a9efe9bb5bce08d48578cda5497...

Powered by Google App Engine
This is Rietveld 408576698