[FFmpeg-devel] [PATCH] avcodec: Add explicit capability flag for encoder flushing

[James Almer]

On 4/10/2020 5:58 PM, Philip Langdale wrote:
> We've been in this fuzzy situation where maybe you could call
> avcodec_flush_buffers() on an encoder but there weren't any encoders
> that supported it except maybe audiotoolboxenc. Then we added flush
> support to nvenc very intentionally, and it worked, but that was more a
> coincidence than anything else. And if you call avcodec_flush_buffers()
> on an encoder that doesn't support it, it'll leave the encoder in an
> undefined state, so that's not great.
> 
> As part of cleaning this up, this change introduces a formal capability
> flag for encoders that support flushing and ensures a flush call is a
> no-op for any other encoder. This allows client code to check if it is
> meaningful to call flush on an encoder before actually doing it.
> 
> I have not attempted to separate the steps taken inside
> avcodec_flush_buffers() because it's not doing anything that's wrong
> for an encoder. But I did add a sanity check to reject attempts to
> flush a frame threaded encoder because I couldn't wrap my head around
> whether that code path was actually safe or not. As this combination
> doesn't exist today, we'll deal with it if it ever comes up.
> 
> Signed-off-by: Philip Langdale <philipl at overt.org>
> ---
>  libavcodec/audiotoolboxenc.c |  3 ++-
>  libavcodec/avcodec.h         | 21 ++++++++++++++++-----
>  libavcodec/decode.c          | 18 ++++++++++++++++++
>  libavcodec/nvenc_hevc.c      |  6 ++++--
>  4 files changed, 40 insertions(+), 8 deletions(-)

Missing changes to nvenc_h264.c

Also a line in doc/APIChanges and a version bump, but that can be added
before pushing.

> 
> diff --git a/libavcodec/audiotoolboxenc.c b/libavcodec/audiotoolboxenc.c
> index 2c1891693e..7cb5ffd915 100644
> --- a/libavcodec/audiotoolboxenc.c
> +++ b/libavcodec/audiotoolboxenc.c
> @@ -627,7 +627,8 @@ static const AVOption options[] = {
>          .encode2        = ffat_encode, \
>          .flush          = ffat_encode_flush, \
>          .priv_class     = &ffat_##NAME##_enc_class, \
> -        .capabilities   = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY __VA_ARGS__, \
> +        .capabilities   = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY | \
> +                          AV_CODEC_CAP_ENCODER_CAN_FLUSH __VA_ARGS__, \
>          .sample_fmts    = (const enum AVSampleFormat[]) { \
>              AV_SAMPLE_FMT_S16, \
>              AV_SAMPLE_FMT_U8,  AV_SAMPLE_FMT_NONE \
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 55151a0b71..216ba3f79a 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -513,6 +513,13 @@ typedef struct RcOverride{
>   */
>  #define AV_CODEC_CAP_ENCODER_REORDERED_OPAQUE (1 << 20)
>  
> +/**
> + * This encoder can be flushed using avcodec_flush_buffers(). If this flag is
> + * not set, the encoder must be closed and reopened to ensure that no frames
> + * remain pending.
> + */
> +#define AV_CODEC_CAP_ENCODER_CAN_FLUSH   (1 << 21)

Maybe just AV_CODEC_CAP_ENCODER_FLUSH.

> +
>  /* Exported side data.
>     These flags can be passed in AVCodecContext.export_side_data before initialization.
>  */
> @@ -4473,13 +4480,17 @@ int avcodec_fill_audio_frame(AVFrame *frame, int nb_channels,
>                               int buf_size, int align);
>  
>  /**
> - * Reset the internal decoder state / flush internal buffers. Should be called
> + * Reset the internal codec state / flush internal buffers. Should be called
>   * e.g. when seeking or when switching to a different stream.
>   *
> - * @note when refcounted frames are not used (i.e. avctx->refcounted_frames is 0),
> - * this invalidates the frames previously returned from the decoder. When
> - * refcounted frames are used, the decoder just releases any references it might
> - * keep internally, but the caller's reference remains valid.
> + * @note for decoders, when refcounted frames are not used
> + * (i.e. avctx->refcounted_frames is 0), this invalidates the frames previously
> + * returned from the decoder. When refcounted frames are used, the decoder just
> + * releases any references it might keep internally, but the caller's reference
> + * remains valid.
> + *
> + * @note for encoders, this function will only do something if the encoder
> + * declares support for AV_CODEC_CAP_ENCODER_CAN_FLUSH.
>   */
>  void avcodec_flush_buffers(AVCodecContext *avctx);
>  
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index b7ae1fbb84..d21b5461e5 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -2084,6 +2084,24 @@ void avcodec_flush_buffers(AVCodecContext *avctx)
>  {
>      AVCodecInternal *avci = avctx->internal;
>  
> +    if (av_codec_is_encoder(avctx->codec)) {

Unrelated to this patch, but avcodec_is_open(avctx) may need to be
checked before even avci is set to avctx->internal above.

> +        int caps = avctx->codec->capabilities;
> +        if (!(caps & AV_CODEC_CAP_ENCODER_CAN_FLUSH)) {
> +            // Only encoders that explicitly declare support for it can be
> +            // flushed. Otherwise, this is a no-op.
> +            av_log(avctx, AV_LOG_WARNING, "Ignoring attempt to flush encoder "
> +                   "that doesn't support it\n");
> +            return;
> +        }
> +        if (caps & AV_CODEC_CAP_FRAME_THREADS) {
> +            // We haven't implemented flushing for frame-threaded encoders.

This needs to be an av_assert0(), then. No encoder should claim support
for both until it's implemented.

> +            av_log(avctx, AV_LOG_WARNING, "Ignoring attempt to flush encoder "
> +                   "that claims to support both flushing and frame "
> +                   "threading\n");
> +            return;
> +        }
> +    }
> +
>      avci->draining      = 0;
>      avci->draining_done = 0;
>      avci->nb_draining_errors = 0;
> diff --git a/libavcodec/nvenc_hevc.c b/libavcodec/nvenc_hevc.c
> index 7c9b3848f1..02ece979fd 100644
> --- a/libavcodec/nvenc_hevc.c
> +++ b/libavcodec/nvenc_hevc.c
> @@ -174,7 +174,8 @@ AVCodec ff_nvenc_hevc_encoder = {
>      .priv_class     = &nvenc_hevc_class,
>      .defaults       = defaults,
>      .pix_fmts       = ff_nvenc_pix_fmts,
> -    .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> +    .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE |
> +                      AV_CODEC_CAP_ENCODER_CAN_FLUSH,
>      .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
>      .wrapper_name   = "nvenc",
>  };
> @@ -203,7 +204,8 @@ AVCodec ff_hevc_nvenc_encoder = {
>      .priv_class     = &hevc_nvenc_class,
>      .defaults       = defaults,
>      .pix_fmts       = ff_nvenc_pix_fmts,
> -    .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE,
> +    .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE |
> +                      AV_CODEC_CAP_ENCODER_CAN_FLUSH,
>      .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
>      .wrapper_name   = "nvenc",
>  };

Seems fine in principle, but I'd like to hear Anton's opinion.

Thanks.