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

[Michael Niedermayer]

On Mon, Dec 13, 2010 at 12:48:25AM +0100, Stefano Sabatini wrote:
> 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 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.

uhm, then why do you need access rights at all ?!
r+w and no exec always should be fine



> 
> 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);

if you use av_log() you must make AVFileContext a AVClass and setup
log_level_offset_offset and parent_log_context_offset
otherwise messages from this cannot be supressed or not associated to the
parent

but even without that the use of av_log() alone makes it already unsuitable
for public API IMO. Because an application cant be expected to have a av_log
callback and all setup for reading a file.

This should be a wraper around
fopen/fread/mmap and as such should be able to replace these in actual use
cases in actual applications and should be simplifying code not require more
code.

Things either should be designed to be useable outside libav* or should not
be part of the public API

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

DNS cache poisoning attacks, popular search engine, Google internet authority
dont be evil, please
-------------- 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/20101213/3a2db817/attachment.pgp>