[FFmpeg-devel] [PATCH] SHOUTcast HTTP Support

[Michael Niedermayer]

On Tue, Apr 06, 2010 at 09:06:35PM -0400, Micah F. Galizia wrote:
> 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?

shoutcast(foobar(shoutcast(foobar(...))))

checking the immedeate first child is not enough
it depends on the non existence of a second shoutcast like demuxer.
thats fragile as we would not consider that when we add such a demuxer


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

url_ftell() ?


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

still sounds wrong, the sum or packet sizes is not the same as the num
of bytes the child demuer consumed. These might match for mp3 but will
not match for most other cases


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

as they are just 4, iam fine with either but i slightly prefer better names

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

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- 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/20100407/c14001fb/attachment.pgp>