[FFmpeg-devel] [PATCH] mov reference files search improvement

[Benoit Fouet]

Hi,

On 2009-09-08 18:55, Maksym Veremeyenko wrote:
> Dear Developers!
> 
> As a conclusion to that topic about reference relative search patches
> proposed:
> 
> Step #1: *mov_refs_alis_read_extend.patch* It almost cosmetic and do
> nothing with changing functionality, just extend *MOVDref* and reading
> procedure.
> 
> Step #2: *mov_refs_open_dref.patch* It implements building and probing
> relative names and absolute name of referenced track file.
> 
> please, commit or reject
> 
> 

I'll try to comment first :)

> Index: libavformat/mov.c
> ===================================================================
> --- libavformat/mov.c	(revision 19754)
> +++ libavformat/mov.c	(working copy)
> @@ -274,19 +274,36 @@
>          if (dref->type == MKTAG('a','l','i','s') && size > 150) {
>              /* macintosh alias record */
>              uint16_t volume_len, len;
> -            char volume[28];
>              int16_t type;
>
>              url_fskip(pb, 10);
>
> +            /* read volume name */
>              volume_len = get_byte(pb);
>              volume_len = FFMIN(volume_len, 27);
> -            get_buffer(pb, volume, 27);
> -            volume[volume_len] = 0;
> -            av_log(c->fc, AV_LOG_DEBUG, "volume %s, len %d\n",
volume, volume_len);
> +            get_buffer(pb, dref->volume, 27);
> +            dref->volume[volume_len] = 0;
> +            av_log(c->fc, AV_LOG_DEBUG, "volume %s, len %d\n",
dref->volume, volume_len);
>
> -            url_fskip(pb, 112);
> +            url_fskip(pb, 12);
>

what happens if volume_len is larger than 27, don't you need to skip more ?

> +            /* read file name */
> +            len = get_byte(pb);
> +            len = FFMIN(len, 63);
> +            get_buffer(pb, dref->filename, 63);
> +            dref->filename[len] = 0;
> +            av_log(c->fc, AV_LOG_DEBUG, "filename %s, len %d\n",
dref->filename, len);
> +
> +            url_fskip(pb, 16);
> +

same here for len

[...]

> +                } else if (type == 0) { // directory name
> +                    av_free(dref->dir);
> +                    dref->dir = av_mallocz(len + 1);

why mallocz, and not malloc + dref->dir[len] = 0 ?
you're going to overwrite len bytes anyway.

[...]

> --- libavformat/mov.c.step1	2009-09-04 09:33:45.000000000 +0300
> +++ libavformat/mov.c	2009-09-04 12:08:32.000000000 +0300
> @@ -1540,6 +1540,76 @@
>      }
>  }
>
> +static int mov_open_dref(ByteIOContext** pb, char* src, MOVDref* ref)
> +{

please write ByteIOContext **pb, char *src, MOVDref *ref

> +    char filename[1024];
> +    char *src_path;
> +    int c, i, l;
> +

those could be moved inside the if

> +    if(ref->nlvl_to > 0 && ref->nlvl_from > 0)
> +    {

{ on the same line as the if statement

> +        /* find a source dir */
> +        src_path = FFMAX(strrchr(src, '/'), strrchr(src, '\\'));
> +        if (src_path)
> +            src_path++;
> +        else
> +            src_path = src;
> +
> +        /* find tail by ref->path and nlvl_To */
> +        for (i = 0, l = strlen(ref->path) - 1; l >= 0; l--)
> +            if ('/' == ref->path[l]) {
> +                if(i == ref->nlvl_to - 1) break;
> +                else                      i++;
> +            }
> +
> +        /* check if it found */
> +        if (i == ref->nlvl_to - 1) {
> +            c = l + 1;
> +
> +            l = 1024;
> +            filename[0] = 0;
> +
> +            strncat(filename, src, src_path - src);
> +            l -= src_path - src;
> +
> +            for (i = 1; i < ref->nlvl_from; i++, l -= 3)
> +                strncat(filename, "../", l);
> +
> +            strncat(filename, ref->path + c, l);
> +
> +            /* probe open */
> +            if (!url_fopen(pb, filename, URL_RDONLY))
> +                return 0;
> +        }
> +
> +        if (1 == ref->nlvl_to && ref->dir) {
> +            l = 1024;
> +            filename[0] = 0;
> +
> +            strncat(filename, src, src_path - src);
> +            l -= src_path - src;
> +
> +            for (i = 1; i <= ref->nlvl_from; i++, l -= 3)
> +                strncat(filename, "../", l);
> +
> +            strncat(filename, ref->dir, l);
> +            l -= strlen(ref->dir);
> +
> +            strncat(filename, "/", l);
> +            l -= 1;
> +
> +            strncat(filename, ref->filename, l);
> +
> +            /* probe open */
> +            if (!url_fopen(pb, filename, URL_RDONLY))
> +                return 0;
> +        }

most of the code from the two if() is duplicated, can't it be shared ?

Ben