[Bug 719938] rtpbin: allow dynamic RTP/RTCP encoders and decoders

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=719938
  GStreamer | gst-plugins-good | git

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

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

--- Comment #2 from Olivier Crete (Tester) <olivier.crete at ocrete.ca> 2013-12-06 00:42:48 UTC ---
Review of attachment 263627:
 --> (https://bugzilla.gnome.org/review?bug=719938&attachment=263627)

::: gst/rtpmanager/gstrtpbin.c
@@ +2523,3 @@
+  g_value_set_object (&ret, NULL);
+
+  g_signal_emitv (args, gst_rtp_bin_signals[signal], 0, &ret);

Why is this not just g_signal_emit (session->bin, gst_rtp_bin_signals[signal],
session->id, &ret); ?

@@ +2547,3 @@
+    } else {
+      GST_DEBUG_OBJECT (session->bin, "adding requested encoder %p", encoder);
+      gst_bin_add (GST_BIN_CAST (session->bin), encoder);

This should check the return value.

@@ +2862,3 @@
+    GstPadTemplate *etempl;
+
+    session->rtp_dec_queue = gst_element_factory_make ("queue", NULL);

Why did you add a queue?

@@ +2870,3 @@
+    GST_DEBUG_OBJECT (rtpbin, "linking RTP decoder");
+    decsink = gst_element_get_static_pad (decoder, "rtp_sink");
+    gst_pad_link_full (qsrc, decsink, GST_PAD_LINK_CHECK_NOTHING);

The decoder is application provided, this should probably do full checking, and
check the return value.

@@ +2876,3 @@
+    decsrc = gst_element_get_static_pad (decoder, "rtp_src");
+    gst_pad_link_full (decsrc, session->recv_rtp_sink,
+        GST_PAD_LINK_CHECK_NOTHING);

Same here.

@@ +2885,3 @@
+    templ = gst_pad_template_new (GST_PAD_TEMPLATE_NAME_TEMPLATE (templ),
+        GST_PAD_TEMPLATE_DIRECTION (templ), GST_PAD_REQUEST,
+        GST_PAD_TEMPLATE_CAPS (etempl));

Creating a fake template looks like a hack.

@@ +3151,3 @@
+    GstPadTemplate *etempl;
+
+    session->rtp_enc_queue = gst_element_factory_make ("queue", NULL);

Again why the queue ?

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