Re: [FFmpeg-devel] [PATCH] lavc/libopenh264: Check for noopenh264

[Akihiko Odaki <akihiko.odaki@gmail.com>]

On 2024/02/11 19:24, Andreas Rheinhardt wrote:
> Akihiko Odaki:
>> noopenh264 is a "fake implementation of the OpenH264 library we can link
>> from regardless of the actual library being available":
>> https://gitlab.com/freedesktop-sdk/noopenh264
>>
>> A distributor may wish to link against openh264/noopenh264 and let
>> the decoder and encoder work only if the actual library is available.
>>
>> On the other hand, an application may want to know if the decoder or
>> encoder is available beforehand so that it can determine what format to
>> download for decoding, or what format to advertise for the peer
>> receiving the encoded video.
>>
>> Check the availability of the actual library at runtime initialization
>> and do not expose the encoder and decoder if they are not available.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
>> ---
>>   libavcodec/codec_internal.h |  2 ++
>>   libavcodec/libopenh264dec.c | 36 +++++++++++++++++++++++------------
>>   libavcodec/libopenh264enc.c | 46 ++++++++++++++++++++++++++++-----------------
>>   libavcodec/tests/avcodec.c  |  2 ++
>>   4 files changed, 57 insertions(+), 29 deletions(-)
>>
>> diff --git a/libavcodec/codec_internal.h b/libavcodec/codec_internal.h
>> index 130a7dc3cd..635af644fa 100644
>> --- a/libavcodec/codec_internal.h
>> +++ b/libavcodec/codec_internal.h
>> @@ -122,6 +122,8 @@ enum FFCodecType {
>>       /* The codec is an encoder using the receive_packet callback;
>>        * audio and video codecs only. */
>>       FF_CODEC_CB_TYPE_RECEIVE_PACKET,
>> +    /* The codec is not available. */
>> +    FF_CODEC_CB_TYPE_NONE,
>>   };
>>   
>>   typedef struct FFCodec {
>> diff --git a/libavcodec/libopenh264dec.c b/libavcodec/libopenh264dec.c
>> index b6a9bba2dc..f5544142aa 100644
>> --- a/libavcodec/libopenh264dec.c
>> +++ b/libavcodec/libopenh264dec.c
>> @@ -48,6 +48,17 @@ static av_cold int svc_decode_close(AVCodecContext *avctx)
>>       return 0;
>>   }
>>   
>> +static av_cold void svc_decode_init_static_data(FFCodec *codec)
>> +{
>> +    ISVCDecoder *decoder;
>> +
>> +    if (WelsCreateDecoder(&decoder)) {
>> +        codec->cb_type = FF_CODEC_CB_TYPE_NONE;
>> +    } else {
>> +        WelsDestroyDecoder(decoder);
>> +    }
>> +}
>> +
>>   static av_cold int svc_decode_init(AVCodecContext *avctx)
>>   {
>>       SVCContext *s = avctx->priv_data;
>> @@ -153,18 +164,19 @@ static int svc_decode_frame(AVCodecContext *avctx, AVFrame *avframe,
>>       return avpkt->size;
>>   }
>>   
>> -const FFCodec ff_libopenh264_decoder = {
>> -    .p.name         = "libopenh264",
>> +FFCodec ff_libopenh264_decoder = {
>> +    .p.name           = "libopenh264",
>>       CODEC_LONG_NAME("OpenH264 H.264 / AVC / MPEG-4 AVC / MPEG-4 part 10"),
>> -    .p.type         = AVMEDIA_TYPE_VIDEO,
>> -    .p.id           = AV_CODEC_ID_H264,
>> -    .priv_data_size = sizeof(SVCContext),
>> -    .init           = svc_decode_init,
>> +    .p.type           = AVMEDIA_TYPE_VIDEO,
>> +    .p.id             = AV_CODEC_ID_H264,
>> +    .priv_data_size   = sizeof(SVCContext),
>> +    .init_static_data = svc_decode_init_static_data,
>> +    .init             = svc_decode_init,
>>       FF_CODEC_DECODE_CB(svc_decode_frame),
>> -    .close          = svc_decode_close,
>> -    .p.capabilities = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_DR1,
>> -    .caps_internal  = FF_CODEC_CAP_SETS_PKT_DTS |
>> -                      FF_CODEC_CAP_INIT_CLEANUP,
>> -    .bsfs           = "h264_mp4toannexb",
>> -    .p.wrapper_name = "libopenh264",
>> +    .close            = svc_decode_close,
>> +    .p.capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_DR1,
>> +    .caps_internal    = FF_CODEC_CAP_SETS_PKT_DTS |
>> +                        FF_CODEC_CAP_INIT_CLEANUP,
>> +    .bsfs             = "h264_mp4toannexb",
>> +    .p.wrapper_name   = "libopenh264",
>>   };
>> diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c
>> index 6f231d22b2..1963f2cf5c 100644
>> --- a/libavcodec/libopenh264enc.c
>> +++ b/libavcodec/libopenh264enc.c
>> @@ -106,6 +106,17 @@ static av_cold int svc_encode_close(AVCodecContext *avctx)
>>       return 0;
>>   }
>>   
>> +static av_cold void svc_encode_init_static_data(FFCodec *codec)
>> +{
>> +    ISVCEncoder *encoder;
>> +
>> +    if (WelsCreateSVCEncoder(&encoder)) {
>> +        codec->cb_type = FF_CODEC_CB_TYPE_NONE;
>> +    } else {
>> +        WelsDestroySVCEncoder(encoder);
>> +    }
>> +}
>> +
>>   static av_cold int svc_encode_init(AVCodecContext *avctx)
>>   {
>>       SVCContext *s = avctx->priv_data;
>> @@ -429,23 +440,24 @@ static const FFCodecDefault svc_enc_defaults[] = {
>>       { NULL },
>>   };
>>   
>> -const FFCodec ff_libopenh264_encoder = {
>> -    .p.name         = "libopenh264",
>> +FFCodec ff_libopenh264_encoder = {
>> +    .p.name           = "libopenh264",
>>       CODEC_LONG_NAME("OpenH264 H.264 / AVC / MPEG-4 AVC / MPEG-4 part 10"),
>> -    .p.type         = AVMEDIA_TYPE_VIDEO,
>> -    .p.id           = AV_CODEC_ID_H264,
>> -    .p.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_OTHER_THREADS |
>> -                      AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE,
>> -    .priv_data_size = sizeof(SVCContext),
>> -    .init           = svc_encode_init,
>> +    .p.type           = AVMEDIA_TYPE_VIDEO,
>> +    .p.id             = AV_CODEC_ID_H264,
>> +    .p.capabilities   = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_OTHER_THREADS |
>> +                        AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE,
>> +    .priv_data_size   = sizeof(SVCContext),
>> +    .init_static_data = svc_encode_init_static_data,
>> +    .init             = svc_encode_init,
>>       FF_CODEC_ENCODE_CB(svc_encode_frame),
>> -    .close          = svc_encode_close,
>> -    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP |
>> -                      FF_CODEC_CAP_AUTO_THREADS,
>> -    .p.pix_fmts     = (const enum AVPixelFormat[]){ AV_PIX_FMT_YUV420P,
>> -                                                    AV_PIX_FMT_YUVJ420P,
>> -                                                    AV_PIX_FMT_NONE },
>> -    .defaults       = svc_enc_defaults,
>> -    .p.priv_class   = &class,
>> -    .p.wrapper_name = "libopenh264",
>> +    .close            = svc_encode_close,
>> +    .caps_internal    = FF_CODEC_CAP_INIT_CLEANUP |
>> +                        FF_CODEC_CAP_AUTO_THREADS,
>> +    .p.pix_fmts       = (const enum AVPixelFormat[]){ AV_PIX_FMT_YUV420P,
>> +                                                      AV_PIX_FMT_YUVJ420P,
>> +                                                      AV_PIX_FMT_NONE },
>> +    .defaults         = svc_enc_defaults,
>> +    .p.priv_class     = &class,
>> +    .p.wrapper_name   = "libopenh264",
>>   };
>> diff --git a/libavcodec/tests/avcodec.c b/libavcodec/tests/avcodec.c
>> index 08ca507bf0..0e2ecccbf9 100644
>> --- a/libavcodec/tests/avcodec.c
>> +++ b/libavcodec/tests/avcodec.c
>> @@ -112,6 +112,8 @@ int main(void){
>>           case FF_CODEC_CB_TYPE_RECEIVE_PACKET:
>>               is_encoder = 1;
>>               break;
>> +        case FF_CODEC_CB_TYPE_NONE:
>> +            continue;
>>           default:
>>               ERR("Codec %s has unknown cb_type\n");
>>               continue;
>>
>> ---
>> base-commit: 81c2557691b12ceb79b3ba92aa496f2301ab4d18
>> change-id: 20240210-noopenh264-650461efbc33
>>
>> Best regards,

Hi,

Thank you for reviewing patch.

> 
> 1. Your patch will only make these codecs unavailable when searching for
> them by name or by codec id, but not when iterating over all codecs.

Right. What about filtering in av_codec_iterate()?

> 2. The init_static_data callbacks are supposed to only perform very
> light work and not create encoder/decoders. After all, they are run
> whenever one requests any codec, even when one is not interested in
> these codecs. > 3. In case of e.g. a temporary allocation failure, creating these codec
> contexts can fail, even if the proper library is present.

Creating encoder/decoders sounds very expensive and prone to allocation 
failure, but in reality it only allocates memory and fill vtable, which 
should never fail in a healthy platform. The real initialization work is 
done by ISVCEncoder::InitializeExt() or ISVCDecoder::Initialize(), where 
parameters necessary to e.g., allocate buffers are given.

That said, I'd like to hear if you have an alternative idea.

> 4. The reindentation of the other initializers should be performed in a
> separate commit (if at all).

I'll do so if I'm going to submit v2.

Regards,
Akihiko Odaki

> 
> - Andreas
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".