Unbounded Element: Connect compositor frames directly to Viz [8.5/N] [chromium/src : main]
Mason Freed (Gerrit)
Mason Freed voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
+nasko for content/browser/renderer_host/*
+chromium-ipc-reviewers for frame.mojom
Related details
- Chromium IPC Reviews
- Nasko Oskov
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
- Chromium IPC Reviews
- Kent Tamura
- Nasko Oskov
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: na...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): na...@chromium.org
Note: IPC gwsq added no new reviewers; existing reviewers satisfied requirements!
Reviewer source(s):
na...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Kent Tamura
- Nasko Oskov
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kent Tamura (Gerrit)
Kent Tamura voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kent Tamura
| Code-Review | +1 |
1 comment
+tkent for audit_non_blink_usage.py
- Mason Freed
- Nasko Oskov
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nasko Oskov (Gerrit)
Nasko Oskov added 2 comments![]( "Open in Gerrit")
Adding dtapuska@ who will likely be better reviewer than I would be for this. My understanding is that rendering and input events are handled by RenderWidgetHost rather than RenderFrameHost, but it is not my area of expertise. Therefore moving myself to cc.
}This looks to me a lot more like RenderWidgetHost style functionality - rendering and input events. Doesn't it belong there?
I think dtapuska@ will likely be better reviewer for this than I would be.
Related details
- Dave Tapuska
- Mason Freed
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 2 comments![]( "Open in Gerrit")
Adding dtapuska@ who will likely be better reviewer than I would be for this. My understanding is that rendering and input events are handled by RenderWidgetHost rather than RenderFrameHost, but it is not my area of expertise. Therefore moving myself to cc.
Sounds good! Thanks.
This looks to me a lot more like RenderWidgetHost style functionality - rendering and input events. Doesn't it belong there?
I think dtapuska@ will likely be better reviewer for this than I would be.
Hmm, I realize it's a bit funny right now since we're doing some work within `RenderWidgetHostImpl` for the unbounded element API. But there's a followup refactor [patch](https://crrev.com/c/7880871) that I'm working on which will rip all of that out of `RenderWidgetHostImpl` and move it into its own `UnboundedSurfaceWindow`. Using a separate RenderWidgetHostImpl for this isn't really right, because both the main content and the unbounded content come from the same underlying RenderWidget - just split apart into two streams.
Having said all of that, these entrypoints (e.g. GetCompositorFrameSink) correspond to the frame-level APIs coming from the renderer, just to set up the connection. So I think they do go here, and not on RenderWidgetHost. Right?
Related details
- Dave Tapuska
- Nasko Oskov
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
Just to clarify the reviewer assignments, because it got a bit garbled across a few comments:
- vmpstr: cc/* and third_party/* (i.e. most of the patch)
- dtapuska: frame.mojom (reassigned from IPC_reviewers->nasko->dtapuska)
- tkent: audit_non_blink_usage.py
Thanks in advance! And hope this helps.
Related details
- Dave Tapuska
- Nasko Oskov
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vladimir Levin (Gerrit)
Vladimir Levin added 1 comment![]( "Open in Gerrit")
void WebFrameWidgetImpl::SubmitUnboundedCompositorFrame(This looks wrong to me, in a sense of a layering violation.
Essentially, and correct me if I'm wrong, when we submit a compositor frame (on impl) we also submit an unbounded compositor frame, which bounces back to main to actually do the submission. This leaves impl thread free to, say, produce another frame before main can pick up this frame. Can that new compositor frame reuse/invalidate the resources that are still in flight for the main thread? I'm not sure I know the answer to that. (also scrolling, composited transform animations or anything else that we may (or may not) want to support will suffer)
This also likely makes trees in viz implementation difficult since I'm not sure how we would pass "tree information" (which I presume we need) back to main thread to pass to the unbounded client. That being said, I don't know how trees in viz implementation would work yet since I don't know how trees in viz works in detail :)
I would have imagined that the solution in this space instead sends the unbounded_frame_sink_ to the impl thread so that it can use the same path as SubmitCompositorFrame does
@jon...@chromium.org for opinions
Related details
- Dave Tapuska
- Jonathan Ross
- Mason Freed
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska added 5 comments![]( "Open in Gerrit")
unbounded_surface_client_;So this allows one unbounded surface per frame. So why do I need to create a new iframe to create multiple unbounded widgets. Should there be a check that there is only one unbounded widget per tab? etc.. (I thought we do this for selects)
Mason FreedThis looks to me a lot more like RenderWidgetHost style functionality - rendering and input events. Doesn't it belong there?
I think dtapuska@ will likely be better reviewer for this than I would be.
Hmm, I realize it's a bit funny right now since we're doing some work within `RenderWidgetHostImpl` for the unbounded element API. But there's a followup refactor [patch](https://crrev.com/c/7880871) that I'm working on which will rip all of that out of `RenderWidgetHostImpl` and move it into its own `UnboundedSurfaceWindow`. Using a separate RenderWidgetHostImpl for this isn't really right, because both the main content and the unbounded content come from the same underlying RenderWidget - just split apart into two streams.
Having said all of that, these entrypoints (e.g. GetCompositorFrameSink) correspond to the frame-level APIs coming from the renderer, just to set up the connection. So I think they do go here, and not on RenderWidgetHost. Right?
I think this is ok. (although I'd like the dependent CL)...
if (unbounded_surface_client_.is_bound()) {Why is this here? Shouldn't 11291 CHECK
RenderWidgetHostView* parent_view = GetRenderWidgetHost()->GetView();(Not for this CL) But how does this work when going between DSF between monitors? Can the unbounded view appear on a different monitor with a different DSF?
unbounded_surface_client_receiver_{this, nullptr};Passing null into these HeapMojo objects basically prevents the context destroyed notifications I believe. Could be dangerous, we should think about associating this with the execution context.
There seems to be a disconnect here. On the blink side we bind these interfaces on the Widget level, but on the browser side we are binding them to the Frame.
Related details
Jonathan Ross (Gerrit)
Jonathan Ross added 5 comments![]( "Open in Gerrit")
widget_host_view->SetBounds(updated_rect);Please fix this WARNING reported by autoreview issue finding: When the bounds are updated, `RenderWidgetHostView::SetBounds` usually results in a new `LocalSurfaceId` being allocated. Since this special widget has no `WebWidget` in the renderer, the standard `SynchronizeVisualProperties` flow won't update the renderer.
You should call `unbounded_surface_client_->OnSurfaceAllocated` here to provide the renderer with the updated ID, otherwise future frames submitted with the stale ID may be rejected by Viz or result in visual artifacts.
const viz::FrameSinkId& frame_sink_id,
const viz::LocalSurfaceId& local_surface_id) {`FrameSinkId` will be the invariant identifier for this process. If that changes then we need to rebind.
`LocalSurfaceId` will be constantly changing. Any resizes or other `blink::VisualProperty` changes initialized from the Browser will lead to this advancing.
The `CompositorFrameSinkClient` -> `CompositorFrameSinkSupport` are setup to handle the changes to the id over time. We shouldn't close the pipe when it changes
void WebFrameWidgetImpl::SubmitUnboundedCompositorFrame(This looks wrong to me, in a sense of a layering violation.
Essentially, and correct me if I'm wrong, when we submit a compositor frame (on impl) we also submit an unbounded compositor frame, which bounces back to main to actually do the submission. This leaves impl thread free to, say, produce another frame before main can pick up this frame. Can that new compositor frame reuse/invalidate the resources that are still in flight for the main thread? I'm not sure I know the answer to that. (also scrolling, composited transform animations or anything else that we may (or may not) want to support will suffer)
This also likely makes trees in viz implementation difficult since I'm not sure how we would pass "tree information" (which I presume we need) back to main thread to pass to the unbounded client. That being said, I don't know how trees in viz implementation would work yet since I don't know how trees in viz works in detail :)
I would have imagined that the solution in this space instead sends the unbounded_frame_sink_ to the impl thread so that it can use the same path as SubmitCompositorFrame does
@jon...@chromium.org for opinions
+1 the frame submission should be done from the impl thread.
Currently `AsyncLayerTreeFrameSink` is setup by `blink::WidgetBase` on the main-thread, but is provided to `LayerTreeHostImpl`. We would similarly want this connection managed on the Impl thread
void WebFrameWidgetImpl::ReclaimResources(The resources being exported will be managed by `viz::ClientResourceProvider` that lives on the Impl thread. As a `LayerTreeImpl` is processed the resources are exported there.
By returning them on the Impl thread we can clean up the current tree state.
We already have the concept of a `main_thread_callback` to complete the release on the Main-thread if required. Canvas makes use of this today: https://source.chromium.org/chromium/chromium/src/+/main:components/viz/client/client_resource_provider.cc;l=178-181#:~:text=177-,178,-179
void WidgetBase::UpdateVisualProperties(
const VisualProperties& visual_properties_from_browser) {
TRACE_EVENT0("renderer", "WidgetBase::UpdateVisualProperties");We will need to connect this to the UnboundedElement. As when properies change its SurfaceId will need to advance. Otherwise Viz will not embed it
Related details
- Mason Freed
- Nasko Oskov