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

Signed-off-by: Zhong Li <zhong.li@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(-)

On 27/03/2019 10:27, Zhong Li wrote:
> Signed-off-by: Zhong Li <zhong.li@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

> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Mark Thompson

> Sent: Thursday, March 28, 2019 7:27 AM

> To: ffmpeg-devel@ffmpeg.org

> Subject: Re: [FFmpeg-devel] [PATCH v2] lavc/qsvenc: get vps extradata from

> MSDK

> 

> On 27/03/2019 10:27, Zhong Li wrote:

> > Signed-off-by: Zhong Li <zhong.li@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.


1. As far I know, MSDK can't support sublayer right now. (I agree this is not an important point since we still need to consider the further thing once MSDK add this).
2. SPS has sublayer too, but both here and the generate_fake_vps() in qsvenc_hevc.c just give 128 bytes too. So would better to work it as separated patch to fix all of them.
  BTW, should 256 bytes be enough? 

> > +    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?


Good point, Mark! 
I have updated the patch, could you please take a look again?