| Message ID | 20190327102416.21704-1-zhong.li@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
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
> 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)
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
> 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))
> 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
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
> 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).
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
> 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.
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;
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(-)