[Bug 779142] mirsink implementation for Mir server

[GStreamer (GNOME Bugzilla)]

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

--- Comment #5 from alfonso.sanchez-beato at canonical.com ---
(In reply to Nicolas Dufresne (stormer) from comment #1)
> Review of attachment 346585 [details] [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.

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