[Bug 711155] wayland: add wl_drm support to wayland sink

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=711155

--- Comment #43 from Benjamin Gaignard <benjamin.gaignard at gmail.com> ---
(In reply to Nicolas Dufresne (stormer) from comment #42)
> Review of attachment 310472 [details] [review]:
> 
> I don't think the approach is right. This seem to track GstBuffer instead of
> GstMemory. About the ifdef, I might have miss-read (and be wrong), but I
> believe it's not because you have a release that support dmabuf that this
> protocol is implement in the compositor. It also seems to assume that all
> DMABUF will be backed by DRM Dumb.

No we track which allocator has been selected in pool set_config() 
self->use_wl_dmabuf = (self->allocator
      && g_strcmp0 (self->allocator->mem_type, GST_ALLOCATOR_DMABUF) == 0);
when we use this to allocate the buffer either with shm or dumb buffer.
We only have use dumb buffer because it what we need on our platform.
We have put dmabuf protocol code in wayland and implement it on weston, to be
sure that functions prototype exist we check if the headers are present or not
in package configuration.
Maybe we have put to much #ifdef HAVE_WAYLAND_DMABUF but at least it indicate
us where we have adding code.

> ::: ext/wayland/gstwaylandsink.c
> @@ +586,3 @@
>      config = gst_buffer_pool_get_config (pool);
>      gst_buffer_pool_config_set_params (config, caps, size, 2, 0);
> +    gst_buffer_pool_config_set_allocator (config, NULL, &params);
> 
> Why bother passing allocation params if you don't have any alignment
> requirement ?

because gst_buffer_pool_config_set_allocator() do not accept to have the both
parameters NULL and was needed for pool initialization.

> ::: ext/wayland/waylandpool.c
> @@ +38,3 @@
>  #include <sys/types.h>
>  
> +#ifdef HAVE_WAYLAND_DMABUF
> 
> I think this should mostly be run-time. It would be sad if we cannot use the
> element at all on a compositor that does not implement this protocol.

I don't understand ... this #ifdef only cover protocol included files.

> @@ +78,3 @@
> +    memset (&destroy_arg, 0, sizeof destroy_arg);
> +    drmPrimeFDToHandle (meta->drm_fd, prime_fd, &destroy_arg.handle);
> +    drmIoctl (meta->drm_fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_arg);
> 
> What if the dmabuf was not DRM dumb ?

In your case dmabuf always means dumb buffer.
It is a short cut we have done for your implementation but we can imagine have
something like v4l2 plugins io-mode to select allocation method.

> 
> ::: ext/wayland/wlvideoformat.c
> @@ +63,3 @@
> +#ifdef HAVE_WAYLAND_DMABUF
> +  {WL_DMABUF_FORMAT_XRGB8888, GST_VIDEO_FORMAT_BGRx},
> +  {WL_DMABUF_FORMAT_ARGB8888, GST_VIDEO_FORMAT_BGRA},
> 
> And no YUV format ? Without any YUV format, it's a bit pointless to have
> this in GStreamer.
Of course we have YUV formats but it is an other patch and link to dmabuf
usage.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.