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

Submitted by Zhong Li on March 27, 2019, 10:24 a.m.

Details

Message ID 20190327102416.21704-1-zhong.li@intel.com
State New
Headers show

Commit Message

Zhong Li March 27, 2019, 10:24 a.m.
Signed-off-by: Zhong Li <zhong.li@intel.com>
---
 libavcodec/qsv_internal.h | 8 ++++----
 libavfilter/qsvvpp.h      | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

Comments

Mark Thompson March 27, 2019, 10:23 p.m.
On 27/03/2019 10:24, Zhong Li wrote:
> Signed-off-by: Zhong Li <zhong.li@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?  I could see a problem if the MFX_VERSION_* fields were macros expanding to something containing operators with lower precedence than relationals, but that doesn't change with what you've done here.)

- Mark
Zhong Li March 28, 2019, 9:09 a.m.
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Mark Thompson

> Sent: Thursday, March 28, 2019 6:23 AM

> To: ffmpeg-devel@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@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))

If (MFX_VERSION.Major == (MAJOR) && MFX_VERSION.Minor >= (MINOR)) is true, judgement above is always true.

But I forgot that QSV_VERSION_ATLEAST is different from QSV_RUNTIME_VERSION_ATLEAST, will update.


>I could see a problem if the MFX_VERSION_* fields were macros expanding to something

> containing operators with lower precedence than relationals, but that

> doesn't change with what you've done here.)

> 

> - Mark


I couldn't see the problem you mentioned, will it happen in a real case?
But I see another case now: 
MFX_VERSION_MAJOR > (MAJOR) || MFX_VERSION_MAJOR == (MAJOR) && MFX_VERSION_MINOR >= (MINOR)
Actually it is equal to:
(MFX_VERSION_MAJOR > (MAJOR) || MFX_VERSION_MAJOR == (MAJOR)) && MFX_VERSION_MINOR >= (MINOR)
This is not expectation and will cause a big issue when MFX_VERSION_MAJOR > (MAJOR)
Carl Eugen Hoyos March 28, 2019, 10:54 a.m.
2019-03-28 10:09 GMT+01:00, Li, Zhong <zhong.li@intel.com>:
>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf
>> Of Mark Thompson
>> Sent: Thursday, March 28, 2019 6:23 AM
>> To: ffmpeg-devel@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@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
Zhong Li March 28, 2019, 11:12 a.m.
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Carl Eugen Hoyos

> Sent: Thursday, March 28, 2019 6:55 PM

> To: FFmpeg development discussions and patches

> <ffmpeg-devel@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@intel.com>:

> >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On

> Behalf

> >> Of Mark Thompson

> >> Sent: Thursday, March 28, 2019 6:23 AM

> >> To: ffmpeg-devel@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@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))
Zhong Li March 28, 2019, 11:13 a.m.
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Li, Zhong

> Sent: Thursday, March 28, 2019 7:12 PM

> To: FFmpeg development discussions and patches

> <ffmpeg-devel@ffmpeg.org>

> Subject: Re: [FFmpeg-devel] [PATCH] qsv: fix the dangerous macro

> definitions

> 

> > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On

> Behalf

> > Of Carl Eugen Hoyos

> > Sent: Thursday, March 28, 2019 6:55 PM

> > To: FFmpeg development discussions and patches

> > <ffmpeg-devel@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@intel.com>:

> > >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On

> > Behalf

> > >> Of Mark Thompson

> > >> Sent: Thursday, March 28, 2019 6:23 AM

> > >> To: ffmpeg-devel@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@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
Carl Eugen Hoyos March 28, 2019, 11:48 a.m.
2019-03-27 11:24 GMT+01:00, Zhong Li <zhong.li@intel.com>:
> Signed-off-by: Zhong Li <zhong.li@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)))

This part is not needed, the existing code starts with an opening
parenthesis that is closed at the end of the macro.

>  #define QSV_RUNTIME_VERSION_ATLEAST(MFX_VERSION, 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)))

This one is of course correct.

Carl Eugen
Zhong Li March 28, 2019, 11:55 a.m.
> 2019-03-27 11:24 GMT+01:00, Zhong Li <zhong.li@intel.com>:

> > Signed-off-by: Zhong Li <zhong.li@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)))

> 

> This part is not needed, the existing code starts with an opening parenthesis

> that is closed at the end of the macro.


Yes, it is not needed (Mark posted same comment too) and I've updated it in V2 with a new issue fixed.

> >  #define QSV_RUNTIME_VERSION_ATLEAST(MFX_VERSION, 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)))

> 

> This one is of course correct.

> 

> Carl Eugen


Thanks for review (Would be nice if you can take a look at patch v2 and then I will apply it).
Carl Eugen Hoyos March 28, 2019, 12:15 p.m.
2019-03-28 10:09 GMT+01:00, Li, Zhong <zhong.li@intel.com>:

> But I see another case now:
> MFX_VERSION_MAJOR > (MAJOR) || MFX_VERSION_MAJOR == (MAJOR) &&
> MFX_VERSION_MINOR >= (MINOR)
> Actually it is equal to:
> (MFX_VERSION_MAJOR > (MAJOR) || MFX_VERSION_MAJOR == (MAJOR)) &&
> MFX_VERSION_MINOR >= (MINOR)

No?

Carl Eugen
Zhong Li March 28, 2019, 12:24 p.m.
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Carl Eugen Hoyos

> Sent: Thursday, March 28, 2019 8:15 PM

> To: FFmpeg development discussions and patches

> <ffmpeg-devel@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@intel.com>:

> 

> > But I see another case now:

> > MFX_VERSION_MAJOR > (MAJOR) || MFX_VERSION_MAJOR == (MAJOR)

> &&

> > MFX_VERSION_MINOR >= (MINOR) Actually it is equal to:

> > (MFX_VERSION_MAJOR > (MAJOR) || MFX_VERSION_MAJOR == (MAJOR))

> &&

> > MFX_VERSION_MINOR >= (MINOR)

> 

> No?

> 

> Carl Eugen


After double-checked, you are right. It is not needed.

Patch hide | download patch | download mbox

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)))
 
 #define QSV_RUNTIME_VERSION_ATLEAST(MFX_VERSION, 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)))
 
 typedef struct QSVMid {
     AVBufferRef *hw_frames_ref;
diff --git a/libavfilter/qsvvpp.h b/libavfilter/qsvvpp.h
index ff02b64c41..a02de6321a 100644
--- a/libavfilter/qsvvpp.h
+++ b/libavfilter/qsvvpp.h
@@ -32,12 +32,12 @@ 
 #define FF_OUTLINK_IDX(link) ((int)((link)->srcpad - (link)->src->output_pads))
 
 #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)))
 
 #define QSV_RUNTIME_VERSION_ATLEAST(MFX_VERSION, 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)))
 
 typedef struct QSVVPPContext QSVVPPContext;