| Message ID | 20230511083902.7262-1-ffmpeg@haasn.xyz |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v2] lavfi/vf_libplacebo: add vulkan device import fallback | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
May 11, 2023, 10:39 by ffmpeg@haasn.xyz: > From: Niklas Haas <git@haasn.dev> > > Recent versions of libplacebo have required Vulkan versions incompatible > with lavu Vulkan hwcontexts. While this is expected to change > eventually, breaking vf_libplacebo every time there is such a transition > period is obviously undesired behavior, as the following sea of bug > reports shows. > > This commit adds a fallback path for pl_vulkan_import failures which > simply creates an internal device instead. Also useful when no interop > with lavu vulkan hwframes is needed or desired. > > Fixes: https://github.com/haasn/libplacebo/issues/170 > Fixes: https://github.com/mpv-player/mpv/issues/9589#issuecomment-1535432185 > Fixes: https://code.videolan.org/videolan/libplacebo/-/issues/270 > NAK. The whole point of the hwcontext infrastructure is to be explicit, and creating a new device behind the scenes is anything but that. Granted, this period of instability has gone too far, but I think we should work together to make sure there's interoperability, and after my patchset gets merged, there won't be much need to change anything in terms of enabled features for a while. Both libplacebo and the hwcontext have had a ton of changes recently, but before we started making changes, they were working fine for over a year.
Quoting Niklas Haas (2023-05-11 10:39:02) > From: Niklas Haas <git@haasn.dev> > > Recent versions of libplacebo have required Vulkan versions incompatible > with lavu Vulkan hwcontexts. Why did that happen? Did something change in lavu that broke things?
On Thu, 11 May 2023 11:53:55 +0200 Anton Khirnov <anton@khirnov.net> wrote: > Quoting Niklas Haas (2023-05-11 10:39:02) > > From: Niklas Haas <git@haasn.dev> > > > > Recent versions of libplacebo have required Vulkan versions incompatible > > with lavu Vulkan hwcontexts. > > Why did that happen? Did something change in lavu that broke things? No, upstream libplacebo dropped support, during a code refactor/simplification, for contexts without various features that used to be optional. That refactor is not yet in any tagged release, but it breaks current master+master building. Easiest alternative fix available right now would be to merge the vulkan lavu hwcontext changes that have been on ML for weeks (and gotten a LGTM from me). Alternatively, I could try and back-port enabling of whatever extensions and features are required onto the current Vulkan code. I understand not wanting random filters to open hardware devices under the hood, but I also think it's a bit silly to have to constantly synchronize requirements between Ffmpeg and libplacebo like this. If both have such tight requirements on device features and extensions, it makes more sense to me if each takes care of their own respective device creation. Or maybe FFmpeg vulkan could directly inspect `pl_vulkan_required_features` and enable whatever's listed in there, for forward compatibility.
On Thu, May 11, 2023 at 11:32 AM Lynne <dev@lynne.ee> wrote: > > May 11, 2023, 10:39 by ffmpeg@haasn.xyz: > > > From: Niklas Haas <git@haasn.dev> > > > > Recent versions of libplacebo have required Vulkan versions incompatible > > with lavu Vulkan hwcontexts. While this is expected to change > > eventually, breaking vf_libplacebo every time there is such a transition > > period is obviously undesired behavior, as the following sea of bug > > reports shows. > > > > This commit adds a fallback path for pl_vulkan_import failures which > > simply creates an internal device instead. Also useful when no interop > > with lavu vulkan hwframes is needed or desired. > > > > Fixes: https://github.com/haasn/libplacebo/issues/170 > > Fixes: https://github.com/mpv-player/mpv/issues/9589#issuecomment-1535432185 > > Fixes: https://code.videolan.org/videolan/libplacebo/-/issues/270 > > > > NAK. The whole point of the hwcontext infrastructure is to be > explicit, and creating a new device behind the scenes is anything but that. > > This is not a native vulkan filter, it is an external library not married to our vulkan infrastructure - it merely has compatibility for it. Ensuring it works, as it might have separate requirements or a different development cycle, is hardly a bad thing. And it greatly simplifies the usability for users that only want a quick GPU processing. - Hendrik
On Thu, 11 May 2023 at 14:21, Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Thu, May 11, 2023 at 11:32 AM Lynne <dev@lynne.ee> wrote: > > > > May 11, 2023, 10:39 by ffmpeg@haasn.xyz: > > > > > From: Niklas Haas <git@haasn.dev> > > > > > > Recent versions of libplacebo have required Vulkan versions > incompatible > > > with lavu Vulkan hwcontexts. While this is expected to change > > > eventually, breaking vf_libplacebo every time there is such a > transition > > > period is obviously undesired behavior, as the following sea of bug > > > reports shows. > > > > > > This commit adds a fallback path for pl_vulkan_import failures which > > > simply creates an internal device instead. Also useful when no interop > > > with lavu vulkan hwframes is needed or desired. > > > > > > Fixes: https://github.com/haasn/libplacebo/issues/170 > > > Fixes: > https://github.com/mpv-player/mpv/issues/9589#issuecomment-1535432185 > > > Fixes: https://code.videolan.org/videolan/libplacebo/-/issues/270 > > > > > > > NAK. The whole point of the hwcontext infrastructure is to be > > explicit, and creating a new device behind the scenes is anything but > that. > > > > > > This is not a native vulkan filter, it is an external library not > married to our vulkan infrastructure - it merely has compatibility for > it. > > Ensuring it works, as it might have separate requirements or a > different development cycle, is hardly a bad thing. And it greatly > simplifies the usability for users that only want a quick GPU > processing. > > - Hendrik > And related to this: We also have multiple filters that can create random devices on demand, a prime example being hwupload_cuda and hwmap (depending on the derivation mode used). Rejecting this patch on the basis of "creating devices behind the scenes" disregards the context and purpose in which the aforementioned device is created, such as in cases where no interop with lavu vulkan hwframes is needed or desired. Without this patch, libplacebo will keep breaking quite frequently with changes to the vk hwcontext code. The maintenance burden, especially on documentation for when working command-lines fail (and they will, without this patch) will be significant. Something to consider, considering how fragile the Vulkan support is in FFmpeg. Warm regards, Dennis.
May 11, 2023, 13:21 by h.leppkes@gmail.com: > On Thu, May 11, 2023 at 11:32 AM Lynne <dev@lynne.ee> wrote: > >> >> May 11, 2023, 10:39 by ffmpeg@haasn.xyz: >> >> > From: Niklas Haas <git@haasn.dev> >> > >> > Recent versions of libplacebo have required Vulkan versions incompatible >> > with lavu Vulkan hwcontexts. While this is expected to change >> > eventually, breaking vf_libplacebo every time there is such a transition >> > period is obviously undesired behavior, as the following sea of bug >> > reports shows. >> > >> > This commit adds a fallback path for pl_vulkan_import failures which >> > simply creates an internal device instead. Also useful when no interop >> > with lavu vulkan hwframes is needed or desired. >> > >> > Fixes: https://github.com/haasn/libplacebo/issues/170 >> > Fixes: https://github.com/mpv-player/mpv/issues/9589#issuecomment-1535432185 >> > Fixes: https://code.videolan.org/videolan/libplacebo/-/issues/270 >> > >> >> NAK. The whole point of the hwcontext infrastructure is to be >> explicit, and creating a new device behind the scenes is anything but that. >> > > This is not a native vulkan filter, it is an external library not > married to our vulkan infrastructure - it merely has compatibility for > it. > > Ensuring it works, as it might have separate requirements or a > different development cycle, is hardly a bad thing. And it greatly > simplifies the usability for users that only want a quick GPU > processing. > vf_libplacebo already creates its own device for software, vaapi and DRM frames, this patch is specifically for vulkan input frames, which have a vulkan device/frames context. If the vulkan device with the frames is unusable, it is either intentional (if given by the API users) or a bug (if using a context created by lavu). Masking over which is not appropriate.
On Thu, 11 May 2023 14:01:34 +0200 Lynne <dev@lynne.ee> wrote: > May 11, 2023, 13:21 by h.leppkes@gmail.com: > > > On Thu, May 11, 2023 at 11:32 AM Lynne <dev@lynne.ee> wrote: > > > >> > >> May 11, 2023, 10:39 by ffmpeg@haasn.xyz: > >> > >> > From: Niklas Haas <git@haasn.dev> > >> > > >> > Recent versions of libplacebo have required Vulkan versions incompatible > >> > with lavu Vulkan hwcontexts. While this is expected to change > >> > eventually, breaking vf_libplacebo every time there is such a transition > >> > period is obviously undesired behavior, as the following sea of bug > >> > reports shows. > >> > > >> > This commit adds a fallback path for pl_vulkan_import failures which > >> > simply creates an internal device instead. Also useful when no interop > >> > with lavu vulkan hwframes is needed or desired. > >> > > >> > Fixes: https://github.com/haasn/libplacebo/issues/170 > >> > Fixes: https://github.com/mpv-player/mpv/issues/9589#issuecomment-1535432185 > >> > Fixes: https://code.videolan.org/videolan/libplacebo/-/issues/270 > >> > > >> > >> NAK. The whole point of the hwcontext infrastructure is to be > >> explicit, and creating a new device behind the scenes is anything but that. > >> > > > > This is not a native vulkan filter, it is an external library not > > married to our vulkan infrastructure - it merely has compatibility for > > it. > > > > Ensuring it works, as it might have separate requirements or a > > different development cycle, is hardly a bad thing. And it greatly > > simplifies the usability for users that only want a quick GPU > > processing. > > > > vf_libplacebo already creates its own device for software, vaapi and DRM > frames, this patch is specifically for vulkan input frames, which have > a vulkan device/frames context. This is not true. Currently, if vf_libplacebo is not provided a working vulkan device, it fails initializing entirely. Which raises an excellent point: Without this patch, interop with vaapi etc. frames is not possible unless the user *also* initializes a vulkan device *and* ensures it's passed to the vf_libplacebo filter (e.g. via -filter_hw_device). And interop with non-vulkan hardware filters is not possible at all, unless there's a way to specify a different hwdevice for different filters? > If the vulkan device with the frames is unusable, it is either intentional > (if given by the API users) or a bug (if using a context created by lavu). > Masking over which is not appropriate. Your analysis omits an important third case: ./ffmpeg -i FILE.mp4 -vf libplacebo OUTPUT.mp4 One might be forgiven a naive user expecting this command to work as written. But before this patch, it does not - one must *also* specify -init_hw_device vulkan, even if not using any `_vulkan` filters! This is IMO very unexpected, and most definitely (in all likelihood) not the user intent.
diff --git a/libavfilter/vf_libplacebo.c b/libavfilter/vf_libplacebo.c index 6fe3e0ea88..74ea3cbcc5 100644 --- a/libavfilter/vf_libplacebo.c +++ b/libavfilter/vf_libplacebo.c @@ -351,58 +351,50 @@ fail: return err; } -static int init_vulkan(AVFilterContext *avctx) +static int init_vulkan(AVFilterContext *avctx, const AVVulkanDeviceContext *hwctx) { int err = 0; LibplaceboContext *s = avctx->priv; - const AVHWDeviceContext *avhwctx; - const AVVulkanDeviceContext *hwctx; uint8_t *buf = NULL; size_t buf_len; - if (!avctx->hw_device_ctx) { - av_log(s, AV_LOG_ERROR, "Missing vulkan hwdevice for vf_libplacebo.\n"); - return AVERROR(EINVAL); - } - - avhwctx = (AVHWDeviceContext *) avctx->hw_device_ctx->data; - if (avhwctx->type != AV_HWDEVICE_TYPE_VULKAN) { - av_log(s, AV_LOG_ERROR, "Expected vulkan hwdevice for vf_libplacebo, got %s.\n", - av_hwdevice_get_type_name(avhwctx->type)); - return AVERROR(EINVAL); + if (hwctx) { + /* Import libavfilter vulkan context into libplacebo */ + s->vulkan = pl_vulkan_import(s->log, pl_vulkan_import_params( + .instance = hwctx->inst, + .get_proc_addr = hwctx->get_proc_addr, + .phys_device = hwctx->phys_dev, + .device = hwctx->act_dev, + .extensions = hwctx->enabled_dev_extensions, + .num_extensions = hwctx->nb_enabled_dev_extensions, + .features = &hwctx->device_features, + .queue_graphics = { + .index = hwctx->queue_family_index, + .count = hwctx->nb_graphics_queues, + }, + .queue_compute = { + .index = hwctx->queue_family_comp_index, + .count = hwctx->nb_comp_queues, + }, + .queue_transfer = { + .index = hwctx->queue_family_tx_index, + .count = hwctx->nb_tx_queues, + }, + /* This is the highest version created by hwcontext_vulkan.c */ + .max_api_version = VK_API_VERSION_1_2, + )); } - hwctx = avhwctx->hwctx; - - /* Import libavfilter vulkan context into libplacebo */ - s->vulkan = pl_vulkan_import(s->log, pl_vulkan_import_params( - .instance = hwctx->inst, - .get_proc_addr = hwctx->get_proc_addr, - .phys_device = hwctx->phys_dev, - .device = hwctx->act_dev, - .extensions = hwctx->enabled_dev_extensions, - .num_extensions = hwctx->nb_enabled_dev_extensions, - .features = &hwctx->device_features, - .queue_graphics = { - .index = hwctx->queue_family_index, - .count = hwctx->nb_graphics_queues, - }, - .queue_compute = { - .index = hwctx->queue_family_comp_index, - .count = hwctx->nb_comp_queues, - }, - .queue_transfer = { - .index = hwctx->queue_family_tx_index, - .count = hwctx->nb_tx_queues, - }, - /* This is the highest version created by hwcontext_vulkan.c */ - .max_api_version = VK_API_VERSION_1_2, - )); - if (!s->vulkan) { - av_log(s, AV_LOG_ERROR, "Failed importing vulkan device to libplacebo!\n"); - err = AVERROR_EXTERNAL; - goto fail; + s->vulkan = pl_vulkan_create(s->log, pl_vulkan_params( + .get_proc_addr = hwctx ? hwctx->get_proc_addr : NULL, + .queue_count = 0, /* enable all queues for parallelization */ + )); + if (!s->vulkan) { + av_log(s, AV_LOG_ERROR, "Failed creating fallback vulkan device!\n"); + err = AVERROR_EXTERNAL; + goto fail; + } } /* Create the renderer */ @@ -695,10 +687,17 @@ static int libplacebo_query_format(AVFilterContext *ctx) { int err; LibplaceboContext *s = ctx->priv; + const AVVulkanDeviceContext *vkhwctx = NULL; const AVPixFmtDescriptor *desc = NULL; AVFilterFormats *infmts = NULL, *outfmts = NULL; - RET(init_vulkan(ctx)); + if (ctx->hw_device_ctx) { + const AVHWDeviceContext *avhwctx = (void *) ctx->hw_device_ctx->data; + if (avhwctx->type == AV_HWDEVICE_TYPE_VULKAN) + vkhwctx = avhwctx->hwctx; + } + + RET(init_vulkan(ctx, vkhwctx)); while ((desc = av_pix_fmt_desc_next(desc))) { enum AVPixelFormat pixfmt = av_pix_fmt_desc_get_id(desc); @@ -710,6 +709,11 @@ static int libplacebo_query_format(AVFilterContext *ctx) continue; #endif + if (pixfmt == AV_PIX_FMT_VULKAN) { + if (!vkhwctx || vkhwctx->act_dev != s->vulkan->device) + continue; + } + if (!pl_test_pixfmt(s->gpu, pixfmt)) continue;
From: Niklas Haas <git@haasn.dev> Recent versions of libplacebo have required Vulkan versions incompatible with lavu Vulkan hwcontexts. While this is expected to change eventually, breaking vf_libplacebo every time there is such a transition period is obviously undesired behavior, as the following sea of bug reports shows. This commit adds a fallback path for pl_vulkan_import failures which simply creates an internal device instead. Also useful when no interop with lavu vulkan hwframes is needed or desired. Fixes: https://github.com/haasn/libplacebo/issues/170 Fixes: https://github.com/mpv-player/mpv/issues/9589#issuecomment-1535432185 Fixes: https://code.videolan.org/videolan/libplacebo/-/issues/270 --- libavfilter/vf_libplacebo.c | 92 +++++++++++++++++++------------------ 1 file changed, 48 insertions(+), 44 deletions(-)