Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2941933002:
viz: Convert a sync api in ServerGpuMemoryBufferManager into async. (Closed)
|
Created:
3 years, 6 months ago by sadrul Modified:
3 years, 6 months ago CC:
Aaron Boodman, abarth-chromium, cc-bugs_chromium.org, chromium-reviews, danakj+watch_chromium.org, darin (slow to review), piman+watch_chromium.org, qsr+mojo_chromium.org, rjkroege, viettrungluu+watch_chromium.org, vmpstr+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionviz: Convert a sync api in ServerGpuMemoryBufferManager into async.
Convert ServerGpuMemoryBufferManager::CreateGpuMemoryBufferHandle() into
an async api. This is in preparation for using this implementation from
content.
BUG=733482
TBR=jbauman@ for gpu_memory_buffer_manager.h doc update
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2941933002
Cr-Commit-Position: refs/heads/master@{#481517}
Committed: https://chromium.googlesource.com/chromium/src/+/fbccda44a34bf3ddaf0aa3119afbba21fa9ab5a9
Patch Set 1 #Patch Set 2 : tot-merge #
Total comments: 15
Patch Set 3 : . #
Total comments: 8
Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : . #
Total comments: 6
Patch Set 7 : . #
Total comments: 20
Patch Set 8 : . #Patch Set 9 : . #Patch Set 10 : . #
Total comments: 10
Patch Set 11 : . #Patch Set 12 : . #
Total comments: 17
Patch Set 13 : . #Patch Set 14 : fix test build on windows #Messages
Total messages: 81 (56 generated)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Description was changed from ========== viz: Convert a sync api in ServerGpuMemoryBufferManager into async. Convert ServerGpuMemoryBufferManager::CreateGpuMemoryBufferHandle() into an async api. This is in preparation for using this implementation from content. BUG=... ========== to ========== viz: Convert a sync api in ServerGpuMemoryBufferManager into async. Convert ServerGpuMemoryBufferManager::CreateGpuMemoryBufferHandle() into an async api. This is in preparation for using this implementation from content. BUG=733482 ==========
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sadrul@chromium.org changed reviewers: + reveman@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... File components/viz/common/server_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.cc:51: if (!gpu::GpuMemoryBufferImplSharedMemory::IsUsageSupported(usage) || Why can't we keep the DCHECK here now? https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.cc:79: auto alloc_callback = nit: s/alloc/allocate/ https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.cc:83: if (task_runner_->BelongsToCurrentThread()) Would it be better to always have this done by PostTask to avoid having a different call stack depending on the thread it's called from? https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.cc:127: if (!pending_buffers_.count(client_id)) { nit: use find instead https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.cc:128: // The client has been destroyed since the memory request was made. s/memory request/allocation request/ ? https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... File components/viz/common/server_gpu_memory_buffer_manager.h (right): https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.h:32: void CreateGpuMemoryBufferHandle( How about renaming this to AllocateGpuMemoryBuffer? As creating a "handle" in some sense doesn't mean that we allocate something that can be used to create a buffer.. Also makes the use of the term "allocate" in the current patch more consistent as otherwise I'd recommend changing the use of that term in this patch to "create" everywhere. https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.h:55: void OnGpuMemoryBufferCreatedWait( WaitOnGpuMemoryBufferCreated?
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... File components/viz/common/server_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.cc:51: if (!gpu::GpuMemoryBufferImplSharedMemory::IsUsageSupported(usage) || On 2017/06/15 15:17:43, reveman wrote: > Why can't we keep the DCHECK here now? I get crashes (with usage == SCANOUT) when running a chromeos build on linux desktop with ozone-x11 backend. (which uses https://cs.chromium.org/chromium/src/ui/ozone/common/stub_client_native_pixma...) https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.cc:79: auto alloc_callback = On 2017/06/15 15:17:43, reveman wrote: > nit: s/alloc/allocate/ Done. https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.cc:83: if (task_runner_->BelongsToCurrentThread()) On 2017/06/15 15:17:43, reveman wrote: > Would it be better to always have this done by PostTask to avoid having a > different call stack depending on the thread it's called from? I have made the change to always PostTask(). The reason I ran immediately in this case was for potential latency improvements. But I am not sure if that can actually be an issue? https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.cc:127: if (!pending_buffers_.count(client_id)) { On 2017/06/15 15:17:43, reveman wrote: > nit: use find instead Done. https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.cc:128: // The client has been destroyed since the memory request was made. On 2017/06/15 15:17:43, reveman wrote: > s/memory request/allocation request/ ? Done. https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... File components/viz/common/server_gpu_memory_buffer_manager.h (right): https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.h:32: void CreateGpuMemoryBufferHandle( On 2017/06/15 15:17:43, reveman wrote: > How about renaming this to AllocateGpuMemoryBuffer? As creating a "handle" in > some sense doesn't mean that we allocate something that can be used to create a > buffer.. Also makes the use of the term "allocate" in the current patch more > consistent as otherwise I'd recommend changing the use of that term in this > patch to "create" everywhere. Changed to Allocate* https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.h:55: void OnGpuMemoryBufferCreatedWait( On 2017/06/15 15:17:43, reveman wrote: > WaitOnGpuMemoryBufferCreated? Renamed this to SignalOnGMBAllocated, since this signals the WaitableEvent, instead of waiting on it.
lgtm with nits https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... File components/viz/common/server_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.cc:51: if (!gpu::GpuMemoryBufferImplSharedMemory::IsUsageSupported(usage) || On 2017/06/15 at 16:58:05, sadrul wrote: > On 2017/06/15 15:17:43, reveman wrote: > > Why can't we keep the DCHECK here now? > > I get crashes (with usage == SCANOUT) when running a chromeos build on linux desktop with ozone-x11 backend. (which uses https://cs.chromium.org/chromium/src/ui/ozone/common/stub_client_native_pixma...) Can we instead avoid trying to create scanout buffers in this case? I think that's preferred when we control the code and it's not exposed to the renderer and the web platform. https://codereview.chromium.org/2941933002/diff/40001/components/viz/common/s... File components/viz/common/server_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/2941933002/diff/40001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.cc:140: const gfx::GpuMemoryBufferHandle& allocated_handle) { nit: allocated_buffer_handle or just handle https://codereview.chromium.org/2941933002/diff/40001/components/viz/common/s... File components/viz/common/server_gpu_memory_buffer_manager.h (right): https://codereview.chromium.org/2941933002/diff/40001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.h:39: void AllocateGpuMemoryBufferHandle( nit: remove "Handle" suffix as we allocate a buffer not a handle, the handle is just how we transfer ownership of the allocated buffer.. https://codereview.chromium.org/2941933002/diff/40001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.h:62: void SignalOnGpuMemoryBufferAllocated( nit: maybe just SignalGpuMemoryBufferAllocated https://codereview.chromium.org/2941933002/diff/40001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.h:65: const gfx::GpuMemoryBufferHandle& allocated_handle); nit: allocated_buffer_handle or just handle
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sadrul@chromium.org changed reviewers: + danakj@chromium.org, kenrb@chromium.org
+kenrb@ for mojom security review. +danakj@ for thread_restrictions. (the new exception will replace the existing one for content::BrowserGpuMemoryBufferManager). https://codereview.chromium.org/2941933002/diff/40001/components/viz/common/s... File components/viz/common/server_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/2941933002/diff/40001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.cc:140: const gfx::GpuMemoryBufferHandle& allocated_handle) { On 2017/06/15 17:46:58, reveman wrote: > nit: allocated_buffer_handle or just handle Done (called allocated_buffer_handle) https://codereview.chromium.org/2941933002/diff/40001/components/viz/common/s... File components/viz/common/server_gpu_memory_buffer_manager.h (right): https://codereview.chromium.org/2941933002/diff/40001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.h:39: void AllocateGpuMemoryBufferHandle( On 2017/06/15 17:46:58, reveman wrote: > nit: remove "Handle" suffix as we allocate a buffer not a handle, the handle is > just how we transfer ownership of the allocated buffer.. Done. https://codereview.chromium.org/2941933002/diff/40001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.h:62: void SignalOnGpuMemoryBufferAllocated( On 2017/06/15 17:46:58, reveman wrote: > nit: maybe just SignalGpuMemoryBufferAllocated Done. https://codereview.chromium.org/2941933002/diff/40001/components/viz/common/s... components/viz/common/server_gpu_memory_buffer_manager.h:65: const gfx::GpuMemoryBufferHandle& allocated_handle); On 2017/06/15 17:46:58, reveman wrote: > nit: allocated_buffer_handle or just handle Done.
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2941933002/diff/100001/components/viz/common/... File components/viz/common/server_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/2941933002/diff/100001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:86: wait_event.Wait(); How is this not a deadlock?
https://codereview.chromium.org/2941933002/diff/100001/components/viz/common/... File components/viz/common/server_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/2941933002/diff/100001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:86: wait_event.Wait(); On 2017/06/15 21:58:57, danakj wrote: > How is this not a deadlock? |task_runner_| is not running in this thread. See DCHECK() above in line 76.
https://codereview.chromium.org/2941933002/diff/100001/components/viz/common/... File components/viz/common/server_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/2941933002/diff/100001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:86: wait_event.Wait(); On 2017/06/15 22:01:32, sadrul wrote: > On 2017/06/15 21:58:57, danakj wrote: > > How is this not a deadlock? > > |task_runner_| is not running in this thread. See DCHECK() above in line 76. What thread owns/creates this class? What threads call Create? Why is CreateGpuMemoryBuffer() calling AllocateGpuMemoryBuffer() but also the other way too, this looks loopy? https://codereview.chromium.org/2941933002/diff/100001/components/viz/common/... File components/viz/common/server_gpu_memory_buffer_manager.h (right): https://codereview.chromium.org/2941933002/diff/100001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.h:78: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; Can this be SequencedTaskRunner?
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
https://codereview.chromium.org/2941933002/diff/100001/components/viz/common/... File components/viz/common/server_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/2941933002/diff/100001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:86: wait_event.Wait(); On 2017/06/15 22:06:00, danakj wrote: > On 2017/06/15 22:01:32, sadrul wrote: > > On 2017/06/15 21:58:57, danakj wrote: > > > How is this not a deadlock? > > > > |task_runner_| is not running in this thread. See DCHECK() above in line 76. > > What thread owns/creates this class? What threads call Create? In mus-ws, this is created on the main-thread. It never calls into CreateGpuMemoryBuffer() though, it directly calls AllocateGpuMemoryBuffer() instead. In mus-viz, this is created on the compositor thread. I am trying to confirm where Create() is called from. In chrome browser, this will be created on the IO thread (or may be created in the UI thread, but the task_runner_ for the IO thread will be injected). And Create() is called from non-io threads [1, 2]. I have a pretty ugly/hacky follow up CL that uses this in chrome browser [3]. [1] https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_memory_b... [2] https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_memory_b... [3] https://codereview.chromium.org/2934203002/ > > Why is CreateGpuMemoryBuffer() calling AllocateGpuMemoryBuffer() but also the > other way too, this looks loopy? AllocateGpuMemoryBuffer() does not call back into CreateGpuMemoryBuffer() here. It can call into mojom::GpuService::CreateGpuMemoryBuffer(), or GpuMemoryBufferImplSharedMemory::CreateGpuMemoryBuffer(). https://codereview.chromium.org/2941933002/diff/100001/components/viz/common/... File components/viz/common/server_gpu_memory_buffer_manager.h (right): https://codereview.chromium.org/2941933002/diff/100001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.h:78: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; On 2017/06/15 22:06:01, danakj wrote: > Can this be SequencedTaskRunner? Done.
Dry run: 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
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... File components/viz/common/server_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:8: #include "base/threading/thread_task_runner_handle.h" sequenced https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:22: task_runner_(base::ThreadTaskRunnerHandle::Get()), sequenced? https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:41: gpu_service_->CreateGpuMemoryBuffer( I guess I'm a lil confused why we're going to use this for the viz process now. The idea was that this provides a mojo interface because we'll use it from compositor thread but the gpuservice is on main. Now this is dchecking that Allocate is only called on main, but still going through mojo which is just extra indirection to understand and overhead. If we didn't plan to use this in viz process would any of this simplify? https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:51: if (!gpu::GpuMemoryBufferImplSharedMemory::IsUsageSupported(usage) || we used to dcheck, is this now possible? https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:52: !gpu::GpuMemoryBufferImplSharedMemory::IsSizeValidForFormat(size, why did this appear here? https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:127: if (!handle.is_null()) {} https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... File components/viz/common/server_gpu_memory_buffer_manager.h (right): https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.h:12: #include "base/single_thread_task_runner.h" sequenced (and forward declare?) https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.h:49: std::unique_ptr<gfx::GpuMemoryBuffer> CreateGpuMemoryBuffer( Please document threading restrictions clearly here for this method. Also perhaps for all other methods in the class comment? Also how about adding OnFooThread suffixes to every method that isn't the interface? OnOriginThread for the thread this is created on? Alternatively.. we could push this impl out to a member nested class which calls back to this outer class...? would that make the threading model clear also?
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... File components/viz/common/server_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:8: #include "base/threading/thread_task_runner_handle.h" On 2017/06/15 23:02:42, danakj wrote: > sequenced Done. https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:22: task_runner_(base::ThreadTaskRunnerHandle::Get()), On 2017/06/15 23:02:42, danakj wrote: > sequenced? Done. https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:41: gpu_service_->CreateGpuMemoryBuffer( On 2017/06/15 23:02:42, danakj wrote: > I guess I'm a lil confused why we're going to use this for the viz process now. > The idea was that this provides a mojo interface because we'll use it from > compositor thread but the gpuservice is on main. > > Now this is dchecking that Allocate is only called on main, but still going > through mojo which is just extra indirection to understand and overhead. > > If we didn't plan to use this in viz process would any of this simplify? viz does not actually see ServerGpuMemoryBufferManager, so it does not directly call Allocate. It calls GpuMemoryBufferManager::CreateGpuMemoryBuffer(). ServerGpuMemoryBufferManager then takes care of doing the thread-hopping as needed. Not using this in viz would not simplify this. We would still have this complexity. (see corresponding code in BrowserGpuMemoryBufferManager) Specifically for viz, I am wondering if we should really just have an InProcessGpuMemoryBuffer, which directly talks to the GpuMemoryBufferFactory, instead. (that still would not simplify this ServerGMBM any further though) I am going to look into that. https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:52: !gpu::GpuMemoryBufferImplSharedMemory::IsSizeValidForFormat(size, On 2017/06/15 23:02:42, danakj wrote: > why did this appear here? See comments in https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... The corresponding code in browser is here [1]. Since this code is meant to replace that, I am starting with the same set of checks. I do plan to avoid attempting to create scanout buffers when appropriate, as reveman@ suggested, but I think that ought to happen separately. [1] https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_memory_b... https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:127: if (!handle.is_null()) On 2017/06/15 23:02:42, danakj wrote: > {} Done. https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... File components/viz/common/server_gpu_memory_buffer_manager.h (right): https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.h:49: std::unique_ptr<gfx::GpuMemoryBuffer> CreateGpuMemoryBuffer( On 2017/06/15 23:02:42, danakj wrote: > Please document threading restrictions clearly here for this method. Also > perhaps for all other methods in the class comment? Also how about adding > OnFooThread suffixes to every method that isn't the interface? OnOriginThread > for the thread this is created on? > > Alternatively.. we could push this impl out to a member nested class which calls > back to this outer class...? would that make the threading model clear also? I can comment on the interface [1] that this can be called from any thread. Other than that, the rest of the functions in this class are expected to be called on the same thread (see DCHECK()s). [1] https://cs.chromium.org/chromium/src/gpu/command_buffer/client/gpu_memory_buf...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... File components/viz/common/server_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:41: gpu_service_->CreateGpuMemoryBuffer( On 2017/06/16 12:52:19, sadrul wrote: > On 2017/06/15 23:02:42, danakj wrote: > > I guess I'm a lil confused why we're going to use this for the viz process > now. > > The idea was that this provides a mojo interface because we'll use it from > > compositor thread but the gpuservice is on main. > > > > Now this is dchecking that Allocate is only called on main, but still going > > through mojo which is just extra indirection to understand and overhead. > > > > If we didn't plan to use this in viz process would any of this simplify? > > viz does not actually see ServerGpuMemoryBufferManager, so it does not directly > call Allocate. It calls GpuMemoryBufferManager::CreateGpuMemoryBuffer(). > ServerGpuMemoryBufferManager then takes care of doing the thread-hopping as > needed. > > Not using this in viz would not simplify this. We would still have this > complexity. (see corresponding code in BrowserGpuMemoryBufferManager) > > Specifically for viz, I am wondering if we should really just have an > InProcessGpuMemoryBuffer, which directly talks to the GpuMemoryBufferFactory, > instead. (that still would not simplify this ServerGMBM any further though) I am > going to look into that. I have put up https://codereview.chromium.org/2942283002/, which introduces InProcessGpuMemoryBufferManager, which viz uses, instead of this ServerGpuMemoryBufferManager. https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:52: !gpu::GpuMemoryBufferImplSharedMemory::IsSizeValidForFormat(size, On 2017/06/16 12:52:19, sadrul wrote: > On 2017/06/15 23:02:42, danakj wrote: > > why did this appear here? > > See comments in > https://codereview.chromium.org/2941933002/diff/20001/components/viz/common/s... > > The corresponding code in browser is here [1]. Since this code is meant to > replace that, I am starting with the same set of checks. I do plan to avoid > attempting to create scanout buffers when appropriate, as reveman@ suggested, > but I think that ought to happen separately. > > [1] > https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_memory_b... I have removed this, and restored the DCHECK(). Turns out, I was not hitting the DCHECK with this CL, but the follow up CL. So this CL can land with the DCHECK() in place.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mojom lgtm
https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... File components/viz/common/server_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:41: gpu_service_->CreateGpuMemoryBuffer( On 2017/06/17 02:54:04, sadrul wrote: > On 2017/06/16 12:52:19, sadrul wrote: > > On 2017/06/15 23:02:42, danakj wrote: > > > I guess I'm a lil confused why we're going to use this for the viz process > > now. > > > The idea was that this provides a mojo interface because we'll use it from > > > compositor thread but the gpuservice is on main. > > > > > > Now this is dchecking that Allocate is only called on main, but still going > > > through mojo which is just extra indirection to understand and overhead. > > > > > > If we didn't plan to use this in viz process would any of this simplify? > > > > viz does not actually see ServerGpuMemoryBufferManager, so it does not > directly > > call Allocate. It calls GpuMemoryBufferManager::CreateGpuMemoryBuffer(). > > ServerGpuMemoryBufferManager then takes care of doing the thread-hopping as > > needed. > > > > Not using this in viz would not simplify this. We would still have this > > complexity. (see corresponding code in BrowserGpuMemoryBufferManager) > > > > Specifically for viz, I am wondering if we should really just have an > > InProcessGpuMemoryBuffer, which directly talks to the GpuMemoryBufferFactory, > > instead. (that still would not simplify this ServerGMBM any further though) I > am > > going to look into that. > > I have put up https://codereview.chromium.org/2942283002/, which introduces > InProcessGpuMemoryBufferManager, which viz uses, instead of this > ServerGpuMemoryBufferManager. After that, does this class still belong in viz/common/, or should it be in viz/host/? https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... File components/viz/common/server_gpu_memory_buffer_manager.h (right): https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.h:49: std::unique_ptr<gfx::GpuMemoryBuffer> CreateGpuMemoryBuffer( On 2017/06/16 12:52:19, sadrul wrote: > On 2017/06/15 23:02:42, danakj wrote: > > Please document threading restrictions clearly here for this method. Also > > perhaps for all other methods in the class comment? Also how about adding > > OnFooThread suffixes to every method that isn't the interface? OnOriginThread > > for the thread this is created on? > > > > Alternatively.. we could push this impl out to a member nested class which > calls > > back to this outer class...? would that make the threading model clear also? > > I can comment on the interface [1] that this can be called from any thread. > Other than that, the rest of the functions in this class are expected to be > called on the same thread (see DCHECK()s). > > [1] > https://cs.chromium.org/chromium/src/gpu/command_buffer/client/gpu_memory_buf... That'd be good, and a top level class comment explaining what you just said here also.
Description was changed from ========== viz: Convert a sync api in ServerGpuMemoryBufferManager into async. Convert ServerGpuMemoryBufferManager::CreateGpuMemoryBufferHandle() into an async api. This is in preparation for using this implementation from content. BUG=733482 ========== to ========== viz: Convert a sync api in ServerGpuMemoryBufferManager into async. Convert ServerGpuMemoryBufferManager::CreateGpuMemoryBufferHandle() into an async api. This is in preparation for using this implementation from content. BUG=733482 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... File components/viz/common/server_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:41: gpu_service_->CreateGpuMemoryBuffer( On 2017/06/20 16:27:26, danakj wrote: > On 2017/06/17 02:54:04, sadrul wrote: > > On 2017/06/16 12:52:19, sadrul wrote: > > > On 2017/06/15 23:02:42, danakj wrote: > > > > I guess I'm a lil confused why we're going to use this for the viz process > > > now. > > > > The idea was that this provides a mojo interface because we'll use it from > > > > compositor thread but the gpuservice is on main. > > > > > > > > Now this is dchecking that Allocate is only called on main, but still > going > > > > through mojo which is just extra indirection to understand and overhead. > > > > > > > > If we didn't plan to use this in viz process would any of this simplify? > > > > > > viz does not actually see ServerGpuMemoryBufferManager, so it does not > > directly > > > call Allocate. It calls GpuMemoryBufferManager::CreateGpuMemoryBuffer(). > > > ServerGpuMemoryBufferManager then takes care of doing the thread-hopping as > > > needed. > > > > > > Not using this in viz would not simplify this. We would still have this > > > complexity. (see corresponding code in BrowserGpuMemoryBufferManager) > > > > > > Specifically for viz, I am wondering if we should really just have an > > > InProcessGpuMemoryBuffer, which directly talks to the > GpuMemoryBufferFactory, > > > instead. (that still would not simplify this ServerGMBM any further though) > I > > am > > > going to look into that. > > > > I have put up https://codereview.chromium.org/2942283002/, which introduces > > InProcessGpuMemoryBufferManager, which viz uses, instead of this > > ServerGpuMemoryBufferManager. > > After that, does this class still belong in viz/common/, or should it be in > viz/host/? It should move to viz/host. I have a follow up CL that moves this: https://codereview.chromium.org/2941423002/ https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... File components/viz/common/server_gpu_memory_buffer_manager.h (right): https://codereview.chromium.org/2941933002/diff/120001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.h:49: std::unique_ptr<gfx::GpuMemoryBuffer> CreateGpuMemoryBuffer( On 2017/06/20 16:27:26, danakj wrote: > On 2017/06/16 12:52:19, sadrul wrote: > > On 2017/06/15 23:02:42, danakj wrote: > > > Please document threading restrictions clearly here for this method. Also > > > perhaps for all other methods in the class comment? Also how about adding > > > OnFooThread suffixes to every method that isn't the interface? > OnOriginThread > > > for the thread this is created on? > > > > > > Alternatively.. we could push this impl out to a member nested class which > > calls > > > back to this outer class...? would that make the threading model clear also? > > > > I can comment on the interface [1] that this can be called from any thread. > > Other than that, the rest of the functions in this class are expected to be > > called on the same thread (see DCHECK()s). > > > > [1] > > > https://cs.chromium.org/chromium/src/gpu/command_buffer/client/gpu_memory_buf... > > That'd be good, and a top level class comment explaining what you just said here > also. Done.
Ok thanks, I think this looks good w/ the following: https://codereview.chromium.org/2941933002/diff/180001/components/viz/common/... File components/viz/common/server_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/2941933002/diff/180001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:52: std::move(callback).Run( Should we post this instead of calling it directly? Its tricky when a callback is sometimes called inline and sometimes called on a new stack, the client has to be able to deal with both. https://codereview.chromium.org/2941933002/diff/180001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:68: // We block with a WaitableEvent until the callbacks are run. So using nit: put the comment immediately before the line it talks about https://codereview.chromium.org/2941933002/diff/180001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:72: &ServerGpuMemoryBufferManager::SignalGpuMemoryBufferAllocated, this could be a lambda, which might be easier to read/follow here, maybe give it a try? I don't see why it's a member function instead of a static one either way. https://codereview.chromium.org/2941933002/diff/180001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:100: DCHECK(task_runner_->RunsTasksOnCurrentThread()); For all these dchecks: // Drepecated: favor RunsTasksInCurrentSequence(). https://codereview.chromium.org/2941933002/diff/180001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:119: if (pending_buffers_.find(client_id) == pending_buffers_.end()) { Can we unit test this class?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2941933002/diff/180001/components/viz/common/... File components/viz/common/server_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/2941933002/diff/180001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:52: std::move(callback).Run( On 2017/06/20 18:54:54, danakj wrote: > Should we post this instead of calling it directly? Its tricky when a callback > is sometimes called inline and sometimes called on a new stack, the client has > to be able to deal with both. Done. https://codereview.chromium.org/2941933002/diff/180001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:68: // We block with a WaitableEvent until the callbacks are run. So using On 2017/06/20 18:54:54, danakj wrote: > nit: put the comment immediately before the line it talks about Done. https://codereview.chromium.org/2941933002/diff/180001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:72: &ServerGpuMemoryBufferManager::SignalGpuMemoryBufferAllocated, On 2017/06/20 18:54:54, danakj wrote: > this could be a lambda, which might be easier to read/follow here, maybe give it > a try? I don't see why it's a member function instead of a static one either > way. Agree. Used lambda. It's much cleaner. Thanks! https://codereview.chromium.org/2941933002/diff/180001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:100: DCHECK(task_runner_->RunsTasksOnCurrentThread()); On 2017/06/20 18:54:54, danakj wrote: > For all these dchecks: // Drepecated: favor RunsTasksInCurrentSequence(). Done. https://codereview.chromium.org/2941933002/diff/180001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager.cc:119: if (pending_buffers_.find(client_id) == pending_buffers_.end()) { On 2017/06/20 18:54:54, danakj wrote: > Can we unit test this class? Yep. Added a simple test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM thanks for the test! https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... File components/viz/common/DEPS (right): https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... components/viz/common/DEPS:3: "+ui/gfx", do you think this should be split to a *_unittest.cc specific rule? https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... File components/viz/common/server_gpu_memory_buffer_manager_unittest.cc (right): https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager_unittest.cc:13: namespace viz { maybe throw everything inside a nested anon namespace, so we don't add symbols to the viz namespace outside this file https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager_unittest.cc:127: class StubClientNativePixmapFactory : public gfx::ClientNativePixmapFactory { nit: I think Fake describes this more than Stub. Stub is empty class, whereas this is making an opinionated decision to report support for native configs. I could see us using this more in future tests and growing that support being optional or something. https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager_unittest.cc:153: ServerGpuMemoryBufferManagerTest() {} nit: = default instead of {} whenever possible https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager_unittest.cc:165: base::test::ScopedTaskEnvironment env; env_ https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager_unittest.cc:166: StubClientNativePixmapFactory pixmap_factory; pixmap_factory_ https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager_unittest.cc:174: // Not all platforms support native configurations (currently only ozone and Can you #ifdef the return statement to only platforms you expect, and if isn't need at that point can you just #ifdef return; #endif with no runtime check? https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager_unittest.cc:179: ServerGpuMemoryBufferManager manager(&gpu_service, 1); Can you mention we're handing the mojo pointer directly instead of binding it, so async operations will become sync in this test?
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sadrul@chromium.org changed reviewers: + jbauman@chromium.org, sky@chromium.org
+sky@ for components/BUILD.gn +jbauman@ for gpu_memory_buffer_manager.h (will TBR for this one, since I think reveman@ is owner for this, looking at the comment in gpu/OWNERS) https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... File components/viz/common/DEPS (right): https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... components/viz/common/DEPS:3: "+ui/gfx", On 2017/06/21 18:50:35, danakj wrote: > do you think this should be split to a *_unittest.cc specific rule? Sure. Done. https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... File components/viz/common/server_gpu_memory_buffer_manager_unittest.cc (right): https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager_unittest.cc:13: namespace viz { On 2017/06/21 18:50:36, danakj wrote: > maybe throw everything inside a nested anon namespace, so we don't add symbols > to the viz namespace outside this file Yep, see next line :) https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager_unittest.cc:127: class StubClientNativePixmapFactory : public gfx::ClientNativePixmapFactory { On 2017/06/21 18:50:36, danakj wrote: > nit: I think Fake describes this more than Stub. Stub is empty class, whereas > this is making an opinionated decision to report support for native configs. I > could see us using this more in future tests and growing that support being > optional or something. Renamed to Fake https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager_unittest.cc:153: ServerGpuMemoryBufferManagerTest() {} On 2017/06/21 18:50:36, danakj wrote: > nit: = default instead of {} whenever possible Done. https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager_unittest.cc:165: base::test::ScopedTaskEnvironment env; On 2017/06/21 18:50:36, danakj wrote: > env_ Done. https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager_unittest.cc:166: StubClientNativePixmapFactory pixmap_factory; On 2017/06/21 18:50:35, danakj wrote: > pixmap_factory_ Done (sorry, moved from within the test body to here, and neglected to rename). https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager_unittest.cc:174: // Not all platforms support native configurations (currently only ozone and On 2017/06/21 18:50:36, danakj wrote: > Can you #ifdef the return statement to only platforms you expect, and if isn't > need at that point can you just #ifdef return; #endif with no runtime check? Done. https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager_unittest.cc:179: ServerGpuMemoryBufferManager manager(&gpu_service, 1); On 2017/06/21 18:50:36, danakj wrote: > Can you mention we're handing the mojo pointer directly instead of binding it, > so async operations will become sync in this test? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... File components/viz/common/server_gpu_memory_buffer_manager_unittest.cc (right): https://codereview.chromium.org/2941933002/diff/220001/components/viz/common/... components/viz/common/server_gpu_memory_buffer_manager_unittest.cc:13: namespace viz { On 2017/06/21 20:38:54, sadrul wrote: > On 2017/06/21 18:50:36, danakj wrote: > > maybe throw everything inside a nested anon namespace, so we don't add symbols > > to the viz namespace outside this file > > Yep, see next line :) Derp, oh I didn't see it closing at the bottom of the file, but you've got it around the stuff that matters. :)
components/BUILD.gn LGTM
The CQ bit was checked by sadrul@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== viz: Convert a sync api in ServerGpuMemoryBufferManager into async. Convert ServerGpuMemoryBufferManager::CreateGpuMemoryBufferHandle() into an async api. This is in preparation for using this implementation from content. BUG=733482 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== viz: Convert a sync api in ServerGpuMemoryBufferManager into async. Convert ServerGpuMemoryBufferManager::CreateGpuMemoryBufferHandle() into an async api. This is in preparation for using this implementation from content. BUG=733482 TBR=jbauman@ for gpu_memory_buffer_manager.h doc update CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by sadrul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, kenrb@chromium.org, danakj@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2941933002/#ps260001 (title: "fix test build on windows")
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_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sadrul@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1498133965002960,
"parent_rev": "a086f601b68520d15e484aeff04efde78c9d596d", "commit_rev":
"fbccda44a34bf3ddaf0aa3119afbba21fa9ab5a9"}
Message was sent while issue was closed.
Description was changed from ========== viz: Convert a sync api in ServerGpuMemoryBufferManager into async. Convert ServerGpuMemoryBufferManager::CreateGpuMemoryBufferHandle() into an async api. This is in preparation for using this implementation from content. BUG=733482 TBR=jbauman@ for gpu_memory_buffer_manager.h doc update CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== viz: Convert a sync api in ServerGpuMemoryBufferManager into async. Convert ServerGpuMemoryBufferManager::CreateGpuMemoryBufferHandle() into an async api. This is in preparation for using this implementation from content. BUG=733482 TBR=jbauman@ for gpu_memory_buffer_manager.h doc update CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2941933002 Cr-Commit-Position: refs/heads/master@{#481517} Committed: https://chromium.googlesource.com/chromium/src/+/fbccda44a34bf3ddaf0aa3119afb... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/fbccda44a34bf3ddaf0aa3119afb... |
