[Bug 707605] Need "reverse-funnel" element

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=707605
  GStreamer | gstreamer (core) | git

Sebastian Dröge (slomo) <slomo> changed:

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

--- Comment #28 from Sebastian Dröge (slomo) <slomo at circular-chaos.org> 2013-09-27 13:33:55 UTC ---
Review of attachment 255075:
 --> (https://bugzilla.gnome.org/review?bug=707605&attachment=255075)

::: plugins/elements/gststreamiddemux.c
@@ +5,3 @@
+ *  @author: Jeongseok Kim <jeongseok.kim at lge.com>
+ *  @author: Wonchul Lee <wonchul86.lee at lge.com>
+ * Copyright 2013 LEG Corp.

LGE Corp, also it's twice here

@@ +57,3 @@
+enum
+{
+  SIGNAL_SRC_PAD_ADDED,

Remove this signal, GstElement::pad-added does the same already

@@ +63,3 @@
+static guint gst_streamid_demux_signals[LAST_SIGNAL] = { 0 };
+
+static GRWLock lock;

Put this either in the instance struct or just use the GstObject lock instead.
No global lock for per-instance things

@@ +171,3 @@
+  }
+
+  GST_OBJECT_LOCK (demux);

Locking shouldn't be necessary here as this will always be calling from the
streaming thread

@@ +189,3 @@
+  g_hash_table_insert (demux->stream_id_pairs, g_strdup (stream_id),
+      (gpointer) srcpad);
+  g_rw_lock_writer_unlock (&lock);

If you just keep the currently active pad in the instance struct, you don't
need to lock anything for this hash table as it will only be called from the
streaming thread.

@@ +192,3 @@
+
+  g_signal_emit (G_OBJECT (demux),
+      gst_streamid_demux_signals[SIGNAL_SRC_PAD_ADDED], 0, srcpad);

Remove this signal

@@ +211,3 @@
+  active_srcpad =
+      gst_streamid_demux_get_srcpad_by_stream_id (demux,
+      gst_pad_get_stream_id (pad));

You can just store the currently active srcpad in the instance structure. And
update that whenever you receive a stream-start event

@@ +217,3 @@
+
+  if (active_srcpad)
+    res = gst_pad_push (active_srcpad, buf);

else gst_buffer_unref (buf);

@@ +236,3 @@
+  g_rw_lock_writer_lock (&lock);
+  srcpad = g_hash_table_lookup (demux->stream_id_pairs, stream_id);
+  g_rw_lock_writer_unlock (&lock);

This lock should go away but you're actually doing a reader lock here

@@ +254,3 @@
+  GST_DEBUG_OBJECT (demux, "stream_id = %s", stream_id);
+
+  g_rw_lock_writer_lock (&lock);

Also reader lock, see above (and should go away)

@@ +283,3 @@
+      if (stream_id != NULL) {
+        caps = gst_pad_get_current_caps (pad);
+        gst_streamid_demux_srcpad_create (demux, pad, caps, stream_id);

You don't need the caps here, and often you don't even have them yet. Also if
the pad already exists you will need to update the sticky events on it from the
srcpad

@@ +285,3 @@
+        gst_streamid_demux_srcpad_create (demux, pad, caps, stream_id);
+      }
+      res = gst_pad_event_default (pad, parent, event);

Only send to the active pad

@@ +291,3 @@
+    {
+      /* Send caps to all src pads */
+      res = gst_pad_event_default (pad, parent, event);

Only send to the active pad, but as you don't have any code here just handle it
in the default case

@@ +303,3 @@
+#endif
+      /* Send newsegment to all src pads */
+      res = gst_pad_event_default (pad, parent, event);

Only send to the active pad, but as you don't have any code here just handle it
in the default case

@@ +309,3 @@
+    {
+      if (GST_EVENT_IS_STICKY (event)) {
+        res = gst_pad_event_default (pad, parent, event);

Only send to the active pad

@@ +348,3 @@
+    }
+    default:
+      res = gst_pad_query_default (pad, parent, query);

All queries should only be done on the active pad

::: plugins/elements/gststreamiddemux.h
@@ +6,3 @@
+ *  @author: JeongSeok Kim  <jeongseok.kim at lge.com>
+ *  @author: WonChul Lee  <wonchul86.lee at lge.com>
+ * Copyright 2013 LEG Corp.

LGE Corp, right? Why twice here? :)

@@ +60,3 @@
+  GstElementClass parent_class;
+
+  void (*src_pad_added) (GstElement * element, GstPad * pad);

Not needed, GstElement::pad-added signal

-- 
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.