GNOME Bugzilla – Bug 797174
rtsp-server: Receiver streams affect latency on the whole pipeline and cause poor performance in senders
Last modified: 2018-10-03 10:28:04 UTC
Created attachment 373702 [details] [review] encapsulate receiving part into a bin This problem is easily reproducible when the configured media has a video sender stream in combination with backchannel receiver stream. The latency of the backchannel sink will be distributed to all sinks in the pipeline and cause poor video performance. The proposed patch encapsulates the receiver part into a bin and thus prevents it from affecting latency on the complete pipeline. It also makes sure that correct latency will be configured for the receiver. As the patch looks now, it is a bit backchannel specific. The bin is added from gst_rtsp_onvif_media_collect_backchannel() when backchannel stream is present. A more generic solution would be to do the work in gst_rtsp_media_collect_streams() whenever the media contains both senders and receivers. However, this will make the code a bit more complicated and backchannel is the only case today when this scenario can happen, so i am not so sure?
Review of attachment 373702 [details] [review]: Only a very short review for now ::: gst/rtsp-server/Makefile.am @@ +33,3 @@ rtsp-sdp.c \ rtsp-thread-pool.c \ + rtsp-latency-bin.c \ Also has to be added to meson.build, and please don't install the header :) Let's keep it private API for now. ::: gst/rtsp-server/rtsp-onvif-media.c @@ +272,3 @@ + + gst_object_ref (backchannel_bin); + gst_bin_remove (GST_BIN (element), backchannel_bin); This needs some comment. Why are you removing the elements, the additional ref, etc. It's a hack for the time being until there's a more generic solution in collect_streams() :)
Review of attachment 373702 [details] [review]: ::: gst/rtsp-server/rtsp-latency-bin.c @@ +196,3 @@ + + switch (GST_MESSAGE_TYPE (message)) { + case GST_MESSAGE_LATENCY:{ You should do the same in change_state() when going from PAUSED->PLAYING or PLAYING->PLAYING. I know that it will fail for your use-case, but in other cases there is never a LATENCY message and it will succeed during the state change @@ +215,3 @@ + if (!gst_element_query (target_element, query)) { + GST_WARNING_OBJECT (bin, "latency query failed"); + gst_object_unref (target_element); You leak the query here @@ +249,3 @@ + */ +GstElement * +gst_rtsp_latency_bin_new (void) This would be nicer by just taking the internal bin/element as a constructor parameter here. Then you don't need to work with the element-added signal, and it won't work well for multiple elements anyway
Created attachment 373769 [details] [review] encapsulate receiving part into a bin updated version
Review of attachment 373769 [details] [review]: Just minor things left, thanks! ::: gst/rtsp-server/rtsp-latency-bin.c @@ +125,3 @@ + case PROP_ELEMENT: + if (!gst_rtsp_latency_bin_add_element (latency_bin, + g_value_get_object (value))) { This is problematic: properties should always be transfer-none :) @@ +308,3 @@ + + switch (transition) { + case GST_STATE_CHANGE_PAUSED_TO_PLAYING: Also PLAYING_TO_PLAYING
Created attachment 373830 [details] [review] encapsulate receiving part into a bin Updated version
commit 7b5e232a9ed1c414fef5d5426cbf382b06d57350 (HEAD -> master) Author: Ognyan Tonchev <ognyan@axis.com> Date: Wed Sep 12 11:55:15 2018 +0200 onvif: encapsulate onvif part into a bin ...and thus do not let onvif affect pipelines latency https://bugzilla.gnome.org/show_bug.cgi?id=797174