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

[Michael Niedermayer]

On Mon, Dec 20, 2010 at 10:50:28AM +0100, Stefano Sabatini wrote:
> On date Sunday 2010-12-19 03:26:00 +0100, Michael Niedermayer encoded:
> > On Sat, Dec 18, 2010 at 11:58:38AM +0100, Stefano Sabatini wrote:
> > > On date Saturday 2010-12-18 02:40:59 +0100, Michael Niedermayer encoded:
> > > > On Fri, Dec 17, 2010 at 12:52:19PM +0100, Stefano Sabatini wrote:
> > > > > On date Wednesday 2010-12-15 04:44:56 +0100, Michael Niedermayer encoded:
> > > > > > On Tue, Dec 14, 2010 at 11:52:31PM +0100, Stefano Sabatini wrote:
> > > > > > > On date Tuesday 2010-12-14 23:18:52 +0100, Michael Niedermayer encoded:
> > > > > > > > On Tue, Dec 14, 2010 at 01:48:52AM +0100, Stefano Sabatini wrote:
> > > > > > > [...]
> > > > > > > > > New try:
> > > > > > > > > int av_file_map(const char *filename, uint8_t **bufptr, size_t *size, int log_offset, void *log_ctx);
> > > > > > > > > void av_file_unmap(uint8_t *bufptr, size_t size);
> > > > > > > > > 
> > > > > > > > > this will simply achieve to create a writable buffer from the content
> > > > > > > > > of the file, buffer which will be completely decoupled from the file
> > > > > > > > > itself (that is: mmapped with MAP_PRIVATE), and which is closed before
> > > > > > > > > returning from av_file_map(), so there is no need to keep the filedes
> > > > > > > > > around anymore.
> > > > > > > > > 
> > > > > > > > > And if you don't like this design please *give more detailed
> > > > > > > > > indications* so we'll avoid to go around in circles and waste precious
> > > > > > > > > time and energy.
> > > > > > > > 
> > > > > > > > this is ok for private (ff_) API
> > > > > > > > for public API use of av_log() should be droped otherwise it is too
> > > > > > > > inconvenient to use in applications that dont use av_log() already.
> > > > > > > 
> > > > > > > what's the problem with:
> > > > > > > ret = av_file_map(filename, &buf, &size, log_offset, NULL);
> > > > > > > ?
> > > > > > 
> > > > > > bloated unneeded APItis
> > > > > > 
> > > > > > mmap and open use errno, a non libav based application that wants to replace
> > > > > > these 2 calls will not use a function that does funny callbacks to a funny
> > > > > > loging system that calls printf() by default
> > > > > > everyone will throw this in the trash bin where the other bloated intermingled
> > > > > > APIs are and they would be quite correct in doing so. 2 calls to open+mmap are
> > > > > > alot cleaner than this
> > > > > 
> > > > > Bah, every application using FFmpeg is supposed to map its own logging
> > > > > system to that of FFmpeg, simply returning an error doesn't give any
> > > > > hint about where the error occurred (opening the file?, file too big?,
> > > > > calling mmap()?).
> > > > > 
> > > > > Also since when av_log() is bloated? Are you suggesting to drop it
> > > > > from FFmpeg?
> > > > 
> > > > We dont disagree i think, we misunderstand each other.
> > > > Yes av_log() use in ffmpeg is all fine. But this is a file+mmap function this
> > > > is not ffmpeg specific nor multimedia specific
> > > > and there are plenty of applications that might want to
> > > > use libavutil and or a fopen+mmap simplification very few of them will switch
> > > > to av_log() for that.
> > > > libavutil and actually not only that should stay clean and free from making
> > > > all its innards interdepend on everything else. Keeping code selfcontained makes
> > > > it much easier to reuse parts in other applications.
> > > > think of libmpcodecs if you like, its interdependancies hit you quite hard.
> > > > If you write public API you should think of someone wanting to use it
> > > > and if you now say they are all ffmpeg users. You wanted to use libmpcodecs and
> > > > not in a mplayer fork or an application using mplayer ...
> > > > That said ive used code from libavutil in many little projects, it is usefull
> > > > code outside multimedia
> > > 
> > > And I agree but note that:
> > > av_file_map(filename, &buf, &size, log_offset, NULL);
> > > 
> > > will disable logging if log_offset is set to AVLOG_MAX-AVLOG_MIN+1,
> > 
> > Why do you use av_log for this at all?
> > Its not the right tool for the job here
> > the error is in errno and perror() and strerror(_r)()/av_log(strerr...())
> > are the correct functions to present that to the user
> 
> The problem is that the external function doesn't know the context of
> where the error occurred, and an error code is not enough, for example
> to distinguish if the error occurred in open(), lstat(), mmap() or if
> the file was too big (in this case an AVERROR(EINVAL) error code
> doesn't give a clue about the exact failure reason).

printing an error message does not affect the insufficiency (or maybe rather
the nitpicking on that) of the error codes that are returned.

anyway feel free to commit the patch if tested

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

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad
-------------- 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/20101221/dd51c542/attachment.pgp>