Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
302603004:
Plumb GpuMemoryBuffer allocation to GPU process. (Closed)
|
Created:
6 years, 7 months ago by alexst (slow to review) Modified:
6 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionPlumb GpuMemoryBuffer allocation to GPU process.
BUG=368716
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276738
Patch Set 1 : #Patch Set 2 : #
Total comments: 10
Patch Set 3 : WIP #
Total comments: 2
Patch Set 4 : #
Total comments: 4
Patch Set 5 : Comments #
Total comments: 5
Patch Set 6 : rebase #Patch Set 7 : Comments #
Total comments: 2
Patch Set 8 : Changed to size #
Total comments: 2
Patch Set 9 : #
Total comments: 5
Patch Set 10 : added a map #
Total comments: 7
Patch Set 11 : removed static #
Total comments: 6
Patch Set 12 : nits #
Total comments: 8
Patch Set 13 : nits #Patch Set 14 : Fix Complex constructor has an inlined body clang error #Patch Set 15 : fix shared library build #Patch Set 16 : win build #Messages
Total messages: 47 (0 generated)
This is very rough still. Uploading a draft for early feedback to make sure I am not going off on a tangent. I am hoping we can maintain a common code path into GpuChannelManager::OnAllocateGpuMemoryBuffer, and do platform specific bits there with Ozone and SurfaceTexture or other. We should be able to plumb all the stuff we need from the browser via GpuMemoryBufferParams
https://codereview.chromium.org/302603004/diff/40001/content/browser/gpu/brow... File content/browser/gpu/browser_gpu_channel_host_factory.h (right): https://codereview.chromium.org/302603004/diff/40001/content/browser/gpu/brow... content/browser/gpu/browser_gpu_channel_host_factory.h:44: virtual void AllocateGpuMemoryBufferAsync( I'd like to have this replace Create/DeleteImage. I think that naming this Create/DestroyGpuMemoryBuffer will represent what it does really well. See my comment below for more details. https://codereview.chromium.org/302603004/diff/40001/content/browser/gpu/brow... content/browser/gpu/browser_gpu_channel_host_factory.h:45: const gfx::GpuMemoryBufferParams& params, The idea I had for this IPC was that it would take a gfx::GpuMemoryBufferHandle and the usual parameters (width, height..). The handle is created by the browser process and might not be more than a unique id from a browser controlled namespace (See gfx::SurfaceTextureId). The browser is still in control over what type of buffer to allocate. The creation of that specific GpuMemoryBuffer type just happens to be done on the gpu process side. The existing CreateImage IPC is replaced by a new type of GpuMemoryBuffer where a gfx::GpuMemoryBufferHandle contains a valid gfx::PluginWindowHandle. Some types might allocate the actual storage when creating a GpuMemoryBuffer on GPU process side (ie. SurfaceTexture) others can create a GpuMemoryBuffer from an existing allocation (ie. gfx::PluginWindowHandle). Hence, the name Create/Destroy instead of Allocate. https://codereview.chromium.org/302603004/diff/40001/content/browser/renderer... File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/302603004/diff/40001/content/browser/renderer... content/browser/renderer_host/render_message_filter.cc:1344: return; This should be hidden in the specific GpuMemoryBufferImpl implementation that needs to create the buffer on the GPU process side. How about we define a separate interface for Create/DestroyGpuMemoryBuffer? The same instance that implements BrowserGpuChannelHostFactory can implement this interface on the browser process side. There wouldn't be anything browser process specific about this interface though. It would live in content/common but only implemented on the browser side. GpuMemoryBufferImpl implementations can use it for buffer allocation whenever the interface is implemented. https://codereview.chromium.org/302603004/diff/40001/ui/gfx/gpu_memory_buffer.h File ui/gfx/gpu_memory_buffer.h (right): https://codereview.chromium.org/302603004/diff/40001/ui/gfx/gpu_memory_buffer... ui/gfx/gpu_memory_buffer.h:38: struct GpuMemoryBufferParams { I don't think we need this. Some of the fields in this struct belong in GpuMemoryBufferHandle. We might want to rename and reuse the SurfaceTextureId struct for other types of buffers but I prefer if we do that as a follow up when switching our surface texture support over to gpu process creation of buffers.
Thanks for clarification, responses inline. https://codereview.chromium.org/302603004/diff/40001/content/browser/gpu/brow... File content/browser/gpu/browser_gpu_channel_host_factory.h (right): https://codereview.chromium.org/302603004/diff/40001/content/browser/gpu/brow... content/browser/gpu/browser_gpu_channel_host_factory.h:44: virtual void AllocateGpuMemoryBufferAsync( On 2014/05/27 22:46:01, reveman wrote: > I'd like to have this replace Create/DeleteImage. > > I think that naming this Create/DestroyGpuMemoryBuffer will represent what it > does really well. See my comment below for more details. I was planning to add the new way first and then remove the old code as a followup to not break existing functionality since I'm thinking it may take a couple of CL to get everything done. That said, if you are okay with CreateImage temporarily not work, I'll go down that way. https://codereview.chromium.org/302603004/diff/40001/content/browser/gpu/brow... content/browser/gpu/browser_gpu_channel_host_factory.h:45: const gfx::GpuMemoryBufferParams& params, On 2014/05/27 22:46:01, reveman wrote: > The idea I had for this IPC was that it would take a gfx::GpuMemoryBufferHandle > and the usual parameters (width, height..). The handle is created by the browser > process and might not be more than a unique id from a browser controlled > namespace (See gfx::SurfaceTextureId). > > The browser is still in control over what type of buffer to allocate. The > creation of that specific GpuMemoryBuffer type just happens to be done on the > gpu process side. > > The existing CreateImage IPC is replaced by a new type of GpuMemoryBuffer where > a gfx::GpuMemoryBufferHandle contains a valid gfx::PluginWindowHandle. > > Some types might allocate the actual storage when creating a GpuMemoryBuffer on > GPU process side (ie. SurfaceTexture) others can create a GpuMemoryBuffer from > an existing allocation (ie. gfx::PluginWindowHandle). Hence, the name > Create/Destroy instead of Allocate. Got it. https://codereview.chromium.org/302603004/diff/40001/content/browser/renderer... File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/302603004/diff/40001/content/browser/renderer... content/browser/renderer_host/render_message_filter.cc:1344: return; On 2014/05/27 22:46:01, reveman wrote: > This should be hidden in the specific GpuMemoryBufferImpl implementation that > needs to create the buffer on the GPU process side. > > How about we define a separate interface for Create/DestroyGpuMemoryBuffer? The > same instance that implements BrowserGpuChannelHostFactory can implement this > interface on the browser process side. There wouldn't be anything browser > process specific about this interface though. It would live in content/common > but only implemented on the browser side. > > GpuMemoryBufferImpl implementations can use it for buffer allocation whenever > the interface is implemented. This was just a bit of stub code to make sure everything compiled. Sounds good on the new interface. I'll get to coding. https://codereview.chromium.org/302603004/diff/40001/ui/gfx/gpu_memory_buffer.h File ui/gfx/gpu_memory_buffer.h (right): https://codereview.chromium.org/302603004/diff/40001/ui/gfx/gpu_memory_buffer... ui/gfx/gpu_memory_buffer.h:38: struct GpuMemoryBufferParams { On 2014/05/27 22:46:01, reveman wrote: > I don't think we need this. Some of the fields in this struct belong in > GpuMemoryBufferHandle. > > We might want to rename and reuse the SurfaceTextureId struct for other types of > buffers but I prefer if we do that as a follow up when switching our surface > texture support over to gpu process creation of buffers. I'll move the relevant fields into GpuMemoryBufferHandle, but mostly I added this struct because PosTask ran out of variables once yuo add width, height, etc.
https://codereview.chromium.org/302603004/diff/40001/content/browser/gpu/brow... File content/browser/gpu/browser_gpu_channel_host_factory.h (right): https://codereview.chromium.org/302603004/diff/40001/content/browser/gpu/brow... content/browser/gpu/browser_gpu_channel_host_factory.h:44: virtual void AllocateGpuMemoryBufferAsync( On 2014/05/28 14:18:57, alexst wrote: > On 2014/05/27 22:46:01, reveman wrote: > > I'd like to have this replace Create/DeleteImage. > > > > I think that naming this Create/DestroyGpuMemoryBuffer will represent what it > > does really well. See my comment below for more details. > > I was planning to add the new way first and then remove the old code as a > followup to not break existing functionality since I'm thinking it may take a > couple of CL to get everything done. > > That said, if you are okay with CreateImage temporarily not work, I'll go down > that way. Removing Create/DeleteImage in follow up patches is fine and sounds like a good plan. We don't have to worry much about temporarily breaking it but I think we should avoid doing that intentionally. https://codereview.chromium.org/302603004/diff/40001/ui/gfx/gpu_memory_buffer.h File ui/gfx/gpu_memory_buffer.h (right): https://codereview.chromium.org/302603004/diff/40001/ui/gfx/gpu_memory_buffer... ui/gfx/gpu_memory_buffer.h:38: struct GpuMemoryBufferParams { On 2014/05/28 14:18:57, alexst wrote: > On 2014/05/27 22:46:01, reveman wrote: > > I don't think we need this. Some of the fields in this struct belong in > > GpuMemoryBufferHandle. > > > > We might want to rename and reuse the SurfaceTextureId struct for other types > of > > buffers but I prefer if we do that as a follow up when switching our surface > > texture support over to gpu process creation of buffers. > > I'll move the relevant fields into GpuMemoryBufferHandle, but mostly I added > this struct because PosTask ran out of variables once yuo add width, height, > etc. Ok, hopefully not needed in the final patch but in case it is, we should try to handle it locally instead of adding something to this file.
Okay, round 2 for more feedback. I'd like to break things up into a couple of patches, do the plumbing here first and then add the creation and tracking bits for the gpu process as a followup. https://codereview.chromium.org/302603004/diff/120001/ui/gfx/gpu_memory_buffer.h File ui/gfx/gpu_memory_buffer.h (right): https://codereview.chromium.org/302603004/diff/120001/ui/gfx/gpu_memory_buffe... ui/gfx/gpu_memory_buffer.h:33: base::ProcessId secondary_id; I'm thinking we can utilize this generically and with current API for AllocateForChildProcess, we can obtain pid from process_handle. GPU channel doesn't deal with process handles, and on windows the handle means more than just pid and requires opening and closing the it, so I think pid is a better choice here. Or we could plumb client_id like you did on android into AllocateForChildProcess.
https://codereview.chromium.org/302603004/diff/120001/ui/gfx/gpu_memory_buffer.h File ui/gfx/gpu_memory_buffer.h (right): https://codereview.chromium.org/302603004/diff/120001/ui/gfx/gpu_memory_buffe... ui/gfx/gpu_memory_buffer.h:33: base::ProcessId secondary_id; On 2014/05/30 22:41:47, alexst wrote: > I'm thinking we can utilize this generically and with current API for > AllocateForChildProcess, we can obtain pid from process_handle. > > GPU channel doesn't deal with process handles, and on windows the handle means > more than just pid and requires opening and closing the it, so I think pid is a > better choice here. Or we could plumb client_id like you did on android into > AllocateForChildProcess. Let's keep this a client id. The client id is controlled by chrome and we know it's unlikely to wrap around and cause a security issue. If it's a ProcessId, then we have to worry about an old id being reused by the system for a new renderer process. We'll end up sharing this when surface texture creation switches over to this new API. You can keep SurfaceTextureId and GpuMemoryBufferId separate for now and SurfaceTextureId will simply be eliminated as part of the switch to the new API. https://codereview.chromium.org/302603004/diff/140001/ui/gfx/gpu_memory_buffer.h File ui/gfx/gpu_memory_buffer.h (right): https://codereview.chromium.org/302603004/diff/140001/ui/gfx/gpu_memory_buffe... ui/gfx/gpu_memory_buffer.h:54: gfx::PluginWindowHandle window; You don't seem to be using this yet. How about we save this for a follow up patch? https://codereview.chromium.org/302603004/diff/140001/ui/gfx/gpu_memory_buffe... ui/gfx/gpu_memory_buffer.h:61: GpuMemoryBufferId gpu_memory_id; gpu_memory_id looks like a lazy typed version of gpu_memory_buffer_id. should it be gpu_memory_buffer_id instead or is something like "global_id" better? or maybe just "id"? https://codereview.chromium.org/302603004/diff/140001/ui/gfx/gpu_memory_buffe... ui/gfx/gpu_memory_buffer.h:61: GpuMemoryBufferId gpu_memory_id; also, can you move this above the idefs?
ptal https://codereview.chromium.org/302603004/diff/140001/ui/gfx/gpu_memory_buffer.h File ui/gfx/gpu_memory_buffer.h (right): https://codereview.chromium.org/302603004/diff/140001/ui/gfx/gpu_memory_buffe... ui/gfx/gpu_memory_buffer.h:61: GpuMemoryBufferId gpu_memory_id; On 2014/05/31 00:34:02, reveman wrote: > gpu_memory_id looks like a lazy typed version of gpu_memory_buffer_id. should it > be gpu_memory_buffer_id instead or is something like "global_id" better? or > maybe just "id"? Sure, I like global_id.
https://codereview.chromium.org/302603004/diff/150001/content/common/gpu/clie... File content/common/gpu/client/gpu_channel_host.h (right): https://codereview.chromium.org/302603004/diff/150001/content/common/gpu/clie... content/common/gpu/client/gpu_channel_host.h:87: class CONTENT_EXPORT GpuChannelHostGpuBufferFactory { We need a better name for this and I think it's probably best that we move it to a separate file. We want to make it as easy as possible to move everything but the implementation of this interface out of content/ in the future. How about GpuMemoryBufferFactoryHost? https://codereview.chromium.org/302603004/diff/150001/content/common/gpu/clie... content/common/gpu/client/gpu_channel_host.h:92: virtual ~GpuChannelHostGpuBufferFactory() {} The dtor doesn't have to be part of the interface. Please make this protected as that prevents the client from being able to delete it and removes the need for CONTENT_EXPORT above. https://codereview.chromium.org/302603004/diff/150001/content/common/gpu/clie... content/common/gpu/client/gpu_channel_host.h:99: const gfx::GpuMemoryBufferHandle& handle, nit: maybe have the handle be first parameter. it's in some sense the most important. https://codereview.chromium.org/302603004/diff/150001/content/common/gpu/clie... content/common/gpu/client/gpu_channel_host.h:101: virtual void DeleteGpuMemoryBuffer(const gfx::GpuMemoryBufferHandle& handle, Hm, should this be "Delete" or "Destroy"? Not sure why I chose Delete in case of Create/DeleteImage but I feel like that was a mistake. Create/Destroy seem more appropriate. https://codereview.chromium.org/302603004/diff/150001/content/common/gpu/gpu_... File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/302603004/diff/150001/content/common/gpu/gpu_... content/common/gpu/gpu_messages.h:285: // Allocate a buffer asynchronously in the GPU process. How about "Tells the GPU process to create a new gpu memory buffer for |handle|" as this will not allocate anything in the case when it's used as a replacement for CreateImage.
Addressed comments, and please excuse the slow turnaround time, I was on vacation all of last week.
https://codereview.chromium.org/302603004/diff/210001/content/common/gpu/clie... File content/common/gpu/client/gpu_memory_buffer_factory_host.h (right): https://codereview.chromium.org/302603004/diff/210001/content/common/gpu/clie... content/common/gpu/client/gpu_memory_buffer_factory_host.h:14: class GpuMemoryBufferFactoryHost { We'll need static Set/GetInstance functions for GpuMemoryBufferImpls to access this but you can leave that to a follow up patch if you like. https://codereview.chromium.org/302603004/diff/210001/content/common/gpu/gpu_... File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/302603004/diff/210001/content/common/gpu/gpu_... content/common/gpu/gpu_messages.h:289: size_t, /* height */ I think this should either be gfx::Size or uint32 to be consistent with ChildProcessHostMsg_SyncAllocateGpuMemoryBuffer. If uint32, then we should probably avoid to temporarily convert it to gfx::Size before passing it to GpuMemoryBufferImpls.
> We'll need static Set/GetInstance functions for GpuMemoryBufferImpls to access > this but you can leave that to a follow up patch if you like. I'll follow that up. > I think this should either be gfx::Size or uint32 to be consistent with > ChildProcessHostMsg_SyncAllocateGpuMemoryBuffer. If uint32, then we should > probably avoid to temporarily convert it to gfx::Size before passing it to > GpuMemoryBufferImpls. I would prefer gfx::Size now that you mention it. There is a mix of size_t and uint32 in various places, which got copy pasted here, but once it turns into gfx::Size in GpuMemoryBufferImpls, it should stay that way throughout. And new patch.
lgtm with a question/possible change and small nit https://codereview.chromium.org/302603004/diff/230001/content/common/gpu/clie... File content/common/gpu/client/gpu_memory_buffer_impl.h (right): https://codereview.chromium.org/302603004/diff/230001/content/common/gpu/clie... content/common/gpu/client/gpu_memory_buffer_impl.h:36: int child_id, This seems fine unless there's some existing mechanism for translating a child process handle into child id? You could also wait to add this until it's used. https://codereview.chromium.org/302603004/diff/230001/content/common/gpu/gpu_... File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/302603004/diff/230001/content/common/gpu/gpu_... content/common/gpu/gpu_messages.h:292: // Tells the GPU process to delete buffer. nit: to destroy buffer
> https://codereview.chromium.org/302603004/diff/230001/content/common/gpu/clie... > content/common/gpu/client/gpu_memory_buffer_impl.h:36: int child_id, > This seems fine unless there's some existing mechanism for translating a child > process handle into child id? You could also wait to add this until it's used. No way to translate handle to id, but there is a way to translate id to handle.
On 2014/06/10 01:30:58, alexst wrote: > > > https://codereview.chromium.org/302603004/diff/230001/content/common/gpu/clie... > > content/common/gpu/client/gpu_memory_buffer_impl.h:36: int child_id, > > This seems fine unless there's some existing mechanism for translating a child > > process handle into child id? You could also wait to add this until it's used. > > No way to translate handle to id, but there is a way to translate id to handle. I'll follow up with id->processhandle in the change that uses it. +piman +kenrb for IPC review.
https://codereview.chromium.org/302603004/diff/250001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_channel_host_factory.cc (right): https://codereview.chromium.org/302603004/diff/250001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.cc:487: callback)); This pattern, passing a callback to another thread, is dangerous in subtle ways. A callback refcounts its storage. Because of this, you can't guarantee which thread the storage gets destroyed, if you pass the callback to another thread. Since the callback comes from the user, you don't know if its storage might contain non-thread-safe references (e.g. a RefCounted class), in which case destroying the storage on another thread would cause bad things. A way around this is to keep the callback in a queue, that'd be only used from the UI thread. In OnGpuMemoryBufferCreated you'd pop the callback and run it (assuming in order callbacks, otherwise keeping an id and a hash_map would work). https://codereview.chromium.org/302603004/diff/250001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel_manager.cc (right): https://codereview.chromium.org/302603004/diff/250001/content/common/gpu/gpu_... content/common/gpu/gpu_channel_manager.cc:265: NOTIMPLEMENTED(); Should we send GpuHostMsg_GpuMemoryBufferCreated with a null handle to signal failure?
Added a map to account for failure conditions that happen out of order, please PTAL. https://codereview.chromium.org/302603004/diff/250001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_channel_host_factory.cc (right): https://codereview.chromium.org/302603004/diff/250001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.cc:487: callback)); On 2014/06/10 02:48:13, piman wrote: > This pattern, passing a callback to another thread, is dangerous in subtle ways. > > A callback refcounts its storage. Because of this, you can't guarantee which > thread the storage gets destroyed, if you pass the callback to another thread. > Since the callback comes from the user, you don't know if its storage might > contain non-thread-safe references (e.g. a RefCounted class), in which case > destroying the storage on another thread would cause bad things. > > A way around this is to keep the callback in a queue, that'd be only used from > the UI thread. In OnGpuMemoryBufferCreated you'd pop the callback and run it > (assuming in order callbacks, otherwise keeping an id and a hash_map would > work). That is very subtle... https://codereview.chromium.org/302603004/diff/250001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel_manager.cc (right): https://codereview.chromium.org/302603004/diff/250001/content/common/gpu/gpu_... content/common/gpu/gpu_channel_manager.cc:265: NOTIMPLEMENTED(); On 2014/06/10 02:48:13, piman wrote: > Should we send GpuHostMsg_GpuMemoryBufferCreated with a null handle to signal > failure? Done.
https://codereview.chromium.org/302603004/diff/270001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_channel_host_factory.cc (right): https://codereview.chromium.org/302603004/diff/270001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.cc:542: DCHECK(instance_); can you avoid the use of |instance_| here by making this and the above function non-static? https://codereview.chromium.org/302603004/diff/270001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_channel_host_factory.h (right): https://codereview.chromium.org/302603004/diff/270001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.h:122: int32 current_create_gpu_memory_buffer_request_id_; nit: prefer next_create_gpu_memory_buffer_request_id_ if you keep this. https://codereview.chromium.org/302603004/diff/270001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.h:125: CreateGpuMemoryBufferCallbackMap create_gpu_memory_buffer_requests_; This can only be used from one thread. Is the use of a map necessary? I would prefer if we used a queue consistent with what we're doing in GpuProcessHost.
https://codereview.chromium.org/302603004/diff/270001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_channel_host_factory.cc (right): https://codereview.chromium.org/302603004/diff/270001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.cc:542: DCHECK(instance_); On 2014/06/10 13:39:00, reveman (ooo until jun 11) wrote: > can you avoid the use of |instance_| here by making this and the above function > non-static? Done. https://codereview.chromium.org/302603004/diff/270001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_channel_host_factory.h (right): https://codereview.chromium.org/302603004/diff/270001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.h:122: int32 current_create_gpu_memory_buffer_request_id_; On 2014/06/10 13:39:00, reveman (ooo until jun 11) wrote: > nit: prefer next_create_gpu_memory_buffer_request_id_ if you keep this. Done. https://codereview.chromium.org/302603004/diff/270001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.h:125: CreateGpuMemoryBufferCallbackMap create_gpu_memory_buffer_requests_; On 2014/06/10 13:39:00, reveman (ooo until jun 11) wrote: > This can only be used from one thread. Is the use of a map necessary? I would > prefer if we used a queue consistent with what we're doing in GpuProcessHost. I added a map because CreateGpuMemoryBufferOnIO failing early would be potentially out of order.
ipc lgtm
https://codereview.chromium.org/302603004/diff/270001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_channel_host_factory.h (right): https://codereview.chromium.org/302603004/diff/270001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.h:125: CreateGpuMemoryBufferCallbackMap create_gpu_memory_buffer_requests_; On 2014/06/10 14:22:44, alexst wrote: > On 2014/06/10 13:39:00, reveman (ooo until jun 11) wrote: > > This can only be used from one thread. Is the use of a map necessary? I would > > prefer if we used a queue consistent with what we're doing in GpuProcessHost. > > I added a map because CreateGpuMemoryBufferOnIO failing early would be > potentially out of order. Is CreateGpuMemoryBufferOnIO failing while some IPC is in flight a possibility? A simple queue and DCHECK to ensure that it's not causing a problem would be preferred if possible IMO.
> > I added a map because CreateGpuMemoryBufferOnIO failing early would be > > potentially out of order. > > Is CreateGpuMemoryBufferOnIO failing while some IPC is in flight a possibility? > A simple queue and DCHECK to ensure that it's not causing a problem would be > preferred if possible IMO. I think it is possible if: we send an IPC, gpu crashes, new create request comes in before gpu restarts. Here is my train of thought. I prefer the queue myself, but even if we assume the condition above is not possible, we'd need to distinguish between a failed allocation from a missing host and one that came from the GPU in order to DCHECK on the right thread. If we failed due to a missing host, we have to DCHECK on UI that nothing is in the queue, if we failed in the GPU for whatever reason, we just pop and run the callback. So in order to get the simplicity of a queue, we'd add complexity elsewhere and at that point, a map feels cleaner to me.
lgtm https://codereview.chromium.org/302603004/diff/250001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_channel_host_factory.cc (right): https://codereview.chromium.org/302603004/diff/250001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.cc:487: callback)); On 2014/06/10 04:38:25, alexst wrote: > On 2014/06/10 02:48:13, piman wrote: > > This pattern, passing a callback to another thread, is dangerous in subtle > ways. > > > > A callback refcounts its storage. Because of this, you can't guarantee which > > thread the storage gets destroyed, if you pass the callback to another thread. > > Since the callback comes from the user, you don't know if its storage might > > contain non-thread-safe references (e.g. a RefCounted class), in which case > > destroying the storage on another thread would cause bad things. > > > > A way around this is to keep the callback in a queue, that'd be only used from > > the UI thread. In OnGpuMemoryBufferCreated you'd pop the callback and run it > > (assuming in order callbacks, otherwise keeping an id and a hash_map would > > work). > > That is very subtle... I know :(
https://codereview.chromium.org/302603004/diff/290001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_channel_host_factory.h (right): https://codereview.chromium.org/302603004/diff/290001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.h:122: int32 next_create_gpu_memory_buffer_request_id_; how about uint32 instead? in case this will ever wrap-around.
https://codereview.chromium.org/302603004/diff/290001/content/common/gpu/clie... File content/common/gpu/client/gpu_memory_buffer_factory_host.h (right): https://codereview.chromium.org/302603004/diff/290001/content/common/gpu/clie... content/common/gpu/client/gpu_memory_buffer_factory_host.h:7: include base/callback.h https://codereview.chromium.org/302603004/diff/290001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel_manager.cc (right): https://codereview.chromium.org/302603004/diff/290001/content/common/gpu/gpu_... content/common/gpu/gpu_channel_manager.cc:271: NOTIMPLEMENTED(); I don't think this should be here. You've implemented OnCreateGpuMemoryBuffer so it makes sense to also consider this implemented.
patch https://codereview.chromium.org/302603004/diff/290001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_channel_host_factory.h (right): https://codereview.chromium.org/302603004/diff/290001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.h:122: int32 next_create_gpu_memory_buffer_request_id_; On 2014/06/11 13:46:07, reveman wrote: > how about uint32 instead? in case this will ever wrap-around. Done. https://codereview.chromium.org/302603004/diff/290001/content/common/gpu/clie... File content/common/gpu/client/gpu_memory_buffer_factory_host.h (right): https://codereview.chromium.org/302603004/diff/290001/content/common/gpu/clie... content/common/gpu/client/gpu_memory_buffer_factory_host.h:7: On 2014/06/11 14:03:22, reveman wrote: > include base/callback.h Done. https://codereview.chromium.org/302603004/diff/290001/content/common/gpu/gpu_... File content/common/gpu/gpu_channel_manager.cc (right): https://codereview.chromium.org/302603004/diff/290001/content/common/gpu/gpu_... content/common/gpu/gpu_channel_manager.cc:271: NOTIMPLEMENTED(); On 2014/06/11 14:03:22, reveman wrote: > I don't think this should be here. You've implemented OnCreateGpuMemoryBuffer so > it makes sense to also consider this implemented. Done.
lgtm after addressing this last set of comments https://codereview.chromium.org/302603004/diff/310001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_channel_host_factory.cc (right): https://codereview.chromium.org/302603004/diff/310001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.cc:480: int32 request_id = next_create_gpu_memory_buffer_request_id_++; uint32 https://codereview.chromium.org/302603004/diff/310001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.cc:510: int32 request_id) { uint32 https://codereview.chromium.org/302603004/diff/310001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.cc:528: int32 request_id, uint32 https://codereview.chromium.org/302603004/diff/310001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.cc:540: int32 request_id, uint32 https://codereview.chromium.org/302603004/diff/310001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.cc:555: } nit: no need for "{" "}" (inconsistent with DeleteImageOnIO but think that's fine as we're going to delete that function soon) https://codereview.chromium.org/302603004/diff/310001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_channel_host_factory.h (right): https://codereview.chromium.org/302603004/diff/310001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.h:123: typedef std::map<int32, CreateGpuMemoryBufferCallback> uint32 here too
https://codereview.chromium.org/302603004/diff/310001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_channel_host_factory.cc (right): https://codereview.chromium.org/302603004/diff/310001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.cc:540: int32 request_id, On 2014/06/11 14:52:09, reveman wrote: > uint32 D'oh!@ thank you. https://codereview.chromium.org/302603004/diff/310001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_channel_host_factory.h (right): https://codereview.chromium.org/302603004/diff/310001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.h:123: typedef std::map<int32, CreateGpuMemoryBufferCallback> On 2014/06/11 14:52:09, reveman wrote: > uint32 here too Done.
The CQ bit was checked by alexst@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexst@chromium.org/302603004/330001
The CQ bit was checked by alexst@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexst@chromium.org/302603004/350001
The CQ bit was checked by alexst@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexst@chromium.org/302603004/370001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was checked by alexst@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexst@chromium.org/302603004/370001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
The CQ bit was checked by alexst@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexst@chromium.org/302603004/370001
The CQ bit was checked by alexst@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexst@chromium.org/302603004/390001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
Message was sent while issue was closed.
Change committed as 276738 |
