[RFC] Treat all types in the Vulkan namespace as fundamental

[MarijnS95]

All Vulkan handles, enumerations, bitfields and structs in G-IR reflection have one fundamental flaw: they are empty records. As such gir is unable to generate anything representative for them and treats them as incomplete instead, breaking pretty much every struct that needs to use these to do anything with it.

Instead of trying to fill this reflection with the right fields and types we should opt to map them to a Rust crate that already provides bindings directly to Vulkan. Introducing Ash, Rust's most widely used Vulkan wrapper crate (disclaimer: I'm an active maintainer). This PR takes care of mapping the types properly. Most notably it makes sure that "safe" and FFI types are the same, instead of generating conversion code with .to_glib_none() and friends - that makes no sense at all.

An initial user of these bindings is GStreamer, but it's very much work-in-progress: https://gitlab.freedesktop.org/MarijnS95/gstreamer-rs/-/tree/vulkan. With this massive issue out of the way though I hope to get started on a Vulkan pipeline and window in Rust (glwindow equivalent) rather soon.

Things to discuss:

• Could Type::Custom be used for this as well (iow: did I just invent something that already exists in a different form already, without explicitly adding a new enum variant)?;
• Infer types from usage (everything touching the Vulkan. namespace) and/or Vulkan-1.0.gir instead of hardcoding them in const VULKAN;
• Treat strings in Fundamental::Vulkan as &'static str so that all this borrowing churn isn't necessary (not possible if we parse types from G-IR);
  OTOH some of the * and ref removal is actually quite a nice simplification!
