[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

--- Comment #3 from Aleix Conchillo Flaqué <aleix at oblong.com> 2013-12-06 00:55:11 UTC ---
Thanks for the review!

(In reply to comment #2)
> Review of attachment 263627 [details]:
> 
> ::: 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); ?
> 

True.

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

OK.

> @@ +2862,3 @@
> +    GstPadTemplate *etempl;
> +
> +    session->rtp_dec_queue = gst_element_factory_make ("queue", NULL);
> 
> Why did you add a queue?
> 

I was doing some performance test and it seemed to perform better. I was just
working on that right now, because I was not completely sure.

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

OK.

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

OK.

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

Will look into that.

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

Same as before, I'll check again.

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