| Message ID | 20220419181850.3392814-1-bohanli@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v2] avcodec/libaomenc: Add unmet target level warning | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
Gentle ping on this :) On Tue, Apr 19, 2022 at 11:20 AM Bohan Li <bohanli@google.com> wrote: > When target levels are set, this patch checks whether they are > satisfied by libaom. If not, a warning is shown. Otherwise the output > levels are also logged. > > This patch applies basically the same approach used for libvpx. > > Signed-off-by: Bohan Li <bohanli@google.com> > --- > libavcodec/libaomenc.c | 64 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 64 insertions(+) > > diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c > index 054903e6e2..77be56fa51 100644 > --- a/libavcodec/libaomenc.c > +++ b/libavcodec/libaomenc.c > @@ -198,6 +198,12 @@ static const char *const ctlidstr[] = { > [AV1E_SET_ENABLE_SMOOTH_INTERINTRA] = > "AV1E_SET_ENABLE_SMOOTH_INTERINTRA", > [AV1E_SET_ENABLE_REF_FRAME_MVS] = "AV1E_SET_ENABLE_REF_FRAME_MVS", > #endif > +#ifdef AOM_CTRL_AV1E_GET_SEQ_LEVEL_IDX > + [AV1E_GET_SEQ_LEVEL_IDX] = "AV1E_GET_SEQ_LEVEL_IDX", > +#endif > +#ifdef AOM_CTRL_AV1E_GET_TARGET_SEQ_LEVEL_IDX > + [AV1E_GET_TARGET_SEQ_LEVEL_IDX] = "AV1E_GET_TARGET_SEQ_LEVEL_IDX", > +#endif > }; > > static av_cold void log_encoder_error(AVCodecContext *avctx, const char > *desc) > @@ -323,10 +329,68 @@ static av_cold int codecctl_int(AVCodecContext > *avctx, > return 0; > } > > +#if defined(AOM_CTRL_AV1E_GET_SEQ_LEVEL_IDX) && \ > + defined(AOM_CTRL_AV1E_GET_TARGET_SEQ_LEVEL_IDX) > +static av_cold int codecctl_intp(AVCodecContext *avctx, > +#ifdef UENUM1BYTE > + aome_enc_control_id id, > +#else > + enum aome_enc_control_id id, > +#endif > + int* ptr) > +{ > + AOMContext *ctx = avctx->priv_data; > + char buf[80]; > + int width = -30; > + int res; > + > + snprintf(buf, sizeof(buf), "%s:", ctlidstr[id]); > + av_log(avctx, AV_LOG_DEBUG, " %*s%d\n", width, buf, *ptr); > + > + res = aom_codec_control(&ctx->encoder, id, ptr); > + if (res != AOM_CODEC_OK) { > + snprintf(buf, sizeof(buf), "Failed to set %s codec control", > + ctlidstr[id]); > + log_encoder_error(avctx, buf); > + return AVERROR(EINVAL); > + } > + > + return 0; > +} > +#endif > + > static av_cold int aom_free(AVCodecContext *avctx) > { > AOMContext *ctx = avctx->priv_data; > > +#if defined(AOM_CTRL_AV1E_GET_SEQ_LEVEL_IDX) && \ > + defined(AOM_CTRL_AV1E_GET_TARGET_SEQ_LEVEL_IDX) > + if (!(avctx->flags & AV_CODEC_FLAG_PASS1)) { > + int levels[32] = { 0 }; > + int target_levels[32] = { 0 }; > + > + if (!codecctl_intp(avctx, AV1E_GET_SEQ_LEVEL_IDX, levels) && > + !codecctl_intp(avctx, AV1E_GET_TARGET_SEQ_LEVEL_IDX, > + target_levels)) { > + for (int i = 0; i < 32; i++) { > + if (levels[i] > target_levels[i]) { > + // Warn when the target level was not met > + av_log(avctx, AV_LOG_WARNING, > + "Could not encode to target level %d.%d for " > + "operating point %d. The output level is > %d.%d.\n", > + 2 + (target_levels[i] >> 2), target_levels[i] > & 3, > + i, 2 + (levels[i] >> 2), levels[i] & 3); > + } else if (target_levels[i] < 31) { > + // Log the encoded level if a target level was given > + av_log(avctx, AV_LOG_INFO, > + "Output level for operating point %d is > %d.%d.\n", > + i, 2 + (levels[i] >> 2), levels[i] & 3); > + } > + } > + } > + } > +#endif > + > aom_codec_destroy(&ctx->encoder); > av_freep(&ctx->twopass_stats.buf); > av_freep(&avctx->stats_out); > -- > 2.36.0.rc0.470.gd361397f0d-goog > >
Another ping :) On Fri, Apr 29, 2022 at 2:46 PM Bohan Li <bohanli@google.com> wrote: > Gentle ping on this :) > > On Tue, Apr 19, 2022 at 11:20 AM Bohan Li <bohanli@google.com> wrote: > >> When target levels are set, this patch checks whether they are >> satisfied by libaom. If not, a warning is shown. Otherwise the output >> levels are also logged. >> >> This patch applies basically the same approach used for libvpx. >> >> Signed-off-by: Bohan Li <bohanli@google.com> >> --- >> libavcodec/libaomenc.c | 64 ++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 64 insertions(+) >> >> diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c >> index 054903e6e2..77be56fa51 100644 >> --- a/libavcodec/libaomenc.c >> +++ b/libavcodec/libaomenc.c >> @@ -198,6 +198,12 @@ static const char *const ctlidstr[] = { >> [AV1E_SET_ENABLE_SMOOTH_INTERINTRA] = >> "AV1E_SET_ENABLE_SMOOTH_INTERINTRA", >> [AV1E_SET_ENABLE_REF_FRAME_MVS] = >> "AV1E_SET_ENABLE_REF_FRAME_MVS", >> #endif >> +#ifdef AOM_CTRL_AV1E_GET_SEQ_LEVEL_IDX >> + [AV1E_GET_SEQ_LEVEL_IDX] = "AV1E_GET_SEQ_LEVEL_IDX", >> +#endif >> +#ifdef AOM_CTRL_AV1E_GET_TARGET_SEQ_LEVEL_IDX >> + [AV1E_GET_TARGET_SEQ_LEVEL_IDX] = >> "AV1E_GET_TARGET_SEQ_LEVEL_IDX", >> +#endif >> }; >> >> static av_cold void log_encoder_error(AVCodecContext *avctx, const char >> *desc) >> @@ -323,10 +329,68 @@ static av_cold int codecctl_int(AVCodecContext >> *avctx, >> return 0; >> } >> >> +#if defined(AOM_CTRL_AV1E_GET_SEQ_LEVEL_IDX) && \ >> + defined(AOM_CTRL_AV1E_GET_TARGET_SEQ_LEVEL_IDX) >> +static av_cold int codecctl_intp(AVCodecContext *avctx, >> +#ifdef UENUM1BYTE >> + aome_enc_control_id id, >> +#else >> + enum aome_enc_control_id id, >> +#endif >> + int* ptr) >> +{ >> + AOMContext *ctx = avctx->priv_data; >> + char buf[80]; >> + int width = -30; >> + int res; >> + >> + snprintf(buf, sizeof(buf), "%s:", ctlidstr[id]); >> + av_log(avctx, AV_LOG_DEBUG, " %*s%d\n", width, buf, *ptr); >> + >> + res = aom_codec_control(&ctx->encoder, id, ptr); >> + if (res != AOM_CODEC_OK) { >> + snprintf(buf, sizeof(buf), "Failed to set %s codec control", >> + ctlidstr[id]); >> + log_encoder_error(avctx, buf); >> + return AVERROR(EINVAL); >> + } >> + >> + return 0; >> +} >> +#endif >> + >> static av_cold int aom_free(AVCodecContext *avctx) >> { >> AOMContext *ctx = avctx->priv_data; >> >> +#if defined(AOM_CTRL_AV1E_GET_SEQ_LEVEL_IDX) && \ >> + defined(AOM_CTRL_AV1E_GET_TARGET_SEQ_LEVEL_IDX) >> + if (!(avctx->flags & AV_CODEC_FLAG_PASS1)) { >> + int levels[32] = { 0 }; >> + int target_levels[32] = { 0 }; >> + >> + if (!codecctl_intp(avctx, AV1E_GET_SEQ_LEVEL_IDX, levels) && >> + !codecctl_intp(avctx, AV1E_GET_TARGET_SEQ_LEVEL_IDX, >> + target_levels)) { >> + for (int i = 0; i < 32; i++) { >> + if (levels[i] > target_levels[i]) { >> + // Warn when the target level was not met >> + av_log(avctx, AV_LOG_WARNING, >> + "Could not encode to target level %d.%d for " >> + "operating point %d. The output level is >> %d.%d.\n", >> + 2 + (target_levels[i] >> 2), target_levels[i] >> & 3, >> + i, 2 + (levels[i] >> 2), levels[i] & 3); >> + } else if (target_levels[i] < 31) { >> + // Log the encoded level if a target level was given >> + av_log(avctx, AV_LOG_INFO, >> + "Output level for operating point %d is >> %d.%d.\n", >> + i, 2 + (levels[i] >> 2), levels[i] & 3); >> + } >> + } >> + } >> + } >> +#endif >> + >> aom_codec_destroy(&ctx->encoder); >> av_freep(&ctx->twopass_stats.buf); >> av_freep(&avctx->stats_out); >> -- >> 2.36.0.rc0.470.gd361397f0d-goog >> >>
On Tue, Apr 19, 2022 at 11:20 AM Bohan Li <bohanli-at-google.com@ffmpeg.org> wrote: > > When target levels are set, this patch checks whether they are > satisfied by libaom. If not, a warning is shown. Otherwise the output > levels are also logged. > > This patch applies basically the same approach used for libvpx. > > Signed-off-by: Bohan Li <bohanli@google.com> > --- > libavcodec/libaomenc.c | 64 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 64 insertions(+) > lgtm.
On Tue, May 17, 2022 at 12:45 PM James Zern <jzern@google.com> wrote: > > On Tue, Apr 19, 2022 at 11:20 AM Bohan Li > <bohanli-at-google.com@ffmpeg.org> wrote: > > > > When target levels are set, this patch checks whether they are > > satisfied by libaom. If not, a warning is shown. Otherwise the output > > levels are also logged. > > > > This patch applies basically the same approach used for libvpx. > > > > Signed-off-by: Bohan Li <bohanli@google.com> > > --- > > libavcodec/libaomenc.c | 64 ++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 64 insertions(+) > > > > lgtm. > + } else if (target_levels[i] < 31) { > + // Log the encoded level if a target level was given > + av_log(avctx, AV_LOG_INFO, > + "Output level for operating point %d is %d.%d.", > + i, 2 + (levels[i] >> 2), levels[i] & 3); > + } Actually this is a bit spammy. If there's only one operating point set then I'd expect a single line output, but this seems to print all 32 regardless. Is that expected?
Thanks for the reply, James! This is indeed not the intended behaviour, but it is due to libaom not initializing all the indices correctly. I've submitted a patch for review in libaom that fixes this, and after that patch the levels are only logged when a target level is given with this ffmpeg patch. Best, Bohan On Tue, May 24, 2022 at 5:43 PM James Zern <jzern@google.com> wrote: > On Tue, May 17, 2022 at 12:45 PM James Zern <jzern@google.com> wrote: > > > > On Tue, Apr 19, 2022 at 11:20 AM Bohan Li > > <bohanli-at-google.com@ffmpeg.org> wrote: > > > > > > When target levels are set, this patch checks whether they are > > > satisfied by libaom. If not, a warning is shown. Otherwise the output > > > levels are also logged. > > > > > > This patch applies basically the same approach used for libvpx. > > > > > > Signed-off-by: Bohan Li <bohanli@google.com> > > > --- > > > libavcodec/libaomenc.c | 64 ++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 64 insertions(+) > > > > > > > lgtm. > > > + } else if (target_levels[i] < 31) { > > + // Log the encoded level if a target level was given > > + av_log(avctx, AV_LOG_INFO, > > + "Output level for operating point %d is > %d.%d.", > > + i, 2 + (levels[i] >> 2), levels[i] & 3); > > + } > > Actually this is a bit spammy. If there's only one operating point set > then I'd expect a single line output, but this seems to print all 32 > regardless. Is that expected? >
On Tue, May 17, 2022 at 12:45 PM James Zern <jzern@google.com> wrote: > > On Tue, Apr 19, 2022 at 11:20 AM Bohan Li > <bohanli-at-google.com@ffmpeg.org> wrote: > > > > When target levels are set, this patch checks whether they are > > satisfied by libaom. If not, a warning is shown. Otherwise the output > > levels are also logged. > > > > This patch applies basically the same approach used for libvpx. > > > > Signed-off-by: Bohan Li <bohanli@google.com> > > --- > > libavcodec/libaomenc.c | 64 ++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 64 insertions(+) > > > > lgtm. applied. thanks for the patch.
diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c index 054903e6e2..77be56fa51 100644 --- a/libavcodec/libaomenc.c +++ b/libavcodec/libaomenc.c @@ -198,6 +198,12 @@ static const char *const ctlidstr[] = { [AV1E_SET_ENABLE_SMOOTH_INTERINTRA] = "AV1E_SET_ENABLE_SMOOTH_INTERINTRA", [AV1E_SET_ENABLE_REF_FRAME_MVS] = "AV1E_SET_ENABLE_REF_FRAME_MVS", #endif +#ifdef AOM_CTRL_AV1E_GET_SEQ_LEVEL_IDX + [AV1E_GET_SEQ_LEVEL_IDX] = "AV1E_GET_SEQ_LEVEL_IDX", +#endif +#ifdef AOM_CTRL_AV1E_GET_TARGET_SEQ_LEVEL_IDX + [AV1E_GET_TARGET_SEQ_LEVEL_IDX] = "AV1E_GET_TARGET_SEQ_LEVEL_IDX", +#endif }; static av_cold void log_encoder_error(AVCodecContext *avctx, const char *desc) @@ -323,10 +329,68 @@ static av_cold int codecctl_int(AVCodecContext *avctx, return 0; } +#if defined(AOM_CTRL_AV1E_GET_SEQ_LEVEL_IDX) && \ + defined(AOM_CTRL_AV1E_GET_TARGET_SEQ_LEVEL_IDX) +static av_cold int codecctl_intp(AVCodecContext *avctx, +#ifdef UENUM1BYTE + aome_enc_control_id id, +#else + enum aome_enc_control_id id, +#endif + int* ptr) +{ + AOMContext *ctx = avctx->priv_data; + char buf[80]; + int width = -30; + int res; + + snprintf(buf, sizeof(buf), "%s:", ctlidstr[id]); + av_log(avctx, AV_LOG_DEBUG, " %*s%d\n", width, buf, *ptr); + + res = aom_codec_control(&ctx->encoder, id, ptr); + if (res != AOM_CODEC_OK) { + snprintf(buf, sizeof(buf), "Failed to set %s codec control", + ctlidstr[id]); + log_encoder_error(avctx, buf); + return AVERROR(EINVAL); + } + + return 0; +} +#endif + static av_cold int aom_free(AVCodecContext *avctx) { AOMContext *ctx = avctx->priv_data; +#if defined(AOM_CTRL_AV1E_GET_SEQ_LEVEL_IDX) && \ + defined(AOM_CTRL_AV1E_GET_TARGET_SEQ_LEVEL_IDX) + if (!(avctx->flags & AV_CODEC_FLAG_PASS1)) { + int levels[32] = { 0 }; + int target_levels[32] = { 0 }; + + if (!codecctl_intp(avctx, AV1E_GET_SEQ_LEVEL_IDX, levels) && + !codecctl_intp(avctx, AV1E_GET_TARGET_SEQ_LEVEL_IDX, + target_levels)) { + for (int i = 0; i < 32; i++) { + if (levels[i] > target_levels[i]) { + // Warn when the target level was not met + av_log(avctx, AV_LOG_WARNING, + "Could not encode to target level %d.%d for " + "operating point %d. The output level is %d.%d.\n", + 2 + (target_levels[i] >> 2), target_levels[i] & 3, + i, 2 + (levels[i] >> 2), levels[i] & 3); + } else if (target_levels[i] < 31) { + // Log the encoded level if a target level was given + av_log(avctx, AV_LOG_INFO, + "Output level for operating point %d is %d.%d.\n", + i, 2 + (levels[i] >> 2), levels[i] & 3); + } + } + } + } +#endif + aom_codec_destroy(&ctx->encoder); av_freep(&ctx->twopass_stats.buf); av_freep(&avctx->stats_out);
When target levels are set, this patch checks whether they are satisfied by libaom. If not, a warning is shown. Otherwise the output levels are also logged. This patch applies basically the same approach used for libvpx. Signed-off-by: Bohan Li <bohanli@google.com> --- libavcodec/libaomenc.c | 64 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)