[FFmpeg-devel] [PATCH] urlconcat protocol

Stefano Sabatini stefano.sabatini-lala
Sat Jan 30 01:03:25 CET 2010


On date Friday 2010-01-29 14:36:31 +0100, Michele Orr? encoded:
> fixed.

> Index: libavformat/concat.c
> ===================================================================
> --- libavformat/concat.c	(revisione 0)
> +++ libavformat/concat.c	(revisione 0)
> @@ -0,0 +1,196 @@
> +/*
> + * Concat URL protocol
> + * Copyright (c) 2006 Steve Lhomme
> + * Copyright (c) 2007 Wolfram Gloger
> + * Copyright (c) 2010 Michele Orr??
> + *
> + * 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_nodes {
> +    URLContext *uc;
> +    int64_t     size;
> +};
> +
> +struct urlconcat_data {
> +    struct urlconcat_nodes *urls;     ///< array  of url entries

I'd rather prefer:
        struct urlconcat_nodes *nodes;   ///< list of nodes to concat

rationale is that should be easy to relate the doxy to the name of
the variable, if not that usually means that either the name or the
doxy is confusing / bad choosen.

> +    size_t                  length;   ///< number of cat'ed urls

ditto, s/urls/nodes/

> +    size_t                  current;  ///< index  of currently read node
> +};
> +
> +static int urlconcat_open(URLContext *h, const char *filename, int flags)

filename is too generic, please use url.

> +{
> +    char *fn = NULL;
> +    int err = 0;
> +    int64_t size;
> +    size_t  len, i;
> +    URLContext *uc;
> +    struct urlconcat_data *udata;
> +    struct urlconcat_nodes *unodes;
> +
> +    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++;

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

Note that this construct can be written idiomatically as:

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

this should help to save some lines.

> +        av_free(udata);
> +        h->priv_data = NULL;
> +        return AVERROR(ENOMEM);

This can be managed by the freeing code.

> +    } else
> +        udata->urls = unodes;
> +
> +    /* 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);

strcspn()

> +        fn = av_realloc(fn, len);

node_url looks like a better name.

> +        if (!fn) {
> +            err = AVERROR(ENOMEM);
> +            goto fail;
> +        }
> +        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);

This is not correct, if you can't open an url that doesn't necessarily
mean that the resource doesn't exist (permission problem, etc...).
I'd say:

if ((err = url_open(...)) < 0) ...

> +            goto fail;
> +        }
> +        /* creating size */
> +        size = url_filesize(uc);
> +        if (size < 0) {
> +            err = AVERROR(ENOSYS);
> +            goto fail;
> +        }
> +        /* assembling */
> +        unodes[i].uc   = uc;
> +        unodes[i].size = size;
> +    }

> +    fail: 

This is not a good name, since this also manages the case where err =
0, I suggest "end" instead.

> +        av_free(fn);
> +        if (err < 0) {
> +            av_free(unodes);
> +            av_free(udata);
> +            h->priv_data = NULL;
> +            return err;
> +        } else {
> +            udata->length = i;

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

Avoid to keep commented code -> confusing.

> +            return 0;

This can be factorized out, writing it at the end of the function as
return err.

> +        }
> +}
> +
> +static int urlconcat_read(URLContext *h, unsigned char *buf, int size)
> +{
> +    int result, total = 0;
> +    struct urlconcat_data *udata = h->priv_data;
> +    struct urlconcat_nodes *unodes = udata->urls;
> +    size_t i = udata->current;
> +
> +    while (size > 0) {
> +        result = url_read(unodes[i].uc, buf, size);
> +        if (result < 0) 
> +            return result;
> +        else if (result == 0)
> +            if (i + 1 == udata->length ||
> +                url_seek(unodes[++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_nodes *unodes = udata->urls;
> +    size_t i;
> +
> +    switch (whence) {
> +    case SEEK_END:
> +        for (i = udata->length - 1; 
> +             i != 0 && pos < -unodes[i-1].size;
> +             i--)
> +            pos += unodes[i-1].size;
> +        break;
> +    case SEEK_CUR:
> +        /* get the absolute position */
> +        for (i = 0; i != udata->current; i++)
> +            pos += unodes[i].size;
> +        pos += url_seek(unodes[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 >= unodes[i].size; i++)
> +            pos -= unodes[i].size;
> +        break;
> +    default:
> +        return AVERROR(EINVAL);
> +    }
> +    result = url_seek(unodes[i].uc, pos, whence);
> +    if (result >= 0) {
> +        udata->current = i;

> +        while (i != 0)

while (i) is generally preferred in the FFmpeg code.

> +            result += unodes[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_nodes *unodes = udata->urls;
> +
> +    for (i=0; i != udata->length; i++)
> +       if ((err = url_close(unodes[i].uc)) < 0)
> +           break;

OK now I see that I gave you a wrong suggestion, yes every url
protocol should be closed or we'll have a leak, in case of error we
may simply return -1 (return err ? -1 : 0;)
 
> +    av_free(unodes);
> +    av_free(udata);
> +    h->priv_data = NULL;
> +    return err;
> +}
> +
> +URLProtocol urlconcat_protocol = {
> +    "cat",
> +    urlconcat_open,
> +    urlconcat_read,
> +    NULL,
> +    urlconcat_seek,
> +    urlconcat_close,
> +};
> Index: libavformat/Makefile
> ===================================================================
> --- libavformat/Makefile	(revisione 21526)
> +++ libavformat/Makefile	(copia locale)
> @@ -268,6 +268,7 @@
>  OBJS-$(CONFIG_RTP_PROTOCOL)              += rtpproto.o
>  OBJS-$(CONFIG_TCP_PROTOCOL)              += tcp.o
>  OBJS-$(CONFIG_UDP_PROTOCOL)              += udp.o
> +OBJS-$(CONFIG_URLCONCAT_PROTOCOL)        += concat.o
>  
>  # libavdevice dependencies
>  OBJS-$(CONFIG_JACK_INDEV)                += timefilter.o
> Index: libavformat/allformats.c
> ===================================================================
> --- libavformat/allformats.c	(revisione 21526)
> +++ libavformat/allformats.c	(copia locale)
> @@ -220,4 +220,5 @@
>      REGISTER_PROTOCOL (RTP, rtp);
>      REGISTER_PROTOCOL (TCP, tcp);
>      REGISTER_PROTOCOL (UDP, udp);
> +    REGISTER_PROTOCOL (URLCONCAT, urlconcat);
>  }

Regards.
-- 
FFmpeg = Furious and Foolish Minimalistic Purposeless Eager Gospel



More information about the ffmpeg-devel mailing list