Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2551083003:
Add Satisfy() and Require() to MojoCompositorFrameSink (Closed)
|
Created:
4 years ago by Alex Z. Modified:
4 years ago CC:
chromium-reviews, rjkroege, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, cc-bugs_chromium.org, darin (slow to review) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd Satisfy() and Require() to MojoCompositorFrameSink
Sometimes Wayland client requires a surface before it is created by
SubmitCompositorFrame() since the request and creation use different message
pipes. This CL moves the implementation of Require and Satisfy to
CompositorFrameSink so that they share one message pipe with
SubmitCompositorFrame and things would happen in the correct order.
This is a temporary fix. kylechar@'s change of surface reference (CL 2541683004)
would formally address this issue.
BUG=659601
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Committed: https://crrev.com/e6b29c494898402e221db3ff490d219ca0bab9fe
Cr-Commit-Position: refs/heads/master@{#436806}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added TODO: Remove the new methods comment to the mojo interface #
Total comments: 4
Patch Set 3 : Implemented Require() and Satisfy() in CompositorFrameSinkSupport #
Total comments: 6
Patch Set 4 : Addressed comments #
Total comments: 2
Patch Set 5 : Addressed a nit #
Total comments: 2
Patch Set 6 : Revert the DCHECK to if and LOG #Patch Set 7 : Rebase; changed LOG to DLOG #
Messages
Total messages: 34 (10 generated)
Description was changed from ========== [TEMP]Added Satisfy() and Require() to MojoCompositorFrameSink Sometime Wayland client requires a surface before it is created by SubmitCompositorFrame() since the request and creation use different message pipes. This CL moves the implementation of Require and Satisfy to CompositorFrameSink so that they share one message pipe with SubmitCompositorFrame and things would happen in the correct order. This is a temporary fix. kylechar@'s change of surface reference (CL 2541683004) would formally addresses this issue. BUG=659601 ========== to ========== [TEMP]Added Satisfy() and Require() to MojoCompositorFrameSink Sometime Wayland client requires a surface before it is created by SubmitCompositorFrame() since the request and creation use different message pipes. This CL moves the implementation of Require and Satisfy to CompositorFrameSink so that they share one message pipe with SubmitCompositorFrame and things would happen in the correct order. This is a temporary fix. kylechar@'s change of surface reference (CL 2541683004) would formally addresses this issue. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== [TEMP]Added Satisfy() and Require() to MojoCompositorFrameSink Sometime Wayland client requires a surface before it is created by SubmitCompositorFrame() since the request and creation use different message pipes. This CL moves the implementation of Require and Satisfy to CompositorFrameSink so that they share one message pipe with SubmitCompositorFrame and things would happen in the correct order. This is a temporary fix. kylechar@'s change of surface reference (CL 2541683004) would formally addresses this issue. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== [TEMP]Added Satisfy() and Require() to MojoCompositorFrameSink Sometime Wayland client requires a surface before it is created by SubmitCompositorFrame() since the request and creation use different message pipes. This CL moves the implementation of Require and Satisfy to CompositorFrameSink so that they share one message pipe with SubmitCompositorFrame and things would happen in the correct order. This is a temporary fix. kylechar@'s change of surface reference (CL 2541683004) would formally address this issue. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
staraz@chromium.org changed reviewers: + danakj@chromium.org, fsamuel@chromium.org, tsepez@chromium.org, xidachen@chromium.org
tsepez@: Please review the changes in mojo_compositor_frame_sink.mojom. fsamuel@: Please review the changes in gpu_compositor_frame_sink.* xidachen@: Please review the changes in off_screen_canvas* danakj@: Please have a look at the changes related to CompositorFrameSink
Please provide an implementation of this in CompositorFrameSinkSupport. https://codereview.chromium.org/2551083003/diff/1/cc/ipc/mojo_compositor_fram... File cc/ipc/mojo_compositor_frame_sink.mojom (right): https://codereview.chromium.org/2551083003/diff/1/cc/ipc/mojo_compositor_fram... cc/ipc/mojo_compositor_frame_sink.mojom:39: // Add the provided |sequence| as a destruction dependency of the Please add a TODO here to delete these once surface references are ready.
https://codereview.chromium.org/2551083003/diff/1/cc/ipc/mojo_compositor_fram... File cc/ipc/mojo_compositor_frame_sink.mojom (right): https://codereview.chromium.org/2551083003/diff/1/cc/ipc/mojo_compositor_fram... cc/ipc/mojo_compositor_frame_sink.mojom:39: // Add the provided |sequence| as a destruction dependency of the On 2016/12/05 20:03:09, Fady Samuel wrote: > Please add a TODO here to delete these once surface references are ready. Done.
https://codereview.chromium.org/2551083003/diff/20001/services/ui/surfaces/gp... File services/ui/surfaces/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2551083003/diff/20001/services/ui/surfaces/gp... services/ui/surfaces/gpu_compositor_frame_sink.cc:52: // TODO(staraz): Implement this. Please add an implementation to CompositorFrameSinkSupport and use it from here. https://codereview.chromium.org/2551083003/diff/20001/services/ui/surfaces/gp... services/ui/surfaces/gpu_compositor_frame_sink.cc:58: NOTIMPLEMENTED(); Please add an implementation to CompositorFrameSinkSupport and use it from here.
mojom itself LGTM.
https://codereview.chromium.org/2551083003/diff/20001/services/ui/surfaces/gp... File services/ui/surfaces/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2551083003/diff/20001/services/ui/surfaces/gp... services/ui/surfaces/gpu_compositor_frame_sink.cc:52: // TODO(staraz): Implement this. On 2016/12/05 20:13:31, Fady Samuel wrote: > Please add an implementation to CompositorFrameSinkSupport and use it from here. Done. https://codereview.chromium.org/2551083003/diff/20001/services/ui/surfaces/gp... services/ui/surfaces/gpu_compositor_frame_sink.cc:58: NOTIMPLEMENTED(); On 2016/12/05 20:13:31, Fady Samuel wrote: > Please add an implementation to CompositorFrameSinkSupport and use it from here. Done.
lgtm
https://codereview.chromium.org/2551083003/diff/40001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2551083003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:87: LOG(ERROR) << "Attempting to require callback on nonexistent surface"; DLOG if you really want it. Strings are expensive to our binary size. This feels like a "soft DCHECK", either this should happen sometimes or it shouldn't normally. Which is this, or why is it more complicated? https://codereview.chromium.org/2551083003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:94: std::vector<uint32_t> sequences; perviously i'd say construct this with size 1. but how about with an initializer list instead? std::vector<uint32_t> sequences = {sequence.sequence};
https://codereview.chromium.org/2551083003/diff/40001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2551083003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:87: LOG(ERROR) << "Attempting to require callback on nonexistent surface"; On 2016/12/06 00:08:17, danakj wrote: > DLOG if you really want it. Strings are expensive to our binary size. This feels > like a "soft DCHECK", either this should happen sometimes or it shouldn't > normally. Which is this, or why is it more complicated? If this happens then this is a bug. This means that we wantto retain a reference to a surface that no longer exists. This means we'll flicker at the very least or parts of the UI will not show up.
https://codereview.chromium.org/2551083003/diff/40001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2551083003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:87: LOG(ERROR) << "Attempting to require callback on nonexistent surface"; On 2016/12/06 00:12:14, Fady Samuel wrote: > On 2016/12/06 00:08:17, danakj wrote: > > DLOG if you really want it. Strings are expensive to our binary size. This > feels > > like a "soft DCHECK", either this should happen sometimes or it shouldn't > > normally. Which is this, or why is it more complicated? > > If this happens then this is a bug. This means that we wantto retain a reference > to a surface that no longer exists. This means we'll flicker at the very least > or parts of the UI will not show up. Then DCHECK instead?
Description was changed from ========== [TEMP]Added Satisfy() and Require() to MojoCompositorFrameSink Sometime Wayland client requires a surface before it is created by SubmitCompositorFrame() since the request and creation use different message pipes. This CL moves the implementation of Require and Satisfy to CompositorFrameSink so that they share one message pipe with SubmitCompositorFrame and things would happen in the correct order. This is a temporary fix. kylechar@'s change of surface reference (CL 2541683004) would formally address this issue. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add Satisfy() and Require() to MojoCompositorFrameSink Sometime Wayland client requires a surface before it is created by SubmitCompositorFrame() since the request and creation use different message pipes. This CL moves the implementation of Require and Satisfy to CompositorFrameSink so that they share one message pipe with SubmitCompositorFrame and things would happen in the correct order. This is a temporary fix. kylechar@'s change of surface reference (CL 2541683004) would formally address this issue. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Add Satisfy() and Require() to MojoCompositorFrameSink Sometime Wayland client requires a surface before it is created by SubmitCompositorFrame() since the request and creation use different message pipes. This CL moves the implementation of Require and Satisfy to CompositorFrameSink so that they share one message pipe with SubmitCompositorFrame and things would happen in the correct order. This is a temporary fix. kylechar@'s change of surface reference (CL 2541683004) would formally address this issue. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add Satisfy() and Require() to MojoCompositorFrameSink Sometimes Wayland client requires a surface before it is created by SubmitCompositorFrame() since the request and creation use different message pipes. This CL moves the implementation of Require and Satisfy to CompositorFrameSink so that they share one message pipe with SubmitCompositorFrame and things would happen in the correct order. This is a temporary fix. kylechar@'s change of surface reference (CL 2541683004) would formally address this issue. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
changes under renderer_host/ lgtm
https://codereview.chromium.org/2551083003/diff/40001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2551083003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:87: LOG(ERROR) << "Attempting to require callback on nonexistent surface"; On 2016/12/06 00:29:28, danakj wrote: > On 2016/12/06 00:12:14, Fady Samuel wrote: > > On 2016/12/06 00:08:17, danakj wrote: > > > DLOG if you really want it. Strings are expensive to our binary size. This > > feels > > > like a "soft DCHECK", either this should happen sometimes or it shouldn't > > > normally. Which is this, or why is it more complicated? > > > > If this happens then this is a bug. This means that we wantto retain a > reference > > to a surface that no longer exists. This means we'll flicker at the very > least > > or parts of the UI will not show up. > > Then DCHECK instead? Done. https://codereview.chromium.org/2551083003/diff/40001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:94: std::vector<uint32_t> sequences; On 2016/12/06 00:08:17, danakj wrote: > perviously i'd say construct this with size 1. but how about with an initializer > list instead? > > std::vector<uint32_t> sequences = {sequence.sequence}; Done.
lgtm + nit https://codereview.chromium.org/2551083003/diff/60001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2551083003/diff/60001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:86: DCHECK(surface); // Ensure the surface exists before requiring callback nit: How about this comment? If this fails then that implies that a client is trying to reference a surface that no longer exists. This could result in flicker or missing components of UI.
https://codereview.chromium.org/2551083003/diff/60001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2551083003/diff/60001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:86: DCHECK(surface); // Ensure the surface exists before requiring callback On 2016/12/06 16:45:25, Fady Samuel wrote: > nit: How about this comment? > > If this fails then that implies that a client is trying to reference a surface > that no longer exists. This could result in flicker or missing components of UI. Done.
https://codereview.chromium.org/2551083003/diff/80001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2551083003/diff/80001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:87: // If this fails then that implies that a client is trying to reference a Oh, fsamuel's comment here helped me see what's happening. This is coming from an IPC call, so we don't have control here over what was sent. So then it was kindof a mistake to ask for DCHECK here since we can't promise that a misbehaving client won't send something wrong. IIUC? But this code is also used in non-IPC stuff. Ideally we'd reject cases we can't control and DCHECK on cases we can. Maybe that means we have a GetSurfaceForId() that can return nullptr on failure and we drop the message when that happens for GpuCompositorFrameSink, and we DCHECK in Require() that the surface is no null. Then non-IPC code doesn't try to branch and handle this error case, and IPCs don't cause DCHECKs and crashes in the other process. Alternatively this method returns true/false if it found the surface or not, and in the IPC case we just drop it when false, but in other cases we DCHECK the result's true, hoping that we remember to do that. wdyt?
https://codereview.chromium.org/2551083003/diff/80001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2551083003/diff/80001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:87: // If this fails then that implies that a client is trying to reference a On 2016/12/06 21:12:45, danakj wrote: > Oh, fsamuel's comment here helped me see what's happening. This is coming from > an IPC call, so we don't have control here over what was sent. So then it was > kindof a mistake to ask for DCHECK here since we can't promise that a > misbehaving client won't send something wrong. IIUC? > > But this code is also used in non-IPC stuff. Ideally we'd reject cases we can't > control and DCHECK on cases we can. Maybe that means we have a GetSurfaceForId() > that can return nullptr on failure and we drop the message when that happens for > GpuCompositorFrameSink, and we DCHECK in Require() that the surface is no null. > Then non-IPC code doesn't try to branch and handle this error case, and IPCs > don't cause DCHECKs and crashes in the other process. > > Alternatively this method returns true/false if it found the surface or not, and > in the IPC case we just drop it when false, but in other cases we DCHECK the > result's true, hoping that we remember to do that. > > wdyt? I have reverted it to the original if and LOG.
I think you're going to need to rebase.
On 2016/12/06 21:39:52, StarAZ1 wrote: > https://codereview.chromium.org/2551083003/diff/80001/cc/surfaces/compositor_... > File cc/surfaces/compositor_frame_sink_support.cc (right): > > https://codereview.chromium.org/2551083003/diff/80001/cc/surfaces/compositor_... > cc/surfaces/compositor_frame_sink_support.cc:87: // If this fails then that > implies that a client is trying to reference a > On 2016/12/06 21:12:45, danakj wrote: > > Oh, fsamuel's comment here helped me see what's happening. This is coming from > > an IPC call, so we don't have control here over what was sent. So then it was > > kindof a mistake to ask for DCHECK here since we can't promise that a > > misbehaving client won't send something wrong. IIUC? > > > > But this code is also used in non-IPC stuff. Ideally we'd reject cases we > can't > > control and DCHECK on cases we can. Maybe that means we have a > GetSurfaceForId() > > that can return nullptr on failure and we drop the message when that happens > for > > GpuCompositorFrameSink, and we DCHECK in Require() that the surface is no > null. > > Then non-IPC code doesn't try to branch and handle this error case, and IPCs > > don't cause DCHECKs and crashes in the other process. > > > > Alternatively this method returns true/false if it found the surface or not, > and > > in the IPC case we just drop it when false, but in other cases we DCHECK the > > result's true, hoping that we remember to do that. > > > > wdyt? > > I have reverted it to the original if and LOG. That doesn't address non-IPC users of this class. Given this is temporary I guess that can stand.. but I stand by DLOG not LOG if you want to print things for debugging.
On 2016/12/06 21:42:26, Fady Samuel wrote: > I think you're going to need to rebase. Done.
On 2016/12/06 22:10:22, danakj wrote: > On 2016/12/06 21:39:52, StarAZ1 wrote: > > > https://codereview.chromium.org/2551083003/diff/80001/cc/surfaces/compositor_... > > File cc/surfaces/compositor_frame_sink_support.cc (right): > > > > > https://codereview.chromium.org/2551083003/diff/80001/cc/surfaces/compositor_... > > cc/surfaces/compositor_frame_sink_support.cc:87: // If this fails then that > > implies that a client is trying to reference a > > On 2016/12/06 21:12:45, danakj wrote: > > > Oh, fsamuel's comment here helped me see what's happening. This is coming > from > > > an IPC call, so we don't have control here over what was sent. So then it > was > > > kindof a mistake to ask for DCHECK here since we can't promise that a > > > misbehaving client won't send something wrong. IIUC? > > > > > > But this code is also used in non-IPC stuff. Ideally we'd reject cases we > > can't > > > control and DCHECK on cases we can. Maybe that means we have a > > GetSurfaceForId() > > > that can return nullptr on failure and we drop the message when that happens > > for > > > GpuCompositorFrameSink, and we DCHECK in Require() that the surface is no > > null. > > > Then non-IPC code doesn't try to branch and handle this error case, and IPCs > > > don't cause DCHECKs and crashes in the other process. > > > > > > Alternatively this method returns true/false if it found the surface or not, > > and > > > in the IPC case we just drop it when false, but in other cases we DCHECK the > > > result's true, hoping that we remember to do that. > > > > > > wdyt? > > > > I have reverted it to the original if and LOG. > > That doesn't address non-IPC users of this class. Given this is temporary I > guess that can stand.. but I stand by DLOG not LOG if you want to print things > for debugging. I changed it to DLOG.
LGTM
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, xidachen@chromium.org, fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/2551083003/#ps120001 (title: "Rebase; changed LOG to DLOG")
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": 120001, "attempt_start_ts": 1481063991384240,
"parent_rev": "4edbd22c890c4f4b14471de8fe4bda6a3b449afc", "commit_rev":
"135d4c39e453a49e45729303467995b20ba72754"}
Message was sent while issue was closed.
Description was changed from ========== Add Satisfy() and Require() to MojoCompositorFrameSink Sometimes Wayland client requires a surface before it is created by SubmitCompositorFrame() since the request and creation use different message pipes. This CL moves the implementation of Require and Satisfy to CompositorFrameSink so that they share one message pipe with SubmitCompositorFrame and things would happen in the correct order. This is a temporary fix. kylechar@'s change of surface reference (CL 2541683004) would formally address this issue. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add Satisfy() and Require() to MojoCompositorFrameSink Sometimes Wayland client requires a surface before it is created by SubmitCompositorFrame() since the request and creation use different message pipes. This CL moves the implementation of Require and Satisfy to CompositorFrameSink so that they share one message pipe with SubmitCompositorFrame and things would happen in the correct order. This is a temporary fix. kylechar@'s change of surface reference (CL 2541683004) would formally address this issue. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add Satisfy() and Require() to MojoCompositorFrameSink Sometimes Wayland client requires a surface before it is created by SubmitCompositorFrame() since the request and creation use different message pipes. This CL moves the implementation of Require and Satisfy to CompositorFrameSink so that they share one message pipe with SubmitCompositorFrame and things would happen in the correct order. This is a temporary fix. kylechar@'s change of surface reference (CL 2541683004) would formally address this issue. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add Satisfy() and Require() to MojoCompositorFrameSink Sometimes Wayland client requires a surface before it is created by SubmitCompositorFrame() since the request and creation use different message pipes. This CL moves the implementation of Require and Satisfy to CompositorFrameSink so that they share one message pipe with SubmitCompositorFrame and things would happen in the correct order. This is a temporary fix. kylechar@'s change of surface reference (CL 2541683004) would formally address this issue. BUG=659601 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/e6b29c494898402e221db3ff490d219ca0bab9fe Cr-Commit-Position: refs/heads/master@{#436806} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e6b29c494898402e221db3ff490d219ca0bab9fe Cr-Commit-Position: refs/heads/master@{#436806} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
