[FFmpeg-devel,v5] avcodec: Add librav1e encoder

Port to the new send/receive API by: James Almer <jamrial@gmail.com>.

Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
---
rav1e now has a release, and is committed to proper semver for its soname:
    https://github.com/xiph/rav1e/releases/tag/0.1.0

* All problems and nits form v4 have been addressed.
* Default mode is now QP 100 to match its CLI.
---
 configure              |   5 +
 doc/encoders.texi      |  37 +++
 doc/general.texi       |   7 +
 libavcodec/Makefile    |   1 +
 libavcodec/allcodecs.c |   1 +
 libavcodec/librav1e.c  | 597 +++++++++++++++++++++++++++++++++++++++++
 6 files changed, 648 insertions(+)
 create mode 100644 libavcodec/librav1e.c

On 11/9/2019 2:06 PM, Derek Buitenhuis wrote:
> Port to the new send/receive API by: James Almer <jamrial@gmail.com>.
> 
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> ---
> rav1e now has a release, and is committed to proper semver for its soname:
>     https://github.com/xiph/rav1e/releases/tag/0.1.0
> 
> * All problems and nits form v4 have been addressed.
> * Default mode is now QP 100 to match its CLI.
> ---
>  configure              |   5 +
>  doc/encoders.texi      |  37 +++
>  doc/general.texi       |   7 +
>  libavcodec/Makefile    |   1 +
>  libavcodec/allcodecs.c |   1 +
>  libavcodec/librav1e.c  | 597 +++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 648 insertions(+)
>  create mode 100644 libavcodec/librav1e.c
> 
> diff --git a/configure b/configure
> index 48e1426013..1de90e93fd 100755
> --- a/configure
> +++ b/configure
> @@ -254,6 +254,7 @@ External library support:
>    --enable-libopenmpt      enable decoding tracked files via libopenmpt [no]
>    --enable-libopus         enable Opus de/encoding via libopus [no]
>    --enable-libpulse        enable Pulseaudio input via libpulse [no]
> +  --enable-librav1e        enable AV1 encoding via rav1e [no]
>    --enable-librsvg         enable SVG rasterization via librsvg [no]
>    --enable-librubberband   enable rubberband needed for rubberband filter [no]
>    --enable-librtmp         enable RTMP[E] support via librtmp [no]
> @@ -1784,6 +1785,7 @@ EXTERNAL_LIBRARY_LIST="
>      libopenmpt
>      libopus
>      libpulse
> +    librav1e
>      librsvg
>      librtmp
>      libshine
> @@ -3203,6 +3205,8 @@ libopenmpt_demuxer_deps="libopenmpt"
>  libopus_decoder_deps="libopus"
>  libopus_encoder_deps="libopus"
>  libopus_encoder_select="audio_frame_queue"
> +librav1e_encoder_deps="librav1e"
> +librav1e_encoder_select="extract_extradata_bsf"
>  librsvg_decoder_deps="librsvg"
>  libshine_encoder_deps="libshine"
>  libshine_encoder_select="audio_frame_queue"
> @@ -6287,6 +6291,7 @@ enabled libopus           && {
>      }
>  }
>  enabled libpulse          && require_pkg_config libpulse libpulse pulse/pulseaudio.h pa_context_new
> +enabled librav1e          && require_pkg_config librav1e "rav1e >= 0.1.0" rav1e.h rav1e_context_new
>  enabled librsvg           && require_pkg_config librsvg librsvg-2.0 librsvg-2.0/librsvg/rsvg.h rsvg_handle_render_cairo
>  enabled librtmp           && require_pkg_config librtmp librtmp librtmp/rtmp.h RTMP_Socket
>  enabled librubberband     && require_pkg_config librubberband "rubberband >= 1.8.1" rubberband/rubberband-c.h rubberband_new -lstdc++ && append librubberband_extralibs "-lstdc++"
> diff --git a/doc/encoders.texi b/doc/encoders.texi
> index eefd124751..922f54df5f 100644
> --- a/doc/encoders.texi
> +++ b/doc/encoders.texi
> @@ -1378,6 +1378,43 @@ makes it possible to store non-rgb pix_fmts.
>  
>  @end table
>  
> +@section librav1e
> +
> +rav1e AV1 encoder wrapper.
> +
> +Requires the presence of the rav1e headers and library during configuration.
> +You need to explicitly configure the build with @code{--enable-librav1e}.
> +
> +@subsection Options
> +
> +@table @option
> +@item qmax
> +Sets the maximum quantizer to use when using bitrate mode.
> +
> +@item qmin
> +Sets the minimum quantizer to use when using bitrate mode.
> +
> +@item qp
> +Uses quantizer mode to encode at the given quantizer.
> +
> +@item speed
> +Selects the speed preset (0-10) to encode with.
> +
> +@item tiles
> +Selects how many tiles to encode with.
> +
> +@item rav1e-params
> +Set rav1e options using a list of @var{key}=@var{value} pairs separated
> +by ":". See @command{rav1e --help} for a list of options.
> +
> +For example to specify librav1e encoding options with @option{-rav1e-params}:
> +
> +@example
> +ffmpeg -i input -c:v librav1e -b:v 500K -rav1e-params speed=5:low_latency=true output.mp4
> +@end example
> +
> +@end table
> +
>  @section libaom-av1
>  
>  libaom AV1 encoder wrapper.
> diff --git a/doc/general.texi b/doc/general.texi
> index 79a23e1718..a5b77e0de1 100644
> --- a/doc/general.texi
> +++ b/doc/general.texi
> @@ -253,6 +253,13 @@ FFmpeg can use the OpenJPEG libraries for decoding/encoding J2K videos.  Go to
>  instructions.  To enable using OpenJPEG in FFmpeg, pass @code{--enable-libopenjpeg} to
>  @file{./configure}.
>  
> +@section rav1e
> +
> +FFmpeg can make use of rav1e (Rust AV1 Encoder) via its C bindings to encode videos.
> +Go to @url{https://github.com/xiph/rav1e/} and follow the instructions to build
> +the C library. To enable using rav1e in FFmpeg, pass @code{--enable-librav1e}
> +to @file{./configure}.
> +
>  @section TwoLAME
>  
>  FFmpeg can make use of the TwoLAME library for MP2 encoding.
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index eee1e505f9..b990c6ba87 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -993,6 +993,7 @@ OBJS-$(CONFIG_LIBOPUS_DECODER)            += libopusdec.o libopus.o     \
>                                               vorbis_data.o
>  OBJS-$(CONFIG_LIBOPUS_ENCODER)            += libopusenc.o libopus.o     \
>                                               vorbis_data.o
> +OBJS-$(CONFIG_LIBRAV1E_ENCODER)           += librav1e.o
>  OBJS-$(CONFIG_LIBSHINE_ENCODER)           += libshine.o
>  OBJS-$(CONFIG_LIBSPEEX_DECODER)           += libspeexdec.o
>  OBJS-$(CONFIG_LIBSPEEX_ENCODER)           += libspeexenc.o
> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> index 23a778e041..0c0741936c 100644
> --- a/libavcodec/allcodecs.c
> +++ b/libavcodec/allcodecs.c
> @@ -704,6 +704,7 @@ extern AVCodec ff_libopenjpeg_encoder;
>  extern AVCodec ff_libopenjpeg_decoder;
>  extern AVCodec ff_libopus_encoder;
>  extern AVCodec ff_libopus_decoder;
> +extern AVCodec ff_librav1e_encoder;
>  extern AVCodec ff_librsvg_decoder;
>  extern AVCodec ff_libshine_encoder;
>  extern AVCodec ff_libspeex_encoder;
> diff --git a/libavcodec/librav1e.c b/libavcodec/librav1e.c
> new file mode 100644
> index 0000000000..6085db7297
> --- /dev/null
> +++ b/libavcodec/librav1e.c
> @@ -0,0 +1,597 @@
> +/*
> + * librav1e encoder
> + *
> + * Copyright (c) 2019 Derek Buitenhuis
> + *
> + * 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
> + */
> +
> +#include <rav1e.h>
> +
> +#include "libavutil/internal.h"
> +#include "libavutil/avassert.h"
> +#include "libavutil/base64.h"
> +#include "libavutil/common.h"
> +#include "libavutil/mathematics.h"
> +#include "libavutil/opt.h"
> +#include "libavutil/pixdesc.h"
> +#include "avcodec.h"
> +#include "internal.h"
> +
> +typedef struct librav1eContext {
> +    const AVClass *class;
> +
> +    RaContext *ctx;
> +    AVBSFContext *bsf;
> +
> +    uint8_t *pass_data;
> +    size_t pass_pos;
> +    int pass_size;
> +
> +    char *rav1e_opts;
> +    int quantizer;
> +    int speed;
> +    int tiles;
> +    int tile_rows;
> +    int tile_cols;
> +} librav1eContext;
> +
> +static inline RaPixelRange range_map(enum AVPixelFormat pix_fmt, enum AVColorRange range)
> +{
> +    switch (pix_fmt) {
> +    case AV_PIX_FMT_YUVJ420P:
> +    case AV_PIX_FMT_YUVJ422P:
> +    case AV_PIX_FMT_YUVJ444P:
> +        return RA_PIXEL_RANGE_FULL;
> +    }
> +
> +    switch (range) {
> +    case AVCOL_RANGE_JPEG:
> +        return RA_PIXEL_RANGE_FULL;
> +    case AVCOL_RANGE_MPEG:
> +    default:
> +        return RA_PIXEL_RANGE_LIMITED;
> +    }
> +}
> +
> +static inline RaChromaSampling pix_fmt_map(enum AVPixelFormat pix_fmt)
> +{
> +    switch (pix_fmt) {
> +    case AV_PIX_FMT_YUV420P:
> +    case AV_PIX_FMT_YUVJ420P:
> +    case AV_PIX_FMT_YUV420P10:
> +    case AV_PIX_FMT_YUV420P12:
> +        return RA_CHROMA_SAMPLING_CS420;
> +    case AV_PIX_FMT_YUV422P:
> +    case AV_PIX_FMT_YUVJ422P:
> +    case AV_PIX_FMT_YUV422P10:
> +    case AV_PIX_FMT_YUV422P12:
> +        return RA_CHROMA_SAMPLING_CS422;
> +    case AV_PIX_FMT_YUV444P:
> +    case AV_PIX_FMT_YUVJ444P:
> +    case AV_PIX_FMT_YUV444P10:
> +    case AV_PIX_FMT_YUV444P12:
> +        return RA_CHROMA_SAMPLING_CS444;
> +    default:
> +        av_assert0(0);
> +    }
> +}
> +
> +static inline RaChromaSamplePosition chroma_loc_map(enum AVChromaLocation chroma_loc)
> +{
> +    switch (chroma_loc) {
> +    case AVCHROMA_LOC_LEFT:
> +        return RA_CHROMA_SAMPLE_POSITION_VERTICAL;
> +    case AVCHROMA_LOC_TOPLEFT:
> +        return RA_CHROMA_SAMPLE_POSITION_COLOCATED;
> +    default:
> +        return RA_CHROMA_SAMPLE_POSITION_UNKNOWN;
> +    }
> +}
> +
> +static int get_stats(AVCodecContext *avctx, int eos)
> +{
> +    librav1eContext *ctx = avctx->priv_data;
> +    RaData* buf = rav1e_twopass_out(ctx->ctx);
> +    if (!buf)
> +        return 0;
> +
> +    if (!eos) {
> +        uint8_t *tmp = av_fast_realloc(ctx->pass_data, &ctx->pass_size,
> +                                      ctx->pass_pos + buf->len);
> +        if (!tmp) {
> +            rav1e_data_unref(buf);
> +            return AVERROR(ENOMEM);
> +        }
> +
> +        ctx->pass_data = tmp;
> +        memcpy(ctx->pass_data + ctx->pass_pos, buf->data, buf->len);
> +        ctx->pass_pos += buf->len;
> +    } else {
> +        size_t b64_size = AV_BASE64_SIZE(ctx->pass_pos);
> +
> +        memcpy(ctx->pass_data, buf->data, buf->len);
> +
> +        avctx->stats_out = av_malloc(b64_size);
> +        if (!avctx->stats_out) {
> +            rav1e_data_unref(buf);
> +            return AVERROR(ENOMEM);
> +        }
> +
> +        av_base64_encode(avctx->stats_out, b64_size, ctx->pass_data, ctx->pass_pos);
> +
> +        av_freep(&ctx->pass_data);
> +    }
> +
> +    rav1e_data_unref(buf);
> +
> +    return 0;
> +}
> +
> +static int set_stats(AVCodecContext *avctx)
> +{
> +    librav1eContext *ctx = avctx->priv_data;
> +    int ret = 1;
> +
> +    while (ret > 0 && ctx->pass_size - ctx->pass_pos > 0) {
> +        ret = rav1e_twopass_in(ctx->ctx, ctx->pass_data + ctx->pass_pos, ctx->pass_size);
> +        if (ret < 0)
> +            return AVERROR_EXTERNAL;
> +        ctx->pass_pos += ret;
> +    }
> +
> +    return 0;
> +}
> +
> +static av_cold int librav1e_encode_close(AVCodecContext *avctx)
> +{
> +    librav1eContext *ctx = avctx->priv_data;
> +
> +    if (ctx->ctx) {
> +        rav1e_context_unref(ctx->ctx);
> +        ctx->ctx = NULL;
> +    }
> +
> +    av_bsf_free(&ctx->bsf);
> +    av_freep(&ctx->pass_data);
> +
> +    return 0;
> +}
> +
> +static av_cold int librav1e_encode_init(AVCodecContext *avctx)
> +{
> +    librav1eContext *ctx = avctx->priv_data;
> +    const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(avctx->pix_fmt);
> +    RaConfig *cfg = NULL;
> +    int rret;
> +    int ret = 0;
> +
> +    cfg = rav1e_config_default();
> +    if (!cfg) {
> +        av_log(avctx, AV_LOG_ERROR, "Could not allocate rav1e config.\n");
> +        return AVERROR_EXTERNAL;
> +    }
> +
> +    rav1e_config_set_time_base(cfg, (RaRational) {
> +                               avctx->time_base.num * avctx->ticks_per_frame,
> +                               avctx->time_base.den
> +                               });
> +
> +    if (avctx->flags & AV_CODEC_FLAG_PASS2) {
> +        if (!avctx->stats_in) {
> +            av_log(avctx, AV_LOG_ERROR, "No stats file provided for second pass.\n");
> +            ret = AVERROR(EINVAL);
> +            goto end;
> +        }
> +
> +        ctx->pass_size = (strlen(avctx->stats_in) * 3) / 4;
> +        ctx->pass_data = av_malloc(ctx->pass_size);
> +        if (!ctx->pass_data) {
> +            av_log(avctx, AV_LOG_ERROR, "Could not allocate stats buffer.\n");
> +            ret = AVERROR(ENOMEM);
> +            goto end;
> +        }
> +
> +        ctx->pass_size = av_base64_decode(ctx->pass_data, avctx->stats_in, ctx->pass_size);
> +        if (ctx->pass_size < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Invalid pass file.\n");
> +            ret = AVERROR(EINVAL);
> +            goto end;
> +        }
> +    }
> +
> +    if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) {
> +         const AVBitStreamFilter *filter = av_bsf_get_by_name("extract_extradata");
> +         int bret;
> +
> +         if (!filter) {
> +            av_log(avctx, AV_LOG_ERROR, "extract_extradata bitstream filter "
> +                   "not found. This is a bug, please report it.\n");
> +            ret = AVERROR_BUG;
> +            goto end;
> +         }
> +
> +         bret = av_bsf_alloc(filter, &ctx->bsf);
> +         if (bret < 0) {
> +             ret = bret;
> +             goto end;
> +         }
> +
> +         bret = avcodec_parameters_from_context(ctx->bsf->par_in, avctx);
> +         if (bret < 0) {
> +             ret = bret;
> +             goto end;
> +         }
> +
> +         bret = av_bsf_init(ctx->bsf);
> +         if (bret < 0) {
> +             ret = bret;
> +             goto end;
> +         }
> +    }
> +
> +    if (ctx->rav1e_opts) {
> +        AVDictionary *dict    = NULL;
> +        AVDictionaryEntry *en = NULL;
> +
> +        if (!av_dict_parse_string(&dict, ctx->rav1e_opts, "=", ":", 0)) {
> +            while (en = av_dict_get(dict, "", en, AV_DICT_IGNORE_SUFFIX)) {
> +                int parse_ret = rav1e_config_parse(cfg, en->key, en->value);
> +                if (parse_ret < 0)
> +                    av_log(avctx, AV_LOG_WARNING, "Invalid value for %s: %s.\n", en->key, en->value);
> +            }
> +            av_dict_free(&dict);
> +        }
> +    }
> +
> +    rret = rav1e_config_parse_int(cfg, "width", avctx->width);
> +    if (rret < 0) {
> +        av_log(avctx, AV_LOG_ERROR, "Invalid width passed to rav1e.\n");
> +        ret = AVERROR_INVALIDDATA;
> +        goto end;
> +    }
> +
> +    rret = rav1e_config_parse_int(cfg, "height", avctx->height);
> +    if (rret < 0) {
> +        av_log(avctx, AV_LOG_ERROR, "Invalid height passed to rav1e.\n");
> +        ret = AVERROR_INVALIDDATA;
> +        goto end;
> +    }
> +
> +    rret = rav1e_config_parse_int(cfg, "threads", avctx->thread_count);
> +    if (rret < 0)
> +        av_log(avctx, AV_LOG_WARNING, "Invalid number of threads, defaulting to auto.\n");
> +
> +    if (ctx->speed >= 0) {
> +        rret = rav1e_config_parse_int(cfg, "speed", ctx->speed);
> +        if (rret < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Could not set speed preset.\n");
> +            ret = AVERROR_EXTERNAL;
> +            goto end;
> +        }
> +    }
> +
> +    /* rav1e handles precedence between 'tiles' and cols/rows for us. */
> +    if (ctx->tiles >= 0) {
> +        rret = rav1e_config_parse_int(cfg, "tiles", ctx->tiles);
> +        if (rret < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Could not set number of tiles to encode with.\n");
> +            ret = AVERROR_EXTERNAL;
> +            goto end;
> +        }
> +    }
> +    if (ctx->tile_rows >= 0) {

Since these are no longer log2 values, does rav1e change 0 to 1 internally?
It may be a better idea to make 0 the default, and only call
rav1e_config_parse_int() if it's > 0.

> +        rret = rav1e_config_parse_int(cfg, "tile_rows", ctx->tile_rows);
> +        if (rret < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Could not set number of tile rows to encode with.\n");
> +            ret = AVERROR_EXTERNAL;
> +            goto end;
> +        }
> +    }
> +    if (ctx->tile_cols >= 0) {

Ditto.

> +        rret = rav1e_config_parse_int(cfg, "tile_cols_log2", ctx->tile_cols);

Should be "tile_cols".

> +        if (rret < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Could not set number of tile cols to encode with.\n");
> +            ret = AVERROR_EXTERNAL;
> +            goto end;
> +        }
> +    }
> +
> +    if (avctx->gop_size > 0) {
> +        rret = rav1e_config_parse_int(cfg, "key_frame_interval", avctx->gop_size);
> +        if (rret < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Could not set max keyint.\n");
> +            ret = AVERROR_EXTERNAL;
> +            goto end;
> +        }
> +    }
> +
> +    if (avctx->keyint_min > 0) {
> +        rret = rav1e_config_parse_int(cfg, "min_key_frame_interval", avctx->keyint_min);
> +        if (rret < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Could not set min keyint.\n");
> +            ret = AVERROR_EXTERNAL;
> +            goto end;
> +        }
> +    }
> +
> +    if (avctx->bit_rate && ctx->quantizer < 0) {
> +        int max_quantizer = avctx->qmax >= 0 ? avctx->qmax : 255;
> +
> +        rret = rav1e_config_parse_int(cfg, "quantizer", max_quantizer);
> +        if (rret < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Could not set max quantizer.\n");
> +            ret = AVERROR_EXTERNAL;
> +            goto end;
> +        }
> +
> +        if (avctx->qmin >= 0) {
> +            rret = rav1e_config_parse_int(cfg, "min_quantizer", avctx->qmin);
> +            if (rret < 0) {
> +                av_log(avctx, AV_LOG_ERROR, "Could not set min quantizer.\n");
> +                ret = AVERROR_EXTERNAL;
> +                goto end;
> +            }
> +        }
> +
> +        rret = rav1e_config_parse_int(cfg, "bitrate", avctx->bit_rate);
> +        if (rret < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Could not set bitrate.\n");
> +            ret = AVERROR_INVALIDDATA;
> +            goto end;
> +        }
> +    } else if (ctx->quantizer >= 0) {

Bitrate will be ignored if set. Maybe the doxy could mention it, or a
log message printed here to let the user know about it.

> +        rret = rav1e_config_parse_int(cfg, "quantizer", ctx->quantizer);
> +        if (rret < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "Could not set quantizer.\n");
> +            ret = AVERROR_EXTERNAL;
> +            goto end;
> +        }
> +    }
> +
> +    rret = rav1e_config_set_pixel_format(cfg, desc->comp[0].depth,
> +                                         pix_fmt_map(avctx->pix_fmt),
> +                                         chroma_loc_map(avctx->chroma_sample_location),
> +                                         range_map(avctx->pix_fmt, avctx->color_range));
> +    if (rret < 0) {
> +        av_log(avctx, AV_LOG_ERROR, "Failed to set pixel format properties.\n");
> +        ret = AVERROR_INVALIDDATA;
> +        goto end;
> +    }
> +
> +    /* rav1e's colorspace enums match standard values. */
> +    rret = rav1e_config_set_color_description(cfg, (RaMatrixCoefficients) avctx->colorspace,
> +                                              (RaColorPrimaries) avctx->color_primaries,
> +                                              (RaTransferCharacteristics) avctx->color_trc);
> +    if (rret < 0) {
> +        av_log(avctx, AV_LOG_WARNING, "Failed to set color properties.\n");
> +        if (avctx->err_recognition & AV_EF_EXPLODE) {
> +            ret = AVERROR_INVALIDDATA;
> +            goto end;
> +        }
> +    }
> +
> +    ctx->ctx = rav1e_context_new(cfg);
> +    if (!ctx->ctx) {
> +        av_log(avctx, AV_LOG_ERROR, "Failed to create rav1e encode context.\n");
> +        ret = AVERROR_EXTERNAL;
> +        goto end;
> +    }
> +
> +    ret = 0;
> +
> +end:
> +
> +    rav1e_config_unref(cfg);
> +
> +    return ret;
> +}
> +
> +static int librav1e_send_frame(AVCodecContext *avctx, const AVFrame *frame)
> +{
> +    librav1eContext *ctx = avctx->priv_data;
> +    RaFrame *rframe = NULL;
> +    int ret;
> +
> +    if (frame) {
> +        const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(frame->format);
> +
> +        rframe = rav1e_frame_new(ctx->ctx);
> +        if (!rframe) {
> +            av_log(avctx, AV_LOG_ERROR, "Could not allocate new rav1e frame.\n");
> +            return AVERROR(ENOMEM);
> +        }
> +
> +        for (int i = 0; i < desc->nb_components; i++) {
> +            int shift = i ? desc->log2_chroma_h : 0;
> +            int bytes = desc->comp[0].depth == 8 ? 1 : 2;
> +            rav1e_frame_fill_plane(rframe, i, frame->data[i],
> +                                   (frame->height >> shift) * frame->linesize[i],
> +                                   frame->linesize[i], bytes);
> +        }
> +    }
> +
> +    ret = rav1e_send_frame(ctx->ctx, rframe);
> +    if (rframe)
> +         rav1e_frame_unref(rframe); /* No need to unref if flushing. */
> +
> +    switch (ret) {
> +    case RA_ENCODER_STATUS_SUCCESS:
> +        break;
> +    case RA_ENCODER_STATUS_ENOUGH_DATA:
> +        return AVERROR(EAGAIN);
> +    case RA_ENCODER_STATUS_FAILURE:
> +        av_log(avctx, AV_LOG_ERROR, "Could not send frame.\n");
> +        return AVERROR_EXTERNAL;
> +    default:
> +        av_log(avctx, AV_LOG_ERROR, "Unknown return code %d from rav1e_send_frame.\n", ret);
> +        return AVERROR_UNKNOWN;
> +    }

You could use rav1e_status_to_str() to get the error string and print it
for the STATUS_FAILURE and default cases.

Something like

    switch (ret) {
    case RA_ENCODER_STATUS_SUCCESS:
        break;
    case RA_ENCODER_STATUS_ENOUGH_DATA:
        return AVERROR(EAGAIN);
    case RA_ENCODER_STATUS_FAILURE:
        ret = AVERROR_EXTERNAL;
        break;
    default:
        ret = AVERROR_UNKNOWN;
        break;
    }

    if (ret < 0)
        av_log(avctx, AV_LOG_ERROR, "rav1e_send_frame(): %s\n",
rav1e_status_to_str(ret));

    return ret;

> +
> +    return 0;
> +}
> +
> +static void librav1e_packet_unref(void *opaque, uint8_t *data)
> +{
> +    RaPacket *rpkt = opaque;
> +
> +    rav1e_packet_unref(rpkt);
> +}
> +
> +static int librav1e_receive_packet(AVCodecContext *avctx, AVPacket *pkt)
> +{
> +    librav1eContext *ctx = avctx->priv_data;
> +    RaPacket *rpkt = NULL;
> +    int ret;
> +
> +retry:
> +
> +    if (avctx->flags & AV_CODEC_FLAG_PASS1) {
> +        int sret = get_stats(avctx, 0);
> +        if (sret < 0)
> +            return sret;
> +    } else if (avctx->flags & AV_CODEC_FLAG_PASS2) {
> +        int sret = set_stats(avctx);
> +        if (sret < 0)
> +            return sret;
> +    }
> +
> +    ret = rav1e_receive_packet(ctx->ctx, &rpkt);
> +    switch (ret) {
> +    case RA_ENCODER_STATUS_SUCCESS:
> +        break;
> +    case RA_ENCODER_STATUS_LIMIT_REACHED:
> +        if (avctx->flags & AV_CODEC_FLAG_PASS1) {
> +            int sret = get_stats(avctx, 1);
> +            if (sret < 0)
> +                return sret;
> +        }
> +        return AVERROR_EOF;
> +    case RA_ENCODER_STATUS_ENCODED:
> +        if (avctx->internal->draining)
> +            goto retry;
> +        return AVERROR(EAGAIN);
> +    case RA_ENCODER_STATUS_NEED_MORE_DATA:
> +        if (avctx->internal->draining) {
> +            av_log(avctx, AV_LOG_ERROR, "Unexpected error when receiving packet after EOF.\n");
> +            return AVERROR_EXTERNAL;
> +        }
> +        return AVERROR(EAGAIN);
> +    case RA_ENCODER_STATUS_FAILURE:
> +        av_log(avctx, AV_LOG_ERROR, "Could not encode frame.\n");
> +        return AVERROR_EXTERNAL;
> +    default:
> +        av_log(avctx, AV_LOG_ERROR, "Unknown return code %d from rav1e_receive_packet.\n", ret);
> +        return AVERROR_UNKNOWN;
> +    }

Ditto here. Only the custom NEED_MORE_DATA message printed while in
draining mode is worth keeping.

> +
> +    pkt->buf = av_buffer_create((uint8_t *) rpkt->data, rpkt->len, librav1e_packet_unref,
> +                                rpkt, AV_BUFFER_FLAG_READONLY);

When i came up with this zero-copy method i didn't realize that rav1e
may not be padding the buffer in question. If the padding is not at
least AV_INPUT_BUFFER_PADDING_SIZE big, then it's technically breaking
the AVPacket API, and we may have to use av_new_packet() and copy the
buffer instead.

> +    if (!pkt->buf) {
> +        rav1e_packet_unref(rpkt);
> +        return AVERROR(ENOMEM);
> +    }
> +
> +    pkt->data = (uint8_t *) rpkt->data;
> +    pkt->size = rpkt->len;
> +
> +    if (rpkt->frame_type == RA_FRAME_TYPE_KEY)
> +        pkt->flags |= AV_PKT_FLAG_KEY;
> +
> +    pkt->pts = pkt->dts = rpkt->input_frameno * avctx->ticks_per_frame;
> +
> +    if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER) {
> +        int ret = av_bsf_send_packet(ctx->bsf, pkt);
> +        if (ret < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "extradata extraction send failed.\n");
> +            av_packet_unref(pkt);
> +            return ret;
> +        }
> +
> +        ret = av_bsf_receive_packet(ctx->bsf, pkt);
> +        if (ret < 0) {
> +            av_log(avctx, AV_LOG_ERROR, "extradata extraction receive failed.\n");
> +            av_packet_unref(pkt);
> +            return ret;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +#define OFFSET(x) offsetof(librav1eContext, x)
> +#define VE AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_ENCODING_PARAM
> +
> +static const AVOption options[] = {
> +    { "qp", "use constant quantizer mode", OFFSET(quantizer), AV_OPT_TYPE_INT, { .i64 = 100 }, -1, 255, VE },
> +    { "speed", "what speed preset to use", OFFSET(speed), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, 10, VE },
> +    { "tiles", "number of tiles encode with", OFFSET(tiles), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, INT64_MAX, VE },
> +    { "tile-rows", "number of tiles rows to encode with", OFFSET(tile_rows), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, INT64_MAX, VE },
> +    { "tile-columns", "number of tiles columns to encode with", OFFSET(tile_cols), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, INT64_MAX, VE },

These two are not documented.

> +    { "rav1e-params", "set the rav1e configuration using a :-separated list of key=value parameters", OFFSET(rav1e_opts), AV_OPT_TYPE_STRING, { 0 }, 0, 0, VE },
> +    { NULL }
> +};
> +
> +static const AVCodecDefault librav1e_defaults[] = {
> +    { "b",           "0" },
> +    { "g",           "0" },
> +    { "keyint_min",  "0" },
> +    { "qmax",       "-1" },
> +    { "qmin",       "-1" },
> +    { NULL }
> +};
> +
> +const enum AVPixelFormat librav1e_pix_fmts[] = {
> +    AV_PIX_FMT_YUV420P,
> +    AV_PIX_FMT_YUVJ420P,
> +    AV_PIX_FMT_YUV420P10,
> +    AV_PIX_FMT_YUV420P12,
> +    AV_PIX_FMT_YUV422P,
> +    AV_PIX_FMT_YUVJ422P,
> +    AV_PIX_FMT_YUV422P10,
> +    AV_PIX_FMT_YUV422P12,
> +    AV_PIX_FMT_YUV444P,
> +    AV_PIX_FMT_YUVJ444P,
> +    AV_PIX_FMT_YUV444P10,
> +    AV_PIX_FMT_YUV444P12,
> +    AV_PIX_FMT_NONE
> +};
> +
> +static const AVClass class = {
> +    .class_name = "librav1e",
> +    .item_name  = av_default_item_name,
> +    .option     = options,
> +    .version    = LIBAVUTIL_VERSION_INT,
> +};
> +
> +AVCodec ff_librav1e_encoder = {
> +    .name           = "librav1e",
> +    .long_name      = NULL_IF_CONFIG_SMALL("librav1e AV1"),
> +    .type           = AVMEDIA_TYPE_VIDEO,
> +    .id             = AV_CODEC_ID_AV1,
> +    .init           = librav1e_encode_init,
> +    .send_frame     = librav1e_send_frame,
> +    .receive_packet = librav1e_receive_packet,
> +    .close          = librav1e_encode_close,
> +    .priv_data_size = sizeof(librav1eContext),
> +    .priv_class     = &class,
> +    .defaults       = librav1e_defaults,
> +    .pix_fmts       = librav1e_pix_fmts,
> +    .capabilities   = AV_CODEC_CAP_DELAY | AV_CODEC_CAP_AUTO_THREADS,
> +    .caps_internal  = FF_CODEC_CAP_INIT_CLEANUP,
> +    .wrapper_name   = "librav1e",
> +};
>

On 09/11/2019 18:03, James Almer wrote:
>> +    if (ctx->tile_rows >= 0) {
> 
> Since these are no longer log2 values, does rav1e change 0 to 1 internally?
> It may be a better idea to make 0 the default, and only call
> rav1e_config_parse_int() if it's > 0.

Yes.

Changed to match this.

>> +    if (ctx->tile_cols >= 0) {
> 
> Ditto.

Fixed.

> 
>> +        rret = rav1e_config_parse_int(cfg, "tile_cols_log2", ctx->tile_cols);
> 
> Should be "tile_cols".

Fixed.

>> +        rret = rav1e_config_parse_int(cfg, "bitrate", avctx->bit_rate);
>> +        if (rret < 0) {
>> +            av_log(avctx, AV_LOG_ERROR, "Could not set bitrate.\n");
>> +            ret = AVERROR_INVALIDDATA;
>> +            goto end;
>> +        }
>> +    } else if (ctx->quantizer >= 0) {
> 
> Bitrate will be ignored if set. Maybe the doxy could mention it, or a
> log message printed here to let the user know about it.

I've added a warning if both are set.

>> +    switch (ret) {
>> +    case RA_ENCODER_STATUS_SUCCESS:
>> +        break;
>> +    case RA_ENCODER_STATUS_ENOUGH_DATA:
>> +        return AVERROR(EAGAIN);
>> +    case RA_ENCODER_STATUS_FAILURE:
>> +        av_log(avctx, AV_LOG_ERROR, "Could not send frame.\n");
>> +        return AVERROR_EXTERNAL;
>> +    default:
>> +        av_log(avctx, AV_LOG_ERROR, "Unknown return code %d from rav1e_send_frame.\n", ret);
>> +        return AVERROR_UNKNOWN;
>> +    }
> 
> You could use rav1e_status_to_str() to get the error string and print it
> for the STATUS_FAILURE and default cases.

Done, but I've kept it inside the switch.

> Ditto here. Only the custom NEED_MORE_DATA message printed while in
> draining mode is worth keeping.

Done.

>> +
>> +    pkt->buf = av_buffer_create((uint8_t *) rpkt->data, rpkt->len, librav1e_packet_unref,
>> +                                rpkt, AV_BUFFER_FLAG_READONLY);
> 
> When i came up with this zero-copy method i didn't realize that rav1e
> may not be padding the buffer in question. If the padding is not at
> least AV_INPUT_BUFFER_PADDING_SIZE big, then it's technically breaking
> the AVPacket API, and we may have to use av_new_packet() and copy the
> buffer instead.

I don't think we can guarantee AV_INPUT_BUFFER_PADDING_SIZE in rav1e's
API for packet data.

I also assume you meant ff_alloc_packet2(), and not av_new_packet().

I've converted it to that.

>> +    { "tile-rows", "number of tiles rows to encode with", OFFSET(tile_rows), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, INT64_MAX, VE },
>> +    { "tile-columns", "number of tiles columns to encode with", OFFSET(tile_cols), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, INT64_MAX, VE },
> 
> These two are not documented.

Fixed.

Thanks for the review! New patch sent.

- Derek

On 11/9/2019 6:15 PM, Derek Buitenhuis wrote:
> On 09/11/2019 18:03, James Almer wrote:
>>> +    if (ctx->tile_rows >= 0) {
>>
>> Since these are no longer log2 values, does rav1e change 0 to 1 internally?
>> It may be a better idea to make 0 the default, and only call
>> rav1e_config_parse_int() if it's > 0.
> 
> Yes.
> 
> Changed to match this.
> 
>>> +    if (ctx->tile_cols >= 0) {
>>
>> Ditto.
> 
> Fixed.
> 
>>
>>> +        rret = rav1e_config_parse_int(cfg, "tile_cols_log2", ctx->tile_cols);
>>
>> Should be "tile_cols".
> 
> Fixed.
> 
>>> +        rret = rav1e_config_parse_int(cfg, "bitrate", avctx->bit_rate);
>>> +        if (rret < 0) {
>>> +            av_log(avctx, AV_LOG_ERROR, "Could not set bitrate.\n");
>>> +            ret = AVERROR_INVALIDDATA;
>>> +            goto end;
>>> +        }
>>> +    } else if (ctx->quantizer >= 0) {
>>
>> Bitrate will be ignored if set. Maybe the doxy could mention it, or a
>> log message printed here to let the user know about it.
> 
> I've added a warning if both are set.
> 
>>> +    switch (ret) {
>>> +    case RA_ENCODER_STATUS_SUCCESS:
>>> +        break;
>>> +    case RA_ENCODER_STATUS_ENOUGH_DATA:
>>> +        return AVERROR(EAGAIN);
>>> +    case RA_ENCODER_STATUS_FAILURE:
>>> +        av_log(avctx, AV_LOG_ERROR, "Could not send frame.\n");
>>> +        return AVERROR_EXTERNAL;
>>> +    default:
>>> +        av_log(avctx, AV_LOG_ERROR, "Unknown return code %d from rav1e_send_frame.\n", ret);
>>> +        return AVERROR_UNKNOWN;
>>> +    }
>>
>> You could use rav1e_status_to_str() to get the error string and print it
>> for the STATUS_FAILURE and default cases.
> 
> Done, but I've kept it inside the switch.
> 
>> Ditto here. Only the custom NEED_MORE_DATA message printed while in
>> draining mode is worth keeping.
> 
> Done.
> 
>>> +
>>> +    pkt->buf = av_buffer_create((uint8_t *) rpkt->data, rpkt->len, librav1e_packet_unref,
>>> +                                rpkt, AV_BUFFER_FLAG_READONLY);
>>
>> When i came up with this zero-copy method i didn't realize that rav1e
>> may not be padding the buffer in question. If the padding is not at
>> least AV_INPUT_BUFFER_PADDING_SIZE big, then it's technically breaking
>> the AVPacket API, and we may have to use av_new_packet() and copy the
>> buffer instead.
> 
> I don't think we can guarantee AV_INPUT_BUFFER_PADDING_SIZE in rav1e's
> API for packet data.
> 
> I also assume you meant ff_alloc_packet2(), and not av_new_packet().

No, this encoder doesn't have an AVCodec->encode2() implementation, so
it can't be used with the avcodec_encode_video2() API, only with the
avcodec_send_frame()/avcodec_receive_packet() one, so no need to take
user provided packets into consideration since those are not an option.
If you use ff_alloc_packet2(), you'll be first copying the RaPacket to
some internal buffer, which will then be copied into a ref counted
buffer before being returned to the user.

You can safely use av_new_packet() to allocate the packet buffer, as the
AVPacket passed to AVCodec->receive_packet() will be freshly initialized
and empty.

> 
> I've converted it to that.
> 
>>> +    { "tile-rows", "number of tiles rows to encode with", OFFSET(tile_rows), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, INT64_MAX, VE },
>>> +    { "tile-columns", "number of tiles columns to encode with", OFFSET(tile_cols), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, INT64_MAX, VE },
>>
>> These two are not documented.
> 
> Fixed.
> 
> Thanks for the review! New patch sent.
> 
> - Derek
> _______________________________________________
> 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".
>

On 09/11/2019 21:47, James Almer wrote:
> No, this encoder doesn't have an AVCodec->encode2() implementation, so
> it can't be used with the avcodec_encode_video2() API, only with the
> avcodec_send_frame()/avcodec_receive_packet() one, so no need to take
> user provided packets into consideration since those are not an option.
> If you use ff_alloc_packet2(), you'll be first copying the RaPacket to
> some internal buffer, which will then be copied into a ref counted
> buffer before being returned to the user.
> 
> You can safely use av_new_packet() to allocate the packet buffer, as the
> AVPacket passed to AVCodec->receive_packet() will be freshly initialized
> and empty.

How "obvious"...

Anyway, simple fix. Update sent.

- Derek

On 11/9/2019 7:01 PM, Derek Buitenhuis wrote:
> On 09/11/2019 21:47, James Almer wrote:
>> No, this encoder doesn't have an AVCodec->encode2() implementation, so
>> it can't be used with the avcodec_encode_video2() API, only with the
>> avcodec_send_frame()/avcodec_receive_packet() one, so no need to take
>> user provided packets into consideration since those are not an option.
>> If you use ff_alloc_packet2(), you'll be first copying the RaPacket to
>> some internal buffer, which will then be copied into a ref counted
>> buffer before being returned to the user.
>>
>> You can safely use av_new_packet() to allocate the packet buffer, as the
>> AVPacket passed to AVCodec->receive_packet() will be freshly initialized
>> and empty.
> 
> How "obvious"...

Actually, i'm partly wrong, avcodec_receive_packet() is not ensuring the
packet returned by the encoder is reference counted. So for this version
it would give the user the output of ff_alloc_packet2() as is... Lovely.

Guess an av_assert0() is in order.

> 
> Anyway, simple fix. Update sent.
> 
> - Derek
> _______________________________________________
> 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".
>