[FFmpeg-devel] [PATCH v2] lavc/qsvenc: get vps extradata from MSDK

[Mark Thompson]

On 27/03/2019 10:27, Zhong Li wrote:
> Signed-off-by: Zhong Li <zhong.li at intel.com>
> ---
> V2: Fix the regression of qsv h264 encoding since no VPS for h264
> 
>  libavcodec/qsvenc.c      | 53 ++++++++++++++++++++++++++++++++++------
>  libavcodec/qsvenc.h      |  3 +++
>  libavcodec/qsvenc_hevc.c | 10 +++++---
>  3 files changed, 54 insertions(+), 12 deletions(-)
> 
> diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
> index 576d88c259..2f128597db 100644
> --- a/libavcodec/qsvenc.c
> +++ b/libavcodec/qsvenc.c
> @@ -810,6 +810,16 @@ static int qsv_retrieve_enc_params(AVCodecContext *avctx, QSVEncContext *q)
>      };
>  #endif
>  
> +#if QSV_HAVE_CO_VPS
> +    uint8_t vps_buf[128];

Is this necessarily enough?  A VPS can be large when it defines sublayers.

> +    mfxExtCodingOptionVPS extradata_vps = {
> +        .Header.BufferId = MFX_EXTBUFF_CODING_OPTION_VPS,
> +        .Header.BufferSz = sizeof(extradata_vps),
> +        .VPSBuffer       = vps_buf,
> +        .VPSBufSize      = sizeof(vps_buf),
> +    };
> +#endif
> +
>      mfxExtBuffer *ext_buffers[] = {
>          (mfxExtBuffer*)&extradata,
>          (mfxExtBuffer*)&co,
> @@ -818,14 +828,24 @@ static int qsv_retrieve_enc_params(AVCodecContext *avctx, QSVEncContext *q)
>  #endif
>  #if QSV_HAVE_CO3
>          (mfxExtBuffer*)&co3,
> +#endif
> +#if QSV_HAVE_CO_VPS
> +        (mfxExtBuffer*)&extradata_vps,
>  #endif
>      };
>  
>      int need_pps = avctx->codec_id != AV_CODEC_ID_MPEG2VIDEO;
> -    int ret;
> +    int ret, extradata_offset = 0;
>  
>      q->param.ExtParam    = ext_buffers;
> +
> +#if QSV_HAVE_CO_VPS
> +    q->hevc_vps = ((avctx->codec_id == AV_CODEC_ID_HEVC) && QSV_RUNTIME_VERSION_ATLEAST(q->ver, 1, 17));
> +    q->param.NumExtParam = FF_ARRAY_ELEMS(ext_buffers) - 1 + q->hevc_vps;

The array definition and then this length feels a bit overly tricky - any change here will be quite confusing (consider adding a new ExtBuffer).

Making ext_buffers large enough with a running nb_ext_buffers total and then adding the extra one only if H.265 would feel safer to me?

