Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1993333002:
Implement GpuMemoryBufferManager::CreateGpuMemoryBufferFromClientId (Closed)
DescriptionImplement GpuMemoryBufferManager::CreateGpuMemoryBufferFromClientId
For buffers created for a child process, open a copy of the buffer in the browser
process before handing the buffer's handle to the child process. When
CreateGpuMemoryBufferFromClientId is called in the browser process, use this
existing copy of the buffer to create a browser-process copy of the buffer.
Update IOSurface implementation to populate the mach port.
BUG=614791
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/394e5eec470ff9a0d2af4cae796ccd69c86ee783
Cr-Commit-Position: refs/heads/master@{#396084}
Patch Set 1 #Patch Set 2 : Just native buffers #Patch Set 3 : All child buffers #Patch Set 4 : restrict to native buffers #Patch Set 5 : Restrict to IOSurface, add traces #
Total comments: 6
Patch Set 6 : Incorporate review feedback #Patch Set 7 : add todos and bug numbers #
Messages
Total messages: 28 (14 generated)
Description was changed from ========== Implement GpuMemoryBufferManager::CreateGpuMemoryBufferFromClientId BUG=604052 ========== to ========== Implement GpuMemoryBufferManager::CreateGpuMemoryBufferFromClientId BUG=604052 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Implement GpuMemoryBufferManager::CreateGpuMemoryBufferFromClientId BUG=604052 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Implement GpuMemoryBufferManager::CreateGpuMemoryBufferFromClientId For buffers created for a child process, open a copy of the buffer in the browser process before handing the buffer's handle to the child process. When CreateGpuMemoryBufferFromClientId is called in the browser process, use this existing copy of the buffer to create a browser-process copy of the buffer. Update IOSurface implementation to populate the mach port. BUG=604052 ==========
ccameron@chromium.org changed reviewers: + reveman@chromium.org
ptal
Description was changed from ========== Implement GpuMemoryBufferManager::CreateGpuMemoryBufferFromClientId For buffers created for a child process, open a copy of the buffer in the browser process before handing the buffer's handle to the child process. When CreateGpuMemoryBufferFromClientId is called in the browser process, use this existing copy of the buffer to create a browser-process copy of the buffer. Update IOSurface implementation to populate the mach port. BUG=604052 ========== to ========== Implement GpuMemoryBufferManager::CreateGpuMemoryBufferFromClientId For buffers created for a child process, open a copy of the buffer in the browser process before handing the buffer's handle to the child process. When CreateGpuMemoryBufferFromClientId is called in the browser process, use this existing copy of the buffer to create a browser-process copy of the buffer. Update IOSurface implementation to populate the mach port. BUG=604052 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
ccameron@chromium.org changed reviewers: + sievers@chromium.org
Adding sievers as owner for gpu/ipc/client.
ping on this
lgtm stamp if reveman@ is happy
lg but I'd love to see this not be mac specific https://codereview.chromium.org/1993333002/diff/80001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/1993333002/diff/80001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_memory_buffer_manager.cc:133: void DestroyGpuMemoryBufferNoOp(const gpu::SyncToken& sync_token) { nit: no need for NoOp in the name https://codereview.chromium.org/1993333002/diff/80001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_memory_buffer_manager.cc:738: if (handle.type == gfx::IO_SURFACE_BUFFER) { This should be the correct behavior on all platforms. Do we need the conditional? https://codereview.chromium.org/1993333002/diff/80001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_memory_buffer_manager.h (right): https://codereview.chromium.org/1993333002/diff/80001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_memory_buffer_manager.h:130: std::unique_ptr<gpu::GpuMemoryBufferImpl> parent_buffer_instance; This is not just needed for FromClientId. It's needed for all buffers in case the GPU process crashes. Can we rename this to simply |buffer|, use it on all platforms and remove the fields above that are no longer needed?
Thanks! Enabling this on all platforms and removing the redundant fields will be a fairly substantial refactor. I'd like to land the smaller IOSurface patch first to [1] ensure that there are no surprises in doing this and [2] un-block the Mac changes depending on this. sg? https://codereview.chromium.org/1993333002/diff/80001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/1993333002/diff/80001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_memory_buffer_manager.cc:133: void DestroyGpuMemoryBufferNoOp(const gpu::SyncToken& sync_token) { On 2016/05/24 22:19:47, reveman wrote: > nit: no need for NoOp in the name Done. https://codereview.chromium.org/1993333002/diff/80001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_memory_buffer_manager.cc:738: if (handle.type == gfx::IO_SURFACE_BUFFER) { On 2016/05/24 22:19:47, reveman wrote: > This should be the correct behavior on all platforms. Do we need the > conditional? Changed this to a TODO. This is the only one that I have extensively tested, so I'd feel safer leaving the conditional for now. I saw some flakey issues with GL tryjobs on Linux, which I'll want to investigate. https://codereview.chromium.org/1993333002/diff/80001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_memory_buffer_manager.h (right): https://codereview.chromium.org/1993333002/diff/80001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_memory_buffer_manager.h:130: std::unique_ptr<gpu::GpuMemoryBufferImpl> parent_buffer_instance; On 2016/05/24 22:19:47, reveman wrote: > This is not just needed for FromClientId. It's needed for all buffers in case > the GPU process crashes. Can we rename this to simply |buffer|, use it on all > platforms and remove the fields above that are no longer needed? Definitely agree with this long-term. Removing the fields is a pretty big refactor, and will need testing for each platform. I'd prefer to get this in piecemeal.
Re: Landing just IOSurface support first: I put a bit more time into using BufferInfo::buffer for the shared-memory fallback (just to start), and that patch is pretty big on its own (and pulls in some changes to GpuMemoryBufferImplSharedMemory). I'll put it up when I'm done with it, but I definitely prefer having this separated out.
Description was changed from ========== Implement GpuMemoryBufferManager::CreateGpuMemoryBufferFromClientId For buffers created for a child process, open a copy of the buffer in the browser process before handing the buffer's handle to the child process. When CreateGpuMemoryBufferFromClientId is called in the browser process, use this existing copy of the buffer to create a browser-process copy of the buffer. Update IOSurface implementation to populate the mach port. BUG=604052 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Implement GpuMemoryBufferManager::CreateGpuMemoryBufferFromClientId For buffers created for a child process, open a copy of the buffer in the browser process before handing the buffer's handle to the child process. When CreateGpuMemoryBufferFromClientId is called in the browser process, use this existing copy of the buffer to create a browser-process copy of the buffer. Update IOSurface implementation to populate the mach port. BUG=614791 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by ccameron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1993333002/#ps120001 (title: "add todos and bug numbers")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993333002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993333002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by ccameron@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993333002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993333002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_optional_gpu_tests_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
The CQ bit was checked by ccameron@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1993333002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1993333002/120001
Message was sent while issue was closed.
Description was changed from ========== Implement GpuMemoryBufferManager::CreateGpuMemoryBufferFromClientId For buffers created for a child process, open a copy of the buffer in the browser process before handing the buffer's handle to the child process. When CreateGpuMemoryBufferFromClientId is called in the browser process, use this existing copy of the buffer to create a browser-process copy of the buffer. Update IOSurface implementation to populate the mach port. BUG=614791 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Implement GpuMemoryBufferManager::CreateGpuMemoryBufferFromClientId For buffers created for a child process, open a copy of the buffer in the browser process before handing the buffer's handle to the child process. When CreateGpuMemoryBufferFromClientId is called in the browser process, use this existing copy of the buffer to create a browser-process copy of the buffer. Update IOSurface implementation to populate the mach port. BUG=614791 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Implement GpuMemoryBufferManager::CreateGpuMemoryBufferFromClientId For buffers created for a child process, open a copy of the buffer in the browser process before handing the buffer's handle to the child process. When CreateGpuMemoryBufferFromClientId is called in the browser process, use this existing copy of the buffer to create a browser-process copy of the buffer. Update IOSurface implementation to populate the mach port. BUG=614791 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Implement GpuMemoryBufferManager::CreateGpuMemoryBufferFromClientId For buffers created for a child process, open a copy of the buffer in the browser process before handing the buffer's handle to the child process. When CreateGpuMemoryBufferFromClientId is called in the browser process, use this existing copy of the buffer to create a browser-process copy of the buffer. Update IOSurface implementation to populate the mach port. BUG=614791 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/394e5eec470ff9a0d2af4cae796ccd69c86ee783 Cr-Commit-Position: refs/heads/master@{#396084} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/394e5eec470ff9a0d2af4cae796ccd69c86ee783 Cr-Commit-Position: refs/heads/master@{#396084} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
