[FFmpeg-devel] [PATCH] urlconcat protocol

[Benoit Fouet]

Hi,

On Tue, 26 Jan 2010 08:01:29 +0100 Michele Orr? wrote:
> HI. As the wiki says, I'm getting throught ffmpeg with a "small and
> interesting patch": the Wolfram Gloger's "fileconcat" protocol.
> I't a simple protocol which let you concat one/more url.
> 
> : http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2007-June/031256.html
> : http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2007-July/032121.html
> 
> I tried to follow each single indication, and what i got is in the
> attachment :)
> 
> I've mainly changed how concat.c manages its urls. I don't like at all to
> call realloc() for each time we need to append another url. Then, the old
> uifo struct had two ints, one for the length (yes, repeated for each node)
> and another for the index of the node.  MIne insted has two pointers, one
> for the previous url, and another for the next, so the final size shuoldn't
> change.
> 
> All this aims to a better management of memory, and improve errors handling.
> 

> Index: libavformat/concat.c
> ===================================================================
> --- libavformat/concat.c	(revisione 0)
> +++ libavformat/concat.c	(revisione 0)
> @@ -0,0 +1,189 @@
[...]
> +static int urlconcat_open(URLContext *h, const char *filename, int flags)
> +{
> +    char *fn = NULL;
> +    int64_t size;
> +    size_t len;
> +    URLContext *uc;
> +
> +    struct urlconcat_data *udata = av_malloc(sizeof(struct urlconcat_data));
> +    struct urlconcat_info *uinfo = av_malloc(sizeof(struct urlconcat_info));

sizeof(*udata) and sizeof(*uinfo)

> +    if (!udata || !uinfo) 
> +        return AVERROR(ENOMEM);

this is leaky if udata is not NULL

> +    h->priv_data = udata;
> +    udata->current = udata->begin = uinfo;
> +
> +    av_strstart(filename, "cat:", &filename);
> +    
> +    if   (!*filename) 
> +        return AVERROR(ENOENT);

you're leaking udata and uinfo memory here (and BTW, every return
AVERROR(...) is leaky in this function)

> +    while (*filename) {
> +        /* parsing filename */
> +        for (len = 0; filename[len] && filename[len] != AV_CAT_SEPARATOR; ++len);
> +        fn = av_realloc(fn, sizeof(char) * len);

you can remove the sizeof(char)

> +        if (!fn)
> +            return AVERROR(ENOMEM);
> +        av_strlcpy(fn, filename, len+1);
> +        filename += len;
> +        while (*filename) ++filename;
> +        /* creating URLContext */
> +        if (url_open(&uc, fn, flags) < 0)
> +            return AVERROR(ENOENT);
> +        /* creating size */
> +        size = url_filesize(uc);
> +        if (size < 0)
> +            return AVERROR(ENOSYS);
> +        /* assembling */
> +        uinfo->uc   = uc;
> +        uinfo->size = size;
> +
> +        uinfo->nexturl = av_malloc(sizeof(struct urlconcat_info));
> +        if (!uinfo->nexturl)
> +            return AVERROR(ENOMEM);
> +        uinfo->nexturl->prevurl = uinfo;
> +        uinfo = uinfo->nexturl;
> +    }
> +    /* switching back to previous url (last node),  *
> +     * and connecting to the beginning of the list. */
> +    uinfo = uinfo->prevurl;
> +    av_freep(&fn);

av_free(fn) should be enough

> +    av_freep(&uinfo->nexturl);
> +    
> +    uinfo->nexturl = udata->begin;
> +    udata->begin->prevurl = uinfo;
> +    return 0;
> +}
> +
> +static int urlconcat_read(URLContext *h, unsigned char *buf, int size)
> +{
> +    int result, total = 0;
> +    struct urlconcat_data *udata = h->priv_data;
> +    struct urlconcat_info *uinfo = udata->current,
> +                          *uend  = udata->begin->prevurl;
> +    while (size > 0) {
> +        result = url_read(uinfo->uc, buf, size);
> +        if (result < 0) 
> +            return result;
> +        else if (result == 0) {
> +            if (uinfo == uend) 
> +                break;
> +            uinfo = uinfo->nexturl;
> +            if (url_seek(uinfo->uc, 0, SEEK_SET) < 0) 
> +                break;
> +        } else {
> +        total += result;
> +        buf   += result;
> +        size  -= result;

indentation

> +        }
> +    }
> +    udata->current = uinfo;
> +    return total;
> +}
> +
[...]
> +static int urlconcat_close(URLContext *h)
> +{
> +    int res = 0;
> +    struct urlconcat_data *udata = h->priv_data;
> +    struct urlconcat_info *uinfo = udata->begin->nexturl;
> +
> +    if (udata) {

this check is happening a bit too late; you already dereferenced udata
above

> +        while (uinfo != udata->begin) {
> +            res |= url_close(uinfo->prevurl->uc);
> +            uinfo = uinfo->nexturl;
> +            av_free(uinfo->prevurl);
> +        } 
> +        av_freep(&h->priv_data);
> +    }
> +    return res < 0 ? AVERROR(EIO) : 0;
> +}
> +

Ben