Message ID | 20200410205801.6279-1-philipl@overt.org |
---|---|
State | Superseded |
Headers | show |
Series | [FFmpeg-devel] avcodec: Add explicit capability flag for encoder flushing | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
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@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.
On Fri, 10 Apr 2020 18:47:06 -0300 James Almer <jamrial@gmail.com> wrote: > > Seems fine in principle, but I'd like to hear Anton's opinion. Thanks. I've sent an updated patch and will wait for Anton. --phil
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) + /* 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)) { + 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. + 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", };
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@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(-)