[FFmpeg-devel] [PATCH] SHOUTcast HTTP Support

Michael Niedermayer michaelni
Sun Feb 21 15:08:59 CET 2010


On Sat, Feb 20, 2010 at 04:51:47PM -0500, Micah F. Galizia wrote:
> On 10-01-25 10:25 AM, Ronald S. Bultje wrote:
>> Hi,
>>
>> On Sun, Jan 24, 2010 at 3:20 PM, Micah F. Galizia
>> <micahgalizia at gmail.com>  wrote:
>>> This implementation just wraps the mp3 demuxer.  I'm able to do this 
>>> because
>>> the mp3 demuxer doesn't actually use private data
>>> (AVFormatContext::priv_data).  If this changes, or wrapping a different
>>> demuxer that use priv_data is necessary, the shoutcast demuxer could
>>> maintain a pointer to the wrapped demuxers private data, and swap it 
>>> before
>>> calling into the wrapped demuxer.
>>
>> As Michael said, you can open a new demuxer from this demuxer, that's
>> e.g. what the mov demuxer does for dv, etc. I think that's cleaner.
>>
>>> The read_probe method of the demuxer is a little long because AVProbeData
>>> does not include any way to reference the URLContext that has already 
>>> been
>>> opened. As a result, I have to open a new one to check if the stream
>>> supports ICY.
>>
>> You're getting closer, but what I meant is that you should find some
>> way to transfer the HTTP header metadata into the probe function of
>> shout, so it can parse the metaint straight from the AVMetadata,
>> rather than re-opening the connection. If this needs any changes in
>> AVProbeData, we can certainly consider them.
>>
>> Other than that, it's starting to look good, thanks for you work!
>
> Greetings again,
>
> Attached is another version of the patch.  This version is not dependent on 
> HTTP or MP3 and all SHOUTcast metadata is handled in the demuxer. The 
> SHOUTcast demuxer does reuse the HTTP header processing code (regardless of 
> if this is a stream dumped to a file or the actual stream itself), so the 
> process_line method of http.c has been changed to make it more reusable.
>
> This change does not affect how we play non-icy http streams since 
> http_open_cnx will continue to do the header processing unless the 
> shoutcast demuxer is configured and the stream supports it. So basically, 
> we only use the shoutcast demuxer if we are really in a shoutcast stream 
> (not if we are just playing a file over http).
>
> I know that previously Michael said that ad-hoc wrapping hacks are not 
> favorable, but I did look at how this sort of thing is done elsewhere, and 
> for mov/DV we explicitly make calls into the DV demuxer, which doesn't 
> help. I also searched for general demuxer chaining (on the list and through 
> the code) and I couldn't find anything. So, in the interest in keeping the 
> changes small (and contained within the demuxer) I am still wrapping a 
> child demuxer. That said, we do probe for the format, and I've tested this 
> successfully with mp3 and vorbis streams (one is listed at dir.xiph.org).
>
> The metadata works out nicely too as the icy-* header data is used for the 
> overall AVFormatContext and the in-band periodic track artist/title updates 
> are used for chapters. I had a look at using an AVMetadataConv for the 
> demuxer (since other demuxers have them) but from what I could tell, they 
> are not actually used (unless the intent is that the application calls 
> metadata_conv?), so I didn't bother including one.
>
> As with the previous few patches, there is some throw-away code in ffplay.c 
> to demonstrate the use of chapters.
>
> I think that is about all (other than the patch), so TIA!
>
> -- 
> 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

>  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


> +
> +                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


> 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



> @@ -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 ?


>      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

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


[...]
> +
> +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

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

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.
-------------- 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/20100221/7f821819/attachment.pgp>



More information about the ffmpeg-devel mailing list