Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2810703004:
Add observers to FrameSinkManagerHost. (Closed)
DescriptionAdd observers to FrameSinkManagerHost.
Add list of in process observers to FrameSinkManagerHost. These work the
same way as SurfaceManager observers, except there is an asynchronous
Mojo call from MojoFrameSinkManager to FrameSinkManagerHost first.
BUG=657959
Review-Url: https://codereview.chromium.org/2810703004
Cr-Commit-Position: refs/heads/master@{#468789}
Committed: https://chromium.googlesource.com/chromium/src/+/8b18a094308ec78705a5aa9aaadd90456b011c61
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix test and add to OCCFSM #Patch Set 3 : Make work with tests. #Patch Set 4 : Rebase. #Patch Set 5 : Change unittests. #Patch Set 6 : Make more better. #
Total comments: 14
Patch Set 7 : Comments. #Patch Set 8 : Add OWNERS. #
Dependent Patchsets: Messages
Total messages: 35 (21 generated)
Description was changed from ========== Flush out FrameSinkManagerHost. Add list of in process observers to FrameSinkManagerHost. These work the same way as SurfaceManager observers, except there is an asynchronous Mojo call from MojoFrameSinkManager to FrameSinkManagerHost first. Also add FrameSinkManagerHost to NoTransportImageTransportFactory for tests. BUG=657959 ========== to ========== Add observers to FrameSinkManagerHost. Add list of in process observers to FrameSinkManagerHost. These work the same way as SurfaceManager observers, except there is an asynchronous Mojo call from MojoFrameSinkManager to FrameSinkManagerHost first. Also add FrameSinkManagerHost to NoTransportImageTransportFactory for tests. BUG=657959 ==========
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
https://codereview.chromium.org/2810703004/diff/1/content/browser/compositor/... File content/browser/compositor/frame_sink_manager_host.h (right): https://codereview.chromium.org/2810703004/diff/1/content/browser/compositor/... content/browser/compositor/frame_sink_manager_host.h:35: void AddObserver(cc::SurfaceObserver* observer); Maybe I'm being pedantic but SurfaceObserver seems like a part of SurfaceManager which is an implementation detail of the display compositor. I feel like we should have a content-side observer.
https://codereview.chromium.org/2810703004/diff/1/content/browser/compositor/... File content/browser/compositor/frame_sink_manager_host.h (right): https://codereview.chromium.org/2810703004/diff/1/content/browser/compositor/... content/browser/compositor/frame_sink_manager_host.h:35: void AddObserver(cc::SurfaceObserver* observer); On 2017/04/11 16:43:53, Fady Samuel wrote: > Maybe I'm being pedantic but SurfaceObserver seems like a part of SurfaceManager > which is an implementation detail of the display compositor. I feel like we > should have a content-side observer. Ya, I think that's sounds totally reasonable to have a different observer base type and maybe even different semantics. Although, for now it might be easier to reuse SurfaceObserver because we can then use it with both SurfaceManager or FrameSinkManagerHost. This will allow using non-Mojo or Mojo at runtime based on a flag while performance is evaluated (eg. for RendererCompositorFrameSink).
https://codereview.chromium.org/2810703004/diff/1/content/browser/compositor/... File content/browser/compositor/frame_sink_manager_host.h (right): https://codereview.chromium.org/2810703004/diff/1/content/browser/compositor/... content/browser/compositor/frame_sink_manager_host.h:35: void AddObserver(cc::SurfaceObserver* observer); On 2017/04/11 16:52:39, kylechar wrote: > On 2017/04/11 16:43:53, Fady Samuel wrote: > > Maybe I'm being pedantic but SurfaceObserver seems like a part of > SurfaceManager > > which is an implementation detail of the display compositor. I feel like we > > should have a content-side observer. > > Ya, I think that's sounds totally reasonable to have a different observer base > type and maybe even different semantics. > > Although, for now it might be easier to reuse SurfaceObserver because we can > then use it with both SurfaceManager or FrameSinkManagerHost. This will allow > using non-Mojo or Mojo at runtime based on a flag while performance is evaluated > (eg. for RendererCompositorFrameSink). If so, I should add a TODO saying as much.
https://codereview.chromium.org/2810703004/diff/1/content/browser/compositor/... File content/browser/compositor/frame_sink_manager_host.h (right): https://codereview.chromium.org/2810703004/diff/1/content/browser/compositor/... content/browser/compositor/frame_sink_manager_host.h:35: void AddObserver(cc::SurfaceObserver* observer); On 2017/04/11 16:53:30, kylechar wrote: > On 2017/04/11 16:52:39, kylechar wrote: > > On 2017/04/11 16:43:53, Fady Samuel wrote: > > > Maybe I'm being pedantic but SurfaceObserver seems like a part of > > SurfaceManager > > > which is an implementation detail of the display compositor. I feel like we > > > should have a content-side observer. > > > > Ya, I think that's sounds totally reasonable to have a different observer base > > type and maybe even different semantics. > > > > Although, for now it might be easier to reuse SurfaceObserver because we can > > then use it with both SurfaceManager or FrameSinkManagerHost. This will allow > > using non-Mojo or Mojo at runtime based on a flag while performance is > evaluated > > (eg. for RendererCompositorFrameSink). > > If so, I should add a TODO saying as much. It seems like those things are orthogonal. FrameSinkManagerHost can observe SurfaceManager directly and forward OnSurfaceCreated via another observer interface?
On 2017/04/11 17:02:47, Fady Samuel wrote: > https://codereview.chromium.org/2810703004/diff/1/content/browser/compositor/... > File content/browser/compositor/frame_sink_manager_host.h (right): > > https://codereview.chromium.org/2810703004/diff/1/content/browser/compositor/... > content/browser/compositor/frame_sink_manager_host.h:35: void > AddObserver(cc::SurfaceObserver* observer); > On 2017/04/11 16:53:30, kylechar wrote: > > On 2017/04/11 16:52:39, kylechar wrote: > > > On 2017/04/11 16:43:53, Fady Samuel wrote: > > > > Maybe I'm being pedantic but SurfaceObserver seems like a part of > > > SurfaceManager > > > > which is an implementation detail of the display compositor. I feel like > we > > > > should have a content-side observer. > > > > > > Ya, I think that's sounds totally reasonable to have a different observer > base > > > type and maybe even different semantics. > > > > > > Although, for now it might be easier to reuse SurfaceObserver because we can > > > then use it with both SurfaceManager or FrameSinkManagerHost. This will > allow > > > using non-Mojo or Mojo at runtime based on a flag while performance is > > evaluated > > > (eg. for RendererCompositorFrameSink). > > > > If so, I should add a TODO saying as much. > > It seems like those things are orthogonal. > > FrameSinkManagerHost can observe SurfaceManager directly and forward > OnSurfaceCreated via another observer interface? Observers might then need to implement SurfaceObserver and FrameSinkManagerHostObserver though? I guess it's not the end of the world.
The CQ bit was checked by kylechar@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kylechar@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/2810703004/diff/120001/content/browser/compos... File content/browser/compositor/frame_sink_manager_host.h (right): https://codereview.chromium.org/2810703004/diff/120001/content/browser/compos... content/browser/compositor/frame_sink_manager_host.h:38: void AddObserver(FrameSinkObserver* observer); I had envisioned this would take in a FrameSinkId AddObserver(const cc::FrameSinkId& frame_sink_id, FrameSinkObserver* observer); RemoveObserver(const cc::FrameSinkId& frame_sink_id); I'm not sure how multi-embedder case would work. Maybe the end point would forward the SurfaceInfo to other embedders? https://codereview.chromium.org/2810703004/diff/120001/content/browser/compos... File content/browser/compositor/frame_sink_observer.h (right): https://codereview.chromium.org/2810703004/diff/120001/content/browser/compos... content/browser/compositor/frame_sink_observer.h:14: virtual void OnSurfaceCreated(const cc::SurfaceInfo& surface_info) = 0; nit: Comment. https://codereview.chromium.org/2810703004/diff/120001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_provider_impl.h (right): https://codereview.chromium.org/2810703004/diff/120001/content/browser/render... content/browser/renderer_host/offscreen_canvas_provider_impl.h:29: void DestroyOffscreenCanvasSurface(const cc::FrameSinkId& frame_sink_id); Should this be passed by value to avoid use-after-free? https://codereview.chromium.org/2810703004/diff/120001/content/browser/render... content/browser/renderer_host/offscreen_canvas_provider_impl.h:53: surfaces_; I hate how we're overloading the term surface here :( https://codereview.chromium.org/2810703004/diff/120001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right): https://codereview.chromium.org/2810703004/diff/120001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_impl.cc:65: if (surface_info.id().local_surface_id() != local_surface_id_) { When would this ever be the case? What's the point of this condition? https://codereview.chromium.org/2810703004/diff/120001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_impl.cc:82: provider_->DestroyOffscreenCanvasSurface(frame_sink_id_); DestroyOffscreenCanvasSurface should take the framesinkid by value, definitely to avoid use after free. https://codereview.chromium.org/2810703004/diff/120001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_surface_impl.h (right): https://codereview.chromium.org/2810703004/diff/120001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_impl.h:22: : public blink::mojom::OffscreenCanvasSurface, Maybe this thing is actually an "OffscreenCanvasEmbedder" then? That way we avoid the term "Surface"? Note maybe we can rename things in a separate CL.
Description was changed from ========== Add observers to FrameSinkManagerHost. Add list of in process observers to FrameSinkManagerHost. These work the same way as SurfaceManager observers, except there is an asynchronous Mojo call from MojoFrameSinkManager to FrameSinkManagerHost first. Also add FrameSinkManagerHost to NoTransportImageTransportFactory for tests. BUG=657959 ========== to ========== Add observers to FrameSinkManagerHost. Add list of in process observers to FrameSinkManagerHost. These work the same way as SurfaceManager observers, except there is an asynchronous Mojo call from MojoFrameSinkManager to FrameSinkManagerHost first. BUG=657959 ==========
https://codereview.chromium.org/2810703004/diff/120001/content/browser/compos... File content/browser/compositor/frame_sink_manager_host.h (right): https://codereview.chromium.org/2810703004/diff/120001/content/browser/compos... content/browser/compositor/frame_sink_manager_host.h:38: void AddObserver(FrameSinkObserver* observer); On 2017/04/28 04:58:00, Fady Samuel wrote: > I had envisioned this would take in a FrameSinkId > > AddObserver(const cc::FrameSinkId& frame_sink_id, FrameSinkObserver* observer); > RemoveObserver(const cc::FrameSinkId& frame_sink_id); > > I'm not sure how multi-embedder case would work. Maybe the end point would > forward the SurfaceInfo to other embedders? After doing a quick implementation with unordered_map<FrameSinkId, FrameSinkObserver*> I'm not sold on this. Handling the simple 1:1 case, the only thing that is eliminated is an if statement in each observer. However, AddObserver() then needs to handle (1) what if there is already an observer for FrameSinkId and (2) what if the observer is already the observer for something else. RemoveObserver() needs to find the FrameSinkId that should be removed. If we allow a FrameSinkObserver to be an observer for more than one FrameSinkId, then it adds complexity in the observer and something like RemoveObserver(FrameSinkId, FrameSinkObserver*) too. If we want multiple things as an observer for one FrameSinkId one day then it's more complexity still. This would all need tests too. So I think using base::ObserverList<> is best way overall, it works for all cases and it already has tests. https://codereview.chromium.org/2810703004/diff/120001/content/browser/compos... File content/browser/compositor/frame_sink_observer.h (right): https://codereview.chromium.org/2810703004/diff/120001/content/browser/compos... content/browser/compositor/frame_sink_observer.h:14: virtual void OnSurfaceCreated(const cc::SurfaceInfo& surface_info) = 0; On 2017/04/28 04:58:00, Fady Samuel wrote: > nit: Comment. Done. https://codereview.chromium.org/2810703004/diff/120001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_provider_impl.h (right): https://codereview.chromium.org/2810703004/diff/120001/content/browser/render... content/browser/renderer_host/offscreen_canvas_provider_impl.h:29: void DestroyOffscreenCanvasSurface(const cc::FrameSinkId& frame_sink_id); On 2017/04/28 04:58:00, Fady Samuel wrote: > Should this be passed by value to avoid use-after-free? Done. https://codereview.chromium.org/2810703004/diff/120001/content/browser/render... content/browser/renderer_host/offscreen_canvas_provider_impl.h:53: surfaces_; On 2017/04/28 04:58:00, Fady Samuel wrote: > I hate how we're overloading the term surface here :( Agreed. I think it should be OffscreenCanvasSurfaceImpl => OffscreenCanvasImpl. https://codereview.chromium.org/2810703004/diff/120001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right): https://codereview.chromium.org/2810703004/diff/120001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_impl.cc:65: if (surface_info.id().local_surface_id() != local_surface_id_) { On 2017/04/28 04:58:00, Fady Samuel wrote: > When would this ever be the case? What's the point of this condition? Hmmm, in the future when MojoFrameSinkManager is in the GPU and the GPU restarts it all I can think of? Although in that case the message is still useful? I can remove the check, it's unlikely to cause problems if did send OnSurfaceCreated twice. https://codereview.chromium.org/2810703004/diff/120001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_impl.cc:82: provider_->DestroyOffscreenCanvasSurface(frame_sink_id_); On 2017/04/28 04:58:00, Fady Samuel wrote: > DestroyOffscreenCanvasSurface should take the framesinkid by value, definitely > to avoid use after free. Good catch. https://codereview.chromium.org/2810703004/diff/120001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_surface_impl.h (right): https://codereview.chromium.org/2810703004/diff/120001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_impl.h:22: : public blink::mojom::OffscreenCanvasSurface, On 2017/04/28 04:58:00, Fady Samuel wrote: > Maybe this thing is actually an "OffscreenCanvasEmbedder" then? That way we > avoid the term "Surface"? Note maybe we can rename things in a separate CL. OffscreenCanvasEmbedder works too.
I moved the offscreen canvas specifics to https://codereview.chromium.org/2851243002.
OK, lgtm
kylechar@chromium.org changed reviewers: + piman@chromium.org
+piman for OWNERS approval.
lgtm
The CQ bit was checked by kylechar@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks!
The CQ bit was checked by kylechar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/2810703004/#ps160001 (title: "Add OWNERS.")
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": 160001, "attempt_start_ts": 1493762745805450,
"parent_rev": "b9b315ec73b88bebaaa1b45dd85b338392eb8fef", "commit_rev":
"8b18a094308ec78705a5aa9aaadd90456b011c61"}
Message was sent while issue was closed.
Description was changed from ========== Add observers to FrameSinkManagerHost. Add list of in process observers to FrameSinkManagerHost. These work the same way as SurfaceManager observers, except there is an asynchronous Mojo call from MojoFrameSinkManager to FrameSinkManagerHost first. BUG=657959 ========== to ========== Add observers to FrameSinkManagerHost. Add list of in process observers to FrameSinkManagerHost. These work the same way as SurfaceManager observers, except there is an asynchronous Mojo call from MojoFrameSinkManager to FrameSinkManagerHost first. BUG=657959 Review-Url: https://codereview.chromium.org/2810703004 Cr-Commit-Position: refs/heads/master@{#468789} Committed: https://chromium.googlesource.com/chromium/src/+/8b18a094308ec78705a5aa9aaadd... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/chromium/src/+/8b18a094308ec78705a5aa9aaadd... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
