[Bug 725187] Add new multiappsrc or dynappsrc element with multiple output streams

[GStreamer (GNOME Bugzilla)]

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

Julien Isorce <julien.isorce at gmail.com> changed:

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

--- Comment #49 from Julien Isorce <julien.isorce at gmail.com> ---
Review of attachment 335415:
 --> (https://bugzilla.gnome.org/review?bug=725187&attachment=335415)

I reviewed only gstmultiappsrc.c , see my comments, maybe add support for meson
build too.
Functionality wise it looks overall good. But I have not been into deep details
for all of it.
That is great that there are some unit tests.
I think the most important bits for now is functionalities (API, signals,
properties)
and providing that it generally works, that is fine for me.
What is your use case btw ? On my side I plan to use this element for MSE
(Media Source Extension) in the future.
So +1 from me with the few remarks addressed, but I would like someone else to
review it as well before to land it.

::: gst-libs/gst/multiapp/gstmultiappsrc.c
@@ +236,3 @@
+typedef struct _GhostPadInfo GhostPadInfo;
+
+struct _GstAppSrcGroup

This name is confusing, it sounds like a group of GstAppSrc whereas it is a
hash map entry.
Maybe do not start it with GstAppSrc, ex: SrcHolder or SrcEntry or ...

@@ +258,3 @@
+                                 * Don't modify this group data by GstPadInfo
*/
+  GstPad *target;               /* target (or to be linked) srcpad of appsrc
*/
+};

Can't just "GstPad *target" be part of GstAppSrcGroup ?

@@ +290,3 @@
+    gpointer user_data);
+
+static GstAppSrcCallbacks appsrc_callbacks;

Please remove this static var, just use a local one when needed.

@@ +333,3 @@
+    }
+    group->pending_pad = NULL;
+  }

Can you make this block a function, then call it with srcpad and pending_pad.

@@ +369,3 @@
+{
+  g_return_val_if_fail (group, FALSE);
+  g_return_val_if_fail (group->appsrc, FALSE);

We use these guards only for API, so for function declared in the header. So
use g_assert here instead.

@@ +735,3 @@
+  appsrc_callbacks.need_data = appsrc_need_data_cb;
+  appsrc_callbacks.enough_data = appsrc_enough_data_cb;
+  appsrc_callbacks.seek_data = appsrc_seek_data_cb;

Remove these 3 lines

@@ +799,3 @@
+  if (priv->notify)
+    priv->notify (priv->user_data);
+  priv->notify = NULL;

Put this into brackets as you did below.

@@ +801,3 @@
+  priv->notify = NULL;
+
+  dispose_reconfig_loop (multiappsrc);

Why not remove_sources ?

@@ +1123,3 @@
+
+  g_return_val_if_fail (group, GST_FLOW_ERROR);
+  g_return_val_if_fail (group->appsrc, GST_FLOW_ERROR);

Use g_assert, see other comment and please check other places.

@@ +1174,3 @@
+    case GST_STATE_CHANGE_READY_TO_PAUSED:
+      setup_sources (bin);
+      gst_task_start (priv->reconfig_task);

Should you move the task start to setup sources ? To be symmetric with
remove_sources ? Maybe choose symmetrical names as well.

@@ +1478,3 @@
+
+  gst_app_src_set_callbacks (GST_APP_SRC (group->appsrc), &appsrc_callbacks,
+      group, NULL);

Just define a local GstAppSrcCallbacks.

@@ +1483,3 @@
+  gst_multi_appsrc_set_format_internal (multiappsrc, group);
+
+  group->multiappsrc = gst_object_ref (multiappsrc);

Is this one really necessary ? Since it is added to the multiappsrc bin anyway.

@@ +2108,3 @@
+  expose_group (multiappsrc, group, TRUE);
+
+  ret = gst_app_src_end_of_stream (GST_APP_SRC (group->appsrc));

why ?

@@ +2412,3 @@
+  MULTI_APPSRC_UNLOCK (multiappsrc);
+
+  emit = priv->emit_signals;

move this up to call it while the lock is taken like in other helper above.

@@ +2463,3 @@
+    priv->user_data = NULL;
+    priv->notify = NULL;
+    MULTI_APPSRC_LOCK (multiappsrc);

Looking at gstappsrc.c this should be unlock then lock, see
https://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst-libs/gst/app/gstappsrc.c#n1987

@@ +2517,3 @@
+
+  MULTI_APPSRC_LOCK (multiappsrc);
+  g_free (priv->uri);

This one is ok because g_free is allowed with NULL param.

@@ +2518,3 @@
+  MULTI_APPSRC_LOCK (multiappsrc);
+  g_free (priv->uri);
+  priv->uri = g_strdup (uri);

Should be priv->uri = uri ? g_strdup (uri) : NULL;

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