[FFmpeg-devel] [PATCH] SHOUTcast HTTP Support

Micah F. Galizia micahgalizia
Sun Feb 21 16:54:35 CET 2010


On 10-02-21 09:08 AM, Michael Niedermayer wrote:

<SNIP>

Thanks for the review. I will address the problems in a future patch. I 
have a few comments inline below.

>
>>   ffplay.c                 |   33 +++
>>   libavformat/allformats.c |    1
>>   libavformat/http.c       |  388 +++++++++++++++++++++++++++++++++++++++++++----
>>   3 files changed, 394 insertions(+), 28 deletions(-)
>> a854a48a1bf4ad0adf8d2b6b530ae75b86a0ae86  shoutcast6.patch
>> Index: ffplay.c
>> ===================================================================
>> --- ffplay.c	(revision 21882)
>> +++ ffplay.c	(working copy)
>> @@ -1980,7 +1980,10 @@
>>       AVPacket pkt1, *pkt =&pkt1;
>>       AVFormatParameters params, *ap =¶ms;
>>       int eof=0;
>> +    unsigned int chapters = 0;
>> +    int64_t decode_start = av_gettime();
>>
>> +
>>       ic = avformat_alloc_context();
>>
>>       video_index = -1;
>> @@ -2180,6 +2183,36 @@
>>           } else {
>>               av_free_packet(pkt);
>>           }
>> +
>> +        if (ic->nb_chapters != chapters) {
>> +            AVChapter *chapter = NULL;
>> +            int64_t current = (av_gettime() - decode_start)/1000000;
>> +
>> +            /* work backwards in chapters */
>> +            for (i = 0; i<  ic->nb_chapters; i++) {
>> +
>> +                AVChapter *temp = ic->chapters[i];
>
>> +                int64_t start = (temp->start * temp->time_base.num) /
>> +                                temp->time_base.den;
>
> that has only an accuracy of 1 second

Yes. All of the code in ffplay.c s junk and just for quick testing.

>
>> +
>> +                if (current>= start) {
>> +                    chapter = ic->chapters[i];
>> +                }
>> +            }
>> +            chapters = ic->nb_chapters;
>> +
>> +            if (chapter) {
>> +                if (chapter) {
>> +                    AVMetadataTag *t, *a, *u;
>> +                    t = av_metadata_get(chapter->metadata, "title", NULL, 0);
>> +                    a = av_metadata_get(chapter->metadata, "artist", NULL, 0);
>> +                    u = av_metadata_get(chapter->metadata, "url", NULL, 0);
>> +                    if (t) av_log(ic, AV_LOG_INFO, "%s='%s'\n", t->key, t->value);
>> +                    if (a) av_log(ic, AV_LOG_INFO, "%s='%s'\n", a->key, a->value);
>> +                    if (u) av_log(ic, AV_LOG_INFO, "%s='%s'\n", u->key, u->value);
>> +                }
>> +            }
>> +        }
>>       }
>>       /* wait until the end */
>>       while (!is->abort_request) {
>
> this code is wrong
> you cannot just call "time of the day" and expect it to be in any way
> compareable to timestamps of chapters, the user can seek and even if he
> does not this is pretty much wrong

Again, junk code, but I'll correct it anyway.

>
>> Index: libavformat/http.c
>> ===================================================================
>> --- libavformat/http.c	(revision 21882)
>> +++ libavformat/http.c	(working copy)
>> @@ -27,6 +27,10 @@
>>   #include "network.h"
>>   #include "os_support.h"
>>
>> +#ifndef CONFIG_SHOUTCAST_DEMUXER
>> +#define CONFIG_SHOUTCAST_DEMUXER 0
>> +#endif
>> +
>>   /* XXX: POST protocol is not completely implemented because ffmpeg uses
>>      only a subset of it. */
>>
>
> that hunk looks wrong

I will take it out. It is only there to prevent compile errors if it 
were not defined in config.h.

>
>
>> @@ -46,7 +50,7 @@
>>   } HTTPContext;
>>
>>   static int http_connect(URLContext *h, const char *path, const char *hoststr,
>> -                        const char *auth, int *new_location);
>> +                        const char *auth, int *new_location, int icy);
>>   static int http_write(URLContext *h, uint8_t *buf, int size);
>>
>>
>
>> @@ -96,8 +100,18 @@
>>           goto fail;
>>
>>       s->hd = hd;
>> -    if (http_connect(h, path, hoststr, auth,&location_changed)<  0)
>> +#if CONFIG_SHOUTCAST_DEMUXER
>> +    if (http_connect(h, path, hoststr, auth,&location_changed, 1)<  0) {
>> +        s->filesize = -1;
>> +        s->chunksize = -1;
>> +        s->off = 0;
>> +        if (http_connect(h, path, hoststr, auth,&location_changed, 0)<  0)
>> +            goto fail;
>> +    }
>> +#else
>> +    if (http_connect(h, path, hoststr, auth,&location_changed, 0)<  0)
>>           goto fail;
>> +#endif
>
> if(!CONFIG_SHOUTCAST_DEMUXER || http_connect(h, path, hoststr, auth,&location_changed, 1)<  0) {
> ...
>
>
>
>>       if ((s->http_code == 302 || s->http_code == 303)&&  location_changed == 1) {
>>           /* url moved, get next */
>>           url_close(hd);
>> @@ -177,64 +191,97 @@
>>   }
>>
>>   static int process_line(URLContext *h, char *line, int line_count,
>> -                        int *new_location)
>> +                        int *new_location, char **new_line,
>> +                        char **header, char **value)
>>   {
>> -    HTTPContext *s = h->priv_data;
>> +    HTTPContext *s = h ? h->priv_data : NULL;
>
> when can h be NULL ?

I also call process line from shoutcast_read_header. If we are reading a 
file dump of the http stream, we don't actually have an HTTPContext (a 
NULL URLContext indicates this).

