[FFmpeg-devel] [PATCH] SHOUTcast HTTP Support

[Micah F. Galizia]

On 10-04-06 06:37 PM, Michael Niedermayer wrote:
> On Wed, Mar 31, 2010 at 06:43:10PM -0400, Micah F. Galizia wrote:
>

[...]

>> +    /* there is no shoutcast in shoutcast */
>> +    if (!strcmp(sc->child->name, SHOUTCAST_DEMUXER_NAME)) {
>> +        av_log(s, AV_LOG_ERROR, "ERROR: Child demuxer is %s\n",
>> +               sc->child->name);
>> +        return AVERROR_IO;
>> +    }
>
> thats fragile, we just need a second shoutcast like demxuer and
> each with such check would not be enough

I have observed the s->iformat and sc->child pointers values are the 
same with when both are shoutcast demuxers.  Can I rely on that, or did 
you have something else in mind?

>
>> +
>> +    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 AVERROR_NOMEM;
>> +    }
>> +
>> +    /* read past the HTTP header (after probing the stream is reset) */
>> +    url_fseek(s->pb, offset, SEEK_CUR);
>> +
>
>> +    /* ensure that we set the data position ahead if the child demuxer moves
>> +     * the buffer pointer forward. TODO Use pb->pos if possible. */
>> +    offset = (unsigned long)(s->pb->buf_ptr);
>
> whatever this is it has undefined behavior
>

What I need is to know how the child demuxer has manipulated the buffer 
(did it move the pointer forwards or backwards?). I need to know this 
because it affects the  index within the actual data between metadata 
chunks. If this is unacceptable and there isn't a preferred way getting 
this information, I can try to seek back to the beginning of the stream 
and failing that close, reopen, and seek beyond the http header to the 
start of real data.

>> +    s->priv_data = sc->child_priv;
>> +    ret = sc->child->read_header(s, ap);
>> +    s->priv_data = sc;
>> +    sc->datapos = (unsigned long)(s->pb->buf_ptr) - offset;
>> +
>> +    return ret;
>> +}
>> +
>
>> +static int shoutcast_read_packet(struct AVFormatContext *s, AVPacket *pkt)
>> +{
>> +    int ret;
>> +    ByteIOContext *pb = s->pb;
>> +    SHOUTcast *sc = s->priv_data;
>> +
>> +    /* allow the child demuxer to read a packet. */
>> +    s->priv_data = sc->child_priv;
>> +    if ((ret = sc->child->read_packet(s, pkt))<  0) {
>> +        return ret;
>> +    }
>> +    s->priv_data = sc;
>
>> +    sc->datapos += pkt->size;
>
> what is datapos supposed to be?
> this just looks wrong, it surely is not the "file" position

No, it is a counter of how much non-metadata data has been read.  When 
it is greater than our metadata interval, we must strip the icy metadata 
out before passing more non-metadata to the child demuxer.

>
>> +
>> +    if (sc->metaint&&  (sc->datapos>  sc->metaint)) {
>> +        unsigned int lm_size = 0, md_size = 0, rm_size = 0, rd_size = 0;
>> +        unsigned int ld_size = pkt->size - sc->datapos + sc->metaint;
>> +        AVPacket pkt2;
>> +
>> +        /* store the metadata segment size */
>> +        md_size = pkt->data[ld_size] * 16;
>> +        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;
>
> lm,md,rm,rd are what?

Poorly named. Originally lm/rm = left/right metadata (the end of our 
packet being the point of reference), md = metadata and rd = right data 
(data following metadata chunk). I wanted to keep the names small to 
avoid excessive line wrapping.  Can I just add a comment describing what 
they all are or would name changes be preferred?

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