> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@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...@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) _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
