From patchwork Tue Apr 14 02:48:19 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Philip Langdale X-Patchwork-Id: 18938 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id DCEC744ADD8 for ; Tue, 14 Apr 2020 05:48:36 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id B8E7668B63A; Tue, 14 Apr 2020 05:48:36 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail.overt.org (mail.overt.org [157.230.92.47]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id EFDFF68B3FF for ; Tue, 14 Apr 2020 05:48:29 +0300 (EEST) Received: from authenticated-user (mail.overt.org [157.230.92.47]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by mail.overt.org (Postfix) with ESMTPSA id B6AF63F24A; Mon, 13 Apr 2020 21:48:27 -0500 (CDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=overt.org; s=mail; t=1586832508; bh=A8SpsMN3Aw+ob1+elzOVNPXiYjpULc7axMZlNOyUNbI=; h=From:To:Cc:Subject:Date:From; b=NBgAttVTvUObCVKo15BgQEoRs1G37qJGQemmLCST4j70I0hB4D3OoYPsxULsC56XE 7rV/L070OcvcxIC8u9jJErbX7NOrAGJj6/8UjGBOuoxC6NZYCFFc3r9N7aGRIxbd26 utVXVO1Y9J5Iptr1sGJhdm5d9fkr4vRJxs9RPySa+SSjDdpVbpwj6n/QjtlnUG9FT8 jTz1mgqrd/ihYLTElY+oxUV7CWaCuBXN8o/CQctPk0ZvdiJ8Su1Fz97sOOcizAZM0z 7l8ikGFe3eyXZJi7woLCiaOq9eTMosYivfukPIj+g/Lel3X8hhUk+mls0Ls1kPQcum hO740FKbvQW/A== From: Philip Langdale To: ffmpeg-devel@ffmpeg.org Date: Mon, 13 Apr 2020 19:48:19 -0700 Message-Id: <20200414024819.24185-1-philipl@overt.org> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH v4] avcodec: Add explicit capability flag for encoder flushing X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Cc: Philip Langdale Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Previously, there was no way to flush an encoder such that after draining, the encoder could be used again. We generally suggested that clients teardown and replace the encoder instance in these situations. However, for at least some hardware encoders, the cost of this tear down/replace cycle is very high, which can get in the way of some use-cases - for example: segmented encoding with nvenc. To help address that use case, we added support for calling avcodec_flush_buffers() to nvenc and things worked in practice, although it was not clearly documented as to whether this should work or not. There was only one previous example of an encoder implementing the flush callback (audiotoolboxenc) and it's unclear if that was untentional or not. However, it was clear that calling avocdec_flush_buffers() on any other encoder would leave the encoder in an undefined state, and 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 --- doc/APIchanges | 6 ++++++ libavcodec/audiotoolboxenc.c | 3 ++- libavcodec/avcodec.h | 25 ++++++++++++++++++++----- libavcodec/decode.c | 15 +++++++++++++++ libavcodec/nvenc_h264.c | 6 ++++-- libavcodec/nvenc_hevc.c | 6 ++++-- libavcodec/version.h | 4 ++-- 7 files changed, 53 insertions(+), 12 deletions(-) diff --git a/doc/APIchanges b/doc/APIchanges index 4cc2367e69..938ccf3aaa 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -15,6 +15,12 @@ libavutil: 2017-10-21 API changes, most recent first: +2020-04-10 - xxxxxxxxxx - lavc 58.79.100 - avcodec.h + Add formal support for calling avcodec_flush_buffers() on encoders. + Encoders that set the cap AV_CODEC_CAP_ENCODER_FLUSH will be flushed. + For all other encoders, the call is now a no-op rather than undefined + behaviour. + 2020-xx-xx - xxxxxxxxxx - lavc 58.78.100 - avcodec.h codec_desc.h codec_id.h packet.h Move AVCodecDesc-related public API to new header codec_desc.h. Move AVCodecID enum to new header codec_id.h. diff --git a/libavcodec/audiotoolboxenc.c b/libavcodec/audiotoolboxenc.c index 2c1891693e..27632decf5 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_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..b79b025e53 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_FLUSH (1 << 21) + /* Exported side data. These flags can be passed in AVCodecContext.export_side_data before initialization. */ @@ -4473,13 +4480,21 @@ 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_FLUSH. When called, the encoder + * will drain any remaining packets, and can then be re-used for a different + * stream (as opposed to sending a null frame which will leave the encoder + * in a permanent EOF state after draining). This can be desirable if the + * cost of tearing down and replacing the encoder instance is high. */ void avcodec_flush_buffers(AVCodecContext *avctx); diff --git a/libavcodec/decode.c b/libavcodec/decode.c index b7ae1fbb84..d4bdb9b1c0 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -2084,6 +2084,21 @@ 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_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; + } + + // We haven't implemented flushing for frame-threaded encoders. + av_assert0(!(caps & AV_CODEC_CAP_FRAME_THREADS)); + } + avci->draining = 0; avci->draining_done = 0; avci->nb_draining_errors = 0; diff --git a/libavcodec/nvenc_h264.c b/libavcodec/nvenc_h264.c index 479155fe15..02392db46a 100644 --- a/libavcodec/nvenc_h264.c +++ b/libavcodec/nvenc_h264.c @@ -214,7 +214,8 @@ AVCodec ff_nvenc_h264_encoder = { .priv_data_size = sizeof(NvencContext), .priv_class = &nvenc_h264_class, .defaults = defaults, - .capabilities = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE, + .capabilities = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE | + AV_CODEC_CAP_ENCODER_FLUSH, .caps_internal = FF_CODEC_CAP_INIT_CLEANUP, .pix_fmts = ff_nvenc_pix_fmts, .wrapper_name = "nvenc", @@ -244,7 +245,8 @@ AVCodec ff_h264_nvenc_encoder = { .priv_data_size = sizeof(NvencContext), .priv_class = &h264_nvenc_class, .defaults = defaults, - .capabilities = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE, + .capabilities = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_HARDWARE | + AV_CODEC_CAP_ENCODER_FLUSH, .caps_internal = FF_CODEC_CAP_INIT_CLEANUP, .pix_fmts = ff_nvenc_pix_fmts, .wrapper_name = "nvenc", diff --git a/libavcodec/nvenc_hevc.c b/libavcodec/nvenc_hevc.c index 7c9b3848f1..ea337a514f 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_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_FLUSH, .caps_internal = FF_CODEC_CAP_INIT_CLEANUP, .wrapper_name = "nvenc", }; diff --git a/libavcodec/version.h b/libavcodec/version.h index b348a6ca7a..a4eb83124c 100644 --- a/libavcodec/version.h +++ b/libavcodec/version.h @@ -28,8 +28,8 @@ #include "libavutil/version.h" #define LIBAVCODEC_VERSION_MAJOR 58 -#define LIBAVCODEC_VERSION_MINOR 78 -#define LIBAVCODEC_VERSION_MICRO 102 +#define LIBAVCODEC_VERSION_MINOR 79 +#define LIBAVCODEC_VERSION_MICRO 100 #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \ LIBAVCODEC_VERSION_MINOR, \