[FFmpeg-devel] [PATCH v9 1/2] lavc/svt_hevc: add libsvt hevc encoder wrapper

[Vittorio Giovara]

On Tue, Mar 26, 2019 at 10:47 PM Jing Sun <jing.a.sun at intel.com> wrote:

> Signed-off-by: Zhengxu Huang <zhengxu.huang at intel.com>
> Signed-off-by: Hassene Tmar <hassene.tmar at intel.com>
> Signed-off-by: Jun Zhao <jun.zhao at intel.com>
> Signed-off-by: Jing Sun <jing.a.sun at intel.com>
> ---
>  configure                |   4 +
>  libavcodec/Makefile      |   1 +
>  libavcodec/allcodecs.c   |   1 +
>  libavcodec/libsvt_hevc.c | 502
> +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 508 insertions(+)
>  create mode 100644 libavcodec/libsvt_hevc.c
>
> +
> +    if (avctx->pix_fmt == AV_PIX_FMT_YUV420P10LE) {
> +        av_log(avctx, AV_LOG_DEBUG , "Encoder 10 bits depth input\n");
> +
> +        // SVT-HEVC's compressed 10-bit format is to supported,
> +        // without which it works well functionally but with a
> +        // slight performance penalty caused by the extra conv
> +        // step from yuv420p10le to that format.
> +
> +        param->compressedTenBitFormat = 0;
> +        param->encoderBitDepth        = 10;
> +    }
>

So what is happening in this case? The encoder is slower because it
converts from 10 to 8 bit internally? And then the output encode is 8 bit
right now? If that is the case, I'd rather have this functionality removed
since the conversion can happen directly within ffmpeg. Having the
conversion performed by the encoder is guaranteed to be slower and less
precise, and if the output is not 10 bit it is very surprising too.

At the same time the comment in the code is useless because users will
never read something buried deep in the code, I'd suggest printing
something at the warning level so that it will be shown during the
conversion (and please have it proofread by a native English-speaking
person).


> +    param->encoderColorFormat = EB_YUV420;
> +
> +    // Update param from options
> +    param->hierarchicalLevels     = svt_enc->hierarchical_level - 1;
> +    param->encMode                = svt_enc->enc_mode;
> +    param->profile                = svt_enc->profile;
> +    param->tier                   = svt_enc->tier;
> +    param->level                  = svt_enc->level;
> +    param->rateControlMode        = svt_enc->rc_mode;
> +    param->sceneChangeDetection   = svt_enc->scd;
> +    param->tune                   = svt_enc->tune;
> +    param->baseLayerSwitchMode    = svt_enc->base_layer_switch_mode;
> +    param->qp                     = svt_enc->qp;
> +    param->accessUnitDelimiter    = svt_enc->aud;
> +
> +    param->targetBitRate          = avctx->bit_rate;
> +    if (avctx->gop_size > 0)
> +        param->intraPeriodLength  = avctx->gop_size - 1;
> +
> +    if (avctx->framerate.num > 0 && avctx->framerate.den > 0) {
> +        param->frameRateNumerator     = avctx->framerate.num;
> +        param->frameRateDenominator   = avctx->framerate.den *
> avctx->ticks_per_frame;
> +    } else {
> +        param->frameRateNumerator     = avctx->time_base.den;
> +        param->frameRateDenominator   = avctx->time_base.num *
> avctx->ticks_per_frame;
> +    }
> +
> +    if (param->rateControlMode) {
> +        param->maxQpAllowed       = avctx->qmax;
> +        param->minQpAllowed       = avctx->qmin;
> +    }
> +
> +    param->intraRefreshType       =
> +        !!(avctx->flags & AV_CODEC_FLAG_CLOSED_GOP) + 1;
> +
> +    // is it repeat headers for MP4 or Annex-b
> +    if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER)
> +        param->codeVpsSpsPps          = 0;
> +    else
> +        param->codeVpsSpsPps          = 1;
> +
> +    param->codeEosNal             = 1;
>

nit: excessive whitespace alignment

+
> +    if (svt_enc->hdr) {
> +        svt_enc->vui_info = 1;
> +        param->highDynamicRangeInput = svt_enc->hdr;
> +    }
>

Where is the warning that notifies the lack of color properties support?


> +
> +        avctx->extradata_size = header_ptr->nFilledLen;
> +        avctx->extradata = av_mallocz(avctx->extradata_size +
> AV_INPUT_BUFFER_PADDING_SIZE);
> +        if (!avctx->extradata) {
> +            av_log(avctx, AV_LOG_ERROR,
> +                   "Cannot allocate HEVC header of size %d.\n",
> avctx->extradata_size);
> +            svt_ret = EB_ErrorInsufficientResources;
> +            goto failed_init_enc;
> +        }
>

initialize extradata_size only in case of success, some code may rely on it

+#define OFFSET(x) offsetof(SvtContext, x)
> +#define VE AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_ENCODING_PARAM
> +static const AVOption options[] = {
> +    { "vui", "Enable vui info", OFFSET(vui_info),
> +      AV_OPT_TYPE_BOOL, { .i64 = 1 }, 0, 1, VE },
> +
> +    { "aud", "Include AUD", OFFSET(aud),
> +      AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, VE },
>

IMO these options help text could be improved.

-- 
Vittorio