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

Li, Zhong zhong.li at intel.com
Thu Mar 28 17:19:42 EET 2019



> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Mark Thompson
> Sent: Thursday, March 28, 2019 7:27 AM
> To: ffmpeg-devel at 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 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.

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?


More information about the ffmpeg-devel mailing list