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

[Maksym Veremeyenko]

Benoit Fouet ???????(??):
> Hi,
> 
> On 2009-09-08 18:55, Maksym Veremeyenko wrote:
[...]
> 
> I'll try to comment first :)
Thanks for your attention to this patch :-)

I would like to notice that Macintosh alias format is seems too closed 
and not published (at least i found some reference to Red Book, but no 
official description of that block i ever found), so there is only one 
source: 
http://www.geocities.com/xhelmboyx/quicktime/formats/alias-layout.txt 
that i will refer to.

[...]
> 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 ?
> 
no, field has a fixed size:
/.../
-> 27 bytes volume name string (used in relative searches)
   - NOTE: if volume name string < 27 chars then pad with zeros
/.../

>> +            /* 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
/.../
-> 63 bytes file name string (used in relative searches)
   - NOTE: if file name string < 63 chars then pad with zeros
/.../


[...]
>> +                } 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.
to make a patch more cosmetic reading *directory name* was done in the 
same way (and a code style too) as *absolute path* (some lines of code 
above), so that is only "traditions honor" (may be *absolute path* 
reading data code submitter knows something more?). Previous patches 
used "malloc + dref->dir[len] = 0"...

So i prepared newer patch that allocate and terminated string...

[...]
>>
>> +static int mov_open_dref(ByteIOContext** pb, char* src, MOVDref* ref)
>> +{
> 
> please write ByteIOContext **pb, char *src, MOVDref *ref
fixed

> 
>> +    char filename[1024];
>> +    char *src_path;
>> +    int c, i, l;
>> +
> 
> those could be moved inside the if
moved

> 
>> +    if(ref->nlvl_to > 0 && ref->nlvl_from > 0)
>> +    {
> 
> { on the same line as the if statement
> 
sure, fixed

>> +        /* 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 ?
It almost duplicated, but it implement two "different" method in a 
straightforward way. It's make it clear and easy to understand the way 
of relative path building. *nlvl_from* and *nlvl_to* parameters meaning 
was described by me in a: 
http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2009-September/075246.html 
and based on some experiments. So if somebody else will find original 
Apple's documentation that can confirm or deny it will be easy to 
fix/drop/optimize method(s).... so i vote to keep it unchanged....

Newer/updated patches attached.

-- 
________________________________________
Maksym Veremeyenko
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mov_refs_alis_read_extend_v2.patch
Type: text/x-patch
Size: 4127 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090910/aef15ccc/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mov_refs_open_dref_v2.patch
Type: text/x-patch
Size: 3025 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090910/aef15ccc/attachment-0001.bin>