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

Michael Niedermayer michaelni
Sat Dec 11 17:37:45 CET 2010


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


> 
> > > +
> > > +/**
> > > + * 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?


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

If you really think that XML is the answer, then you definitly missunderstood
the question -- Attila Kinali
-------------- 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/20101211/624193c6/attachment.pgp>



More information about the ffmpeg-devel mailing list