[FFmpeg-devel] Meaningful return codes (was [PATCH 7/9] Replace AVERROR_NOTSUPP by AVERROR(ENOSYS))

[Panagiotis Issaris]

Hi

Op 20-jul-07, om 12:07 heeft Michael Niedermayer het volgende  
geschreven:

> Hi
>
> On Fri, Jul 20, 2007 at 11:05:12AM +0200, Panagiotis Issaris wrote:
>> Hi
>>
>> Op 19-jul-07, om 22:28 heeft Michael Niedermayer het volgende
>> geschreven:
>>
>>> Hi
>>>
>>> On Thu, Jul 19, 2007 at 07:22:00PM +0200, Panagiotis Issaris wrote:
>>>> Hi
>>>>
>>>> [...]
>>>> I see. Thanks for the information!
>>>>
>>>> Related to this: As I've mentioned before on this list, one of the
>>>> things
>>>> I'd like to change in libavcodec is the handling of errors. I'd  
>>>> like
>>>> returncodes indicating failure to carry information about the cause
>>>> of the
>>>> failure. So, for example, in the case of mpegvideo_enc.c, I'd
>>>> prefer to
>>>> see MPV_encode_init() return different errorcodes for different
>>>> kinds of
>>>> failures occurring instead of the currently used "return -1;"  
>>>> for all
>>>> errors.
>>>>
>>>> I'm not sure about the granularity though...
>>>>
>>>> Would something like this be nice & useful?
>>>>
>>>> #define AVERROR_PIXFMT_NOTSUP       <somevalue>
>>>> #define AVERROR_BFRAMES_NOTSUP      <somevalue+1>
>>>> #define AVERROR_RC_PARSE_ERROR      <somevalue+2>
>>>> ...
>>>
>>> whats the sense of this?
>>> an application which has code to parse these and do some appropriate
>>> action beyond printing an error message can as well just contain  
>>> code
>>> to set the values properly in the first place
>>>
>>
>> No, I think it's different. For example, it could be that the values
>> are not
>> set by the application but by an end-user. Ofcourse the app should  
>> parse
>> the values for legality in some sense, but lets say your writing a  
>> GUI
>> for transcoding files. The end-user tries to transcode a DVD to
>> MPEG-4 but
>> then tries to use the H.261 codec instead. Then you can either  
>> just show
>> an error-message explaining that only CIF and QCIF are supported  
>> or _for
>> some applications_ it could be useful to switch to CIF automatically.
>> I'm
>> pretty sure most of us here would prefer that _not_ to happen, but
>> for some
>> end-users, this could be the prefered way of doing things.
>>
>> When you're returning "-1" in all possible cases something is going
>> wrong,
>> an application can not choose how to handle such situations; it can
>> only display
>> the error message.
>
> well and if you get a return saying invalid_size like you suggest  
> above
> your application does what? try every width x height bruteforce?

No, the application could for example suggest a list of resolutions,  
not one specific for this codec, but just a generic list of commonly  
used resolutions (such as the one the -s parameter accepts).


> now if codecs contain a list of valid sizes, so can the application
> just check the user param against that and select a appropriate one
> the error code doesnt simplify that

The codecs themselves _know_ which sizes and parameters they can  
handle, as they fail on invalid ones. There's no need to duplicate  
that knowledge, _if_ only the application could be aware _why_  
av_codec_open returned -1. Because that is the current situation,  
whatever goes wrong av_codec_open gives you -1. That's not very helpful.


>
>
>>
>> Ofcourse, the application could also contain all the logic to do  
>> these
>> checks by itself, but imho that would be unnecessary code duplication
>> as that
>> exact logic is already available in the library.
>
> the exact is NOT available in the lib, lavc will generally fail  
> randomly
> if some nonsense combinations are used

