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

[Li, Zhong]

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

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)