GNOME Bugzilla – Bug 779142
mirsink implementation for Mir server
Last modified: 2017-08-27 17:42:39 UTC
Created attachment 346585 [details] [review] mirsink patch This is an initial approach for a sink for the Mir server. I am not proposing yet, but opening for comments and suggestions. The mirsink works in two modes: if "export-buffers" property is set, it exports dma buffers that are sent to the application by posting a GstMessage. If not, it creates its own rendering Mir window. The idea behind exporting dma buf fds is to be able to pass them around to another process so they can be by it.
Review of attachment 346585 [details] [review]: ::: ext/mir/gstmirsink.c @@ +46,3 @@ +{ + SIGNAL_0, + LAST_SIGNAL You don't have signals, remove this then. @@ +107,3 @@ + + gst_element_class_add_pad_template (gstelement_class, + gst_static_pad_template_get (&gst_mirsink_sink_caps_template)); Use gst_element_class_add_static_pad_template() in newly written code. @@ +115,3 @@ + + //gstelement_class->change_state = + // GST_DEBUG_FUNCPTR (gst_mir_sink_change_state); Did you forget some cleanup ? @@ +122,3 @@ + gstbasesink_class->start = GST_DEBUG_FUNCPTR (gst_mir_sink_start); + gstbasesink_class->stop = GST_DEBUG_FUNCPTR (gst_mir_sink_stop); + gstbasesink_class->preroll = GST_DEBUG_FUNCPTR (gst_mir_sink_preroll); You should implement GstVideoSinkClass::show_frame instead, this way the property “show-preroll-frame” will work. @@ +129,3 @@ + + g_object_class_install_property (gobject_class, PROP_MIR_EXPORT_BUFFERS, + g_param_spec_boolean ("export-buffers", "Export buffers flag", Maybe a name that clearly state this will propose dmabuf ? Why would you disable this btw ? @@ +141,3 @@ + g_param_spec_uint ("width", "Video Width", + "Width of each video frame", 0, + UINT_MAX, 0, G_PARAM_READABLE | G_PARAM_STATIC_STRINGS)); Height/width property looks weird to me. What is it meant for ? Why is this needed ? It ways video width/height, which normally means the width/height found in caps. Is this somehow the window size or something ? @@ +246,3 @@ + + intersection = + gst_caps_intersect_full (filter, caps, GST_CAPS_INTERSECT_FIRST); Remove this, the default will pick the template for you already. I understand the Mir only support 1 native format which you color convert in GL. I believe a clean bridge with libgstgl would be required here. @@ +657,3 @@ + GstMirSink *sink = (GstMirSink *) bsink; + + GST_DEBUG_OBJECT (sink, "start"); Empty, remove. @@ +748,3 @@ + * so let's propose it anyway, but also propose the allocator on its own + */ + gst_query_add_allocation_pool (query, sink->pool, size, min_bufs, max_bufs); Never offer the same pool twice, it will break renegotiation. This was fixed in all upstream videosink recently. @@ +749,3 @@ + */ + gst_query_add_allocation_pool (query, sink->pool, size, min_bufs, max_bufs); + gst_query_add_allocation_param (query, gst_allocator_find (NULL), NULL); There is no reason no to enable GstVideoMeta (strides and offsets). gst_query_add_allocation_meta (query, GST_VIDEO_META_API_TYPE, NULL); @@ +1051,3 @@ + return GST_FLOW_ERROR; + + gst_buffer_map (buffer, &src, GST_MAP_READ); Use GstVideoFrame API here. Always check return value for any kind of _map() calls. @@ +1086,3 @@ + gst_element_post_message (GST_ELEMENT (sink), + gst_message_new_custom (GST_MESSAGE_ELEMENT, GST_OBJECT (sink), + gst_structure_new_empty ("frame-ready"))); Now that I read this, have no idea what export_buffers is doing here. I think you'll have to explain. Custom message need to be documented in the header. Clearly is has nothing to do with doing zero-copy with upstream, which is what I would expect. I thought you would propose a pool that allocate DMABuf using Mir extensions, so they can be imported, or at least rendered zero-copy if there was a software decoder. Does this mechanism skips the rendering completly ?
(In reply to alfonso.sanchez-beato from comment #0) > The mirsink works in two modes: if "export-buffers" property is set, it > exports dma buffers that are sent to the application by posting a > GstMessage. If not, it creates its own rendering Mir window. > > The idea behind exporting dma buf fds is to be able to pass them around to > another process so they can be by it. This should be solved differently. You probably want to just use an appsink or similar for that, or a "mirupload" element followed by an appsink (and have mirupload be used inside mirsink). Sending the messages like this is not really ideal from an API usage point of view.
@stormer, @slomo, thanks for the reviews. About dma-bufs, the idea is to not do any rendering here in a next version, that is, removing the rendering I am doing here in the framebuffer object and instead pass a decoded buffer to the application, that will in turn export it using the dma-buf fd to another process that will directly render+perform YUV->RGB conversion. Said this, I do not quite understand how could use appsink to grab the dma-buf meta data. How would that work exactly? I will probably refactor all this a bit and re-send.
Created attachment 346886 [details] [review] mirsink v2 New patch, addressing most of the comments.
(In reply to Nicolas Dufresne (stormer) from comment #1) > Review of attachment 346585 [details] [review] [review]: Thanks for the review, I have addressed most of the comments. See also responses below. > > ::: ext/mir/gstmirsink.c > @@ +46,3 @@ > +{ > + SIGNAL_0, > + LAST_SIGNAL > > You don't have signals, remove this then. > > @@ +107,3 @@ > + > + gst_element_class_add_pad_template (gstelement_class, > + gst_static_pad_template_get (&gst_mirsink_sink_caps_template)); > > Use gst_element_class_add_static_pad_template() in newly written code. > > @@ +115,3 @@ > + > + //gstelement_class->change_state = > + // GST_DEBUG_FUNCPTR (gst_mir_sink_change_state); > > Did you forget some cleanup ? > > @@ +122,3 @@ > + gstbasesink_class->start = GST_DEBUG_FUNCPTR (gst_mir_sink_start); > + gstbasesink_class->stop = GST_DEBUG_FUNCPTR (gst_mir_sink_stop); > + gstbasesink_class->preroll = GST_DEBUG_FUNCPTR (gst_mir_sink_preroll); > > You should implement GstVideoSinkClass::show_frame instead, this way the > property “show-preroll-frame” will work. > > @@ +129,3 @@ > + > + g_object_class_install_property (gobject_class, PROP_MIR_EXPORT_BUFFERS, > + g_param_spec_boolean ("export-buffers", "Export buffers flag", > > Maybe a name that clearly state this will propose dmabuf ? Why would you > disable this btw ? I would like to keep the name kind of tech-independent. It is disabled by default so a new window is created if not set explicitly. This way the sink can render a video when used with gst-launch. > > @@ +141,3 @@ > + g_param_spec_uint ("width", "Video Width", > + "Width of each video frame", 0, > + UINT_MAX, 0, G_PARAM_READABLE | G_PARAM_STATIC_STRINGS)); > > Height/width property looks weird to me. What is it meant for ? Why is this > needed ? It ways video width/height, which normally means the width/height > found in caps. Is this somehow the window size or something ? Actually the video size, I have removed these properties in the new version as they are certainly not needed. > > @@ +246,3 @@ > + > + intersection = > + gst_caps_intersect_full (filter, caps, GST_CAPS_INTERSECT_FIRST); > > Remove this, the default will pick the template for you already. I > understand the Mir only support 1 native format which you color convert in > GL. I believe a clean bridge with libgstgl would be required here. > > @@ +657,3 @@ > + GstMirSink *sink = (GstMirSink *) bsink; > + > + GST_DEBUG_OBJECT (sink, "start"); > > Empty, remove. > > @@ +748,3 @@ > + * so let's propose it anyway, but also propose the allocator on its own > + */ > + gst_query_add_allocation_pool (query, sink->pool, size, min_bufs, > max_bufs); > > Never offer the same pool twice, it will break renegotiation. This was fixed > in all upstream videosink recently. > > @@ +749,3 @@ > + */ > + gst_query_add_allocation_pool (query, sink->pool, size, min_bufs, > max_bufs); > + gst_query_add_allocation_param (query, gst_allocator_find (NULL), NULL); > > There is no reason no to enable GstVideoMeta (strides and offsets). > > gst_query_add_allocation_meta (query, GST_VIDEO_META_API_TYPE, NULL); > > @@ +1051,3 @@ > + return GST_FLOW_ERROR; > + > + gst_buffer_map (buffer, &src, GST_MAP_READ); > > Use GstVideoFrame API here. Always check return value for any kind of _map() > calls. > > @@ +1086,3 @@ > + gst_element_post_message (GST_ELEMENT (sink), > + gst_message_new_custom (GST_MESSAGE_ELEMENT, GST_OBJECT (sink), > + gst_structure_new_empty ("frame-ready"))); > > Now that I read this, have no idea what export_buffers is doing here. I > think you'll have to explain. Custom message need to be documented in the > header. Clearly is has nothing to do with doing zero-copy with upstream, > which is what I would expect. I thought you would propose a pool that > allocate DMABuf using Mir extensions, so they can be imported, or at least > rendered zero-copy if there was a software decoder. Does this mechanism > skips the rendering completly ? Eventually that will be the case, the sink will propose a pool of DMA buffers to avoid copies, and will also offer VAbuffers as possible input to make possible to work with vaapidecode so we have HW decoding too. In that scenario, the buffers will be sent to the application without further rendering, so YUV->RGB conversion is handled elsewhere. This plugin is an intermediate point until I have that. I have documented the messages in the header now.
(In reply to Sebastian Dröge (slomo) from comment #2) > (In reply to alfonso.sanchez-beato from comment #0) Thanks for the comments. > > > The mirsink works in two modes: if "export-buffers" property is set, it > > exports dma buffers that are sent to the application by posting a > > GstMessage. If not, it creates its own rendering Mir window. > > > > The idea behind exporting dma buf fds is to be able to pass them around to > > another process so they can be by it. > > This should be solved differently. You probably want to just use an appsink > or similar for that, or a "mirupload" element followed by an appsink (and > have mirupload be used inside mirsink). > > Sending the messages like this is not really ideal from an API usage point > of view. I understand using appsink would let me use standard messages, but for the moment I prefer to keep the code that does the buffer export in the sink instead of in the app.
(In reply to alfonso.sanchez-beato from comment #6) > I understand using appsink would let me use standard messages, but for the > moment I prefer to keep the code that does the buffer export in the sink > instead of in the app. You misunderstood then. There would be some kind of mirupload element that does the dmabuf exporting, and in appsink you just get the GstBuffer. Having such a custom element message upstream doesn't seem great, it's a non-standard interface for something that we usually handle different.
How do recent developments in Ubuntu affect this? (Unity-8 to be abandoned in favour of GNOME, using Wayland instead of Mir) Even if Mir lives on for various Canonical IoT efforts (as per quote on wikipedia), it seems to me that such a sink would likely be better maintained in a canonical gst-mir respository.
@Tim we have no intention to upstream this at the moment, so please go ahead an put the bug in the state that you consider adequate.