• Map types to a vulkan/vulkan_sys crate in gtk-rs-core instead (representing Vulkan-1.0.gir, this is where I "started" but that's the broken approach). This crate is then solely responsible for selecting an Ash version and only consists of pub use ash::vk::*;. Of course it can more easily map to a different crate as well if desired, assuming names are the same (ie. stripped of their vk prefix). Where necessary it would be a nice place for generic Vulkan-related helpers, re-mapping types that are wrongly named if any, etc.

[sdroege]

All Vulkan handles, enumerations, bitfields and structs in G-IR reflection have one fundamental flaw: they are empty records. As such gir is unable to generate anything representative for them and treats them as incomplete instead, breaking pretty much every struct that needs to use these to do anything with it.

How is it different for GL, and can the Vulkan-1.0.gir maybe be changed the same way?

[MarijnS95]

How is it different for GL, and can the Vulkan-1.0.gir maybe be changed the same way?

None of the GL types appear to be used in public API anywhere (try searching for those names in other .gir files). If they were this would present the same issue.

[sdroege]

* Map types to a `vulkan`/`vulkan_sys` crate in `gtk-rs-core` instead

As discussed on IRC, let's go with that because also GTK4 will likely need such a thing sooner or later.

[sdroege]

* Treat strings in `Fundamental::Vulkan` as `&'static str` so that all this borrowing churn isn't necessary (not possible if we parse types from G-IR)

I'd treat them as such by using some string interning library. That's going to make all this simpler (although the "borrowing churn" is in now, it's independent of this really).

[sdroege]

Infer types from usage (everything touching the Vulkan. namespace) and/or Vulkan-1.0.gir instead of hardcoding them in const VULKAN;

I don't know what this means exactly, but I prefer no hardcoding and you should be able to just go via namespace or not? :)

[MarijnS95]

Infer types from usage (everything touching the Vulkan. namespace) and/or Vulkan-1.0.gir instead of hardcoding them in const VULKAN;

I don't know what this means exactly, but I prefer no hardcoding and you should be able to just go via namespace or not? :)

See the massive list in const VULKAN: &[&str] = &[, I want to automate that one way or another:

• Read from Vulkan-1.0.gir and treated specially (when generating a vulkan crate in gtk-rs-core);
• "Assumed" to exist whenever other bindings use anything from the Vulkan. namespace.

I'd treat them as such by using some string interning library. That's going to make all this simpler (although the "borrowing churn" is in now, it's independent of this really).

We'll see, probably going to keep it owned String assuming we'll parse the names from Vulkan-1.0.gir.

I'll give this another shot later this weekend.

[spencercw]

Hi @MarijnS95. Don't suppose you have any capacity to progress this? Getting gstreamer-vulkan pushed through (even if just the -sys bindings) would be very useful, and it looks like this is the main blocker at the moment.

[MarijnS95]

@spencercw I'm willing to invent time for it as I'd hate to see all this effort thus far go to waste.
(You probably worked that out from running into me in quite a few other repositories/projects... Hi again!)

For starters I think I should rebase all relevant branches. Then we can go from there, and this (binding Vulkan-1.0.gir) is indeed the biggest blocker, because that autogenerated .gir file underspecifies the type.

We should either extend the file to include relevant type information, and/or extend the gir Rust tooling here.

[spencercw]

Good stuff, thanks. Yes your name seems to come up all over the place 😂

I'm not particularly familiar with the gstreamer stack, but might be able to help fill in some of the details once you've got the fundamentals in place.

[MarijnS95]

You're welcome 🙂

Conveniently I got back into gstreamer-rs development just two weeks ago by refactoring the GL example to use the new winit-less glutin 0.31 crate - and have a proof-of-concept working Android build for it 😁

---

For starters, if you're interested in just -sys bindings my branch has been rebased just over a year ago and might still provide some value:

https://gitlab.freedesktop.org/MarijnS95/gstreamer-rs/-/tree/vulkan

Meanwhile I'll see what/when can be done to rebase these branches and figure something out for gir.

[spencercw]

Thanks, yes I found your branch yesterday and was able to hack together a working -sys, but I'm not enjoying using the unsafe C bindings directly! A real gstreamer-vulkan package would be most welcome 🙂

[MarijnS95]

@spencercw I went through rebasing all my branches and many tangents-of-tangents to update some horrid out-of-date state (.gir files are copy-pasted into every crate).

Unfortunately it's not compiling yet, because the C developers of gstreamer have time and time again rejected my fixes to their pointer mutability issues (shrug). It has demotivated and drained me thus far that I'm not even willing to work around fake and wrong bindings in Rust anymore.

Instead, I think I'll call it a day and fix it by replacing the type definitions in the .gir files when they're imported to gst-gir-files via the fix.sh wrapper script.

[sdroege]

Unfortunately it's not compiling yet, because the C developers of gstreamer have time and time again rejected my fixes to their pointer mutability issues (shrug). It has demotivated and drained me thus far that I'm not even willing to work around fake and wrong bindings in Rust anymore.

There are reasons for why things are the way they are.

Instead, I think I'll call it a day and fix it by replacing the type definitions in the .gir files when they're imported to gst-gir-files via the fix.sh wrapper script.

That seems like the best way forward indeed.

[MarijnS95]

@sdroege either that or Gir.toml parameter overrides. Any that has your preference, or any method that you know up-front won't work?

I.e. I don't know (remember, really) if Gir.toml is expressive enough to change mutability of the outer pointer in an array without affecting the inner pointer.

[sdroege]

I.e. I don't know (remember, really) if Gir.toml is expressive enough to change mutability of the outer pointer in an array without affecting the inner pointer.

It's not. There's no such concept in gobject-introspection, there's not even a concept of mutability in general. We're adding this here on top with some very basic heuristics that easily go wrong.

Any that has your preference

I don't mind either way

[MarijnS95]

Well Gir.toml is adding the Rust flavour on top of gobject-introspection so it might be something that's already supported, or am I misunderstanding you?

[sdroege]

It doesn't support that. For boxed types it is possible to change mutability of function parameters (if taken by reference), but that's all you can do right now in that regard. Arrays are IIRC always considered immutable (both the array as well as the elements) and you can't configure that, and usually functions that need mutability there in some way need manual bindings (and often are slightly different in behaviour).

[MarijnS95]

Arrays are IIRC always considered immutable (both the array as well as the elements) and you can't configure that

Yeah the code is generated with immutable slices (correct!) but the FFI function signature expects *mut *mut T instead of *const *mut T, for which ToGlibContainerFromSlice is (obviously) not implemented.

I think if we fix it in GstVulkan-1.0.gir via fix.sh the -sys bindings will carry the correct type as well and solve everything outright.

Again, I'm not going to reimplement every Rust function by hand just to work around this bindings issue.

[sdroege]

the FFI function signature expects *mut *mut T instead of *const *mut T

It should use the same mix of const/mut as the C API. Doesn't it?

for which ToGlibContainerFromSlice is (obviously) not implemented.

But why not. I think this can actually be implemented just fine? The returned array is always a copy anyway, the items are either full copies (including for e.g. i32) or refcounted types with interior mutability so it shouldn't matter really.

The only problem is for functions that assume that the array is passed in by the caller, modified by the function, and then the caller has the modified version (I guess that would also have to be marked (inout) in gobject-introspection then). Those require manual implementations for now.

[MarijnS95]

It should use the same mix of const/mut as the C API. Doesn't it?

Right now it does, but as said the C API is wrong and so is the resulting Rust -sys bindings.

But why not. I think this can actually be implemented just fine?

Can I implement ToGlibContainerFromSlice that takes an immutable &[] slice and returns a mutable *mut slice over some T? IIRC this couldn't be expressed back in the day, but I can try again.

The only problem is for functions that assume that the array is passed in by the caller, modified by the function, and then the caller has the modified version (I guess that would also have to be marked (inout) in gobject-introspection then).

The reason I marked them as *const in the C API (which got rejected) was to solidify this contract: the array nor its values are modified.

---

If you want to take a look the arrays in question are layouts in gst_vulkan_descriptor_cache_new() and gst_vulkan_descriptor_pool_create().

[MarijnS95]

The implementation that I had (assuming the C bindings were fixed) uses *const *mut:

https://gitlab.freedesktop.org/MarijnS95/gstreamer-rs/-/blob/7ede3ec18c0b67b3a56235557c34f634958171f9/gstreamer-vulkan/src/vulkan_handle.rs#L22-47

It doesn't copy anything for now; should it?

[MarijnS95]

@spencercw I've squashed and pushed the results of various short hacking sessions over the past few days; the bindings now compile and should be in a somewhat-usable state. Please let me know if anything is off or missing, but note that I still have to do another pass to recollect all the new functions and types, and either generate or manually implement them.

As for the things to discuss, I don't think it's worth repeating what I already wrote in the PR description (most/all are still applicable), doing it in GitHub discussions has so far proven worthless. @sdroege is there some kind of weekly syncup for gtk-rs/gstreamer-rs that I might be able to join and where we could have an agenda item to discuss the various points of contention to unblock Vulkan bindings in gstreamer-rs?

---

For now, specific to this gir PR, I've gone back to the idea of keeping the vulkan namespace/"crate" (1:1 mapping to Vulkan-1.0.gir), and that crate for now purely consists of use ash::vk::*; 🎉

We still have to figure out:

• How to annotate what types are passed as borrow/pointer;
• Why [[object]] name = "Vulkan.*" status = "manual" complains about the object not existing (hence I've pushed a hack to this PR to not treat the lowercase "vulkan" namespace as ignored()). This would otherwise generate all Vulkan types as /* Ignored */ making most bindings useless.

[spencercw]

Thanks for working on this. Couple of issues (possibly self-inflicted; not really sure what I'm doing):

In gstreamer-vulkan/sys/src/lib.rs, gst_vulkan_device_select_queue is generated using vulkan::QueueFlagBits for expected_flags, but QueueFlagBits does not exist in ash. Changing it to QueueFlags works.

If I re-run generator.py, in gstreamer-vulkan/src/auto/vulkan_image_view.rs the & is removed from the create_info parameter in VulkanImageView::new() which breaks compilation.

[MarijnS95]

@spencercw good point, I ran into that on my development machine where I have the 1.23 development gstreamer available, but don't on my laptop and hence didn't test with the v1_24 feature. There's currently only one use of a *Bits type which is behind that feature.

I think for that the custom vulkan package will have:

use ash::vk::*;
use ash::vk::QueueFlags as QueueFlagBits;

And it might require more manual conversion.

If I re-run generator.py, in gstreamer-vulkan/src/auto/vulkan_image_view.rs the & is removed from the create_info parameter in VulkanImageView::new() which breaks compilation.

Correct, that is the first point above regarding "How to annotate what types are passed as borrow/pointer;".

[MarijnS95]

@spencercw in the latest push everything compiles fine with the v1_24 feature. Note that generator output will still mismatch, until I figure out how to tweak the code generator to understand pointer transfers for non-glib types (if at all...).

[spencercw]

Thanks. Is there a safe way to access fields on the underlying C type from the safe wrappers? E.g., I want to read GstVulkanQueue::family from a VulkanQueue (among various others). I can do something like this, which is obviously not ideal: unsafe { (*queue.as_ptr()).family }.

Some issues I found while migrating to the safe bindings (sorry if you're already aware of these):

• VulkanDevice::select_queue should return Option
• vulkan_get_or_create_image_view should return Option
• VulkanImageView tries to use gst_vulkan_image_view_ref and gst_vulkan_image_view_unref, which are inline header functions so don't exist in the binary, resulting in linker errors

[MarijnS95]

@spencercw Thanks for checking this out. I don't exactly recall what the right/desired way was to get at the public fields of glib objects, maybe manual getters on the type?

It's already quite a handful to deal with so many repositories and changes just to get some bindings generated and see how close we can get to a compiling repo, that there has not yet been any time to actually test-drive the bindings, not to mention analyzing every individual generated struct and function to tell whether they make sense. I hope to get that at some point, but you're beating me to it quite nicely and it helps polishing the bindings before even getting the boilerplate in a barely acceptable state 👍

The first two points are obviously yet again missing annotations upstream; I've opened a PR for it and the patches are coming in fast and hot :(

For the ref/unref, these turn out to be GStreamer mini objects that gir detects as "shared" GLib objects because of the presence of those fake inline functions. I'll have to generate manual bindings for them and see how many types remain.

[spencercw]

One more thing that caught me out is I can't work out how to call VulkanImageBufferPool::config_set_allocation_params(). I want to do something like this:

let pool = gst_vulkan::VulkanImageBufferPool::new(device);
let mut config: gst::BufferPoolConfig = pool.config();
VulkanImageBufferPool::config_set_allocation_params(&mut config, ...);

But config_set_allocation_params takes a &mut gst::Structure, and I can only seem to get a &mut gst::StructureRef from gst::BufferPoolConfig.

[MarijnS95]

@spencercw just like the changes I pushed for context_get/set_* yesterday, this also needs to take a gst::StructureRef because it's a miniobject.
(Structure derefs into StructureRef but not vice-versa, and I don't think the code generator can deal with that)

However, maybe there is a case for an extension struct instead (like VulkanContextExt...), will investigate!

[MarijnS95]

Anyway, what kind of app/usage are you building with this @spencercw? My initial intent was to start with a glupload variant for Vulkan, shortly meaning:

1. We create an app/window with winit;
2. We create a Vulkan Surface and Swapchain on the window;
3. We create a GStreamer Pipeline that emits test images and uploads them to Vulkan images;
4. We handle the context query for the pipeline to give back our already-initialized (in the Rust example app) instance, device and queue;
5. When video frames (uploaded as Vulkan images) are available, they are copied and presented to the screen.
   (Seeing your snippet, maybe I can control what kind of images vkupload will generate by giving it a VulkanImageBufferPool)

And in the future this could be extended to use a fullscreen quad shader like glupload, and to chain extra GStreamer transformations into the pipeline (like GLFilter).

---

Then we're spiraling quite out of context for this PR. A better place might be the issue for adding Vulkan bindings to gstreamer-rs instead: https://gitlab.freedesktop.org/gstreamer/gstreamer-rs/-/issues/389

Alternatively we could find another place, or I could open a preliminary MR where we can start discussions on the diff (though I'm afraid it'd get too much review before I can even go over things myself).

[MarijnS95]

@spencercw here's an example of how a setter extension on BufferPoolConfig works:

https://gstreamer.pages.freedesktop.org/gstreamer-rs/stable/latest/docs/gstreamer_video/prelude/trait.VideoBufferPoolConfig.html

Now it seems that the function(s) you're linking are only relevant for a BufferPoolConfig, so perhaps it should extend that and not a StructureRef?

[MarijnS95]

@spencercw so we now have this API:

                let pool = gst_vulkan::VulkanImageBufferPool::new(&d);
                let mut config = pool.config();
                config.set_allocation_params(
                    ImageUsageFlags::TRANSFER_DST,
                    MemoryPropertyFlags::DEVICE_LOCAL,
                );
                dbg!(config);

Looks like it works:

[examples/src/bin/../vkupload.rs:624] config = BufferPoolConfig(
    Structure(
        GstBufferPoolConfig {
            caps: (GstCaps) NULL,
            size: (guint) 0,
            min-buffers: (guint) 0,
            max-buffers: (guint) 0,
            allocator: (GstAllocator) NULL,
            params: (GstAllocationParams) ((GstAllocationParams*) 0x5568936dde40),
            usage: (guint) 2,
            memory-properties: (guint) 1,
        },
    ),
)

Will tackle the same functions on images before pushing later.

[spencercw]

That sounds like a good starting point.

Personally, I'm implementing a filter deriving BaseTransform which runs a Vulkan render pass over the image. In propose_allocation I create a Vulkan image buffer pool and configure it like this, which allows the upstream non-Vulkan source to write directly into GPU memory:¹

gst_vulkan_sys::gst_vulkan_image_buffer_pool_config_set_allocation_params(
    config.as_mut_ptr(),
    vk::ImageUsageFlags::SAMPLED,
    vk::MemoryPropertyFlags::DEVICE_LOCAL
        | vk::MemoryPropertyFlags::HOST_VISIBLE
        | vk::MemoryPropertyFlags::HOST_COHERENT,
    vk::ImageLayout::PREINITIALIZED,
);

In the sink caps I don't specify memory:VulkanImage, so as far as the upstream source is concerned it's just writing into system memory. Not really sure whether this is correct, but I couldn't get it to negotiate with memory:VulkanImage in the caps, and I wanted to avoid an additional copy from vulkanupload.

I do use memory:VulkanImage on the output side, which for now I'm just feeding straight into vulkandownload.

Within the transform logic I'm mostly just using plain ash to do the actual rendering.

---

Happy to take this discussion somewhere else if you prefer.

---

The revised set_allocation_params API looks much better, thanks.

Footnotes

1. Currently the Vulkan image buffer pool automatically applies an illegal TRANSFER_DST_OPTIMAL image layout transition to the newly created images, which I've had to hack around for now: https://gitlab.freedesktop.org/spencercw/gstreamer/-/commit/17cd138ccdd6ca7feb02c4fe86171ee3c221b507 ↩

[spencercw]

By the way, I'm relying on gstreamer to initialise Vulkan for me, rather than the other way around, so my logic for getting the instance/device is basically a direct translation of this. I use ash::Instance::load and ash::Device::load to convert these into ash types.

[MarijnS95]

Hmm this is nasty, there are two functions (currently) doing the exact same thing:

gst_vulkan_buffer_pool_config_set_allocation_params();
gst_vulkan_image_buffer_pool_config_set_allocation_params();

And they should both be implemented on the BufferPoolConfigRef.

---

In the sink caps I don't specify memory:VulkanImage, so as far as the upstream source is concerned it's just writing into system memory. Not really sure whether this is correct, but I couldn't get it to negotiate with memory:VulkanImage in the caps, and I wanted to avoid an additional copy from vulkanupload.

Good point, I've also been wondering this while looking in DMA scenarios and wondering how "smart" GStreamer is with automatically uploading, copying, or mapping (and allocating compatible in the first place) buffers in an efficient way.

E.g. Vulkan could map DMA buffers and vice versa, and as you've found out Vulkan can create host-visible/writable memory. IIRC you can specify multiple memory types, maybe that works here? (And then you allocate matching memory on the Vulkan side, depending on the final caps that were negotiated?)

---

By the way, I'm relying on gstreamer to initialise Vulkan for me, rather than the other way around, so my logic for getting the instance/device is basically a direct translation of this. I use ash::Instance::load and ash::Device::load to convert these into ash types.

I expected this to be tricky when extra extensions (e.g. for WSI) are needed. There's an add_extension/layer API on GstVulkanDevice so it should be possible to specify needs before a VkDevice is created.

But it sounds like you're only using the device for a filter, and leaving windowing/presentation to vulkandownload etc.

---

I'd have proposed https://gitlab.freedesktop.org/gstreamer/gstreamer-rs/-/issues/389 to continue our discussions but it's already a cesspool of unrelated comments and noise. Maybe we should start a new issue; perhaps on my personal fork where the branch lives? https://gitlab.freedesktop.org/MarijnS95/gstreamer-rs

[spencercw]

Good point, I've also been wondering this while looking in DMA scenarios and wondering how "smart" GStreamer is with automatically uploading, copying, or mapping (and allocating compatible in the first place) buffers in an efficient way.

Yeah it doesn't seem to have much smarts in that regard at the moment. If you do a vulkanupload then you get two copies: once from system memory to a Vulkan staging buffer, then another from the staging buffer into the Vulkan image.

E.g. Vulkan could map DMA buffers and vice versa, and as you've found out Vulkan can create host-visible/writable memory. IIRC you can specify multiple memory types, maybe that works here? (And then you allocate matching memory on the Vulkan side, depending on the final caps that were negotiated?)

I was thinking something like that might be possible, but right now I know my upstream source is in system memory so I haven't looked into it any further.

I expected this to be tricky when extra extensions (e.g. for WSI) are needed.

Probably, yeah. I don't need any extra extensions right now, so again I haven't looked into it.

But it sounds like you're only using the device for a filter, and leaving windowing/presentation to vulkandownload etc.

I'm actually just feeding into an encoder for streaming. At some point I may need to import the images into CUDA for use with NVENC or something else, which is sure to bring its own set of headaches because while you can import Vulkan semaphores into CUDA, gstreamer doesn't seem to use CUDA semaphores anywhere at the moment.

Maybe we should start a new issue; perhaps on my personal fork where the branch lives? https://gitlab.freedesktop.org/MarijnS95/gstreamer-rs

Sounds good to me.

[MarijnS95]

@spencercw my reply is in https://gitlab.freedesktop.org/gstreamer/gstreamer-rs/-/issues/389#note_2188252

Feel free to jump back here if you have anything about mapping the vulkan namespace/crate specifically in gir / gtk-rs-core!