GNOME Bugzilla – Bug 725187
Add new multiappsrc or dynappsrc element with multiple output streams
Last modified: 2018-11-03 13:21:56 UTC
if Audio/Video streams are separately given, playbin isn't able to handle two sources. This is a suggestion about how to deploy multiple appsrcs and to use with playbin. The basic idea started from here; http://lists.freedesktop.org/archives/gstreamer-devel/2014-January/045771.html Simple documentation is included in codes.
Created attachment 270341 [details] [review] dynappsrc codes with unit tests
Some of applications push each of elementary stream to pipeline, at that cases we should implement custom pipeline for it, like, pipeline appsrc - parser - decoder - sink (audio) appsrc - parser - decoder - sink (video) However we want to use playbin instead of create custom pipeline. It can be handled by using dynappsrc with playbin. Dynappsrc is for containing multiple appsrc element inside of single src bin. If I put the uri start with "dynappsrc://" protocol, then uridecodebin created dynappsrc element like appsrc. Then developer should create multiple appsrc by using "new-appsrc" signal action.
Created attachment 290125 [details] [review] add dynamic appsrc plugins for handling multiple appsrc fix some comments
Created attachment 290127 [details] dot graph of playbin with dynappsrc attach dot graph of playbin with dynappsrc
I'm very curious about this feature. Nobody except Wonchul and I has seen this kind of requirements, feeding separated streams?
It's something that is asked every now and then on IRC or the mailing list. But in general it's a better idea to create a custom bin with two specific source elements in these cases instead of using something like appsrc. Nonetheless I think having some kind of multiappsrc / multiappsink could be useful to have. I just shortly looked at your patch now, but what's the smart-properties property exactly doing? Seems like this should be split out of a first version as it doesn't seem to be required and is solving something separate?
I agree with creating a custom bin for specific sources, but I think it can be also true another event and query handler is required when making custom bin and deploying in playbin. :) Regarding smart-properties, it was posted mistakenly here. I will suggest the feature as a kind of proposal with another bugzilla ticket which Wonchul and I were using in my company internally.
Created attachment 290145 [details] [review] add dynamic appsrc plugins for handling multiple appsrc I remove some unnecessary code, like smart-properties.
Comment on attachment 290145 [details] [review] add dynamic appsrc plugins for handling multiple appsrc >From 15ba3c36f5aa60e8a4209b863583b088c9594321 Mon Sep 17 00:00:00 2001 >From: Wonchul Lee <wonchul86.lee@lge.com> >Date: Fri, 7 Nov 2014 08:45:41 +0900 >Subject: [PATCH] dynappsrc: Add new element to handle multiple appsrc element > >--- > configure.ac | 2 + > gst/dynappsrc/Makefile.am | 13 ++ > gst/dynappsrc/gstdynappsrc.c | 526 +++++++++++++++++++++++++++++++++++++++++++ > gst/dynappsrc/gstdynappsrc.h | 79 +++++++ > gst/dynappsrc/plugins.c | 45 ++++ > 5 files changed, 665 insertions(+) > create mode 100644 gst/dynappsrc/Makefile.am > create mode 100644 gst/dynappsrc/gstdynappsrc.c > create mode 100644 gst/dynappsrc/gstdynappsrc.h > create mode 100644 gst/dynappsrc/plugins.c > >diff --git a/configure.ac b/configure.ac >index fdb6c88..63101a1 100644 >--- a/configure.ac >+++ b/configure.ac >@@ -445,6 +445,7 @@ AG_GST_CHECK_PLUGIN(dccp) > AG_GST_CHECK_PLUGIN(debugutils) > AG_GST_CHECK_PLUGIN(dvbsuboverlay) > AG_GST_CHECK_PLUGIN(dvdspu) >+AG_GST_CHECK_PLUGIN(dynappsrc) > AG_GST_CHECK_PLUGIN(faceoverlay) > AG_GST_CHECK_PLUGIN(festival) > AG_GST_CHECK_PLUGIN(fieldanalysis) >@@ -3253,6 +3254,7 @@ gst/dccp/Makefile > gst/debugutils/Makefile > gst/dvbsuboverlay/Makefile > gst/dvdspu/Makefile >+gst/dynappsrc/Makefile > gst/faceoverlay/Makefile > gst/festival/Makefile > gst/fieldanalysis/Makefile >diff --git a/gst/dynappsrc/Makefile.am b/gst/dynappsrc/Makefile.am >new file mode 100644 >index 0000000..93abbae >--- /dev/null >+++ b/gst/dynappsrc/Makefile.am >@@ -0,0 +1,13 @@ >+plugin_LTLIBRARIES = libgstdynappsrc.la >+ >+# sources used to compile this plug-in >+libgstdynappsrc_la_SOURCES = plugins.c gstdynappsrc.c >+ >+# compiler and linker flags used to compile this plugin, set in configure.ac >+libgstdynappsrc_la_CFLAGS = $(GST_CFLAGS) >+libgstdynappsrc_la_LIBADD = $(GST_LIBS) -lgstvideo-@GST_API_VERSION@ -lgstaudio-@GST_API_VERSION@ >+libgstdynappsrc_la_LDFLAGS = $(GST_PLUGIN_LDFLAGS) >+libgstdynappsrc_la_LIBTOOLFLAGS = --tag=disable-static >+ >+# headers we need but don't want installed >+noinst_HEADERS = gstdynappsrc.h >diff --git a/gst/dynappsrc/gstdynappsrc.c b/gst/dynappsrc/gstdynappsrc.c >new file mode 100644 >index 0000000..ade2527 >--- /dev/null >+++ b/gst/dynappsrc/gstdynappsrc.c >@@ -0,0 +1,526 @@ >+/* GStreamer Dynamic App Source element >+ * Copyright (C) 2014 LG Electronics, Inc. >+ * Author : Wonchul Lee <wonchul86.lee@lge.com> >+ * >+ * This library is free software; you can redistribute it and/or >+ * modify it under the terms of the GNU Library General Public >+ * License as published by the Free Software Foundation; either >+ * version 2 of the License, or (at your option) any later version. >+ * >+ * This library is distributed in the hope that it will be useful, >+ * but WITHOUT ANY WARRANTY; without even the implied warranty of >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >+ * Library General Public License for more details. >+ * >+ * You should have received a copy of the GNU Library General Public >+ * License along with this library; if not, write to the >+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330, >+ * Boston, MA 02111-1307, USA. >+ */ >+ >+ >+/** >+ * SECTION:element-dynappsrc >+ * >+ * Dynappsrc provides multiple appsrc elements inside a single source bin >+ * for handling separated audio, video and text stream. >+ * >+ * <refsect2> >+ * <title>Usage</title> >+ * <para> >+ * A dynappsrc element is created by pipeline based on "dynappsrc://" protocal URI. >+ * >+ * Dynappsrc is #GstBin. An appsrc element is created by new-appsrc signal action >+ * to dynappsrc. >+ * >+ * When playback finished, received error or the application want to switch a >+ * different track, dynappsrc should be set back to READY or NULL state. >+ * Then appsrc elements in dynappsrc should be set to the NULL state and removed >+ * from it. >+ * </para> >+ * </refsect2> >+ * <refsect2> >+ * <title>Examples</title> >+ * |[ >+ * app->playbin = gst_element_factory_make ("playbin", NULL); >+ * g_object_set (app->playbin, "uri", "dynappsrc://", NULL); >+ * g_signal_connect (app->playbin, "deep-notify::source", >+ * (GCallback) found_source, app); >+ * ]| >+ * Create dynappsrc element in pipeline and watch source notify. >+ * |[ >+ * gst_object_ref (appsrc1); >+ * gst_object_ref (appsrc2); >+ * >+ * g_signal_emit_by_name (dynappsrc, "new-appsrc", &appsrc1); >+ * g_signal_emit_by_name (dynappsrc, "new-appsrc", &appsrc2); >+ * >+ * g_signal_connect (app->appsrc1, "need-data", G_CALLBACK (start_feed), app); >+ * g_signal_connect (app->appsrc1, "enough-data", G_CALLBACK (stop_feed), app); >+ * >+ * g_signal_connect (app->appsrc2, "need-data", G_CALLBACK (start_feed), app); >+ * g_signal_connect (app->appsrc2, "enough-data", G_CALLBACK (stop_feed), app); >+ * ]| >+ * Create appsrc elements when called source notify handler. >+ * </refsect2> >+ */ >+ >+#ifdef HAVE_CONFIG_H >+#include <config.h> >+#endif >+ >+#include <stdio.h> >+#include <string.h> >+#include <unistd.h> >+ >+#include "gstdynappsrc.h" >+ >+GST_DEBUG_CATEGORY_STATIC (dyn_appsrc_debug); >+#define GST_CAT_DEFAULT dyn_appsrc_debug >+ >+#define parent_class gst_dyn_appsrc_parent_class >+ >+#define DEFAULT_PROP_URI NULL >+ >+enum >+{ >+ PROP_0, >+ PROP_URI, >+ PROP_N_SRC, >+ PROP_LAST >+}; >+ >+enum >+{ >+ SIGNAL_NEW_APPSRC, >+ >+ /* actions */ >+ SIGNAL_END_OF_STREAM, >+ LAST_SIGNAL >+}; >+ >+static GstStaticPadTemplate src_template = GST_STATIC_PAD_TEMPLATE ("src_%u", >+ GST_PAD_SRC, >+ GST_PAD_SOMETIMES, >+ GST_STATIC_CAPS_ANY); >+ >+static guint gst_dyn_appsrc_signals[LAST_SIGNAL] = { 0 }; >+ >+static void gst_dyn_appsrc_set_property (GObject * object, guint prop_id, >+ const GValue * value, GParamSpec * pspec); >+static void gst_dyn_appsrc_get_property (GObject * object, guint prop_id, >+ GValue * value, GParamSpec * pspec); >+static void gst_dyn_appsrc_finalize (GObject * self); >+static GstStateChangeReturn gst_dyn_appsrc_change_state (GstElement * element, >+ GstStateChange transition); >+static GstElement *gst_dyn_appsrc_new_appsrc (GstDynAppSrc * bin, >+ const gchar * name); >+static void gst_dyn_appsrc_uri_handler_init (gpointer g_iface, >+ gpointer iface_data); >+ >+G_DEFINE_TYPE_WITH_CODE (GstDynAppSrc, gst_dyn_appsrc, GST_TYPE_BIN, >+ G_IMPLEMENT_INTERFACE (GST_TYPE_URI_HANDLER, >+ gst_dyn_appsrc_uri_handler_init)); >+ >+ >+static void >+gst_dyn_appsrc_class_init (GstDynAppSrcClass * klass) >+{ >+ GObjectClass *gobject_class; >+ GstElementClass *gstelement_class; >+ >+ gobject_class = G_OBJECT_CLASS (klass); >+ gstelement_class = GST_ELEMENT_CLASS (klass); >+ >+ gobject_class->set_property = gst_dyn_appsrc_set_property; >+ gobject_class->get_property = gst_dyn_appsrc_get_property; >+ gobject_class->finalize = gst_dyn_appsrc_finalize; >+ >+ gst_element_class_add_pad_template (gstelement_class, >+ gst_static_pad_template_get (&src_template)); >+ >+ g_object_class_install_property (gobject_class, PROP_URI, >+ g_param_spec_string ("uri", "URI", >+ "URI to get protected content", >+ NULL, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); >+ >+ /** >+ * GstDynAppSrc:n-src >+ * >+ * Get the total number of available streams. >+ */ >+ g_object_class_install_property (gobject_class, PROP_N_SRC, >+ g_param_spec_int ("n-source", "Number Source", >+ "Total number of source streams", 0, G_MAXINT, 0, >+ G_PARAM_READABLE | G_PARAM_STATIC_STRINGS)); >+ >+ /** >+ * GstDynAppSrc::new-appsrc >+ * @dynappsrc: a #GstDynAppSrc >+ * @name : name of appsrc element >+ * >+ * Action signal to create a appsrc element. >+ * This signal should be emitted before changing state READY to PAUSED. >+ * The application emit this signal as soon as receiving source-setup signal from pipeline. >+ * >+ * Returns: a GstElement of appsrc element or NULL when element creation failed. >+ */ >+ gst_dyn_appsrc_signals[SIGNAL_NEW_APPSRC] = >+ g_signal_new ("new-appsrc", G_TYPE_FROM_CLASS (klass), >+ G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION, >+ G_STRUCT_OFFSET (GstDynAppSrcClass, new_appsrc), NULL, NULL, >+ g_cclosure_marshal_generic, GST_TYPE_ELEMENT, 1, G_TYPE_STRING); >+ >+ /** >+ * GstDynAppSrc::end-of-stream: >+ * @dynappsrc: the dynappsrc >+ * >+ * Notify all of @appsrc that no more buffer are available. >+ */ >+ gst_dyn_appsrc_signals[SIGNAL_END_OF_STREAM] = >+ g_signal_new ("end-of-stream", G_TYPE_FROM_CLASS (klass), >+ G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION, G_STRUCT_OFFSET (GstDynAppSrcClass, >+ end_of_stream), NULL, NULL, g_cclosure_marshal_generic, >+ GST_TYPE_FLOW_RETURN, 0, G_TYPE_NONE); >+ >+ klass->new_appsrc = gst_dyn_appsrc_new_appsrc; >+ klass->end_of_stream = gst_dyn_appsrc_end_of_stream; >+ >+ gstelement_class->change_state = >+ GST_DEBUG_FUNCPTR (gst_dyn_appsrc_change_state); >+ >+ gst_element_class_set_static_metadata (gstelement_class, >+ "Dynappsrc", "Source/Bin", >+ "Dynamic App Source", "Wonchul Lee <wonchul86.lee@lge.com>"); >+ >+ GST_DEBUG_CATEGORY_INIT (dyn_appsrc_debug, "dynappsrc", 0, >+ "Dynamic App Source"); >+} >+ >+static void >+gst_dyn_appsrc_init (GstDynAppSrc * bin) >+{ >+ /* init member variable */ >+ bin->uri = g_strdup (DEFAULT_PROP_URI); >+ bin->appsrc_list = NULL; >+ bin->n_source = 0; >+ >+ GST_OBJECT_FLAG_SET (bin, GST_ELEMENT_FLAG_SOURCE); >+} >+ >+static void >+gst_dyn_appsrc_set_property (GObject * object, guint prop_id, >+ const GValue * value, GParamSpec * pspec) >+{ >+ GstDynAppSrc *bin = GST_DYN_APPSRC (object); >+ >+ switch (prop_id) { >+ case PROP_URI: >+ { >+ GstURIHandler *handler; >+ GstURIHandlerInterface *iface; >+ >+ handler = GST_URI_HANDLER (bin); >+ iface = GST_URI_HANDLER_GET_INTERFACE (handler); >+ iface->set_uri (handler, g_value_get_string (value), NULL); >+ } >+ break; >+ default: >+ G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); >+ break; >+ } >+} >+ >+static void >+gst_dyn_appsrc_get_property (GObject * object, guint prop_id, >+ GValue * value, GParamSpec * pspec) >+{ >+ GstDynAppSrc *bin = GST_DYN_APPSRC (object); >+ >+ switch (prop_id) { >+ case PROP_URI: >+ g_value_set_string (value, bin->uri); >+ break; >+ case PROP_N_SRC: >+ { >+ GST_OBJECT_LOCK (bin); >+ g_value_set_int (value, bin->n_source); >+ GST_OBJECT_UNLOCK (bin); >+ break; >+ } >+ default: >+ G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); >+ break; >+ } >+} >+ >+static void >+gst_dyn_appsrc_finalize (GObject * self) >+{ >+ GstDynAppSrc *bin = GST_DYN_APPSRC (self); >+ >+ g_free (bin->uri); >+ >+ G_OBJECT_CLASS (parent_class)->finalize (self); >+} >+ >+static gboolean >+gst_dyn_appsrc_handle_src_query (GstPad * pad, GstObject * parent, >+ GstQuery * query) >+{ >+ GstPad *target = gst_ghost_pad_get_target (GST_GHOST_PAD_CAST (pad)); >+ gboolean res = FALSE; >+ >+ /* forward the query to the proxy target pad */ >+ if (target) { >+ res = gst_pad_query (target, query); >+ gst_object_unref (target); >+ } >+ >+ return res; >+} >+ >+static gboolean >+gst_dyn_appsrc_handle_src_event (GstPad * pad, GstObject * parent, >+ GstEvent * event) >+{ >+ gboolean res = TRUE; >+ GstPad *target; >+ GstDynAppSrc *bin = GST_DYN_APPSRC (parent); >+ GstIterator *it; >+ GValue data = { 0, }; >+ >+ /* >+ * dynappsrc handle a seek event that it send to all of linked appsrce elements. >+ */ >+ 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) { >+ GstPad *srcpad = g_value_get_object (&data); >+ target = gst_ghost_pad_get_target (GST_GHOST_PAD_CAST (srcpad)); >+ res = gst_pad_send_event (target, gst_event_ref (event)); >+ gst_object_unref (target); >+ g_value_reset (&data); >+ } >+ gst_event_unref (event); >+ g_value_unset (&data); >+ gst_iterator_free (it); >+ } else if ((target = gst_ghost_pad_get_target (GST_GHOST_PAD_CAST (pad)))) { >+ res = gst_pad_send_event (target, event); >+ gst_object_unref (target); >+ } else >+ gst_event_unref (event); >+ >+ return res; >+} >+ >+static gboolean >+setup_source (GstDynAppSrc * bin) >+{ >+ GList *item; >+ GstPadTemplate *pad_tmpl; >+ gchar *padname; >+ gboolean ret = FALSE; >+ >+ pad_tmpl = gst_static_pad_template_get (&src_template); >+ >+ for (item = bin->appsrc_list; item; item = g_list_next (item)) { >+ GstAppSourceGroup *appsrc_group = (GstAppSourceGroup *) item->data; >+ GstPad *srcpad = NULL; >+ >+ gst_bin_add (GST_BIN_CAST (bin), appsrc_group->appsrc); >+ >+ srcpad = gst_element_get_static_pad (appsrc_group->appsrc, "src"); >+ padname = >+ g_strdup_printf ("src_%u", g_list_position (bin->appsrc_list, item)); >+ appsrc_group->srcpad = >+ gst_ghost_pad_new_from_template (padname, srcpad, pad_tmpl); >+ gst_pad_set_event_function (appsrc_group->srcpad, >+ gst_dyn_appsrc_handle_src_event); >+ gst_pad_set_query_function (appsrc_group->srcpad, >+ gst_dyn_appsrc_handle_src_query); >+ >+ gst_pad_set_active (appsrc_group->srcpad, TRUE); >+ gst_element_add_pad (GST_ELEMENT_CAST (bin), appsrc_group->srcpad); >+ >+ gst_object_unref (srcpad); >+ g_free (padname); >+ >+ ret = TRUE; >+ } >+ gst_object_unref (pad_tmpl); >+ >+ if (ret) { >+ GST_DEBUG_OBJECT (bin, "all appsrc elements are added"); >+ gst_element_no_more_pads (GST_ELEMENT_CAST (bin)); >+ } >+ >+ return ret; >+} >+ >+static void >+remove_source (GstDynAppSrc * bin) >+{ >+ GList *item; >+ >+ for (item = bin->appsrc_list; item; item = g_list_next (item)) { >+ GstAppSourceGroup *appsrc_group = (GstAppSourceGroup *) item->data; >+ >+ GST_DEBUG_OBJECT (bin, "removing appsrc element and ghostpad"); >+ >+ gst_ghost_pad_set_target (GST_GHOST_PAD_CAST (appsrc_group->srcpad), NULL); >+ gst_element_remove_pad (GST_ELEMENT_CAST (bin), appsrc_group->srcpad); >+ appsrc_group->srcpad = NULL; >+ >+ gst_element_set_state (appsrc_group->appsrc, GST_STATE_NULL); >+ gst_bin_remove (GST_BIN_CAST (bin), appsrc_group->appsrc); >+ gst_object_unref (appsrc_group->appsrc); >+ appsrc_group->appsrc = NULL; >+ } >+ >+ g_list_free_full (bin->appsrc_list, g_free); >+ bin->appsrc_list = NULL; >+ >+ bin->n_source = 0; >+} >+ >+static GstElement * >+gst_dyn_appsrc_new_appsrc (GstDynAppSrc * bin, const gchar * name) >+{ >+ GstAppSourceGroup *appsrc_group; >+ >+ GST_OBJECT_LOCK (bin); >+ >+ if (GST_STATE (bin) >= GST_STATE_PAUSED) { >+ GST_WARNING_OBJECT (bin, >+ "deny to create appsrc when state is in PAUSED or PLAYING state"); >+ GST_OBJECT_UNLOCK (bin); >+ return NULL; >+ } >+ appsrc_group = g_malloc0 (sizeof (GstAppSourceGroup)); >+ appsrc_group->appsrc = gst_element_factory_make ("appsrc", name); >+ >+ bin->appsrc_list = g_list_append (bin->appsrc_list, appsrc_group); >+ bin->n_source++; >+ >+ GST_INFO_OBJECT (bin, "appsrc %p is appended to a list", >+ appsrc_group->appsrc); >+ GST_INFO_OBJECT (bin, "source number = %d", bin->n_source); >+ >+ GST_OBJECT_UNLOCK (bin); >+ >+ return appsrc_group->appsrc; >+} >+ >+/** >+ * gst_dyn_appsrc_end_of_stream: >+ * @dynappsrc: a #GstDynAppSrc >+ * >+ * Indicates to all of appsrc elements that the last buffer queued in the >+ * element is the last buffer of the stream. >+ * >+ * Returns: #GST_FLOW_OK when the EOS was successfuly queued. >+ * #GST_FLOW_FLUSHING when @appsrc is not PAUSED or PLAYING. >+ */ >+GstFlowReturn >+gst_dyn_appsrc_end_of_stream (GstDynAppSrc * bin) >+{ >+ GstFlowReturn ret = GST_FLOW_OK; >+ GList *item; >+ >+ for (item = bin->appsrc_list; item; item = g_list_next (item)) { >+ GstAppSourceGroup *appsrc_group = (GstAppSourceGroup *) item->data; >+ >+ GST_DEBUG_OBJECT (bin, "indicate to appsrc element for EOS"); >+ g_signal_emit_by_name (appsrc_group->appsrc, "end-of-stream", &ret); >+ GST_DEBUG_OBJECT (bin, "%s[ret:%s]", >+ GST_ELEMENT_NAME (appsrc_group->appsrc), gst_flow_get_name (ret)); >+ if (ret != GST_FLOW_OK) >+ break; >+ } >+ >+ return ret; >+} >+ >+static GstStateChangeReturn >+gst_dyn_appsrc_change_state (GstElement * element, GstStateChange transition) >+{ >+ GstStateChangeReturn ret; >+ GstDynAppSrc *bin = GST_DYN_APPSRC (element); >+ >+ switch (transition) { >+ case GST_STATE_CHANGE_READY_TO_PAUSED: >+ if (!setup_source (bin)) >+ return GST_STATE_CHANGE_FAILURE; >+ break; >+ default: >+ break; >+ } >+ >+ ret = GST_ELEMENT_CLASS (parent_class)->change_state (element, transition); >+ >+ switch (transition) { >+ case GST_STATE_CHANGE_READY_TO_PAUSED: >+ GST_DEBUG_OBJECT (bin, "ready to paused"); >+ if (ret == GST_STATE_CHANGE_FAILURE) >+ goto setup_failed; >+ break; >+ case GST_STATE_CHANGE_PAUSED_TO_READY: >+ GST_DEBUG_OBJECT (bin, "paused to ready"); >+ remove_source (bin); >+ break; >+ case GST_STATE_CHANGE_READY_TO_NULL: >+ remove_source (bin); >+ break; >+ default: >+ break; >+ } >+ >+ return ret; >+ >+setup_failed: >+ { >+ /* clean up leftover groups */ >+ return GST_STATE_CHANGE_FAILURE; >+ } >+} >+ >+static GstURIType >+gst_dyn_appsrc_uri_get_type (GType type) >+{ >+ return GST_URI_SRC; >+} >+ >+static const gchar *const * >+gst_dyn_appsrc_uri_get_protocols (GType type) >+{ >+ static const gchar *protocols[] = { "dynappsrc", NULL }; >+ >+ return protocols; >+} >+ >+static gchar * >+gst_dyn_appsrc_uri_get_uri (GstURIHandler * handler) >+{ >+ GstDynAppSrc *bin = GST_DYN_APPSRC (handler); >+ >+ return bin->uri ? g_strdup (bin->uri) : NULL; >+} >+ >+static gboolean >+gst_dyn_appsrc_uri_set_uri (GstURIHandler * handler, const gchar * uri, >+ GError ** error) >+{ >+ return TRUE; >+} >+ >+static void >+gst_dyn_appsrc_uri_handler_init (gpointer g_iface, gpointer iface_data) >+{ >+ GstURIHandlerInterface *iface = (GstURIHandlerInterface *) g_iface; >+ >+ iface->get_type = gst_dyn_appsrc_uri_get_type; >+ iface->get_protocols = gst_dyn_appsrc_uri_get_protocols; >+ iface->get_uri = gst_dyn_appsrc_uri_get_uri; >+ iface->set_uri = gst_dyn_appsrc_uri_set_uri; >+} >diff --git a/gst/dynappsrc/gstdynappsrc.h b/gst/dynappsrc/gstdynappsrc.h >new file mode 100644 >index 0000000..7221bb4 >--- /dev/null >+++ b/gst/dynappsrc/gstdynappsrc.h >@@ -0,0 +1,79 @@ >+/* GStreamer Dynamic App Source element >+ * Copyright (C) 2014 LG Electronics, Inc. >+ * Author : Wonchul Lee <wonchul86.lee@lge.com> >+ * >+ * This library is free software; you can redistribute it and/or >+ * modify it under the terms of the GNU Library General Public >+ * License as published by the Free Software Foundation; either >+ * version 2 of the License, or (at your option) any later version. >+ * >+ * This library is distributed in the hope that it will be useful, >+ * but WITHOUT ANY WARRANTY; without even the implied warranty of >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >+ * Library General Public License for more details. >+ * >+ * You should have received a copy of the GNU Library General Public >+ * License along with this library; if not, write to the >+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330, >+ * Boston, MA 02111-1307, USA. >+ */ >+ >+#ifndef __GST_DYN_APPSRC_H__ >+#define __GST_DYN_APPSRC_H__ >+ >+#include <gst/gst.h> >+ >+G_BEGIN_DECLS >+ >+#define GST_TYPE_DYN_APPSRC (gst_dyn_appsrc_get_type()) >+#define GST_DYN_APPSRC(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj),GST_TYPE_DYN_APPSRC,GstDynAppSrc)) >+#define GST_DYN_APPSRC_CLASS(obj) (G_TYPE_CHECK_CLASS_CAST((obj),GST_TYPE_DYN_APPSRC,GstDynAppSrcClass)) >+#define GST_IS_DYN_APPSRC(obj) (G_TYPE_CHECK_INSTANCE_TYPE((obj),GST_TYPE_DYN_APPSRC)) >+#define GST_IS_DYN_APPSRC_CLASS(obj) (G_TYPE_CHECK_CLASS_TYPE((obj),GST_TYPE_DYN_APPSRC)) >+ >+typedef struct _GstDynAppSrc GstDynAppSrc; >+typedef struct _GstDynAppSrcClass GstDynAppSrcClass; >+typedef struct _GstAppSourceGroup GstAppSourceGroup; >+ >+/** >+ * GstDynAppSrc: >+ * >+ * dynappsrc element data structure >+ */ >+struct _GstDynAppSrc >+{ >+ GstBin parent; >+ >+ GstPad* srcpad; >+ gchar* uri; >+ >+ GList *appsrc_list; >+ >+ gint n_source; >+ >+}; >+ >+struct _GstDynAppSrcClass >+{ >+ GstBinClass parent_class; >+ >+ /* create a appsrc element */ >+ GstElement *(*new_appsrc) (GstDynAppSrc * dynappsrc, const gchar * name); >+ >+ /* actions */ >+ GstFlowReturn (*end_of_stream) (GstDynAppSrc * dynappsrc); >+}; >+ >+struct _GstAppSourceGroup >+{ >+ GstElement *appsrc; >+ GstPad *srcpad; >+}; >+ >+GType gst_dyn_appsrc_get_type (void); >+ >+GstFlowReturn gst_dyn_appsrc_end_of_stream (GstDynAppSrc * dynappsrc); >+ >+G_END_DECLS >+#endif /* __GST_DYN_APPSRC_H__ */ >diff --git a/gst/dynappsrc/plugins.c b/gst/dynappsrc/plugins.c >new file mode 100644 >index 0000000..cf0a386 >--- /dev/null >+++ b/gst/dynappsrc/plugins.c >@@ -0,0 +1,45 @@ >+/* GStreamer Dynamic App Source Plugins >+ * >+ * Copyright (C) 2014 LG Electronics, Inc. >+ * Author : Wonchul Lee <wonchul86.lee@lge.com> >+ * >+ * This library is free software; you can redistribute it and/or >+ * modify it under the terms of the GNU Library General Public >+ * License as published by the Free Software Foundation; either >+ * version 2 of the License, or (at your option) any later version. >+ * >+ * This library is distributed in the hope that it will be useful, >+ * but WITHOUT ANY WARRANTY; without even the implied warranty of >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >+ * Library General Public License for more details. >+ * >+ * You should have received a copy of the GNU Library General Public >+ * License along with this library; if not, write to the >+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330, >+ * Boston, MA 02111-1307, USA. >+ */ >+ >+#ifdef HAVE_CONFIG_H >+#include <config.h> >+#endif >+ >+#include <string.h> >+#include <gst/gst.h> >+ >+#include "gstdynappsrc.h" >+ >+static gboolean >+plugin_init (GstPlugin * plugin) >+{ >+ if (!gst_element_register (plugin, "dynappsrc", GST_RANK_PRIMARY, >+ GST_TYPE_DYN_APPSRC)) >+ return FALSE; >+ >+ return TRUE; >+} >+ >+GST_PLUGIN_DEFINE (GST_VERSION_MAJOR, >+ GST_VERSION_MINOR, >+ dynappsrc, >+ "Dynamic App Source", >+ plugin_init, PACKAGE_VERSION, "LGPL", PACKAGE_NAME, PACKAGE_BUGREPORT) >-- >1.9.1 >
Created attachment 290148 [details] [review] add dynamic appsrc plugins for handling multiple appsrc reupload patch
Created attachment 290295 [details] [review] Add dynamic appsrc plugins for handling multiple appsrc rebase patch to master, and.. can anybody remove comment #9?
Interesting idea, though would multiappsrc be a better name, or am I missing some concept involved here ?
This is just for deploying more than one appsrc by uri and signal action. If you think muliappsrc is more proper, I'll change the element name in next patch set. By the way, except name, how about the feature and the codes? Do you think it is ready to ship?
Created attachment 290297 [details] [review] Add new element to handle multiple appsrc Thanks Nicolas, I change the name of plugin to multiappsrc.
Review of attachment 290297 [details] [review]: ::: configure.ac @@ +3255,3 @@ gst/dvbsuboverlay/Makefile gst/dvdspu/Makefile +gst/multiappsrc/Makefile Alphabetical Order?
Created attachment 290298 [details] [review] Add new element to handle multiple appsrc change configure order
Review of attachment 290298 [details] [review]: 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@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.
Also, to help adoption, it's always well persived when new elements comes with unit tests, shall be a test in tests/check/element/multiappsrc.c. See other tests for examples.
Review of attachment 290298 [details] [review]: ::: gst/multiappsrc/gstmultiappsrc.c @@ +440,3 @@ + GST_ELEMENT_NAME (appsrc_group->appsrc), gst_flow_get_name (ret)); + if (ret != GST_FLOW_OK) + break; I am still think about the subtitle case, but I am not clear about that yet. For now, I modify code except it.
Created attachment 290570 [details] [review] #2Add new element to handle multiple appsrc I modify some code following the review and add test code.
Created attachment 290675 [details] [review] #2 Add new element to handle multiple appsrc reupload patch
Comment on attachment 290298 [details] [review] Add new element to handle multiple appsrc obsolete old one
Comment on attachment 290675 [details] [review] #2 Add new element to handle multiple appsrc Hi > [..] >+ >+/** >+ * SECTION:element-multiappsrc >+ * >+ * MultiAppsrc provides multiple appsrc elements inside a single source bin >+ * for handling separated audio, video and text stream. stream/streams >+ * >+ * <refsect2> >+ * <title>Usage</title> >+ * <para> >+ * A multiappsrc element is created by pipeline based on "multiappsrc://" protocal URI. protocal/protocol > [..] >+/* must be called with OBJECT_LOCK */ >+static gboolean >+setup_sources (GstMultiAppsrc * bin) >+{ >+ GList *item; >+ GstPadTemplate *pad_tmpl; >+ gchar *padname; >+ gboolean ret = FALSE; >+ >+ pad_tmpl = gst_static_pad_template_get (&src_template); >+ >+ for (item = bin->appsrc_list; item; item = g_list_next (item)) { >+ GstAppSourceGroup *appsrc_group = (GstAppSourceGroup *) item->data; >+ GstPad *srcpad = NULL; >+ >+ gst_bin_add (GST_BIN_CAST (bin), appsrc_group->appsrc); >+ >+ srcpad = gst_element_get_static_pad (appsrc_group->appsrc, "src"); >+ padname = >+ g_strdup_printf ("src_%u", g_list_position (bin->appsrc_list, item)); >+ appsrc_group->srcpad = >+ gst_ghost_pad_new_from_template (padname, srcpad, pad_tmpl); >+ gst_pad_set_event_function (appsrc_group->srcpad, >+ gst_multi_appsrc_handle_src_event); >+ gst_pad_set_query_function (appsrc_group->srcpad, >+ gst_multi_appsrc_handle_src_query); >+ >+ gst_pad_set_active (appsrc_group->srcpad, TRUE); >+ gst_element_add_pad (GST_ELEMENT_CAST (bin), appsrc_group->srcpad); >+ >+ gst_object_unref (srcpad); >+ g_free (padname); >+ >+ ret = TRUE; >+ } Whats the point of unconditionally setting ret = TRUE on each iteration? shouldn't you be checking some of the while loop's functions return values like the one from gst_bin_add()? >+ gst_object_unref (pad_tmpl); >+ >+ if (ret) { >+ GST_DEBUG_OBJECT (bin, "all appsrc elements are added"); >+ gst_element_no_more_pads (GST_ELEMENT_CAST (bin)); >+ } Coming from the above, the if(ret) is superfluous unless something can break the for() loop above with ret = FALSE, Unless all you wanted to check was (bin->appsrc_list) at the beginning, in which case, possibly a simpler if(!list){ warn(""); return FALSE } do{}while(next) would do. >+ >+ return ret; >+} >+ >+/* must be called with OBJECT_LOCK */ >+static void >+remove_sources (GstMultiAppsrc * bin) >+{ >+ GList *item; >+ >+ for (item = bin->appsrc_list; item; item = g_list_next (item)) { >+ GstAppSourceGroup *appsrc_group = (GstAppSourceGroup *) item->data; >+ >+ GST_DEBUG_OBJECT (bin, "removing appsrc element and ghostpad"); >+ >+ gst_ghost_pad_set_target (GST_GHOST_PAD_CAST (appsrc_group->srcpad), NULL); >+ gst_element_remove_pad (GST_ELEMENT_CAST (bin), appsrc_group->srcpad); >+ appsrc_group->srcpad = NULL; >+ >+ gst_element_set_state (appsrc_group->appsrc, GST_STATE_NULL); >+ gst_bin_remove (GST_BIN_CAST (bin), appsrc_group->appsrc); >+ appsrc_group->appsrc = NULL; >+ } >+ >+ g_list_free_full (bin->appsrc_list, g_free); >+ bin->appsrc_list = NULL; >+ >+ bin->n_source = 0; >+} Is there any guarantee funcs like _pad_set_target() and _element_remove_pad() will never fail in this scenario? Would the state at function exit be sane if they do? Otherwise you might want to add some return checking and make this one return a gboolean instead. > [..] >+static GstPadProbeReturn >+_appsrc_event_probe (GstPad * pad, GstPadProbeInfo * info, gpointer udata) >+{ >+ GstEvent *event = GST_PAD_PROBE_INFO_DATA (info); >+ GstElement *appsrc = udata; >+ const GstStructure *s; >+ App *app = &s_app; >+ >+ GST_DEBUG ("element:%s got an event:%s", GST_ELEMENT_NAME (appsrc), >+ GST_EVENT_TYPE_NAME (event)); >+ >+ if (GST_EVENT_TYPE (event) == GST_EVENT_CUSTOM_UPSTREAM) { >+ s = gst_event_get_structure (event); >+ if (g_str_equal (gst_structure_get_name (s), "custom-upstream")) >+ app->nb_received_event++; >+ } else if (GST_EVENT_TYPE (event) == GST_EVENT_SEEK) { >+ app->nb_received_event++; >+ } Would probably look cleaner as a switch() but it's OK anyway. > [..] >+GST_START_TEST (test_appsrc_creation) >+{ >+ GstElement *multiappsrc; >+ guint pad_added_id = 0; >+ gint n_added = 0; >+ GstStateChangeReturn ret; >+ GstElement *appsrc1 = NULL; >+ GstElement *appsrc2 = NULL; >+ GstIterator *iter; >+ GValue item = { 0, }; >+ gboolean done = FALSE; >+ gboolean exist_srcpad = FALSE; >+ gint n_source = 0; >+ >+ multiappsrc = >+ gst_element_make_from_uri (GST_URI_SRC, "multiappsrc://", "source", NULL); >+ >+ fail_unless (multiappsrc != NULL, >+ "fail to create multiappsrc element by uri"); >+ >+ pad_added_id = >+ g_signal_connect (multiappsrc, "pad-added", >+ G_CALLBACK (pad_added_cb), &n_added); >+ >+ g_signal_emit_by_name (multiappsrc, "new-appsrc", NULL, &appsrc1); >+ g_signal_emit_by_name (multiappsrc, "new-appsrc", "thisisappsrc", &appsrc2); >+ >+ /* user should do ref appsrc elements before using it */ >+ gst_object_ref (appsrc1); >+ gst_object_ref (appsrc2); >+ >+ fail_unless (appsrc1 && appsrc2, "fail to create appsrc element"); >+ fail_unless (GST_OBJECT_NAME (appsrc2) >+ && "thisisappsrc", "fail to set user-defined name"); >+ >+ iter = gst_element_iterate_src_pads (multiappsrc); >+ while (!done) { >+ switch (gst_iterator_next (iter, &item)) { >+ case GST_ITERATOR_OK: >+ exist_srcpad = TRUE; >+ done = TRUE; >+ break; >+ case GST_ITERATOR_RESYNC: >+ gst_iterator_resync (iter); >+ break; >+ case GST_ITERATOR_ERROR: >+ case GST_ITERATOR_DONE: >+ done = TRUE; >+ break; >+ } >+ } >+ >+ fail_unless (!exist_srcpad, >+ "srcpad is added too earlier, it should be added during state change"); >+ >+ g_value_unset (&item); >+ gst_iterator_free (iter); >+ >+ g_object_get (multiappsrc, "n-source", &n_source, NULL); >+ fail_unless (n_source == 2, "the number of source element is not matched"); >+ >+ ret = gst_element_set_state (multiappsrc, GST_STATE_PAUSED); >+ fail_unless (ret == GST_STATE_CHANGE_SUCCESS, >+ "fail to state change to PAUSED"); >+ fail_unless (n_added == 2, "srcpad of multiappsrc does not added"); >+ >+ /* user should do unref appsrc elements before destroy pipeline */ >+ gst_object_unref (appsrc1); >+ gst_object_unref (appsrc2); >+ >+ /* >+ In general, we don't need to call set_state(NULL) before unref element >+ because if ref_count is zero by gst_object_unref, the state of the element >+ automatically becomes NULL. >+ However, in here, set_state(NULL) is called in order to clarify automatically becomes NULL. However, ... >+ how to handle appsrc from multiappsrc. Without internal appsrcs de-referencing, >+ the appsrcs are remained as detached elements from multiappsrc. are remained/remain >+ This means that the upper layer cannot re-use appsrcs after calling set_state(NULL). Breaking your lines at 80 characters wouldn't hurt. > [..] >+GST_START_TEST (test_appsrc_create_when_paused) >+{ >+ GstElement *multiappsrc; >+ guint pad_added_id = 0; >+ gint n_added = 0; >+ GstStateChangeReturn ret; >+ GstElement *appsrc1 = NULL; >+ GstElement *appsrc2 = NULL; >+ gint n_source = 0; >+ >+ multiappsrc = >+ gst_element_make_from_uri (GST_URI_SRC, "multiappsrc://", "source", NULL); >+ >+ pad_added_id = >+ g_signal_connect (multiappsrc, "pad-added", >+ G_CALLBACK (pad_added_cb), &n_added); >+ >+ g_signal_emit_by_name (multiappsrc, "new-appsrc", "appsrc", &appsrc1); >+ >+ /* user should do ref appsrc elements before using it */ >+ gst_object_ref (appsrc1); >+ >+ fail_unless (appsrc1 != NULL, "failed to create appsrc element"); >+ >+ g_object_get (multiappsrc, "n-source", &n_source, NULL); >+ fail_unless (n_source == 1, "the number of source element is not matched"); >+ >+ ret = gst_element_set_state (multiappsrc, GST_STATE_PAUSED); >+ fail_unless (ret == GST_STATE_CHANGE_SUCCESS, >+ "fail to state change to PAUSED"); >+ >+ /* Try to generate a appsrc element when state is paused. >+ * If it is genetated, it is not make sence. >+ * Because, in this time, configuratoin of all appsrc elements in multiappsrc bin is already done. >+ * Thus, you should call "new-appsrc" singal action before paused state. >+ */ Ditto. > [...] >+GST_START_TEST (test_appsrc_upstream_event) >+{ >+ App *app = &s_app; >+ guint pad_added_id = 0; >+ gint n_added = 0; >+ GstStateChangeReturn ret; >+ gint n_source = 0; >+ gint count = 0; >+ GstEvent *event; >+ >+ GST_DEBUG ("Creating pipeline"); >+ app->pipeline = gst_pipeline_new ("pipeline"); >+ fail_if (app->pipeline == NULL); >+ >+ GST_DEBUG ("Creating multiappsrc"); >+ app->multiappsrc = >+ gst_element_make_from_uri (GST_URI_SRC, "multiappsrc://", "source", NULL); >+ fail_unless (app->multiappsrc != NULL, >+ "fail to create multiappsrc element by uri"); >+ gst_bin_add (GST_BIN (app->pipeline), app->multiappsrc); >+ >+ GST_DEBUG ("Creating fakesink"); >+ for (count = 0; count < NUM_APPSRC; count++) { >+ app->fakesink[count] = gst_element_factory_make ("fakesink", NULL); >+ fail_if (app->fakesink[count] == NULL); >+ g_object_set (G_OBJECT (app->fakesink[count]), "sync", TRUE, NULL); >+ gst_bin_add (GST_BIN (app->pipeline), app->fakesink[count]); >+ } >+ >+ pad_added_id = >+ g_signal_connect (app->multiappsrc, "pad-added", >+ G_CALLBACK (pad_added_cb), &n_added); >+ >+ GST_DEBUG ("Creating appsrc"); >+ for (count = 0; count < NUM_APPSRC; count++) { >+ gchar *appsrc_name; >+ appsrc_name = g_strdup_printf ("foot%d", count); >+ g_signal_emit_by_name (app->multiappsrc, "new-appsrc", appsrc_name, >+ &app->appsrc[count]); >+ /* user should do ref appsrc elements before using it */ >+ gst_object_ref (app->appsrc[count]); >+ >+ fail_unless (app->appsrc[count] != NULL, "failed to create appsrc element"); >+ g_free (appsrc_name); >+ } >+ >+ g_object_get (app->multiappsrc, "n-source", &n_source, NULL); >+ fail_unless (n_source == NUM_APPSRC, >+ "the number of source element is not matched"); >+ >+ ret = gst_element_set_state (app->pipeline, GST_STATE_PAUSED); >+ fail_unless (ret == GST_STATE_CHANGE_ASYNC); >+ >+ fail_unless (n_added == NUM_APPSRC, "srcpad of multiappsrc does not added"); >+ >+ ret = gst_element_set_state (app->pipeline, GST_STATE_PLAYING); >+ fail_unless (ret == GST_STATE_CHANGE_ASYNC); >+ >+ GST_DEBUG ("Sending upstream custom event"); >+ app->nb_received_event = 0; >+ event = gst_event_new_custom (GST_EVENT_CUSTOM_UPSTREAM, >+ gst_structure_new_empty ("custom-upstream")); >+ >+ gst_element_send_event (app->pipeline, event); >+ fail_unless (app->nb_received_event == NUM_APPSRC, >+ "the number of received events are not matched"); >+ >+ /* >+ * First of all, multiappsrc handle a seek-event that it send all of appsrc elements. >+ * Try to send seek-event in a fakesink. >+ * Received seek-events in all of appsrc elements should be 10. >+ */ Same. Check elsewhere too please. >+ GST_DEBUG ("Sending flush seek event to fakesink"); >+ for (count = 0; count < NUM_APPSRC; count++) { >+ app->nb_received_event = 0; >+ gst_element_seek (app->fakesink[count], 1.0, GST_FORMAT_TIME, >+ GST_SEEK_FLAG_FLUSH, GST_SEEK_TYPE_SET, 0, GST_SEEK_TYPE_SET, -1); >+ >+ fail_unless (app->nb_received_event == NUM_APPSRC, >+ "the number of received events are not matched"); >+ } >+ >+ /* >+ * Try to send seek-event in pipeline. >+ * In this case, received seek-event in all of appsrc elements should be 100. >+ */ >+ GST_DEBUG ("Sending flush seek event to pipeline"); Please see bellow >+ app->nb_received_event = 0; >+ gst_element_seek (app->pipeline, 1.0, GST_FORMAT_TIME, >+ GST_SEEK_FLAG_FLUSH, GST_SEEK_TYPE_SET, 0, GST_SEEK_TYPE_SET, -1); >+ >+ fail_unless (app->nb_received_event == (NUM_APPSRC * 10), >+ "the number of received events are not matched"); >+ >+ GST_DEBUG ("Release pipeline"); Please see bellow >+ /* user should do unref appsrc elements before destroy pipeline */ >+ for (count = 0; count < NUM_APPSRC; count++) >+ gst_object_unref (app->appsrc[count]); >+ >+ gst_element_set_state (app->pipeline, GST_STATE_NULL); >+ g_signal_handler_disconnect (app->multiappsrc, pad_added_id); >+ gst_object_unref (app->pipeline); >+} >+ >+GST_END_TEST; >+ >+static gboolean >+bus_message (GstBus * bus, GstMessage * message, App * app) >+{ >+ GST_DEBUG ("got message %s \n", >+ gst_message_type_get_name (GST_MESSAGE_TYPE (message))); Please see bellow > [..] >+GST_START_TEST (test_appsrc_eos) >+{ >+ App *app = &s_app; >+ guint pad_added_id = 0; >+ gint n_added = 0; >+ GstStateChangeReturn ret; >+ gint n_source = 0; >+ gint count = 0; >+ GstBus *bus; >+ >+ GST_DEBUG ("Creating pipeline"); >+ app->pipeline = gst_pipeline_new ("pipeline"); >+ fail_if (app->pipeline == NULL); >+ >+ app->loop = g_main_loop_new (NULL, FALSE); >+ >+ bus = gst_pipeline_get_bus (GST_PIPELINE (app->pipeline)); >+ >+ /* add watch for messages */ >+ gst_bus_add_watch (bus, (GstBusFunc) bus_message, app); >+ gst_object_unref (bus); >+ >+ GST_DEBUG ("Creating multiappsrc"); >+ app->multiappsrc = >+ gst_element_make_from_uri (GST_URI_SRC, "multiappsrc://", "source", NULL); >+ fail_unless (app->multiappsrc != NULL, >+ "fail to create multiappsrc element by uri"); >+ gst_bin_add (GST_BIN (app->pipeline), app->multiappsrc); >+ >+ GST_DEBUG ("Creating fakesink"); >+ for (count = 0; count < NUM_APPSRC; count++) { >+ app->fakesink[count] = gst_element_factory_make ("fakesink", NULL); >+ fail_if (app->fakesink[count] == NULL); >+ g_object_set (G_OBJECT (app->fakesink[count]), "sync", TRUE, NULL); >+ gst_bin_add (GST_BIN (app->pipeline), app->fakesink[count]); >+ } >+ >+ pad_added_id = >+ g_signal_connect (app->multiappsrc, "pad-added", >+ G_CALLBACK (pad_added_cb), &n_added); >+ >+ GST_DEBUG ("Creating appsrc"); This and the above ones fail the "is not the expected default behavior" so should probably be _INFO () instead Also, please add bug number to your commit message in your next iteration. Thanks a lot
Created attachment 291612 [details] [review] #3 multiappsrc: Add new element to handle multiple appsrc I modify the patch by following the Reynaldo's advices, thanks. The unclear thing is to guarantee funcs like _pad_set_target() to NULL and _element_remove_pad(). I think it should be success, and I can find any example for it in gstreamer code.
Comment on attachment 290675 [details] [review] #2 Add new element to handle multiple appsrc Please mark previous patch as obsolete when you submit a new one
Hello, I'm very interrested in this multiappsrc element but I don't know how to integrate this to my project. I'm currently working with gstreamer 1.4.5 and android. Is it possible to integrate the gstmultiappsrc.c directly to my native code or do I have to recompile the whole gstreamer with cerbero? Thanks Thomas
>> Is it possible to >> integrate the gstmultiappsrc.c directly to my native code or do I have to >> recompile the whole gstreamer with cerbero? Yes, you can use this code with gstreamer 1.4.5, but you don't need to recompile entire gstreamer matters. just build gst-plugins-bad after applying my patch.
can somebody review it?
Reynaldo, are you going to continue this review ?
still review is on going? or need some work?
Review of attachment 291612 [details] [review]: I suggest to also add an action signal "remove-source" (to dynamically remove a source or else) and rename "new-appsrc" to "add-source". ::: gst/multiappsrc/Makefile.am @@ +6,3 @@ +# compiler and linker flags used to compile this plugin, set in configure.ac +libgstmultiappsrc_la_CFLAGS = $(GST_CFLAGS) +libgstmultiappsrc_la_LIBADD = $(GST_LIBS) -lgstvideo-@GST_API_VERSION@ -lgstaudio-@GST_API_VERSION@ You do not depends on -lgstvideo-@GST_API_VERSION@ -lgstaudio-@GST_API_VERSION@. I do not think you even need $(GST_BASE_CFLAGS) / $(GST_BASE_LIBS). I would suggest adding -lgstapp-@GST_API_VERSION@ even if for now you are using appsrc's signal instead of gst_app_src_end_of_stream directly for example. ::: gst/multiappsrc/gstmultiappsrc.c @@ +56,3 @@ + * gst_object_ref (appsrc2); + * + * g_signal_emit_by_name (multiappsrc, "new-appsrc", &appsrc1); That would have been great to return an ID instead of the element directly. And then translating app's signals: g_signal_connect (multiappsrc, "need-data", G_CALLBACK (start_feed), id1); g_signal_emit_by_name (multiappsrc, "push-buffer", id1, buff); Maybe we can add this later and add signal action "new-appsrc-id": g_signal_emit_by_name (multiappsrc, "new-appsrc-id", &id1);
I forgot to say I am pleased there is a such element and cannot wait to find it in main stream :) That would be great to have it upstream before 1.6. Thx for this work.
It would be nice to have a sample code as well in our examples folder to showcase how to use the element. It seems one has to keep references to all appsrcs that were created internally? There is no way to list/get those when needed? The EOS signal could be replaced by the standard gst_element_send_event call as it would be the expected behavior of the main API itself.
Thank you for good advice. @Julien, > I suggest to also add an action signal "remove-source" (to dynamically remove a source or else) but imho, to support this, it can be done only after completion of disconnecting, or only the state <= READY. Otherwise, sending critical messages or assert failure is good? @Thiago > There is no way to list/get those when needed? I missed this way, I think, I have overlooked iterator like function. I'll add it.
Julien, > That would have been great to return an ID instead of the element directly. And then > translating app's signals: I agreed with that, then it need to be added some api for using ID instead of element directly. >I forgot to say I am pleased there is a such element and cannot wait to find it in main stream :) That would be great to have it upstream before 1.6. Thx for this work. you're welcome! thanks for review it. Thiago, > It seems one has to keep references to all appsrcs that were created internally? There is no way to list/get those when needed? If we decide to expose appsrc ID instead of element directly, then this may need a different API in my opinion. > The EOS signal could be replaced by the standard gst_element_send_event call as it would be the expected behavior of the main API itself. yes, I totally agree with that.
(In reply to Justin J. Kim from comment #37) > Thank you for good advice. > > @Julien, > > > I suggest to also add an action signal "remove-source" (to dynamically remove a source or else) > > > but imho, to support this, it can be done only after completion of > disconnecting, or only the state <= READY. Otherwise, sending critical > messages or assert failure is good? > Please read https://coaxion.net/blog/2014/01/gstreamer-dynamic-pipelines/, you have to use pad probes for that. Sebastian's blog contains examples (https://github.com/sdroege/gst-snippets/blob/217ae015aaddfe3f7aa66ffc936ce93401fca04e/dynamic-filter.c) multiappsrc could be use for MSE (Media Source Extensions) in WebkitGTK and ChromiumGStreamerBackend. In MSE the id actually comes from the app, so we would need "add-source-id" and "remove-source-id" action signals: g_signal_emit_by_name (multiappsrc, "add-source-id", id1); g_signal_emit_by_name (multiappsrc, "remove-source-id", id1); idN being a gchar* input param. Or it can be a int it does not mater much. +all the appsrc signal translated, for example: g_signal_connect (multiappsrc, "need-data", G_CALLBACK (start_feed), id1); g_signal_emit_by_name (multiappsrc, "push-buffer", id1, buff); What do you think ? Is seeking working ? What is your concrete use case ? Thx!
> multiappsrc could be use for MSE (Media Source Extensions) in WebkitGTK. yes, MSE is one of our use cases, I don't know much about Webkit though. > What do you think ? Your advice is a great help to control reference matter. I always think about how can I hide reference. You gave me the answer. To do this, I need some more work than I thought. :) > Is seeking working ? In my scene, seeking was not mandatory so I haven't checked this operation, but I think it can work if I add another signal translator like need-data. > What is your concrete use case ? The first idea was from legacy system which sent audio/video streams separately. But while testing and implementing this, I found it can be useful for various systems if someone wants to handle each stream in memory before feeding into gstreamer pipeline. DRM, secure video path, etc. I'll try to update our patch which reflects your review soon. Thanks.
About "remove-source-id" do not bother too much since I have some doubts now if it can really happen for MSE while the stream is playing. Maybe for now just warn is someone try to "push-buffer" after "remove-source-id" has been called and prevent emitting any signal.
If remove-source-id is not core feature, how about define limitation of usage of this API. Without dynamic pipeline function, the last patch is verified. To hide reference by using id mechanism is not big burden and it is not required to modify lots of codes. So I can prepare it in short time. However, to support remove-source-id elegantly, we need to verify deeply, I think. :)
Created attachment 308404 [details] [review] more fluent multiappsrc (signals and actions, and its api) @Julien Thank you for showing your interesting to my proposed element. On advice from you and Thiago I added and modified original proposal in order to provide more general APIs. Review is still required!
Dear all Are there more progress or plan on this topic? I need some features on top of MultiAppSrc. That is, - adding/removing appsrc during playing state. (use case: during live streaming, "new source such as subtitle came up" or "existing source was removed and it will never be used") - replacing appsrc in order to indicate "it's different stream from previous" to downstream elements. It will be like as adaptivedemux without download feature. Can I continue on this work if there are someone who interest above features?
Hi Seungha, that would be great if you can continue this work. Maybe just double check with Justin. Hi Justin, sorry I had not time to review it and then I forgot. I can try to find some in the near future. Cheers.
Seungha, Great to see you again. Please keep going to land multiappsrc into upstream. Your use case of dynamically allocation/deallocation elements seems to be very complex and have some duplicated functionality with the other elements. Who will decide if new stream is coming, or the stream is dropped? Will someone do that outside of pipeline by action? Julien, No problem. Actually, I lost my use case for a while :)
(In reply to Justin J. Kim from comment #46) > Seungha, > Great to see you again. Please keep going to land multiappsrc into upstream. > Your use case of dynamically allocation/deallocation elements seems to be > very complex and have some duplicated functionality with the other elements. > > Who will decide if new stream is coming, or the stream is dropped? > Will someone do that outside of pipeline by action? > > Julien, > No problem. Actually, I lost my use case for a while :) Thanks for your quick reply :) add/remove/replacement of a source will be decided application. So, the application must be smart to detect dynamically changed streaming condition. With urisourcebin (which is used with playbin3), my suggested feature can be perfectly supported from my opinion.
Created attachment 335415 [details] [review] PATCH#1 MultiAppSrc element MultiAppSrc - wrapping all the appsrc's API and some properties (stream-type, format) - Allowing add/remove appsrc during playing state - Support track change by using push-discont-buffer/sample () API Parent bin should properly handle pad-added/removed signal from MultiAppSrc during playing state.
Review of attachment 335415 [details] [review]: 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;
(In reply to Julien Isorce from comment #49) > Review of attachment 335415 [details] [review] [review]: > > I reviewed only gstmultiappsrc.c , see my comments, maybe add support for > meson build too. Since I'm not familiar with meson build system, I'll prepare it after studying about it :) > What is your use case btw ? On my side I plan to use this element for MSE > (Media Source Extension) in the future. MSE is one of my future use-case. However, the most urgent use-case on my side is MPEGDASH/HLS based streaming/broadcasting with playbin but without adaptivedemux element. For that services, I've used external module. A role of the external module is to download fragmented MP4/MPEGTS media segment, and feed it to playbin pipeline. It can be imaged that like generic DASH/HLS pipeline but adaptivedemux is omitted. Anyway, content providers asked dynamically changing streaming environment nowadays, such as - video/audio codec change (e.g., with AD insertion) - insertion of subtitle track during playing (e.g., broadcasting). And maybe, unexpected removed track can be possible especially broadcasting system. So, to support that features, we need very flexible source element.
Created attachment 335479 [details] Design of push-discont-buffer/sample and remove-source-id Please refer to attached diagram. Because we cannot modify appsrc's state in the streaming thread, I introduced a GstTask and internal bus. Note that, push-discont-buffer/sample API is for track switching. Interface between adaptivedemux and urisourcebin was referenced for designing push-discont-buffer/sample API. One reason why I change appsrc's state (PLAYING->READY->PLAYING) is to invoke new sticky event such as stream-start, caps.
Created attachment 335491 [details] [review] PATCH#2 MultiAppSrc element
Thanks for quick but detailed review. (In reply to Julien Isorce from comment #49) > Review of attachment 335415 [details] [review] [review]: > > ::: 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 ... > I changed naming of GstAppSrcGroup to ChildAppSrcInfo like how urisourcebin did. And GhostPadInfo to OutputSlot. > @@ +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 ? variable for the output ghostpad was moved from GstAppSrcGroup to OutputSlot. OutputSlot is mainly used for forwarding query or event. > @@ +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. Without this, it's ok to access multiappsrc since a pad or an appsrc is a child of the bin. But, I thought it might burden that calling gst_pad_get_parent() or gst_element_get_parent () whenever we needed. How about your opinion? > @@ +2108,3 @@ > + expose_group (multiappsrc, group, TRUE); > + > + ret = gst_app_src_end_of_stream (GST_APP_SRC (group->appsrc)); > > why ? push-discont-buffer/sample needs draining appsrc's data and reconfiguration of appsrc. Please review attached design diagram.
Review of attachment 335491 [details] [review]: Thx for the updated patch and the diagram. All looks good! Is someone else interested to review it ? ::: tests/check/elements/multiappsrc.c @@ +208,3 @@ + source_id2, gst_buffer_new_and_alloc (4)) == GST_FLOW_OK, + "fail to push buffer"); + Ok I just realized that the API gst_multi_appsrc_push_buffer takes ownership whereas the action signal does not. Like GstAppSrc.
Created attachment 335841 [details] [review] PATCH#3 MultiAppSrc element with meson build - Add script for meson build - Add "push-buffer" signal-action use-case in unit test code
Sorry Seungha Yang for the long delay. <capOM> anyone interested to review multiappsrc ? <slomo> capOM: having it as yet another micro library is ugly :) <slomo> capOM: string based source ids seem not optimal. maybe add a source by string and get back an integer id? For the first comment it sounds like you can move the part from gst-libs into the gst dir. For the later comment what do you think of Sebastian suggestion ?
(In reply to Julien Isorce from comment #56) Thanks for your and Sebastian's feedback :) Frankly speaking, I'd like to hear what's the point of the first comment, it's difficult to understand.. So to speak based on your comment, I made external API for multiappsrc element. In this case, isn't it violation of Gstreamer's convention, if source dir is moved to to gst dir? Sebastian's suggestion about source-id seems to be a good idea! But, I'd like to listen to Gstreamer backend chromium usecase of the multiappsrc element. What's the convenient way in your case?
We already have lots of little libraries and it seems like it is not really needed here, just having a plugin with an action-signal API should be sufficient.
Sorry for the late reply. I think gst_multi_appsrc_add_source_id should return nothing, in the latest version of the patch: gchar * gst_multi_appsrc_add_source_id (GstMultiAppSrc * multiappsrc, const gchar * name); I had in mind (see #39 also) that the provided id is a unique id, which is what MSE does in its implementation. So the called is responsible to provide a unique id. Then internally to GstMultiAppSrc, you can still translate this string id to an integer id to match Sebastian's suggestion. But do not return it. So should be: gboolean gst_multi_appsrc_add_source_id (GstMultiAppSrc * multiappsrc, const gchar * unique_id); // return false if failure or if the id was already provided so no unique. gboolean gst_multi_appsrc_remove_source_id (GstMultiAppSrc * multiappsrc, const gchar * unique_id);
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/135.