[FFmpeg-devel] [PATCH] handle RTSP server keep-alive

[Nicolas George]

Le sextidi 6 ventôse, an CCXX, Tommy Winther a écrit :
> This is an attempt at a fix for :
> https://ffmpeg.org/trac/ffmpeg/ticket/945
> and this thread on ffmpeg-user:
> http://ffmpeg.org/pipermail/ffmpeg-user/2012-February/005100.html
> 
> The problem is that the RTSP server sends a keep-alive request as:
> OPTIONS * RTSP/1.0
> CSeq: 1
> Session: 9789-3
> and ffmpeg incorrectly parses this as a RTSP response, thus overwriting
> the cseq.
> 
> The patch checks for the OPTIONS request from the server and sends a
> RTSP/1.0 200 OK response, this fixes the keep-alive and the stream keeps
> playing.
> 
> There is a tcpdump attached to the trac issue with full communication
> for reference.

Thanks for the patch. I do not know the RTSP protocol, therefore, my remarks
are purely general and/or stylistic; hopefully, addressing them immediately
will gain time for those who actually know RTSP.

> 
> br.
> Tommy

> diff --git libavformat/rtsp.c libavformat/rtsp.c
> index 0d919ae..147be51 100644
> --- libavformat/rtsp.c
> +++ libavformat/rtsp.c
> @@ -900,7 +900,7 @@ int ff_rtsp_read_reply(AVFormatContext *s, RTSPMessageHeader *reply,
>      char buf[4096], buf1[1024], *q;
>      unsigned char ch;
>      const char *p;
> -    int ret, content_length, line_count = 0;
> +    int ret, content_length, line_count = 0, optionsRequest = 0;

Please avoid ugly camelCase: options_request; or even better:
is_options_request.

>      unsigned char *content = NULL;
>  
>      memset(reply, 0, sizeof(*reply));
> @@ -936,11 +936,16 @@ int ff_rtsp_read_reply(AVFormatContext *s, RTSPMessageHeader *reply,
>              break;
>          p = buf;
>          if (line_count == 0) {
> -            /* get reply code */
> -            get_word(buf1, sizeof(buf1), &p);
> -            get_word(buf1, sizeof(buf1), &p);
> -            reply->status_code = atoi(buf1);
> -            av_strlcpy(reply->reason, p, sizeof(reply->reason));

Do not reindent lines that you do not change: that way, diff can detect they
are the same line, that makes the patch smaller. Furthermore, the people
reading your patch will know for sure you did not change these lines, your
intent will be more obvious.

> +        	if(strstr(buf, "OPTIONS * RTSP/1.0")) {

Nit: space after if, please.

Your lines are indented with tabs, and tabs, because they mess the
indentation (tabs are usually every 8 columns, but your settings appear to
put them every 4), are automatically rejected by the Git repository
settings.

Also, are you certain that "OPTIONS  *\tRTSP/1.0", with two spaces and then
a tab, can not happen?

> +        		// Check if server sends OPTIONS request (keep-alive)
> +        		optionsRequest = 1;
> +        	} else {
> +				/* get reply code */
> +				get_word(buf1, sizeof(buf1), &p);
> +				get_word(buf1, sizeof(buf1), &p);
> +				reply->status_code = atoi(buf1);
> +				av_strlcpy(reply->reason, p, sizeof(reply->reason));
> +        	}
>          } else {
>              ff_rtsp_parse_line(reply, p, rt, method);
>              av_strlcat(rt->last_reply, p,    sizeof(rt->last_reply));
> @@ -949,6 +954,12 @@ int ff_rtsp_read_reply(AVFormatContext *s, RTSPMessageHeader *reply,
>          line_count++;
>      }
>  
> +    if(optionsRequest) {
> +    	// Keep-alive from server; send RTSP 200 OK response
> +    	ff_rtsp_send_response_async(s, reply);
> +    	return 0;
> +    }
> +
>      if (rt->session_id[0] == '\0' && reply->session_id[0] != '\0')
>          av_strlcpy(rt->session_id, reply->session_id, sizeof(rt->session_id));
>  
> @@ -1101,6 +1112,21 @@ retry:
>      return 0;
>  }
>  
> +int ff_rtsp_send_response_async(AVFormatContext *s, RTSPMessageHeader *request) {
> +    RTSPState *rt = s->priv_data;
> +	char buf[4096], *out_buf = buf;

What is out_buf for? Why not use buf directly?

And isn't buf a bit large for that? 1024 should be plenty enough. Not that
it matters, though.

> +
> +	snprintf(buf, sizeof(buf), "RTSP/1.0 200 OK\r\n");
> +    av_strlcatf(buf, sizeof(buf), "CSeq: %d\r\n", request->seq);
> +    av_strlcatf(buf, sizeof(buf), "Session: %s\r\n", request->session_id);
> +    av_strlcat(buf, "\r\n", sizeof(buf));

This could be done with a single snprintf; it would be more efficient.

> +
> +    ffurl_write(rt->rtsp_hd_out, out_buf, strlen(out_buf));
> +    rt->last_cmd_time = av_gettime();
> +
> +    return 0;
> +}

Your function can not fail, and you do not check its return value: what is
the point of returning 0?

> +
>  int ff_rtsp_make_setup_request(AVFormatContext *s, const char *host, int port,
>                                int lower_transport, const char *real_challenge)
>  {
> diff --git libavformat/rtsp.h libavformat/rtsp.h
> index f67ceb8..b6494ab 100644
> --- libavformat/rtsp.h
> +++ libavformat/rtsp.h
> @@ -553,6 +553,8 @@ int ff_rtsp_make_setup_request(AVFormatContext *s, const char *host, int port,
>   */
>  void ff_rtsp_undo_setup(AVFormatContext *s);
>  
> +int ff_rtsp_send_response_async(AVFormatContext *s, RTSPMessageHeader *request);

This function is used only once, in the same file it is defined: better make
it static. You need to move it up to avoid a forward declaration.

> +
>  extern const AVOption ff_rtsp_options[];
>  
>  #endif /* AVFORMAT_RTSP_H */

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120225/497cd4b8/attachment.asc>