[FFmpeg-devel] [PATCH] urlconcat protocol

[Michele Orrù]

Fixed:

[...]
I don't know if "uinfos" and "urlconcat_node" are more readable; if not,
just suggest some other names.

[...]
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?

> > +    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.



> > +    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.



Michele Orr?.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: uconcatp.patch
Type: application/octet-stream
Size: 6645 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100127/6efabe6a/attachment.obj>