| Message ID | 20181206111905.28881-1-linjie.fu@intel.com |
|---|---|
| State | Accepted |
| Headers | show |
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Linjie Fu > Sent: Thursday, December 6, 2018 7:19 PM > To: ffmpeg-devel@ffmpeg.org > Cc: Fu, Linjie <linjie.fu@intel.com> > Subject: [FFmpeg-devel] [PATCH, v2] lavc/qsvenc: replace assert with error > return > > bs->FrameType is not set in MSDK in some cases (mjpeg encode for > bs->example), > and assert on a value coming from an external library is not proper. > > Add default type check for bs->FrameType, and return invalid data error in > function ff_qsv_encode to avoid using uninitialized value. > > Fix #7593. > > Signed-off-by: Linjie Fu <linjie.fu@intel.com> > --- > [v2]: Add default bs->FrameType check. > > libavcodec/qsvenc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index > 7f4592f878..917344b60c 100644 > --- a/libavcodec/qsvenc.c > +++ b/libavcodec/qsvenc.c > @@ -1337,8 +1337,10 @@ int ff_qsv_encode(AVCodecContext *avctx, > QSVEncContext *q, > pict_type = AV_PICTURE_TYPE_P; > else if (bs->FrameType & MFX_FRAMETYPE_B || bs->FrameType > & MFX_FRAMETYPE_xB) > pict_type = AV_PICTURE_TYPE_B; > + else if (bs->FrameType == MFX_FRAMETYPE_UNKNOWN) Unknown frame type has potential risk and would better give a waring log message here. > + pict_type = AV_PICTURE_TYPE_NONE; > else > - av_assert0(!"Uninitialized pict_type!"); > + return AVERROR_INVALIDDATA; If frame type is MFX_FRAMETYPE_IDR or MFX_FRAMETYPE_xIDR, will go here but this is not invalid data. So pict_tpye should be set to AV_PICTURE_TYPE_I in this case I believe.
> -----Original Message----- > From: Li, Zhong > Sent: Thursday, December 6, 2018 20:30 > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Cc: Fu, Linjie <linjie.fu@intel.com> > Subject: RE: [FFmpeg-devel] [PATCH, v2] lavc/qsvenc: replace assert with > error return > > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On > Behalf > > Of Linjie Fu > > Sent: Thursday, December 6, 2018 7:19 PM > > To: ffmpeg-devel@ffmpeg.org > > Cc: Fu, Linjie <linjie.fu@intel.com> > > Subject: [FFmpeg-devel] [PATCH, v2] lavc/qsvenc: replace assert with error > > return > > > > bs->FrameType is not set in MSDK in some cases (mjpeg encode for > > bs->example), > > and assert on a value coming from an external library is not proper. > > > > Add default type check for bs->FrameType, and return invalid data error in > > function ff_qsv_encode to avoid using uninitialized value. > > > > Fix #7593. > > > > Signed-off-by: Linjie Fu <linjie.fu@intel.com> > > --- > > [v2]: Add default bs->FrameType check. > > > > libavcodec/qsvenc.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index > > 7f4592f878..917344b60c 100644 > > --- a/libavcodec/qsvenc.c > > +++ b/libavcodec/qsvenc.c > > @@ -1337,8 +1337,10 @@ int ff_qsv_encode(AVCodecContext *avctx, > > QSVEncContext *q, > > pict_type = AV_PICTURE_TYPE_P; > > else if (bs->FrameType & MFX_FRAMETYPE_B || bs->FrameType > > & MFX_FRAMETYPE_xB) > > pict_type = AV_PICTURE_TYPE_B; > > + else if (bs->FrameType == MFX_FRAMETYPE_UNKNOWN) > > Unknown frame type has potential risk and would better give a waring log > message here. Thanks, will add. > > > + pict_type = AV_PICTURE_TYPE_NONE; > > else > > - av_assert0(!"Uninitialized pict_type!"); > > + return AVERROR_INVALIDDATA; > > If frame type is MFX_FRAMETYPE_IDR or MFX_FRAMETYPE_xIDR, will go > here but this is not invalid data. > So pict_tpye should be set to AV_PICTURE_TYPE_I in this case I believe. It seems that Frame types such as MFX_FRAMETYPE_IDR and MFX_FRAMETYPE_xIDR will be set along with MFX_FRAMETYPE_I in MSDK. But frame type which only contains IDR or xIDR should also be set to type I. Will also add an log message to report an error. Thanks Linjie
> > > Subject: [FFmpeg-devel] [PATCH, v2] lavc/qsvenc: replace assert with > > > error return > > > > > > bs->FrameType is not set in MSDK in some cases (mjpeg encode for > > > bs->example), > > > and assert on a value coming from an external library is not proper. > > > > > > Add default type check for bs->FrameType, and return invalid data > > > error in function ff_qsv_encode to avoid using uninitialized value. > > > > > > Fix #7593. > > > > > > Signed-off-by: Linjie Fu <linjie.fu@intel.com> > > > --- > > > [v2]: Add default bs->FrameType check. > > > > > > libavcodec/qsvenc.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index > > > 7f4592f878..917344b60c 100644 > > > --- a/libavcodec/qsvenc.c > > > +++ b/libavcodec/qsvenc.c > > > @@ -1337,8 +1337,10 @@ int ff_qsv_encode(AVCodecContext *avctx, > > > QSVEncContext *q, > > > pict_type = AV_PICTURE_TYPE_P; > > > else if (bs->FrameType & MFX_FRAMETYPE_B || > bs->FrameType & > > > MFX_FRAMETYPE_xB) > > > pict_type = AV_PICTURE_TYPE_B; > > > + else if (bs->FrameType == MFX_FRAMETYPE_UNKNOWN) > > > > Unknown frame type has potential risk and would better give a waring > > log message here. > > Thanks, will add. > > > > > > + pict_type = AV_PICTURE_TYPE_NONE; > > > else > > > - av_assert0(!"Uninitialized pict_type!"); > > > + return AVERROR_INVALIDDATA; > > > > If frame type is MFX_FRAMETYPE_IDR or MFX_FRAMETYPE_xIDR, will go > here > > but this is not invalid data. > > So pict_tpye should be set to AV_PICTURE_TYPE_I in this case I believe. > > It seems that Frame types such as MFX_FRAMETYPE_IDR and > MFX_FRAMETYPE_xIDR will be set along with MFX_FRAMETYPE_I in MSDK. > But frame type which only contains IDR or xIDR should also be set to type I. Should be more robust in ffmpeg level since there are set as different mask bits in MSDK API. I've sent a separated patch to handle IDR frames. (It is also aligned with nvenc: https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/nvenc.c#L1852)
diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index 7f4592f878..917344b60c 100644 --- a/libavcodec/qsvenc.c +++ b/libavcodec/qsvenc.c @@ -1337,8 +1337,10 @@ int ff_qsv_encode(AVCodecContext *avctx, QSVEncContext *q, pict_type = AV_PICTURE_TYPE_P; else if (bs->FrameType & MFX_FRAMETYPE_B || bs->FrameType & MFX_FRAMETYPE_xB) pict_type = AV_PICTURE_TYPE_B; + else if (bs->FrameType == MFX_FRAMETYPE_UNKNOWN) + pict_type = AV_PICTURE_TYPE_NONE; else - av_assert0(!"Uninitialized pict_type!"); + return AVERROR_INVALIDDATA; #if FF_API_CODED_FRAME FF_DISABLE_DEPRECATION_WARNINGS
bs->FrameType is not set in MSDK in some cases (mjpeg encode for example), and assert on a value coming from an external library is not proper. Add default type check for bs->FrameType, and return invalid data error in function ff_qsv_encode to avoid using uninitialized value. Fix #7593. Signed-off-by: Linjie Fu <linjie.fu@intel.com> --- [v2]: Add default bs->FrameType check. libavcodec/qsvenc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)