[FFmpeg-devel] [PATCH v2] lavc/qsv: suppress code scan complain

[Carl Eugen Hoyos]

2018-06-14 5:08 GMT+02:00, Li, Zhong <zhong.li at intel.com>:
>> -----Original Message-----
>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
>> Of Carl Eugen Hoyos
>> Sent: Wednesday, June 13, 2018 6:11 PM
>> To: FFmpeg development discussions and patches
>> <ffmpeg-devel at ffmpeg.org>
>> Subject: Re: [FFmpeg-devel] [PATCH v2] lavc/qsv: suppress code scan
>> complain
>>
>> 2018-06-13 12:05 GMT+02:00, Li, Zhong <zhong.li at intel.com>:
>> >> -----Original Message-----
>> >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On
>> Behalf
>> >> Of Carl Eugen Hoyos
>> >> Sent: Wednesday, June 13, 2018 5:42 PM
>> >> To: FFmpeg development discussions and patches
>> >> <ffmpeg-devel at ffmpeg.org>
>> >> Subject: Re: [FFmpeg-devel] [PATCH v2] lavc/qsv: suppress code scan
>> >> complain
>> >>
>> >> 2018-05-24 11:39 GMT+02:00, Li, Zhong <zhong.li at intel.com>:
>> >> >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On
>> >> Behalf
>> >> >> Of Carl Eugen Hoyos
>> >> >> Sent: Thursday, May 24, 2018 5:33 PM
>> >> >> To: FFmpeg development discussions and patches
>> >> >> <ffmpeg-devel at ffmpeg.org>
>> >> >> Subject: Re: [FFmpeg-devel] [PATCH v2] lavc/qsv: suppress code
>> >> >> scan complain
>> >> >>
>> >> >> 2018-05-24 10:35 GMT+02:00, Li, Zhong <zhong.li at intel.com>:
>> >> >> >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org]
>> On
>> >> >> Behalf
>> >> >> >> Of Carl Eugen Hoyos
>> >> >> >> Sent: Wednesday, May 23, 2018 8:32 PM
>> >> >> >> To: FFmpeg development discussions and patches
>> >> >> >> <ffmpeg-devel at ffmpeg.org>
>> >> >> >> Subject: Re: [FFmpeg-devel] [PATCH v2] lavc/qsv: suppress code
>> >> >> >> scan complain
>> >> >> >>
>> >> >> >> 2018-05-23 12:46 GMT+02:00, Zhong Li <zhong.li at intel.com>:
>> >> >> >> > Suppress the complain "variables 'type' is used but maybe
>> >> >> >> > uninitialized".
>> >> >> >> > ---
>> >> >> >> >  libavcodec/qsv.c | 5 ++++-
>> >> >> >> >  1 file changed, 4 insertions(+), 1 deletion(-)
>> >> >> >> >
>> >> >> >> > diff --git a/libavcodec/qsv.c b/libavcodec/qsv.c index
>> >> >> >> > 45e1c25..3ff4f2c 100644
>> >> >> >> > --- a/libavcodec/qsv.c
>> >> >> >> > +++ b/libavcodec/qsv.c
>> >> >> >> > @@ -31,6 +31,7 @@
>> >> >> >> >  #include "libavutil/hwcontext.h"
>> >> >> >> >  #include "libavutil/hwcontext_qsv.h"
>> >> >> >> >  #include "libavutil/imgutils.h"
>> >> >> >> > +#include "libavutil/avassert.h"
>> >> >> >> >
>> >> >> >> >  #include "avcodec.h"
>> >> >> >> >  #include "qsv_internal.h"
>> >> >> >> > @@ -197,7 +198,7 @@ int
>> >> ff_qsv_find_surface_idx(QSVFramesContext
>> >> >> >> *ctx,
>> >> >> >> > QSVFrame *frame)
>> >> >> >> >
>> >> >> >> >  enum AVPictureType ff_qsv_map_pictype(int mfx_pic_type)  {
>> >> >> >> > -    enum AVPictureType type;
>> >> >> >> > +    enum AVPictureType type = AV_PICTURE_TYPE_NONE;
>> >> >> >> >      switch (mfx_pic_type & 0x7) {
>> >> >> >> >      case MFX_FRAMETYPE_I:
>> >> >> >> >          if (mfx_pic_type & MFX_FRAMETYPE_S) @@ -214,6
>> >> +215,8
>> >> >> >> @@ enum
>> >> >> >> > AVPictureType ff_qsv_map_pictype(int mfx_pic_type)
>> >> >> >> >          else
>> >> >> >> >              type = AV_PICTURE_TYPE_P;
>> >> >> >> >          break;
>> >> >> >> > +    default:
>> >> >> >> > +        av_assert0(0);
>> >> >> >>
>> >> >> >> I didn't test but I would have expected that exactly one of
>> >> >> >> these changes is sufficient to silence the warning, no?
>> >> >> >
>> >> >> > Thanks for review. It is not a compile warning and just found by
>> >> >> > Coverity Scan, I've double-confirmed this patch is useful to
>> >> >> > suppress the code scan complain.
>> >> >>
>> >> >> Of course, I understand.
>> >> >>
>> >> >> My question was if one of the two changes (ie either the variable
>> >> >> initialization or the assert) isn't enough to suppress the code
>> >> >> scan complain.
>> >> >
>> >> > I've confirmed that, running scan again. The complain is
>> >> > disappeared now.
>> >>
>> >> Then why did you push both changes instead of only one of them?
>> >
>> >  This one has been reviewed by you
>>
>> My two questions were:
>> Isn't it enough to only change the variable initialization (and change
>> nothing
>> in the switch) to silence the complain?
>> Isn't it enough to only add the default case to the switch (and change
>> nothing in the variable initialization) to silence the complain?
>>
>> You committed changing both the variable initialization and added a
>> default
>> case, one should have been enough.
>
> Ok, I misunderstood you doubted any one of these two changes can silence the
> complain.
> Yes, one is enough for the complain. (the first version of this patch is
> just to change the variable initialization. Mark suggested to add the
> assert() to check the input is valid).

> So maybe you would like to keep the assert() and remove the variable
> initialization?

That would have been my original suggestion, yes.

Carl Eugen