Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(204)

Issue 2740833005: [cc] Pass on BeginFrameAcks from CompositorEBFS through RWHVAura, DFH. (Closed)

Created:
1 year, 3 months ago by Eric Seckler
Modified:
1 year, 3 months ago
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, creis+watch_chromium.org, yusukes+watch_chromium.org, shuchen+watch_chromium.org, xjz+watch_chromium.org, jam, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, cc-bugs_chromium.org, James Su, danakj+watch_chromium.org, miu+watch_chromium.org, Sami, enne (OOO), Tom Sepez, Alexei Svitkine (slow)
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[cc] Pass on BeginFrameAcks from CompositorEBFS through RWHVAura, DFH. This patch adds forwarding of BeginFrameAcks from the renderer through the SwapCompositorFrame and BeginFrameDidNotSwap IPCs to the browser, where they are received and passed on by RenderWidgetHostViewAura. Other platforms to follow. This is work towards unified BeginFrame acknowledgments, see: Tracking bug: https://crbug.com/697086 Design doc: http://bit.ly/beginframeacks BUG=697086, 646774 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2740833005 Cr-Commit-Position: refs/heads/master@{#457962} Committed: https://chromium.googlesource.com/chromium/src/+/31ef6f5c55de6abf3e1c7cf108411491f38632cc

Patch Set 1 #

Total comments: 11

Patch Set 2 : updated mojoms, rename new IPC. #

Total comments: 15

Patch Set 3 : move latest_confirmed tracking to DFH. #

Patch Set 4 : Add BeginFrameAck to compositor_frame_metadata.mojom. #

Patch Set 5 : add struct_traits tests. #

Total comments: 7

Patch Set 6 : move setting has_damage and dchecks to RWHI. #

Patch Set 7 : moved Read() back to .cc #

Patch Set 8 : fix android compilation. #

Patch Set 9 : fix RWH unittest. #

Patch Set 10 : add todo to QueueMessageSwapPromise::DidNotSwap. #

Total comments: 11

