[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