GNOME Bugzilla – Bug 747839
gst-rtsp-server: doesn't perform retransmission to both streams in test-video-rtx
Last modified: 2015-04-15 13:15:13 UTC
There are 2 problems. 1. Duplicated call of g_signal_connect During start-up of test-video-rtx, it makes signal handler by code below. - g_signal_connect (rtpbin, "request-aux-sender", (GCallback) request_aux_sender, stream); In case of 2 streams, in the second call of request_aux_sender, userdata is not the second stream but the first stream, which is previous stream. Even though it makes another aux sender bin, but it's not doing retransmission because rtx_pt_map is same as the first aux sender. In this case, signal handler needs to be removed, and makes new handler. 2. In case that payload type is different. In this test-code, it makes pipeline with PCMA payloader, which has "pt=97" But it's changed to 8 later according to spec. Because of this, audio retransmission always failed. Audio's aux sender has wrong rtx_pt_map. IMHO, rtx_pt_map has to be updated in this case. But I'm not sure it's valid. I'm working on this issue, but I want to hear some advice. Thanks
(In reply to Hyunjun from comment #0) > There are 2 problems. > > 1. Duplicated call of g_signal_connect > During start-up of test-video-rtx, it makes signal handler by code below. > - g_signal_connect (rtpbin, "request-aux-sender", (GCallback) > request_aux_sender, stream); > In case of 2 streams, in the second call of request_aux_sender, userdata is > not the second stream but the first stream, which is previous stream. > Even though it makes another aux sender bin, but it's not doing > retransmission because rtx_pt_map is same as the first aux sender. > In this case, signal handler needs to be removed, and makes new handler. Passing the stream there seems wrong then, instead it should pass the media or something and get the relevant streams from it later. > 2. In case that payload type is different. > In this test-code, it makes pipeline with PCMA payloader, which has "pt=97" > But it's changed to 8 later according to spec. > Because of this, audio retransmission always failed. Audio's aux sender has > wrong rtx_pt_map. > IMHO, rtx_pt_map has to be updated in this case. But I'm not sure it's valid. Yes, it would have to be updated. Where exactly is it changed from 97 to 8 later, and how can we know that inside the rtsp server?
> Passing the stream there seems wrong then, instead it should pass the media > or something and get the relevant streams from it later. IMHO, there is nothing which has both streams' information to pass instead of stream. Because rtsp-stream has information for each stream. How about reset signal handler in each stream like below? signal_id = g_signal_lookup ("request-aux-sender", G_OBJECT_TYPE (rtpbin)); /* If there's already signal handler for another stream, it needs to be removed */ if ((sig_handler_id = g_signal_handler_find (rtpbin, G_SIGNAL_MATCH_ID, signal_id, 0, NULL, NULL, NULL)) != 0) { GST_DEBUG_OBJECT (stream, "disconnect previous stream's signal handler"); g_signal_handler_disconnect (rtpbin, sig_handler_id); } > Yes, it would have to be updated. Where exactly is it changed from 97 to 8 later, and how can we know that inside the rtsp server? There is caps_notify callback for signal "notify::caps" from recv_rtp_sink pad. I think I can use this callback to update pt info.
(In reply to Hyunjun from comment #2) > > Passing the stream there seems wrong then, instead it should pass the media > or something and get the relevant streams from it later. > IMHO, there is nothing which has both streams' information to pass instead > of stream. Because rtsp-stream has information for each stream. > > How about reset signal handler in each stream like below? > > signal_id = g_signal_lookup ("request-aux-sender", G_OBJECT_TYPE (rtpbin)); > > /* If there's already signal handler for another stream, it needs to be > removed */ > if ((sig_handler_id = > g_signal_handler_find (rtpbin, G_SIGNAL_MATCH_ID, signal_id, 0, > NULL, NULL, NULL)) != 0) { > GST_DEBUG_OBJECT (stream, "disconnect previous stream's signal > handler"); > g_signal_handler_disconnect (rtpbin, sig_handler_id); > } I don't think that's a good idea, it's not guaranteed that the signal was already emitted at this point with the other stream. > > Yes, it would have to be updated. Where exactly is it changed from 97 to 8 later, and how can we know that inside the rtsp server? > > There is caps_notify callback for signal "notify::caps" from recv_rtp_sink > pad. I think I can use this callback to update pt info. Seems reasonable
(In reply to Sebastian Dröge (slomo) from comment #3) > I don't think that's a good idea, it's not guaranteed that the signal was > already emitted at this point with the other stream. > Yes. It makes sense. I think I try to find other way to fix this.
Created attachment 301524 [details] [review] rtsp-stream: update payload type to rtx_pt_map in rtx sender
Created attachment 301533 [details] [review] rtsp-stream: fix to get valid each stream data for request-aux-sender signal Instead of passing each stream, pass stream array, get the relevant stream, and make aux sender.
Comment on attachment 301524 [details] [review] rtsp-stream: update payload type to rtx_pt_map in rtx sender While this makes sense, I think more is needed. Things like gst_rtsp_stream_get_pt() would still return the wrong pt, as IIRC the property on the payloader does not change I think the correct pt should be set on the payloader and not negotiated via caps in this case. It's inconsistent to have a property and caps for that.
Review of attachment 301533 [details] [review]: ::: gst/rtsp-server/rtsp-media.c @@ +2448,3 @@ + + streams = (GPtrArray *) userdata; + stream = g_ptr_array_index (streams, sessid); I don't think the index is always the same as the session id. You'll have to iterate over all streams and check their index via gst_rtsp_stream_get_index() @@ +2472,3 @@ + /* enable retransmission by setting rtprtxsend as the "aux" element of rtpbin */ + g_signal_connect (priv->rtpbin, "request-aux-sender", + (GCallback) request_aux_sender, priv->streams); The streams array has to be protected by a mutex. Maybe just pass the GstRTSPMedia* here, then take the mutex in the signal handler and then access the streams.
Created attachment 301585 [details] [review] rtsp-stream: fix to get valid each stream data for request-aux-sender signal Instead of passing each stream, pass GstRTSPMedia, get the relevant stream, and make aux sender. And protect streams array by mutex lock.
(In reply to Sebastian Dröge (slomo) from comment #7) > While this makes sense, I think more is needed. Things like > gst_rtsp_stream_get_pt() would still return the wrong pt, as IIRC the > property on the payloader does not change > > I think the correct pt should be set on the payloader and not negotiated via > caps in this case. It's inconsistent to have a property and caps for that. Pcmapayloader update pt in its own setcaps. But as you said, many other payloaders are not doing it. Probably, app should set exact payload type to payloader.
Created attachment 301586 [details] [review] test-video-rtx: set exact payload type to PCMA payloader Setting wrong payload type causes failure to do retransmission through audio stream
Can you file another bug against gst-plugins-good for the pt property problems? commit fabde79bc3edf9b6c47a62ae274ad84b2d444813 Author: Hyunjun Ko <zzoon.ko@samsung.com> Date: Wed Apr 15 10:06:30 2015 +0900 test-video-rtx: set exact payload type to PCMA payloader Setting wrong payload type causes failure to do retransmission through audio stream https://bugzilla.gnome.org/show_bug.cgi?id=747839 commit de590b4b2a8aa1c9de9bf263cd63e23aca4ca0ea Author: Hyunjun Ko <zzoon.ko@samsung.com> Date: Wed Apr 15 09:45:23 2015 +0900 rtsp-stream: fix to get valid each stream data for request-aux-sender signal Because of duplicated g_signal_connect for request-aux-sender signal, wrong stream pointer is passed to the signal handler. Instead of passing each stream, pass stream array and get the relevant stream. https://bugzilla.gnome.org/show_bug.cgi?id=747839