After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 768709 - streamiddemux: Reconfigure output pads when stream is changed from upstream
streamiddemux: Reconfigure output pads when stream is changed from upstream
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
1.9.1
Other Linux
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-07-12 01:42 UTC by HoonHee Lee
Modified: 2018-11-03 12:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
streamiddemux: Reconfigure output pads when stream is changed from upstream (11.98 KB, patch)
2016-07-12 01:42 UTC, HoonHee Lee
needs-work Details | Review

Description HoonHee Lee 2016-07-12 01:42:57 UTC
Created attachment 331290 [details] [review]
streamiddemux: Reconfigure output pads when stream is changed from upstream

Dear All.
When stream is changed from upstream, it is better to remove output pads
for deactivated streams and to keep it for activated streams.

Please review my patch and feedback me.
Comment 1 Edward Hervey 2016-07-12 06:06:53 UTC
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) ?
Comment 2 HoonHee Lee 2016-07-12 22:53:24 UTC
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.
Comment 3 HoonHee Lee 2016-08-19 23:12:44 UTC
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.
Comment 4 Tim-Philipp Müller 2016-11-11 14:15:44 UTC
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 5 Tim-Philipp Müller 2016-11-11 14:19:52 UTC
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/
Comment 6 GStreamer system administrator 2018-11-03 12:35:30 UTC
-- 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.