| Message ID | 1a215429e2ac714005e4bc2ccb08c0c3d0bf9356.camel@haerdin.se |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] lavc/mediacodecenc: Probe supported pixel formats | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/configure_loongarch64 | warning | Failed to apply patch |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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.
lör 2023-01-07 klockan 02:03 +0800 skrev Zhao Zhili: > On Fri, 2023-01-06 at 16:42 +0100, Tomas Härdin wrote: > > > + 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. Oops. I had the call to ff_AMediaCodec_createEncoderByType() inside the loop earlier. Will fix. > init_static_data should be determinate, no matter when it was called, > it should > give the same result. You mean across devices? That obviously won't work. On the same device? I would assume it always returns the same results, modulo what you wrote below. > 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. Won't the encoder fail anyway in that case? Also will the JNI probe still fail in that case? > 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. Well, the code can try to probe the color formats more than once inside the function. But that feels very wrong. /Tomas
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Tomas Härdin > Sent: 2023年1月7日 2:22 > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH 1/2] lavc/mediacodecenc: Probe supported pixel formats > > lör 2023-01-07 klockan 02:03 +0800 skrev Zhao Zhili: > > On Fri, 2023-01-06 at 16:42 +0100, Tomas Härdin wrote: > > > > > + 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. > > Oops. I had the call to ff_AMediaCodec_createEncoderByType() inside the > loop earlier. Will fix. > > > > init_static_data should be determinate, no matter when it was called, > > it should > > give the same result. > > You mean across devices? That obviously won't work. On the same device? > I would assume it always returns the same results, modulo what you > wrote below. I mean on the same device. > > > 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. > > Won't the encoder fail anyway in that case? Also will the JNI probe > still fail in that case? Create encoder can fail, and recover after a while. If JNI probe depends on a codec instance, it has the same issue. Use the codeclist API should be fine, but then we don't know which omx codec is the default one. The JNI probe can tell which pixel format is supported after encoder configure failed at runtime. That's a safe and valid usecase. And it can be implemented out side of FFmpeg, so it can be called earlier and multiple times, without the limitation of init_static_data() being called only once. > > > 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. > > Well, the code can try to probe the color formats more than once inside > the function. But that feels very wrong. That's the problem, init_static_data() can't do retry by design. Probe multiple times inside init_static_data() doesn't work, unless there is a sleep after each loop, and we can't put sleep inside init_static_data(). > > /Tomas > > _______________________________________________ > 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".
> > > 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. > > > > Won't the encoder fail anyway in that case? Also will the JNI probe > > still fail in that case? > > Create encoder can fail, and recover after a while. If JNI probe > depends on > a codec instance, it has the same issue. Use the codeclist API should > be fine, > but then we don't know which omx codec is the default one. I did see the codeclist API but using a codec instance seemed simpler than replicating much of ff_AMediaCodecList_getCodecNameByType(). Is omx used only for software decoders or does it also handle hardware ones? I feel it bears pointing out again that for users in the US having the software codecs is also sometimes desirable. > > > 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. > > > > Well, the code can try to probe the color formats more than once > > inside > > the function. But that feels very wrong. > > That's the problem, init_static_data() can't do retry by design. > Probe multiple > times inside init_static_data() doesn't work, unless there is a sleep > after each > loop, and we can't put sleep inside init_static_data(). Indeed that would be very wrong. Is there some sort of mutex we can instruct users to lock before running ffmpeg? /Tomas
From eb6d090967b8ed7ea0ee0651a1f557633fa23517 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se> Date: Thu, 22 Dec 2022 13:29:58 +0100 Subject: [PATCH 1/2] lavc/mediacodecenc: Probe supported pixel formats 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; + } + + 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