GNOME Bugzilla – Bug 781446
rtsp: Add support for the new RTSP 2.0 protocol (rfc7826)
Last modified: 2017-10-05 16:39:39 UTC
Currently our RTSP stack only supports RTSP 1.X (RFC 2326) which has been obsoleted by the version 2.0 of the protocol[0]. We should implement support for the new version. It is not backward compatible but it is most probably possible to add support for it in our current implementation, it has to be tried though. [0] https://tools.ietf.org/html/rfc7826
Created attachment 354929 [details] [review] rtsp: Start implementing support RTSP 2.0 Properly handle protocol version in the connection Add the following headers types: * Pipelined-Request * Media-Properties * Seek-Style * Accept-Ranges
Created attachment 354930 [details] [review] rtsp: Start implementing support for RTSP 2.0 - Handle version negotation: Added a `default-version` property so that the user can configure what to use in case the server does not support version negotation (which actually exist) - Handle pipelined requests, which allow avoiding full round trip to setup the RTP streams (request are sent in a raw, and response are handled as they arrive). - Handle the new Media-Properties header - Handle the new Seek-Style header - Handle the new Accept-Ranges header Handling of IPV6 should already be OK. We are still missing (at least) the following features (which do not seem really mandatory as they require a "persistent connection between server and client"): - Server to Client TEARDOWN command (Not so usefull fmpov) - PLAY_NOTIFY (not needed for our server yet) - Support for the new REDIRECT features and probably some more protocol changes might not be handled yet.
Created attachment 354931 [details] [review] Start support for RTSP 2.0 This adds basic support for new 2.0 features, though the protocol is subposdely backward incompatible, most semantics are the sames. This commit adds: - features: * version negotiation * pipelined requests support * Media-Properties support * Accept-Ranges support - APIs: * gst_rtsp_media_seekable The RTSP methods that have been removed when using 2.0 now return BAD_REQUEST.
Created attachment 354932 [details] [review] rtsp: Start implementing support for RTSP 2.0 - Handle version negotation: Added a `default-version` property so that the user can configure what to use in case the server does not support version negotation (which actually exist) - Handle pipelined requests, which allow avoiding full round trip to setup the RTP streams (request are sent in a raw, and response are handled as they arrive). - Handle the new Media-Properties header - Handle the new Seek-Style header - Handle the new Accept-Ranges header Handling of IPV6 should already be OK. We are still missing (at least) the following features (which do not seem really mandatory as they require a "persistent connection between server and client"): - Server to Client TEARDOWN command (Not so usefull fmpov) - PLAY_NOTIFY (not needed for our server yet) - Support for the new REDIRECT features and probably some more protocol changes might not be handled yet.
Created attachment 354933 [details] [review] validate: launcher: Add an option to run the testsuite with RTSP 2.0
This first set of patches implements basic support for RTSP 2.0, I might still be missing some semantic changes, but overall it should be quite compliant with the new version of the standard. This is the first RTSP 2.0 implementation (I asked protocol authors and this is the answer I got from them!) so I developed both the server and the client at the same time. As stated in the commit messages it is still missing some features, but those should not be really important for a start. Also support for extensions has not been implemented for the simple reason there is not real extension defined yet (afaict). Those patches are to be applied in the different GStreamer repositories as follow: * 354929: gst-plugins-base * 354932: gst-plugins-good * 354931: gst-rtsp-server * 354933: gst-devtools I am also mantaining up to date branches on the following repositories: * https://github.com/thiblahute/gst-plugins-base/commits/rtsp2.0 * https://github.com/thiblahute/gst-plugins-good/commits/rtsp2.0 * https://github.com/thiblahute/gst-rtsp-server/commits/rtsp2.0 * https://github.com/thiblahute/gst-devtools/commits/rtsp2.0 Extra care has been taken to make sure to not break 1.0 support, and a -validate testsuite has been previously added, no regression has been found (yet:)).
Review of attachment 354929 [details] [review]: Looking good, a few unrelated changes but I don't know if that matters, also need to not forget to update the Since comments :) ::: gst-libs/gst/rtsp/gstrtspconnection.c @@ +1661,3 @@ + if (rversion != GST_RTSP_VERSION_1_0 && + rversion != GST_RTSP_VERSION_2_0 && rversion != GST_RTSP_VERSION_2_0) Hm, don't you mean 1_1 instead of one of the two 2_0 ?
Review of attachment 354933 [details] [review]: Would it be possible to run both RTSP 1.0 and RTSP 2.0 in the test suite when just running gst-validate-launcher ?
(In reply to Olivier Crête from comment #8) > Review of attachment 354933 [details] [review] [review]: > > Would it be possible to run both RTSP 1.0 and RTSP 2.0 in the test suite > when just running gst-validate-launcher ? It sure is indeed, both pass all the time here.
Review of attachment 354932 [details] [review]: The Random-Access features handling seems to be a bit incomplete, a casual glance at the RFC sounds like they have added a lot of different modes! Otherwise looks good to me ::: gst/rtsp/gstrtspsrc.c @@ +2513,3 @@ + if (seekable) { + if (src->seekable > 0.0) { + start = src->last_pos - src->seekable * GST_SECOND; Why would start not be 0 ? I understand that this "seekable" value is the maximum distance between two keyframes, no ? Should the value be used here be for Time-Duration? Then you also want segment_end to be src->last_pos + src->time_duration ?
Review of attachment 354931 [details] [review]: Generally looks good, just one little nit related to the one on the rtspsrc patch about the seeking stuff. ::: gst/rtsp-server/rtsp-media.c @@ +127,2 @@ gboolean is_live; + GstClockTimeDiff seekable; Is this really a time diff? The current mode seems to only have -1, 0 and MAXINT ? So like a 3 flags enum? Reading from the spec, this should really be the worse inter-iframe distance (which we can't query in Gst right now). I dont think we have ways to query information about modes like "live + recording" from update elements, I guess ideally we'd add more data to the seeking query for this.
Review of attachment 354932 [details] [review]: > The Random-Access features handling seems to be a bit incomplete, a casual glance at the RFC sounds like they have added a lot of different modes! I tried (and think managed) to synthesis all features related to the seeking possiblities in the seekable field, now, as our sever is not able to do live retention I am probably missing a few features there indeed. ::: gst/rtsp/gstrtspsrc.c @@ +2513,3 @@ + if (seekable) { + if (src->seekable > 0.0) { + start = src->last_pos - src->seekable * GST_SECOND; The seekable value is defined in gst_rtsp_media_seekable as follow: Returns: -1 if the stream is not seekable, 0 if seekable only to the beginning and > 0 to indicate the longest duration between any two random access points. G_MAXINT64 means any value is possible.
Review of attachment 354932 [details] [review]: ::: gst/rtsp/gstrtspsrc.c @@ +2513,3 @@ + if (seekable) { + if (src->seekable > 0.0) { + start = src->last_pos - src->seekable * GST_SECOND; so, according to that logic, in this case, it shoudl do start=0, just it shouldnt have the ACCURATE flag on. Even if seekable is 10, Gstreamer should be able to seek all the way to time 0.. whil putting the segment_start and segment_end to anything else than 0 and duration means that you can't sek outside of that range.
Review of attachment 354932 [details] [review]: ::: gst/rtsp/gstrtspsrc.c @@ +2513,3 @@ + if (seekable) { + if (src->seekable > 0.0) { + start = src->last_pos - src->seekable * GST_SECOND; I guess you mean if seekable == 0. If it is > 0, user can basically seek wherever he wants. In the case seekable == 0, I consider the seek query should let the user know, and it is represented in the following branch with start=0 and duration=0, maybe this is not the right way though, I am not sure. I mean in practice if the user seeks to 1hr, he does not want to seek to 0, which is afaiu what you are proposing here.
Review of attachment 354932 [details] [review]: ::: gst/rtsp/gstrtspsrc.c @@ +2513,3 @@ + if (seekable) { + if (src->seekable > 0.0) { + start = src->last_pos - src->seekable * GST_SECOND; Unless I misunderstand the seeking query, the values passed to the last two parameters of gst_query_set_seeking () are the start and end position to which seeking is possible? So if you where to return "10, 20", a seek to 5 or to 25 should fail (but to 10, 15 or 20 should work). As a side-note, we need better documentation for the gst_query_*_seeking() parameters. The ==0 case is fine, it's the other one I'm worried about.
Review of attachment 354932 [details] [review]: ::: gst/rtsp/gstrtspsrc.c @@ +2513,3 @@ + if (seekable) { + if (src->seekable > 0.0) { + start = src->last_pos - src->seekable * GST_SECOND; Sorry, I misinterpreted you previous comment. The idea here is to make sure that we handle the case where the server is able to do live retention, that formula is wrong now (I changed the meaning of seekable during dev and looks like I did not fix it properly), I think the computation should now be: start = (src->seekable == G_MAXINT64) ? 0 : src->last_pos - src->seekable; Also what you explain here is racy and I think we need the seek to clamp to the start position in the case we have a running window of possible seeking time (not totally related to that issue though).
Created attachment 355229 [details] [review] rtsp: Start implementing support for RTSP 2.0 Fix seek query start result computation.
Created attachment 355230 [details] [review] rtsp: Start implementing support for RTSP 2.0 Fix seek query start result computation.
Created attachment 355547 [details] [review] rtspsrc: Handle TCP as lower transport with RTSP 2.0 Meaning that the interleave fields have to be updated as if streams setup was working when using pipelined setup request. Otherwise there is a mismatch between the server channel count and our own. This also makes RTSP 2.0 over HTTP working.
Dear Thibault Sunier. I download your project and complie in the ubuntu 14.04. I find I can't complie it, because of autogen.sh not work. Can you give me some advice?
Anyone willing to review more? Anything you want me to do to get the ball rolling?
Can you give a cient to test rtsp 2.0？
Created attachment 357749 [details] [review] (-base) rtsp: Start implementing support RTSP 2.0 Since V1: - Fixed s/RTSP_2_0/RTSP_1_1/ typo
Created attachment 357751 [details] [review] (-good) rtsp: Start implementing support for RTSP 2.0 Since V1: - rebase
Created attachment 357752 [details] [review] (-good) - rtspsrc: Handle TCP as lower transport with RTSP 2.0 Since V1: - Rebase
Created attachment 357754 [details] [review] validate: launcher: Run rtsp tests against both V1 and V2 Since V1: - Always run tests against both RTSP 1.X and 2.X
Created attachment 357758 [details] [review] (rtsp-server) - Start support for RTSP 2.0 Since V1: - Rebase
Comment on attachment 357754 [details] [review] validate: launcher: Run rtsp tests against both V1 and V2 Attachment 357754 [details] pushed as 4fac7bf - validate: launcher: Run rtsp tests against both V1 and V2