[FFmpeg-devel] [PATCH] avcodec/openh264: return (DE|EN)CODER_NOT_FOUND if version check fails

[Martin Storsjö]

On Wed, 20 Apr 2022, Martin Storsjö wrote:

> On Fri, 18 Feb 2022, Andreas Schneider wrote:
>
>> Signed-off-by: Andreas Schneider <asn at cryptomilk.org>
>> ---
>> libavcodec/libopenh264dec.c | 2 +-
>> libavcodec/libopenh264enc.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/libavcodec/libopenh264dec.c b/libavcodec/libopenh264dec.c
>> index 7f5e85402a..97d3630df6 100644
>> --- a/libavcodec/libopenh264dec.c
>> +++ b/libavcodec/libopenh264dec.c
>> @@ -56,7 +56,7 @@ static av_cold int svc_decode_init(AVCodecContext *avctx)
>>     WelsTraceCallback callback_function;
>>
>>     if ((err = ff_libopenh264_check_version(avctx)) < 0)
>> -        return err;
>> +        return AVERROR_DECODER_NOT_FOUND;
>>
>>     if (WelsCreateDecoder(&s->decoder)) {
>>         av_log(avctx, AV_LOG_ERROR, "Unable to create decoder\n");
>> diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
>> index 7c0501a2eb..7649e7b025 100644
>> --- a/libavcodec/libopenh264enc.c
>> +++ b/libavcodec/libopenh264enc.c
>> @@ -137,7 +137,7 @@ static av_cold int svc_encode_init(AVCodecContext 
>> *avctx)
>>     AVCPBProperties *props;
>>
>>     if ((err = ff_libopenh264_check_version(avctx)) < 0)
>> -        return err;
>> +        return AVERROR_ENCODER_NOT_FOUND;
>>
>>     if (WelsCreateSVCEncoder(&s->encoder)) {
>>         av_log(avctx, AV_LOG_ERROR, "Unable to create encoder\n");
>> -- 
>> 2.35.1
>
> This looks reasonable to me, so I could push this in a little while if 
> there's no more comments on it.
>
> But the patch lacks an explanation of _why_ this is done, in addition to 
> _what_ it does. I presume that's because the current error code makes some 
> decoder/encoder selection logic error out entirely, instead of continuing 
> trying some other codec - is that right? That would really be valuable to 
> include in the commit message.

Actually, I don't see anything in the code where returning 
AVERROR_DECODER_NOT_FOUND would behave differently than returning 
AVERROR(EINVAL) (as I would say it does right now).

So what's the motive for this patch, what does it change in practice?

I guess it changes what error message is printed (which certainly makes 
sense to change), but it would be very relevant to know whether this is a 
cosmetic change or if it changes something functionally that I haven't 
found.

// Martin