[PR #48] [MERGED] feat(vulkan): own the GstVulkanInstance/Device per element instead of process-globals #49

New issue

Closed

opened 2026-10-15 12:47:55 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-15 12:47:55 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/games-on-whales/gst-wayland-display/pull/48
Author: @salty2011
Created: 7/26/2026
Status: ✅ Merged
Merged: 8/19/2026
Merged by: @JBailes

Base: feat/vulkan-encode ← Head: fix/per-element-vulkan-device

📝 Commits (1)

672d603 feat(vulkan): own the GstVulkanInstance/Device per element instead of process-globals

📊 Changes

5 files changed (+231 additions, -124 deletions)

View changed files

📝 gst-plugin-wayland-display/src/waylandsrc/imp.rs (+18 -9)
📝 wayland-display-core/src/comp/mod.rs (+16 -1)
📝 wayland-display-core/src/lib.rs (+13 -0)
📝 wayland-display-core/src/utils/allocator/mod.rs (+4 -2)
📝 wayland-display-core/src/utils/vulkan_share.rs (+180 -112)

📄 Description

Problem

The owned GstVulkanInstance + GstVulkanDevice live in two process-global OnceLock slots, so every waylanddisplaysrc in a process shares whichever one was minted first. That means:

Session 2..N inherit session 1's target_minor, ignoring their own.
One session's DEVICE_LOST corrupts all N.
Nothing ever hands the device back. The slot leaks by design.

Change

One VulkanShare per element, cloned once into that element's compositor thread. Same pattern as the existing app_surface_commits / renderer_degraded Arcs.
Each element mints its own device and answers gst.vulkan.{instance,device} with it. Ownership alone retires it: the Arc drops when the element is finalized, so N sessions no longer leak N devices.
Storage location only. ensure_owned_device is byte-identical to the old global path, no vendor branch, RADV unchanged. C bridge untouched.

Rebased onto b49f7c0 as a single commit now that #46 and #47 have landed.

Your review points

Fan-out topology. Inside this crate, every buffer is minted on its own element's device (set_caps -> ensure_owned_device, ring built from the same share). Which device the encoder adopts is the embedder's context wiring, since the encoder is in a separate pipeline across interpipe.

handle_set_context now keeps the device this element already holds and warns if a different one is offered. Your bin-fanout path is what convinced me: GstBin fans a have-context to every child, so two sources in one bin is enough, no mis-wired embedder needed. The cross-device pairing is refused in the crate rather than left to the embedder to check.

Ordering contract worth stating: inject a device before caps negotiation or not at all. set_caps mints eagerly, so a later injection is refused.

stop()/start(). clear() is gone. Per-element ownership plus the compositor thread join already retire everything that matters at stop(), so it only made release earlier, and it was the sole source of the desync caveat this PR previously pushed onto embedders. What remains after stop() is a bare VkDevice+VkInstance held until the element is finalized.

Feature-gate. I believe this is already true on base, so I've left it: Cargo.toml declares gstreamer-vulkan features = ["v1_28"] unconditionally, and utils/mod.rs declares vulkan_share/vulkan_nv12 unconditionally. A CUDA-only gst-1.24 build fails before this PR. The only new requirement was headers + a C compiler, documented in ci/README.md by #47. Say the word if you still want a vulkan feature.

target_minor after an absorbed device. Left as-is, documented. The slot records no provenance, so the short-circuit cannot tell a self-minted device from an injected one. When they differ the encoder is already bound to the injected device, and re-minting on the requested node would split producer and encoder across two VkDevices, which is worse than honouring the wrong node.

Also done: tautological test dropped · repr(C) overlay doc claim fixed · comp/mod.rs doc drift fixed · lock order documented on ensure_owned_device (only method holding both, so the inverse order can't arise) · lib.rs five-second comment deleted, that path cannot produce a VULKAN video info.

Not needed: new_ret_no_self doesn't fire (clippy exempts new returning Arc<Self>) · raw_handles is byte-identical to base, so the family-0 point is moot.

Verified

cargo fmt clean · check and check --features cuda clean · rustdoc no broken links · 23 tests pass.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/games-on-whales/gst-wayland-display/pull/48 **Author:** [@salty2011](https://github.com/salty2011) **Created:** 7/26/2026 **Status:** ✅ Merged **Merged:** 8/19/2026 **Merged by:** [@JBailes](https://github.com/JBailes) **Base:** `feat/vulkan-encode` ← **Head:** `fix/per-element-vulkan-device` --- ### 📝 Commits (1) - [`672d603`](https://github.com/games-on-whales/gst-wayland-display/commit/672d6038aaa0386446c0223b0652756dba86459e) feat(vulkan): own the GstVulkanInstance/Device per element instead of process-globals ### 📊 Changes **5 files changed** (+231 additions, -124 deletions) <details> <summary>View changed files</summary> 📝 `gst-plugin-wayland-display/src/waylandsrc/imp.rs` (+18 -9) 📝 `wayland-display-core/src/comp/mod.rs` (+16 -1) 📝 `wayland-display-core/src/lib.rs` (+13 -0) 📝 `wayland-display-core/src/utils/allocator/mod.rs` (+4 -2) 📝 `wayland-display-core/src/utils/vulkan_share.rs` (+180 -112) </details> ### 📄 Description ## Problem The owned `GstVulkanInstance` + `GstVulkanDevice` live in two process-global `OnceLock` slots, so every `waylanddisplaysrc` in a process shares whichever one was minted first. That means: - Session 2..N inherit session 1's `target_minor`, ignoring their own. - One session's `DEVICE_LOST` corrupts all N. - Nothing ever hands the device back. The slot leaks by design. ## Change - One `VulkanShare` per element, cloned once into that element's compositor thread. Same pattern as the existing `app_surface_commits` / `renderer_degraded` `Arc`s. - Each element mints its own device and answers `gst.vulkan.{instance,device}` with it. Ownership alone retires it: the `Arc` drops when the element is finalized, so N sessions no longer leak N devices. - Storage location only. `ensure_owned_device` is byte-identical to the old global path, no vendor branch, RADV unchanged. C bridge untouched. Rebased onto `b49f7c0` as a single commit now that #46 and #47 have landed. ## Your review points **Fan-out topology.** Inside this crate, every buffer is minted on its own element's device (`set_caps` -> `ensure_owned_device`, ring built from the same share). Which device the *encoder* adopts is the embedder's context wiring, since the encoder is in a separate pipeline across interpipe. `handle_set_context` now keeps the device this element already holds and warns if a different one is offered. Your bin-fanout path is what convinced me: GstBin fans a have-context to every child, so two sources in one bin is enough, no mis-wired embedder needed. The cross-device pairing is refused in the crate rather than left to the embedder to check. Ordering contract worth stating: inject a device before caps negotiation or not at all. `set_caps` mints eagerly, so a later injection is refused. **stop()/start().** `clear()` is gone. Per-element ownership plus the compositor thread join already retire everything that matters at `stop()`, so it only made release earlier, and it was the sole source of the desync caveat this PR previously pushed onto embedders. What remains after `stop()` is a bare `VkDevice`+`VkInstance` held until the element is finalized. **Feature-gate.** I believe this is already true on base, so I've left it: `Cargo.toml` declares `gstreamer-vulkan` `features = ["v1_28"]` unconditionally, and `utils/mod.rs` declares `vulkan_share`/`vulkan_nv12` unconditionally. A CUDA-only gst-1.24 build fails before this PR. The only new requirement was headers + a C compiler, documented in `ci/README.md` by #47. Say the word if you still want a `vulkan` feature. **target_minor after an absorbed device.** Left as-is, documented. The slot records no provenance, so the short-circuit cannot tell a self-minted device from an injected one. When they differ the encoder is already bound to the injected device, and re-minting on the requested node would split producer and encoder across two `VkDevice`s, which is worse than honouring the wrong node. **Also done:** tautological test dropped · `repr(C)` overlay doc claim fixed · `comp/mod.rs` doc drift fixed · lock order documented on `ensure_owned_device` (only method holding both, so the inverse order can't arise) · `lib.rs` five-second comment deleted, that path cannot produce a VULKAN video info. **Not needed:** `new_ret_no_self` doesn't fire (clippy exempts `new` returning `Arc<Self>`) · `raw_handles` is byte-identical to base, so the family-0 point is moot. ## Verified `cargo fmt` clean · `check` and `check --features cuda` clean · rustdoc no broken links · 23 tests pass. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-15 12:47:55 +00:00