[FFmpeg-soc] [PATCH] rtsp tunneling

[Ronald S. Bultje]

Hi,

On Sat, Jun 5, 2010 at 4:28 AM, Josh Allmann <joshua.allmann at gmail.com> wrote:
> Fixed that, and other minor errors (s/handler/handle, spelling fixes
> in the proper commit, etc). Soc repo has been updated as well.
[..]
> +    if (!s->init) {
> +        int ret = http_open_cnx(h);
> +        if (ret != 0) {
> +            av_free (s);

The av_free(s) should be removed in read/write, otherwise subsequent
calls to read/write (or close) will/might crash.

> +/**
> + * Sets custom HTTP headers. Note this overwrites all default options.
> + *
> + * The following headers will always be set, no matter what:
> + *  -HTTP verb, path, HTTP version
> + *  -User-Agent
> + *  -Transfer-Encoding (if applicable)
> + *  - Authorization (if applicable)
> + *
> + * The following headers are set by default, and will be overwritten if
> + * custom headers are set. Be sure to re-specify them if needed.
> + *  -Accept
> + *  -Range
> + *  -Host
> + *  -Connection
> + *
> + * @param h URL context for this HTTP connection
> + * @param is_chunked 0 to disable chunking, nonzero otherwise.
> + */
> +void ff_http_set_headers(URLContext *h, const char *headers);

You shouldn't omit them if missing from the "custom" header. The
expected behaviour if custom header is "Accept: bla\r\n" is that the
default Host is kept (same for the others). Also, please document that
the caller is required to maintain trailing "\r\n" in the custom
header. Assert this in the implementation. Also document that "\0"
resets the custom header.

> @@ -391,7 +398,7 @@ static int http_read(URLContext *h, uint8_t *buf, int size)
>  /* used only when posting data */
>  static int http_write(URLContext *h, const uint8_t *buf, int size)
>  {
> -    char temp[11];  /* 32-bit hex + CRLF + nul */
> +    char temp[11] = "";  /* 32-bit hex + CRLF + nul */
>      int ret;
>      char crlf[] = "\r\n";
>      HTTPContext *s = h->priv_data;
> @@ -413,11 +420,12 @@ static int http_write(URLContext *h, const uint8_t *buf, int size)
>       * signal EOF */
>      if (size > 0) {
>          /* upload data using chunked encoding */
> +        if(s->is_chunked)
>          snprintf(temp, sizeof(temp), "%x\r\n", size);
>
>          if ((ret = url_write(s->hd, temp, strlen(temp))) < 0 ||

is_chunked && (ret = url_write... More generally, feel free to unroll this into:
if (is_chunked){
snprintf();
url_write();
}
url_write();
if (is_chunked)
url_write();

How does the caller or callee define the length of the data if it's
not chunked? We need some defined way of getting that, otherwise this
will break depending on the receiving server implementation.

#4 is fine, if Luca is OK (or Martin), please apply.

> +    /** RTSP transport mode, such as plain or tunneled. */
> +    int mode;

enum RTSPMode mode. RTSPMode is also a little generic, but I don't
really have suggestions for a better name. Luca?
RTSPProtocolConnectionMode?

I think #6 is OK, again Luca/Martin for second review and then it can
be applied.

Ronald