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

[Stefano Sabatini]

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

I spent unnamerable hours figuring out the meaning of lame error
messages which provided no hint about where the failure occurred, so
as a software user I tend to give *much* importance to these usability
issues.

Note also that we perfectly agree that it should be possible to
inhibit such logging, that's why I'm using log_ctx+log_offset (as you
yourself did in eval.h).

> calling av_log() in there could even change errno if for example it tries to
> write to a file and that fails.

Fixed.

> But thats not the point the point is
> public API is intended to be used and this av_log use makes it really annoying
> for most cases where one might want to use it.

It's even more annoying when the programmer can't give enough
information for pointing to the problem, and I don't believe that the
additional log_offset+log_ctx parameters will scare away the users.
-- 
FFmpeg = Faithful Frightening Merciful Peaceful Enlightened Goblin