GNOME Bugzilla – Bug 699220
rtsp-sdp: add bandwidth line
Last modified: 2018-05-16 17:08:37 UTC
Created attachment 242795 [details] [review] add sdp bandwidth The following patch adds the bandwidth line to media descriptions. As far as I understand there is no need to add the bandwidth information to the session description block.
Looks fine, but let's remove all these internal g_assert() which seem a bit over the top (this is not super-complicated code after all) and the g_return_*() in the internal function (those should only be used for external API). Also, tag lists may be 'global' or 'stream' tags. I don't know if you want to only take into account stream tags here? (imagine a case where the input comes form demuxer ! parser ! ... then there might be a global taglist with the bandwidth for the container stream and one for the elementary stream).
Please also use GST_EVENT_TYPE (*event).
You may want to fallback on tag "bitrate" if there is no maximum-bitrate..
Yes, thank you for your comments. I'll fix this. However, after I've updated the gstreamer, good and base packages I've noticed that somewhere in the pipeline, the tag event is lost so now the the test_client_sdp (see my patch) fails (frequency: always). Sometimes, I also see the following warning: 0:00:00.044633518 17326 0x9b220f0 WARN GST_PADS gstpad.c:3595:gst_pad_peer_query:<udpsrc1:src> could not send sticky events Is it a new bug?
The problem is in the rtpgstpay: the sticky event is not forwarded to the srcpad. The problem is related to the commit 9532b04947baf701585a8643c90b1e1c48498be3 in the gst-plugins-good. I'll report this as a new bug.
Created attachment 243996 [details] [review] add sdp bandwidth I've fixed the review comments and I also added more unit-tests.
Review of attachment 243996 [details] [review]: ::: gst/rtsp-server/rtsp-sdp.c @@ +58,3 @@ + src_pad = gst_rtsp_stream_get_srcpad (stream); + + gst_pad_sticky_events_foreach (src_pad, get_info_from_tags, stream_media); You should be able to just use gst_pad_get_sticky_event() here
Is the patch correct? Is it something more I should change?
As discussed on IRC, iterating the sticky events is the most efficient here. commit 0951aa37e19d1ad285c24bab6de4b268993a1c28 Author: Patricia Muscalu <patricia@axis.com> Date: Mon Apr 29 14:46:30 2013 +0200 rtsp-sdp: add bandwidth line https://bugzilla.gnome.org/show_bug.cgi?id=699220
This is not quite right, the AS bitrate parameter should contain the bandwidth used by the session manager (which eventually could be derived from the tags).
It's also a bit more complicated than just using the tags: AS should contain the bandwidth with all the IP/UDP/RTP overhead, so it is not equal to the tag bitrate. TIAS (RFC3890) is more similar to the bitrate tag but also requires a maxprate property to be able to add the overhead caused by a particular transport. It looks like these values should be calculated by the session manager (it already does the bandwidth (without IP overhead)). They should probably be placed on the caps of the session manager and be picked up by the rtsp server to fill up the SDP (like it does with other caps fields).
I see. I'll try to solve it.
Ping ?
Status: Not working on a better solution.
Thanks for the update. Closing for now. Please re-open if someone has a better solution.