[FFmpeg-devel] [PATCH 1/3] vaapi_encode: Initialize the pointer

[Xiang, Haihao]

On Wed, 2018-05-09 at 14:48 +0100, Mark Thompson wrote:
> On 09/05/18 05:29, Xiang, Haihao wrote:
> > On Tue, 2018-05-08 at 22:34 +0100, Mark Thompson wrote:
> > > On 08/05/18 03:35, Xiang, Haihao wrote:
> > > > On Mon, 2018-05-07 at 21:46 +0100, Mark Thompson wrote:
> > > > > On 04/05/18 15:41, Haihao Xiang wrote:
> > > > > > Otherwise it might use unitialized last_pic in av_assert0(last_pic)
> > > > > > 
> > > > > > Signed-off-by: Haihao Xiang <haihao.xiang at intel.com>
> > > > > > ---
> > > > > >  libavcodec/vaapi_encode.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> > > > > > index 36c85a3815..141e50c8ad 100644
> > > > > > --- a/libavcodec/vaapi_encode.c
> > > > > > +++ b/libavcodec/vaapi_encode.c
> > > > > > @@ -760,7 +760,7 @@ fail:
> > > > > >  static int vaapi_encode_truncate_gop(AVCodecContext *avctx)
> > > > > >  {
> > > > > >      VAAPIEncodeContext *ctx = avctx->priv_data;
> > > > > > -    VAAPIEncodePicture *pic, *last_pic, *next;
> > > > > > +    VAAPIEncodePicture *pic, *last_pic = NULL, *next;
> > > > > >  
> > > > > >      // Find the last picture we actually have input for.
> > > > > >      for (pic = ctx->pic_start; pic; pic = pic->next) {
> > > > > > 
> > > > > 
> > > > > How do you hit this?  last_pic should always get set.
> > > > > 
> > > > 
> > > > It was reported by some static analysis tools, but I agree with you that
> > > > last_pic should get set in the code path, however if we consider
> > > > vaapi_encode_truncate_gop() only,  last_pic will be uninitialized if
> > > > ctx-
> > > > > pic_start is non-NULL but ctx->pic_start->input_available is not set.
> > > > > How
> > > > > about
> > > > 
> > > > to add av_assert0(!ctx->pic_start || ctx->pic_start->input_available)
> > > > before
> > > > the
> > > >  for () statement and remove av_assert0(last_pic)?
> > > 
> > > I think you mean "av_assert0(ctx->pic_start && ctx->pic_start-
> > > > input_available)"?
> > 
> > No. I meant "av_assert0(!ctx->pic_start || ctx->pic_start-
> > >input_available)". I
> > thought ctx->pic_start might be NULL when vaapi_encode_truncate_gop() is
> > called
> > in vaapi_encode.c, line 865. 
> 
> Apologies, you are correct.  

Never mind.

> I was thinking of the test in the wrong place.
> 
> > However testing a simple transcode of H265 showed input_image->pict_type is
> > always AV_PICTURE_TYPE_NONE (a bug?), which means
> > vaapi_encode_truncate_gop() in
> > vaapi_encode.c, line 865 is never called. This piece of code was added in
> > commit
> > c667c0979cbc2e04d1d00964b82ac49746caa43c to support forcing IDR frame. How
> > do I
> > set a forced IDR via AVFrame.pict_type?
> 
> The setting of pict_type on encoders is intended for library users to force
> key frames where required by other constraints (e.g. when stream
> synchronisation is lost in a videoconferencing-type setup).  Usually an
> encoder has free choice of what GOP structure to use and where to place key
> frames, and this is only used in specific setups requiring it.
> 
> Given that, the ffmpeg utility doesn't set any frame types by default.  For
> testing with it you can override the key frame behaviour with the
> -force_key_frames option, which sets an expression for frames to force as key
> frames by the pict_type mechanism, or you could write a program using
> libavcodec which sets them directly.
> 

Thanks for your explanation, I confirmed pic->start can be NULL when using
-force_key_frame option.

> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel