Re: [FFmpeg-devel] [PATCH v5 2/9] avcodec/vaapi_encode: introduce a base layer for vaapi encode

[Mark Thompson <sw@jkqxz.net>]

On 18/02/2024 08:45, tong1.wu-at-intel.com@ffmpeg.org wrote:
> From: Tong Wu <tong1.wu@intel.com>
> 
> Since VAAPI and future D3D12VA implementation may share the same dpb
> logic and some other functions. A base layer encode context is introduced
> as vaapi context's base and extract the related funtions to a common
> file such as receive_packet, init, close, etc.
> 
> Signed-off-by: Tong Wu <tong1.wu@intel.com>
> ---
>   libavcodec/Makefile             |   2 +-
>   libavcodec/hw_base_encode.c     | 637 ++++++++++++++++++++++
>   libavcodec/hw_base_encode.h     | 277 ++++++++++
>   libavcodec/vaapi_encode.c       | 924 +++++++-------------------------
>   libavcodec/vaapi_encode.h       | 229 +-------
>   libavcodec/vaapi_encode_av1.c   |  73 +--
>   libavcodec/vaapi_encode_h264.c  | 201 +++----
>   libavcodec/vaapi_encode_h265.c  | 163 +++---
>   libavcodec/vaapi_encode_mjpeg.c |  22 +-
>   libavcodec/vaapi_encode_mpeg2.c |  53 +-
>   libavcodec/vaapi_encode_vp8.c   |  28 +-
>   libavcodec/vaapi_encode_vp9.c   |  70 +--
>   12 files changed, 1427 insertions(+), 1252 deletions(-)
>   create mode 100644 libavcodec/hw_base_encode.c
>   create mode 100644 libavcodec/hw_base_encode.h
> 
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 470d7cb9b1..23946f6ea3 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -164,7 +164,7 @@ OBJS-$(CONFIG_STARTCODE)               += startcode.o
>   OBJS-$(CONFIG_TEXTUREDSP)              += texturedsp.o
>   OBJS-$(CONFIG_TEXTUREDSPENC)           += texturedspenc.o
>   OBJS-$(CONFIG_TPELDSP)                 += tpeldsp.o
> -OBJS-$(CONFIG_VAAPI_ENCODE)            += vaapi_encode.o
> +OBJS-$(CONFIG_VAAPI_ENCODE)            += vaapi_encode.o hw_base_encode.o
>   OBJS-$(CONFIG_AV1_AMF_ENCODER)         += amfenc_av1.o
>   OBJS-$(CONFIG_VC1DSP)                  += vc1dsp.o
>   OBJS-$(CONFIG_VIDEODSP)                += videodsp.o
> diff --git a/libavcodec/hw_base_encode.c b/libavcodec/hw_base_encode.c
> ...

I'm going to trust that the contents of this file are only moved around with no functional changes.  If that isn't the case please do say.

