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

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=725187
  GStreamer | gst-plugins-bad | git

Nicolas Dufresne (stormer) <nicolas.dufresne> changed:

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

--- Comment #18 from Nicolas Dufresne (stormer) <nicolas.dufresne at collabora.co.uk> 2014-11-10 02:46:44 UTC ---
Review of attachment 290298:
 --> (https://bugzilla.gnome.org/review?bug=725187&attachment=290298)

Here's few comments, generally speaking it's quite clean. I think this element
can be useful and fill a certain gap. It will obviously need consensus across
GStreamer maintainers. For this reason, don't worry if it takes a little time.

::: gst/multiappsrc/gstmultiappsrc.c
@@ +84,3 @@
+#define parent_class gst_multi_appsrc_parent_class
+
+#define DEFAULT_PROP_URI NULL

That a little weird.

@@ +146,3 @@
+      g_param_spec_string ("uri", "URI",
+          "URI to get protected content",
+          NULL, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

Specially that you are not using it here.

@@ +184,3 @@
+      g_signal_new ("end-of-stream", G_TYPE_FROM_CLASS (klass),
+      G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION, G_STRUCT_OFFSET
(GstMultiAppsrcClass,
+          end_of_stream), NULL, NULL, g_cclosure_marshal_generic,

Marshaler can be set to NULL with same effect.

@@ +195,3 @@
+  gst_element_class_set_static_metadata (gstelement_class,
+      "MultiAppsrc", "Source/Bin",
+      "Multiple App Sources", "Wonchul Lee <wonchul86.lee at lge.com>");

Short description could be a little better, remember this is the first thing
people see in gst-inspect-1.0, they should figure-out what this is used for.

@@ +205,3 @@
+{
+  /* init member variable */
+  bin->uri = g_strdup (DEFAULT_PROP_URI);

Maybe just drop this define ?

@@ +207,3 @@
+  bin->uri = g_strdup (DEFAULT_PROP_URI);
+  bin->appsrc_list = NULL;
+  bin->n_source = 0;

I don't mind keeping, but not that all object are allocated and initialized
with 0. So no real need to set things to NULL or 0 here.

@@ +292,3 @@
+  GstMultiAppsrc *bin = GST_MULTI_APPSRC (parent);
+  GstIterator *it;
+  GValue data = { 0, };

Use data = G_VALUE_INIT;

@@ +299,3 @@
+  if (GST_EVENT_TYPE (event) == GST_EVENT_SEEK) {
+    it = gst_element_iterate_src_pads (GST_ELEMENT_CAST (bin));
+    while (gst_iterator_next (it, &data) == GST_ITERATOR_OK) {

The isn't correct way of handling lockless iteration. You need to handle resync
return value. It will be return if the list have changed.

@@ +319,3 @@
+
+static gboolean
+setup_source (GstMultiAppsrc * bin)

setup_sources would be more correct. Also, please add a comment /* must be
called with OBJECT_LOCK */, so nobody get tricked.

@@ +363,3 @@
+
+static void
+remove_source (GstMultiAppsrc * bin)

remove_sources would also be more correct. Also a comment about OBJECT_LOCK

@@ +432,3 @@
+  GList *item;
+
+  for (item = bin->appsrc_list; item; item = g_list_next (item)) {

This list may change is state is lower then PLAYING, so this code need some
protection.

@@ +440,3 @@
+        GST_ELEMENT_NAME (appsrc_group->appsrc), gst_flow_get_name (ret));
+    if (ret != GST_FLOW_OK)
+      break;

I'm not completly sure this will always be correct. If 1 appsrc isn't link, you
probably don't want to stop sending EOS. Maybe make a test where you have
subtitle, but subtitle disable, to make sure this is all right ?

@@ +454,3 @@
+  switch (transition) {
+    case GST_STATE_CHANGE_READY_TO_PAUSED:
+      if (!setup_source (bin))

Need to protect this somehow.

@@ +474,3 @@
+      break;
+    case GST_STATE_CHANGE_READY_TO_NULL:
+      remove_source (bin);

This is not needed, states are never skipped.

@@ +498,3 @@
+gst_multi_appsrc_uri_get_protocols (GType type)
+{
+  // dynappsrc protocal is used internally.

Don't use C++ style comment. What do you mean used internally ?

@@ +500,3 @@
+  // dynappsrc protocal is used internally.
+  static const gchar *protocols[] = { "multiappsrc",
+      "dynappsrc", NULL };

Should be multiappsrc only now.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.