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

[Li, Zhong]

> 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:

#define QSV_RUNTIME_VERSION_ATLEAST(MFX_VERSION, MAJOR, MINOR) \
    (MFX_VERSION.Major > (MAJOR)) ||                           \
    (MFX_VERSION.Major == (MAJOR) && MFX_VERSION.Minor >= (MINOR))