Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1263323004:
Add NativePixmapHandle type & interface for exporting them (Closed)
|
Created:
5 years, 4 months ago by spang Modified:
5 years, 4 months ago Reviewers:
reveman, kenrb, vignatti (out of this project), dshwang, dcheng, jln (very slow on Chromium), no sievers, sadrul CC:
chromium-reviews, danakj+watch_chromium.org, darin-cc_chromium.org, jam, jbauman+watch_chromium.org, kalyank, ozone-reviews_chromium.org, piman+watch_chromium.org, sievers+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd NativePixmapHandle type & interface for exporting them
This adds a platform handle to gfx::GpuMemoryBufferHandle for
NativePixmap that can hold both the file descriptor & any needed
metadata (currently just buffer stride) instead of reusing the shared
memory handle.
GpuMemoryBufferFactoryOzoneNativePixmap creates one of these handles
via NativePixmap::ExportHandle() and passes it to the client
requesting buffer creation, which then passes it to
ClientNativePixmapFactory::ImportFromHandle() to create the client
object.
It's similar to Mac, where GpuMemoryBuffer relies on a lower handle
(IOSurfaceRef) for transporting the buffer (including metadata such as
the stride). The difference is file descriptor based handles can be
transported directly inside gfx::GpuMemoryBufferHandle.
BUG=475633
TEST=Manually tested Chrome OS link with flags:
--no-sandbox
--enable-native-gpu-memory-buffers
(1) Chrome comes up
(2) No rendering artifacts due to assuming minimum stride.
(tested with additional patch to implement VgemPixmap)
Committed: https://crrev.com/9b0433a3d10440d8b90cc7fd99d0203e1ead4d28
Cr-Commit-Position: refs/heads/master@{#344671}
Patch Set 1 #
Total comments: 5
Patch Set 2 : comments from reveman@ #
Total comments: 3
Patch Set 3 : rm some unneeded hunks #
Total comments: 6
Patch Set 4 : remove unnecesary #include and DCHECK msgs #
Total comments: 8
Patch Set 5 : couple more comments #Patch Set 6 : aand more compiling #Patch Set 7 : remove unnecessary & wrong forward decls #
Total comments: 5
Patch Set 8 : comment from sievers #Patch Set 9 : oops - dont fail allocating scanout pixmaps #
Total comments: 4
Patch Set 10 : docs & non-functional changes #Patch Set 11 : DCHECK on import failure #
Total comments: 10
Patch Set 12 : comments from dcheng #Patch Set 13 : rebase #Patch Set 14 : fix cast build #Messages
Total messages: 60 (12 generated)
spang@chromium.org changed reviewers: + dongseong.hwang@intel.com, reveman@chromium.org
on top of https://codereview.chromium.org/1128113011/ This one way to communicate the stride. One nice property of adding an inner struct to GpuMemoryBufferHandle is that we can add other metadata without changing a ton of method signatures. We could also just dump everything including stride GpuMemoryBufferHandle, but then its less clear who's responsible for each part. In particular the "type" and "id" fields are definitely managed at a higher layer while the "fd" and "stride" fields are managed by the lower one. I have rebased the vgem patch on top of this locally, and it works without resulting in artifacts from incorrect stride usage (with the factory opening vgem directly in --no-sandbox mode).
https://codereview.chromium.org/1263323004/diff/1/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1263323004/diff/1/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:79: if (usage == gfx::BufferUsage::MAP && Can you remove the MAP check from here and not fail if ExportHandle fails? We might have other usage modes that require the handle and maybe SCANOUT will in the future. MAP_PERSISTENT does for example. We'd fail later if the supported configurations are not correct anyhow. https://codereview.chromium.org/1263323004/diff/1/ui/gfx/buffer_types.h File ui/gfx/buffer_types.h (right): https://codereview.chromium.org/1263323004/diff/1/ui/gfx/buffer_types.h#newco... ui/gfx/buffer_types.h:40: struct NativePixmapHandle { Can you move this out of this otherwise platform independent file? Maybe add it to ui/ozone/public/native_pixmap.h
https://codereview.chromium.org/1263323004/diff/1/content/common/gpu/gpu_memo... File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1263323004/diff/1/content/common/gpu/gpu_memo... content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:79: if (usage == gfx::BufferUsage::MAP && On 2015/08/05 04:18:00, reveman wrote: > Can you remove the MAP check from here and not fail if ExportHandle fails? We > might have other usage modes that require the handle and maybe SCANOUT will in > the future. MAP_PERSISTENT does for example. We'd fail later if the supported > configurations are not correct anyhow. Done.. now it imports scanout as well. It is not useful to import them other than unifying code paths, though, since they are only used by name. https://codereview.chromium.org/1263323004/diff/1/ui/gfx/buffer_types.h File ui/gfx/buffer_types.h (right): https://codereview.chromium.org/1263323004/diff/1/ui/gfx/buffer_types.h#newco... ui/gfx/buffer_types.h:40: struct NativePixmapHandle { On 2015/08/05 04:18:00, reveman wrote: > Can you move this out of this otherwise platform independent file? Maybe add it > to ui/ozone/public/native_pixmap.h If it is embedded in GpuMemoryBufferHandle, it can't go in ui/ozone without breaking the include rules. I've moved it to ui/gfx/native_pixmap_handle_ozone.h.
thx for fixing stride issue. https://codereview.chromium.org/1263323004/diff/20001/ui/gfx/gpu_memory_buffer.h File ui/gfx/gpu_memory_buffer.h (right): https://codereview.chromium.org/1263323004/diff/20001/ui/gfx/gpu_memory_buffe... ui/gfx/gpu_memory_buffer.h:40: NativePixmapHandle native_pixmap_handle; we can pass |dmabuf_fd| via GpuMemoryBufferHandle::handle, but use NativePixmapHandle::fd instead. GpuMemoryBufferHandle::handle is not used in ozone. So additional member is |stride|. I'm not sure if we will add additional metadata. imho, it looks over-generalized.
On 2015/08/05 13:30:10, dshwang wrote: > thx for fixing stride issue. > > https://codereview.chromium.org/1263323004/diff/20001/ui/gfx/gpu_memory_buffer.h > File ui/gfx/gpu_memory_buffer.h (right): > > https://codereview.chromium.org/1263323004/diff/20001/ui/gfx/gpu_memory_buffe... > ui/gfx/gpu_memory_buffer.h:40: NativePixmapHandle native_pixmap_handle; > we can pass |dmabuf_fd| via GpuMemoryBufferHandle::handle, but use > NativePixmapHandle::fd instead. GpuMemoryBufferHandle::handle is not used in > ozone. > So additional member is |stride|. I'm not sure if we will add additional > metadata. imho, it looks over-generalized. SharedMemoryHandle is intended for use with base::SharedMemory. I don't think it's a big deal either way, we either end up with #if defined(USE_OZONE) int stride; #endif or #if defined(USE_OZONE) NativePixmapHandle native_pixmap_handle; #endif the latter captures the intent better. We get an extra empty fd in the message but the cost is negligible.
lgtm with some dcheck comments that you can choose to ignore https://codereview.chromium.org/1263323004/diff/1/ui/gfx/buffer_types.h File ui/gfx/buffer_types.h (right): https://codereview.chromium.org/1263323004/diff/1/ui/gfx/buffer_types.h#newco... ui/gfx/buffer_types.h:40: struct NativePixmapHandle { On 2015/08/05 at 13:26:15, spang wrote: > On 2015/08/05 04:18:00, reveman wrote: > > Can you move this out of this otherwise platform independent file? Maybe add it > > to ui/ozone/public/native_pixmap.h > > If it is embedded in GpuMemoryBufferHandle, it can't go in ui/ozone without breaking the include rules. > > I've moved it to ui/gfx/native_pixmap_handle_ozone.h. Acknowledged. https://codereview.chromium.org/1263323004/diff/20001/ui/gfx/gpu_memory_buffer.h File ui/gfx/gpu_memory_buffer.h (right): https://codereview.chromium.org/1263323004/diff/20001/ui/gfx/gpu_memory_buffe... ui/gfx/gpu_memory_buffer.h:40: NativePixmapHandle native_pixmap_handle; On 2015/08/05 at 13:30:10, dshwang wrote: > we can pass |dmabuf_fd| via GpuMemoryBufferHandle::handle, but use NativePixmapHandle::fd instead. GpuMemoryBufferHandle::handle is not used in ozone. > So additional member is |stride|. I'm not sure if we will add additional metadata. imho, it looks over-generalized. I prefer having the fd as part of |native_pixmap_handle|. https://codereview.chromium.org/1263323004/diff/40001/content/common/gpu/gpu_... File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1263323004/diff/40001/content/common/gpu/gpu_... content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:84: << "pixmap with this key must not exist"; not new to this patch but I think this log output is overkill. we have similar dchecks in a lot of places but never log a string like this as the code itself makes that obvious. https://codereview.chromium.org/1263323004/diff/40001/content/common/gpu/gpu_... content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:96: DCHECK(it != native_pixmaps_.end()) << "pixmap with this key must exist"; ditto https://codereview.chromium.org/1263323004/diff/40001/ui/ozone/public/native_... File ui/ozone/public/native_pixmap.h (right): https://codereview.chromium.org/1263323004/diff/40001/ui/ozone/public/native_... ui/ozone/public/native_pixmap.h:10: #include "ui/gfx/buffer_types.h" is this include needed?
https://codereview.chromium.org/1263323004/diff/40001/content/common/gpu/gpu_... File content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1263323004/diff/40001/content/common/gpu/gpu_... content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:84: << "pixmap with this key must not exist"; On 2015/08/05 13:58:52, reveman wrote: > not new to this patch but I think this log output is overkill. we have similar > dchecks in a lot of places but never log a string like this as the code itself > makes that obvious. Agreed, it looked verbose to me as well. Done. https://codereview.chromium.org/1263323004/diff/40001/content/common/gpu/gpu_... content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc:96: DCHECK(it != native_pixmaps_.end()) << "pixmap with this key must exist"; On 2015/08/05 13:58:52, reveman wrote: > ditto Done. https://codereview.chromium.org/1263323004/diff/40001/ui/ozone/public/native_... File ui/ozone/public/native_pixmap.h (right): https://codereview.chromium.org/1263323004/diff/40001/ui/ozone/public/native_... ui/ozone/public/native_pixmap.h:10: #include "ui/gfx/buffer_types.h" On 2015/08/05 13:58:53, reveman wrote: > is this include needed? Seems not, removed.
https://codereview.chromium.org/1263323004/diff/20001/ui/gfx/gpu_memory_buffer.h File ui/gfx/gpu_memory_buffer.h (right): https://codereview.chromium.org/1263323004/diff/20001/ui/gfx/gpu_memory_buffe... ui/gfx/gpu_memory_buffer.h:40: NativePixmapHandle native_pixmap_handle; On 2015/08/05 13:58:52, reveman wrote: > On 2015/08/05 at 13:30:10, dshwang wrote: > > we can pass |dmabuf_fd| via GpuMemoryBufferHandle::handle, but use > NativePixmapHandle::fd instead. GpuMemoryBufferHandle::handle is not used in > ozone. > > So additional member is |stride|. I'm not sure if we will add additional > metadata. imho, it looks over-generalized. > > I prefer having the fd as part of |native_pixmap_handle|. I see. lgtm
https://codereview.chromium.org/1263323004/diff/60001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1263323004/diff/60001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/gbm_buffer.cc:101: int dmabuf_fd = dup(dma_buf_); other code uses HANDLE_EINTR with dup() int dmabuf_fd = HANDLE_EINTR(dup(dma_buf_));
spang@chromium.org changed reviewers: + kenrb@chromium.org, sadrul@chromium.org, sievers@chromium.org
+sievers for content/ +kenrb for IPC +sadrul for ui/gfx
kenrb@chromium.org changed reviewers: + jln@chromium.org
jln: I'm not comfortable reviewing the IPC change here. Would you mind having a look?
https://codereview.chromium.org/1263323004/diff/60001/ui/gfx/BUILD.gn File ui/gfx/BUILD.gn (right): https://codereview.chromium.org/1263323004/diff/60001/ui/gfx/BUILD.gn#newcode145 ui/gfx/BUILD.gn:145: "native_pixmap_handle_ozone.h", Should be in 'if (use_ozone)' block?
tiago.vignatti@intel.com changed reviewers: + tiago.vignatti@intel.com
https://codereview.chromium.org/1263323004/diff/60001/ui/gfx/native_pixmap_ha... File ui/gfx/native_pixmap_handle_ozone.h (right): https://codereview.chromium.org/1263323004/diff/60001/ui/gfx/native_pixmap_ha... ui/gfx/native_pixmap_handle_ozone.h:12: struct NativePixmapHandle { class instead?
https://codereview.chromium.org/1263323004/diff/60001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/gbm_buffer.h (right): https://codereview.chromium.org/1263323004/diff/60001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/gbm_buffer.h:19: class NativePixmapHandle; as Tiago mentioned, it's defined as struct in ui/gfx/native_pixmap_handle_ozone.h
On 2015/08/06 17:30:18, vignatti wrote: > https://codereview.chromium.org/1263323004/diff/60001/ui/gfx/native_pixmap_ha... > File ui/gfx/native_pixmap_handle_ozone.h (right): > > https://codereview.chromium.org/1263323004/diff/60001/ui/gfx/native_pixmap_ha... > ui/gfx/native_pixmap_handle_ozone.h:12: struct NativePixmapHandle { > class instead? spang@, I actually don't know if you want use struct or class for the NativePixmapHandle (there's not much difference beside the visibility after all, right?), but this is what I've needed to patch to build chrome: https://gist.githubusercontent.com/tiagovignatti/59c528b1a2f293f6bfdc/raw/fd0...
https://codereview.chromium.org/1263323004/diff/60001/ui/gfx/BUILD.gn File ui/gfx/BUILD.gn (right): https://codereview.chromium.org/1263323004/diff/60001/ui/gfx/BUILD.gn#newcode145 ui/gfx/BUILD.gn:145: "native_pixmap_handle_ozone.h", On 2015/08/06 15:32:05, sadrul wrote: > Should be in 'if (use_ozone)' block? Yeah oops. I keep forgetting it's not automatic in GN. https://codereview.chromium.org/1263323004/diff/60001/ui/gfx/native_pixmap_ha... File ui/gfx/native_pixmap_handle_ozone.h (right): https://codereview.chromium.org/1263323004/diff/60001/ui/gfx/native_pixmap_ha... ui/gfx/native_pixmap_handle_ozone.h:12: struct NativePixmapHandle { On 2015/08/06 17:30:18, vignatti wrote: > class instead? struct This is a topic in the style guide, please review it there. https://codereview.chromium.org/1263323004/diff/60001/ui/ozone/platform/drm/g... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1263323004/diff/60001/ui/ozone/platform/drm/g... ui/ozone/platform/drm/gpu/gbm_buffer.cc:101: int dmabuf_fd = dup(dma_buf_); On 2015/08/06 08:14:48, dshwang wrote: > other code uses HANDLE_EINTR with dup() > int dmabuf_fd = HANDLE_EINTR(dup(dma_buf_)); Done.
https://codereview.chromium.org/1263323004/diff/60001/ui/gfx/native_pixmap_ha... File ui/gfx/native_pixmap_handle_ozone.h (right): https://codereview.chromium.org/1263323004/diff/60001/ui/gfx/native_pixmap_ha... ui/gfx/native_pixmap_handle_ozone.h:12: struct NativePixmapHandle { On 2015/08/06 20:53:29, spang wrote: > On 2015/08/06 17:30:18, vignatti wrote: > > class instead? > > struct > > This is a topic in the style guide, please review it there. Ah I see why you pointed out. Fixed bad forward declaration, thanks.
On 2015/08/06 21:04:15, spang wrote: > https://codereview.chromium.org/1263323004/diff/60001/ui/gfx/native_pixmap_ha... > File ui/gfx/native_pixmap_handle_ozone.h (right): > > https://codereview.chromium.org/1263323004/diff/60001/ui/gfx/native_pixmap_ha... > ui/gfx/native_pixmap_handle_ozone.h:12: struct NativePixmapHandle { > On 2015/08/06 20:53:29, spang wrote: > > On 2015/08/06 17:30:18, vignatti wrote: > > > class instead? > > > > struct > > > > This is a topic in the style guide, please review it there. > > Ah I see why you pointed out. Fixed bad forward declaration, thanks. yup. But I think we will need also the similar change in the other header files, like I pointed in the gist.
On 2015/08/06 21:14:01, vignatti wrote: > On 2015/08/06 21:04:15, spang wrote: > > > https://codereview.chromium.org/1263323004/diff/60001/ui/gfx/native_pixmap_ha... > > File ui/gfx/native_pixmap_handle_ozone.h (right): > > > > > https://codereview.chromium.org/1263323004/diff/60001/ui/gfx/native_pixmap_ha... > > ui/gfx/native_pixmap_handle_ozone.h:12: struct NativePixmapHandle { > > On 2015/08/06 20:53:29, spang wrote: > > > On 2015/08/06 17:30:18, vignatti wrote: > > > > class instead? > > > > > > struct > > > > > > This is a topic in the style guide, please review it there. > > > > Ah I see why you pointed out. Fixed bad forward declaration, thanks. > > yup. But I think we will need also the similar change in the other header files, > like I pointed in the gist. Fixed, thanks. Some of them weren't triggering the warning because the real definition isn't included in the same compilation unit.
lgtm
jln: ping sievers: ping
https://codereview.chromium.org/1263323004/diff/120001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1263323004/diff/120001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:31: ui::ClientNativePixmapFactory::GetInstance()->ImportFromHandle( Can this fail? Below we are accessing |pixmap_| without null-checks, so maybe here is a good place to check for null and return early?
https://codereview.chromium.org/1263323004/diff/120001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1263323004/diff/120001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:31: ui::ClientNativePixmapFactory::GetInstance()->ImportFromHandle( On 2015/08/11 21:16:33, sievers wrote: > Can this fail? > > Below we are accessing |pixmap_| without null-checks, so maybe here is a good > place to check for null and return early? Done. However since cc does not check for allocation failure, it will still crash later.
content lgtm
https://codereview.chromium.org/1263323004/diff/120001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1263323004/diff/120001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:31: ui::ClientNativePixmapFactory::GetInstance()->ImportFromHandle( On 2015/08/11 at 22:03:31, spang wrote: > On 2015/08/11 21:16:33, sievers wrote: > > Can this fail? > > > > Below we are accessing |pixmap_| without null-checks, so maybe here is a good > > place to check for null and return early? > > Done. However since cc does not check for allocation failure, it will still crash later. cc handles GMB allocation failure properly as it can happen during lost context situations. However, I expect ImportFromHandle to not be affected by this as after the browser has successfully allocating a buffer and returned a valid handle there's no good reason for ImportFromHandle to fail, or is there? What would be the reason for this to fail? I'd rather see a DCHECK or CHECK if it's not something we expect to happen.
https://codereview.chromium.org/1263323004/diff/120001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1263323004/diff/120001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:31: ui::ClientNativePixmapFactory::GetInstance()->ImportFromHandle( On 2015/08/12 08:20:29, reveman (OOO Aug 10 - Aug 11) wrote: > On 2015/08/11 at 22:03:31, spang wrote: > > On 2015/08/11 21:16:33, sievers wrote: > > > Can this fail? > > > > > > Below we are accessing |pixmap_| without null-checks, so maybe here is a > good > > > place to check for null and return early? > > > > Done. However since cc does not check for allocation failure, it will still > crash later. > > cc handles GMB allocation failure properly as it can happen during lost context > situations. However, I expect ImportFromHandle to not be affected by this as > after the browser has successfully allocating a buffer and returned a valid > handle there's no good reason for ImportFromHandle to fail, or is there? What > would be the reason for this to fail? I'd rather see a DCHECK or CHECK if it's > not something we expect to happen. I think null check is not needed, as well as nullptr is possible in SCANOUT case. Until now, this class works well in SCANOUT case without |pixmap|. Because Map/Unmap/Stride must not be called in SCANOUT case. The next CL (https://codereview.chromium.org/1134993003/) will not create |pixmap| in SCANOUT case as-is. spang, reveman, Do I think same to you?
https://codereview.chromium.org/1263323004/diff/120001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1263323004/diff/120001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:31: ui::ClientNativePixmapFactory::GetInstance()->ImportFromHandle( On 2015/08/12 08:28:09, dshwang wrote: > On 2015/08/12 08:20:29, reveman (OOO Aug 10 - Aug 11) wrote: > > On 2015/08/11 at 22:03:31, spang wrote: > > > On 2015/08/11 21:16:33, sievers wrote: > > > > Can this fail? > > > > > > > > Below we are accessing |pixmap_| without null-checks, so maybe here is a > > good > > > > place to check for null and return early? > > > > > > Done. However since cc does not check for allocation failure, it will still > > crash later. > > > > cc handles GMB allocation failure properly as it can happen during lost > context > > situations. However, I expect ImportFromHandle to not be affected by this as > > after the browser has successfully allocating a buffer and returned a valid > > handle there's no good reason for ImportFromHandle to fail, or is there? What > > would be the reason for this to fail? I'd rather see a DCHECK or CHECK if it's > > not something we expect to happen. > > I think null check is not needed, as well as nullptr is possible in SCANOUT > case. > Until now, this class works well in SCANOUT case without |pixmap|. Because > Map/Unmap/Stride must not be called in SCANOUT case. > The next CL (https://codereview.chromium.org/1134993003/) will not create > |pixmap| in SCANOUT case as-is. > spang, reveman, Do I think same to you? I understand that ClientNativePixmapGbm is always created, so now I change my mind. It's good to add DCHECK(native_pixmap) as sievers said. https://codereview.chromium.org/1263323004/diff/150001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1263323004/diff/150001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:47: base::ScopedFD close_fd(handle.fd.fd); I missed this when reviewing. nice. How about adding DCHECK(handle.fd.auto_close); ? On 2015/08/11 20:15:16, reveman (OOO Aug 10 - Aug 11) wrote: > base::ScopedFD close_fd(handle.fd.fd); > it's a bit weird to reach into the fd and check auto_close from here. why do we > need this and base::ScopedFD? is the caller not responsible for this? GbmPixmap::ExportHandle() dup dmabuf_fd in GPU process. So some code must close it. I think this is right place.
https://codereview.chromium.org/1263323004/diff/150001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1263323004/diff/150001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:47: base::ScopedFD close_fd(handle.fd.fd); On 2015/08/12 at 08:41:43, dshwang wrote: > I missed this when reviewing. nice. > How about adding DCHECK(handle.fd.auto_close); ? > > On 2015/08/11 20:15:16, reveman (OOO Aug 10 - Aug 11) wrote: > > base::ScopedFD close_fd(handle.fd.fd); > > it's a bit weird to reach into the fd and check auto_close from here. why do we > > need this and base::ScopedFD? is the caller not responsible for this? > > GbmPixmap::ExportHandle() dup dmabuf_fd in GPU process. So some code must close it. I think this is right place. Ok, so the ownership of the fd is passed to this function and the client pixmap is supposed to close it when done. I think auto_close exists mainly for the IPC system and I don't think we should use it here. It's better to simply have the description of this function make it clear that it expects ownership of the FD to be passed to it. nit: s/close_fd/scoped_fd/ or maybe pass this fd to ClientNativePixmapGbm instead of closing it early. I think passing it to the pixmap is nice as that would better match the expected usage and result in the same lifetime as we'll have in the future.
https://codereview.chromium.org/1263323004/diff/150001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1263323004/diff/150001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:47: base::ScopedFD close_fd(handle.fd.fd); On 2015/08/12 09:13:38, reveman (OOO Aug 10 - Aug 11) wrote: > On 2015/08/12 at 08:41:43, dshwang wrote: > > I missed this when reviewing. nice. > > How about adding DCHECK(handle.fd.auto_close); ? > > > > On 2015/08/11 20:15:16, reveman (OOO Aug 10 - Aug 11) wrote: > > > base::ScopedFD close_fd(handle.fd.fd); > > > it's a bit weird to reach into the fd and check auto_close from here. why do > we > > > need this and base::ScopedFD? is the caller not responsible for this? > > > > GbmPixmap::ExportHandle() dup dmabuf_fd in GPU process. So some code must > close it. I think this is right place. > > Ok, so the ownership of the fd is passed to this function and the client pixmap > is supposed to close it when done. I think auto_close exists mainly for the IPC > system and I don't think we should use it here. It's better to simply have the > description of this function make it clear that it expects ownership of the FD > to be passed to it. > > nit: s/close_fd/scoped_fd/ or maybe pass this fd to ClientNativePixmapGbm > instead of closing it early. I think passing it to the pixmap is nice as that > would better match the expected usage and result in the same lifetime as we'll > have in the future. I'm ok with as-is because ClientNativePixmapVgem will not keep the fd alive. https://codereview.chromium.org/1134993003/diff/440001/ui/ozone/platform/drm/...
On 2015/08/12 08:20:30, reveman (OOO Aug 10 - Aug 11) wrote: > https://codereview.chromium.org/1263323004/diff/120001/content/common/gpu/cli... > File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc > (right): > > https://codereview.chromium.org/1263323004/diff/120001/content/common/gpu/cli... > content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:31: > ui::ClientNativePixmapFactory::GetInstance()->ImportFromHandle( > On 2015/08/11 at 22:03:31, spang wrote: > > On 2015/08/11 21:16:33, sievers wrote: > > > Can this fail? > > > > > > Below we are accessing |pixmap_| without null-checks, so maybe here is a > good > > > place to check for null and return early? > > > > Done. However since cc does not check for allocation failure, it will still > crash later. > > cc handles GMB allocation failure properly as it can happen during lost context > situations. Ah whoops, I misread the code. > However, I expect ImportFromHandle to not be affected by this as > after the browser has successfully allocating a buffer and returned a valid > handle there's no good reason for ImportFromHandle to fail, or is there? What > would be the reason for this to fail? I'd rather see a DCHECK or CHECK if it's > not something we expect to happen. No good reason is probably accurate for gbm, but I think it's probably ideal to treat import failures the same as allocation failures in the API (which may have other implementations). ChildGpuMemoryBufferManager is already prepared for import failures and responds by deleting the buffer. Do you feel strongly about using a DCHECK()?
https://codereview.chromium.org/1263323004/diff/150001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1263323004/diff/150001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:47: base::ScopedFD close_fd(handle.fd.fd); On 2015/08/12 09:13:38, reveman (OOO Aug 10 - Aug 11) wrote: > On 2015/08/12 at 08:41:43, dshwang wrote: > > I missed this when reviewing. nice. > > How about adding DCHECK(handle.fd.auto_close); ? > > > > On 2015/08/11 20:15:16, reveman (OOO Aug 10 - Aug 11) wrote: > > > base::ScopedFD close_fd(handle.fd.fd); > > > it's a bit weird to reach into the fd and check auto_close from here. why do > we > > > need this and base::ScopedFD? is the caller not responsible for this? > > > > GbmPixmap::ExportHandle() dup dmabuf_fd in GPU process. So some code must > close it. I think this is right place. > > Ok, so the ownership of the fd is passed to this function and the client pixmap > is supposed to close it when done. I think auto_close exists mainly for the IPC > system and I don't think we should use it here. It's better to simply have the > description of this function make it clear that it expects ownership of the FD > to be passed to it. Agreed. I've added it to the description. > > nit: s/close_fd/scoped_fd/ or maybe pass this fd to ClientNativePixmapGbm > instead of closing it early. I think passing it to the pixmap is nice as that > would better match the expected usage and result in the same lifetime as we'll > have in the future. Done (the former, I'm not sure if it will go in the ctor or maybe an Initialize() function in the next patch so let's worry about it there).
On 2015/08/12 at 16:18:17, spang wrote: > On 2015/08/12 08:20:30, reveman (OOO Aug 10 - Aug 11) wrote: > > https://codereview.chromium.org/1263323004/diff/120001/content/common/gpu/cli... > > File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc > > (right): > > > > https://codereview.chromium.org/1263323004/diff/120001/content/common/gpu/cli... > > content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:31: > > ui::ClientNativePixmapFactory::GetInstance()->ImportFromHandle( > > On 2015/08/11 at 22:03:31, spang wrote: > > > On 2015/08/11 21:16:33, sievers wrote: > > > > Can this fail? > > > > > > > > Below we are accessing |pixmap_| without null-checks, so maybe here is a > > good > > > > place to check for null and return early? > > > > > > Done. However since cc does not check for allocation failure, it will still > > crash later. > > > > cc handles GMB allocation failure properly as it can happen during lost context > > situations. > > Ah whoops, I misread the code. > > > However, I expect ImportFromHandle to not be affected by this as > > after the browser has successfully allocating a buffer and returned a valid > > handle there's no good reason for ImportFromHandle to fail, or is there? What > > would be the reason for this to fail? I'd rather see a DCHECK or CHECK if it's > > not something we expect to happen. > > No good reason is probably accurate for gbm, but I think it's probably ideal to treat import failures the same as allocation failures in the API (which may have other implementations). ChildGpuMemoryBufferManager is already prepared for import failures and responds by deleting the buffer. Do you feel strongly about using a DCHECK()? Failing to create the buffer will result in the compositor failing to produce new contents. That's fine (and necessary) in a lost context situation but results in very hard to diagnose effects for other types of failures. It's much better if other failures such as OOM will cause a DCHECK or CHECK to fail close to the source of the error. FYI, GpuMemoryBufferImplSharedMemory::CreateFromHandle should be updated to fail hard on OOM errors instead of returning null. OOM errors there are currently going unnoticed and causing very confusing results.
On 2015/08/12 16:36:45, reveman (OOO Aug 10 - Aug 11) wrote: > On 2015/08/12 at 16:18:17, spang wrote: > > On 2015/08/12 08:20:30, reveman (OOO Aug 10 - Aug 11) wrote: > > > > https://codereview.chromium.org/1263323004/diff/120001/content/common/gpu/cli... > > > File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc > > > (right): > > > > > > > https://codereview.chromium.org/1263323004/diff/120001/content/common/gpu/cli... > > > content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:31: > > > ui::ClientNativePixmapFactory::GetInstance()->ImportFromHandle( > > > On 2015/08/11 at 22:03:31, spang wrote: > > > > On 2015/08/11 21:16:33, sievers wrote: > > > > > Can this fail? > > > > > > > > > > Below we are accessing |pixmap_| without null-checks, so maybe here is a > > > good > > > > > place to check for null and return early? > > > > > > > > Done. However since cc does not check for allocation failure, it will > still > > > crash later. > > > > > > cc handles GMB allocation failure properly as it can happen during lost > context > > > situations. > > > > Ah whoops, I misread the code. > > > > > However, I expect ImportFromHandle to not be affected by this as > > > after the browser has successfully allocating a buffer and returned a valid > > > handle there's no good reason for ImportFromHandle to fail, or is there? > What > > > would be the reason for this to fail? I'd rather see a DCHECK or CHECK if > it's > > > not something we expect to happen. > > > > No good reason is probably accurate for gbm, but I think it's probably ideal > to treat import failures the same as allocation failures in the API (which may > have other implementations). ChildGpuMemoryBufferManager is already prepared for > import failures and responds by deleting the buffer. Do you feel strongly about > using a DCHECK()? > > Failing to create the buffer will result in the compositor failing to produce > new contents. That's fine (and necessary) in a lost context situation but > results in very hard to diagnose effects for other types of failures. It's much > better if other failures such as OOM will cause a DCHECK or CHECK to fail close > to the source of the error. > > FYI, GpuMemoryBufferImplSharedMemory::CreateFromHandle should be updated to fail > hard on OOM errors instead of returning null. OOM errors there are currently > going unnoticed and causing very confusing results. Ok I have changed it to a DCHECK.
On 2015/08/12 17:58:53, spang wrote: > On 2015/08/12 16:36:45, reveman (OOO Aug 10 - Aug 11) wrote: > > On 2015/08/12 at 16:18:17, spang wrote: > > > On 2015/08/12 08:20:30, reveman (OOO Aug 10 - Aug 11) wrote: > > > > > > > https://codereview.chromium.org/1263323004/diff/120001/content/common/gpu/cli... > > > > File > content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc > > > > (right): > > > > > > > > > > > https://codereview.chromium.org/1263323004/diff/120001/content/common/gpu/cli... > > > > > content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:31: > > > > ui::ClientNativePixmapFactory::GetInstance()->ImportFromHandle( > > > > On 2015/08/11 at 22:03:31, spang wrote: > > > > > On 2015/08/11 21:16:33, sievers wrote: > > > > > > Can this fail? > > > > > > > > > > > > Below we are accessing |pixmap_| without null-checks, so maybe here is > a > > > > good > > > > > > place to check for null and return early? > > > > > > > > > > Done. However since cc does not check for allocation failure, it will > > still > > > > crash later. > > > > > > > > cc handles GMB allocation failure properly as it can happen during lost > > context > > > > situations. > > > > > > Ah whoops, I misread the code. > > > > > > > However, I expect ImportFromHandle to not be affected by this as > > > > after the browser has successfully allocating a buffer and returned a > valid > > > > handle there's no good reason for ImportFromHandle to fail, or is there? > > What > > > > would be the reason for this to fail? I'd rather see a DCHECK or CHECK if > > it's > > > > not something we expect to happen. > > > > > > No good reason is probably accurate for gbm, but I think it's probably ideal > > to treat import failures the same as allocation failures in the API (which may > > have other implementations). ChildGpuMemoryBufferManager is already prepared > for > > import failures and responds by deleting the buffer. Do you feel strongly > about > > using a DCHECK()? > > > > Failing to create the buffer will result in the compositor failing to produce > > new contents. That's fine (and necessary) in a lost context situation but > > results in very hard to diagnose effects for other types of failures. It's > much > > better if other failures such as OOM will cause a DCHECK or CHECK to fail > close > > to the source of the error. > > > > FYI, GpuMemoryBufferImplSharedMemory::CreateFromHandle should be updated to > fail > > hard on OOM errors instead of returning null. OOM errors there are currently > > going unnoticed and causing very confusing results. > > Ok I have changed it to a DCHECK. jln: ping
dongseong.hwang@intel.com changed reviewers: + dcheng@chromium.org
On 2015/08/17 18:31:14, spang wrote: > jln: ping jln seems to be afk. dcheng, could you review IPC such as content/common/child_process_messages.h and ui/gfx/ipc/gfx_param_traits_macros.h ?
https://codereview.chromium.org/1263323004/diff/190001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1263323004/diff/190001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:34: return make_scoped_ptr<GpuMemoryBufferImpl>( Note: not caused by this patch, but the idea of using make_scoped_ptr is that the template type is deduced, so you don't need to explicitly instantiate it with the type. e.g. make_scoped_ptr(new GpuMemoryBufferImplOzoneNativePixmap(...)) is sufficient. https://codereview.chromium.org/1263323004/diff/190001/ui/gfx/native_pixmap_h... File ui/gfx/native_pixmap_handle_ozone.h (right): https://codereview.chromium.org/1263323004/diff/190001/ui/gfx/native_pixmap_h... ui/gfx/native_pixmap_handle_ozone.h:12: struct NativePixmapHandle { Is this class copyable? It seems quite dangerous to copy a base::FileDescriptor object with auto_close == true. https://codereview.chromium.org/1263323004/diff/190001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1263323004/diff/190001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:47: base::ScopedFD scoped_fd(handle.fd.fd); Is this getting invoked in response to an IPC? base::FileDescriptor's ParamTraits has this to say: / The received file descriptor will have the |auto_close| flag set to true. The // code which handles the message is responsible for taking ownership of it. // File descriptors are OS resources and must be closed when no longer needed. So doesn't this cause a double close()? That seems bad. https://codereview.chromium.org/1263323004/diff/190001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:48: return make_scoped_ptr<ClientNativePixmapGbm>(new ClientNativePixmapGbm); Nit: make_scoped_ptr(new ClientNativePixmapGbm) Also, this doesn't appear to be consuming any of the arguments? Is that expected? https://codereview.chromium.org/1263323004/diff/190001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1263323004/diff/190001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:108: handle.fd = base::FileDescriptor(base::ScopedFD(dmabuf_fd)); FWIW, I personally don't think having a ScopedFD intermediate here is helpful (though it's true that the boolean parameter of the raw FD version is not very clear)
Actually, apparently the auto_close flag doesn't mean what I thought it did. I'm going to have to think about my comments about fd usage a bit more.
https://codereview.chromium.org/1263323004/diff/190001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc (right): https://codereview.chromium.org/1263323004/diff/190001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc:34: return make_scoped_ptr<GpuMemoryBufferImpl>( On 2015/08/18 18:43:25, dcheng wrote: > Note: not caused by this patch, but the idea of using make_scoped_ptr is that > the template type is deduced, so you don't need to explicitly instantiate it > with the type. e.g. make_scoped_ptr(new > GpuMemoryBufferImplOzoneNativePixmap(...)) is sufficient. Done. https://codereview.chromium.org/1263323004/diff/190001/ui/gfx/native_pixmap_h... File ui/gfx/native_pixmap_handle_ozone.h (right): https://codereview.chromium.org/1263323004/diff/190001/ui/gfx/native_pixmap_h... ui/gfx/native_pixmap_handle_ozone.h:12: struct NativePixmapHandle { On 2015/08/18 18:43:25, dcheng wrote: > Is this class copyable? It seems quite dangerous to copy a base::FileDescriptor > object with auto_close == true. Should be fine to copy with auto_close := true fine as long as you don't write to more than one IPC::Message. I'm not sure if we can make it not copyable. Doesn't IPC require copyable parameters? https://codereview.chromium.org/1263323004/diff/190001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc (right): https://codereview.chromium.org/1263323004/diff/190001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:47: base::ScopedFD scoped_fd(handle.fd.fd); On 2015/08/18 18:43:25, dcheng wrote: > Is this getting invoked in response to an IPC? > > base::FileDescriptor's ParamTraits has this to say: > / The received file descriptor will have the |auto_close| flag set to true. The > // code which handles the message is responsible for taking ownership of it. > // File descriptors are OS resources and must be closed when no longer needed. > > So doesn't this cause a double close()? That seems bad. Right, we are required to take ownership. Putting it into ScopedFD is how we accomplish that. Doesn't cause a double close since auto_close is just a hint to the recipient and is known to be true here (per the comment you cited). https://codereview.chromium.org/1263323004/diff/190001/ui/ozone/platform/drm/... ui/ozone/platform/drm/common/client_native_pixmap_factory_gbm.cc:48: return make_scoped_ptr<ClientNativePixmapGbm>(new ClientNativePixmapGbm); On 2015/08/18 18:43:26, dcheng wrote: > Nit: make_scoped_ptr(new ClientNativePixmapGbm) > > Also, this doesn't appear to be consuming any of the arguments? Is that > expected? See the next patch for non-stub implementation: https://codereview.chromium.org/1134993003/ https://codereview.chromium.org/1263323004/diff/190001/ui/ozone/platform/drm/... File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right): https://codereview.chromium.org/1263323004/diff/190001/ui/ozone/platform/drm/... ui/ozone/platform/drm/gpu/gbm_buffer.cc:108: handle.fd = base::FileDescriptor(base::ScopedFD(dmabuf_fd)); On 2015/08/18 18:43:26, dcheng wrote: > FWIW, I personally don't think having a ScopedFD intermediate here is helpful > (though it's true that the boolean parameter of the raw FD version is not very > clear) Done.
So... I think the current patch is OK. But I also think that base::FileDescriptor is simply too error-prone. Out of curiosity, how much of a burden would it be if NativePixmapHandle wasn't actually copyable? Note that there's nothing about the IPC subsystem that actually requires the types be copyable. I'm thinking about adding a ParamTraits specialization for ScopedFD...
On 2015/08/19 05:38:13, dcheng wrote: > So... > > I think the current patch is OK. But I also think that base::FileDescriptor is > simply too error-prone. I agree that base::FileDescriptor's design is error prone. I'd be happy to have something better. > > Out of curiosity, how much of a burden would it be if NativePixmapHandle wasn't > actually copyable? Note that there's nothing about the IPC subsystem that > actually requires the types be copyable. I'm thinking about adding a ParamTraits > specialization for ScopedFD... Shrug, doesn't matter to me if it's copyable or just movable. It's always the latter in practice for the exact reason you've raised.
Per our discussion, IPC changes LGTM but let's work on improving the fd situation innfollowup patches. Daniel
The CQ bit was checked by spang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, dongseong.hwang@intel.com, sadrul@chromium.org, sievers@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1263323004/#ps230001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1263323004/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1263323004/230001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by spang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, dongseong.hwang@intel.com, dcheng@chromium.org, sievers@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1263323004/#ps250001 (title: "fix cast build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1263323004/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1263323004/250001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by spang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1263323004/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1263323004/250001
Message was sent while issue was closed.
Committed patchset #14 (id:250001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/9b0433a3d10440d8b90cc7fd99d0203e1ead4d28 Cr-Commit-Position: refs/heads/master@{#344671} |
