[FFmpeg-devel] [PATCH] Add av_file_get_size() and av_file_read(), replace cmdutils.h:read_file().
Stefano Sabatini
stefano.sabatini-lala
Mon Dec 13 00:48:25 CET 2010
On date Saturday 2010-12-11 17:37:45 +0100, Michael Niedermayer encoded:
> On Sat, Dec 11, 2010 at 03:50:00PM +0100, Stefano Sabatini wrote:
> > On date Saturday 2010-12-11 14:33:54 +0100, Michael Niedermayer encoded:
> > > On Mon, Dec 06, 2010 at 12:30:09PM +0100, Stefano Sabatini wrote:
> > > > 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.
> > >
> > > read_file* wastes memory and with large files thats alot of memory, i dont know
> > > how relevant this is here but for the rather stylistic difference of having 2
> > > variables in a existing struct or local it seems better to go with the less
> > > wastefull API to me
> > >
> > >
> > > > --
> > > > FFmpeg = Foolish & Frenzy Mournful Plastic Excellent Gnome
> > >
> > > > configure | 2 +
> > > > libavutil/Makefile | 2 +
> > > > libavutil/file.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > libavutil/file.h | 57 ++++++++++++++++++++++++++++
> > > > 4 files changed, 167 insertions(+)
> > > > 87e8917be52cfdf043e55a42fcd61a1d82c36b10 0002-Add-av_file_map-and-av_file_unmap-functions.patch
> > > > 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
> > > >
> > > > diff --git a/configure b/configure
> > > > index 8f7cf12..02a89c7 100755
> > > > --- a/configure
> > > > +++ b/configure
> > > > @@ -1041,6 +1041,7 @@ HAVE_LIST="
> > > > malloc_h
> > > > memalign
> > > > mkstemp
> > > > + mmap
> > > > pld
> > > > posix_memalign
> > > > round
> > > > @@ -2670,6 +2671,7 @@ check_func inet_aton $network_extralibs
> > > > check_func isatty
> > > > check_func ${malloc_prefix}memalign && enable memalign
> > > > check_func mkstemp
> > > > +check_func mmap
> > > > check_func ${malloc_prefix}posix_memalign && enable posix_memalign
> > > > check_func setrlimit
> > > > check_func strerror_r
> > > > diff --git a/libavutil/Makefile b/libavutil/Makefile
> > > > index e9ac965..fe0302c 100644
> > > > --- a/libavutil/Makefile
> > > > +++ b/libavutil/Makefile
> > > > @@ -15,6 +15,7 @@ HEADERS = adler32.h \
> > > > error.h \
> > > > eval.h \
> > > > fifo.h \
> > > > + file.h \
> > > > intfloat_readwrite.h \
> > > > intreadwrite.h \
> > > > lfg.h \
> > > > @@ -42,6 +43,7 @@ OBJS = adler32.o \
> > > > error.o \
> > > > eval.o \
> > > > fifo.o \
> > > > + file.o \
> > > > intfloat_readwrite.o \
> > > > inverse.o \
> > > > lfg.o \
> > > > diff --git a/libavutil/file.c b/libavutil/file.c
> > > > new file mode 100644
> > > > index 0000000..6ca7a6b
> > > > --- /dev/null
> > > > +++ b/libavutil/file.c
> > > > @@ -0,0 +1,106 @@
> > > > +/*
> > > > + * This file is part of FFmpeg.
> > > > + *
> > > > + * FFmpeg is free software; you can redistribute it and/or
> > > > + * modify it under the terms of the GNU Lesser General Public
> > > > + * License as published by the Free Software Foundation; either
> > > > + * version 2.1 of the License, or (at your option) any later version.
> > > > + *
> > > > + * FFmpeg is distributed in the hope that it will be useful,
> > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > > > + * Lesser General Public License for more details.
> > > > + *
> > > > + * You should have received a copy of the GNU Lesser General Public
> > > > + * License along with FFmpeg; if not, write to the Free Software
> > > > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> > > > + */
> > > > +
> > > > +#include "file.h"
> > > > +#include <fcntl.h>
> > > > +#include <sys/stat.h>
> > > > +#include <unistd.h>
> > > > +#if HAVE_MMAP
> > > > +#include <sys/mman.h>
> > > > +#endif
> > > > +
> > > > +int av_file_map(const char *filename, uint8_t **bufptr, size_t *size, int protect,
> > > > + void *log_ctx)
> > > > +{
> > > > + int protect2, fd = open(filename, O_RDONLY);
> > > > + struct stat st;
> > > > + void *ptr;
> > > > + off_t off_size;
> > > > +
> > > > + *bufptr = NULL;
> > > > + if (fd < 0) {
> > > > + av_log(log_ctx, AV_LOG_ERROR,
> > > > + "Cannot read file '%s': %s\n", filename, strerror(errno));
> > > > + return AVERROR(errno);
> > > > + }
> > > > +
> > > > + if (lstat(filename, &st) < 0) {
> > > > + close(fd);
> > > > + return AVERROR(errno);
> > > > + }
> > > > +
> > > > + off_size = st.st_size;
> > > > + if (off_size > SIZE_MAX) {
> > > > + av_log(log_ctx, AV_LOG_ERROR,
> > > > + "File size for file '%s' is too big\n", filename);
> > > > + close(fd);
> > > > + return AVERROR(EINVAL);
> > > > + }
> > > > + *size = off_size;
> > > > +
> > > > +#if HAVE_MMAP
> > > > + protect2 = protect & AV_FILE_PROT_READ ? PROT_READ : 0;
> > > > + protect2 |= protect & AV_FILE_PROT_WRITE ? PROT_WRITE: 0;
> > > > + protect2 |= protect & AV_FILE_PROT_EXEC ? PROT_EXEC : 0;
> > > > + ptr = mmap(NULL, *size, protect2, MAP_PRIVATE, fd, 0);
> > > > + if ((int)(ptr) == -1) {
> > > > + av_log(log_ctx, AV_LOG_ERROR, "Error occurred in mmap(): %s\n", strerror(errno));
> > > > + close(fd);
> > > > + return (int)ptr;
> > > > + }
> > > > + *bufptr = ptr;
> > > > +#else
> > >
> > > > + *bufptr = av_malloc(*size);
> > >
> > > there are several type convertions that are exploitable on the wrong platform
> > > and that kind of issue has just recently been discussed :/
> > >
> > >
> > > > + if (!*bufptr) {
> > > > + close(fd);
> > > > + return AVERROR(ENOMEM);
> > > > + }
> > > > + read(fd, *bufptr, *size);
> > > > +#endif
> > > > +
> > > > + return fd;
> > > > +}
> > > > +
> > > > +void av_file_unmap(int fd, uint8_t *bufptr, size_t size)
> > > > +{
> > > > +#if HAVE_MMAP
> > > > + munmap(bufptr, size);
> > > > +#else
> > > > + av_free(bufptr);
> > > > +#endif
> > > > + close(fd);
> > > > +}
> > > > +
> > > > +#ifdef TEST
> > > > +
> > > > +#undef printf
> > > > +
> > > > +int main(void)
> > > > +{
> > > > + uint8_t *buf;
> > > > + size_t size;
> > > > + int fd = av_file_map("file.c", &buf, &size, AV_FILE_PROT_READ|AV_FILE_PROT_WRITE, NULL);
> > > > + if (fd < 0)
> > > > + return 1;
> > > > +
> > > > + buf[0] = 's';
> > > > + printf("%s", buf);
> > > > + av_file_unmap(fd, buf, size);
> > > > + return 0;
> > > > +}
> > > > +#endif
> > > > diff --git a/libavutil/file.h b/libavutil/file.h
> > > > new file mode 100644
> > > > index 0000000..b599d94
> > > > --- /dev/null
> > > > +++ b/libavutil/file.h
> > > > @@ -0,0 +1,57 @@
> > > > +/*
> > > > + * This file is part of FFmpeg.
> > > > + *
> > > > + * FFmpeg is free software; you can redistribute it and/or
> > > > + * modify it under the terms of the GNU Lesser General Public
> > > > + * License as published by the Free Software Foundation; either
> > > > + * version 2.1 of the License, or (at your option) any later version.
> > > > + *
> > > > + * FFmpeg is distributed in the hope that it will be useful,
> > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > > > + * Lesser General Public License for more details.
> > > > + *
> > > > + * You should have received a copy of the GNU Lesser General Public
> > > > + * License along with FFmpeg; if not, write to the Free Software
> > > > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> > > > + */
> > > > +
> > > > +#ifndef AVUTIL_FILE_H
> > > > +#define AVUTIL_FILE_H
> > > > +
> > > > +#include "avutil.h"
> > > > +
> > > > +/**
> > > > + * @file misc file utilities
> > > > + */
> > > > +
> > >
> > > > +#define AV_FILE_PROT_READ 1
> > > > +#define AV_FILE_PROT_WRITE 2
> > > > +#define AV_FILE_PROT_EXEC 4
> > >
> > > they should be defined to PROT_READ, ... when available
> >
> > and have different values if they are not available?
>
> hmm
> you could use O_RDONLY, O_WRONLY, O_RDWR
> but as is you must remap them twice as you introduce a 3rd set of
> values
this is not equivalent to a combination of three flags
>
>
> >
> > > > +
> > > > +/**
> > > > + * Read the file with name filename, and put its content in a newly
> > > > + * allocated buffer or map it with mmap() when available.
> > >
> > > + * If filename
> > > > + * references a link, the content of the linked file is read.
> > >
> > > remove this
> > >
> > >
> > > > + *
> > > > + * @param bufptr pointer where the pointer to the file buffer is
> > > > + * returned, *bufptr can be an allocated buffer or accessed through
> > > > + * mmaplocation, is set to NULL in case of failure
> > >
> > > your english is making my head hurt
> > >
> > > @param[out] bufptr The read or mmaped data.
> > >
> > >
> > >
> > > > + * @param size pointer where the size of the file buffer is returned
> > >
> > > @param[out] size file size
> > >
> > >
> > > > + * @param protect flags that control what kind of access is permitted,
> > > > + * it can be any combination of the AV_FILE_PROT* flags, it is ignored
> > > > + * if mmap() is not available
> > > > + * @param log_ctx context used for logging
> > >
> > > in/out info missing
> >
> > in/out is not used in most doxies, and its semantics is not clear
> > (e.g. buftpr and size should be [in][out])
>
> that should be out not in/out, size is not input, at least not in the current
> design. That we supply a pointer that is used to return size is a detail that
> doesnt affect that its a pure output, every output in C needs us to supply such
> a pointer, if one considered that as input then nothing in C could be a output
> this is absurd thus it cannot be defined like that.
>
>
> > so I prefer to just avoid
> > it, I put this bit of info in the function description as done in
> > other places, should be simpler.
> >
> > > > + * @return the file descriptor in case of success, a negative value
> > > > + * corresponding to an AVERROR error code in case of failure
> > > > + */
> > > > +int av_file_map(const char *filename, uint8_t **bufptr, size_t *size, int protect,
> > > > + void *log_ctx);
> > > > +
> > > > +/**
> > > > + * Unmap the file with filedescriptor fd, and free the allocated or
> > > > + * mmapped buffer in *bufptr with size size.
> > > > + */
> > > > +void av_file_unmap(int fd, uint8_t *bufptr, size_t size);
> >
> > I'm not sure it's a good idea to keep fd as we could simply close the
> > file at the end of av_file_map().
>
> if you support writeable buffers, of course you have to write things back
> to the file at the end and that needs the fd
> otherwise what is AV_FILE_PROT_WRITE doing?
Uhm, well actually I was using MAP_PRIVATE rather than MAP_SHARED.
Anyway if we want to be able to write to the original file then we
need to keep around the permission bits for the case when mmap() is
not available and we need to write back the modified buffer to the
file, so in this case maybe is better to keep a context, e.g.:
AVFileContext { prot; filename; size; data; log_ctx };
int av_file_map(AVFileContext **ctx, const char *filename, protect, log_ctx);
void av_file_unmap(AVFileContext *ctx);
--
FFmpeg = Fiendish and Fundamental Maxi Ponderous Esoteric Genius
More information about the ffmpeg-devel
mailing list