GNOME Bugzilla – Bug 701587
rtsp-client: send new-session signal at the right time.
Last modified: 2014-02-25 22:28:29 UTC
Created attachment 245999 [details] [review] rtsp-client: send new-session signal at the right time Send the new-session signal before the SETUP response has been generated.
Looks good to me, Wim?
@@ -1252,6 +1277,18 @@ handle_setup_request (GstRTSPClient * client, GstRTSPClientState * state) /* we need a new media configuration in this session */ sessmedia = NULL; + + /* find the session in the session pool */ + sessid = gst_rtsp_session_get_sessionid (session); + if (!(session = gst_rtsp_session_pool_find (priv->session_pool, sessid))) + goto no_session; you get the sessionid of a session and then get the session with that id again? also, the previous session would be leaked then. static void handle_request (GstRTSPClient * client, GstRTSPMessage * request) @@ -1672,10 +1693,6 @@ handle_request (GstRTSPClient * client, GstRTSPMessage * request) if (!(session = gst_rtsp_session_pool_find (priv->session_pool, sessid))) goto session_not_found; - /* we add the session to the client list of watched sessions. When a session - * disappears because it times out, we will be notified. If all sessions are - * gone, we will close the connection */ - client_watch_session (client, session); } This bothers me, if a new client connects with an old existing session, it will not be tracked by this client, that's not right.
@@ -1252,6 +1277,18 @@ handle_setup_request (GstRTSPClient * client, GstRTSPClientState * state) /* we need a new media configuration in this session */ sessmedia = NULL; + + /* find the session in the session pool */ + sessid = gst_rtsp_session_get_sessionid (session); + if (!(session = gst_rtsp_session_pool_find (priv->session_pool, sessid))) + goto no_session; I realize, that these code line are not really need. However, I do not understand how the code would result in a leaked (previous?) session. Please explain. static void handle_request (GstRTSPClient * client, GstRTSPMessage * request) @@ -1672,10 +1693,6 @@ handle_request (GstRTSPClient * client, GstRTSPMessage * request) if (!(session = gst_rtsp_session_pool_find (priv->session_pool, sessid))) goto session_not_found; - /* we add the session to the client list of watched sessions. When a session - * disappears because it times out, we will be notified. If all sessions are - * gone, we will close the connection */ - client_watch_session (client, session); } I don't understand your comment. What's done here is that client_watch_session is called as soon as the SETUP request is received, just before the SETUP response is generated and sent to the client. Doing that, the new-session signal is sent at the right time and it's possible e.g, to configure session timeout (<=> we make sure that correct timeout parameter in the session response header is included). Without this patch, the new-signal is sent as soon as the received request includes the session header (<=> the SETUP request has been previously handled).
(In reply to comment #3) > @@ -1252,6 +1277,18 @@ handle_setup_request (GstRTSPClient * client, > GstRTSPClientState * state) > > /* we need a new media configuration in this session */ > sessmedia = NULL; > + > + /* find the session in the session pool */ > + sessid = gst_rtsp_session_get_sessionid (session); > + if (!(session = gst_rtsp_session_pool_find (priv->session_pool, sessid))) > + goto no_session; > > I realize, that these code line are not really need. However, I do not > understand how the code would result in a leaked (previous?) session. Please > explain. > In the lines above, there is a call to gst_rtsp_session_pool_create(), which gives you a ref to a session that you need to unref. Your code then does gst_rtsp_session_pool_find() and overwrites the variable you were supposed to unref. > > static void > handle_request (GstRTSPClient * client, GstRTSPMessage * request) > @@ -1672,10 +1693,6 @@ handle_request (GstRTSPClient * client, GstRTSPMessage * > request) > if (!(session = gst_rtsp_session_pool_find (priv->session_pool, sessid))) > goto session_not_found; > > - /* we add the session to the client list of watched sessions. When a > session > - * disappears because it times out, we will be notified. If all sessions > are > - * gone, we will close the connection */ > - client_watch_session (client, session); > } > > I don't understand your comment. > What's done here is that client_watch_session is called as soon as the SETUP > request is received, just before the SETUP response is generated and sent to > the client. Doing that, the new-session signal is sent at the right time and > it's possible e.g, to configure session timeout (<=> we make sure that correct > timeout parameter in the session response header is included). Without this > patch, the new-signal is sent as soon as the received request includes the > session header (<=> the SETUP request has been previously handled). client_watch_session() makes sure that when a session is closed that we also close the client connection and remove any refs to the session that we have. In RTSP you can connect and do a setup (and make a session). Then you can close the connection (and finalize the client) and reopen the connection (this makes a new client) object. Then you can access your previous session by sessionid. What we need to do is watch this old previous session in the new client so that when it disappears we can close the client. The key is that there is no 1-to-1 relation between client and session.
What exactly do you want to do? change the session timeout?
Yes, I want to change the session timeout.
And send the correct timeout information in the SETUP response.
For that, in the new-session signal, use gst_rtsp_session_set_timeout(). If you change it to something other than 60, it will include a ;timeout= parameter after the sessionid, see gst_rtsp_session_get_header().
Yes, I know but the point is that the new-session signal is sent to late. This is what this whole patch is about ;)
to late -> too late
Ah, now I see. It's because the session is *created* in SETUP but it doesn't call the NEW_SESSION signal.
I slightly changed your patch into 3 parts: - unit tests - signal the new-session only when it is created - watch the newly created session as well commit a8a051f780da6cfc43882b79ffadebaa1a577be8 Author: Patricia Muscalu <patricia@axis.com> Date: Thu Jun 20 12:18:23 2013 +0200 tests: add unit test for new-session See https://bugzilla.gnome.org/show_bug.cgi?id=701587 commit 949f11c643676c6b0aa2317a4a7e0555cb8334b2 Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Thu Jun 20 12:16:07 2013 +0200 client: emit new-session when new session is created Only emit new-session when we created a new session for a client, not when a client picked up a previous session. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=701587 commit fa1d3354c090916e11347989b11fe360186f819e Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Thu Jun 20 12:20:21 2013 +0200 client: also watch newly created session When we newly created a session, start watching it immediately instead of on the next request.
Thank you!