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

[Panagiotis Issaris]

Hi

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

> Hi
>
> On Fri, Jul 20, 2007 at 03:12:15PM +0200, Panagiotis Issaris wrote:
>> 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).
>
> so you suggest that the application tries per brute force values from
> that list?
> this doesnt sound like a sane solution, even less so considering that
> the application can do that too with -1 return

No, it can't. If it gets a -1 return value, it cannot know what to  
brute force. It would have to brute force all parameters, right?  
Otherwise (if it knew the resolution was a problem) it could brute- 
force the list, yes. Not saying that this is a great idea though.




>>
>>> 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
>
> i did not suggest to duplicate it, you suggest to have a list of  
> "common"
> resultions which is equivalent to duplicating the specificially  
> supported
> resolutions, you just drop the information which codec supports them
>
>
>> , _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.
>
> there is little difference your suggestions needs a brure force  
> search,
> knowing that any parameter is wrong is enough for that, just start  
> with
> all defaults and add 1 user parameter at a time skiping all which  
> cause
> failure
> its ugly but doing such a search on the width/height or thread_count
> is already as ugly

Well, checking one list is certainly easier then checking all  
possible combination of lots of parameters. But, that was not really  
my point. I was not really suggesting to add such a monster list,  
just trying to get an idea of an acceptable improvement for the  
current situation.


> the advantage of the -1 based search is that its much simpler as it  
> doesnt
> need a monster list of error codes one for each parameter

I do not think brute forcing all combinations is simpler... but then  
again, I don't like monsters either :)


> [...]
>>> 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
>
> no, iam thinking having one error code corresponding for each of  
> several
> hundread arguments is a stupid idea if you can refer to them in a much
> simpler and generic way

Of course, that's one extreme of the solutionspace, having 1 error  
code corresponding to each problem with each argument. The other  
extreme, which I dislike too, is having only 1 error code, -1 :)

>
>
>> you've even seen here? So, would you have prefered if all of the
>> POSIX calls returned -1 on failure? I strongly disagree.
>
> posix is wisely designed, it has EINVAL, not 5000 codes one for each
> argument of each function

Yes, indeed it is wisely designed, as it does define a list of error  
codes. And not just EINVAL, but lots of 'em and some with a quite  
specific and concrete meaning.

And it defines more than 1 return code, which is currently the amount  
which most of libavcodec is using... and that is exactly why I  
started this discussion.


>
>> 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.
>
> you suggested error codes for specific fields of AVCodecContext,  
> nearly

Well, I was just wondering about all possible solutions, beside the  
currently used return -1. I was not requesting to have error codes  
per parameters, just trying to get a discussion started on an  
improvement for the current errorhandling. At least I succeeded in  
getting a discussion started :)

> every field has limits beyond which its invalid, there are 308 lines
> in the corresponding AVOption which mean there are approximately 300
> fields which would need an error code

The example I gave was one possible solution albeit an extreme one.  
But clearly, I do not hate it as much as you do.


>
>
>> 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?
>
> it could be usefull

I'll have a look at the current "return -1;"'s and see on a case-by- 
case basis which could be useful.

>
>
>> And, do they have to be identical to the
>> POSIX ones, or do we need others more adapted to the multimedia  
>> domain?
>
> i do not know

Me neither, which is why I'm trying to get some info from others  
through this mailinglist.

>
>>
>> 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...)
>
> the POSIX codes above tell you about failures which _you_ cannot know
> purely from the fact that a funtion failed

Sounds reasonable, although EDESTADDRREQ seems to different.

> and its arguments
> for invalid arguments there is EINVAL
> it certainly would make sense to have error codes like
> EVBVFAILURE

Excellent. So, now I know you would except extra error codes (if they  
are sane, needed, etc.).


>
>>
>>
>>
>>> 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.
>
> as large as needed

:) Can't disagree with that.

>
>
> [...]
>>> 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.
>
> you must have missunderstood me, i did not mean we should always
> return -1

Great.

> i just flamed you

Luckily, I'm always wearing my asbestos jacket when posting to ffmpeg- 
devel :)

> as it seemd you suggested to add a error code for
> each field in AVCodecContext, this was also what your example was
> showing (there was one for b_frames IIRC)

Yes, my example was maybe pretty bad. I often like to point out the  
extreme solutions to a problem when trying to find a commonly  
acceptable and useful solution. So, in this case:
1 error code ... heaps of error codes for each possible indivual failure

But that doesn't mean I think those solutions are great ideas, just  
that I'm trying to get a discussion started.


>
>
>>
>>
>> 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.
>
> max + multiple + fixed list would be enough ...

Yes, for the contraints I mentioned that would be enough.

Slightly OT: But sometimes you need even more complex contraints.  
Some time ago I was looking into ways for describing some kind of  
profile for specific devices. And it turned out to be quite hard imho  
as some of the constraints weren't simple statements like  
"resolutions A,B,C are supported" but more like, "resolution A is  
supported with framerate < B and bitrate < C" etc.. I couldn't find a  
_simple_ way to describe such constraints.


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