[FFmpeg-soc] [PATCH] rtsp tunneling

[Martin Storsjö]

On Mon, 7 Jun 2010, Josh Allmann wrote:

> Luca, Martin, any more comments?

#1 looked ok, applied.

Overpedantic nit: The norm for commit messages seems to be imperative 
form, e.g. "Rewrite", "Fix", "Change", instead of describing the commit in 
retrospect saying what you did.

#2, some minor nits:

> @@ -46,12 +47,23 @@ typedef struct {
>      char location[URL_SIZE];
>      HTTPAuthState auth_state;
>      int init;
> +    unsigned char headers[BUFFER_SIZE];
>  } HTTPContext;
>  

The size of this field isn't related at all to the buffer size, so reusing 
BUFFER_SIZE feels wrong IMO.

> +void ff_http_set_headers(URLContext *h, const char *headers)
> +{
> +    HTTPContext *s = h->priv_data;
> +    int len = strlen(headers);
> +
> +    if (len && strcmp("\r\n", headers+len-2))

Some space around the operators would be nice.

> +static inline int has_header(const char *str, const char *header)
> +{
> +    /* header+2 to skip over CRLF prefix. (make sure you have one!) */
> +    return av_stristart(str, header+2, NULL) || av_stristr(str, header);
> +}

This could use spaces around the +, too.

> +
> +     /* set default headers if needed */
> +     if(!has_header(s->headers, "\r\nUser-Agent: "))
> +        len += av_strlcatf(headers+len, sizeof(headers)-len, "User-Agent: %s\r\n", LIBAVFORMAT_IDENT);
> +    if(!has_header(s->headers, "\r\nAccept: "))
> +        len += av_strlcpy(headers+len, "Accept: */*\r\n", sizeof(headers)-len);
> +    if(!has_header(s->headers, "\r\nRange: "))
> +        len += av_strlcatf(headers+len, sizeof(headers)-len, "Range: bytes=%"PRId64"\r\n", s->off);
> +    if(!has_header(s->headers, "\r\nConnection: "))
> +        len += av_strlcpy(headers+len, "Connection: close\r\n", sizeof(headers)-len);
> +    if(!has_header(s->headers, "\r\nHost: "))
> +        len += av_strlcatf(headers+len, sizeof(headers)-len, "Host: %s\r\n", hoststr);

Space between if and the parenthesis, the first if and the comment is off. 
Also, if possible, try to keep lines below 80 characters in length. Also, 
mixing av_strlcpy and av_strlcatf makes it hard to read even if it's 
slightly more efficient this way.

> +/**
> + * Sets custom HTTP headers. Note this overwrites all default options.

Not true any more?

> + * A trailing CRLF ("\r\n") is required for custom headers.
> + * Passing in an empty header string ("\0") will reset to defaults.
> + *
> + * The following headers can be overriden by custom values,
> + * otherwise they will be set to their defaults.
> + *  -User-Agent
> + *  -Accept
> + *  -Range
> + *  -Host
> + *  -Connection
> + *
> + * @param h URL context for this HTTP connection
> + * @param is_chunked 0 to disable chunking, nonzero otherwise.

Uhm, copypaste error? :-)

Applied with most of the nits adjusted.


(later, after writing the above)

And applied bugfixes after noticing them after applying the patches.

Please test error handling in code you modify too, not just the normal 
cases. Preferrably, test it under valgrind too, to catch the use of 
uninitialized buffers. You want to catch those bugs yourself, so I don't 
have to. :-)


Ronald, Josh: What about adding if (!s->hd) return AVERROR(EIO); after the 
if (!s->init) check? Currently, if the delayed open fails, you'd get a 
crash on the second url_read(), if the calling code doesn't check the 
return value from every single call and avoid touching it again after an 
error. Earlier, after a successful url_open(), there was always a working 
s->hd initialized.

Also, Josh, in your patch, you said that the connection is initialized on 
http_read, *write and *seek, but I'm not sure if http_seek handles being 
called before the first url_read/url_write. As the very minimum, it 
requires an if (old_hd) before the url_close at the end.


Now on to the rest of the patches...

// Martin