GNOME Bugzilla – Bug 772610
rtspsrc doesn't support redirect on play
Last modified: 2016-11-04 17:40:03 UTC
Created attachment 337233 [details] [review] If a redirect is encountered on RTSP PLAY, close, reset protocols and restart the play command As mentioned by comments in rtspsrc.c, redirect is only implemented for the RTSP DESCRIBE call. For servers that redirect on PLAY, redirect sort of works, but only because UDP recv times out and the reconnect behavior ends up doing the job. I suspect that's what was going on with related bug, https://bugzilla.gnome.org/show_bug.cgi?id=768232 The fix for that bug makes sure protocols aren't forced to tcp-only in gst_rtspsrc_reconnect if a src->needs_reconnect is set. However, since reconnect only happens after a successful connect, it must have been trying to reconnect on PLAY. I suspect that only appeared to worked because the initial PLAY call ignored the redirect but then UDP recv subsequently timed out, and then reconnect used the new URL. A better fix would be to implement redirect-on-play as shown in the attached patch.
Review of attachment 337233 [details] [review]: ::: gst/rtsp/gstrtspsrc.c @@ +7326,3 @@ + /* reset protocols to force re-negotiation with redirected url */ + src->cur_protocols = src->protocols; + gst_rtspsrc_loop_send_cmd (src, CMD_PLAY, 0); This basically calls the same function again indirectly later. Couldn't we just do it all here instead of returning and waiting for the thread to call us again?
Created attachment 338205 [details] [review] new patch for bug 772610 (replaces previous patch) @slomo, good catch. I originally thought the indirect approach was necessary to get it to do all of the re-connection logic, but I changed this to a simpler try-again behavior (via goto) and that works fine. (Probably faster as well.) I attached a new patch with that change. The attached patch also backs out Brad Lackey's previous change for bug 768232. After discussing it with him, I confirmed that it was addressing the same redirect-on-play problem, and that this proposed fix obviates his previous fix. (The need_redirect logic was previously only applicable in gst_rtspsrc_reconnect because that case had been ignored gst_rtspsrc_play.)
Review of attachment 338205 [details] [review]: Looks good, thanks! Do you think the previous patch that was merged can cause any harm?
No, the previous patch was safe. It would only come into play if a redirect (on some query after DESCRIBE) was ignored and then UDP recv subsequently timed out, causing a reconnect attempt. By honoring the redirect when it happens, the proposed change just makes the reconnect to the new URL happen sooner. And it makes previous patch superfluous.
commit cd71e3a8e86440a3d9e6b69e1ec529d89e6a13b4 Author: Matt Staples <staples255@gmail.com> Date: Fri Oct 21 17:31:00 2016 +0000 rtspsrc: Also handle redirect on PLAY https://bugzilla.gnome.org/show_bug.cgi?id=772610