[FFmpeg-devel] [PATCH] File concat protocol

[Michael Niedermayer]

Hi

On Sun, Jun 24, 2007 at 01:52:40PM -0000, Wolfram Gloger wrote:
> Hi,
> 
> > this should be in a seperate file not in file.c
> 
> Ok.
> 
> > also it should not be specific to the file protocol but rather work
> > with all, its kinda silly if we next add a httpconcat then a filehttpconcat
> > ..
> 
> Ok, I can't really see much point in a non-file concat, but since it
> was rather easy to implement, I did it.
> 
> > no doxygen comment
> 
> Added.
> 
> > > +#if defined(__MINGW32__) || defined(CONFIG_OS2) || defined(__CYGWIN__)
> > > +    access |=3D O_BINARY;
> > > +#endif
> > 
> > this is duplicated relative to existing code from file.c
> 
> As a nice side effect, this goes away when using url rather than file..
> 
> > i think such ifdef hell should not be accepted in new patches
> > configure or some other appropriate thing should check availability of
> > _fstat64/stat and provide a working stat() or set HAVE__STAT64 or whatever
> 
> Same for this.
> 
[...]
> +    uinfo = (struct urlconcat_info *) h->priv_data;

unneeded cast



> +
> +    av_strstart(filename, "cat:", &filename);
> +
> +    for (;;) {
> +        for (len=0; filename[len] && filename[len]!=AV_CAT_SEPARATOR; ++len);
> +        if (len == 0)
> +            break;
> +        fn = av_malloc(len+1);
> +        strncpy(fn, filename, len);

shouldnt av_strlcpy() be used here?


[...]
> +        url_desc = av_realloc(uinfo->url_desc, sizeof(*url_desc)*(uinfo->urlnum+1));
> +        if (!url_desc)
> +            return AVERROR_NOMEM;

please use AVERROR() AVERROR_* is deprecated


> +        uinfo->url_desc = url_desc;
> +        uinfo->url_desc[uinfo->urlnum] = uc;
> +        fsizes = av_realloc(uinfo->url_sizes, sizeof(*fsizes)*(uinfo->urlnum+1));
> +        if (!fsizes)
> +            return AVERROR_NOMEM;
> +        uinfo->url_sizes = fsizes;
> +        uinfo->url_sizes[uinfo->urlnum] = size;
> +
> +        ++uinfo->urlnum;

i think a single array of a struct of url_desc + size would be simpler
also the uc and size (temporary) variables are unneeded



[...]
> +static int urlconcat_read(URLContext *h, unsigned char *buf, int size)
> +{
> +    struct urlconcat_info *uinfo = (struct urlconcat_info *) h->priv_data;

unneeded cast


[...]
> +static offset_t urlconcat_seek(URLContext *h, offset_t pos, int whence)
> +{
> +    int i;
> +    offset_t result;
> +    struct urlconcat_info *uinfo = (struct urlconcat_info *) h->priv_data;
> +
> +    switch (whence) {
> +    case SEEK_END:
> +        for (i=uinfo->urlnum-1; i>0 && (pos < -uinfo->url_sizes[i-1]); --i)
> +            pos += uinfo->url_sizes[i-1];
> +        break;
> +    case SEEK_CUR:
> +        /* get the absolute position */
> +        for (i=0; i<uinfo->current; i++)
> +            pos += uinfo->url_sizes[i];
> +        pos += url_seek(uinfo->url_desc[uinfo->current], 0, SEEK_CUR);
> +        whence = SEEK_SET;
> +        /* fall through with the absolute position */
> +    case SEEK_SET:
> +        for (i=0; i+1<uinfo->urlnum && (pos >= uinfo->url_sizes[i]); i++)
> +            pos -= uinfo->url_sizes[i];
> +        break;
> +    default:
> +        return AVERROR_INVALIDDATA;

AVERROR()

and i think the seeking code should be simpler if instead of storing the
sizes of all urls the sum of sizes of the previous urls would be stored
(so that url_sizes[i] is the size of url 0 to i)
though if its not simpler then just disregard this comment


[...]
> +        av_free(uinfo);
> +        h->priv_data = NULL;

av_freep(&h->priv_data);


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070624/226b15db/attachment.pgp>