[FFmpeg-devel] [PATCH] SHOUTcast HTTP Support

[Micah F. Galizia]

On 10-02-21 10:54 AM, Micah F. Galizia wrote:
> 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

Greetings, I've addressed all of the critiques from the shoutcast6.patch 
in shoutcast7.patch. For the most part, it is generally the same idea as 
the previous version, but a few less lines overall.

As with the previous patch, for the chapters, StreamTitle is turned into 
a "title" tag (and an "artist" tag if the format is artist - title), and 
StreamUrl is turned into a url tag. Also, if anyone wants some stream 
dumps with our without metadata I can post some.

ffplay_chaps.patch adds ffplay support for printing the current chapter 
whenever it changes.  I'm not posting it for inclusion (as nobody seemed 
to like the idea on IRC last night), but it does demonstrate the 
functionality. It doesn't look great because we are using avlog with 
ffplay, so the old stream position (27.63 A-V:....) is always showing up 
behind our log lines. This version also uses the master clock instead of 
the time of day.

Both patches have been patchecked and make tested.

Thanks in advance!
-- 
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: shoutcast7.patch
Type: text/x-diff
Size: 14276 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100331/37950088/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffplay_chaps.patch
Type: text/x-diff
Size: 4917 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100331/37950088/attachment-0001.patch>