GNOME Bugzilla – Bug 734908
[API] Add gstgralloc library
Last modified: 2018-11-03 13:26:10 UTC
Created attachment 283596 [details] [review] proposed patch Attaching a patch that adds a GstGralloc allocator with its GstGrallocMemory and also a buffer pool (GstGrallocBufferPool) for usage with elements utilizing the above mentioned allocator. gralloc is the library used by Android to allocate android native buffers used for graphics rendering and zero copy media decoding (Probably it has other uses as well). The only issue I see with the patch is I had to lie and set the format to GST_VIDEO_FORMAT_I420 because I cannot use GST_VIDEO_FORMAT_ENCODED since it's size is 0 and various GStreamer API dealing with allocation queries expect size which is not 0 The allocator will be used to add support to gst-omx (And perhaps glimagesink too)
Probably should add a new format then.
I think the same problem was also there for gstreamer-vaapi. We probably need something like an "opaque" video format that is only allowed to be used with custom caps features and that can have size==0.
Review of attachment 283596 [details] [review]: ::: gst-libs/gst/Makefile.am @@ +17,2 @@ SUBDIRS = interfaces basecamerabinsrc codecparsers \ + insertbin uridownloader mpegts base video $(GL_DIR) $(WAYLAND_DIR) $(GRALLOC_DIR) Also it needs to be added to DIST_SUBDIRS ::: gst-libs/gst/gralloc/gstgralloc.c @@ +97,3 @@ + +GstAllocator * +gst_gralloc_allocator_new (void) Shouldn't the gralloc device (and module?) be a parameter of the constructor? @@ +115,3 @@ + + alloc->mem_map = NULL; + alloc->mem_unmap = NULL; map could be implemented with lock(), no? @@ +117,3 @@ + alloc->mem_unmap = NULL; + alloc->mem_copy = NULL; + alloc->mem_share = NULL; There is no copy or sharing (via refcounting!) support in gralloc? @@ +161,3 @@ + GstVideoInfo info; + + if (!GST_IS_GRALLOC_ALLOCATOR (allocator)) { Should be a g_return_val_if_fail() as it's a programming error ::: gst-libs/gst/gralloc/gstgrallocbufferpool.c @@ +39,3 @@ +gst_gralloc_buffer_pool_get_options (GstBufferPool * pool) +{ + static const gchar *options[] = { GST_BUFFER_POOL_OPTION_VIDEO_META, NULL Why do you need the videometa at all for an opaque format?
(In reply to comment #3) > Review of attachment 283596 [details] [review]: > > ::: gst-libs/gst/Makefile.am > @@ +17,2 @@ > SUBDIRS = interfaces basecamerabinsrc codecparsers \ > + insertbin uridownloader mpegts base video $(GL_DIR) $(WAYLAND_DIR) > $(GRALLOC_DIR) > > Also it needs to be added to DIST_SUBDIRS Done. > ::: gst-libs/gst/gralloc/gstgralloc.c > @@ +97,3 @@ > + > +GstAllocator * > +gst_gralloc_allocator_new (void) > > Shouldn't the gralloc device (and module?) be a parameter of the constructor? I never found that to be necessary. It's enough to be able to get the buffer_handle_t and/or ANativeWindowBuffer. The allocator will open gralloc and obtain the needed allocator. I can still add a function that creates a GstGralloc allocator from an existing gralloc and allocator. > @@ +115,3 @@ > + > + alloc->mem_map = NULL; > + alloc->mem_unmap = NULL; > > map could be implemented with lock(), no? Not in all situations unfortunately. The problem is there is no proper way to obtain the size of the data exposed via lock(). I guess we can still calculate the size for formats such as RGB and maybe YUV. We unfortunately cannot do that for all vendor invented formats. > @@ +117,3 @@ > + alloc->mem_unmap = NULL; > + alloc->mem_copy = NULL; > + alloc->mem_share = NULL; > > There is no copy or sharing (via refcounting!) support in gralloc? I am not aware of any. buffer_handle_t is a set of file descriptors. ANativeWindowBuffer is what has ref counting but it comes to the picture only when we start rendering via GL. I was working around the bug that caused GStreamer to crash if the memory has no share functionality by simply reffing the memory and returning it. It's an ugly hack IMHO and I cannot tell whether it will work for all cases or not. > @@ +161,3 @@ > + GstVideoInfo info; > + > + if (!GST_IS_GRALLOC_ALLOCATOR (allocator)) { > > Should be a g_return_val_if_fail() as it's a programming error Done. > ::: gst-libs/gst/gralloc/gstgrallocbufferpool.c > @@ +39,3 @@ > +gst_gralloc_buffer_pool_get_options (GstBufferPool * pool) > +{ > + static const gchar *options[] = { GST_BUFFER_POOL_OPTION_VIDEO_META, NULL > > Why do you need the videometa at all for an opaque format? Because we still need width and height for rendering. That part is among the part I don't fully understand yet.
Created attachment 284633 [details] [review] patch V2 Updated patch: Use g_return_val_if_fail() Add gralloc to DIST_SUBDIRS
(In reply to comment #2) > I think the same problem was also there for gstreamer-vaapi. We probably need > something like an "opaque" video format that is only allowed to be used with > custom caps features and that can have size==0. Yup! I always thought that ENCODED is the way to go but I was apparently mistaken. I can try to add GST_VIDEO_FORMAT_OPAQUE. Should I create a separate bug for that?
Yes please. We'll also have to fix a few places to properly work with size==0 then btw.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/170.