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

[Xiang, Haihao]

On Fri, 2020-08-21 at 20:11 +0000, Soft Works wrote:
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> > Soft Works
> > Sent: Friday, August 21, 2020 9:45 AM
> > To: FFmpeg development discussions and patches <ffmpeg-
> > devel at 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 at ffmpeg.org> On Behalf Of
> > > Xiang, Haihao
> > > Sent: Friday, August 21, 2020 9:29 AM
> > > To: ffmpeg-devel at 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 at ffmpeg.org> On Behalf
> > 
> > Of
> > > > > Haihao Xiang
> > > > > Sent: Friday, August 21, 2020 7:23 AM
> > > > > To: ffmpeg-devel at ffmpeg.org
> > > > > Cc: Haihao Xiang <haihao.xiang at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".