[FFmpeg-devel] [PATCH] urlconcat protocol

[Stefano Sabatini]

On date Tuesday 2010-01-26 18:55:30 +0100, Michele Orr? encoded:
> 2010/1/26 Michele Orr? <maker.py at gmail.com>
> >
> >
> > 2010/1/26 Benoit Fouet <benoit.fouet at free.fr>
> >
> >> Hi,
> >>
> >> [..]
> >
> > ok, thanks, I forgot checking for memory leak ( :O ).  Now it should be ok.
> >
> > Regards,
> >     Michele Orr?.
> >
> 
> Oh, sorry, please use this patch insted.

> Index: libavformat/concat.c
> ===================================================================
> --- libavformat/concat.c	(revisione 0)
> +++ libavformat/concat.c	(revisione 0)
> @@ -0,0 +1,194 @@
> +/*
> + * Concat URL protocol
> + * Copyright (c) 2006 Steve Lhomme
> + * Copyright (c) 2007 Wolfram Gloger
> + * Copyright (c) 2010 Michele Orru'

Are not we using UTF-8 in files? In that case Orr? is fine.

First of all, won't this feature be superseded by the concat muxer/demuxer
(that implemented in the GSOC concat repo)? (This is more intended as a
question to the other devs.)

> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include <fcntl.h>
> +
> +#include "avformat.h"
> +#include "libavutil/avstring.h"
> +#include "libavutil/mem.h"
> +
> +#define AV_CAT_SEPARATOR '|'
> +
> +struct urlconcat_info {
> +    URLContext *           uc;

Nit: * ... uc -> *uc;

> +    int64_t                size;
> +};
> +
> +struct urlconcat_data {
> +    struct urlconcat_info *urls;
> +    size_t                 length;
> +    size_t                 current;
> +};

Please doxify.

Also I'm not very happy with the urlconcat_info name, "info" is easily
confused with "data", maybe something like urlconcat_entry -> entries
may be better.

> +static int urlconcat_open(URLContext *h, const char *filename, int flags)
> +{
> +    char *fn = NULL;
> +    int err = 0;
> +    int64_t size;
> +    size_t  len, i;
> +    URLContext *uc;
> +    struct urlconcat_data *udata;
> +    struct urlconcat_info *uinfo;
> +
> +    av_strstart(filename, "cat:", &filename);
> +
> +    /* creating udata */
> +    udata = av_mallocz(sizeof(*udata));
> +    if (!udata)
> +        return AVERROR(ENOMEM);
> +    h->priv_data = udata;
> +    /* creating udata->urls */
> +    for (i=0, len = 1; filename[i]; i++)  // cat:[url]|[url]->urls = sep+1
> +        if (filename[i] == AV_CAT_SEPARATOR) len++;
> +    uinfo = av_malloc(sizeof(*uinfo) * len);
> +    if (!uinfo) {
> +        av_free(udata);
> +        h->priv_data = NULL;
> +        return AVERROR(ENOMEM);
> +    } else
> +        udata->urls = uinfo;
> +
> +    /* handle input */
> +    if (!*filename) err = AVERROR(ENOENT);
> +    for (i = 0; *filename; i++) {
> +        /* parsing filename */
> +        for (len = 0; filename[len] && filename[len] != AV_CAT_SEPARATOR; ++len);
> +        fn = av_realloc(fn, len);
> +        if (!fn) {
> +            err = AVERROR(ENOMEM);
> +            break;
> +        }
> +        av_strlcpy(fn, filename, len+1);
> +        filename += len;
> +        if (*filename == AV_CAT_SEPARATOR) ++filename;
> +        /* creating URLContext */
> +        if (url_open(&uc, fn, flags) < 0) {
> +            err = AVERROR(ENOENT);
> +            break;
> +        }

Suggestion: maybe it's just simpler to use goto in case of failure,
and deinit stuff just in a single point:

        do_all_fine();
        return 0;
fail:
        free_stuff();
        return ret;

> +        /* creating size */
> +        size = url_filesize(uc);
> +        if (size < 0) {
> +            err = AVERROR(ENOSYS);
> +            break;
> +        }
> +        /* assembling */
> +        uinfo[i].uc   = uc;
> +        uinfo[i].size = size;
> +    }
> +    av_free(fn);
> +    if (err < 0) {
> +        av_free(uinfo);
> +        av_free(udata);
> +        h->priv_data = NULL;
> +        return err;
> +    }
> +
> +    udata->length = i;
> +    //av_realloc(uinfo, udata->length+1);
> +    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->urls;


> +    size_t i = udata->current;
> +
> +    while (size > 0) {
> +        result = url_read(uinfo[i].uc, buf, size);
> +        if (result < 0) 
> +            return result;
> +        else if (result == 0)
> +            if (i + 1 == udata->length ||
> +                url_seek(uinfo[++i].uc, 0, SEEK_SET) < 0)
> +                break;
> +        total += result;
> +        buf   += result;
> +        size  -= result;
> +    }
> +    udata->current = i;
> +    return total;
> +}
> +
> +static int64_t urlconcat_seek(URLContext *h, int64_t pos, int whence)
> +{
> +    int64_t result;
> +    struct urlconcat_data *udata = h->priv_data;
> +    struct urlconcat_info *uinfo = udata->urls;

Since uinfo is used to reference an array, maybe it's better to use a
variable name which is a plural form.

> +    size_t i;
> +
> +    switch (whence) {
> +    case SEEK_END:
> +        for (i = udata->length - 1; 
> +             i != 0 && pos < -uinfo[i-1].size;
> +             i--)
> +            pos += uinfo[i-1].size;
> +        break;
> +    case SEEK_CUR:
> +        /* get the absolute position */
> +        for (i = 0; i != udata->current; i++)
> +            pos += uinfo[i].size;
> +        pos += url_seek(uinfo[i].uc, 0, SEEK_CUR);
> +        whence = SEEK_SET;
> +        /* fall through with the absolute position */
> +    case SEEK_SET:
> +        for (i = 0; i != udata->length - 1 && pos >= uinfo[i].size; i++)
> +            pos -= uinfo->size;
> +        break;
> +    default:
> +        return AVERROR(EINVAL);
> +    }
> +    result = url_seek(uinfo[i].uc, pos, whence);
> +    if (result >= 0) {
> +        udata->current = i;
> +        while (i != 0)
> +            result += uinfo[i--].size;
> +    }
> +    return result;
> +}
> +
> +static int urlconcat_close(URLContext *h)
> +{
> +    int err = 0;
> +    size_t i;
> +    struct urlconcat_data *udata = h->priv_data;
> +    struct urlconcat_info *uinfo = udata->urls;
> +
> +    for (i=0; i != udata->length; i++)
> +       err |= url_close(uinfo[i].uc);

Why '|=' ?

> +    av_free(uinfo);
> +    av_free(udata);
> +    h->priv_data = NULL;
> +    return err < 0 ? AVERROR(EIO) : 0;

Why not simply err?

Regards, and thanks for the patch!
-- 
FFmpeg = Furious & Faboulous Mind-dumbing Plastic Elastic God