[FFmpeg-devel] [PATCH] Windows support for av_file_map()

[Michael Niedermayer]

On Wed, Jan 05, 2011 at 12:16:44AM -0700, Daniel Verkamp wrote:
> On Tue, Jan 4, 2011 at 10:47 PM, Daniel Verkamp <daniel at drv.nu> wrote:
> > On Sat, Jan 1, 2011 at 11:39 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> >> On Fri, Dec 31, 2010 at 02:35:50AM -0700, Daniel Verkamp wrote:
> >>> Hi,
> >>>
> >>> Here's a simple implementation of av_file_map() using
> >>> CreateFileMapping() and MapViewOfFile(), which are the rough Win32
> >>> equivalent of mmap().
> >>>
> >>> This currently maps a read-only view of the file; this could be
> >>> changed to R/W to match the mmap() path if desired, but any code
> >>> expecting to actually use it for writing will fail with the non-mmap()
> >>> path anyway since the file is never written back in that case.
> >>>
> >>> Thanks,
> >>> -- Daniel Verkamp
> >>
> >> lgtm if tested
> >>
> >
> > Applied with a small modification recommended by Ramiro on IRC.
> >
> > An (ugly hack of an) idea occurred to me: for the malloc() (non-mmap)
> > case, the buffer could be allocated with some extra space at the
> > beginning to store a fd or other information to be used to flush the
> > file at close time; this follows the same API with working write
> > access. ?A first try at implementing this is attached.
> >
> > On the other hand, this API is not very useful on 32-bit machines,
> > since it only maps full files, and large files will obviously not fit
> > into a 32-bit address space no matter how hard they are squeezed. ?It
> > might not be a bad idea to have some kind of windowing/view support
> > like mmap() et. al. if this is really intended to be used for anything
> > other than the trivial small file case (and if that's all it's for, it
> > seems like overkill).
> >
> 
> Slightly changed version that doesn't depend on undefined integer
> overflow semantics attached.

> Index: file.c
> ===================================================================
> --- file.c	(revision 26221)
> +++ file.c	(working copy)
> @@ -33,16 +33,21 @@
>      void *log_ctx;
>  } FileLogContext;
>  
> +/// Stored immediately preceding file data in memory in non-memory-mapped implementation
> +typedef struct {
> +    int fd;
> +} FileMapContext;
> +

fd could be put in FileLogContext and FileLogContext be put in the array


>  static const AVClass file_log_ctx_class = {
>      "FILE", av_default_item_name, NULL, LIBAVUTIL_VERSION_INT,
> -    offsetof(FileLogContext, log_offset), offsetof(FileLogContext, log_ctx)
> +/////    offsetof(FileLogContext, log_offset), offsetof(FileLogContext, log_ctx)
>  };

hmm?



>  
>  int av_file_map(const char *filename, uint8_t **bufptr, size_t *size,
>                  int log_offset, void *log_ctx)
>  {
>      FileLogContext file_log_ctx = { &file_log_ctx_class, log_offset, log_ctx };
> -    int err, fd = open(filename, O_RDONLY);
> +    int err, fd = open(filename, O_RDWR);
>      struct stat st;
>      av_unused void *ptr;
>      off_t off_size;
> @@ -84,6 +89,7 @@
>          return err;
>      }
>      *bufptr = ptr;
> +    close(fd);
>  #elif HAVE_MAPVIEWOFFILE
>      {
>          HANDLE mh, fh = (HANDLE)_get_osfhandle(fd);
> @@ -104,18 +110,31 @@
>          }
>  
>          *bufptr = ptr;
> +        close(fd);
>      }
>  #else
> -    *bufptr = av_malloc(*size);
> -    if (!*bufptr) {
> +    if (*size > max_size - sizeof(FileMapContext))
> +    {
> +        av_log(&file_log_ctx, AV_LOG_ERROR,
> +               "File size for file '%s' is too big\n", filename);
> +        close(fd);
> +        return AVERROR(EINVAL);
> +    }

something like

p=NULL;
if(small enough)
    p=malloc()
if(!p)
    ...

seems simpler


> +

> +    {
> +    FileMapContext* buf = av_malloc(*size + sizeof(FileMapContext));

this block in the middle is a bit ugly


> +    if (!buf) {
>          av_log(&file_log_ctx, AV_LOG_ERROR, "Memory allocation error occurred\n");
>          close(fd);
>          return AVERROR(ENOMEM);
>      }
> +
> +    buf->fd = fd;
> +    *bufptr = (uint8_t*)(buf + 1);
>      read(fd, *bufptr, *size);
> +    }
>  #endif
>  
> -    close(fd);
>      return 0;
>  }
>  
> @@ -126,7 +145,13 @@
>  #elif HAVE_MAPVIEWOFFILE
>      UnmapViewOfFile(bufptr);
>  #else
> -    av_free(bufptr);
> +    {
> +        FileMapContext* buf = (FileMapContext*)bufptr - 1;
> +        if (!lseek(buf->fd, 0, SEEK_SET))
> +            write(buf->fd, bufptr, size);

some error should be printed if lseek/write fails

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

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110105/ef1e4a42/attachment.pgp>