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

[Mark Thompson]

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.  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.

- Mark