[Bug 707605] Need "reverse-funnel" element

[GStreamer (bugzilla.gnome.org)]

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

Olivier Crete (Tester) <olivier.crete> changed:

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

--- Comment #46 from Olivier Crete (Tester) <olivier.crete at ocrete.ca> 2013-10-12 17:35:46 UTC ---
Review of attachment 257087:
 --> (https://bugzilla.gnome.org/review?bug=707605&attachment=257087)

::: plugins/elements/gststreamiddemux.c
@@ +199,3 @@
+    demux->stream_id_pairs =
+        g_hash_table_new_full (g_str_hash, g_str_equal, (GDestroyNotify)
g_free,
+        (GDestroyNotify) gst_object_unref);

Any reason you don't create the hashtable in the _init method ?

@@ +213,3 @@
+  demux->active_srcpad = gst_object_ref (srcpad);
+  g_hash_table_insert (demux->stream_id_pairs, g_strdup (stream_id),
+      (gpointer) gst_object_ref (srcpad));

gst_object_ref() already returns a gpointer

@@ +324,3 @@
+  } else if (demux->active_srcpad) {
+    GST_OBJECT_LOCK (demux);
+    GstPad *srcpad = gst_object_ref (demux->active_srcpad);

Please declare the variable at the start of the block please

@@ +337,3 @@
+no_stream_id:
+  {
+    GST_ELEMENT_ERROR (demux, STREAM, DEMUX, (NULL), ("no stream_id found"));

Please add a translatable user friendly error string as the 4th parameter.

@@ +364,3 @@
+    gst_object_unref (demux->active_srcpad);
+    GST_OBJECT_UNLOCK (demux);
+    demux->active_srcpad = NULL;

Why do you unlock before setting the pointer to NULL ?

@@ +375,3 @@
+
+  it = gst_element_iterate_src_pads (GST_ELEMENT_CAST (demux));
+  GstIteratorResult itret = GST_ITERATOR_OK;

Please declare all variables at the beginning of the block, C89-style

--- Comment #47 from Olivier Crete (Tester) <olivier.crete at ocrete.ca> 2013-10-12 17:36:40 UTC ---
Review of attachment 257087:
 --> (https://bugzilla.gnome.org/review?bug=707605&attachment=257087)

::: plugins/elements/gststreamiddemux.c
@@ +199,3 @@
+    demux->stream_id_pairs =
+        g_hash_table_new_full (g_str_hash, g_str_equal, (GDestroyNotify)
g_free,
+        (GDestroyNotify) gst_object_unref);

Any reason you don't create the hashtable in the _init method ?

@@ +213,3 @@
+  demux->active_srcpad = gst_object_ref (srcpad);
+  g_hash_table_insert (demux->stream_id_pairs, g_strdup (stream_id),
+      (gpointer) gst_object_ref (srcpad));

gst_object_ref() already returns a gpointer

@@ +324,3 @@
+  } else if (demux->active_srcpad) {
+    GST_OBJECT_LOCK (demux);
+    GstPad *srcpad = gst_object_ref (demux->active_srcpad);

Please declare the variable at the start of the block please

@@ +337,3 @@
+no_stream_id:
+  {
+    GST_ELEMENT_ERROR (demux, STREAM, DEMUX, (NULL), ("no stream_id found"));

Please add a translatable user friendly error string as the 4th parameter.

@@ +364,3 @@
+    gst_object_unref (demux->active_srcpad);
+    GST_OBJECT_UNLOCK (demux);
+    demux->active_srcpad = NULL;

Why do you unlock before setting the pointer to NULL ?

@@ +375,3 @@
+
+  it = gst_element_iterate_src_pads (GST_ELEMENT_CAST (demux));
+  GstIteratorResult itret = GST_ITERATOR_OK;

Please declare all variables at the beginning of the block, C89-style

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