[FFmpeg-devel] [PATCH] urlconcat protocol

[Stefano Sabatini]

On date Monday 2010-02-01 22:53:17 +0100, Michele Orr? encoded:
> 2010/1/31 Michael Niedermayer <michaelni at gmx.at>

> Index: libavformat/concat.c
[...]
> +#define AV_CAT_SEPARATOR "|"

why a string not a char?

> +
> +struct urlconcat_nodes {
> +    URLContext *uc;

> +    int64_t     size;

A doxy may be useful here.

> +};
> +
> +struct urlconcat_data {

> +    struct urlconcat_nodes *urls;     ///< list of nodes to concat

I insist, please keep struct names / variable names / doxy consistent
or they will confuse the reader, "nodes" here sounds better than
"urls".

> +    size_t                  length;   ///< number of cat'ed nodes
> +    size_t                  current;  ///< index of currently read node
> +};
> +
> +static int urlconcat_open(URLContext *h, const char *uri, int flags)
> +{

> +    char *upath = NULL, *tmpath;

mmh path, uri... a path is just part of an uri, I suggest node_uri
instead.

> +    int err = 0;
> +    int64_t size;
> +    size_t  len, i;
> +    URLContext *uc;
> +    struct urlconcat_data *udata;
> +    struct urlconcat_nodes *unodes;
> +
> +    av_strstart(uri, "cat:", &uri);
> +
> +    /* creating udata */
> +    if (!(udata = av_mallocz(sizeof(*udata))))
> +        return AVERROR(ENOMEM);
> +    h->priv_data = udata;
> +    /* creating udata->urls */

> +    for (i=0, len = 1; uri[i]; i++)  /* cat:[url]|[url] -> urls = sep+1 */
> +        if (uri[i] == *AV_CAT_SEPARATOR)
> +            if (++len == 0) {        /* integer overflow */
> +                av_free(udata);
> +                h->priv_data = NULL;
> +                return AVERROR(ENAMETOOLONG);
> +            }

I don't believe this check is needed.

> +    if (!(unodes = av_malloc(sizeof(*unodes) * len))) {

you can check here with:
if ((uint64_t)(sizeof(*unodes)) * len > UINT_MAX) ...

or something similar.

> +        av_free(udata);
> +        h->priv_data = NULL;
> +        return AVERROR(ENOMEM);
> +    } else
> +        udata->urls = unodes;
> +
> +    /* handle input */
> +    if (!*uri) err = AVERROR(ENOENT);

nit, for K&R:
if (...)
   err = ...
slightly more readable IMO.

> +    for (i = 0; *uri; i++) {
> +        /* parsing uri */
> +        len = strcspn(uri, AV_CAT_SEPARATOR);
> +        if (!(tmpath = av_realloc(upath, len))) {

+1 for the terminating 0.

> +            err = AVERROR(ENOMEM);
> +            break;
> +        } else 
> +            upath = tmpath;
> +        av_strlcpy(upath, uri, len+1);
> +        uri += len + strspn(uri+len, AV_CAT_SEPARATOR);
> +        /* creating URLContext */

> +        if ((err = url_open(&uc, upath, flags)) < 0) {
> +            break;
> +        }

superfluos {}, also what happens if N have been opened but N+1 fails?
maybe a call to url_close() may help.

> +        /* creating size */
> +        size = url_filesize(uc);
> +        if (size < 0) {
> +            err = AVERROR(ENOSYS);
> +            break;
> +        }
> +        /* assembling */
> +        unodes[i].uc   = uc;
> +        unodes[i].size = size;
> +    }
> +    av_free(upath);
> +    if (err < 0) {
> +        av_free(unodes);
> +        av_free(udata);
> +        h->priv_data = NULL;
> +    } else {
> +        udata->length = i;

> +        //av_realloc(unodes, udata->length+1);

residual code

Regards.
-- 
FFmpeg = Forgiving & Fantastic Maxi Pacific Educated Gorilla