[Bug 734908] [API] Add gstgralloc library

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=734908
  GStreamer | gst-plugins-bad | git

--- Comment #4 from Mohammed Sameer <msameer at foolab.org> 2014-08-27 19:41:47 UTC ---
(In reply to comment #3)
> Review of attachment 283596 [details]:
> 
> ::: 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.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.