[FFmpeg-devel] [PATCH] http Transfer-Encoding chunked

[Michael Niedermayer]

On Mon, Jun 01, 2009 at 01:21:07PM +0200, Peter Holik wrote:
> > On Mon, Jun 01, 2009 at 12:20:03PM +0200, Peter Holik wrote:
> >> > On Wed, May 27, 2009 at 08:54:37AM +0200, Peter Holik wrote:
> >> >> > Hi Peter,
> >> >> >
> >> >> > On Tue, May 26, 2009 at 3:20 PM, Peter Holik <peter at holik.at> wrote:
> >> >> >> i used printf with DEBUG like i saw in http.c:
> >> >> >>
> >> >> >> process_line
> >> >> >>
> >> >> >> #ifdef DEBUG
> >> >> >> ? ? ? ?printf("http_code=%d\n", s->http_code);
> >> >> >> #endif
> >> >> >>
> >> >> >>
> >> >> >> http_connect
> >> >> >>
> >> >> >> #ifdef DEBUG
> >> >> >> ? ? ? ? ? ?printf("header='%s'\n", line);
> >> >> >> #endif
> >> >> >>
> >> >> >>
> >> >> >> why now use av_log?
> >> >> >
> >> >> > That's a good catch. These lines of code are rather old, and most
> >> >> > likely predate the "forbidding" of printf(). They were not converted
> >> >> > for the simple reason that the compilation doesn't fail because DEBUG
> >> >> > is, by default, not included in CFLAGS. A separate patch which
> >> >> > converts them to av_log() at debugging-level would be much
> >> >> > appreciated. Alternatively, they could also be removed.
> >> >> >
> >> >>
> >> > [...]
> >> >> +            for(;;) {
> >> >> +                ch = http_getc(s);
> >> >> +                if (ch < 0)
> >> >> +                    return 0;
> >> >> +                if (ch == '\n') {
> >> >> +                    /* process chunk size */
> >> >> +                    if (q > line && q[-1] == '\r')
> >> >> +                         q--;
> >> >> +                    *q = '\0';
> >> >> +                    /* skip CR LF from last chunk */
> >> >> +                    if (!(*line)) continue;
> >> >> +
> >> >> +                    s->chunksize = strtoll(line, NULL, 16);
> >> >> +
> >> >> +                    av_log(NULL, AV_LOG_DEBUG, "Chunked encoding data size: %"PRId64"'\n",
> >> >> s->chunksize);
> >> >> +
> >> >> +                    if (!s->chunksize)
> >> >> +                        return 0;
> >> >> +                    break;
> >> >> +                } else
> >> >> +                    if ((q - line) < sizeof(line) - 1)
> >> >> +                        *q++ = ch;
> >> >> +            }
> >> >
> >> > looks like code duplication
> >>
> >> looks like, but it is not exactly the same.
> >
> > can it be factorized?
> 
> maybe like this patch?

factorizing http_get_line() out should be a seperate patch


[...]
> @@ -151,6 +152,30 @@ static int http_getc(HTTPContext *s)
>      return *s->buf_ptr++;
>  }
>  
> +static int http_get_line(HTTPContext *s, char *line, int line_size)
> +{
> +    int ch;
> +    char *q;
> +
> +    q = line;
> +    for(;;) {
> +        ch = http_getc(s);
> +        if (ch < 0)
> +            return AVERROR(EIO);
> +        if (ch == '\n') {
> +            /* process line */
> +            if (q > line && q[-1] == '\r')
> +                q--;
> +            *q = '\0';
> +
> +            return 0;
> +        } else {
> +            if ((q - line) < line_size - 1)
> +                *q++ = ch;
> +        }
> +    }
> +}
> +
>  static int process_line(URLContext *h, char *line, int line_count,
>                          int *new_location)
>  {
[...]
> @@ -251,30 +279,18 @@ static int http_connect(URLContext *h, const char *path, const char *hoststr,
>      }
>  
>      /* wait for header */
> -    q = line;
>      for(;;) {
> -        ch = http_getc(s);
> -        if (ch < 0)
> +        if (http_get_line(s, line, sizeof(line)) < 0)
>              return AVERROR(EIO);
> -        if (ch == '\n') {
> -            /* process line */
> -            if (q > line && q[-1] == '\r')
> -                q--;
> -            *q = '\0';
> -#ifdef DEBUG
> -            printf("header='%s'\n", line);
> -#endif
> -            err = process_line(h, line, s->line_count, new_location);
> -            if (err < 0)
> -                return err;
> -            if (err == 0)
> -                break;
> -            s->line_count++;
> -            q = line;
> -        } else {
> -            if ((q - line) < sizeof(line) - 1)
> -                *q++ = ch;
> -        }
> +
> +        av_log(NULL, AV_LOG_DEBUG, "http header='%s'\n", line);
> +
> +        err = process_line(h, line, s->line_count, new_location);
> +        if (err < 0)
> +            return err;
> +        if (err == 0)
> +            break;
> +        s->line_count++;
>      }
>  
>      return (off == s->off) ? 0 : -1;



[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090601/d385da44/attachment.pgp>