[FFmpeg-devel] [PATCH v3 3/6] lavc/qsvdec: Replace current parser with MFXVideoDECODE_DecodeHeader()

[Rogozhkin, Dmitry V]

On Thu, 2019-03-14 at 19:16 +0800, Li, Zhong wrote:
> > From: Rogozhkin, Dmitry V
> > Sent: Tuesday, March 12, 2019 8:04 AM
> > To: Li, Zhong <zhong.li at intel.com>; ffmpeg-devel at ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH v3 3/6] lavc/qsvdec: Replace
> > current
> > parser with MFXVideoDECODE_DecodeHeader()
> > 
> > pix_fmts[1] is misleading. And I think it can be quite easily
> > avoided.
> > 
> > If I understand the code correctly, qsv_decode_preinit is the place
> > (and the
> > only place) where you need an array of pix_fmts. You actually
> > request
> > ffmpeg to select one of the formats and program it into avctx.
> > Is that right?
> > 
> > If so, could we try to define the array exactly in this function
> > and avoid
> > passing the whole array around? See comments below for
> > suggestions...
> > 
> > 
> > On Mon, 2019-03-11 at 17:23 +0800, Li, Zhong wrote:
> > > > -----Original Message-----
> > > > From: Rogozhkin, Dmitry V
> > > > Sent: Saturday, March 9, 2019 8:48 AM
> > > > To: ffmpeg-devel at ffmpeg.org
> > > > Cc: Li, Zhong <zhong.li at intel.com>
> > > > Subject: Re: [FFmpeg-devel] [PATCH v3 3/6] lavc/qsvdec: Replace
> > > > current parser with MFXVideoDECODE_DecodeHeader()
> > > > 
> > > > On Fri, 2019-03-08 at 15:40 +0800, Zhong Li wrote:
> > > > > Using MSDK parser can improve qsv decoder pass rate in some
> > > > > cases
> > > > > (E.g:
> > > > > sps declares a wrong level_idc, smaller than it should be).
> > > > > And it is necessary for adding new qsv decoders such as MJPEG
> > > > > and
> > > > > VP9
> > > > > since current parser can't provide enough information.
> > > > > Actually using MFXVideoDECODE_DecodeHeader() was disscussed
> > > > > at
> > > > > https://ffmpeg.org/pipermail/ffmpeg-devel/2015-July/175734.ht
> > > > > ml
> > > > > and
> > > > > merged as commit 1acb19d, but was overwritten when merged
> > > > > libav
> > > > > patches (commit: 1f26a23) without any explain.
> > > > > 
> > > > > v2: split decode header from decode_init, and call it for
> > > > > everyframe to detect format/resoultion change. It can fix
> > > > > some
> > > > > regression issues such as hevc 10bits decoding.
> > > > > 
> > > > > Signed-off-by: Zhong Li <zhong.li at intel.com>
> > > > > ---
> > > > >  libavcodec/qsvdec.c       | 172
> > 
> > ++++++++++++++++++++----------
> > > > > ----
> > > > > ----
> > > > >  libavcodec/qsvdec.h       |   2 +
> > > > >  libavcodec/qsvdec_h2645.c |   1 +
> > > > >  libavcodec/qsvdec_other.c |   1 +
> > > > >  4 files changed, 93 insertions(+), 83 deletions(-)
> > > > > 
> > > > > diff --git a/libavcodec/qsvdec.c b/libavcodec/qsvdec.c index
> > > > > 4a0be811fb..b78026e14d 100644
> > > > > --- a/libavcodec/qsvdec.c
> > > > > +++ b/libavcodec/qsvdec.c
> > > > > @@ -120,19 +120,17 @@ static inline unsigned int
> > > > > qsv_fifo_size(const
> > > > > AVFifoBuffer* fifo)
> > > > >      return av_fifo_size(fifo) / qsv_fifo_item_size();
> > > > >  }
> > > > > 
> > > > > -static int qsv_decode_init(AVCodecContext *avctx, QSVContext
> > > > > *q)
> > > > > +static int qsv_decode_preinit(AVCodecContext *avctx,
> > > > > QSVContext
> > > > > *q,
> > > > > enum AVPixelFormat *pix_fmts, mfxVideoParam *param)
> > 
> > Just: enum AVPixelFormat pix_fmt. Single one we need.
> 
> No, it must be array, and the last element of this array must be
> AV_PIX_FMT_NONE (else function ff_get_format() doesn't know how many
> elements contained by this array). 
> The first element of pix_fmt[] is HW pixel format, the second element
> of pix_fmt[] is SW pixel format. 
> Once again, I would like you to take a look the implementation detail
> of ff_get_format ().
> 

I understand that, but what you have in your patch is overcomplicate
design. You pass the whole array around from one function to another
while you just use single element from this array, i.e. pix_fmt[1] -
you need to pass only it and construct array when you will actually use
it. Original code had this closer together and was ok. But in your
refactor it you step into problem.

Here is the patch which I think you need to amend into this change: htt
ps://github.com/lizhong1008/ffmpeg-1/pull/1