[FFmpeg-devel] [PATCH] qsv: fix the dangerous macro definitions

[Li, Zhong]

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Li, Zhong
> Sent: Thursday, March 28, 2019 7:12 PM
> To: FFmpeg development discussions and patches
> <ffmpeg-devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH] qsv: fix the dangerous macro
> definitions
> 
> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On
> Behalf
> > Of Carl Eugen Hoyos
> > Sent: Thursday, March 28, 2019 6:55 PM
> > To: FFmpeg development discussions and patches
> > <ffmpeg-devel at ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH] qsv: fix the dangerous macro
> > definitions
> >
> > 2019-03-28 10:09 GMT+01:00, Li, Zhong <zhong.li at intel.com>:
> > >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On
> > Behalf
> > >> Of Mark Thompson
> > >> Sent: Thursday, March 28, 2019 6:23 AM
> > >> To: ffmpeg-devel at ffmpeg.org
> > >> Subject: Re: [FFmpeg-devel] [PATCH] qsv: fix the dangerous macro
> > >> definitions
> > >>
> > >> On 27/03/2019 10:24, Zhong Li wrote:
> > >> > Signed-off-by: Zhong Li <zhong.li at intel.com>
> > >> > ---
> > >> >  libavcodec/qsv_internal.h | 8 ++++----
> > >> >  libavfilter/qsvvpp.h      | 8 ++++----
> > >> >  2 files changed, 8 insertions(+), 8 deletions(-)
> > >> >
> > >> > diff --git a/libavcodec/qsv_internal.h
> > >> > b/libavcodec/qsv_internal.h index 394c558883..86a5dbad98 100644
> > >> > --- a/libavcodec/qsv_internal.h
> > >> > +++ b/libavcodec/qsv_internal.h
> > >> > @@ -35,12 +35,12 @@
> > >> >  #define QSV_MAX_ENC_PAYLOAD 2       // # of mfxEncodeCtrl
> > >> payloads supported
> > >> >
> > >> >  #define QSV_VERSION_ATLEAST(MAJOR, MINOR)   \
> > >> > -    (MFX_VERSION_MAJOR > (MAJOR) ||         \
> > >> > -     MFX_VERSION_MAJOR == (MAJOR) &&
> > MFX_VERSION_MINOR >=
> > >> (MINOR))
> > >> > +    ((MFX_VERSION_MAJOR > (MAJOR) ||         \
> > >> > +     MFX_VERSION_MAJOR == (MAJOR) &&
> > MFX_VERSION_MINOR >=
> > >> (MINOR)))
> > >>
> > >> I don't understand why this makes a difference?
> > >>
> > >> Removing the whitespace, I see:
> > >>
> > >> -  (MFX_VERSION_MAJOR > (MAJOR) || MFX_VERSION_MAJOR ==
> > (MAJOR) &&
> > >> MFX_VERSION_MINOR >= (MINOR))
> > >> + ((MFX_VERSION_MAJOR > (MAJOR) || MFX_VERSION_MAJOR ==
> > (MAJOR)
> > >> &&
> > >> + MFX_VERSION_MINOR >= (MINOR)))
> > >>
> > >> which looks like you've just added redundant parentheses around the
> > >> whole thing which already had them?
> > >>
> > >> (Alternative question: what case is this trying to fix?
> > >
> > > It is to fix the dangerous case of QSV_RUNTIME_VERSION_ATLEAST.
> >
> > > It is introducing a big issue, e:g
> > > if ((avctx->codec_id != AV_CODEC_ID_HEVC) ||
> > > QSV_RUNTIME_VERSION_ATLEAST(q->ver, 1, 28)) it is equal to:
> > > (avctx->codec_id != AV_CODEC_ID_HEVC) || (MFX_VERSION.Major >
> > (MAJOR))
> > > || (MFX_VERSION.Major == (MAJOR) && MFX_VERSION.Minor >=
> > (MINOR))
> >
> > No, they are not equal, you added the second ")" after MAJOR.
> >
> > Carl Eugen
> 
> I can find the second "}" you mentioned. This is the original define:

Sorry for the typo, can -> can't