[FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton

[Li, Zhong]

> From: Rogozhkin, Dmitry V
> Sent: Wednesday, October 31, 2018 2:07 AM
> To: Li, Zhong <zhong.li at intel.com>; ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/5] lavc/qsvenc: add forced_idr opiton
> 
> On Tue, 2018-10-30 at 18:05 +0800, Li, Zhong wrote:
> > > > +        } else
> > > > +            enc_ctrl->FrameType =
> MFX_FRAMETYPE_UNKNOWN;
> > >
> > > "else" block don't make much sense to me. You eventually already had
> > > enc_ctrl structure passed to the encoder. Thus, it should be
> > > initialized to default (already). And you don't make anything
> > > specific/new in the "else".
> > > From my perspective "else" just obscures the code and should be
> > > dropped.
> >
> > This was a case I had concern. I doubt the default initialization is
> > always zero (you know MFX_FRAMETYPE_UNKNOWN is zero). Isn't it
> > possible?
> > Please check the regression case I fixed: https://patchwork.ffmpeg.or
> > g/patch/10517/
> 
> Patch 10517 deals with unitialized variable on a compilation level. As for the
> enc_ctrl I very much hope that ffmpeg-qsv code takes care to memset the
> mfcEncodeCtrl (as well as all other mediasdk structures) _before_ usage. I.e.
> there should be a code somewhere similar to:
>   memset(enc_ctrl, 0, sizeof(mfxEncodeCtrl)); If this is missing, there is a
> VERY big problem in the QSV code since indeed compiler may initialize
> structures to everything it wants and there will be very bad consequences.
> 
> As for the usage of mfxEncodeCtrl, the idea is the following. User allocates
> this control and memsets it to 0. If this will be passed in that way mediasdk
> will change nothing to encode the frame. If user wants to change some
> parameter, it can do this changing only the parameter it wants to take effect.
> So, the "else" should really not be needed.
> 
> Dmitry.

Yup, we are on the same page now to avoid uninitialized value. 
Memset is a good idea, will update.