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

Fri Jul 20 16:02:28 CEST 2007

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

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

[...]
> > 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

> 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

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

> 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

> 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

> 
> 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 and its arguments
for invalid arguments there is EINVAL
it certainly would make sense to have error codes like
EVBVFAILURE

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

[...]
> > 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
i just flamed you 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)

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

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

Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070720/221a50f9/attachment.pgp>