[Bug 772740] rtpbin: receiving RTP bundle support

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=772740

Olivier Crête <olivier.crete at ocrete.ca> changed:

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

--- Comment #9 from Olivier Crête <olivier.crete at ocrete.ca> ---
Review of attachment 337827:
 --> (https://bugzilla.gnome.org/review?bug=772740&attachment=337827)

Except for the little nits below, this generally looks good. I'd really
appreciate if there was a unit test to make sure this doesn't get broken in the
future. I think it would also be beneficial to avoid plugin the extra ssrcdemux
if the signal is not connected (or if it returns 0).

::: gst/rtpmanager/gstrtpbin.c
@@ +672,3 @@
+  g_value_init (&result, G_TYPE_UINT);
+  g_value_init (&params[0], GST_TYPE_ELEMENT);
+  g_value_set_object (&params[0], rtpbin);

You need to unset the params[0] after emitting the signal otherwise you'll leak
the element.

@@ +678,3 @@
+  g_signal_emitv (params,
+      gst_rtp_bin_signals[SIGNAL_ON_BUNDLED_SSRC], 0, &result);
+  if (G_VALUE_HOLDS_UINT (&result)) {

This will always be true, you just initialized it to G_TYPE_UINT earlier.

@@ +3560,3 @@
+    goto no_name;
+
+  GST_DEBUG_OBJECT (rtpbin, "finding session %d", sessid);

Use %u for debug since it's a guint.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.