[FFmpeg-devel] [PATCH] Fix ffmpeg weird 'Unknown option' error messages

[Michael Niedermayer]

On Mon, Oct 05, 2009 at 09:07:53PM +0200, Stefano Sabatini wrote:
> On date Monday 2009-10-05 00:55:39 +0200, Michael Niedermayer encoded:
> > On Sun, Oct 04, 2009 at 01:34:00PM -0700, Baptiste Coudurier wrote:
> > > On 6/18/09 10:07 PM, Baptiste Coudurier wrote:
> > >> Michael Niedermayer wrote:
> > >>> On Wed, May 27, 2009 at 11:37:42PM +0200, Stefano Sabatini wrote:
> > >>>> On date Wednesday 2009-05-27 04:01:54 +0200, Michael Niedermayer 
> > >>>> encoded:
> > >>>>> On Tue, May 26, 2009 at 10:13:42PM +0200, Stefano Sabatini wrote:
> > >>>>>> On date Tuesday 2009-05-26 03:25:04 +0200, Michael Niedermayer 
> > >>>>>> encoded:
> > >>>>>>> On Tue, May 26, 2009 at 01:32:44AM +0200, Stefano Sabatini wrote:
> > >>>> [...]
> > >>>>>>>> Can you see another solution?
> > >>>>>>> functions should not print errors that might not be errors to the 
> > >>>>>>> appliction
> > >>>>>> Sorry but this looks like a distorted way of thinking, functions
> > >>>>> so you do know of a single function in the standard c lib that does not
> > >>>>> follow this statement?
> > >>>>>
> > >>>>>> should print errors when it makes sense, they should be the
> > >>>>>> applications to adapt themselves to the library and not vice-versa.
> > >>>>>>
> > >>>>>> That said, I'll revert r18826 if there is no better solution for that.
> > >>>> So should I revert it?
> > >>>
> > >>> if you want
> > >>
> > >> Well, IMHO av_set_string3 is right to print an error if the option does
> > >> not exists, it's an error.
> > >>
> > >> So IMHO patch is correct, application should not call it this way.
> > >>
> > >
> > > So what about this patch ? Can we please apply it to remove the annoying 
> > > nonsense messages ?
> > 
> > as i said av_set_string*() should not print these messages. They should
> > cleanly return the errorcode and let the user application decide or the user
> > application should redirect the av_log() output somehow.
> > 
> > with stefanos design, that is av_set_string() printing errors directly
> > for every little thing, things really cannot work. av_find_opt() should
> > then also print a "option not found message" for consistencies sake,
> > this of course doesnt work so why should av_set_string() print
> > something for a failure when av_find_opt() does not print anything
> > for a failure ?  Its just not consistent or logic IMHO
> 
> No, what av_opt_find() does is:
> "Look for an option in obj",
> 
> there are only two possible results for that operation, found or not
> found and this is perfectly clear from the returned value.

av_set_string3() also returns the option or NULL in o_out, that matches
av_opt_find() exactly, if its NULL it is not found



> 
> As for av_set_string3() it sets a value in an option whose is given
> the key, so there are many kind of failure which can occurr here, so
> it makes sense to print an error messages *togheter* with an error
> code to detail useful information for the user.
> 
> To make another example, avcodec_open() may fail for many reasons, but
> only returns -1, if we'll ever manage to return an error code we would
> still need an error message telling precisely the failure reason
> ("option foo is not implemented, samplerate 12345 is not supported,
> etc etc).
> 
> Now, I proposed the patch when I was implementing
> libavfilter.c:parse_key_value_pair(), and I have:
> 
>     ret = av_set_string3(ctx, key, val, 1, NULL);
> 
> I don't see why I should treat the "option not found case" in a
> different way, and have for example this:
>         
>     ret = av_set_string3(ctx, key, val, 1, NULL);
>     if (ret == AVERROR_ENOENT)
>        av_log(ctx, AV_LOG_ERROR, "Option '%s' not found\n", key);
> 
> So the question is:
> * is the case of a not found option in a context to be treated as a
>   special case by av_set_string3()?

The first question is, if "not finding an option" is an error or not
The second question is, how error messages are returned, simply printing
may be annoying as we see

If we consider not finding an option an error and decide to print errors
then av_opt_find() should print an error as well.
else neither of these functions should print an error for a not found
option.

Iam asking for consistency and there are several solutions that work
perfectly and are consistent but you cannot just return errors from
one function and pretent the identical condition is not an error in
a similar function and then use the second explicitly in the user app
to skip calling the first -> this is a really ugly hack
its like
if((x=open(fname))>=0){
    close(x);
    f=fopen(fname);
}

if each use of av_set_string3() needs a call to av_find_opt() first then
the API is crap! That is not convenient at all, besides being slow as the
search is done twice ...



> 
> My opinion is that no it shouldn't, others may prefer the opposite.
> 
> Anyway I won't object if someone will revert r18826, but I won't do
> that.

Well i dont know if reverting it is the best but its certainly the most
obvious solution, your patch that calls av_find_opt() explixitly before
each av_set_string3() is certainly worse, av_set_string3() calls 
av_find_opt() as well and av_find_opt() is slow. That cant be how we
would want the system used ...

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

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20091005/ddbe6345/attachment.pgp>