Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2190033002:
content: Add ContextProviderFactory to create a render ContextProvider. (Closed)
Descriptioncontent: Add ContextProviderFactory to create a render ContextProvider.
Add a ContextProviderFactory to ui to create a ContextProvider for an
external compositor rendering web content. This will be used by the
Blimp Compositor. Also,
1) Consolidate the GpuChannel initialization logic
and ownership of the VulkanContextProvider for the UI compositor in
ContextProviderFactoryImpl.
2) Move cc::SurfaceManager and surface_ids allocation to
ContextProviderFactory.
BUG=624830
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/c81e43b82548d619a0b8ebd4dfd2362a8bbf5e2a
Cr-Commit-Position: refs/heads/master@{#410522}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Move ContextProviderFactory to ui #
Total comments: 2
Patch Set 3 : .. #
Total comments: 23
Patch Set 4 : .. #
Total comments: 25
Patch Set 5 : Addressed comments #Patch Set 6 : format for realz #
Total comments: 13
Patch Set 7 : Addressed comments #
Total comments: 10
Patch Set 8 : Addressed comments. #
Total comments: 4
Patch Set 9 : .. #Patch Set 10 : .. #Patch Set 11 : copy ctor. #
Total comments: 1
Patch Set 12 : comments + callback trigger logic fix. #Patch Set 13 : use the surface handle, not widget. #Patch Set 14 : add setup for tests. #Patch Set 15 : remove display_ DCHECK. #Messages
Total messages: 82 (29 generated)
khushalsagar@chromium.org changed reviewers: + sievers@chromium.org
Only added code to create a context provider for Blimp. Its not really necessary to change it for the UI compositor. https://codereview.chromium.org/2190033002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/context_provider_factory_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/context_provider_factory_impl_android.cc:218: if (!shared_worker_context_provider_->BindToCurrentThread()) Does this look correct? We do it on the main thread in the renderer too but I am not sure.
Description was changed from ========== content: Add ContextProviderFactory to create a render ContextProvider. Add a content public API to create a ContextProvider for an external compositor rendering web content. This will be used by the BLimp Compositor. Also consolidate the GpuChannel initialization logic and ownership of the VulkanContextProvider for the UI compositor in ContextProviderFactoryImpl. BUG=624830 ========== to ========== content: Add ContextProviderFactory to create a render ContextProvider. Add a content public API to create a ContextProvider for an external compositor rendering web content. This will be used by the BLimp Compositor. Also consolidate the GpuChannel initialization logic and ownership of the VulkanContextProvider for the UI compositor in ContextProviderFactoryImpl. BUG=624830 ==========
khushalsagar@chromium.org changed reviewers: + danakj@chromium.org, dtrainor@chromium.org
Moved the ContextProviderFactory to ui/. +dtrainor for ui/android +danakj for cc/ DEPS.
https://codereview.chromium.org/2190033002/diff/20001/ui/android/context_prov... File ui/android/context_provider_factory.h (right): https://codereview.chromium.org/2190033002/diff/20001/ui/android/context_prov... ui/android/context_provider_factory.h:20: class UI_ANDROID_EXPORT ContextProviderFactory { Hm, I'm planning to make a ContextProvider::Factory.. https://codereview.chromium.org/1985973002/diff/80001/cc/output/context_provi... That defers creation to another thread. Why are you not using the contexts on RenderThreadImpl?
https://codereview.chromium.org/2190033002/diff/20001/ui/android/context_prov... File ui/android/context_provider_factory.h (right): https://codereview.chromium.org/2190033002/diff/20001/ui/android/context_prov... ui/android/context_provider_factory.h:20: class UI_ANDROID_EXPORT ContextProviderFactory { On 2016/07/29 01:26:15, danakj wrote: > Hm, I'm planning to make a ContextProvider::Factory.. > > https://codereview.chromium.org/1985973002/diff/80001/cc/output/context_provi... > > That defers creation to another thread. Why are you not using the contexts on > RenderThreadImpl? Your change looks like it is to have cc embedder pass in a factory so we can ask them to create the context on the compositor thread. This ContextProviderFactory is so blimp can ask content to create a context provider, since it requires access to GpuChannelHost and CommandBuffer, which are in content and we can't use any of the content code in blimp. After your change, this class will create a ContextProvider::Factory I guess. It would be nice to move the setup for the render contexts so we can at least share it with the impl of this class. Perhaps moving it to someplace in content/common because the impl of this class will live in the browser.
Description was changed from ========== content: Add ContextProviderFactory to create a render ContextProvider. Add a content public API to create a ContextProvider for an external compositor rendering web content. This will be used by the BLimp Compositor. Also consolidate the GpuChannel initialization logic and ownership of the VulkanContextProvider for the UI compositor in ContextProviderFactoryImpl. BUG=624830 ========== to ========== content: Add ContextProviderFactory to create a render ContextProvider. Add a ContextProviderFactory to ui to create a ContextProvider for an external compositor rendering web content. This will be used by the Blimp Compositor. Also, 1) Consolidate the GpuChannel initialization logic and ownership of the VulkanContextProvider for the UI compositor in ContextProviderFactoryImpl. 2) Move cc::SurfaceManager and surface_ids allocation to ContextProviderFactory. BUG=624830 ==========
Hm.. oh so blimp is outside of content/ right okay. Like ui::Compositor, and in the browser, that reminds me: Can it just use ui::Compositor's API? We already have that ContextFactory: https://cs.chromium.org/chromium/src/ui/compositor/compositor.h?rcl=146973376... That's implemented inside content in the browser.
On 2016/07/29 02:24:43, danakj wrote: > Hm.. oh so blimp is outside of content/ right okay. Like ui::Compositor, and in > the browser, that reminds me: > > Can it just use ui::Compositor's API? We already have that ContextFactory: > https://cs.chromium.org/chromium/src/ui/compositor/compositor.h?rcl=146973376... > That's implemented inside content in the browser. That would have been nice, but the ui compositor and its ContextFactory is all for aura, and android doesn't use any of it. :) Irrespective of which one we would have used, I would assume that there would be some settings which would be set differently for the renderer and UI compositor. With Blimp, we are effectively running the renderer compositor on the browser side.
https://codereview.chromium.org/2190033002/diff/40001/content/browser/browser... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2190033002/diff/40001/content/browser/browser... content/browser/browser_main_loop.cc:119: #include "ui/android/context_provider_factory.h" nit: not needed for base class https://codereview.chromium.org/2190033002/diff/40001/content/browser/browser... content/browser/browser_main_loop.cc:1240: ContextProviderFactoryImpl::GetInstance()); do we leak this? Can you unset it somewhere around where we also do ImageTransportFactory::Terminate() in this file? https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (left): https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:494: establish_gpu_channel_timeout_.Stop(); Hmm I wonder why this was here, and how we could convince ourselves that it's not needed anymore (or never was). https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... File content/browser/renderer_host/context_provider_factory_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... content/browser/renderer_host/context_provider_factory_impl_android.cc:39: ContextProviderCallback result_callback) { DCHECK(!in_handle_pending_requests_) since it would break the iterator https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... content/browser/renderer_host/context_provider_factory_impl_android.cc:131: void ContextProviderFactoryImpl::CreateRenderCompositorContexts( nit: this is mainly used for the browser compositor, so maybe just CreateCompositorContexts()? https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... content/browser/renderer_host/context_provider_factory_impl_android.cc:148: gpu::SharedMemoryLimits limits = gpu::SharedMemoryLimits::ForMailboxContext(); Why would 'ForMailboxContext' be correct for any sort of compositor context? // These are limits for contexts only used for creating textures, mailboxing // them and dealing with synchronization. https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... File content/browser/renderer_host/context_provider_factory_impl_android.h (right): https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... content/browser/renderer_host/context_provider_factory_impl_android.h:48: void RequestGpuChannel( Could we maybe make this 'private' by also having the actual context creation go through here for the browser compositor? Basically instead of CompositorImpl::CreateOutputSurface(gpu_channel_host) make it CompositorImpl::CreateOutputSurface(context_provider). You might have to pass in a few things like the SharedMemoryLimits (and surface id for onscreen), but then also see the comment below which that would probably address. https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... content/browser/renderer_host/context_provider_factory_impl_android.h:66: void CreateRenderCompositorContexts( Something is a bit funky about this: This implementation lives in content/browser/renderer_host, so it shouldn't deal with 'renderer' stuff. And also the browser compositor does not use the context factory in here at all. https://codereview.chromium.org/2190033002/diff/40001/ui/android/context_prov... File ui/android/context_provider_factory.cc (right): https://codereview.chromium.org/2190033002/diff/40001/ui/android/context_prov... ui/android/context_provider_factory.cc:7: #include "ui/android/context_provider_factory.h" nit: put this include at the top with an empty line after https://codereview.chromium.org/2190033002/diff/40001/ui/android/context_prov... File ui/android/context_provider_factory.h (right): https://codereview.chromium.org/2190033002/diff/40001/ui/android/context_prov... ui/android/context_provider_factory.h:41: virtual ~ContextProviderFactory(){}; nit: space in between () and {} https://codereview.chromium.org/2190033002/diff/40001/ui/android/context_prov... ui/android/context_provider_factory.h:45: virtual void RequestRenderContextProvider( nit: maybe 'CreateOffscreenContextProvider()'?
> That would have been nice, but the ui compositor and its ContextFactory is all > for aura, and android doesn't use any of it. :) > Irrespective of which one we would have used, I would assume that there would be > some settings which would be set differently for the renderer and UI compositor. > With Blimp, we are effectively running the renderer compositor on the browser > side. OK I can see why you wouldn't want to use ui::Compositor, since ui::Layer is tied to aura. I guess ContextFactory is using ui::Compositor pointers, tho not for too much. How much of GpuProcessTransportFactory is going to be recreated? It might be worth making the one interface usable by android. What do you think sievers? If not, maybe the new one can be called ContextFactoryAndroid to make it more different than ContextProvider::Factory, and show its relationship to the existing ContextFactory? https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... File content/browser/renderer_host/context_provider_factory_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... content/browser/renderer_host/context_provider_factory_impl_android.cc:148: gpu::SharedMemoryLimits limits = gpu::SharedMemoryLimits::ForMailboxContext(); On 2016/07/29 19:14:30, sievers wrote: > Why would 'ForMailboxContext' be correct for any sort of compositor context? > > // These are limits for contexts only used for creating textures, mailboxing > // them and dealing with synchronization. The (non-display) compositor context doesn't do anything except mailboxing .. if it has a worker context for GPU raster work. The Display needs more limits, as does the worker for GPU raster.
On 2016/07/29 19:23:24, danakj wrote: > > That would have been nice, but the ui compositor and its ContextFactory is all > > for aura, and android doesn't use any of it. :) > > Irrespective of which one we would have used, I would assume that there would > be > > some settings which would be set differently for the renderer and UI > compositor. > > With Blimp, we are effectively running the renderer compositor on the browser > > side. > > OK I can see why you wouldn't want to use ui::Compositor, since ui::Layer is > tied to aura. I guess ContextFactory is using ui::Compositor pointers, tho not > for too much. How much of GpuProcessTransportFactory is going to be recreated? > It might be worth making the one interface usable by android. What do you think > sievers? > Some sort of code sharing for the GPU channel establishment, context creation (incl. some of the failure handling), and even potentially some of the Gpu(BrowserCompositor)OutputSurface implementation would be great. But it's probably best to attempt that later and sort of extract the logic from compositor_impl_android.cc first. There are also some caveats as Android doesn't support EstablishGpuChannelSync() and therefore doesn't want a synchronous SharedMainThreadContextProvider() API, for example. (We really don't want a main thread context at all, but currently need it for GLHelper.) And yea, GpuProcessTransportFactory would need to be made independent of Aura/ui::Compositor somehow. Maybe it also shouldn't implement both ImageTransportFactory and ContextFactory.
On Fri, Jul 29, 2016 at 12:31 PM, <sievers@chromium.org> wrote: > On 2016/07/29 19:23:24, danakj wrote: > > > That would have been nice, but the ui compositor and its > ContextFactory is > all > > > for aura, and android doesn't use any of it. :) > > > Irrespective of which one we would have used, I would assume that there > would > > be > > > some settings which would be set differently for the renderer and UI > > compositor. > > > With Blimp, we are effectively running the renderer compositor on the > browser > > > side. > > > > OK I can see why you wouldn't want to use ui::Compositor, since > ui::Layer is > > tied to aura. I guess ContextFactory is using ui::Compositor pointers, > tho not > > for too much. How much of GpuProcessTransportFactory is going to be > recreated? > > It might be worth making the one interface usable by android. What do you > think > > sievers? > > > > Some sort of code sharing for the GPU channel establishment, context > creation > (incl. some of the failure handling), and even potentially some of the > Gpu(BrowserCompositor)OutputSurface implementation would be great. > > But it's probably best to attempt that later and sort of extract the logic > from > compositor_impl_android.cc first. > Okie. > There are also some caveats as Android doesn't support > EstablishGpuChannelSync() > and therefore doesn't want a synchronous SharedMainThreadContextProvider() > API, > for example. (We really don't want a main thread context at all, but > currently > need it for GLHelper.) > > And yea, GpuProcessTransportFactory would need to be made independent of > Aura/ui::Compositor somehow. > > Maybe it also shouldn't implement both ImageTransportFactory and > ContextFactory. > Haha yah, that's the dream! > > > > > https://codereview.chromium.org/2190033002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2190033002/diff/40001/ui/android/DEPS File ui/android/DEPS (right): https://codereview.chromium.org/2190033002/diff/40001/ui/android/DEPS#newcode6 ui/android/DEPS:6: "+cc/output/context_provider.h", DEPS LGTM
https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... File content/browser/renderer_host/context_provider_factory_impl_android.h (right): https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... content/browser/renderer_host/context_provider_factory_impl_android.h:48: void RequestGpuChannel( On 2016/07/29 19:14:31, sievers wrote: > Could we maybe make this 'private' by also having the actual context creation go > through here for the browser compositor? > > Basically instead of CompositorImpl::CreateOutputSurface(gpu_channel_host) make > it CompositorImpl::CreateOutputSurface(context_provider). > > You might have to pass in a few things like the SharedMemoryLimits (and surface > id for onscreen), but then also see the comment below which that would probably > address. Ok I see now what the slightly tricky part if with the onscreen context: If surfaceDestroyed() happens while we are establishing a GPU channel, then we must not try to create an OutputSurface with that surface_id. It's the early-out here: void CompositorImpl::CreateOutputSurface() { // We might have had a request from a LayerTreeHost that was then // hidden (and hidden means we don't have a native surface). // Also make sure we only handle this once. if (!output_surface_request_pending_ || !host_->visible()) return; Maybe it's good enough though to check with the GpuSurfaceTracker or so to see if it's still valid before we attempt to create an onscreen context with it.
https://codereview.chromium.org/2190033002/diff/40001/content/browser/browser... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2190033002/diff/40001/content/browser/browser... content/browser/browser_main_loop.cc:1240: ContextProviderFactoryImpl::GetInstance()); On 2016/07/29 19:14:30, sievers wrote: > do we leak this? > > Can you unset it somewhere around where we also do > ImageTransportFactory::Terminate() in this file? The singleton would still remain with the ContextProviderFactoryImpl right? I was following a pattern similar to DiscardableMemoryAllocator. https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (left): https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:494: establish_gpu_channel_timeout_.Stop(); On 2016/07/29 19:14:30, sievers wrote: > Hmm I wonder why this was here, and how we could convince ourselves that it's > not needed anymore (or never was). This happens when the surface goes away. Is that related to the gpu channel initialization in anyway? If we reset the timeout now in the ContextProviderFactoryImpl when this happens, we could still have context requests from other compositors. https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... File content/browser/renderer_host/context_provider_factory_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... content/browser/renderer_host/context_provider_factory_impl_android.cc:39: ContextProviderCallback result_callback) { On 2016/07/29 19:14:30, sievers wrote: > DCHECK(!in_handle_pending_requests_) since it would break the iterator We copy the list before iterating. I figured that we can expect more requests while we are handling the current ones, because we will be running callbacks and if the context initialization fails then we might get another request? https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... content/browser/renderer_host/context_provider_factory_impl_android.cc:131: void ContextProviderFactoryImpl::CreateRenderCompositorContexts( On 2016/07/29 19:14:30, sievers wrote: > nit: this is mainly used for the browser compositor, so maybe just > CreateCompositorContexts()? This is going to be used for Blimp compositor too. How about CreateCompositorContexts for those and CreateDisplayContexts for the browser one instead? The implementation below is for the former. https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... File content/browser/renderer_host/context_provider_factory_impl_android.h (right): https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... content/browser/renderer_host/context_provider_factory_impl_android.h:66: void CreateRenderCompositorContexts( On 2016/07/29 19:14:30, sievers wrote: > Something is a bit funky about this: > > This implementation lives in content/browser/renderer_host, so it shouldn't deal > with 'renderer' stuff. Render is a bad word. I called it that because effectively the Blimp compositor will do the work that the renderer compositor does. > And also the browser compositor does not use the context factory in here at all. Will move that to this class within this patch too.
https://codereview.chromium.org/2190033002/diff/40001/content/browser/browser... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2190033002/diff/40001/content/browser/browser... content/browser/browser_main_loop.cc:1240: ContextProviderFactoryImpl::GetInstance()); On 2016/07/29 21:30:55, Khushal wrote: > On 2016/07/29 19:14:30, sievers wrote: > > do we leak this? > > > > Can you unset it somewhere around where we also do > > ImageTransportFactory::Terminate() in this file? > > The singleton would still remain with the ContextProviderFactoryImpl right? I > was following a pattern similar to DiscardableMemoryAllocator. sounds good https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (left): https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:494: establish_gpu_channel_timeout_.Stop(); On 2016/07/29 21:30:55, Khushal wrote: > On 2016/07/29 19:14:30, sievers wrote: > > Hmm I wonder why this was here, and how we could convince ourselves that it's > > not needed anymore (or never was). > > This happens when the surface goes away. Is that related to the gpu channel > initialization in anyway? If we reset the timeout now in the > ContextProviderFactoryImpl when this happens, we could still have context > requests from other compositors. I don't remember why this is here. It seems to somehow suggest that OnGpuChannelEstablished() might not happen if we became invisible. Don't worry about it for now. https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... File content/browser/renderer_host/context_provider_factory_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... content/browser/renderer_host/context_provider_factory_impl_android.cc:131: void ContextProviderFactoryImpl::CreateRenderCompositorContexts( On 2016/07/29 21:30:55, Khushal wrote: > On 2016/07/29 19:14:30, sievers wrote: > > nit: this is mainly used for the browser compositor, so maybe just > > CreateCompositorContexts()? > > This is going to be used for Blimp compositor too. How about > CreateCompositorContexts for those and CreateDisplayContexts for the browser one > instead? > > The implementation below is for the former. Hmm or how about just passing the config bits as arguments that are different?
https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... File content/browser/renderer_host/context_provider_factory_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... content/browser/renderer_host/context_provider_factory_impl_android.cc:131: void ContextProviderFactoryImpl::CreateRenderCompositorContexts( On 2016/07/29 21:42:41, sievers wrote: > On 2016/07/29 21:30:55, Khushal wrote: > > On 2016/07/29 19:14:30, sievers wrote: > > > nit: this is mainly used for the browser compositor, so maybe just > > > CreateCompositorContexts()? > > > > This is going to be used for Blimp compositor too. How about > > CreateCompositorContexts for those and CreateDisplayContexts for the browser > one > > instead? > > > > The implementation below is for the former. > > Hmm or how about just passing the config bits as arguments that are different? That's cool too. Let me put up a patch and see what that looks like.
On 2016/07/29 22:02:26, Khushal wrote: > https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... > File content/browser/renderer_host/context_provider_factory_impl_android.cc > (right): > > https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... > content/browser/renderer_host/context_provider_factory_impl_android.cc:131: void > ContextProviderFactoryImpl::CreateRenderCompositorContexts( > On 2016/07/29 21:42:41, sievers wrote: > > On 2016/07/29 21:30:55, Khushal wrote: > > > On 2016/07/29 19:14:30, sievers wrote: > > > > nit: this is mainly used for the browser compositor, so maybe just > > > > CreateCompositorContexts()? > > > > > > This is going to be used for Blimp compositor too. How about > > > CreateCompositorContexts for those and CreateDisplayContexts for the browser > > one > > > instead? > > > > > > The implementation below is for the former. > > > > Hmm or how about just passing the config bits as arguments that are different? > > That's cool too. Let me put up a patch and see what that looks like. So, I'll end up having to add a dependency on gpu/ if I need to add the SurfaceHandle to ContextProviderFactory API. Would it make more sense to have the display context creation request be on ContextProviderFactoryImpl instead? Its either way to be accessed in content/ only.
On 2016/07/30 00:52:48, Khushal wrote: > On 2016/07/29 22:02:26, Khushal wrote: > > > https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... > > File content/browser/renderer_host/context_provider_factory_impl_android.cc > > (right): > > > > > https://codereview.chromium.org/2190033002/diff/40001/content/browser/rendere... > > content/browser/renderer_host/context_provider_factory_impl_android.cc:131: > void > > ContextProviderFactoryImpl::CreateRenderCompositorContexts( > > On 2016/07/29 21:42:41, sievers wrote: > > > On 2016/07/29 21:30:55, Khushal wrote: > > > > On 2016/07/29 19:14:30, sievers wrote: > > > > > nit: this is mainly used for the browser compositor, so maybe just > > > > > CreateCompositorContexts()? > > > > > > > > This is going to be used for Blimp compositor too. How about > > > > CreateCompositorContexts for those and CreateDisplayContexts for the > browser > > > one > > > > instead? > > > > > > > > The implementation below is for the former. > > > > > > Hmm or how about just passing the config bits as arguments that are > different? > > > > That's cool too. Let me put up a patch and see what that looks like. > > So, I'll end up having to add a dependency on gpu/ if I need to add the > SurfaceHandle to ContextProviderFactory API. Would it make more sense to have > the display context creation request be on ContextProviderFactoryImpl instead? > Its either way to be accessed in content/ only. Never mind. Didn't realize I can pass in the gfx::AcceleratedWidget and use the GpuSurfaceTracker to get the surface handle later.
Looks better now. PTAL. I'll use this change to move the DelegatedFrameHostAndroid to ui/ as well. https://codereview.chromium.org/2190033002/diff/60001/ui/android/context_prov... File ui/android/context_provider_factory.h (right): https://codereview.chromium.org/2190033002/diff/60001/ui/android/context_prov... ui/android/context_provider_factory.h:70: virtual cc::SharedBitmapManager* GetSharedBitmapManager() = 0; Added these too, we'll need them for Blimp anyway.
https://codereview.chromium.org/2190033002/diff/60001/content/browser/browser... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2190033002/diff/60001/content/browser/browser... content/browser/browser_main_loop.cc:1238: ContextProviderFactoryImpl::GetInstance()); still leaking. (i'm not worried about a normal app that wouldn't shut down on android, but maybe in a test setup it could leak.) https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (left): https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:585: << "Too many context creation failures. Giving up... "; you accidentally removed the log string. https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:526: RequestNewOutputSurface(); This will fail the DCHECK() you added in RequestNewOutputSurface(). So need to call HandlePending..() instead. https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:596: CreateVulkanOutputSurface() if (display_) return; This also used to check the kEnableVulkan command line but doesn't anymore. https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:596: CreateVulkanOutputSurface() if (display_) return; formatting https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:599: DCHECK(!surface_handle_ == gpu::kNullSurfaceHandle); DCHECK(surface_handle_ != gpu::kNullSurfaceHandle)? https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... File content/browser/renderer_host/context_provider_factory_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... content/browser/renderer_host/context_provider_factory_impl_android.cc:130: ContextProvidersRequest& context_request, nit: pass by pointer per style guide for non-const https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... content/browser/renderer_host/context_provider_factory_impl_android.cc:131: scoped_refptr<gpu::GpuChannelHost> gpu_channel_host) { nit: const scoped_refptr<>& https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:350: id_allocator_.reset(new cc::SurfaceIdAllocator( This can probably all be behind |using_browser_compositor_| to not allocate anything in WebView. https://codereview.chromium.org/2190033002/diff/60001/ui/android/context_prov... File ui/android/context_provider_factory.cc (right): https://codereview.chromium.org/2190033002/diff/60001/ui/android/context_prov... ui/android/context_provider_factory.cc:21: DCHECK(!g_context_provider_factory); DCHECK(!g_context_provider_factory || !context_provider_factory); https://codereview.chromium.org/2190033002/diff/60001/ui/android/context_prov... File ui/android/context_provider_factory.h (right): https://codereview.chromium.org/2190033002/diff/60001/ui/android/context_prov... ui/android/context_provider_factory.h:50: virtual ~ContextProviderFactory(){}; nit: space missing in between () and {} https://codereview.chromium.org/2190033002/diff/60001/ui/android/context_prov... ui/android/context_provider_factory.h:58: virtual void RequestContextProviders( nit: I think 'Create' is more precise than 'Request'
Thanks Daniel. Done. https://codereview.chromium.org/2190033002/diff/60001/content/browser/browser... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2190033002/diff/60001/content/browser/browser... content/browser/browser_main_loop.cc:1238: ContextProviderFactoryImpl::GetInstance()); On 2016/08/02 20:44:33, sievers wrote: > still leaking. (i'm not worried about a normal app that wouldn't shut down on > android, but maybe in a test setup it could leak.) Resetting it on shutdown. Is that better? https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (left): https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:585: << "Too many context creation failures. Giving up... "; On 2016/08/02 20:44:33, sievers wrote: > you accidentally removed the log string. Woops. Done. https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:526: RequestNewOutputSurface(); On 2016/08/02 20:44:34, sievers wrote: > This will fail the DCHECK() you added in RequestNewOutputSurface(). > So need to call HandlePending..() instead. Thanks. DOne. https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:596: CreateVulkanOutputSurface() if (display_) return; On 2016/08/02 20:44:34, sievers wrote: > This also used to check the kEnableVulkan command line but doesn't anymore. Sorry. Added it back in CreateVulkanOutputSurface. https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:596: CreateVulkanOutputSurface() if (display_) return; On 2016/08/02 20:44:33, sievers wrote: > formatting Apparently git cl format likes the previous one. Done. https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:599: DCHECK(!surface_handle_ == gpu::kNullSurfaceHandle); On 2016/08/02 20:44:34, sievers wrote: > DCHECK(surface_handle_ != gpu::kNullSurfaceHandle)? Done. https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... File content/browser/renderer_host/context_provider_factory_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... content/browser/renderer_host/context_provider_factory_impl_android.cc:130: ContextProvidersRequest& context_request, On 2016/08/02 20:44:34, sievers wrote: > nit: pass by pointer per style guide for non-const Done. https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... content/browser/renderer_host/context_provider_factory_impl_android.cc:131: scoped_refptr<gpu::GpuChannelHost> gpu_channel_host) { On 2016/08/02 20:44:34, sievers wrote: > nit: const scoped_refptr<>& Done. https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2190033002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:350: id_allocator_.reset(new cc::SurfaceIdAllocator( On 2016/08/02 20:44:34, sievers wrote: > This can probably all be behind |using_browser_compositor_| to not allocate > anything in WebView. Done. https://codereview.chromium.org/2190033002/diff/60001/ui/android/context_prov... File ui/android/context_provider_factory.cc (right): https://codereview.chromium.org/2190033002/diff/60001/ui/android/context_prov... ui/android/context_provider_factory.cc:21: DCHECK(!g_context_provider_factory); On 2016/08/02 20:44:34, sievers wrote: > DCHECK(!g_context_provider_factory || !context_provider_factory); Done. https://codereview.chromium.org/2190033002/diff/60001/ui/android/context_prov... File ui/android/context_provider_factory.h (right): https://codereview.chromium.org/2190033002/diff/60001/ui/android/context_prov... ui/android/context_provider_factory.h:50: virtual ~ContextProviderFactory(){}; On 2016/08/02 20:44:34, sievers wrote: > nit: space missing in between () and {} Done. https://codereview.chromium.org/2190033002/diff/60001/ui/android/context_prov... ui/android/context_provider_factory.h:58: virtual void RequestContextProviders( On 2016/08/02 20:44:34, sievers wrote: > nit: I think 'Create' is more precise than 'Request' Done.
https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:613: bool CompositorImpl::CreateVulkanOutputSurface() { s/bool/void https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:615: switches::kEnableVulkan)) !kEnableVulkan https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:646: if (!host_->visible()) If we became invisible then we also shouldn't have a context here since the surface is not valid anymore. SO maybe DCHECK() that the passed contexts are null. https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... File content/browser/renderer_host/context_provider_factory_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... content/browser/renderer_host/context_provider_factory_impl_android.cc:125: HandlePendingRequests(); what's this for? https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:807: ui::ContextProviderFactory::GetInstance() this needs an if-check whether the id_allocator_ exists now. (you could also use GetSurfaceClientId() here)
https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:613: bool CompositorImpl::CreateVulkanOutputSurface() { On 2016/08/02 22:47:36, sievers wrote: > s/bool/void Done. https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:615: switches::kEnableVulkan)) On 2016/08/02 22:47:36, sievers wrote: > !kEnableVulkan Umm, sorry. Done. https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:646: if (!host_->visible()) On 2016/08/02 22:47:36, sievers wrote: > If we became invisible then we also shouldn't have a context here since the > surface is not valid anymore. > > SO maybe DCHECK() that the passed contexts are null. Done. Added the worker context provider check at the beginning, since we never have that for the browser compositor. https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... File content/browser/renderer_host/context_provider_factory_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... content/browser/renderer_host/context_provider_factory_impl_android.cc:125: HandlePendingRequests(); On 2016/08/02 22:47:36, sievers wrote: > what's this for? In case any new requests came in while we were looping over the previous list above. https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:807: ui::ContextProviderFactory::GetInstance() On 2016/08/02 22:47:36, sievers wrote: > this needs an if-check whether the id_allocator_ exists now. > (you could also use GetSurfaceClientId() here) Done. In my next change I'm moving all the surfaces stuff to DelegatedFrameHost. I shouldn't be building it at all then for webview?
lgtm don't break world plz. :P https://codereview.chromium.org/2190033002/diff/120001/ui/android/context_pro... File ui/android/context_provider_factory.cc (right): https://codereview.chromium.org/2190033002/diff/120001/ui/android/context_pro... ui/android/context_provider_factory.cc:11: ContextProviderFactory* g_context_provider_factory = nullptr; anonymous namespace? https://codereview.chromium.org/2190033002/diff/120001/ui/android/ui_android.gyp File ui/android/ui_android.gyp (right): https://codereview.chromium.org/2190033002/diff/120001/ui/android/ui_android.... ui/android/ui_android.gyp:27: 'context_provider_factory.cc', diez!
https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... File content/browser/renderer_host/context_provider_factory_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... content/browser/renderer_host/context_provider_factory_impl_android.cc:125: HandlePendingRequests(); On 2016/08/02 23:31:53, Khushal wrote: > On 2016/08/02 22:47:36, sievers wrote: > > what's this for? > > In case any new requests came in while we were looping over the previous list > above. Oh triggered from the callback? Good point. https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:807: ui::ContextProviderFactory::GetInstance() On 2016/08/02 23:31:54, Khushal wrote: > On 2016/08/02 22:47:36, sievers wrote: > > this needs an if-check whether the id_allocator_ exists now. > > (you could also use GetSurfaceClientId() here) > > Done. In my next change I'm moving all the surfaces stuff to DelegatedFrameHost. > I shouldn't be building it at all then for webview? Yes, I think WebView could possibly not instantiate a DFH based on |using_browser_compositor_|. https://codereview.chromium.org/2190033002/diff/120001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/120001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:648: DCHECK(!context_providers.compositor_context_provider); can you put a comment saying that 'if we don't have a surface (invisible), we should not have created a context.' https://codereview.chromium.org/2190033002/diff/120001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:659: pending_swapbuffers_ = 0; the vulkan path should reset |pending_swapbuffers_| also. move it into InitializeDisplay() maybe? https://codereview.chromium.org/2190033002/diff/120001/content/browser/render... File content/browser/renderer_host/context_provider_factory_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/120001/content/browser/render... content/browser/renderer_host/context_provider_factory_impl_android.cc:151: SharedCompositorWorkerContextProvider(gpu_channel_host); Now that I think about it: If the browser compositor were to raster, then SharedCompositorWorkerContextProvider() should not really be shared with a different compositor (Blimp). So this is really exposing something Blimp-specific now, esp. since it creates it for offscreen compositors only which is otherwise not a thing in the browser. The easy fix would be to explicitly let the caller request a worker context (or not). But then the other thing is the notion of 'shared' here and caching it in this instance. Do you think the caller could do that instead?
https://codereview.chromium.org/2190033002/diff/120001/content/browser/render... File content/browser/renderer_host/context_provider_factory_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/120001/content/browser/render... content/browser/renderer_host/context_provider_factory_impl_android.cc:151: SharedCompositorWorkerContextProvider(gpu_channel_host); On 2016/08/03 23:13:25, sievers wrote: > Now that I think about it: If the browser compositor were to raster, then > SharedCompositorWorkerContextProvider() should not really be shared with a > different compositor (Blimp). So this is really exposing something > Blimp-specific now, esp. since it creates it for offscreen compositors only > which is otherwise not a thing in the browser. > > The easy fix would be to explicitly let the caller request a worker context (or > not). But then the other thing is the notion of 'shared' here and caching it in > this instance. > Do you think the caller could do that instead? I'll have to pass it back to this class for building the compositor context, and then static cast to the ContextProviderCommandBuffer. Ugly! :P I do think the impl class should be the one caching the shared context for the browser compositors at least. On that note, should we add the browser compositor stuff to the ContextProviderFactory API right now? Finally the request from the browser compositor would be to build the OutputSurface, and all the display bits would be stored by this class, something similar to, https://cs.chromium.org/chromium/src/content/browser/compositor/gpu_process_t... So ContextProviderFactory API should just become a utility for creating offscreen contexts, with methods like CreateWorkerContext and CreateCompositorContext. When browser compositor stuff gets added to it, it would be more along the lines of CreateDisplayOutputSurface. For now, the onscreen context request can be on the impl and the browser compositor can just use that. WDYT?
Moved all the worker context stuff out. When we do it for the browser compositor, PTAL. https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... File content/browser/renderer_host/context_provider_factory_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... content/browser/renderer_host/context_provider_factory_impl_android.cc:125: HandlePendingRequests(); On 2016/08/03 23:13:25, sievers wrote: > On 2016/08/02 23:31:53, Khushal wrote: > > On 2016/08/02 22:47:36, sievers wrote: > > > what's this for? > > > > In case any new requests came in while we were looping over the previous list > > above. > > Oh triggered from the callback? Good point. Yup. https://codereview.chromium.org/2190033002/diff/120001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/120001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:648: DCHECK(!context_providers.compositor_context_provider); On 2016/08/03 23:13:25, sievers wrote: > can you put a comment saying that 'if we don't have a surface (invisible), we > should not have created a context.' Done. https://codereview.chromium.org/2190033002/diff/120001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:659: pending_swapbuffers_ = 0; On 2016/08/03 23:13:25, sievers wrote: > the vulkan path should reset |pending_swapbuffers_| also. > move it into InitializeDisplay() maybe? Done. https://codereview.chromium.org/2190033002/diff/120001/ui/android/context_pro... File ui/android/context_provider_factory.cc (right): https://codereview.chromium.org/2190033002/diff/120001/ui/android/context_pro... ui/android/context_provider_factory.cc:11: ContextProviderFactory* g_context_provider_factory = nullptr; On 2016/08/03 21:48:13, David Trainor wrote: > anonymous namespace? Done. https://codereview.chromium.org/2190033002/diff/120001/ui/android/ui_android.gyp File ui/android/ui_android.gyp (right): https://codereview.chromium.org/2190033002/diff/120001/ui/android/ui_android.... ui/android/ui_android.gyp:27: 'context_provider_factory.cc', On 2016/08/03 21:48:13, David Trainor wrote: > diez! Done.
On 2016/08/04 18:55:15, Khushal wrote: > Moved all the worker context stuff out. When we do it for the browser > compositor, PTAL. Sorry, I meant when we do it for the browser compositor, we can have it in the impl and change CreateDisplayContextProvider to CreateDisplayOutputSurface, which will then go to the ContextProviderFactory interface. > > https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... > File content/browser/renderer_host/context_provider_factory_impl_android.cc > (right): > > https://codereview.chromium.org/2190033002/diff/100001/content/browser/render... > content/browser/renderer_host/context_provider_factory_impl_android.cc:125: > HandlePendingRequests(); > On 2016/08/03 23:13:25, sievers wrote: > > On 2016/08/02 23:31:53, Khushal wrote: > > > On 2016/08/02 22:47:36, sievers wrote: > > > > what's this for? > > > > > > In case any new requests came in while we were looping over the previous > list > > > above. > > > > Oh triggered from the callback? Good point. > > Yup. > > https://codereview.chromium.org/2190033002/diff/120001/content/browser/render... > File content/browser/renderer_host/compositor_impl_android.cc (right): > > https://codereview.chromium.org/2190033002/diff/120001/content/browser/render... > content/browser/renderer_host/compositor_impl_android.cc:648: > DCHECK(!context_providers.compositor_context_provider); > On 2016/08/03 23:13:25, sievers wrote: > > can you put a comment saying that 'if we don't have a surface (invisible), we > > should not have created a context.' > > Done. > > https://codereview.chromium.org/2190033002/diff/120001/content/browser/render... > content/browser/renderer_host/compositor_impl_android.cc:659: > pending_swapbuffers_ = 0; > On 2016/08/03 23:13:25, sievers wrote: > > the vulkan path should reset |pending_swapbuffers_| also. > > move it into InitializeDisplay() maybe? > > Done. > > https://codereview.chromium.org/2190033002/diff/120001/ui/android/context_pro... > File ui/android/context_provider_factory.cc (right): > > https://codereview.chromium.org/2190033002/diff/120001/ui/android/context_pro... > ui/android/context_provider_factory.cc:11: ContextProviderFactory* > g_context_provider_factory = nullptr; > On 2016/08/03 21:48:13, David Trainor wrote: > > anonymous namespace? > > Done. > > https://codereview.chromium.org/2190033002/diff/120001/ui/android/ui_android.gyp > File ui/android/ui_android.gyp (right): > > https://codereview.chromium.org/2190033002/diff/120001/ui/android/ui_android.... > ui/android/ui_android.gyp:27: 'context_provider_factory.cc', > On 2016/08/03 21:48:13, David Trainor wrote: > > diez! > > Done.
lgtm https://codereview.chromium.org/2190033002/diff/140001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/140001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:58: #include "content/common/gpu_process_launch_causes.h" nit: launch causes not used here anymore
https://codereview.chromium.org/2190033002/diff/140001/content/browser/render... File content/browser/renderer_host/context_provider_factory_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/140001/content/browser/render... content/browser/renderer_host/context_provider_factory_impl_android.cc:175: ? command_buffer_metrics::DISPLAY_COMPOSITOR_ONSCREEN_CONTEXT Btw, is this okay? Or do we need something else for the command buffer metrics for offscreen contexts here?
https://codereview.chromium.org/2190033002/diff/140001/content/browser/render... File content/browser/renderer_host/context_provider_factory_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/140001/content/browser/render... content/browser/renderer_host/context_provider_factory_impl_android.cc:175: ? command_buffer_metrics::DISPLAY_COMPOSITOR_ONSCREEN_CONTEXT On 2016/08/04 20:49:33, Khushal wrote: > Btw, is this okay? Or do we need something else for the command buffer metrics > for offscreen contexts here? You can add a type if you don't want it tracked as RENDER_COMPOSITOR_CONTEXT for blimp. It might be useful for context-lost stats. But then you should pass it in from blimp to override it. You also have to add it to histograms.xml (see histogram_suffixes name="ContextType"). But looks like that list already doesn't match the order in command_buffer_metrics.h anymore.
khushalsagar@chromium.org changed reviewers: + isherman@chromium.org
+isherman for histograms stamp. https://codereview.chromium.org/2190033002/diff/140001/content/browser/render... File content/browser/renderer_host/context_provider_factory_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/140001/content/browser/render... content/browser/renderer_host/context_provider_factory_impl_android.cc:175: ? command_buffer_metrics::DISPLAY_COMPOSITOR_ONSCREEN_CONTEXT On 2016/08/04 21:02:34, sievers wrote: > On 2016/08/04 20:49:33, Khushal wrote: > > Btw, is this okay? Or do we need something else for the command buffer metrics > > for offscreen contexts here? > > You can add a type if you don't want it tracked as RENDER_COMPOSITOR_CONTEXT for > blimp. > It might be useful for context-lost stats. > But then you should pass it in from blimp to override it. > > You also have to add it to histograms.xml (see histogram_suffixes > name="ContextType"). > But looks like that list already doesn't match the order in > command_buffer_metrics.h anymore. Thanks, added another type. I figured it shouldn't affect the existing stats for the render compositor contexts as well.
histograms.xml lgtm
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, dtrainor@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2190033002/#ps160001 (title: "..")
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
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, dtrainor@chromium.org, isherman@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2190033002/#ps180001 (title: "..")
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
Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, dtrainor@chromium.org, isherman@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2190033002/#ps200001 (title: "copy ctor.")
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 khushalsagar@chromium.org
https://codereview.chromium.org/2190033002/diff/200001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2190033002/diff/200001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:657: HandlePendingOutputSurfaceRequest(); I was thinking about the case where we go visible, invisible and then visible again, before the Gpu Channel is initialized, and this callback runs. So we would have 2 queued callbacks when we become visible again. When the first callback runs, we would queue another callback here and that's not good. Its better to have the factory drop the request if the widget goes away. Could you take another look?
Moved to using the surface handle instead of the widget in the factory as well.
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, dtrainor@chromium.org, isherman@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2190033002/#ps240001 (title: "use the surface handle, not widget.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/05 20:33:00, Khushal wrote: > Moved to using the surface handle instead of the widget in the factory as well. lgtm, thanks.
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by khushalsagar@chromium.org
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
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...)
The CQ bit was checked by khushalsagar@chromium.org
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
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...)
Description was changed from ========== content: Add ContextProviderFactory to create a render ContextProvider. Add a ContextProviderFactory to ui to create a ContextProvider for an external compositor rendering web content. This will be used by the Blimp Compositor. Also, 1) Consolidate the GpuChannel initialization logic and ownership of the VulkanContextProvider for the UI compositor in ContextProviderFactoryImpl. 2) Move cc::SurfaceManager and surface_ids allocation to ContextProviderFactory. BUG=624830 ========== to ========== content: Add ContextProviderFactory to create a render ContextProvider. Add a ContextProviderFactory to ui to create a ContextProvider for an external compositor rendering web content. This will be used by the Blimp Compositor. Also, 1) Consolidate the GpuChannel initialization logic and ownership of the VulkanContextProvider for the UI compositor in ContextProviderFactoryImpl. 2) Move cc::SurfaceManager and surface_ids allocation to ContextProviderFactory. BUG=624830 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, dtrainor@chromium.org, isherman@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2190033002/#ps260001 (title: "add setup for tests.")
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
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by khushalsagar@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/08 21:37:32, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... The display gets re-initialized each time we create a new OutputSurface, while we explicitly destroy it only when the visibility changes and we ask the host to release the output surface. So the DCHECK there is not always true.
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, dtrainor@chromium.org, isherman@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2190033002/#ps280001 (title: "remove display_ DCHECK.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== content: Add ContextProviderFactory to create a render ContextProvider. Add a ContextProviderFactory to ui to create a ContextProvider for an external compositor rendering web content. This will be used by the Blimp Compositor. Also, 1) Consolidate the GpuChannel initialization logic and ownership of the VulkanContextProvider for the UI compositor in ContextProviderFactoryImpl. 2) Move cc::SurfaceManager and surface_ids allocation to ContextProviderFactory. BUG=624830 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== content: Add ContextProviderFactory to create a render ContextProvider. Add a ContextProviderFactory to ui to create a ContextProvider for an external compositor rendering web content. This will be used by the Blimp Compositor. Also, 1) Consolidate the GpuChannel initialization logic and ownership of the VulkanContextProvider for the UI compositor in ContextProviderFactoryImpl. 2) Move cc::SurfaceManager and surface_ids allocation to ContextProviderFactory. BUG=624830 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/c81e43b82548d619a0b8ebd4dfd2362a8bbf5e2a Cr-Commit-Position: refs/heads/master@{#410522} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/c81e43b82548d619a0b8ebd4dfd2362a8bbf5e2a Cr-Commit-Position: refs/heads/master@{#410522} |
