[FFmpeg-devel] [RFC] The meaning of AVERROR_NOTSUPP

[Howard Chu]

Stefano Sabatini wrote:
> On date Saturday 2010-03-27 00:51:13 +0100, Stefano Sabatini encoded:
>> On date Tuesday 2010-03-23 00:57:17 +0100, Stefano Sabatini encoded:
>>> On date Monday 2010-03-22 10:27:03 +0100, Michael Niedermayer encoded:
>>>> On Sun, Mar 21, 2010 at 10:51:12PM +0100, Stefano Sabatini wrote:
>>>>> On date Sunday 2010-03-21 22:38:28 +0100, Michael Niedermayer encoded:
>> [...]
>>>> AVERROR_NOTSUPP feels more toward a feature that can be supported but is not
>>>> we also might want to have a
>>>> AVERROR_DISABLED for a feature suported but not compiled in
>>>> but then there is AVERROR_PATCHWELCOME and iam not sure what AVERROR_NOTSUPP is
>>>> supposed to mean then
>>>> anyway, error docs are poor and you are changing error code without their
>>>> meaning being clearly documented.
>>>
>>> Description for AVERROR_NOTSUPP:
>>> #define AVERROR_NOTSUPP     AVERROR(ENOSYS)  /**<  Operation not supported. */
>>>
>>> Description for AVERROR_PATCHWELCOME:
>>> #define AVERROR_PATCHWELCOME    (-MKTAG('P','A','W','E')) /**<  Not yet implemented in FFmpeg. Patches welcome. */
>>>
>>> My interpretation of AVERROR_NOTSUPP:
>>> The *operation* requested is not legal/valid, as it is not supported
>>> by the element being requested.
>>>
>>> So AVERROR_NOTSUPP should be returned just in case a certain operation
>>> is requested to an object for which that operation doesn't make sense
>>> / cannot be implemented, and in this sense is semantically near to the
>>> meaning of ENOSYS.
>>>
>>> Think for example of a seek operation in a pipe, for which
>>> AVERROR_PATCHWELCOME wouldn't make sense.
>>>
>>> I recognize though that the term "supported" in the "not supported"
>>> expression may lead to confusion, as it suggests what you said, so I'd
>>> suggest to use instead AVERROR_ILLEGALOPERATION or
>>> AVERROR_INVALIDOPERATION.
>>>
>>> As for the distinction between AVERROR_DISABLED and
>>> AVERROR_PATCHWELCOME, I don't think that would be a good idea
>>> implementation-wise, as most of the time a program can't tell the
>>> distinction amongst the two.
>>>
>>> For example av_error_input_format() doesn't know if it can't recognize
>>> a format because it has not been compiled in, because the format is
>>> not implemented, or because we inflicted to him bogus random data.
>>>
>>> If you agree with the semantics I proposed, I'll post a patch for
>>> clarifying the description of AVERROR_NOTSUPP, also tell me what do
>>> you think about the AVERROR_INVALIDOPERATION rename.
>>
>> Currently we have just one use of AVERROR_NOTSUPP:
>>
>> in libavformat/file.c:
>> static int64_t file_seek(URLContext *h, int64_t pos, int whence)
>> {
>>      int fd = (intptr_t) h->priv_data;
>>      if (whence != SEEK_SET&&  whence != SEEK_CUR&&  whence != SEEK_END)
>>          return AVERROR_NOTSUPP;
>>      return lseek(fd, pos, whence);
>> }
>>
>> This suggests the interpretation of AVERROR_NOTSUPP as "Operation
>> non-valid or illegal".
>>
>> Currently AVERROR_NOTSUPP is defined as AVERROR(ENOSYS), definition from [1]:
>> |[ENOSYS]
>> | Function not implemented. An attempt was made to use a function that
>> | is not available in this implementation.
>>
>> Then we have the description of ENOTSUP, lexically close to AVERROR_NOTSUPP:
>> |[ENOTSUP]
>> | Not supported. The implementation does not support this feature of
>> | the Realtime Option Group.
>>
>> Both these descriptions have some resemblance with the interpretation
>> of AVERROR_NOTSUPP ->  feature not implemented.
>>
>> Since the semantics of AVERROR_NOTSUPP ->  feature not
>> implemented is already covered by AVERROR_PATCHWELCOME, I suggest to
>> prefer the semantics of AVERROR_NOTSUPP ->  operation non-valid or
>> illegal.
>>
>> So I suggest to *rename* the symbol:
>> AVERROR_NOTSUPP ->  AVERROR_INVALIDOPERATION
>>
>> and drop the old definition at the next major bump, and adopt the
>> following description:
>> "Invalid or illegal operation requested. An operation has been
>> requested which does not make sense in the given context or cannot be
>> implemented in the requested object."
>>
>> Comments are welcome, regards.
>>
>> [1] IEEE 1003.1 error code descriptions: http://www.opengroup.org/onlinepubs/000095399/functions/xsh_chap02_03.html
>
> See attached patches.

Seems to me you're trying to split hairs. Also, using these codes as you 
suggest will lend them false significance over time. E.g., the spec for a 
stream type may change in the future, and what was once illegal/invalid 
becomes legal. If you're reading the code and see INVALIDOPERATION you will 
simply dismiss this function as never being valid (when in fact you can never 
say never). It's better to leave the question open (not implemented today, 
can't say anything about the future).

-- 
   -- Howard Chu
   CTO, Symas Corp.           http://www.symas.com
   Director, Highland Sun     http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP  http://www.openldap.org/project/