Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2731833002:
content: Add public context creation API. (Closed)
Descriptioncontent: Add public context creation API.
The change adds a hook to public content/ API for an embedder to create
a GL context for a given gpu surface.
BUG=698643
Review-Url: https://codereview.chromium.org/2731833002
Cr-Commit-Position: refs/heads/master@{#455324}
Committed: https://chromium.googlesource.com/chromium/src/+/003796caeb8de246a1d5719f439f25f26ca7f4f1
Patch Set 1 #
Total comments: 4
Patch Set 2 : .. #Patch Set 3 : piman's comments #
Dependent Patchsets: Messages
Total messages: 22 (11 generated)
klausw@chromium.org changed reviewers: + klausw@chromium.org
LGTM, this would fix the WebVR use case. Thank you! https://codereview.chromium.org/2731833002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2731833002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/compositor_impl_android.cc:357: gpu::GpuChannelEstablishFactory* Compositor::GetGpuChannelEstablishFactory() { Add // static comment https://codereview.chromium.org/2731833002/diff/1/content/public/browser/andr... File content/public/browser/android/compositor.h (right): https://codereview.chromium.org/2731833002/diff/1/content/public/browser/andr... content/public/browser/android/compositor.h:41: static gpu::GpuChannelEstablishFactory* GetGpuChannelFactory(); Should this get a brief comment how it should or should not be used? For example, it's probably not a good idea to use this context for drawing to the same surface the compositor is trying to use.
khushalsagar@chromium.org changed reviewers: + boliu@chromium.org
+boliu, as it turns out Vr was intending to use the ui::ContextProviderFactory that I removed here: https://codereview.chromium.org/2686243002/ I don't know if this is the best place to expose this. Let me know if you have a better idea. https://codereview.chromium.org/2731833002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2731833002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/compositor_impl_android.cc:357: gpu::GpuChannelEstablishFactory* Compositor::GetGpuChannelEstablishFactory() { On 2017/03/05 01:49:54, klausw wrote: > Add // static comment Done. https://codereview.chromium.org/2731833002/diff/1/content/public/browser/andr... File content/public/browser/android/compositor.h (right): https://codereview.chromium.org/2731833002/diff/1/content/public/browser/andr... content/public/browser/android/compositor.h:41: static gpu::GpuChannelEstablishFactory* GetGpuChannelFactory(); On 2017/03/05 01:49:54, klausw wrote: > Should this get a brief comment how it should or should not be used? For > example, it's probably not a good idea to use this context for drawing to the > same surface the compositor is trying to use. Added a comment. And I think its in general understood that when the compositor is provided with a surface, no one else should be drawing to it.
Description was changed from ========== content: Expose GpuChannelEstablishFactory. BUG= ========== to ========== content: Expose GpuChannelEstablishFactory. The change adds a hook to public content/ API for an embedder to get a GpuChannel for building GpuCommandBuffers. ==========
looking around, launching gpu process is always a content concern, and has never been exposed outside of content what's the exact use case here? maybe it should be refactored instead
On 2017/03/06 17:02:45, boliu wrote: > looking around, launching gpu process is always a content concern, and has never > been exposed outside of content > > what's the exact use case here? maybe it should be refactored instead I'm not sure that it makes sense to refactor vr rendering into content/ as we're not rendering content, we're rendering the browser (vr doesn't change the path for content getting rendered). I would expect VR is fairly unique in this respect? Are there components that establish gpu channels? We're heading in the direction of componentizing VR, so if there's a path there it might make sense to go through that. Would it be better, instead of exposing gpu::GpuChannelEstablishFactory* to expose an EstablishGpuChannel function with a callback?
On 2017/03/06 17:02:45, boliu wrote: > looking around, launching gpu process is always a content concern, and has never > been exposed outside of content > > what's the exact use case here? maybe it should be refactored instead For context, here's my attempt to summarize the result of the VC meeting, please correct me if I misunderstood things. Based on the discussion, it seems that VrShell/WebVR is a somewhat unique use case that doesn't quite match how content/ was using the GPU process. It normally does its own GL rendering with a native context, but it needs to read the WebVR-drawn frames that are inside a GPU process texture. The specific problem we're trying to solve is to transfer the content of a Mailbox-backed texture on the GPU process to a separate process with a native GL context, and (at least on Android) the only official way to do cross-process texture transfer is to read it from a SurfaceTexture after drawing it onto the corresponding Surface. I've attempted to encapsulate this in the MailboxToSurfaceBridge class in http://crrev.com/2729523002 . Bo, you said you're ok with this as long as access to the GpuChannelEstablishFactory is locked down via DEPS to restrict it to this use case?
On 2017/03/07 22:42:11, klausw wrote: > On 2017/03/06 17:02:45, boliu wrote: > > looking around, launching gpu process is always a content concern, and has > never > > been exposed outside of content > > > > what's the exact use case here? maybe it should be refactored instead > > For context, here's my attempt to summarize the result of the VC meeting, > please correct me if I misunderstood things. > > Based on the discussion, it seems that VrShell/WebVR is a somewhat unique use > case that doesn't quite match how content/ was using the GPU process. > It normally does its own GL rendering with a native context, but it > needs to read the WebVR-drawn frames that are inside a GPU process > texture. > > The specific problem we're trying to solve is to transfer the content > of a Mailbox-backed texture on the GPU process to a separate > process with a native GL context, and (at least on Android) the only > official way to do cross-process texture transfer is to read it from > a SurfaceTexture after drawing it onto the corresponding Surface. > I've attempted to encapsulate this in the MailboxToSurfaceBridge class > in http://crrev.com/2729523002 . > > Bo, you said you're ok with this as long as access to the > GpuChannelEstablishFactory is locked down via DEPS to restrict it > to this use case? DEPS restriction is supposed to be temporary so you can merge land/merge this. I still expect it to be fixed later. We (me, brandon, khushal) talked to some other gpu folks after the meeting, and the general feeling is same as me: concerned about exposing gpu details. Brandon said he was going to work with you at the end of that disucssion.
I think given the use case in the related patch set, I think I would be more comfortable with a tighter public API, like: void CreateContextProvider(gpu::SurfaceHandle handle, base::Callback<void(scoped_refptr<cc::ContextProvider>)> callback) That would internally (inside of content/) establish the channel and if successful create the context provider, and return it via the callback. That way we don't need to expose the details of creating a channel / launching processes in the public API.
khushalsagar@chromium.org changed reviewers: + piman@chromium.org
Done. I changed the API to return a ContextProvider instead. piman@, could you take a look.
Description was changed from ========== content: Expose GpuChannelEstablishFactory. The change adds a hook to public content/ API for an embedder to get a GpuChannel for building GpuCommandBuffers. ========== to ========== content: Add public context creation API. The change adds a hook to public content/ API for an embedder to create a GL context for a given gpu surface. BUG= ==========
Description was changed from ========== content: Add public context creation API. The change adds a hook to public content/ API for an embedder to create a GL context for a given gpu surface. BUG= ========== to ========== content: Add public context creation API. The change adds a hook to public content/ API for an embedder to create a GL context for a given gpu surface. BUG= ==========
lgtm
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from klausw@chromium.org Link to the patchset: https://codereview.chromium.org/2731833002/#ps40001 (title: "piman's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== content: Add public context creation API. The change adds a hook to public content/ API for an embedder to create a GL context for a given gpu surface. BUG= ========== to ========== content: Add public context creation API. The change adds a hook to public content/ API for an embedder to create a GL context for a given gpu surface. BUG=698643 ==========
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488933659661250,
"parent_rev": "fd9e5cd40b36a72ab0803325f3d3910c90c84ec5", "commit_rev":
"003796caeb8de246a1d5719f439f25f26ca7f4f1"}
Message was sent while issue was closed.
Description was changed from ========== content: Add public context creation API. The change adds a hook to public content/ API for an embedder to create a GL context for a given gpu surface. BUG=698643 ========== to ========== content: Add public context creation API. The change adds a hook to public content/ API for an embedder to create a GL context for a given gpu surface. BUG=698643 Review-Url: https://codereview.chromium.org/2731833002 Cr-Commit-Position: refs/heads/master@{#455324} Committed: https://chromium.googlesource.com/chromium/src/+/003796caeb8de246a1d5719f439f... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/003796caeb8de246a1d5719f439f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
