| Message ID | 20180504144107.16201-1-haihao.xiang@intel.com |
|---|---|
| State | New |
| Headers | show |
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@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. - Mark
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@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)? > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
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@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)"? But yes, that would be sensible. I guess it's the assert on last_pic which is confusing the static analysis, since it kindof implies that it could be null. - Mark (I'm currently looking at refactoring all of this logic to assign picture types at encode time rather than input time - the current method makes it very hard to implement more complex reference structures.)
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@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. 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? > But yes, that would be sensible. I guess it's the assert on last_pic which is > confusing the static analysis, since it kindof implies that it could be null. > > - Mark > > > (I'm currently looking at refactoring all of this logic to assign picture > types at encode time rather than input time - the current method makes it very > hard to implement more complex reference structures.) > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
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@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
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@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
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) {
Otherwise it might use unitialized last_pic in av_assert0(last_pic) Signed-off-by: Haihao Xiang <haihao.xiang@intel.com> --- libavcodec/vaapi_encode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)