[FFmpeg-devel] ffmpeg_hw: make hardware selection for filters more user friendly

When a device is derived from a source device, there are at least 2
devices, and usually the derived device is the expected device, so let's
pick the last device if user doesn't specify the filter device with
filter_hw_device option

After applying this patch, the command below can work:

$> ffmpeg -init_hw_device vaapi=va:/dev/dri/renderD128 -init_hw_device
qsv=hw@va -f lavfi -i yuvtestsrc  -vf
format=nv12,hwupload=extra_hw_frames=64 -c:v h264_qsv -y out.h264
---
 fftools/ffmpeg_hw.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Haihao Xiang
> Sent: Tuesday, 29 June 2021 03:46
> To: ffmpeg-devel@ffmpeg.org
> Cc: Haihao Xiang <haihao.xiang@intel.com>
> Subject: [FFmpeg-devel] [PATCH] ffmpeg_hw: make hardware selection for
> filters more user friendly
> 
> When a device is derived from a source device, there are at least 2 devices,
> and usually the derived device is the expected device, so let's pick the last
> device if user doesn't specify the filter device with filter_hw_device option
> 
> After applying this patch, the command below can work:
> 
> $> ffmpeg -init_hw_device vaapi=va:/dev/dri/renderD128 -init_hw_device
> qsv=hw@va -f lavfi -i yuvtestsrc  -vf
> format=nv12,hwupload=extra_hw_frames=64 -c:v h264_qsv -y out.h264
> ---
>  fftools/ffmpeg_hw.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/fftools/ffmpeg_hw.c b/fftools/ffmpeg_hw.c index
> fc4a5d31d6..14e2cb0177 100644
> --- a/fftools/ffmpeg_hw.c
> +++ b/fftools/ffmpeg_hw.c
> @@ -527,15 +527,21 @@ int hw_device_setup_for_filter(FilterGraph *fg)
>      HWDevice *dev;
>      int i;
> 
> -    // If the user has supplied exactly one hardware device then just
> -    // give it straight to every filter for convenience.  If more than
> -    // one device is available then the user needs to pick one explcitly
> -    // with the filter_hw_device option.
> +    // Pick the last hardware device if the user doesn't pick the device for
> +    // filters explicitly with the filter_hw_device option.
>      if (filter_hw_device)
>          dev = filter_hw_device;
> -    else if (nb_hw_devices == 1)
> -        dev = hw_devices[0];
> -    else
> +    else if (nb_hw_devices > 0) {
> +        dev = hw_devices[nb_hw_devices - 1];
> +
> +        if (nb_hw_devices > 1)
> +            av_log(NULL, AV_LOG_WARNING, "There are %d hardware devices.
> device "
> +                   "%s of type %s is picked for filters by default. Set hardware "
> +                   "device explicitly with the filter_hw_device option if device "
> +                   "%s is not usable for filters.\n",
> +                   nb_hw_devices, dev->name,
> +                   av_hwdevice_get_type_name(dev->type), dev->name);
> +    } else
>          dev = NULL;
> 
>      if (dev) {
> --

Haihao,

Thanks for the patch, I hadn't seen it. Due to working off an older baseline version,  I wasn't actually aware of the current code which had been added by Mark Thompson in April 2020: 

- when a single hw device is initialized, all filters are automatically set to that device context
- when more than one device is initialized, it silently does nothing

This leads to the following behavior: you have a working command line with a single device. Then you add initialization for a second device and the command line stops working.

@mark - would you mind to comment?
I suppose the idea was that when there's more than a single device, the user needs to set filter_hw_device explicitly. But IMO, this doesn't go well together with making an automatic selection: either there's always an automatic selection or never, but the current behavior is confusing.

Now, that there's already an automatic selection for cases with a single device, the "never" option is practically off the table. For those reasons, Haihao's patch would LGTM.

softworkz

On Wed, 2021-08-04 at 05:17 +0000, Soft Works wrote:
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > Haihao Xiang
> > Sent: Tuesday, 29 June 2021 03:46
> > To: ffmpeg-devel@ffmpeg.org
> > Cc: Haihao Xiang <haihao.xiang@intel.com>
> > Subject: [FFmpeg-devel] [PATCH] ffmpeg_hw: make hardware selection for
> > filters more user friendly
> > 
> > When a device is derived from a source device, there are at least 2 devices,
> > and usually the derived device is the expected device, so let's pick the
> > last
> > device if user doesn't specify the filter device with filter_hw_device
> > option
> > 
> > After applying this patch, the command below can work:
> > 
> > $> ffmpeg -init_hw_device vaapi=va:/dev/dri/renderD128 -init_hw_device
> > qsv=hw@va -f lavfi -i yuvtestsrc  -vf
> > format=nv12,hwupload=extra_hw_frames=64 -c:v h264_qsv -y out.h264
> > ---
> >  fftools/ffmpeg_hw.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fftools/ffmpeg_hw.c b/fftools/ffmpeg_hw.c index
> > fc4a5d31d6..14e2cb0177 100644
> > --- a/fftools/ffmpeg_hw.c
> > +++ b/fftools/ffmpeg_hw.c
> > @@ -527,15 +527,21 @@ int hw_device_setup_for_filter(FilterGraph *fg)
> >      HWDevice *dev;
> >      int i;
> > 
> > -    // If the user has supplied exactly one hardware device then just
> > -    // give it straight to every filter for convenience.  If more than
> > -    // one device is available then the user needs to pick one explcitly
> > -    // with the filter_hw_device option.
> > +    // Pick the last hardware device if the user doesn't pick the device
> > for
> > +    // filters explicitly with the filter_hw_device option.
> >      if (filter_hw_device)
> >          dev = filter_hw_device;
> > -    else if (nb_hw_devices == 1)
> > -        dev = hw_devices[0];
> > -    else
> > +    else if (nb_hw_devices > 0) {
> > +        dev = hw_devices[nb_hw_devices - 1];
> > +
> > +        if (nb_hw_devices > 1)
> > +            av_log(NULL, AV_LOG_WARNING, "There are %d hardware devices.
> > device "
> > +                   "%s of type %s is picked for filters by default. Set
> > hardware "
> > +                   "device explicitly with the filter_hw_device option if
> > device "
> > +                   "%s is not usable for filters.\n",
> > +                   nb_hw_devices, dev->name,
> > +                   av_hwdevice_get_type_name(dev->type), dev->name);
> > +    } else
> >          dev = NULL;
> > 
> >      if (dev) {
> > --
> 
> Haihao,
> 
> Thanks for the patch, I hadn't seen it. Due to working off an older baseline
> version,  I wasn't actually aware of the current code which had been added by
> Mark Thompson in April 2020: 
> 
> - when a single hw device is initialized, all filters are automatically set to
> that device context
> - when more than one device is initialized, it silently does nothing
> 
> This leads to the following behavior: you have a working command line with a
> single device. Then you add initialization for a second device and the command
> line stops working.
> 
> @mark - would you mind to comment?
> I suppose the idea was that when there's more than a single device, the user
> needs to set filter_hw_device explicitly. But IMO, this doesn't go well
> together with making an automatic selection: either there's always an
> automatic selection or never, but the current behavior is confusing.
> 
> Now, that there's already an automatic selection for cases with a single
> device, the "never" option is practically off the table. For those reasons,
> Haihao's patch would LGTM.
> 

Could someone help to merge this patch if no more comment ?

Thanks
Haihao

On Thu, 2021-08-19 at 04:44 +0000, Xiang, Haihao wrote:
> On Wed, 2021-08-04 at 05:17 +0000, Soft Works wrote:
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > > Haihao Xiang
> > > Sent: Tuesday, 29 June 2021 03:46
> > > To: ffmpeg-devel@ffmpeg.org
> > > Cc: Haihao Xiang <haihao.xiang@intel.com>
> > > Subject: [FFmpeg-devel] [PATCH] ffmpeg_hw: make hardware selection for
> > > filters more user friendly
> > > 
> > > When a device is derived from a source device, there are at least 2
> > > devices,
> > > and usually the derived device is the expected device, so let's pick the
> > > last
> > > device if user doesn't specify the filter device with filter_hw_device
> > > option
> > > 
> > > After applying this patch, the command below can work:
> > > 
> > > $> ffmpeg -init_hw_device vaapi=va:/dev/dri/renderD128 -init_hw_device
> > > qsv=hw@va -f lavfi -i yuvtestsrc  -vf
> > > format=nv12,hwupload=extra_hw_frames=64 -c:v h264_qsv -y out.h264
> > > ---
> > >  fftools/ffmpeg_hw.c | 20 +++++++++++++-------
> > >  1 file changed, 13 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/fftools/ffmpeg_hw.c b/fftools/ffmpeg_hw.c index
> > > fc4a5d31d6..14e2cb0177 100644
> > > --- a/fftools/ffmpeg_hw.c
> > > +++ b/fftools/ffmpeg_hw.c
> > > @@ -527,15 +527,21 @@ int hw_device_setup_for_filter(FilterGraph *fg)
> > >      HWDevice *dev;
> > >      int i;
> > > 
> > > -    // If the user has supplied exactly one hardware device then just
> > > -    // give it straight to every filter for convenience.  If more than
> > > -    // one device is available then the user needs to pick one explcitly
> > > -    // with the filter_hw_device option.
> > > +    // Pick the last hardware device if the user doesn't pick the device
> > > for
> > > +    // filters explicitly with the filter_hw_device option.
> > >      if (filter_hw_device)
> > >          dev = filter_hw_device;
> > > -    else if (nb_hw_devices == 1)
> > > -        dev = hw_devices[0];
> > > -    else
> > > +    else if (nb_hw_devices > 0) {
> > > +        dev = hw_devices[nb_hw_devices - 1];
> > > +
> > > +        if (nb_hw_devices > 1)
> > > +            av_log(NULL, AV_LOG_WARNING, "There are %d hardware devices.
> > > device "
> > > +                   "%s of type %s is picked for filters by default. Set
> > > hardware "
> > > +                   "device explicitly with the filter_hw_device option if
> > > device "
> > > +                   "%s is not usable for filters.\n",
> > > +                   nb_hw_devices, dev->name,
> > > +                   av_hwdevice_get_type_name(dev->type), dev->name);
> > > +    } else
> > >          dev = NULL;
> > > 
> > >      if (dev) {
> > > --
> > 
> > Haihao,
> > 
> > Thanks for the patch, I hadn't seen it. Due to working off an older baseline
> > version,  I wasn't actually aware of the current code which had been added
> > by
> > Mark Thompson in April 2020: 
> > 
> > - when a single hw device is initialized, all filters are automatically set
> > to
> > that device context
> > - when more than one device is initialized, it silently does nothing
> > 
> > This leads to the following behavior: you have a working command line with a
> > single device. Then you add initialization for a second device and the
> > command
> > line stops working.
> > 
> > @mark - would you mind to comment?
> > I suppose the idea was that when there's more than a single device, the user
> > needs to set filter_hw_device explicitly. But IMO, this doesn't go well
> > together with making an automatic selection: either there's always an
> > automatic selection or never, but the current behavior is confusing.
> > 
> > Now, that there's already an automatic selection for cases with a single
> > device, the "never" option is practically off the table. For those reasons,
> > Haihao's patch would LGTM.
> > 
> 
> Could someone help to merge this patch if no more comment ?

Ping again.

Thanks
Haihao