I see checks all over the place checking for correct colorspace, for  
correct bitrate settings, if too many threads are being requested, etc..
But, in the current code, it does indeed return -1 in all of these  
cases. (As I've just noticed, for av_codec_open() even when memory  
allocation fails, I'll send a patch for that though).

It might fail on the first of the invalid parameters it encounters,  
but that's not random.

> you are totally going into the wrong direction here, the error codes
> would be a true nightmare for an application to do anything with
> its the most stupid idea ive seen here since a long time

So you think having meaningful return codes is the most stupid idea  
you've even seen here? So, would you have prefered if all of the  
POSIX calls returned -1 on failure? I strongly disagree. Now, if you  
are talking about having return code for every possible failure for  
every kind of codec, then yes, I agree. But that was not what I was  
trying to say.

Let's make this clear:
I'm *not* trying to get a list of thousands of return codes in  
libavcodec, I am just trying to determine how many return codes would  
be considered useful. Currently, libavcodec is using 8 out of the 80  
or so values POSIX defines. Is that considered enough, or could it be  
useful to bring in more? And, do they have to be identical to the  
POSIX ones, or do we need others more adapted to the multimedia domain?

I'm saying this because POSIX defines stuff which is quite concrete  
when compared to EINVAL and ENOTSUP:
[ECHILD]
No child processes.
[ECONNABORTED]
Connection aborted.
[ECONNREFUSED]
Connection refused.
[ECONNRESET]
Connection reset.
[EDEADLK]
Resource deadlock would occur.
[EDESTADDRREQ]
Destination address required.

These codes express pretty specific problems in the operating system/ 
networking domain, and can't express specific problems in a video/ 
audio codec library such as libavcodec. (Except for the RTP/HTTP  
stuff of course in libavformat...)



> just look at how many parameters we have, having an error code for  
> each
> is total sick, no application will contain code to do anything with  
> that
> monster list of codes

Sigh. I did not state that I think libavcodec should get a "monster  
list of codes". I just asked how large you'd prefer such a list of  
returncodes to be.


Currently, most of libavcodec acts as if there's only one possible  
return code for failure, being -1. The POSIX API for example doesn't  
work this way, it does specify a list of return codes for operating  
systems to use, and for each function these return codes can have a  
more specific meaning. We could do something similar. Maybe not,  
maybe it isn't needed.


Moreover, in this very thread it occurred to me that the meaning of  
ENOTSUP and ENOSYS aren't really clear, which made me wonder if it  
could be useful to have some return codes which express some concrete  
meaning like the ones POSIX defines (EDESTADDRREQ). Which is why I  
asked if it would be useful o add more concrete ones. Maybe the  
examples I gave were lousy, but I don't think the idea is lousy.




>
> what makes sense is a generic solution, that is one which tells you
> per indexes into an AVOption array which combination of fields are
> causing a problem and then somehow return a list describing  
> alternative
> minimal changes needed to avoid the problem
> like
>
> if someone tried mpeg4 + 4mv + ilme + simple_profile + 2_b_frames
> {flags, 4mv, 0}, {profile, main}
> {flags, ilme, 0}, {profile, main}
> {flags, ilme, 0}, {b_frames, 0}
>
> now tell me how do you want to describe these 3 alternative solutions
> with an error code

I am not saying we'd have to do that. But maybe we could  
differentiate in some way between different classes of problems.  
Maybe that's not even needed and we only need to return EINVAL in the  
cases you describe above, but _certainly_ I think we need to do  
something different from returning -1 in any case of failure.


Furthermore, I'm not sure it's possible to do what you are  
suggesting. Let's say you want to express things like the allowed  
framesize. Some codecs accept only a fixed list of resolutions,  
others might only accept even resolutions, of maybe some resolutions  
within some range.

>
>>
>> A good example would be the rc_eq parsing code. If libavcodec would
>> return
>> meaningful return codes, it would be easy to write an app providing
>> the posibility
>> to alter it. If not, it would have to duplicate a lot of the code
>> which is already
>> in libavcodec.
>
> it can easily be shown that its impossible to check rc_eq for sanity
> and a syntactically wrong rc_eq (which is the less likely case)
> should not silently be overridden

That depends on what you call a sanity check.


With friendly regards,
Takis
--
vCard: http://issaris.org/pi.vcf
PGP key: http://issaris.org/pi.key