Shouldn't you be comparing the new collection streams against the current collection streams and only remove those that are no longer present (and keep those that are still present) ?

Hello Edward Hervey
Thanks to leave your comment.
 
I just thought that it is better to remove deactivated(no-longer used) pads when collection is changed.
 
Could you elaborate a bit what you concern?
I am always welcome any questions and any opinions.

Hello Edward Hervey

You want to compare each GstStream(stream-id) between new collection and current collection? Then, re-use exist GstStream(stream-id) and remove no longer exist GstStream.
 
May I know your detailed intention and concern?
 
I am always welcome any questions and opinions.
 
Thanks.

Regarding Edward's comment, consider the following scenario:

Active collection:
 - Stream 0: audio
 - Stream 1: video
 - Stream 2: subtitle 1
 - Stream 3: subtitle 2

New collection:
 - Stream 0: audio
 - Stream 1: video
 - Stream 2: subtitle 1

or the other way round. In this case you just want to remove the pad for subtitle 2 and keep the other pads.

Comment on attachment 331290 [details] [review]
streamiddemux: Reconfigure output pads when stream is changed from upstream

Couple of drive-by comments and nitpicks from me:


>+  if (demux->active_collection && demux->active_collection != collection) {
>+    GST_DEBUG_OBJECT (demux, "Handle stream changes : %" GST_PTR_FORMAT,
>+        collection);
>+    // release deactivated streams
>+    for (i = 0; i < gst_stream_collection_get_size (demux->active_collection);
>+        i++) {

- Please use /* old-style C comments */ not // c++ style comments.

- Please move the get_size() out of the loop (below as well)


>--- a/plugins/elements/gststreamiddemux.h
>+++ b/plugins/elements/gststreamiddemux.h
>@@ -39,6 +39,33 @@ G_BEGIN_DECLS
>   (G_TYPE_CHECK_INSTANCE_TYPE ((obj), GST_TYPE_STREAMID_DEMUX))
> #define GST_IS_STREAMID_DEMUX_CLASS(klass) \
>   (G_TYPE_CHECK_CLASS_TYPE ((klass), GST_TYPE_STREAMID_DEMUX))
>+
>+#define GST_COLLECTION_GET_LOCK(demux) (&((GstStreamidDemux*)(demux))->collection_lock)
>+
>+#define GST_COLLECTION_LOCK(demux)   G_STMT_START { \
>+  GST_LOG_OBJECT (demux, "locking from thread %p", g_thread_self ()); \
>+  g_mutex_lock (GST_COLLECTION_GET_LOCK (demux)); \
>+  GST_LOG_OBJECT (demux, "locked from thread %p", g_thread_self ()); \
>+} G_STMT_END
>+
>+#define GST_COLLECTION_UNLOCK(demux)   G_STMT_START { \
>+  GST_LOG_OBJECT (demux, "unlocking from thread %p", g_thread_self ()); \
>+    g_mutex_unlock (GST_COLLECTION_GET_LOCK (demux)); \
>+} G_STMT_END
>+
>+#define GST_OUTPUT_GET_LOCK(demux) (&((GstStreamidDemux*)(demux))->output_lock)
>+
>+#define GST_OUTPUT_LOCK(demux)   G_STMT_START { \
>+  GST_LOG_OBJECT (demux, "locking from thread %p", g_thread_self ()); \
>+  g_mutex_lock (GST_OUTPUT_GET_LOCK (demux)); \
>+  GST_LOG_OBJECT (demux, "locked from thread %p", g_thread_self ()); \
>+} G_STMT_END
>+
>+#define GST_OUTPUT_UNLOCK(demux)   G_STMT_START { \
>+  GST_LOG_OBJECT (demux, "unlocking from thread %p", g_thread_self ()); \
>+    g_mutex_unlock (GST_OUTPUT_GET_LOCK (demux)); \
>+} G_STMT_END
>+

What's with all these new locks? Are they actually needed?

It looks to me like everything is effectively protected by the pad streaming lock already. I didn't see any code that uses any of these locks from a non-streaming thread. Did I miss anything?


>--- a/tests/check/elements/streamiddemux.c
>+++ b/tests/check/elements/streamiddemux.c

Unit test \o/

-- 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/gstreamer/issues/180.