[FFmpeg-devel] [PATCH] Add av_file_get_size() and av_file_read(), replace cmdutils.h:read_file().

[Stefano Sabatini]

On date Monday 2010-12-06 12:30:09 +0100, Stefano Sabatini encoded:
> On date Monday 2010-12-06 02:32:45 +0100, Michael Niedermayer encoded:
> > On Mon, Dec 06, 2010 at 12:02:18AM +0100, Stefano Sabatini wrote:
> > > On date Friday 2010-12-03 18:43:46 +0100, Michael Niedermayer encoded:
> > > > On Fri, Dec 03, 2010 at 05:15:44PM +0100, Stefano Sabatini wrote:
> > > > > On date Thursday 2010-12-02 02:32:30 +0100, Michael Niedermayer encoded:
> > > > > [...]
> > > > > > > +/**
> > > > > > > + * Read the file with name filename, and put its content in a newly
> > > > > > > + * allocated 0-terminated buffer. If filename references a link, the
> > > > > > > + * content of the linked file is read.
> > > > > > > + *
> > > > > > > + * @param bufptr location where pointer to buffer is returned
> > > > > > > + * @param size   location where size of buffer is returned
> > > > > > > + * @param log_ctx context used for logging
> > > > > > > + * @return 0 in case of success, a negative value corresponding to an
> > > > > > > + * AVERROR error code in case of failure
> > > > > > > + */
> > > > > > > +int av_file_read(const char *filename, char **bufptr, size_t *size, void *log_ctx);
> > > > > > 
> > > > > > this API is crap.
> > > > > > at a minimunm it should support memory mapped files on platforms supporting them
> > > > > 
> > > > > We could have a:
> > > > > int av_file_get(const char *filename, size_t *size, void *log_ctx);
> > > > > 
> > > > > (alternative name: av_file_open())
> > > > > 
> > > > > which returns the filedes and the size performing the boring checks,
> > > > > then the application may suck the content in a buffer or using mmap()
> > > > > access. Would be this acceptable?
> > > > 
> > > > its worse in every respect, and still doenst support mmap
> > > > 
> > > > what i was thinking of:
> > > > @param **bufptr The file content, can be a allocated buffer or access through mmap
> > > > fd=av_file_map(const char *filename, uint8_t **bufptr, size_t *size, int access_rights, void *log_ctx);
> > > > 
> > > > av_file_unmap(int fd, uint8_t *bufptr, size_t size);
> > > 
> > > Yet this API is overkill and awkward when you only need to open a
> > > file, allocate a buffer and copy the file content to it, without to
> > > keep track of the fd and size (the buffer may need to be released
> > > later in a different context), as it is the case for ffmpeg.c.
> > 
> > You need to keep track of these things in your API as well. (fd to close(),
> > size, to not read/write over the array end)
> 
> In case of reading of text file that's not an issue because the fd is
> closed by the function itself, and the read/write checks are not
> necessary since the buffer is 0-terminated (and the user is supposed
> to use the buffer like a 0-terminated string).
> 
> > but your API is not suitable for anything but your use case.
> > One could give the functions a struct context, but if that would be simpler
> > iam not so sure.
> 
> On the other hand av_file_map()/unmap() is useful for all the cases
> when you have a context, and here it is my implementation of it, but
> is awkward for the ffmpeg.c case. So we could simply keep read_file()
> in cmdutils.c or implement a corresponding av_file_read() in file.c.
> -- 
> FFmpeg = Foolish & Frenzy Mournful Plastic Excellent Gnome

> From c21bee05dffa8c8f877bcd5343dd79b6231515ed Mon Sep 17 00:00:00 2001
> From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> Date: Fri, 26 Nov 2010 01:27:58 +0100
> Subject: [PATCH] Add av_file_map() and av_file_unmap() functions.
> 
> ---
>  configure          |    2 +
>  libavutil/Makefile |    2 +
>  libavutil/file.c   |  106 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  libavutil/file.h   |   57 ++++++++++++++++++++++++++++
>  4 files changed, 167 insertions(+), 0 deletions(-)
>  create mode 100644 libavutil/file.c
>  create mode 100644 libavutil/file.h

Ping, I'll apply after three days if there are no comments.
-- 
FFmpeg = Fundamental & Furious Merciless Portable Extensive Gadget