Message ID | 20200821052301.1864733-1-haihao.xiang@intel.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,v2,1/2] qsv: needn't load user plugin since libmfx 1.28 | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Haihao Xiang > Sent: Friday, August 21, 2020 7:23 AM > To: ffmpeg-devel@ffmpeg.org > Cc: Haihao Xiang <haihao.xiang@intel.com> > Subject: [FFmpeg-devel] [PATCH v2 1/2] qsv: needn't load user plugin since > libmfx 1.28 > > MFXVideoUSER_Load call is redundant since libmfx 1.28 > --- > Fixed merge conflict when applying this patch by 'git am' > > libavcodec/qsv.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/qsv.c b/libavcodec/qsv.c index > 17720070f1..56a30ad642 100644 > --- a/libavcodec/qsv.c > +++ b/libavcodec/qsv.c > @@ -19,7 +19,6 @@ > */ > > #include <mfx/mfxvideo.h> > -#include <mfx/mfxplugin.h> > #include <mfx/mfxjpeg.h> > > #include <stdio.h> > @@ -36,6 +35,10 @@ > #include "avcodec.h" > #include "qsv_internal.h" > > +#if !QSV_VERSION_ATLEAST(1, 28) > +#include <mfx/mfxplugin.h> > +#endif > + > #if QSV_VERSION_ATLEAST(1, 12) > #include "mfx/mfxvp8.h" > #endif > @@ -295,6 +298,9 @@ enum AVPictureType ff_qsv_map_pictype(int > mfx_pic_type) static int qsv_load_plugins(mfxSession session, const char > *load_plugins, > void *logctx) { > +#if QSV_VERSION_ATLEAST(1, 28) > + return 0; > +#else > if (!load_plugins || !*load_plugins) > return 0; > > @@ -340,6 +346,7 @@ load_plugin_fail: > } > > return 0; > +#endif > > } Hi, Are you sure this check is right? You are checking the libmfx version against which ffmpeg is compiled. What happens, when a graphics driver supports only API level 1.21? Regards, softworkz
On Fri, 2020-08-21 at 05:48 +0000, Soft Works wrote: > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > Haihao Xiang > > Sent: Friday, August 21, 2020 7:23 AM > > To: ffmpeg-devel@ffmpeg.org > > Cc: Haihao Xiang <haihao.xiang@intel.com> > > Subject: [FFmpeg-devel] [PATCH v2 1/2] qsv: needn't load user plugin since > > libmfx 1.28 > > > > MFXVideoUSER_Load call is redundant since libmfx 1.28 > > --- > > Fixed merge conflict when applying this patch by 'git am' > > > > libavcodec/qsv.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/libavcodec/qsv.c b/libavcodec/qsv.c index > > 17720070f1..56a30ad642 100644 > > --- a/libavcodec/qsv.c > > +++ b/libavcodec/qsv.c > > @@ -19,7 +19,6 @@ > > */ > > > > #include <mfx/mfxvideo.h> > > -#include <mfx/mfxplugin.h> > > #include <mfx/mfxjpeg.h> > > > > #include <stdio.h> > > @@ -36,6 +35,10 @@ > > #include "avcodec.h" > > #include "qsv_internal.h" > > > > +#if !QSV_VERSION_ATLEAST(1, 28) > > +#include <mfx/mfxplugin.h> > > +#endif > > + > > #if QSV_VERSION_ATLEAST(1, 12) > > #include "mfx/mfxvp8.h" > > #endif > > @@ -295,6 +298,9 @@ enum AVPictureType ff_qsv_map_pictype(int > > mfx_pic_type) static int qsv_load_plugins(mfxSession session, const char > > *load_plugins, > > void *logctx) { > > +#if QSV_VERSION_ATLEAST(1, 28) > > + return 0; > > +#else > > if (!load_plugins || !*load_plugins) > > return 0; > > > > @@ -340,6 +346,7 @@ load_plugin_fail: > > } > > > > return 0; > > +#endif > > > > } > > > Hi, > > Are you sure this check is right? You are checking the libmfx version > against which ffmpeg is compiled. > > What happens, when a graphics driver supports only API level 1.21? > I understand your concern, however lots of features in FFmpeg are disabled/enabled against api version at compile-time. Thanks Haihao > Regards, > softworkz > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Fri, Aug 21, 2020 at 9:29 AM Xiang, Haihao <haihao.xiang@intel.com> wrote: > On Fri, 2020-08-21 at 05:48 +0000, Soft Works wrote: > > > -----Original Message----- > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > > Haihao Xiang > > > Sent: Friday, August 21, 2020 7:23 AM > > > To: ffmpeg-devel@ffmpeg.org > > > Cc: Haihao Xiang <haihao.xiang@intel.com> > > > Subject: [FFmpeg-devel] [PATCH v2 1/2] qsv: needn't load user plugin > since > > > libmfx 1.28 > > > > > > MFXVideoUSER_Load call is redundant since libmfx 1.28 > > > --- > > > Fixed merge conflict when applying this patch by 'git am' > > > > > > libavcodec/qsv.c | 9 ++++++++- > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/libavcodec/qsv.c b/libavcodec/qsv.c index > > > 17720070f1..56a30ad642 100644 > > > --- a/libavcodec/qsv.c > > > +++ b/libavcodec/qsv.c > > > @@ -19,7 +19,6 @@ > > > */ > > > > > > #include <mfx/mfxvideo.h> > > > -#include <mfx/mfxplugin.h> > > > #include <mfx/mfxjpeg.h> > > > > > > #include <stdio.h> > > > @@ -36,6 +35,10 @@ > > > #include "avcodec.h" > > > #include "qsv_internal.h" > > > > > > +#if !QSV_VERSION_ATLEAST(1, 28) > > > +#include <mfx/mfxplugin.h> > > > +#endif > > > + > > > #if QSV_VERSION_ATLEAST(1, 12) > > > #include "mfx/mfxvp8.h" > > > #endif > > > @@ -295,6 +298,9 @@ enum AVPictureType ff_qsv_map_pictype(int > > > mfx_pic_type) static int qsv_load_plugins(mfxSession session, const > char > > > *load_plugins, > > > void *logctx) { > > > +#if QSV_VERSION_ATLEAST(1, 28) > > > + return 0; > > > +#else > > > if (!load_plugins || !*load_plugins) > > > return 0; > > > > > > @@ -340,6 +346,7 @@ load_plugin_fail: > > > } > > > > > > return 0; > > > +#endif > > > > > > } > > > > > > Hi, > > > > Are you sure this check is right? You are checking the libmfx version > > against which ffmpeg is compiled. > > > > What happens, when a graphics driver supports only API level 1.21? > > > > I understand your concern, however lots of features in FFmpeg are > disabled/enabled against api version at compile-time. > > have you checked if runtime checks with QSV_RUNTIME_VERSION_ATLEAST can be avoided? regards Max
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Xiang, Haihao > Sent: Friday, August 21, 2020 9:29 AM > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH v2 1/2] qsv: needn't load user plugin > since libmfx 1.28 > > On Fri, 2020-08-21 at 05:48 +0000, Soft Works wrote: > > > -----Original Message----- > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > > Haihao Xiang > > > Sent: Friday, August 21, 2020 7:23 AM > > > To: ffmpeg-devel@ffmpeg.org > > > Cc: Haihao Xiang <haihao.xiang@intel.com> > > > Subject: [FFmpeg-devel] [PATCH v2 1/2] qsv: needn't load user plugin > > > since libmfx 1.28 > > > > > > MFXVideoUSER_Load call is redundant since libmfx 1.28 > > > --- > > > Fixed merge conflict when applying this patch by 'git am' > > > > > > libavcodec/qsv.c | 9 ++++++++- > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/libavcodec/qsv.c b/libavcodec/qsv.c index > > > 17720070f1..56a30ad642 100644 > > > --- a/libavcodec/qsv.c > > > +++ b/libavcodec/qsv.c > > > @@ -19,7 +19,6 @@ > > > */ > > > > > > #include <mfx/mfxvideo.h> > > > -#include <mfx/mfxplugin.h> > > > #include <mfx/mfxjpeg.h> > > > > > > #include <stdio.h> > > > @@ -36,6 +35,10 @@ > > > #include "avcodec.h" > > > #include "qsv_internal.h" > > > > > > +#if !QSV_VERSION_ATLEAST(1, 28) > > > +#include <mfx/mfxplugin.h> > > > +#endif > > > + > > > #if QSV_VERSION_ATLEAST(1, 12) > > > #include "mfx/mfxvp8.h" > > > #endif > > > @@ -295,6 +298,9 @@ enum AVPictureType ff_qsv_map_pictype(int > > > mfx_pic_type) static int qsv_load_plugins(mfxSession session, const > > > char *load_plugins, > > > void *logctx) { > > > +#if QSV_VERSION_ATLEAST(1, 28) > > > + return 0; > > > +#else > > > if (!load_plugins || !*load_plugins) > > > return 0; > > > > > > @@ -340,6 +346,7 @@ load_plugin_fail: > > > } > > > > > > return 0; > > > +#endif > > > > > > } > > > > > > Hi, > > > > Are you sure this check is right? You are checking the libmfx version > > against which ffmpeg is compiled. > > > > What happens, when a graphics driver supports only API level 1.21? > > > > I understand your concern, however lots of features in FFmpeg are > disabled/enabled against api version at compile-time. That is in no way an argument to break compatibility with downlevel drivers without any need. Things are working fine without that patch for all API versions. If you really want to avoid the plugin-load, then you should check the driver's API level at runtime. Kind regards, softworkz
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Soft Works > Sent: Friday, August 21, 2020 9:45 AM > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH v2 1/2] qsv: needn't load user plugin > since libmfx 1.28 > > > > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > Xiang, Haihao > > Sent: Friday, August 21, 2020 9:29 AM > > To: ffmpeg-devel@ffmpeg.org > > Subject: Re: [FFmpeg-devel] [PATCH v2 1/2] qsv: needn't load user > > plugin since libmfx 1.28 > > > > On Fri, 2020-08-21 at 05:48 +0000, Soft Works wrote: > > > > -----Original Message----- > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf > Of > > > > Haihao Xiang > > > > Sent: Friday, August 21, 2020 7:23 AM > > > > To: ffmpeg-devel@ffmpeg.org > > > > Cc: Haihao Xiang <haihao.xiang@intel.com> > > > > Subject: [FFmpeg-devel] [PATCH v2 1/2] qsv: needn't load user > > > > plugin since libmfx 1.28 > > > > > > > > MFXVideoUSER_Load call is redundant since libmfx 1.28 > > > > --- > > > > Fixed merge conflict when applying this patch by 'git am' > > > > > > > > libavcodec/qsv.c | 9 ++++++++- > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/libavcodec/qsv.c b/libavcodec/qsv.c index > > > > 17720070f1..56a30ad642 100644 > > > > --- a/libavcodec/qsv.c > > > > +++ b/libavcodec/qsv.c > > > > @@ -19,7 +19,6 @@ > > > > */ > > > > > > > > #include <mfx/mfxvideo.h> > > > > -#include <mfx/mfxplugin.h> > > > > #include <mfx/mfxjpeg.h> > > > > > > > > #include <stdio.h> > > > > @@ -36,6 +35,10 @@ > > > > #include "avcodec.h" > > > > #include "qsv_internal.h" > > > > > > > > +#if !QSV_VERSION_ATLEAST(1, 28) > > > > +#include <mfx/mfxplugin.h> > > > > +#endif > > > > + > > > > #if QSV_VERSION_ATLEAST(1, 12) > > > > #include "mfx/mfxvp8.h" > > > > #endif > > > > @@ -295,6 +298,9 @@ enum AVPictureType ff_qsv_map_pictype(int > > > > mfx_pic_type) static int qsv_load_plugins(mfxSession session, > > > > const char *load_plugins, > > > > void *logctx) { > > > > +#if QSV_VERSION_ATLEAST(1, 28) > > > > + return 0; > > > > +#else > > > > if (!load_plugins || !*load_plugins) > > > > return 0; > > > > > > > > @@ -340,6 +346,7 @@ load_plugin_fail: > > > > } > > > > > > > > return 0; > > > > +#endif > > > > > > > > } > > > > > > > > > Hi, > > > > > > Are you sure this check is right? You are checking the libmfx > > > version against which ffmpeg is compiled. > > > > > > What happens, when a graphics driver supports only API level 1.21? > > > > > > > I understand your concern, however lots of features in FFmpeg are > > disabled/enabled against api version at compile-time. > > That is in no way an argument to break compatibility with downlevel drivers > without any need. Things are working fine without that patch for all API > versions. > > If you really want to avoid the plugin-load, then you should check the driver's > API level at runtime. Haihao, let me add a few more points to this subject: - Backwards-Compatibility is a key feature of the Intel Media SDK Your colleagues which are doing the MSDK development are continuously taking care to make that possible - When you look at the latest release notes (MSDK 2020 R1, API 1.32), you can see that 3rd, 4th and 5th gen CPUs are still supported The latest 3rd gen drivers are API level 1.11 The latest 4th and 5th gen drivers are API level 1.20 Also later CPUs do not always have the latest drivers (> 1.28) installed - Even with libmfx 1.32, it is still possible to use those CPUs or older driver versions, and that applies to ffmpeg as well, when it's compiled against libmfx 1.32 - Your change would only be legitimate if there would have been a change In the dispatcher code since API 1.28 that would automatically handle the Plugin loading for drivers < 1.28. But I have just gone through the recent changes to the dispatcher code And there doesn't exist such functionality. - For those reasons, your commit is not acceptable as it would break HEVC encoding for all 3,4,5 gen CPUs and later CPUs with downlevel Drivers What remains is the question whether there's any benefit at all in Avoiding the plugin load call. Does it cost time in cases when it would not be needed (driver level >= 1.28)? - If yes, then the driver API level can be checked at runtime to avoid the Plugin load. - Otherwise, I can't imagine any reason to make any change at all What are yours? Kind regards, softworkz
On Fri, 2020-08-21 at 20:11 +0000, Soft Works wrote: > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > Soft Works > > Sent: Friday, August 21, 2020 9:45 AM > > To: FFmpeg development discussions and patches <ffmpeg- > > devel@ffmpeg.org> > > Subject: Re: [FFmpeg-devel] [PATCH v2 1/2] qsv: needn't load user plugin > > since libmfx 1.28 > > > > > > > > > -----Original Message----- > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > > Xiang, Haihao > > > Sent: Friday, August 21, 2020 9:29 AM > > > To: ffmpeg-devel@ffmpeg.org > > > Subject: Re: [FFmpeg-devel] [PATCH v2 1/2] qsv: needn't load user > > > plugin since libmfx 1.28 > > > > > > On Fri, 2020-08-21 at 05:48 +0000, Soft Works wrote: > > > > > -----Original Message----- > > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf > > > > Of > > > > > Haihao Xiang > > > > > Sent: Friday, August 21, 2020 7:23 AM > > > > > To: ffmpeg-devel@ffmpeg.org > > > > > Cc: Haihao Xiang <haihao.xiang@intel.com> > > > > > Subject: [FFmpeg-devel] [PATCH v2 1/2] qsv: needn't load user > > > > > plugin since libmfx 1.28 > > > > > > > > > > MFXVideoUSER_Load call is redundant since libmfx 1.28 > > > > > --- > > > > > Fixed merge conflict when applying this patch by 'git am' > > > > > > > > > > libavcodec/qsv.c | 9 ++++++++- > > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/libavcodec/qsv.c b/libavcodec/qsv.c index > > > > > 17720070f1..56a30ad642 100644 > > > > > --- a/libavcodec/qsv.c > > > > > +++ b/libavcodec/qsv.c > > > > > @@ -19,7 +19,6 @@ > > > > > */ > > > > > > > > > > #include <mfx/mfxvideo.h> > > > > > -#include <mfx/mfxplugin.h> > > > > > #include <mfx/mfxjpeg.h> > > > > > > > > > > #include <stdio.h> > > > > > @@ -36,6 +35,10 @@ > > > > > #include "avcodec.h" > > > > > #include "qsv_internal.h" > > > > > > > > > > +#if !QSV_VERSION_ATLEAST(1, 28) > > > > > +#include <mfx/mfxplugin.h> > > > > > +#endif > > > > > + > > > > > #if QSV_VERSION_ATLEAST(1, 12) > > > > > #include "mfx/mfxvp8.h" > > > > > #endif > > > > > @@ -295,6 +298,9 @@ enum AVPictureType ff_qsv_map_pictype(int > > > > > mfx_pic_type) static int qsv_load_plugins(mfxSession session, > > > > > const char *load_plugins, > > > > > void *logctx) { > > > > > +#if QSV_VERSION_ATLEAST(1, 28) > > > > > + return 0; > > > > > +#else > > > > > if (!load_plugins || !*load_plugins) > > > > > return 0; > > > > > > > > > > @@ -340,6 +346,7 @@ load_plugin_fail: > > > > > } > > > > > > > > > > return 0; > > > > > +#endif > > > > > > > > > > } > > > > > > > > > > > > Hi, > > > > > > > > Are you sure this check is right? You are checking the libmfx > > > > version against which ffmpeg is compiled. > > > > > > > > What happens, when a graphics driver supports only API level 1.21? > > > > > > > > > > I understand your concern, however lots of features in FFmpeg are > > > disabled/enabled against api version at compile-time. > > > > That is in no way an argument to break compatibility with downlevel drivers > > without any need. Things are working fine without that patch for all API > > versions. > > > > If you really want to avoid the plugin-load, then you should check the > > driver's > > API level at runtime. > > Haihao, > > let me add a few more points to this subject: > > - Backwards-Compatibility is a key feature of the Intel Media SDK > Your colleagues which are doing the MSDK development are continuously > taking care to make that possible > > - When you look at the latest release notes (MSDK 2020 R1, API 1.32), > you can see that 3rd, 4th and 5th gen CPUs are still supported > The latest 3rd gen drivers are API level 1.11 > The latest 4th and 5th gen drivers are API level 1.20 > Also later CPUs do not always have the latest drivers (> 1.28) installed > > - Even with libmfx 1.32, it is still possible to use those CPUs or older > driver versions, and that applies to ffmpeg as well, when it's compiled > against libmfx 1.32 > > - Your change would only be legitimate if there would have been a change > In the dispatcher code since API 1.28 that would automatically handle the > Plugin loading for drivers < 1.28. > But I have just gone through the recent changes to the dispatcher code > And there doesn't exist such functionality. > > - For those reasons, your commit is not acceptable as it would break > HEVC encoding for all 3,4,5 gen CPUs and later CPUs with downlevel > Drivers > > What remains is the question whether there's any benefit at all in > Avoiding the plugin load call. > > Does it cost time in cases when it would not be needed (driver level >= 1.28)? > > - If yes, then the driver API level can be checked at runtime to avoid the > Plugin load. > - Otherwise, I can't imagine any reason to make any change at all > > What are yours? Hi softworkz, Thanks for your comment. - I definitely agree with you that it is important to keep backwards compatibility. - I am aware that using QSV_VERSION_ATLEAST at compile time might break backwards compatibility, but considering QSV_VERSION_ATLEAST has been used widely at compile time, I used it in this patch too. - Considering that the plugins are redundant now, it is possible to deprecate or remove these plugins in the future. I am fine not to accept this patch presently, and will pick it back once MFXVideoUSER_Load is deprecated or removed from the SDK in the future Thanks Haihao > > Kind regards, > softworkz > > > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Fri, 2020-08-21 at 09:36 +0200, Max Dmitrichenko wrote: > On Fri, Aug 21, 2020 at 9:29 AM Xiang, Haihao <haihao.xiang@intel.com> > wrote: > > > On Fri, 2020-08-21 at 05:48 +0000, Soft Works wrote: > > > > -----Original Message----- > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > > > Haihao Xiang > > > > Sent: Friday, August 21, 2020 7:23 AM > > > > To: ffmpeg-devel@ffmpeg.org > > > > Cc: Haihao Xiang <haihao.xiang@intel.com> > > > > Subject: [FFmpeg-devel] [PATCH v2 1/2] qsv: needn't load user plugin > > > > since > > > > libmfx 1.28 > > > > > > > > MFXVideoUSER_Load call is redundant since libmfx 1.28 > > > > --- > > > > Fixed merge conflict when applying this patch by 'git am' > > > > > > > > libavcodec/qsv.c | 9 ++++++++- > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/libavcodec/qsv.c b/libavcodec/qsv.c index > > > > 17720070f1..56a30ad642 100644 > > > > --- a/libavcodec/qsv.c > > > > +++ b/libavcodec/qsv.c > > > > @@ -19,7 +19,6 @@ > > > > */ > > > > > > > > #include <mfx/mfxvideo.h> > > > > -#include <mfx/mfxplugin.h> > > > > #include <mfx/mfxjpeg.h> > > > > > > > > #include <stdio.h> > > > > @@ -36,6 +35,10 @@ > > > > #include "avcodec.h" > > > > #include "qsv_internal.h" > > > > > > > > +#if !QSV_VERSION_ATLEAST(1, 28) > > > > +#include <mfx/mfxplugin.h> > > > > +#endif > > > > + > > > > #if QSV_VERSION_ATLEAST(1, 12) > > > > #include "mfx/mfxvp8.h" > > > > #endif > > > > @@ -295,6 +298,9 @@ enum AVPictureType ff_qsv_map_pictype(int > > > > mfx_pic_type) static int qsv_load_plugins(mfxSession session, const > > > > char > > > > *load_plugins, > > > > void *logctx) { > > > > +#if QSV_VERSION_ATLEAST(1, 28) > > > > + return 0; > > > > +#else > > > > if (!load_plugins || !*load_plugins) > > > > return 0; > > > > > > > > @@ -340,6 +346,7 @@ load_plugin_fail: > > > > } > > > > > > > > return 0; > > > > +#endif > > > > > > > > } > > > > > > > > > Hi, > > > > > > Are you sure this check is right? You are checking the libmfx version > > > against which ffmpeg is compiled. > > > > > > What happens, when a graphics driver supports only API level 1.21? > > > > > > > I understand your concern, however lots of features in FFmpeg are > > disabled/enabled against api version at compile-time. > > > > > > have you checked if runtime checks with QSV_RUNTIME_VERSION_ATLEAST can be > avoided? Thanks for your comment, it works with version check at runtime. However my original thought was to make sure the compilation is not broken when plugins are removed from the SDK. Regards Haihao > > regards > Max > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Xiang, Haihao > Sent: Monday, August 24, 2020 8:15 AM > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH v2 1/2] qsv: needn't load user plugin > since libmfx 1.28 > > On Fri, 2020-08-21 at 20:11 +0000, Soft Works wrote: > > > -----Original Message----- > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > > Soft Works > > > Sent: Friday, August 21, 2020 9:45 AM > > > To: FFmpeg development discussions and patches <ffmpeg- > > > devel@ffmpeg.org> > > > Subject: Re: [FFmpeg-devel] [PATCH v2 1/2] qsv: needn't load user > > > plugin since libmfx 1.28 > > > > > > > > > > > > > -----Original Message----- > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf > Of > > > > Xiang, Haihao > > > > Sent: Friday, August 21, 2020 9:29 AM > > > > To: ffmpeg-devel@ffmpeg.org > > > > Subject: Re: [FFmpeg-devel] [PATCH v2 1/2] qsv: needn't load user > > > > plugin since libmfx 1.28 > > > > > > > > On Fri, 2020-08-21 at 05:48 +0000, Soft Works wrote: > > > > > > -----Original Message----- > > > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On > Behalf > > > > > > Of > > > > > > Haihao Xiang > > > > > > Sent: Friday, August 21, 2020 7:23 AM > > > > > > To: ffmpeg-devel@ffmpeg.org > > > > > > Cc: Haihao Xiang <haihao.xiang@intel.com> > > > > > > Subject: [FFmpeg-devel] [PATCH v2 1/2] qsv: needn't load user > > > > > > plugin since libmfx 1.28 > > > > > > > > > > > > MFXVideoUSER_Load call is redundant since libmfx 1.28 > > > > > > --- > > > > > > Fixed merge conflict when applying this patch by 'git am' > > > > > > > > > > > > libavcodec/qsv.c | 9 ++++++++- > > > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/libavcodec/qsv.c b/libavcodec/qsv.c index > > > > > > 17720070f1..56a30ad642 100644 > > > > > > --- a/libavcodec/qsv.c > > > > > > +++ b/libavcodec/qsv.c > > > > > > @@ -19,7 +19,6 @@ > > > > > > */ > > > > > > > > > > > > #include <mfx/mfxvideo.h> > > > > > > -#include <mfx/mfxplugin.h> > > > > > > #include <mfx/mfxjpeg.h> > > > > > > > > > > > > #include <stdio.h> > > > > > > @@ -36,6 +35,10 @@ > > > > > > #include "avcodec.h" > > > > > > #include "qsv_internal.h" > > > > > > > > > > > > +#if !QSV_VERSION_ATLEAST(1, 28) #include <mfx/mfxplugin.h> > > > > > > +#endif > > > > > > + > > > > > > #if QSV_VERSION_ATLEAST(1, 12) #include "mfx/mfxvp8.h" > > > > > > #endif > > > > > > @@ -295,6 +298,9 @@ enum AVPictureType > ff_qsv_map_pictype(int > > > > > > mfx_pic_type) static int qsv_load_plugins(mfxSession session, > > > > > > const char *load_plugins, > > > > > > void *logctx) { > > > > > > +#if QSV_VERSION_ATLEAST(1, 28) > > > > > > + return 0; > > > > > > +#else > > > > > > if (!load_plugins || !*load_plugins) > > > > > > return 0; > > > > > > > > > > > > @@ -340,6 +346,7 @@ load_plugin_fail: > > > > > > } > > > > > > > > > > > > return 0; > > > > > > +#endif > > > > > > > > > > > > } > > > > > > > > > > > > > > > Hi, > > > > > > > > > > Are you sure this check is right? You are checking the libmfx > > > > > version against which ffmpeg is compiled. > > > > > > > > > > What happens, when a graphics driver supports only API level 1.21? > > > > > > > > > > > > > I understand your concern, however lots of features in FFmpeg are > > > > disabled/enabled against api version at compile-time. > > > > > > That is in no way an argument to break compatibility with downlevel > > > drivers without any need. Things are working fine without that patch > > > for all API versions. > > > > > > If you really want to avoid the plugin-load, then you should check > > > the driver's API level at runtime. > > > > Haihao, > > > > let me add a few more points to this subject: > > > > - Backwards-Compatibility is a key feature of the Intel Media SDK > > Your colleagues which are doing the MSDK development are continuously > > taking care to make that possible > > > > - When you look at the latest release notes (MSDK 2020 R1, API 1.32), > > you can see that 3rd, 4th and 5th gen CPUs are still supported > > The latest 3rd gen drivers are API level 1.11 > > The latest 4th and 5th gen drivers are API level 1.20 > > Also later CPUs do not always have the latest drivers (> 1.28) > > installed > > > > - Even with libmfx 1.32, it is still possible to use those CPUs or older > > driver versions, and that applies to ffmpeg as well, when it's compiled > > against libmfx 1.32 > > > > - Your change would only be legitimate if there would have been a change > > In the dispatcher code since API 1.28 that would automatically handle the > > Plugin loading for drivers < 1.28. > > But I have just gone through the recent changes to the dispatcher code > > And there doesn't exist such functionality. > > > > - For those reasons, your commit is not acceptable as it would break > > HEVC encoding for all 3,4,5 gen CPUs and later CPUs with downlevel > > Drivers > > > > What remains is the question whether there's any benefit at all in > > Avoiding the plugin load call. > > > > Does it cost time in cases when it would not be needed (driver level >= > 1.28)? > > > > - If yes, then the driver API level can be checked at runtime to avoid the > > Plugin load. > > - Otherwise, I can't imagine any reason to make any change at all > > > > What are yours? > > Hi softworkz, > > Thanks for your comment. > > - I definitely agree with you that it is important to keep backwards > compatibility. > > - I am aware that using QSV_VERSION_ATLEAST at compile time might break > backwards compatibility, but considering QSV_VERSION_ATLEAST has been > used widely at compile time, I used it in this patch too. > > - Considering that the plugins are redundant now, it is possible to deprecate > or remove these plugins in the future. I am fine not to accept this patch > presently, and will pick it back once MFXVideoUSER_Load is deprecated or > removed from the SDK in the future Hi Haihao, > - I am aware that using QSV_VERSION_ATLEAST at compile time might break > backwards compatibility, but considering QSV_VERSION_ATLEAST has been > used widely at compile time, I used it in this patch too. At the other places where this macro is being used, it's about checking whether certain new features are available at the SDK level and in many cases this is accompanied by runtime checks as well. > - Considering that the plugins are redundant now, it is possible to deprecate > or remove these plugins in the future The only thing that is deprecated are the plugins that are delivered as part of the MSDK. Two HEVC software plugins and one HEVC partial-hw plugin. The real hardware plugins have always been part of the driver and with the latest versions, it just no longer required to load them explicitly. > once MFXVideoUSER_Load is deprecated or > removed from the SDK in the future Even the latest drivers (1.31) are still using the plugin mechanism internally (see exports of mfxplugin64_hw.dll for example) and the SDK docs do not indicate an imminent deprecation of the plugin api either. > I am fine not to accept this patch presently Thank you very much for your understanding, this would have affected many users. Kind regards, softworkz
diff --git a/libavcodec/qsv.c b/libavcodec/qsv.c index 17720070f1..56a30ad642 100644 --- a/libavcodec/qsv.c +++ b/libavcodec/qsv.c @@ -19,7 +19,6 @@ */ #include <mfx/mfxvideo.h> -#include <mfx/mfxplugin.h> #include <mfx/mfxjpeg.h> #include <stdio.h> @@ -36,6 +35,10 @@ #include "avcodec.h" #include "qsv_internal.h" +#if !QSV_VERSION_ATLEAST(1, 28) +#include <mfx/mfxplugin.h> +#endif + #if QSV_VERSION_ATLEAST(1, 12) #include "mfx/mfxvp8.h" #endif @@ -295,6 +298,9 @@ enum AVPictureType ff_qsv_map_pictype(int mfx_pic_type) static int qsv_load_plugins(mfxSession session, const char *load_plugins, void *logctx) { +#if QSV_VERSION_ATLEAST(1, 28) + return 0; +#else if (!load_plugins || !*load_plugins) return 0; @@ -340,6 +346,7 @@ load_plugin_fail: } return 0; +#endif }