Re: [FFmpeg-devel] [PATCH 2/3] Provided support for MPEG-5 EVC (Essential Video Coding) codec

[Anton Khirnov <anton@khirnov.net>]

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.kozinski@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-request@ffmpeg.org with subject "unsubscribe".