[chromium] Compositor frame metadata API
Created attachment 168518 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 168518 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168518&action=review > Source/Platform/chromium/public/WebCompositorFrameMetadata.h:35 > +struct WebCompositorFrameMetadata { Should this struct have a constructor that initializes the scalars?
I'm not sure that would provide a lot of value. There will be exactly one piece of code producing these in the compositor. We may as well omit the constructor for readability.
This seems a bit weird. Why isn't this sent along with the frame? It looks like WebKit will not be producing or consuming this data, so why is it in a WebKit API?
Well, this information needs to arrive at RenderWidgetHost. So my thinking is it would cover all cases: - In non-ubercomp mode, the structure will be received by the renderer WebCompositorOutputSurface implementation and sent up via special IPC. - In ubercomp mode, the structure will be included in the WebCompositorFrame subclass. Then the browser CC implementation will call out to the browser WebCompositorOutputSurface implementation which will call the method on RenderWidgetHost. I'm open to an alternative approach though.
I think you should find a way to make a single call that contains metadata and, in ubercomp mode, the frame data. Its seriously brain damaged to have two separate calls here... on many levels. One, its unintuitive, and two, its going to require two ipcs. Recall all the pain we've had in the past with ipcs causing descheds.
We've been shipping with these extra IPCs on M18 for a long time with some simple rate limiting in render_view_impl, it hasn't turned up as a cause of performance problems in a while. We won't be shipping M24 with ubercomp so we need that as a stopgap. Anyway, for the more important long-run use case, there's also the question of how best to send the metadata from CC to RenderWidgetHost. For that, this would work, or we could do something like this alternate API: class WebCompositorFrameMetadataObserver { public: void didChangeFrameMetadata(const WebCompositorFrameMetadata&) = 0; }; class WebLayerTreeView { public: ... void registerFrameMetadataObserver(WebCompositorFrameMetadataObserver*); ... };
(In reply to comment #8) > We've been shipping with these extra IPCs on M18 for a long time with some simple rate limiting in render_view_impl, it hasn't turned up as a cause of performance problems in a while. We won't be shipping M24 with ubercomp so we need that as a stopgap. > > Anyway, for the more important long-run use case, there's also the question of how best to send the metadata from CC to RenderWidgetHost. For that, this would work, or we could do something like this alternate API: > > class WebCompositorFrameMetadataObserver { > public: > void didChangeFrameMetadata(const WebCompositorFrameMetadata&) = 0; > }; > > class WebLayerTreeView { > public: > ... > void registerFrameMetadataObserver(WebCompositorFrameMetadataObserver*); > ... > }; I don't see why this has to touch WebKit APIs at all. content/renderer provides the implementation of WebCompositorOutputSurface and the consumer of the frames, why not provide a subclass with more data and downcast where necessary?
There's an implementation of WebCompositorOutputSurface in the layout tests. It doesn't care about the metadata but it would crash if you tried that downcast+call.
content/... will never see that type. In the DumpRenderTree case, DRT is providing the WebCompositorOutputSurface.
OK, now I understand your proposal better. So we should change the GLRenderer and SoftwareRenderer to send out a WebCompositorFrame that only contains metadata and is otherwise empty? That should work although seems a bit weird in itself.
(In reply to comment #12) > OK, now I understand your proposal better. > > So we should change the GLRenderer and SoftwareRenderer to send out a WebCompositorFrame that only contains metadata and is otherwise empty? That should work although seems a bit weird in itself. Can it be refactored into cc::renderer perhaps?
(In reply to comment #12) > OK, now I understand your proposal better. > > So we should change the GLRenderer and SoftwareRenderer to send out a WebCompositorFrame that only contains metadata and is otherwise empty? That should work although seems a bit weird in itself. We would only send metadata with real frames, wouldn't we? I think it's in addition to a normal frame not instead of.
Comment on attachment 168518 [details] Patch We talked a bit offline and think we have an approach that doesn't require patching WebKit.