[Bug 779142] mirsink implementation for Mir server

[GStreamer (GNOME Bugzilla)]

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

Nicolas Dufresne (stormer) <nicolas at ndufresne.ca> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #346585|none                        |needs-work
             status|                            |

--- Comment #1 from Nicolas Dufresne (stormer) <nicolas at ndufresne.ca> ---
Review of attachment 346585:
 --> (https://bugzilla.gnome.org/review?bug=779142&attachment=346585)

::: 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
?

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