GNOME Bugzilla – Bug 758179
GstRTSPStream : Create pipeline based on enabled transport type
Last modified: 2016-08-25 14:09:55 UTC
In the current setup of RTSP stream pipeline, the server will add tee, queue and multiudpsink even though UDP is not enabled. But when TCP is not enabled it only adds the udp related elements. Because of the queues and tees, the overall performance of the pipeline degrades. To avoid this situation, if only TCP or only UDP is set as the transport protocol in SETUP request, it will not add the extra tee or queue elements to the pipeline. The queues (appqueue & udpqueue) and tee elements will be added, if the media supports both TCP and UDP protocols. This improves the pipeline performance when one of the protocols is enabled.
I will provide a patch soon for this issue.
Created attachment 315688 [details] [review] Patch for creating pipeline based on transport protocol
Review of attachment 315688 [details] [review]: ::: gst/rtsp-server/rtsp-client.c @@ +1826,3 @@ + /* need to suspend the media, if the protocol has changed */ + if (media != NULL) + gst_rtsp_media_suspend (media); You can't do that in the general case, because the media may be shared between multiple sessions. For example if you have multiple viewers for the same camera. You can probably just always leave the tee there and then add or remove branches. If you gst_element_release_request_pad() on a tee src pad, then you can unlink the downstream bits while it's playing.
To give you a small background of the issue: Queues have a huge impact on performance for us and apart from that we never share GstRTSPMedia's, or we do actually share but only for multicast clients. Long time ago we had a discussion with wim about that and he suggested allowing specific transport on a per media level based on RTSP SETUP and having 2 factories, one creating GstRTPMedia's for streaming TCP and another one for streaming UDP. That sounded feasible but now I see that the server will add tee, queue and udpsink even though UDP is not enabled, the other way around seem to be ok though (when only UDP is enabled)
(In reply to Olivier Crête from comment #3) > Review of attachment 315688 [details] [review] [review]: > > ::: gst/rtsp-server/rtsp-client.c > @@ +1826,3 @@ > + /* need to suspend the media, if the protocol has changed */ > + if (media != NULL) > + gst_rtsp_media_suspend (media); > > You can't do that in the general case, because the media may be shared > between multiple sessions. For example if you have multiple viewers for the > same camera. I should have submitted the above particular change as a separate patch instead. handle_play_request() and handle_setup_request() expect the media to be suspended (for example block-size from setup request will not affect preroll buffer unless the media is suspended). Shared medias are no problems since rtsp_media_suspend() takes care about that (priv->n_active). > > You can probably just always leave the tee there and then add or remove > branches. If you gst_element_release_request_pad() on a tee src pad, then > you can unlink the downstream bits while it's playing. Adding and removing branches dynamically and reconfiguring the pipeline is the right solution and we agree on that. But this patch is still an improvement.
Created attachment 315801 [details] [review] Also avoid creating udp if stream is not UDP I noticed that with your patch, you still create the udpsrc/sink for TCP-only streams. Here is a patch to avoid that.
Srimanta, are you going to split your patch and including Olivier's suggestions?
yes, I will split the patch and give the fix for both.
Created a new ticket to split the fix logically into two parts. New bugzilla ticket : https://bugzilla.gnome.org/show_bug.cgi?id=758268
Created attachment 315837 [details] [review] rtsp-stream: create stream pipeline based on transport included the patch provided as well.
Review of attachment 315837 [details] [review]: Olivier, looks good to you? Srimanta, is there a unit test already that tests all 3 variants? UDP-only, TCP-only, UDP & TCP. ::: gst/rtsp-server/rtsp-stream.c @@ +2218,3 @@ + + if (is_udp && is_tcp) { + g_object_set (priv->appsink[i], "async", FALSE, "sync", FALSE, NULL); Why and why only on the appsink and not both? As we have queues after the tee now on both branches, this should not be required anymore AFAIU and might even cause problems. @@ +2274,3 @@ + * sink used for RTP data, not the RTCP data. */ + if (i == 1) + g_object_set (priv->appsink[i], "async", FALSE, "sync", FALSE, NULL); The comment is wrong I think? This i==1 is the RTCP sink, i==0 is the RTP sink. For RTCP it makes sense to disable both (we should do that for RTCP! But also for the udpsink)
(In reply to Sebastian Dröge (slomo) from comment #11) > Review of attachment 315837 [details] [review] [review]: > > Olivier, looks good to you? > > Srimanta, is there a unit test already that tests all 3 variants? UDP-only, > TCP-only, UDP & TCP. Already test are present in the rtspserver test for these cases. > ::: gst/rtsp-server/rtsp-stream.c > @@ +2218,3 @@ > + > + if (is_udp && is_tcp) { > + g_object_set (priv->appsink[i], "async", FALSE, "sync", FALSE, NULL); > > Why and why only on the appsink and not both? As we have queues after the > tee now on both branches, this should not be required anymore AFAIU and > might even cause problems. Only put the async and sync property on appsink, because by during the creation of the udpsink it is already set to FALSE for async and sync property. > @@ +2274,3 @@ > + * sink used for RTP data, not the RTCP data. */ > + if (i == 1) > + g_object_set (priv->appsink[i], "async", FALSE, "sync", FALSE, > NULL); > > The comment is wrong I think? This i==1 is the RTCP sink, i==0 is the RTP > sink. For RTCP it makes sense to disable both (we should do that for RTCP! > But also for the udpsink) fixed this comment and uploaded a new patch.
Created attachment 316125 [details] [review] rtsp-stream: create stream pipeline based on transport
(In reply to Srimanta Panda (trollkarlen) from comment #12) > > Why and why only on the appsink and not both? As we have queues after the > > tee now on both branches, this should not be required anymore AFAIU and > > might even cause problems. > > Only put the async and sync property on appsink, because by during the > creation of the udpsink it is already set to FALSE for async and sync > property. Ok, so we set it now on all possible rtcp sinks. Good :) Maybe add a comment that for the udpsink we don't have to because we did it elsewhere already. In think in theory this patch is good then, will give it a proper review soon if nobody else is faster.
Comment on attachment 316125 [details] [review] rtsp-stream: create stream pipeline based on transport This does not apply anymore, probably because of my commit from today. But apart from that this looks correct and ready to be merged Can you update it to latest GIT master? I'll merge it then
Created attachment 316743 [details] [review] New patch rebased on master
commit 82dffd17b34c408ab2cf9ce3d991068f44757dd7 Author: Srimanta Panda <srimanta@axis.com> Date: Fri Dec 4 08:01:37 2015 +0100 rtsp-stream: create stream pipeline based on transport Based on the protocol, create the rtsp stream pipeline. If only TCP or only UDP is set as the transport protocol, it will not add the extra tee or queue element to the pipeline. Both these elements will be added, if it supports both TCP and UDP protocols. This improves the pipeline performance when one protocol is present. https://bugzilla.gnome.org/show_bug.cgi?id=758179
*** Bug 722095 has been marked as a duplicate of this bug. ***