diff --git a/crates/pf-encode/src/enc/windows/pyrowave.rs b/crates/pf-encode/src/enc/windows/pyrowave.rs index ffaa754a..a128e2d8 100644 --- a/crates/pf-encode/src/enc/windows/pyrowave.rs +++ b/crates/pf-encode/src/enc/windows/pyrowave.rs @@ -297,8 +297,10 @@ impl PyroWaveEncoder { /// Import one capturer plane D3D11 texture (`R8_UNORM` Y or `R8G8_UNORM` CbCr) into pyrowave's /// Vulkan device. Creates a fresh shared NT handle from the texture (the capturer marked the ring /// `SHARED | SHARED_NTHANDLE`); `pyrowave_image_create` takes ownership of the handle and closes - /// it on import. Single/two-component textures import reliably on NVIDIA at any size — unlike a - /// planar NV12 — so no MUTABLE_FORMAT / planar-layout workaround is involved. + /// it on SUCCESSFUL import only (pyrowave-sys patch 0006 — same contract as the fence import in + /// [`Self::import_fence`]), so this fn closes it on every failure return. Single/two-component + /// textures import reliably on NVIDIA at any size — unlike a planar NV12 — so no + /// MUTABLE_FORMAT / planar-layout workaround is involved. /// /// # Safety /// `texture` must be a live `ID3D11Texture2D` of format `vk_format`, sized `w`×`h`, created @@ -349,7 +351,11 @@ impl PyroWaveEncoder { }; let mut image: pw::pyrowave_image = std::ptr::null_mut(); if let Err(e) = pw_check(pw::pyrowave_image_create(&info, &mut image), "image_create") { - // pyrowave only closes the handle on a SUCCESSFUL import — close it ourselves on failure. + // pyrowave consumes the handle ONLY on a successful import (pyrowave-sys patch 0006 + // pinned this at the API's success boundary) — so on EVERY failure return the handle + // is still ours and this close is the single one. Before the patch, an + // allocate-stage failure inside Granite had already closed it and this was a double + // close of a possibly-recycled handle value. let _ = CloseHandle(handle); return Err(e); } @@ -419,7 +425,9 @@ impl PyroWaveEncoder { pw::pyrowave_sync_object_create(&info, &mut sync), "sync_object_create", ) { - // pyrowave only closes the handle on a SUCCESSFUL import — close the dup on failure. + // pyrowave only closes the handle on a SUCCESSFUL import (Granite's semaphore import + // has always had these semantics; patch 0006 made the image import match) — close + // the dup on failure. let _ = CloseHandle(dup); return Err(e); } diff --git a/crates/pyrowave-sys/patches/0006-external-memory-consume-on-success-only.patch b/crates/pyrowave-sys/patches/0006-external-memory-consume-on-success-only.patch new file mode 100644 index 00000000..d6785bde --- /dev/null +++ b/crates/pyrowave-sys/patches/0006-external-memory-consume-on-success-only.patch @@ -0,0 +1,88 @@ +NT-handle import: consume on SUCCESS only — PUNKTFUNK LOCAL PATCH. + +Not upstream. Granite's DeviceAllocator::internal_allocate closed the imported +by-reference NT handle (OPAQUE_WIN32 / D3D11_TEXTURE / D3D12_RESOURCE — the +predicate excludes fd and KMT types) UNCONDITIONALLY after the FIRST +vkAllocateMemory — on success AND on failure — while every failure before the +allocator (c-shim validation, vkCreateImage, find_memory_type) left the handle +open. Both classes surface to pyrowave_image_create's caller as the same error +code, so the caller could not know whether to close: punktfunk's import_plane +closed on every failure and double-closed whenever the failure was at or after +the allocate — and a recycled handle value makes that close an unrelated live +handle's. The unconditional consume also broke the allocator's own +block-recycling retry loop, which re-ran vkAllocateMemory with import_info +still pointing at the just-closed handle. + +Fixed by moving the consume to the API commit point: pyrowave_image_create +closes the by-reference handle exactly on its success return. This pins +pyrowave.h's documented contract ("take ownership and close the HANDLE on +import") to mean on SUCCESS, matching pyrowave_sync_object_create (Granite's +semaphore import already closes only after a successful vkImportSemaphore*). +NT-handle memory imports never transfer ownership at the Vulkan level (the +implementation holds its own reference), so the post-create close is legal. + +Behavior note: Granite code that imports by-reference memory WITHOUT going +through pyrowave_image_create — in this vendored subset only wsi_dxgi.cpp's +interop swapchain, which the pyrowave C API never reaches — now keeps the +caller's handle open on allocate-stage failure instead of consuming it. That +matches the semaphore contract; those callers own their close-on-failure. + +Correctness re-validated by pyrowave_win_smoke on real hardware after the +change. + +diff --git a/crates/pyrowave-sys/vendor/pyrowave/Granite/vulkan/memory_allocator.cpp b/crates/pyrowave-sys/vendor/pyrowave/Granite/vulkan/memory_allocator.cpp +index f33bdac3..fd035b66 100644 +--- a/crates/pyrowave-sys/vendor/pyrowave/Granite/vulkan/memory_allocator.cpp ++++ b/crates/pyrowave-sys/vendor/pyrowave/Granite/vulkan/memory_allocator.cpp +@@ -732,16 +732,15 @@ bool DeviceAllocator::internal_allocate( + res = table->vkAllocateMemory(device->get_device(), &info, nullptr, &device_memory); + } + +- // If we're importing, make sure we consume the native handle. +- if (external && bool(*external) && +- ExternalHandle::memory_handle_type_imports_by_reference(external->memory_handle_type)) +- { +-#ifdef _WIN32 +- ::CloseHandle(external->handle); +-#else +- ::close(external->handle); +-#endif +- } ++ // PUNKTFUNK (patch 0006): the consume of by-reference native handles used to happen HERE, ++ // unconditionally — success AND failure of the first vkAllocateMemory. That made the caller's ++ // failure contract ambiguous (an allocate-stage failure had already closed the handle while a ++ // create/find_memory_type failure had not), and the block-recycling retry loop below re-ran ++ // vkAllocateMemory with import_info still pointing at the just-closed handle. The consume now ++ // lives at the API commit point (pyrowave_c.cpp: pyrowave_image_create's success return), so ++ // the allocator never closes a caller's handle and retries import a still-open handle. ++ // (fd-type imports were never affected: memory_handle_type_imports_by_reference excludes ++ // OPAQUE_FD/DMA_BUF, whose ownership vkAllocateMemory itself transfers on success.) + + if (res == VK_SUCCESS) + { +diff --git a/crates/pyrowave-sys/vendor/pyrowave/pyrowave_c.cpp b/crates/pyrowave-sys/vendor/pyrowave/pyrowave_c.cpp +index eb917e64..985cd0a9 100644 +--- a/crates/pyrowave-sys/vendor/pyrowave/pyrowave_c.cpp ++++ b/crates/pyrowave-sys/vendor/pyrowave/pyrowave_c.cpp +@@ -592,6 +592,21 @@ pyrowave_result pyrowave_image_create(const pyrowave_image_create_info *info, py + if (!img) + return PYROWAVE_ERROR_FAILED_EXTERNAL_HANDLE; + ++ // PUNKTFUNK (patch 0006): consume the imported by-reference NT handle exactly here, at the ++ // API's success boundary — the allocator no longer closes it (see memory_allocator.cpp). ++ // This pins pyrowave.h's documented contract ("take ownership and close the HANDLE on ++ // import") to mean ON SUCCESS, matching pyrowave_sync_object_create's semantics: on ANY ++ // failure return the caller still owns the handle and can close it unconditionally. ++ // By-reference NT-handle imports never transfer ownership at the Vulkan level (the ++ // implementation keeps its own reference), so a post-create close here is always legal. ++#ifdef _WIN32 ++ if (info->external_handle && ++ ExternalHandle::memory_handle_type_imports_by_reference(info->handle_type)) ++ { ++ ::CloseHandle(reinterpret_cast(info->external_handle)); ++ } ++#endif ++ + auto *image = new pyrowave_image_opaque(); + image->device = &device; + image->img = std::move(img); diff --git a/crates/pyrowave-sys/vendor/pyrowave/Granite/vulkan/memory_allocator.cpp b/crates/pyrowave-sys/vendor/pyrowave/Granite/vulkan/memory_allocator.cpp index f33bdac3..fd035b66 100644 --- a/crates/pyrowave-sys/vendor/pyrowave/Granite/vulkan/memory_allocator.cpp +++ b/crates/pyrowave-sys/vendor/pyrowave/Granite/vulkan/memory_allocator.cpp @@ -732,16 +732,15 @@ bool DeviceAllocator::internal_allocate( res = table->vkAllocateMemory(device->get_device(), &info, nullptr, &device_memory); } - // If we're importing, make sure we consume the native handle. - if (external && bool(*external) && - ExternalHandle::memory_handle_type_imports_by_reference(external->memory_handle_type)) - { -#ifdef _WIN32 - ::CloseHandle(external->handle); -#else - ::close(external->handle); -#endif - } + // PUNKTFUNK (patch 0006): the consume of by-reference native handles used to happen HERE, + // unconditionally — success AND failure of the first vkAllocateMemory. That made the caller's + // failure contract ambiguous (an allocate-stage failure had already closed the handle while a + // create/find_memory_type failure had not), and the block-recycling retry loop below re-ran + // vkAllocateMemory with import_info still pointing at the just-closed handle. The consume now + // lives at the API commit point (pyrowave_c.cpp: pyrowave_image_create's success return), so + // the allocator never closes a caller's handle and retries import a still-open handle. + // (fd-type imports were never affected: memory_handle_type_imports_by_reference excludes + // OPAQUE_FD/DMA_BUF, whose ownership vkAllocateMemory itself transfers on success.) if (res == VK_SUCCESS) { diff --git a/crates/pyrowave-sys/vendor/pyrowave/pyrowave_c.cpp b/crates/pyrowave-sys/vendor/pyrowave/pyrowave_c.cpp index eb917e64..985cd0a9 100644 --- a/crates/pyrowave-sys/vendor/pyrowave/pyrowave_c.cpp +++ b/crates/pyrowave-sys/vendor/pyrowave/pyrowave_c.cpp @@ -592,6 +592,21 @@ pyrowave_result pyrowave_image_create(const pyrowave_image_create_info *info, py if (!img) return PYROWAVE_ERROR_FAILED_EXTERNAL_HANDLE; + // PUNKTFUNK (patch 0006): consume the imported by-reference NT handle exactly here, at the + // API's success boundary — the allocator no longer closes it (see memory_allocator.cpp). + // This pins pyrowave.h's documented contract ("take ownership and close the HANDLE on + // import") to mean ON SUCCESS, matching pyrowave_sync_object_create's semantics: on ANY + // failure return the caller still owns the handle and can close it unconditionally. + // By-reference NT-handle imports never transfer ownership at the Vulkan level (the + // implementation keeps its own reference), so a post-create close here is always legal. +#ifdef _WIN32 + if (info->external_handle && + ExternalHandle::memory_handle_type_imports_by_reference(info->handle_type)) + { + ::CloseHandle(reinterpret_cast(info->external_handle)); + } +#endif + auto *image = new pyrowave_image_opaque(); image->device = &device; image->img = std::move(img);