diff mbox series

[FFmpeg-devel,v2,1/2] qsv: needn't load user plugin since libmfx 1.28

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

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Xiang, Haihao Aug. 21, 2020, 5:23 a.m. UTC 
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(-)

Comments

Soft Works Aug. 21, 2020, 5:48 a.m. UTC | #1 
> -----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
Xiang, Haihao Aug. 21, 2020, 7:29 a.m. UTC | #2 
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".
Max Dmitrichenko Aug. 21, 2020, 7:36 a.m. UTC | #3 
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
Soft Works Aug. 21, 2020, 7:45 a.m. UTC | #4 
> -----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
Soft Works Aug. 21, 2020, 8:11 p.m. UTC | #5 
> -----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
Xiang, Haihao Aug. 24, 2020, 6:15 a.m. UTC | #6 
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".
Xiang, Haihao Aug. 24, 2020, 6:31 a.m. UTC | #7 
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".
Soft Works Aug. 24, 2020, 5:54 p.m. UTC | #8 
> -----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 mbox series

Patch

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
 
 }