Re: [FFmpeg-devel] [PATCH 1/2] lavc/mediacodecenc: Probe supported pixel formats

[Zhao Zhili <quinkblack@foxmail.com>]

On Fri, 2023-01-06 at 16:42 +0100, Tomas Härdin wrote:


> For each entry in color_formats[] an encoder is configured and opened.
> If this succeeds then the corresponding pixel format is added to probed_pix_fmts[].
> 
> This patch has been released by Epic Games' legal department.
> ---
>  libavcodec/mediacodecenc.c | 76 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 68 insertions(+), 8 deletions(-)
> 
> diff --git a/libavcodec/mediacodecenc.c b/libavcodec/mediacodecenc.c
> index 4c1809093c..fd90d41625 100644
> --- a/libavcodec/mediacodecenc.c
> +++ b/libavcodec/mediacodecenc.c
> @@ -2,6 +2,7 @@
>   * Android MediaCodec encoders
>   *
>   * Copyright (c) 2022 Zhao Zhili <zhilizhao@tencent.com>
> + * Modifications by Epic Games, Inc., 2022.
>   *
>   * This file is part of FFmpeg.
>   *
> @@ -89,12 +90,8 @@ static const struct {
>      { COLOR_FormatSurface,              AV_PIX_FMT_MEDIACODEC },
>  };
>  
> -static const enum AVPixelFormat avc_pix_fmts[] = {
> -    AV_PIX_FMT_MEDIACODEC,
> -    AV_PIX_FMT_YUV420P,
> -    AV_PIX_FMT_NV12,
> -    AV_PIX_FMT_NONE
> -};
> +// filled in by mediacodec_init_static_data()
> +static enum AVPixelFormat probed_pix_fmts[FF_ARRAY_ELEMS(color_formats)+1];
>  
>  static void mediacodec_output_format(AVCodecContext *avctx)
>  {
> @@ -534,6 +531,69 @@ static av_cold int mediacodec_close(AVCodecContext *avctx)
>      return 0;
>  }
>  
> +static av_cold void mediacodec_init_static_data(FFCodec *ffcodec)
> +{
> +    const char *codec_mime = ffcodec->p.id == AV_CODEC_ID_H264 ? "video/avc" : "video/hevc";
> +    FFAMediaCodec *codec;
> +    int num_pix_fmts = 0;
> +    int use_ndk_codec = !av_jni_get_java_vm(NULL);
> +
> +    if (!(codec = ff_AMediaCodec_createEncoderByType(codec_mime, use_ndk_codec))) {
> +        av_log(NULL, AV_LOG_ERROR, "Failed to create encoder for type %s\n", codec_mime);
> +        return;
> +    }
> +
> +    for (int i = 0; i < FF_ARRAY_ELEMS(color_formats); i++) {
> +        if (color_formats[i].pix_fmt == AV_PIX_FMT_MEDIACODEC) {
> +            // assumme AV_PIX_FMT_MEDIACODEC always works
> +            // we don't have a context at this point with which to test it
> +            probed_pix_fmts[num_pix_fmts++] = color_formats[i].pix_fmt;
> +        } else {
> +            FFAMediaFormat *format;
> +            int ret;
> +
> +            if (!(format = ff_AMediaFormat_new(use_ndk_codec))) {
> +                av_log(NULL, AV_LOG_ERROR, "Failed to create media format\n");
> +                ff_AMediaCodec_delete(codec);
> +                continue;

Here is a use-after-free (codec) issue.

> +            }
> +
> +            ff_AMediaFormat_setString(format, "mime", codec_mime);
> +            ff_AMediaFormat_setInt32(format, "width", 1280);
> +            ff_AMediaFormat_setInt32(format, "height", 720);
> +            ff_AMediaFormat_setInt32(format, "color-format", color_formats[i].color_format);
> +            ff_AMediaFormat_setInt32(format, "bitrate", 1000000);
> +            ff_AMediaFormat_setInt32(format, "bitrate-mode", BITRATE_MODE_VBR);
> +            ff_AMediaFormat_setInt32(format, "frame-rate", 30);
> +            ff_AMediaFormat_setInt32(format, "i-frame-interval", 1);
> +
> +            // no need to set profile, level or number of B-frames it seems
> +            ret = ff_AMediaCodec_getConfigureFlagEncode(codec);
> +            ret = ff_AMediaCodec_configure(codec, format, NULL, NULL, ret);
> +            if (ret) {
> +                av_log(NULL, AV_LOG_ERROR, "MediaCodec configure failed, %s\n", av_err2str(ret));
> +                goto bailout;
> +            }
> +
> +            ret = ff_AMediaCodec_start(codec);
> +            if (ret) {
> +                av_log(NULL, AV_LOG_ERROR, "MediaCodec failed to start, %s\n", av_err2str(ret));
> +                goto bailout;
> +            }
> +            ff_AMediaCodec_stop(codec);
> +
> +            probed_pix_fmts[num_pix_fmts++] = color_formats[i].pix_fmt;
> +        bailout:
> +            // format is never NULL here
> +            ff_AMediaFormat_delete(format);
> +        }
> +    }
> +
> +    probed_pix_fmts[num_pix_fmts] = AV_PIX_FMT_NONE;
> +    ffcodec->p.pix_fmts = probed_pix_fmts;
> +    ff_AMediaCodec_delete(codec);
> +}
> +
>  static const AVCodecHWConfigInternal *const mediacodec_hw_configs[] = {
>      &(const AVCodecHWConfigInternal) {
>          .public          = {
> @@ -579,7 +639,7 @@ static const AVClass name ## _mediacodec_class = {  \
>  
>  #define DECLARE_MEDIACODEC_ENCODER(short_name, long_name, codec_id)     \
>  MEDIACODEC_ENCODER_CLASS(short_name)                                    \
> -const FFCodec ff_ ## short_name ## _mediacodec_encoder = {              \
> +FFCodec ff_ ## short_name ## _mediacodec_encoder = {              \
>      .p.name           = #short_name "_mediacodec",                      \
>      CODEC_LONG_NAME(long_name " Android MediaCodec encoder"),           \
>      .p.type           = AVMEDIA_TYPE_VIDEO,                             \
> @@ -587,7 +647,6 @@ const FFCodec ff_ ## short_name ## _mediacodec_encoder = {              \
>      .p.capabilities   = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY           \
>                          | AV_CODEC_CAP_HARDWARE,                        \
>      .priv_data_size   = sizeof(MediaCodecEncContext),                   \
> -    .p.pix_fmts       = avc_pix_fmts,                                   \
>      .init             = mediacodec_init,                                \
>      FF_CODEC_RECEIVE_PACKET_CB(mediacodec_encode),                      \
>      .close            = mediacodec_close,                               \
> @@ -595,6 +654,7 @@ const FFCodec ff_ ## short_name ## _mediacodec_encoder = {              \
>      .caps_internal    = FF_CODEC_CAP_INIT_CLEANUP,                      \
>      .p.wrapper_name = "mediacodec",                                     \
>      .hw_configs     = mediacodec_hw_configs,                            \
> +    .init_static_data = mediacodec_init_static_data,                    \
>  };                                                                      \
>  
>  #if CONFIG_H264_MEDIACODEC_ENCODER
> -- 
> 2.30.2
> 

init_static_data should be determinate, no matter when it was called, it should
give the same result. In addition to the 'different MediaCodec backends support
different pixel format' issue, another concern of this method is that it's not
determinate, it can give different results at different time/condition.

MediaCodec can fail for all kinds of reasons, and it can fail dynamically. For
example, the supported instance is limited (getMaxSupportedInstances()). Some
low end/legacy chip only support one instance. So it can fail when another app
or another SDK in the same app has already created a codec instance. It can
fail when out of other resouce (like GPU memory) temporarily. Since
init_static_data() only being called once, there is no way to recover from a
temporary failure.

If there is no other reason, stick to AV_PIX_FMT_MEDIACODEC/AV_PIX_FMT_NV12
should be safe.

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