Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2706493002:
Check for BeginFrame time/seqnum continuity in ExternalBFS::OnBF. (Closed)
|
Created:
3 years, 10 months ago by Eric Seckler Modified:
3 years, 10 months ago Reviewers:
brianderson CC:
chromium-reviews, cc-bugs_chromium.org, scheduler-bugs_chromium.org, Alex Z., Fady Samuel Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCheck for BeginFrame time/seqnum continuity in ExternalBFS::OnBF.
BUG=690127
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2706493002
Cr-Commit-Position: refs/heads/master@{#451459}
Committed: https://chromium.googlesource.com/chromium/src/+/d69ef1ad3f5700af10d560a4912751394b9918cc
Patch Set 1 #
Total comments: 5
Patch Set 2 : Use const ref. #Messages
Total messages: 21 (12 generated)
Description was changed from ========== Check for BeginFrame time/seqnum continuity in ExternalBFS::OnBF. BUG=690127 ========== to ========== Check for BeginFrame time/seqnum continuity in ExternalBFS::OnBF. BUG=690127 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by eseckler@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...
eseckler@chromium.org changed reviewers: + brianderson@chromium.org
Brian, PTAL. This seems to fix staraz's problems. WDYT, can we add this check?
Description was changed from ========== Check for BeginFrame time/seqnum continuity in ExternalBFS::OnBF. BUG=690127 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Check for BeginFrame time/seqnum continuity in ExternalBFS::OnBF. BUG=690127 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2706493002/diff/1/cc/scheduler/begin_frame_so... File cc/scheduler/begin_frame_source.cc (right): https://codereview.chromium.org/2706493002/diff/1/cc/scheduler/begin_frame_so... cc/scheduler/begin_frame_source.cc:301: void ExternalBeginFrameSource::OnBeginFrame(const BeginFrameArgs& args) { I didn't catch this in the source_id patch you landed earlier, but I wonder if the ExternalBeginFrameSource should override the source_id / sequence number of the args received here with it's own source_id / sequence number. https://codereview.chromium.org/2706493002/diff/1/cc/scheduler/begin_frame_so... cc/scheduler/begin_frame_source.cc:309: BeginFrameArgs last_args = obs->LastUsedBeginFrameArgs(); const ref
PTAL :) https://codereview.chromium.org/2706493002/diff/1/cc/scheduler/begin_frame_so... File cc/scheduler/begin_frame_source.cc (right): https://codereview.chromium.org/2706493002/diff/1/cc/scheduler/begin_frame_so... cc/scheduler/begin_frame_source.cc:301: void ExternalBeginFrameSource::OnBeginFrame(const BeginFrameArgs& args) { On 2017/02/17 20:29:09, brianderson wrote: > I didn't catch this in the source_id patch you landed earlier, but I wonder if > the ExternalBeginFrameSource should override the source_id / sequence number of > the args received here with it's own source_id / sequence number. I don't think that makes sense. BeginFrame creators, not forwarders, assign BeginFrame identifiers (source_id+seqnum). ExternalBFS is normally used as a "middle piece" that forwards BeginFrames across process boundaries, e.g. browser to renderer. I think that we want to be able to track the same BeginFrame identifier in the renderer (behind the ExternalBFS) so that we can "report back" to the browser (original BFS) when everyone in the renderer has finished a particular BeginFrame. https://codereview.chromium.org/2706493002/diff/1/cc/scheduler/begin_frame_so... cc/scheduler/begin_frame_source.cc:309: BeginFrameArgs last_args = obs->LastUsedBeginFrameArgs(); On 2017/02/17 20:29:09, brianderson wrote: > const ref done, also above.
lgtm to unblock Alex. Please add a unit test as part of this patch (or as a followup if Alex needs to be unblocked sooner.) https://codereview.chromium.org/2706493002/diff/1/cc/scheduler/begin_frame_so... File cc/scheduler/begin_frame_source.cc (right): https://codereview.chromium.org/2706493002/diff/1/cc/scheduler/begin_frame_so... cc/scheduler/begin_frame_source.cc:301: void ExternalBeginFrameSource::OnBeginFrame(const BeginFrameArgs& args) { On 2017/02/17 22:38:39, Eric Seckler wrote: > On 2017/02/17 20:29:09, brianderson wrote: > > I didn't catch this in the source_id patch you landed earlier, but I wonder if > > the ExternalBeginFrameSource should override the source_id / sequence number > of > > the args received here with it's own source_id / sequence number. > > I don't think that makes sense. BeginFrame creators, not forwarders, assign > BeginFrame identifiers (source_id+seqnum). ExternalBFS is normally used as a > "middle piece" that forwards BeginFrames across process boundaries, e.g. browser > to renderer. I think that we want to be able to track the same BeginFrame > identifier in the renderer (behind the ExternalBFS) so that we can "report back" > to the browser (original BFS) when everyone in the renderer has finished a > particular BeginFrame. My thinking was that it's a little weird for this class to have a source_id that isn't used. Also, mapping the browser's source_id to this classes source_id would solve the source_id cross-process aliasing (non-)issue. In any case, there's no benefit to do the mapping now and it would only add complexity, so I'll drop the idea.
The CQ bit was checked by eseckler@chromium.org
Thanks, Brian, I will add a unit test next week. ExternalBFS doesn't have any tests right now, so I won't manage before the flight :)
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 commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by eseckler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487418460387570,
"parent_rev": "fe16437ea76d582b4a931e219cd452756e2c6498", "commit_rev":
"d69ef1ad3f5700af10d560a4912751394b9918cc"}
Message was sent while issue was closed.
Description was changed from ========== Check for BeginFrame time/seqnum continuity in ExternalBFS::OnBF. BUG=690127 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Check for BeginFrame time/seqnum continuity in ExternalBFS::OnBF. BUG=690127 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2706493002 Cr-Commit-Position: refs/heads/master@{#451459} Committed: https://chromium.googlesource.com/chromium/src/+/d69ef1ad3f5700af10d560a49127... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/d69ef1ad3f5700af10d560a49127... |
