[FFmpeg-devel,v2] lavc/qsvenc: replace assert with error return

Submitted by Linjie Fu on Dec. 6, 2018, 11:19 a.m.

Details

Message ID 20181206111905.28881-1-linjie.fu@intel.com
State New
Headers show

Commit Message

Linjie Fu Dec. 6, 2018, 11:19 a.m.
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(-)

Comments

Zhong Li Dec. 6, 2018, 12:30 p.m.
> 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.
Linjie Fu Dec. 7, 2018, 4:29 a.m.
> -----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
Zhong Li Dec. 7, 2018, 9:46 a.m.
> > > 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)

Patch hide | download patch | download mbox

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