[FFmpeg-devel] [PATCH v5 2/2] avformat/os_support: Support long file names on Windows

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Martin
> Storsjö
> Sent: Tuesday, May 24, 2022 2:44 PM
> To: Soft Works <softworkz at hotmail.com>
> Cc: Hendrik Leppkes <h.leppkes at gmail.com>; FFmpeg development discussions
> and patches <ffmpeg-devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v5 2/2] avformat/os_support: Support
> long file names on Windows
> 
> On Tue, 24 May 2022, Soft Works wrote:
> 
> >> -----Original Message-----
> >> From: Martin Storsjö <martin at martin.st>
> >> Sent: Tuesday, May 24, 2022 1:26 PM
> >> To: Soft Works <softworkz at hotmail.com>
> >> Cc: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>;
> >> Hendrik Leppkes <h.leppkes at gmail.com>
> >> Subject: RE: [FFmpeg-devel] [PATCH v5 2/2] avformat/os_support: Support
> >> long file names on Windows
> >>
> >> On Tue, 24 May 2022, Soft Works wrote:
> >>
> >>>> but Clang doesn't. (It's possible to use it
> >>>> in Clang too if you enable it with -fms-extensions though.)
> >>>
> >>> Is it possible to compile ffmpeg for Windows using Clang?
> >>> And if yes, does it even work without that flag?
> >>> (assuming it was introduced in order to be able to
> >>> compile Windows stuff).
> >>
> >> Yes, it is possible to build it with Clang without any custom extra
> flags
> >> to enable nondefault modes. In fact, it's tested continuously on FATE
> too:
> >>
> >> http://fate.ffmpeg.org/history.cgi?slot=x86_64-mingw32-clang-trunk
> >>
> >> Also for other architectures, e.g.:
> >>
> >> http://fate.ffmpeg.org/history.cgi?slot=aarch64-mingw32-clang-trunk
> >
> >
> > OK, thanks for the pointers. I'm not sure whether it would be
> > acceptable to require this compilation flag for Windows builds?
> 
> I would very much prefer not to require adding -fms-extensions when
> building with Clang - that option unlocks a lot of stuff that we generally
> shouldn't be enabling.

OK, sure, it always smells when doing something like that just to
achieve a single thing.

> 
> > Can you think of any other ideas?
> 
> Right now, mainly doing a #define ff_stat_struct which would require
> updating the calling code. It's not ideal but worse things have been done
> anyway (there's not that many stat calls).
> 
> I was exploring the idea of just redefining the struct, but e.g. "typedef
> struct _stati64 win32_stat", but that only works when referring to the
> type as "win32_stat", not "struct win32_stat". So that doesn't seem like a
> good path forward either.
> 
> I'd prefer to slow down and think more about other alternatives here,
> rather than rushing forward with adding -fms-extensions.

I have a new idea, see below

> Also note that currently, we don't even have a proper automatic redirect
> from stat to win32_stat, see the ifdef in libavformat/file.c. 

Yes, that can be dropped (once we got it)...


What do you think of the following:

We could define our own win32_stat struct, but not in a way that matches
the Windows API, just matching the POSIX definition (like the consuming 
code expects), e.g.:

struct win_32stat {
    dev_t          st_dev;     /* ID of device containing file */
    ino_t          st_ino;     /* inode number */
    unsigned short st_mode;    /* protection */
    short          st_nlink;   /* number of hard links */
    short          st_uid;     /* user ID of owner */
    short          st_gid;     /* group ID of owner */
    dev_t          st_rdev;    /* device ID (if special file) */
    off_t          st_size;    /* total size, in bytes */
    time_t         st_atime;   /* time of last access */
    time_t         st_mtime;   /* time of last modification */
    time_t         st_ctime;   /* time of last status change */
};

And then, in our win32_stat() function, we call the win api with
the "right" struct and simply copy over the values..:

static int win32_stat(const char *filename_utf8, struct stat *par)
{
    wchar_t *filename_w;
    int ret;
    struct _stati64 winstat;

    if (get_extended_win32_path(filename_utf8, &filename_w))
        return -1;

    if (filename_w) {
        ret = _wstat64(filename_w, &winstat);
        av_free(filename_w);
    } else
        ret = _stat64(filename_utf8, &winstat);

    par->st_dev   = winstat.st_dev;
    par->st_ino   = winstat.st_ino;
    par->st_mode  = winstat.st_mode;
    par->st_nlink = winstat.st_nlink;
    par->st_uid   = winstat.st_uid;
    par->st_gid   = winstat.st_gid;
    par->st_rdev  = winstat.st_rdev;
    par->st_size  = winstat.st_size;
    par->st_atime = winstat.st_atime;
    par->st_mtime = winstat.st_mtime;
    par->st_ctime = winstat.st_ctime;

    return ret;
}

This would be safe and without any weirdness (just a bit more
code).

What do you think about it?


Thanks,
sw