Patch Set 11 : address security comments #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+429 lines, -23 lines) Patch
M cc/ipc/begin_frame_args.mojom View 1 2 1 chunk +8 lines, -0 lines 0 comments Download
M cc/ipc/begin_frame_args.typemap View 1 3 4 5 6 1 chunk +11 lines, -2 lines 0 comments Download
A + cc/ipc/begin_frame_args_for_blink.typemap View 1 2 3 4 5 6 1 chunk +8 lines, -2 lines 0 comments Download
M cc/ipc/begin_frame_args_struct_traits.h View 1 2 3 4 5 6 1 chunk +19 lines, -12 lines 0 comments Download
A cc/ipc/begin_frame_args_struct_traits.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +39 lines, -0 lines 0 comments Download
M cc/ipc/cc_param_traits_macros.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +8 lines, -0 lines 0 comments Download
M cc/ipc/cc_serialization_perftest.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M cc/ipc/compositor_frame_metadata.mojom View 1 2 3 2 chunks +2 lines, -0 lines 0 comments Download
M cc/ipc/compositor_frame_metadata_struct_traits.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +5 lines, -0 lines 0 comments Download
M cc/ipc/compositor_frame_metadata_struct_traits.cc View 1 2 3 2 chunks +3 lines, -1 line 0 comments Download
M cc/ipc/struct_traits_unittest.cc View 1 2 3 4 5 6 7 8 chunks +39 lines, -0 lines 0 comments Download
M cc/ipc/traits_test_service.mojom View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M cc/scheduler/begin_frame_source.h View 1 2 1 chunk +6 lines, -0 lines 0 comments Download
M content/browser/bad_message.h View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/renderer_host/delegated_frame_host.h View 1 2 2 chunks +4 lines, -1 line 0 comments Download
M content/browser/renderer_host/delegated_frame_host.cc View 1 2 1 chunk +25 lines, -1 line 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.h View 1 2 3 4 5 2 chunks +5 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +32 lines, -0 lines 2 comments Download
M content/browser/renderer_host/render_widget_host_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.h View 1 2 3 chunks +4 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.cc View 1 2 3 4 5 1 chunk +11 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura_unittest.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +163 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_base.h View 1 2 chunks +6 lines, -0 lines 0 comments Download
M content/common/view_messages.h View 1 1 chunk +5 lines, -0 lines 0 comments Download
M content/renderer/gpu/compositor_external_begin_frame_source.h View 1 chunk +1 line, -2 lines 0 comments Download
M content/renderer/gpu/compositor_external_begin_frame_source.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +10 lines, -1 line 0 comments Download
M content/renderer/gpu/queue_message_swap_promise.cc View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M content/renderer/gpu/renderer_compositor_frame_sink.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/public/blink_typemaps.gni View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 77 (39 generated)
Eric Seckler
Hi Brian, I'm wondering if there's a sensible way to test that the renderer actually ...
1 year, 3 months ago (2017-03-10 16:36:52 UTC) #4
Fady Samuel
Drive-by: Please update mojoms too! https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macros.h File cc/ipc/cc_param_traits_macros.h (right): https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macros.h#newcode209 cc/ipc/cc_param_traits_macros.h:209: IPC_STRUCT_TRAITS_MEMBER(begin_frame_ack) Please update mojoms ...
1 year, 3 months ago (2017-03-10 16:39:30 UTC) #6
danakj
https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macros.h File cc/ipc/cc_param_traits_macros.h (right): https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macros.h#newcode209 cc/ipc/cc_param_traits_macros.h:209: IPC_STRUCT_TRAITS_MEMBER(begin_frame_ack) On 2017/03/10 16:39:30, Fady Samuel wrote: > Please ...
1 year, 3 months ago (2017-03-10 16:40:46 UTC) #8
Fady Samuel
On 2017/03/10 16:40:46, danakj wrote: > https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macros.h > File cc/ipc/cc_param_traits_macros.h (right): > > https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macros.h#newcode209 > ...
1 year, 3 months ago (2017-03-10 16:44:53 UTC) #9
Eric Seckler
On 2017/03/10 16:40:46, danakj wrote: > https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macros.h > File cc/ipc/cc_param_traits_macros.h (right): > > https://codereview.chromium.org/2740833005/diff/1/cc/ipc/cc_param_traits_macros.h#newcode209 > ...
1 year, 3 months ago (2017-03-10 16:48:37 UTC) #10
Fady Samuel
On 2017/03/10 16:48:37, Eric Seckler wrote: > On 2017/03/10 16:40:46, danakj wrote: > > > ...
1 year, 3 months ago (2017-03-10 16:49:28 UTC) #11
Eric Seckler
On 2017/03/10 16:49:28, Fady Samuel wrote: > On 2017/03/10 16:48:37, Eric Seckler wrote: > > ...
1 year, 3 months ago (2017-03-10 16:53:57 UTC) #12
brianderson
I wouldn't worry about integration tests. This code path will be exercised extensively as part ...
1 year, 3 months ago (2017-03-13 22:55:39 UTC) #13
Eric Seckler
+dcheng for cc/ipc. Brian, Fady: PTAL at comment in rwhva.cc :) Daniel: I was trying ...
1 year, 3 months ago (2017-03-14 11:13:42 UTC) #15
Fady Samuel
https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode975 content/browser/renderer_host/render_widget_host_view_aura.cc:975: begin_frame_source_->DidFinishFrame(this, ack); On 2017/03/14 11:13:41, Eric Seckler wrote: > ...
1 year, 3 months ago (2017-03-14 11:31:38 UTC) #16
Eric Seckler
On 2017/03/14 11:31:38, Fady Samuel wrote: > https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_host/render_widget_host_view_aura.cc > File content/browser/renderer_host/render_widget_host_view_aura.cc (right): > > https://codereview.chromium.org/2740833005/diff/1/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode975 ...
1 year, 3 months ago (2017-03-14 11:41:55 UTC) #17
Eric Seckler
On 2017/03/14 11:41:55, Eric Seckler wrote: > On 2017/03/14 11:31:38, Fady Samuel wrote: > > ...
1 year, 3 months ago (2017-03-14 12:46:58 UTC) #18
Fady Samuel
On 2017/03/14 12:46:58, Eric Seckler wrote: > On 2017/03/14 11:41:55, Eric Seckler wrote: > > ...
1 year, 3 months ago (2017-03-14 12:48:46 UTC) #19
Eric Seckler
On 2017/03/14 12:46:58, Eric Seckler wrote: > It looks like the interface used between CompositorFrameSinkSupport ...
1 year, 3 months ago (2017-03-14 15:53:00 UTC) #21
Eric Seckler
+tsepez@ for cc/ipc Fady mentioned that Tom is likely more familar with cc/mus-related IPCs. Tom, ...
1 year, 3 months ago (2017-03-14 16:02:49 UTC) #24
Tom Sepez
On 2017/03/14 16:02:49, Eric Seckler wrote: > +tsepez@ for cc/ipc > > Fady mentioned that ...
1 year, 3 months ago (2017-03-14 16:38:07 UTC) #25
dcheng
On 2017/03/14 16:02:49, Eric Seckler wrote: > +tsepez@ for cc/ipc > > Fady mentioned that ...
1 year, 3 months ago (2017-03-14 16:41:17 UTC) #26
piman
https://codereview.chromium.org/2740833005/diff/20001/cc/ipc/cc_param_traits_macros.h File cc/ipc/cc_param_traits_macros.h (right): https://codereview.chromium.org/2740833005/diff/20001/cc/ipc/cc_param_traits_macros.h#newcode180 cc/ipc/cc_param_traits_macros.h:180: IPC_STRUCT_TRAITS_MEMBER(latest_confirmed_sequence_number) Are there assumptions about this? E.g. I assume ...
1 year, 3 months ago (2017-03-14 20:06:44 UTC) #27
Eric Seckler
Brian, Antoine, PTAL :) On 2017/03/14 16:41:17, dcheng wrote: > For your compilation issue, see ...
1 year, 3 months ago (2017-03-15 11:56:48 UTC) #31
brianderson
lgtm
1 year, 3 months ago (2017-03-15 18:54:23 UTC) #36
dcheng
On 2017/03/15 11:56:48, Eric Seckler wrote: > Brian, Antoine, PTAL :) > > On 2017/03/14 ...
1 year, 3 months ago (2017-03-15 20:10:47 UTC) #37
piman
Mostly good. Main thing is the question about the validity of the DCHECK assuming untrusted ...
1 year, 3 months ago (2017-03-15 21:17:10 UTC) #38
Eric Seckler
Thanks, all addressed! Daniel, could you have a look at ipc security including third_party/WebKit/public/blink_typemaps.gni +asvitkine@ ...
1 year, 3 months ago (2017-03-16 10:24:15 UTC) #40
Eric Seckler
On 2017/03/16 10:24:15, Eric Seckler wrote: > +asvitkine@ for: > tools/metrics/histograms/histograms.xml Alexei is "very slow", ...
1 year, 3 months ago (2017-03-16 10:26:48 UTC) #42
piman
LGTM + couple of nits. https://codereview.chromium.org/2740833005/diff/100001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2740833005/diff/100001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode943 content/browser/renderer_host/render_widget_host_view_aura.cc:943: DCHECK_LE(cc::BeginFrameArgs::kStartingFrameNumber, ack.sequence_number); On 2017/03/16 ...
1 year, 3 months ago (2017-03-16 18:32:14 UTC) #56
Steven Holte
histograms lgtm
1 year, 3 months ago (2017-03-16 20:35:56 UTC) #57
dcheng
https://codereview.chromium.org/2740833005/diff/220001/cc/ipc/begin_frame_args_struct_traits.cc File cc/ipc/begin_frame_args_struct_traits.cc (right): https://codereview.chromium.org/2740833005/diff/220001/cc/ipc/begin_frame_args_struct_traits.cc#newcode22 cc/ipc/begin_frame_args_struct_traits.cc:22: out->type = static_cast<cc::BeginFrameArgs::BeginFrameArgsType>(data.type()); Mind adding a TODO to use ...
1 year, 3 months ago (2017-03-17 05:54:03 UTC) #58
Eric Seckler
On 2017/03/16 18:32:14, piman wrote: > That page explicitly says "Avoid browser side DCHECKs and ...
1 year, 3 months ago (2017-03-17 11:06:45 UTC) #61
piman
LGTM. I don't feel that strongly about ReceivedBadMessage, just that in the past we had ...
1 year, 3 months ago (2017-03-17 17:38:09 UTC) #64
dcheng
LGTM with one thought https://codereview.chromium.org/2740833005/diff/240001/content/browser/renderer_host/render_widget_host_impl.cc File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2740833005/diff/240001/content/browser/renderer_host/render_widget_host_impl.cc#newcode1839 content/browser/renderer_host/render_widget_host_impl.cc:1839: // |has_damage| and |remaining_frames| are ...
1 year, 3 months ago (2017-03-17 18:39:19 UTC) #65
piman
On Fri, Mar 17, 2017 at 11:39 AM, <dcheng@chromium.org> wrote: > LGTM with one thought ...
1 year, 3 months ago (2017-03-17 19:20:55 UTC) #66
dcheng
On 2017/03/17 19:20:55, piman wrote: > On Fri, Mar 17, 2017 at 11:39 AM, <mailto:dcheng@chromium.org> ...
1 year, 3 months ago (2017-03-17 19:24:40 UTC) #67
Eric Seckler
On 2017/03/17 19:24:40, dcheng wrote: > On 2017/03/17 19:20:55, piman wrote: > > On Fri, ...
1 year, 3 months ago (2017-03-18 06:04:06 UTC) #68
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/2740833005/240001
1 year, 3 months ago (2017-03-18 06:04:29 UTC) #71
commit-bot: I haz the power
Committed patchset #11 (id:240001) as https://chromium.googlesource.com/chromium/src/+/31ef6f5c55de6abf3e1c7cf108411491f38632cc
1 year, 3 months ago (2017-03-18 06:11:52 UTC) #74
Fady Samuel
1 year, 3 months ago (2017-03-18 11:04:31 UTC) #75
dcheng
On 2017/03/18 06:04:06, Eric Seckler wrote: > On 2017/03/17 19:24:40, dcheng wrote: > > On ...
1 year, 3 months ago (2017-03-18 23:14:02 UTC) #76
Eric Seckler
1 year, 3 months ago (2017-03-19 06:49:32 UTC) #77
Message was sent while issue was closed.
On 2017/03/18 23:14:02, dcheng wrote:
> Hmm, I may be missing something, but I thought piman's suggestion was to just
> not send the other fields: in other words, don't send across an entire
> BeginFrameAck, but just send across the sequence number?

We've explicitly left those fields out of the mojom and param_traits_macros so
that they are not transmitted - see comments on patch set 2.

Powered by Google App Engine
This is Rietveld 408576698