> +#else
> +    q->hevc_vps = 0;
>      q->param.NumExtParam = FF_ARRAY_ELEMS(ext_buffers);
> +#endif
>  
>      ret = MFXVideoENCODE_GetVideoParam(q->session, &q->param);
>      if (ret < 0)
> @@ -834,20 +854,37 @@ static int qsv_retrieve_enc_params(AVCodecContext *avctx, QSVEncContext *q)
>  
>      q->packet_size = q->param.mfx.BufferSizeInKB * q->param.mfx.BRCParamMultiplier * 1000;
>  
> -    if (!extradata.SPSBufSize || (need_pps && !extradata.PPSBufSize)) {
> +    if (!extradata.SPSBufSize || (need_pps && !extradata.PPSBufSize)
> +#if QSV_HAVE_CO_VPS
> +        || (q->hevc_vps && !extradata_vps.VPSBufSize)
> +#endif
> +    ) {
>          av_log(avctx, AV_LOG_ERROR, "No extradata returned from libmfx.\n");
>          return AVERROR_UNKNOWN;
>      }
>  
> -    avctx->extradata = av_malloc(extradata.SPSBufSize + need_pps * extradata.PPSBufSize +
> -                                 AV_INPUT_BUFFER_PADDING_SIZE);
> +    avctx->extradata_size = extradata.SPSBufSize + need_pps * extradata.PPSBufSize;
> +#if QSV_HAVE_CO_VPS
> +    avctx->extradata_size += q->hevc_vps * extradata_vps.VPSBufSize;
> +#endif
> +
> +    avctx->extradata = av_malloc(avctx->extradata_size + AV_INPUT_BUFFER_PADDING_SIZE);
>      if (!avctx->extradata)
>          return AVERROR(ENOMEM);
>  
> -    memcpy(avctx->extradata,                        sps_buf, extradata.SPSBufSize);
> -    if (need_pps)
> -        memcpy(avctx->extradata + extradata.SPSBufSize, pps_buf, extradata.PPSBufSize);
> -    avctx->extradata_size = extradata.SPSBufSize + need_pps * extradata.PPSBufSize;
> +#if QSV_HAVE_CO_VPS
> +    if (q->hevc_vps) {
> +        memcpy(avctx->extradata, vps_buf, extradata_vps.VPSBufSize);
> +        extradata_offset += extradata_vps.VPSBufSize;
> +    }
> +#endif
> +
> +    memcpy(avctx->extradata + extradata_offset, sps_buf, extradata.SPSBufSize);
> +    extradata_offset += extradata.SPSBufSize;
> +    if (need_pps) {
> +        memcpy(avctx->extradata + extradata_offset, pps_buf, extradata.PPSBufSize);
> +        extradata_offset += extradata.PPSBufSize;
> +    }
>      memset(avctx->extradata + avctx->extradata_size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>  
>      cpb_props = ff_add_cpb_side_data(avctx);
> diff --git a/libavcodec/qsvenc.h b/libavcodec/qsvenc.h
> index 25105894f2..f01453e67f 100644
> --- a/libavcodec/qsvenc.h
> +++ b/libavcodec/qsvenc.h
> @@ -36,6 +36,7 @@
>  
>  #define QSV_HAVE_CO2 QSV_VERSION_ATLEAST(1, 6)
>  #define QSV_HAVE_CO3 QSV_VERSION_ATLEAST(1, 11)
> +#define QSV_HAVE_CO_VPS  QSV_VERSION_ATLEAST(1, 17)
>  
>  #define QSV_HAVE_TRELLIS QSV_VERSION_ATLEAST(1, 8)
>  #define QSV_HAVE_MAX_SLICE_SIZE QSV_VERSION_ATLEAST(1, 9)
> @@ -135,6 +136,8 @@ typedef struct QSVEncContext {
>  
>      mfxVersion          ver;
>  
> +    int hevc_vps;
> +
>      // options set by the caller
>      int async_depth;
>      int idr_interval;
> diff --git a/libavcodec/qsvenc_hevc.c b/libavcodec/qsvenc_hevc.c
> index 9e15d3ce6b..df6ef24b72 100644
> --- a/libavcodec/qsvenc_hevc.c
> +++ b/libavcodec/qsvenc_hevc.c
> @@ -195,10 +195,12 @@ static av_cold int qsv_enc_init(AVCodecContext *avctx)
>      if (ret < 0)
>          return ret;
>  
> -    ret = generate_fake_vps(&q->qsv, avctx);
> -    if (ret < 0) {
> -        ff_qsv_enc_close(avctx, &q->qsv);
> -        return ret;
> +    if (!q->qsv.hevc_vps) {
> +        ret = generate_fake_vps(&q->qsv, avctx);
> +        if (ret < 0) {
> +            ff_qsv_enc_close(avctx, &q->qsv);
> +            return ret;
> +        }
>      }
>  
>      return 0;
> 

Rest looks good.

Thanks,

- Mark