[FFmpeg-devel] [PATCH] lavfi: add a libplacebo filter

[Lynne]

Mar 11, 2021, 12:34 by ffmpeg at haasn.xyz:

> On Tue, 09 Mar 2021 23:55:36 +0100 Lynne <dev at lynne.ee> wrote:
>
>> It's simpler because it makes no effort to integrate with anything
>> we use.
>>
>
> To be clear, I agree that integration with the hwcontext is the ultimate
> goal. I'm just arguing about the usefulness of allowing
> vf_libplacebo to exist as-is until then. The amount of extra code to
> support a CPU filter path is negligible, so why not merge it as-is
> rather than let it rot? Do you have a good reason to NAK this patch
> as-is other than "it's slower than it could be"?
>

Yes, it's a hardware filter library that will be poorly integrated. I've said
it before, when it comes to hardware it's our policy to leave everything
to the user, right down to device selection, uploading and choosing a
hardware version of a filter. And it also duplicates functionality that
we already have "as a workaround", while it's my opinion that if there's
something lacking that prevents us from being able to integrate with
other libraries, it should be improved, rather than being left to stagnate
due to workarounds which have become the standard, with the proper
solution being relegated to obscurity.


>> There is, it's just hidden in libplacebo. If you integrate with the
>> framework code like in the previous PR, there's no duplication.
>> And no, we will not be removing the current Vulkan filters and
>> we intend to continue expanding them. Not even if you add similar
>> filters to libplacebo.
>>
>
> The duplication in libplacebo exists already for reasons independent of
> ffmpeg, so there's no wasted effort here. And this duplication of logic
> will fundamentally continue existing even if we do integrate with
> hwcontext_vulkan, because libplacebo performs its own vulkan calls one
> way or the other.
>

It's because you chose to bundle everything into libplacebo. It's hard to
not have code duplication unless we did GUI code or window initialization.
I think this was possibly a wrong direction, and a more minimal approach would
have been better for the main library, with other components like uploading,
downloading, rendering, filtering, wrappers being split into separate libraries.
That way libplacebo's API would have been less monolithic and more
accommodating to API users who don't want to have everything handled
for them.


> (Also, as I argued before, it's not strictly speaking a duplication
> because the context creation code in libplacebo is technically different
> from the one in libavutil - notably, libplacebo enables a different set
> of extensions and features.)
>

Which we can easily update.
I think our queue hwcontext code and image uploading/downloading
is also done better than what libplacebo does.


>> It's a good approach since API users work with VkImages. It's more
>> complex because it's explicit.
>>
>
> It may be a good approach in principle but it doesn't seem to be working
> in practice, which is my concern. The downside of vulkan's explicitness
> is that it's harder to share vulkan state across API or ABI boundaries,
> and as the synchronization issue highlights, getting this API right
> seems to be nontrivial enough that it's preventing me from using
> hwcontext_vulkan.
>

We designed the APIs to work together, and you already submitted
an older version of the patch which used the API. It's me who blocked
it because I wanted to rely on having proper synchronization.

If you want to blame me for not working on it right away and fixing it
2 months ago, feel free to PM me. But please, do not use patches
like these to force me to do it.


> Khronos specifically points out that they intended for existing
> platform-specific resource sharing mechanisms (fds and handles) to be
> used to communicate VkImages across process boundaries or between
> VkDevice instances, which is why VK_KHR_external_memory exists.
>

Even if they do (which I don't recall reading about, and no, the APIs
being there do not indicate that's their intended purpose), why bother
with external image representations when you still have to follow the
exact same rules regarding synchronization as everything else?
Also, it gets better - some platforms (like Linux) completely lack synchronization
primitives for their images. There's no API, no way to link a semaphore
to an FD, nothing. So it would be worse than having difficult to get
synchronization, unless you wanted to see screen tearing in lavfi.


>> With an extension. Only supported on Linux. Only supported on recent
>> AMD cards and drivers. Which is also buggy and crashes drivers. And
>> the Intel implementation has been sitting as a PR for more than 3 years!
>> You may have the privilege of using it because you lucked out on
>> hardware, but the rest of us are not so lucky.
>>
>
> (I'm not sure what you mean - what extension is required? Are you talking
> about DRM modifier support inside vulkan? Those work fine on intel, and
> now also AMD.)
>

Those do NOT work fine on Intel. If you had any contact with lower levels
of hardware you'd have known. Modifiers work in that a SINGLE modifier
is assumed (X tiling IIRC) and all others are ignored. So screen sharing
doesn't work if your buffer is compressed or uses Y tiling. VAAPI to
Vulkan importing works purely out of coincidence.


> That said, I changed my mind and agree that something inherently
> Linux-only is probably not a long-term solution, so we definitely do
> need some vendor-agnostic vulkan-based sharing API. Whether it's based
> on sharing VkImages directly or by sharing vulkan memory objects
> indirectly is not as important as that it works. But I guess let's not
> confuse the issue of how to fix hwcontext_vulkan with the issue of
> whether it's okay to merge this patch as-is for the time being.
>

It's not okay to merge as long as it has software frame support still enabled.