>
>>       char *tag, *p;
>>
>> +    /* default to NULL */
>> +    if (header) *header = NULL;
>> +    if (value) *value = NULL;
>> +
>>       /* end of header */
>> -    if (line[0] == '\0')
>> +    if (line[0] == '\0') {
>>           return 0;
>> +    } else if (line[0] == '\r'&&  line[1] == '\n') {
>> +        if (new_line) *new_line = line + 2;
>> +        return 0;
>> +    }
>>
>>       p = line;
>>       if (line_count == 0) {
>> +        int http_code;
>> +
>>           while (!isspace(*p)&&  *p != '\0')
>>               p++;
>>           while (isspace(*p))
>>               p++;
>> -        s->http_code = strtol(p, NULL, 10);
>>
>> -        dprintf(NULL, "http_code=%d\n", s->http_code);
>> +        http_code = strtol(p, NULL, 10);
>> +        if (s) s->http_code = http_code;
>>
>> +        dprintf(NULL, "http_code=%d\n", http_code);
>> +
>>           /* error codes are 4xx and 5xx */
>> -        if (s->http_code>= 400&&  s->http_code<  600)
>> +        if (http_code>= 400&&  http_code<  600)
>>               return -1;
>> +
>>       } else {
>> +        if (header)
>> +            *header = p;
>> +
>>           while (*p != '\0'&&  *p != ':')
>>               p++;
>> -        if (*p != ':')
>> +
>> +        if (*p != ':') {
>> +            if (new_line) *new_line = p;
>>               return 1;
>> +        }
>>
>>           *p = '\0';
>>           tag = line;
>>           p++;
>
>>           while (isspace(*p))
>>               p++;
>> -        if (!strcmp(tag, "Location")) {
>> -            strcpy(s->location, p);
>> -            *new_location = 1;
>> -        } else if (!strcmp (tag, "Content-Length")&&  s->filesize == -1) {
>> -            s->filesize = atoll(p);
>> -        } else if (!strcmp (tag, "Content-Range")) {
>> -            /* "bytes $from-$to/$document_size" */
>> -            const char *slash;
>> -            if (!strncmp (p, "bytes ", 6)) {
>> -                p += 6;
>> -                s->off = atoll(p);
>> -                if ((slash = strchr(p, '/'))&&  strlen(slash)>  0)
>> -                    s->filesize = atoll(slash+1);
>> +        if (value) *value = p;
>> +
>> +        if (s) {
>> +            if (!strcmp(tag, "Location")) {
>> +                strcpy(s->location, p);
>> +                *new_location = 1;
>> +            } else if (!strcmp (tag, "Content-Length")&&  s->filesize == -1) {
>> +                s->filesize = atoll(p);
>> +            } else if (!strcmp (tag, "Content-Range")) {
>
> reindentions should be seperate patches to keep things easy reviewable
> both on the ML and svnlog
>
>
> [...]
>> +AVInputFormat* shoutcast_probe_format(struct AVFormatContext *s, char *buffer, int buf_size)
>> +{
>> +    AVInputFormat *child;
>
>> +    AVProbeData *p;
>> +
>> +    if (!(p = av_malloc(sizeof(AVProbeData)))) {
>> +        return NULL;
>> +    }
>
> AVProbeData p;
> seems more logic to me as it avoids the alloc&free
>
>
>> +
>> +    p->filename = s->filename;
>> +    p->buf_size = buf_size;
>> +
>> +    if (!(p->buf = av_malloc(p->buf_size))) {
>> +        av_free(p);
>> +        return NULL;
>> +    }
>> +
>> +    memcpy(p->buf, buffer, p->buf_size);
>
> and why not p->buf= buffer;
> ?
>
> thats besides your malloc() being to small and segfaulting
> (see AVPROBE_PADDING_SIZE)
>
> also this code is a buggy copy of what is in
> av_open_input_file()
> you need to successifly double the buffer size until PROBE_BUF_MAX
> there are also early termination conditions that we cannot just drop
>
> I think the only way to do this is to split the probing loop out of
> av_open_input_file() in a seperate patch and then use it

ok.

> and the code can possibly overflow the stack by repeatly detecting
> a shoutcast demuxer on a crafted stream

Good point! I will check for that!

>
> [...]
>> +
>> +static int shoutcast_read_header(struct AVFormatContext *s, AVFormatParameters *ap)
>> +{
>> +    int rem, ret;
>> +    unsigned long offset;
>> +    ByteIOContext *pb = s->pb;
>> +    SHOUTcast *sc = s->priv_data;
>> +    char *data;
>> +
>> +    sc->datapos = 0;
>> +    sc->metaint = 0;
>> +    sc->chapters = 0;
>> +    sc->time_base = (AVRational){1,1000000};
>> +    sc->start_time = av_gettime();
>> +    sc->child_priv = NULL;
>> +    s->start_time = 0;
>> +    s->duration = INT64_MAX;
>> +
>> +    if (pb->opaque) {
>> +        int err, new_location, line_count = 0;
>> +        URLContext *ctx = pb->opaque;
>> +
>> +        /* ignore the context if it is not HTTP */
>> +        if (!(ctx->prot&&  (!strncmp(ctx->prot->name, "http", 4)))) {
>> +            ctx = NULL;
>> +        }
>> +
>> +        data = pb->buffer;
>> +
>> +        /* use the header information to initialize the HTTPContext */
>> +        do {
>> +            char *header, *value;
>> +            err = process_line(ctx, data, line_count,&new_location,&data,
>> +&header,&value);
>> +            line_count++;
>> +
>> +            /* set the icy metadata */
>> +            if (header&&  value) {
>> +                if (!strcmp(header, "icy-name")) {
>> +                    av_metadata_set(&s->metadata, "title", value);
>> +                } else if (!strcmp(header, "icy-genre")) {
>> +                    av_metadata_set(&s->metadata, "genre", value);
>> +                } else if (!strcmp(header, "icy-url")) {
>> +                    av_metadata_set(&s->metadata, "url", value);
>> +                } else if (!strcmp(header, "icy-metaint")) {
>> +                    sc->metaint = atoi(value);
>> +                }
>> +            }
>> +        } while (err>  0);
>
> that doesnt look like it would work more than by luck with non http
> buffer and buffer_size might not hold the whole metadata.
> Theres no gurantee that what was in teh first http packet would be
> in the first read block, later may be smaller, or larger
>
> demuxers read data by using things like get_byte or get_partial_buffer
> not accessing internal field of the ByteIOContext
>
>
>> +
>> +        if (err<  0)
>> +            return -1;
>> +    }
>> +
>> +    /* move the buffer beyond the http header data */
>> +    offset = (long)data - (long)pb->buffer;
>> +    rem = pb->buffer_size - offset;
>> +    memcpy(pb->buffer, (pb->buffer + offset), rem);
>> +    memset(pb->buffer + rem, '\0', offset);
>> +    pb->buf_end -= offset;
>> +
>> +    /* find a proper demuxer for the stream data */
>> +    if (!(sc->child = shoutcast_probe_format(s, pb->buffer, rem))) {
>> +        av_log(s, AV_LOG_ERROR,
>> +               "Unable to determine SHOUTcast stream format.\n");
>> +        return -1;
>> +    }
>> +
>> +    if (!(sc->child_priv = av_mallocz(sc->child->priv_data_size))) {
>> +        av_log(s, AV_LOG_ERROR,
>> +               "Unable to allocate SHOUTcast child private data.\n");
>> +        return -1;
>> +    }
>> +
>> +    av_log(s, AV_LOG_INFO, "SHOUTcast child demuxer is %s\n", sc->child->name);
>> +
>> +    /* ensure that we set the data position ahead if the child demuxer moves
>> +     * the buffer pointer forward */
>> +    offset = (unsigned long)(pb->buf_ptr);
>> +    s->priv_data = sc->child_priv;
>> +    ret = sc->child->read_header(s, ap);
>> +    s->priv_data = sc;
>> +    sc->datapos = (unsigned long)(pb->buf_ptr) - offset;
>> +
>> +    return ret;
>> +}
>> +
>
>> +static int shoutcast_read_packet(struct AVFormatContext *s, AVPacket *pkt)
>> +{
>> +    ByteIOContext *pb = s->pb;
>> +    SHOUTcast *sc = s->priv_data;
>> +
>> +    /* allow the child demuxer to read a packet. */
>> +    s->priv_data = sc->child_priv;
>> +    sc->datapos += sc->child->read_packet(s, pkt);
>
> missing handling of error returns
>
>
>> +    s->priv_data = sc;
>> +
>> +    if (sc->metaint&&  (sc->datapos>  sc->metaint)) {
>> +        unsigned long lm_size = 0, md_size = 0, rm_size = 0, rd_size = 0;
>> +        unsigned long ld_size = pkt->size - sc->datapos + sc->metaint;
>> +        AVPacket pkt2;
>
> long can be 32bit, int gurantees 32bit too so there should be no need for
> long in general
>
>
>> +
>> +        /* store the metadata segment size */
>> +        md_size = (*(pkt->data + ld_size) * 16);
>
> pkt->data[ld_size]
>
>
>> +        lm_size = (ld_size + md_size + 1>  pkt->size) ?
>> +                  sc->datapos - sc->metaint - 1 : md_size;
>> +
>> +        /* get remaining metadata size and right data size */
>> +        rm_size = md_size - lm_size;
>> +        rd_size = (md_size>  lm_size) ? 0 : pkt->size - ld_size - md_size - 1;
>> +
>> +        /* read remaining metadata */
>> +        av_new_packet(&pkt2, rm_size);
>> +        av_get_packet(pb,&pkt2, pkt2.size);
>
> first line in av_get_packet() is av_new_packet() your code is broken
> allocating the packet twice having a memleak and allocating a size different
> from what you expect
>
> [...]

-- 
Micah F. Galizia
micahgalizia at gmail.com

"The mark of an immature man is that he wants to die nobly for a cause, 
while the mark of the mature man is that he wants to live humbly for 
one."   --W. Stekel



More information about the ffmpeg-devel mailing list