Quoting Dawid Kozinski (2022-06-22 08:49:04)
> - Added xeve encoder wrapper
> - Added xevd dencoder wrapper
> - Added documentation for xeve and xevd wrappers
> - Added parser for EVC format
> - Changes in project configuration file and libavcodec Makefile
>
> Signed-off-by: Dawid Kozinski <d.kozin...@samsung.com>
> ---
> configure | 8 +
> doc/decoders.texi | 24 ++
> doc/encoders.texi | 112 ++++++
> doc/general_contents.texi | 19 +
> libavcodec/Makefile | 3 +
> libavcodec/allcodecs.c | 2 +
> libavcodec/avcodec.h | 3 +
> libavcodec/evc_parser.c | 527 ++++++++++++++++++++++++++
> libavcodec/libxevd.c | 440 ++++++++++++++++++++++
> libavcodec/libxeve.c | 753 ++++++++++++++++++++++++++++++++++++++
> libavcodec/parsers.c | 1 +
> 11 files changed, 1892 insertions(+)
> create mode 100644 libavcodec/evc_parser.c
> create mode 100644 libavcodec/libxevd.c
> create mode 100644 libavcodec/libxeve.c
Numerous violations of our coding style, such as
- keywords (if, for, etc.) should be followed by a space before parentheses
- opening brace for blocks inside functions should be on the same line
as their keyword (if, for, etc.)
- useless extra parentheses in if()s
I will not point out each instance, please find and fix those.
> diff --git a/doc/encoders.texi b/doc/encoders.texi
> index 1850c99fe9..245df680d2 100644
> --- a/doc/encoders.texi
> +++ b/doc/encoders.texi
> @@ -2892,6 +2892,118 @@ ffmpeg -i input -c:v libxavs2 -xavs2-params
> RdoqLevel=0 output.avs2
> @end example
> @end table
>
> +@section libxeve
> +
> +eXtra-fast Essential Video Encoder (XEVE) MPEG-5 EVC encoder wrapper.
> +
> +This encoder requires the presence of the libxeve headers and library
> +during configuration. You need to explicitly configure the build with
> +@option{--enable-libxeve}.
> +
> +@float NOTE
> +Many libxeve encoder options are mapped to FFmpeg global codec options,
> +while unique encoder options are provided through private options.
> +Additionally the xeve-params private options allows one to pass a list
> +of key=value tuples as accepted by the libxeve @code{parse_xeve_params}
> function.
> +@end float
> +
> +The xeve project website is at @url{https://github.com/mpeg5/xeve}.
> +
> +@subsection Options
> +
> +The following options are supported by the libxeve wrapper.
> +The xeve-equivalent options or values are listed in parentheses for easy
> migration.
> +
> +@float NOTE
> +To reduce the duplication of documentation, only the private options
> +and some others requiring special attention are documented here. For
> +the documentation of the undocumented generic options, see
> +@ref{codec-options,,the Codec Options chapter}.
> +@end float
> +
> +@float NOTE
> +To get a more accurate and extensive documentation of the libxeve options,
> +invoke the command @code{xeve_app --help} or consult the libxeve
> documentation.
> +@end float
> +
> +@table @option
> +@item b (@emph{bitrate})
> +Set target video bitrate in bits/s.
> +Note that FFmpeg’s b option is expressed in bits/s, while xeve’s bitrate
> is in kilobits/s.
Looks broken.
> diff --git a/libavcodec/libxeve.c b/libavcodec/libxeve.c
> new file mode 100644
> index 0000000000..e115ce63d2
> --- /dev/null
> +++ b/libavcodec/libxeve.c
> +/**
> + * The structure stores all the states associated with the instance of Xeve
> MPEG-5 EVC encoder
> + */
> +typedef struct XeveContext {
> + const AVClass *class;
> +
> + XEVE id; // XEVE instance identifier
> + XEVE_CDSC cdsc; // coding parameters i.e profile, width & height of
> input frame, num of therads, frame rate ...
> + XEVE_BITB bitb; // bitstream buffer (output)
> + XEVE_STAT stat; // encoding status (output)
> + XEVE_IMGB imgb; // image buffer (input)
> +
> + State state; // encoder state (skipping, encoding, bumping)
> +
> + // Chroma subsampling
> + int width_luma;
> + int height_luma;
> + int width_chroma;
> + int height_chroma;
All these are only used in init, no need to store them in the context.
> +/**
> + * Gets Xeve pre-defined preset
> + *
> + * @param preset string describing Xeve encoder preset (fast, medium, slow,
> placebo)
> + * @return XEVE pre-defined profile on success, negative value on failure
> + */
> +static int get_preset_id(const char *preset)
> +{
> + if((!strcmp(preset, "fast")))
> + return XEVE_PRESET_FAST;
> + else if (!strcmp(preset, "medium"))
> + return XEVE_PRESET_MEDIUM;
> + else if (!strcmp(preset, "slow"))
> + return XEVE_PRESET_SLOW;
> + else if (!strcmp(preset, "placebo"))
> + return XEVE_PRESET_PLACEBO;
> + else
> + return AVERROR(EINVAL);
> +}
This parsing is unnecessary, change the relevant option into an int and
add named constants for it (see e.g. "coder" in libx264.c and many
others).
Same for tune.
> +/**
> + * Convert FFmpeg pixel format (AVPixelFormat) into XEVE pre-defined color
> space
> + *
> + * @param[in] px_fmt pixel format (@see
> https://ffmpeg.org/doxygen/trunk/pixfmt_8h.html#a9a8e335cf3be472042bc9f0cf80cd4c5)
> + *
> + * @return XEVE pre-defined color space (@see xeve.h) on success,
> XEVE_CF_UNKNOWN on failure
> + */
> +static int xeve_color_space(enum AVPixelFormat av_pix_fmt)
IIUC the "xeve_" namespace is used by the library, so we should not
declare any such names ourselves. Use "libxeve_" or something.
> +/**
> + * Parse : separated list of key=value parameters
> + *
> + * @param[in] avctx context for logger
> + * @param[in] key
> + * @param[in] value
> + * @param[out] cdsc contains all Xeve MPEG-5 EVC encoder encoder parameters
> that
> + * should be initialized before the encoder is use
> + *
> + * @return 0 on success, negative value on failure
> + */
> +static int parse_xeve_params(AVCodecContext *avctx, const char *key, const
> char *value, XEVE_CDSC* cdsc)
What is the point of parsing options here instead of declaring them as
AVOptions? Especially when vbv-bufsize can already be set using the
global "bufsize" option.
> + xeve_color_fmt(avctx->pix_fmt, &xectx->color_format);
> +
> +#if AV_HAVE_BIGENDIAN
> + cdsc->param.cs = XEVE_CS_SET(xectx->color_format,
> cdsc->param.codec_bit_depth, 1);
> +#else
> + cdsc->param.cs = XEVE_CS_SET(xectx->color_format,
> cdsc->param.codec_bit_depth, 0);
> +#endif
just pass AV_HAVE_BIGENDIAN as the last macro parameter, no need for
#ifs
> +static int set_extra_config(AVCodecContext* avctx, XEVE id, XeveContext *ctx)
> +{
> + int ret, size, value;
> + size = 4;
> +
> + // embed SEI messages identifying encoder parameters and command line
> arguments
> + // - 0: off\n"
> + // - 1: emit sei info"
> + //
> + // SEI - Supplemental enhancement information contains information
> + // that is not necessary to decode the samples of coded pictures from
> VCL NAL units.
> + // Some SEI message information is required to check bitstream
> conformance
> + // and for output timing decoder conformance.
> + // @see ISO_IEC_23094-1_2020 7.4.3.5
> + // @see ISO_IEC_23094-1_2020 Annex D
> + ret = xeve_config(id, XEVE_CFG_SET_SEI_CMD, &value, &size); //
> sei_cmd_info
Is this not reading value, which is uninitialized?
> +/**
> + * Encode raw data frame into EVC packet
> + *
> + * @param[in] avctx codec context
> + * @param[out] pkt output AVPacket containing encoded data
> + * @param[in] frame AVFrame containing the raw data to be encoded
> + * @param[out] got_packet encoder sets to 0 or 1 to indicate that a
> + * non-empty packet was returned in pkt
> + *
> + * @return 0 on success, negative error code on failure
> + */
> +static int libxeve_encode(AVCodecContext *avctx, AVPacket *avpkt,
> + const AVFrame *frame, int *got_packet)
> +{
> + XeveContext *xectx = avctx->priv_data;
> + int ret = -1;
> + int xeve_cs;
> +
> + if(xectx->state == STATE_SKIPPING && frame ) {
> + xectx->state = STATE_ENCODING; // Entering encoding process
> + } else if(xectx->state == STATE_ENCODING && frame == NULL) {
This state switching looks wrong.
frame will be NULL only at the end of encoding. It should never happen
that libxeve_encode() is called with frame==NULL and then later with
frame!=NULL.
Also, the logic might be simpler if you used the receive_packet callback
rather than encode.
> + if (setup_bumping(xectx->id) == 0)
> + xectx->state = STATE_BUMPING; // Entering bumping process
> + else {
> + av_log(avctx, AV_LOG_ERROR, "Failed to setup bumping\n");
> + xectx->state = STATE_SKIPPING;
> + }
> + }
> +
> + if(xectx->state == STATE_ENCODING) {
> + const AVPixFmtDescriptor *pixel_fmt_desc = av_pix_fmt_desc_get
> (frame->format);
> + if(!pixel_fmt_desc) {
> + av_log(avctx, AV_LOG_ERROR, "Invalid pixel format descriptor for
> pixel format: %s\n", av_get_pix_fmt_name(avctx->pix_fmt));
> + return AVERROR_INVALIDDATA;
> + }
This seems unused.
> +
> + xeve_cs = xeve_color_space(avctx->pix_fmt);
> + if(xeve_cs != XEVE_CS_YCBCR420 && xeve_cs != XEVE_CS_YCBCR420_10LE) {
> + av_log(avctx, AV_LOG_ERROR, "Invalid pixel format: %s\n",
> av_get_pix_fmt_name(avctx->pix_fmt));
> + return AVERROR_INVALIDDATA;
> + }
This should be done once during encoder init.
Also declaring a list of supported pixel formats in the FFCodec struct
guarantees that you will only get one of those formats, so you don't
need to check for it yourself.
> +
> + {
> + int i;
> + XEVE_IMGB *imgb = NULL;
> +
> + imgb = &xectx->imgb;
> +
> + for (i = 0; i < imgb->np; i++) {
> + imgb->a[i] = frame->data[i];
> + imgb->s[i] = frame->linesize[i];
> + }
How does the ownership semantics work here? Does libxeve make a copy?
> + if(xectx->id == NULL) {
> + av_log(avctx, AV_LOG_ERROR, "Invalid XEVE encoder\n");
> + return AVERROR_INVALIDDATA;
> + }
Can this happen?
> +
> + imgb->ts[0] = frame->pts;
> + imgb->ts[1] = 0;
Will a proper dts value be generated by the library?
--
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
