[FFmpeg-devel] [PATCH] urlconcat protocol

[Stefano Sabatini]

On date Wednesday 2010-01-27 20:14:38 +0100, Michele Orr? encoded:
> Fixed:
> 
> [...]
> I don't know if "uinfos" and "urlconcat_node" are more readable; if not,
> just suggest some other names.

urlconcat_node or urlconcat_entry is fine.

> [...]
> Doxygen doc should be ok.
> 
> 
> > +        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;
> >
> Nope:
>    -  first of all goto is buggy, so why should we prefer it to a break?

goto is not buggy, its use may be buggy or stilistically deprecable,
but there are perfectly licit cases where goto may be used to achieve
more readable / maintainable code, grep the FFmpeg sources for
examples.

> > > +    av_free(fn);
> > > +    if (err < 0) {
> > > +        av_free(uinfo);
> > > +        av_free(udata);
> > > +        h->priv_data = NULL;
> > > +        return err;
> > > +    }
> >
>    - also, here we have to free(fn) in both cases (err <||= 0), so it's just
> better to write it only once.

What about:

end:
        av_free(fn);
        if (ret < 0) {
           ...
        }
        return ret;
           
> > > +    av_free(uinfo);
> > > +    av_free(udata);
> > > +    h->priv_data = NULL;
> > > +    return err < 0 ? AVERROR(EIO) : 0;
> >
> > Why not simply err?
> >
> Suppose we have multiple errors, it's not so good to return only the last
> errorcode.

err is not supposed to be a set of flags, but just a code associated
to an error, possibly the first one found as there is no sense into
going on and collect others after the first one is encountered.

[...] 
> 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 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_node {
> +    URLContext *uc;
> +    int64_t     size;
> +};
> +
> +struct urlconcat_data {
> +    struct urlconcat_node *urls;     /**< array  of url entries          */
> +    size_t                 length;   /**< number of cat'ed urls          */

Please use unodes/nodes (I have a preference for the latter), same in
the doxy is better to use terms already used in the variable
names/types, decrease the sense of wonder of the reader.

nit: "///<" is preferred for inline doxy (more consistent with other
doxies in the codebase.

> +    size_t                 current;  /**< index  of current reading url  */

"currently read node" sounds 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_node *uinfos;

Please use "unodes" or "nodes", more meaningful and consistent with
the name of the struct variable, also info[rmation] is indeclinable.

[...]

Regards.
-- 
FFmpeg = Forgiving Fundamentalist Mastering Ponderous Exxagerate Gorilla