> diff --git a/libavcodec/hw_base_encode.h b/libavcodec/hw_base_encode.h
> new file mode 100644
> index 0000000000..70982c97b2
> --- /dev/null
> +++ b/libavcodec/hw_base_encode.h
> @@ -0,0 +1,277 @@
> +/*
> + * Copyright (c) 2024 Intel Corporation

I would avoid adding this.  Most of these files have content mostly copied from other files which are not exclusively copyright by Intel Corporation.

> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#ifndef AVCODEC_HW_BASE_ENCODE_H
> +#define AVCODEC_HW_BASE_ENCODE_H
> +
> +#include "libavutil/hwcontext.h"
> +#include "libavutil/fifo.h"
> +
> +#include "avcodec.h"
> +#include "hwconfig.h"

This file doesn't do anything with hwconfig stuff?

> +
> +enum {
> +    MAX_DPB_SIZE           = 16,
> +    MAX_PICTURE_REFERENCES = 2,
> +    MAX_REORDER_DELAY      = 16,
> +    MAX_ASYNC_DEPTH        = 64,
> +    MAX_REFERENCE_LIST_NUM = 2,
> +};
> +
> +enum {
> +    PICTURE_TYPE_IDR = 0,
> +    PICTURE_TYPE_I   = 1,
> +    PICTURE_TYPE_P   = 2,
> +    PICTURE_TYPE_B   = 3,
> +};
> +
> +enum {
> +    // Codec supports controlling the subdivision of pictures into slices.
> +    FLAG_SLICE_CONTROL         = 1 << 0,
> +    // Codec only supports constant quality (no rate control).
> +    FLAG_CONSTANT_QUALITY_ONLY = 1 << 1,
> +    // Codec is intra-only.
> +    FLAG_INTRA_ONLY            = 1 << 2,
> +    // Codec supports B-pictures.
> +    FLAG_B_PICTURES            = 1 << 3,
> +    // Codec supports referencing B-pictures.
> +    FLAG_B_PICTURE_REFERENCES  = 1 << 4,
> +    // Codec supports non-IDR key pictures (that is, key pictures do
> +    // not necessarily empty the DPB).
> +    FLAG_NON_IDR_KEY_PICTURES  = 1 << 5,
> +    // Codec output packet without timestamp delay, which means the
> +    // output packet has same PTS and DTS.
> +    FLAG_TIMESTAMP_NO_DELAY    = 1 << 6,
> +};
> +
> +enum {
> +    RC_MODE_AUTO,
> +    RC_MODE_CQP,
> +    RC_MODE_CBR,
> +    RC_MODE_VBR,
> +    RC_MODE_ICQ,

I thought ICQ was an Intel name which leaked into libva?  This probably shouldn't be in a generic API, maybe something about constant-quality.

> +    RC_MODE_QVBR,
> +    RC_MODE_AVBR,

More generally, I think you want to document what these modes are intended to be.  When they mapped directly to VAAPI then it was clear that the responsibility was "whatever libva gives you", but when there are multiple possibilities which do not use exactly the same options it is less clear.

> +    RC_MODE_MAX = RC_MODE_AVBR,
> +};
> +
> +typedef struct HWBaseEncodePicture {
> +    struct HWBaseEncodePicture *next;
> +
> +    int64_t         display_order;
> +    int64_t         encode_order;
> +    int64_t         pts;
> +    int64_t         duration;
> +    int             force_idr;
> +
> +    void           *opaque;
> +    AVBufferRef    *opaque_ref;
> +
> +    int             type;
> +    int             b_depth;
> +    int             encode_issued;
> +    int             encode_complete;
> +
> +    AVFrame        *input_image;
> +    AVFrame        *recon_image;
> +
> +    void           *priv_data;
> +
> +    // Whether this picture is a reference picture.
> +    int             is_reference;
> +
> +    // The contents of the DPB after this picture has been decoded.
> +    // This will contain the picture itself if it is a reference picture,
> +    // but not if it isn't.
> +    int                     nb_dpb_pics;
> +    struct HWBaseEncodePicture *dpb[MAX_DPB_SIZE];
> +    // The reference pictures used in decoding this picture. If they are
> +    // used by later pictures they will also appear in the DPB. ref[0][] for
> +    // previous reference frames. ref[1][] for future reference frames.
> +    int                     nb_refs[MAX_REFERENCE_LIST_NUM];
> +    struct HWBaseEncodePicture *refs[MAX_REFERENCE_LIST_NUM][MAX_PICTURE_REFERENCES];
> +    // The previous reference picture in encode order.  Must be in at least
> +    // one of the reference list and DPB list.
> +    struct HWBaseEncodePicture *prev;
> +    // Reference count for other pictures referring to this one through
> +    // the above pointers, directly from incomplete pictures and indirectly
> +    // through completed pictures.
> +    int             ref_count[2];
> +    int             ref_removed[2];
> +} HWBaseEncodePicture;
> +
> +typedef struct HWEncodeType {
> +    HWBaseEncodePicture * (*alloc)(AVCodecContext *avctx, AVFrame *frame);
> +
> +    int (*issue)(AVCodecContext *avctx, HWBaseEncodePicture *base_pic);

The semantics of this are a bit blurred across the boundary of this call.  I think ideally you want the picture structure to be const here to make it clear?

(Move the recon_image allocation and issued-flag setting out of the callee, in particular.)

> +
> +    int (*output)(AVCodecContext *avctx, HWBaseEncodePicture *base_pic, AVPacket *pkt);

Feels like the picture structure should again be const for the same reason.

> +
> +    int (*free)(AVCodecContext *avctx, HWBaseEncodePicture *base_pic);

The VAAPI implementation of this is definitely doing lots of things that should be in the base layer.

> +}  HWEncodeType;

I think the naming of this wants to be clear that they are operations on a single picture (either the type name or the fields).

Some documentation of the precise semantics of the callbacks would definitely help as well to be sure that the split is correct.

> +
> +typedef struct HWBaseEncodeContext {
> +    const AVClass *class;
> +
> +    // Hardware-specific hooks.
> +    const struct HWEncodeType *hw;
> +
> +    // Global options.
> +
> +    // Number of I frames between IDR frames.
> +    int             idr_interval;
> +
> +    // Desired B frame reference depth.
> +    int             desired_b_depth;
> +
> +    // Explicitly set RC mode (otherwise attempt to pick from
> +    // available modes).
> +    int             explicit_rc_mode;
> +
> +    // Explicitly-set QP, for use with the "qp" options.
> +    // (Forces CQP mode when set, overriding everything else.)
> +    int             explicit_qp;
> +
> +    // The required size of surfaces.  This is probably the input
> +    // size (AVCodecContext.width|height) aligned up to whatever
> +    // block size is required by the codec.
> +    int             surface_width;
> +    int             surface_height;
> +
> +    // The block size for slice calculations.
> +    int             slice_block_width;
> +    int             slice_block_height;
> +
> +    // RC quality level - meaning depends on codec and RC mode.
> +    // In CQP mode this sets the fixed quantiser value.
> +    int             rc_quality;
> +
> +    AVBufferRef    *device_ref;
> +    AVHWDeviceContext *device;
> +
> +    // The hardware frame context containing the input frames.
> +    AVBufferRef    *input_frames_ref;
> +    AVHWFramesContext *input_frames;
> +
> +    // The hardware frame context containing the reconstructed frames.
> +    AVBufferRef    *recon_frames_ref;
> +    AVHWFramesContext *recon_frames;
> +
> +    // Current encoding window, in display (input) order.
> +    HWBaseEncodePicture *pic_start, *pic_end;
> +    // The next picture to use as the previous reference picture in
> +    // encoding order. Order from small to large in encoding order.
> +    HWBaseEncodePicture *next_prev[MAX_PICTURE_REFERENCES];
> +    int                  nb_next_prev;
> +
> +    // Next input order index (display order).
> +    int64_t         input_order;
> +    // Number of frames that output is behind input.
> +    int64_t         output_delay;
> +    // Next encode order index.
> +    int64_t         encode_order;
> +    // Number of frames decode output will need to be delayed.
> +    int64_t         decode_delay;
> +    // Next output order index (in encode order).
> +    int64_t         output_order;
> +
> +    // Timestamp handling.
> +    int64_t         first_pts;
> +    int64_t         dts_pts_diff;
> +    int64_t         ts_ring[MAX_REORDER_DELAY * 3 +
> +                            MAX_ASYNC_DEPTH];
> +
> +    // Frame type decision.
> +    int gop_size;
> +    int closed_gop;
> +    int gop_per_idr;
> +    int p_per_i;
> +    int max_b_depth;
> +    int b_per_p;
> +    int force_idr;
> +    int idr_counter;
> +    int gop_counter;
> +    int end_of_stream;
> +    int p_to_gpb;
> +
> +    // Whether the driver supports ROI at all.
> +    int             roi_allowed;
> +
> +    // The encoder does not support cropping information, so warn about
> +    // it the first time we encounter any nonzero crop fields.
> +    int             crop_warned;
> +    // If the driver does not support ROI then warn the first time we
> +    // encounter a frame with ROI side data.
> +    int             roi_warned;
> +
> +    AVFrame         *frame;
> +
> +    // Whether the HW supports sync buffer function.
> +    // If supported, encode_fifo/async_depth will be used together.
> +    // Used for output buffer synchronization.
> +    int             async_encode;
> +
> +    // Store buffered pic
> +    AVFifo          *encode_fifo;
> +    // Max number of frame buffered in encoder.
> +    int             async_depth;
> +
> +    /** Tail data of a pic, now only used for av1 repeat frame header. */
> +    AVPacket        *tail_pkt;
> +} HWBaseEncodeContext;
> +
> +int ff_hw_base_encode_receive_packet(AVCodecContext *avctx, AVPacket *pkt);
> +
> +int ff_hw_base_encode_init(AVCodecContext *avctx);
> +
> +int ff_hw_base_encode_close(AVCodecContext *avctx);
> +
> +#define HW_BASE_ENCODE_COMMON_OPTIONS \
> +    { "idr_interval", \
> +      "Distance (in I-frames) between IDR frames", \
> +      OFFSET(common.base.idr_interval), AV_OPT_TYPE_INT, \
> +      { .i64 = 0 }, 0, INT_MAX, FLAGS }, \
> +    { "b_depth", \
> +      "Maximum B-frame reference depth", \
> +      OFFSET(common.base.desired_b_depth), AV_OPT_TYPE_INT, \
> +      { .i64 = 1 }, 1, INT_MAX, FLAGS }, \
> +    { "async_depth", "Maximum processing parallelism. " \
> +      "Increase this to improve single channel performance.", \
> +      OFFSET(common.base.async_depth), AV_OPT_TYPE_INT, \
> +      { .i64 = 2 }, 1, MAX_ASYNC_DEPTH, FLAGS }
> +
> +#define HW_BASE_ENCODE_RC_MODE(name, desc) \
> +    { #name, desc, 0, AV_OPT_TYPE_CONST, { .i64 = RC_MODE_ ## name }, \
> +      0, 0, FLAGS, "rc_mode" }
> +#define HW_BASE_ENCODE_RC_OPTIONS \
> +    { "rc_mode",\
> +      "Set rate control mode", \
> +      OFFSET(common.base.explicit_rc_mode), AV_OPT_TYPE_INT, \
> +      { .i64 = RC_MODE_AUTO }, RC_MODE_AUTO, RC_MODE_MAX, FLAGS, .unit = "rc_mode" }, \
> +    { "auto", "Choose mode automatically based on other parameters", \
> +      0, AV_OPT_TYPE_CONST, { .i64 = RC_MODE_AUTO }, 0, 0, FLAGS, .unit = "rc_mode" }, \
> +    HW_BASE_ENCODE_RC_MODE(CQP,  "Constant-quality"), \
> +    HW_BASE_ENCODE_RC_MODE(CBR,  "Constant-bitrate"), \
> +    HW_BASE_ENCODE_RC_MODE(VBR,  "Variable-bitrate"), \
> +    HW_BASE_ENCODE_RC_MODE(ICQ,  "Intelligent constant-quality"), \
> +    HW_BASE_ENCODE_RC_MODE(QVBR, "Quality-defined variable-bitrate"), \
> +    HW_BASE_ENCODE_RC_MODE(AVBR, "Average variable-bitrate")

This set of options is rather nasty because the set which makes sense depends on the API (see how your later D3D12 patch can't map some of them but has to deal with them anyway).  Maybe avoid extracting this to the common layer?

> +
> +#endif /* AVCODEC_HW_BASE_ENCODE_H */
> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> ...

Thanks,

- Mark
_______________________________________________
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".