Message ID | 20210629014538.3065929-1-haihao.xiang@intel.com |
---|---|
State | Accepted |
Commit | 6803d8b4403ecea18fc288475b4d7cdedd0fb744 |
Headers | show |
Series | [FFmpeg-devel] ffmpeg_hw: make hardware selection for filters more user friendly | expand |
Context | Check | Description |
---|---|---|
andriy/x86_make | success | Make finished |
andriy/x86_make_fate | success | Make fate finished |
andriy/PPC64_make | success | Make finished |
andriy/PPC64_make_fate | success | Make fate finished |
> -----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
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) {