Repository navigation
feat: SG-43947: Present through Vulkan for 10-bit output on Linux and Windows - #1427
Merged
cedrik-fuoco-adsk merged 60 commits intoOct 13, 2026
Conversation
cedrik-fuoco-adsk
force-pushed
the
SG-43947-vulkan-presentation
branch
6 times, most recently
from
September 29, 2026 14:57
9315785 to
e871428
Compare
…osted by VulkanView Bridge from AcademySoftwareFoundation#1319 to the restructured viewport this branch builds on. The Vulkan viewport becomes a native VulkanWindow (QWindow) embedded by a VulkanView container through createWindowContainer(), mirroring GLWindow/GLView, in place of the WA_NativeWindow/WA_PaintOnScreen QWidget. The GL interop negotiation and presentation-path record from AcademySoftwareFoundation#1319 are re-ported onto VulkanWindow by "fix(vulkan): port the Vulkan presentation interop negotiation" later in this series. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
…t probe and the viewport Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
VulkanWindow::render() presented either the session output device or the main viewport, never both. Unlike the GL path, where QOpenGLWidget composites the control widget after paintGL regardless, the Vulkan viewport only appears via an explicit present -- so once presentation mode was on the main window sat on a stale frame. Present the viewport unconditionally, then additionally present a distinct output device. Also guard render() on m_initialized: initialize() runs from exposeEvent(), but resizeEvent() also calls requestUpdate(), so an UpdateRequest can reach the present path before the surface and swapchain exist. And present once from exposeEvent() for a doc-less window, which is a passive presentation output whose render() returns early at !session and would otherwise stay blank until the next main-view frame. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
… output The presentation output went through DesktopVideoDevice's OpenGL ScreenView even when the main view was already presenting 10-bit through Vulkan, so the second display was capped at 8 bits. Add a DesktopVideoDevice subclass that delivers the frame through a Vulkan swapchain instead. It reuses the base class's frame handoff wholesale -- transfer()/transfer2() composite into m_viewDevice->defaultFBO() exactly as they do for the ScreenView, every stereo mode included -- and overrides only the window lifecycle and the present, since Vulkan has no QOpenGLWidget auto-composite. The output window is a top-level VulkanView constructed with a null doc. That null doc is what makes it passive: VulkanWindow::render() returns at !session, requestGLFallback() returns at !m_doc, and presentationAllowed() guards on it, so a presentation surface never drives the session nor drags the main window into a GL fallback. Supporting changes: - QTVulkanVideoDevice::fboID() reports the offscreen FBO once it exists. DesktopVideoDevice::transfer() returns early while the view device reports 0, which is how the first composite is deferred until the target exists -- without the override it would never composite. - A doc-less VulkanView keys its device name on the view rather than the doc, which would otherwise collide across screens. - DesktopVideoDevice::shouldUseVulkanPresentation() is the single GL-vs-Vulkan rule, used both by createDesktopVideoDevices() and by the RvDocument constructor so the main view and the presentation output cannot disagree on the backend. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
DesktopVideoDevice's constructor calls addDefaultDataFormats(), whose depth defaults to 8, so a VulkanDesktopVideoDevice reported "RGB8" in Preferences > Video, in the top-view toolbar's device table and in humanReadableID() while presenting through a 10-bit swapchain. This was only ever a labelling bug, not truncation: depth is used solely to build the description string, and DesktopDataFormat forwards to DataFormat(const std::string&), which sets iformat = RGBA16F. That iformat is what picks the render target, so the presentation path is RGBA16F -> GL_RGB10_A2 shared texture -> A2B10G10R10 swapchain throughout. The number shown to the user was simply wrong. Two consequences worth knowing: - The persisted preference is safe. RvPreferences stores "dataFormat" as an index and setVideoDeviceStateFromSettings reads it back with toInt(); addDefaultDataFormats appends the same six stereo modes in the same order at any depth, so a user's stereo choice survives a GL <-> Vulkan device rebuild. - -presentData matches by description string, so "-presentData RGB8" no longer selects anything on a Vulkan presentation device, and a display profile saved against a VideoAndDataFormatID ending in RGB8 will not be found once the device reports RGB10. The coarser ModuleNameID and DeviceNameID profiles are unaffected. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
…kend changes The presentation devices were built once at startup and only ever had their share device re-pointed afterwards, so their backend was frozen at whatever it was at launch. After a main-view backend transition the second display went black. Add DesktopVideoModule::rebuildDevices(), which re-evaluates the GL-vs-Vulkan rule and rebuilds the per-screen devices to match. It is a no-op returning false when the backend has not changed, so an unrelated depth change does not put a teardown transient on the second display; on a real change it closes each open device before destroying it, so no swapchain or ScreenView resources leak. It deliberately does not touch the session output device, because the devices it destroys may be referenced as that output. Add RvApplication::rebuildDesktopVideoDevices() to orchestrate: rebuild, re-bind the share device afterwards (so it never writes to an about-to-be-destroyed device), and re-open the presentation output, re-resolved by name through Options::presentDevice rather than by a pointer the rebuild may have invalidated. The critical piece is refreshing the session graph on a real rebuild. The DisplayGroupIPNodes built at startup by setPhysicalDevices still held the destroyed device pointers, so a later setOutputVideoDevice(newDevice) -> connectDisplayGroup -> findDisplayGroupByDevice(newDevice) matched nothing and silently no-op'd. That is the reported 10 -> 8 -> 10 -> enable-presentation black-screen repro, and it has to run even while presentation is off, since the pointers can change then and only be bound as the output later. Both hand-rolled setShareDevice loops in RvDocument now call through this instead. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
…t change Selecting 10-bit while running on the OpenGL GLView ran the display output through rebuildGLView() at 10 bits. OpenGL cannot present 10-bit on the affected hardware (Mesa GLX, Windows WGL negotiation), so the rebuild failed validity, popped a misleading "Display Configuration is Invalid" dialog and zeroed the preference on the way out. The backend was otherwise fixed at window construction, so a depth change needed a restart. setDisplayOutput() now persists the requested depth to both Options and QSettings before doing anything else -- it never zeroes the preference -- and then applies it: - 10-bit while on GLView: promote the live view with swapGLViewToVulkan() when the probe says 10-bit can be presented, or show an honest "this display cannot present 10-bit output" dialog and stay at 8-bit when it cannot. The GL rebuild path is never reached for a 10-bit request on these platforms. - 8-bit or default while Vulkan is live: fall back with fallbackVulkanToGLView(), which rebuilds GLView from the depth just persisted. That direction is always available, so it needs no restart. swapGLViewToVulkan() is the forward mirror of fallbackVulkanToGLView(). It commits optimistically: VulkanWindow creates its surface and swapchain from exposeEvent(), so initialization cannot be verified synchronously. m_vulkanView is assigned before show() because the existing backstop -- requestGLFallback() -> fallbackVulkanToGLView() -- is guarded on it, and render() no-ops until initialized, so nothing presents to an uninitialized swapchain. Worst case is a brief blank frame during the swap. m_glView is set to null on the Vulkan path because backend-neutral code across RvDocument keys the active backend on (!m_glView). The old GLView is lazy-deleted on a timer, mirroring rebuildGLView, since deleting it inline while the swap is settling can dump core. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
…View Disabling presentation mode aborted with ASSERT failure in Rv::VulkanView: "Called object is not of the correct type (class destructor may have already run)", qobjectdefs_impl.h:121 VulkanDesktopVideoDevice::close() deletes its VulkanView directly. The ~VulkanView body runs first, then ~QWidget destroys the widget's own QWidgetWindow and its children -- both of which emit destroyed() at a point where the VulkanView sub-object no longer exists. Qt's assertObjectType dynamic_casts the receiver before invoking a member slot, that cast fails, and Q_ASSERT_X aborts. A presentation output view is what makes this reachable. It is top-level, so window() returns the view itself and watchParentWindow() connects parentWindowDestroyed to its *own* QWidgetWindow, which dies with it. The main view watches the enclosing document window, which outlives it, so it never delivered the signal during destruction. Fix both ends. The destructor severs the watched-window connection and the viewport-window destroyed lambda before the base destructors run -- the lambda would not assert, being a functor, but would write m_vulkanWindow through a dangling this. And watchParentWindow() no longer watches anything when the view is its own top level: the point of that machinery is surviving Qt replacing the *enclosing* window, and a standalone output window has no enclosing tree to lose. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
With 10-bit presentation enabled, an annotation stroke drew its first point and then stopped following the pointer. 10-bit alone was fine. Session::askForRedraw() redraws the control device *and* the output device, and it runs from arbitrary places -- including a mouse-motion handler, mid-stroke, with the main view's GL context current. VulkanDesktopVideoDevice::redraw() presented from there, and presenting means QTVulkanVideoDevice::syncBuffers(), which makes its own offscreen context current and never restores the previous one. The stroke's first point landed, the context was then silently swapped out from under the paint code, and every subsequent GL call went to the presentation device's context. It also ran a full vsync-blocking swapchain present per motion event. The GL output path never had this problem: its syncBuffers() is a QOpenGLWidget update(), which schedules a composite without touching the current context. Make redraw()/redrawImmediately() do nothing. The present is already driven in-frame: askForRedraw() redraws the control device too, which schedules VulkanWindow::render(), which renders, composites into this device via the inherited transfer(), and presents it through syncBuffers(). A doc-less presentation window also presents once on expose, so it is never left blank. Restore the viewport's context in VulkanWindow::render() after presenting the output device, for the same reason -- otherwise postRender() and everything after the frame run with the presentation device's context current. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
A doc-less VulkanView is a passive presentation output owned by a VulkanDesktopVideoDevice: it is composited into and presented by that device and must never take part in input handling. Giving it focus is actively harmful -- a second focusable top-level fights the main window for activation, and the resulting WindowActivate storm starves the event loop, which was observed as annotation strokes never receiving their drag events. Build a passive output with no focus, no focus proxy and no event widget, so QTVulkanVideoDevice never creates a translator for it and VulkanWindow::event() bails at !hasTranslator() -- the window is inert by construction rather than relying on each handler to notice it has no document. WA_ShowWithoutActivating keeps show() from stealing activation, and WindowDoesNotAcceptFocus keeps the window manager from handing it back later. Also correct the reasoning in VulkanDesktopVideoDevice::redraw(): it claimed Session::askForRedraw() reaches it, which it cannot, because askForRedraw() casts to TwkGLF::GLVideoDevice while every desktop output device derives from the sibling GLBindableVideoDevice. The empty override still matters, because DesktopVideoDevice::syncBuffers() does reach redraw(). Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
…ug gpu Presentation-mode latency could not be attributed from the outside: the frame loop, the GPU, the swapchain and the Qt event loop all plausibly explain "the annotation trails the cursor", and guessing between them wasted several rounds. Measure them instead. VulkanWindow::render() now accumulates an averaged breakdown every 60 frames -- session->render(), viewport present split into fence-wait vs acquire, output present, postRender, and the loop period -- plus the pointer side: per-event handler cost, eventToRender (input to start-of-render), and eventToRetire. eventToRetire is the one that matters and the one nothing measured before: the age of the pointer event a frame answered, closed out when that frame's GPU work retires, sampled with a non-blocking vkGetFenceStatus poll per slot per frame. It is end-to-end interactive latency. eventToRender covers only the input half, and stayed flat at ~4.5ms across every configuration while the feel changed completely, which is precisely why it explained nothing. GLWindow gets the comparable subset so the two backends can be read side by side. It has no mainPresent term by construction: QOpenGLWindow swaps the control surface after paintGL returns, so that cost lands outside the measured region. QTVulkanVideoDevice reports its present path (GPU-interop vs CPU-fallback) per device and on every transition, not latched on the first frame -- the first syncBuffers() can run before that window's Vulkan is initialised, and latching there reported CPU-fallback for a device that then spent its whole life on interop. Also records, above useOptimalTilingForInterop(), that OPTIMAL tiling was measured on AMD (RADV, Mesa) and renders tile-pattern garbage, because GL is radeonsi there and GL_OPTIMAL_TILING_EXT carries no cross-driver layout guarantee. All of it is gated on ImageRenderer::debugGpu(). Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
Annotation trailed the cursor whenever presentation mode was on. The cause was not throughput: measured on a 4K presentation output, OpenGL and Vulkan ran presentation mode at the same frame interval (24-44ms vs 23-41ms), and only the Vulkan one felt like it lagged. What differed was how old the displayed pixels were. With two frames in flight the CPU runs a frame ahead, so the screen answers input from two frames back -- some 60ms at 30fps. The OpenGL path is effectively one deep, because paintGL() draws and Qt's following swap waits on that same work. Giving up the second frame costs no measurable frame rate here, since the loop is GPU-bound either way: fenceWait was 11-25ms in presentation mode against 0.004ms without it. So block on the frame's own fence after its present is queued, rather than letting the next frame's start-of-frame wait absorb it two frames later. The present is queued first so the driver still gets the frame as early as possible; this only stops the CPU running ahead. The passive presentation output is excluded. Measured after: eventToRetire settled at eventToRender plus one frame interval, with no queue left behind it. RV_VULKAN_MAX_FRAMES_IN_FLIGHT=2 restores the previous behaviour, for a machine where the CPU has enough independent work to be worth overlapping. The effective depth is reported in the -debug gpu frame line, because a silent knob cost a wasted test round. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
…rmat QTVulkanVideoDevice allocated its offscreen FBO as GL_RGBA16F_ARB for every device. The control viewport needs that depth -- session->render() composites the whole main view into it across blended passes -- but a passive presentation output never does. It is only ever a blit destination for the inherited transfer()/transfer2()/fillWithTexture(), which hand over an already-composited frame. Keeping it at 16F there cost two full passes' worth of bandwidth at a 3840x2160 output: transfer() wrote 8 bytes/px, some 66MB, and syncBuffers() read all of it back to convert down to the 10-bit shared image. Matching the shared image's format halves both, and the conversion happens once, in a blit that was already going to run. The output is 10-bit either way, so no precision is lost that the present did not already discard. Measured on a 4K presentation output: the per-frame GPU wait dropped from 23ms to 5.5-16ms, frame interval from 33-45ms to 18-29ms, and eventToRetire from 38-49ms to 22-33ms -- parity with presentation off. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
The OpenGL presentation path never lets the second display gate the viewport's loop: DesktopVideoDevice::syncBuffers() is a coalesced QOpenGLWidget::update() on an empty paintGL() that Qt drops when it falls behind. The Vulkan path queued its output work unconditionally, every frame, and at a 4K output that work is what the control viewport ends up waiting for in vkWaitForFences. Gate the output present on whether this device's own GPU work has caught up, rather than on whether the swapchain is full -- the latter never fires, because a loop slower than the display always leaves the queue room. canPresentNow() is checked from syncBuffers() ahead of any GL work, so a skipped frame costs nothing at all instead of costing the full-resolution blit, and no GL semaphore has been signalled yet so there is nothing to rebalance. The blocking paths inside presentSharedImage()/presentPixelData() keep a second, later skip for safety, which does have to drain the semaphore pair. A skipped present must be retried or the output is left on the frame before the one just composited, with nothing else coming back for it: the control viewport only renders when the session asks. Qt's update() is a dirty flag it must eventually honour, so parity needs the same guarantee -- requestBestEffortRetry() posts a coalesced UpdateRequest that render() serves from its passive-output branch, and a staleness timer forces a blocking frame through if the output has gone unpresented for more than 100ms. Note this does not currently fire on the hardware it was developed against: with a one-deep control pipeline the viewport waits for its own GPU work each frame, which gives the output's fences time to retire, so the gate passes every time and outputPresent never reaches zero. It is kept as the protection for the reverse balance -- a slower output display, a heavier output scene, or a faster control GPU -- which also means its skip and starvation paths are untested in practice. Isolated in its own commit so it can be dropped with a single revert. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
…nd swap
Switching between 8- and 10-bit reset the display transfer function from
sRGB to None, discarding any assigned display profile with it.
Two causes, in sequence.
IPGraph::deviceChanged() adopted newDevice->physicalDevice()
unconditionally. VideoDevice's constructor seeds m_physicalDevice with
the device itself, and a viewport device only learns the monitor it sits
on when it first renders, via setAbsolutePosition() ->
deviceFromPosition(). Its one caller is
Session::setControlVideoDevice(), which during a backend swap runs on a
view that has never rendered -- so physicalDevice() was still that view,
and the display group's device.name was rewritten from the monitor
("Dell Inc. DELL U2725QE DP-1") to the viewport's own name ("RV Main
Window (Vulkan)/0x..."). Only adopt a physical device the new device
actually knows; the monitor has not changed, only the object drawing to
it.
RvApplication::rebuildDesktopVideoDevices() then called
IPGraph::setPhysicalDevices() to refresh stale device pointers, but that
is the startup routine: it deletes every DisplayGroupIPNode and rebuilds
them, and a new display group comes with a new colorPipeline holding
default contents. So the group was discarded and its colour state with
it. Add IPGraph::refreshPhysicalDevices(), which re-points existing
groups at the rebuilt devices by (module name, device name) -- the same
key display profiles are stored under, and stable across a rebuild
because createDesktopVideoDevices() names every screen from its QScreen
regardless of backend. Groups are created or deleted only for devices
that genuinely appeared or vanished.
It also clears a group's output device when that pointer is not among
the new devices. findDisplayGroupByDevice() compares raw pointers, so a
dangling one can alias a freshly allocated device at the same address
and return the wrong group. The control device is kept, being alive and
never in the module list.
Both changes are needed: the guard alone still lost the group to the
rebuild, and the refresh alone could not match a group whose name had
already been rewritten.
Note this touches IPCore paths shared with SDI/AJA output and
multi-monitor setups, which were not exercised here. The guard assumes a
device reporting itself as its own physical device carries no monitor
information -- true for viewport devices, and deviceChanged() only ever
runs on the control device today, but a real physical device
legitimately is its own physical device.
Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
QTVulkanVideoDevice now compares the GL physical-device UUID against the Vulkan physical device via VulkanWindow::physicalDeviceMatchesUUID(). If the UUIDs do not match (or cannot be queried), it falls back to the CPU pack-and-upload path instead of exporting GL memory to a different GPU. The result is cached and can be reset. VulkanWindow now creates a Vulkan 1.1 instance so vkGetPhysicalDeviceProperties2 and device UUIDs are available, and tightens physical-device selection to require a graphics+present queue family, VK_KHR_swapchain, and a 10-bit surface format when applicable. RvDocument's Vulkan-to-OpenGL fallback now preserves the requested display depth except when recovering from a 10-bit Vulkan failure, in which case it explicitly falls back to 8-bit OpenGL with clearer log messages. MuUICommands::colorAtCursor now reads the cursor pixel through the active GLVideoDevice with glReadPixels instead of requiring a GLView and QImage, making it backend-agnostic. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
With no context current, glGetError() on Windows returns GL_INVALID_OPERATION for every call, for as long as nothing is current. TWK_GLDEBUG reported that as a GL error at each instrumented site that followed, so one missing context surfaced as a dozen copies of itself, attributed to whichever innocent line checked next. A missing context during presentation teardown showed up as an error inside makeCurrent(), a frame late and in the wrong place. twkGlAnyContextIsCurrent() answers the question directly. QOpenGLContext::currentContext() only knows about contexts Qt made current, and TwkGLFFBO's FBOVideoDevice binds its own natively, so trusting Qt alone would claim "no context" while a perfectly good one is current and would suppress the real errors the macro exists to print. glGetString() settles the cases Qt cannot see, and is only reached when Qt says no. twkGlPrintError() now checks that first and, when nothing is current, reports once per episode at the first site to notice, resetting when a context returns so a later episode is not swallowed. Both are declared outside the NDEBUG guard. TWK_GLDEBUG still compiles out in release -- polling glGetError() at every instrumented site is a debug-only cost -- but "no current GL context" is not instrumentation. It fires only when GL work cannot land, which is a fault in a release build too, and the callers that need to say so are compiled in both. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
glDeleteFramebuffers() and friends are silent no-ops with no context current: the C++ object goes away, the driver's does not, and nothing says so. Teardown paths are where this bites, because they run from destructors and event callbacks rather than from inside a render, so nothing has arranged a context for them. Fixing that per call site does not converge -- each one found reveals the next, because nothing in the code states the invariant. This scope states it once. Open it at the top of anything that deletes GL objects and the question stops being the caller's problem. It resolves a context in three steps: already current, so do nothing -- the common case, a pointer compare, safe to put on paths that also run mid-render; else makeCurrent() on a supplied device, which is cheaper and is the context the objects were most likely created under; else a process-lifetime fallback. Destruction restores what was current, which -- because it only acquires when nothing was -- means making nothing current again. The fallback is what lets this work in destructors that have already had their device pointers cleared out from under them, which is exactly where the problem lives. It insists on QOpenGLContext::globalShareContext(): GL names belong to a share group, so deleting an FBO under a context outside the group that created it does nothing at all, and a non-sharing fallback would look like a fix while behaving like the bug. It is created once and never destroyed, because the paths needing it run while the application object is being torn down, so any owner freeing it at static-destruction time would free it either too early to be useful or after QGuiApplication has gone. If no context can be resolved it reports once and does nothing. It never throws and never aborts: a teardown helper must not be the reason a process fails to exit. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
Leaving presentation mode resizes the main view, which reaches Session::deviceSizeChanged() from the resize rather than from a render, so no context is current. It flushed the renderer's entire ImageFBO pool into nothing: the C++ objects went away, the driver's did not, and the only sign was a GL_ERROR from ~GLFBO after the fact. Open a GLContextScope at the owner instead of at each caller. ImageFBOManager::flushImageFBOs() and destroyImageFBO() cover every path that reaches them, present and future, so the hand-placed makeCurrent() in Session::clearVideoDeviceCaches() and the context restore in ~RvDocument go away rather than accumulating. Session passes the device it already knows -- deviceSizeChanged() its argument, clearVideoDeviceCaches() the control device -- which is cheaper than the fallback and is the context the objects were created under. ImageRenderer needs the same at three more points. Device::clearFBOs() deletes the FBO ring buffer, clearState() deletes the program cache immediately after flushing the pool -- so the pool's own scope closing would leave a gap -- and ~ImageRenderer covers both plus the program cache object. Session clears the renderer's device pointers before destroying it, so by then it has nothing to ask and the scope falls back to its own context; that case is the reason the fallback exists. ~GLFBO keeps a backstop. With no context current it reports once, naming the destruction site, and skips the GL calls rather than pretending the names were released. It asks only for an FBO that owns GL names: the GLFBO(const GLVideoDevice*) constructor builds a handle onto whatever the device has bound, with no id and no PBO, so destroying one issues nothing and needs no context. Checking the context before checking for work reported a leak that cannot happen on the ordinary path where a device outlives its window. It reports rather than asserts. A leaked FBO is worth a line of output; it is not worth aborting a shutdown that would otherwise have completed, least of all in the debug build someone is using to diagnose that shutdown. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
UninitPBOPools() never released a PBO. PBOWrap::uninitPBOPool() only flipped an initialized flag, leaving gPoolToGPU and gPoolFromGPU -- both file-scope statics -- to delete their buffers from ~GLPixelBufferObjectPool at static destruction, after main() has returned. Qt is gone by then and no context can be obtained on any platform, so every glDeleteBuffers() in there was a silent no-op and the driver reclaimed the memory with the process. Give the pool a clear() that does the release, and call it from uninitPBOPool() so it happens while the application is still up. clear() empties the containers and resets the accounting as well as deleting, because the destructor still runs later and would otherwise walk the same entries a second time. UninitPBOPools() is called from main() once the event loop has returned, so the views and their contexts are already gone and nothing is current. A GLContextScope supplies the fallback context -- still available there, since the QApplication outlives the call -- which also covers the GLSyncObject fences deleted alongside each buffer. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
RvConsoleWindow installs its cout/cerr redirect under
#if defined(NDEBUG) || defined(PLATFORM_WINDOWS)
and took it down under
#if defined(NDEBUG) || !defined(PLATFORM_WINDOWS)
A Windows debug build is the one combination where those disagree, so
there the redirect went in and never came out. ConsoleBuf stayed on
cout and cerr with m_console pointing at the destroyed window.
main() deletes RvApplication before finalizePython(), and Py_Finalize's
garbage collection can still write -- a ResourceWarning from an
unclosed socket, for one. That write reached ConsoleBuf, followed
m_console into freed memory, and locked a QMutex whose bits happened to
read "contended". Nothing ever releases it, so RV hung on the way out
and had to be killed. It also meant any late shutdown output was lost.
Guard on having installed the redirect rather than on a second
attempt at the same #if. m_stdoutBuf and m_stderrBuf are non-null only
if the install ran, and processLastTextBuffer() nulls them if it got
there first, so this is correct in every build and safe to run twice.
Delete the ConsoleBuf too, so nothing is left pointing at the window.
Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
RV never calls quit(). It relies entirely on Qt's quitOnLastWindowClosed, and QApplicationPrivate::shouldQuit() counts every visible top-level widget carrying WA_QuitOnClose, which is on by default. So any auxiliary window left visible when the session window goes stops exec() from ever returning: the process stays up with a stray dialog on screen and has to be killed. The console reached that state on its own. RvConsoleWindow::processTextBuffer() calls show() and raise() for any line the show-on preference considers interesting, and at showOn=3 processLine() returns true for every line. It runs from a queued event, so it lands after ~RvDocument has closed the console, reopening it as the last visible window while the rest of shutdown is still producing output. Two changes, because either alone leaves a hole. WA_QuitOnClose is cleared on the console, the preferences dialog and the profile manager, so a log or settings window can never hold the process open however it came to be visible -- including a user deliberately leaving one open. And RvApplication::isShuttingDown(), set in ~RvDocument before the last document closes those windows, gates the auto-show so no console appears on the way out. The attribute is applied to all three because they are closed in the same ~RvDocument block and have exactly the same exposure; fixing only the one that was observed would leave two identical bugs behind. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
detachAudioOutputDevice() hands work to the audio thread with Qt::BlockingQueuedConnection three times over -- emitStopDevice(), emitStopAudio(), and the delete of the output objects -- and none of them checked that the thread could run it. A BlockingQueuedConnection blocks until the target's event loop dispatches the call, so a thread that never started, whose createAudioOutput() failed, or that has already left exec() never releases the caller. detachAudioOutputDevice() then never reaches its own quit() and wait(). Calling from the audio thread itself would deadlock outright. canBlockOnAudioThread() answers that before each handoff: the thread is running, it has an event dispatcher, and it is not us. Skipping the stops costs nothing when it is false, since a thread not running its loop is not playing either. The delete becomes best-effort rather than all-or-nothing. It is still marshalled when there is a loop, which is what keeps Qt6 debug builds from tripping QObject::~QObject()'s cross-thread assertion on the QIODevice that QWindowsAudioSink parents. When there is not, the now idempotent deleteAudioOutputObjects() runs after wait() has returned and the thread is finished. A Qt warning on the way out beats never getting out. wait() is bounded and reports once if the bound is reached, so a wedged audio thread degrades to a slow exit rather than no exit. This is not the hang reported on Windows -- a captured stack put that in RvConsoleWindow -- but it is the same failure waiting to happen, and it is not reachable by inspection alone. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
front() on an empty container is undefined behaviour, and a hard assert
("front() called on empty vector") in an MSVC debug build. Callers pair
data()/rawData() with size(), so reporting the absence of storage with a
null pointer makes a zero-length copy out of or into a cleared property
a no-op rather than a crash.
Release builds already returned null here for any property that never
held a value, so this only makes the existing behaviour well defined.
Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
- VulkanWindow: failSharedImage lambda for the getSharedImageInfo error paths; ScopedDiagTimer for the -debug gpu timings; DiagStats struct in an anonymous namespace, with std::optional instead of the slot parallel arrays and the -1.0 sentinel; presentWaitTimeout, waitForFrameFence, acquireFrameImage and presentFrame shared by both present paths; beginOneTimeCommands; kMemoryHandleType and kSemaphoreHandleType defined once; descriptive width/height names. - QTVulkanVideoDevice: resolveGL<>, cleanupAllSharedGLObjects, drainGLErrors and toDevicePixels helpers; std::optional for m_refresh, m_loggedPresentPath and m_glVulkanDeviceMatch; descriptive names. - RvDocument: createGLView and configureViewWidget helpers replace the three copies of the GLView construction and widget setup. - GLWindow: -debug gpu accumulators in an anonymous-namespace struct. - Descriptive names in IPGraph, RvApplication and GLView. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
…sized The presentation output only shows what the control session composites into it, and nothing requested a frame once its surface became usable, so it stayed black until the main viewport repainted (e.g. on mouse move). On GL, transfer() cannot run before ScreenWindow's first paint creates its backing FBO; on Vulkan, the first expose presents an empty FBO. Add DesktopVideoDevice::requestOutputRecomposite() and call it from ScreenWindow::paintGL() when the painted size changes, and from VulkanWindow's passive expose and resize. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
- VulkanWindow: findTenBit returns std::optional instead of a bool plus out-parameter, and the two search passes become one loop. - DesktopVideoDevice: cloneForSource owns the clone with unique_ptr until it is cached; ScreenView's container is a constructor local. - GLContextScope: note that the fallback statics are GUI-thread only. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
…ard unbind - DesktopVideoDevice: m_translator is now a std::unique_ptr<QTTranslator>; update construction, destruction and assignment accordingly. - Add a null check for m_viewDevice in unbind() to avoid dereferencing it before the view has painted. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
cedrik-fuoco-adsk
added a commit
that referenced
this pull request
Oct 2, 2026
### fix: Make the ShibokenHelpers.cmake patch safe to re-run ### Linked issues None. Found on the macOS CI job of #1427 ([run 36735866190](https://github.com/AcademySoftwareFoundation/OpenRV/actions/runs/36735866190/job/109957361937)). ### Summarize your change. `make_pyside6.py` now skips the `ShibokenHelpers.cmake` Clang warning patch when the source tree is already patched, and only renames the file to `.old` once the replacement is known to apply. ### Describe the reason for the change. CI restores `_build/_deps` from the dependency cache. That cache can hold a PySide6 source tree that an earlier run already patched. The patch step only matched the unpatched block, so it raised: ``` RuntimeError: Failed to patch .../ShibokenHelpers.cmake: the expected compiler-id block was not found. ``` It also renamed `ShibokenHelpers.cmake` to `.old` before checking, so a failed match left the file missing and broke every later run on the same tree. ### Describe what you have tested and on which operating system. macOS: ran the patch logic against a temporary source tree: - clean file: patched, `.old` backup written - already patched: skipped, file unchanged - unexpected content: `RuntimeError`, `ShibokenHelpers.cmake` left in place ### Add a list of changes, and note any that might need special attention during the review. - `src/build/make_pyside6.py`: read the file in place, return early if `-Wno-cast-function-type-mismatch` is already present, and rename to `.old` only after the replacement succeeds. The patch is the last step of `prepare()`, so the early return skips nothing else. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
cedrik-fuoco-adsk
marked this pull request as ready for review
October 5, 2026 18:25
cedrik-fuoco-adsk
requested review from
bernie-laberge and
eloisebrosseau
as code owners
October 5, 2026 18:25
bernie-laberge
approved these changes
Oct 7, 2026
bernie-laberge
left a comment
bernie-laberge
left a comment
Contributor
There was a problem hiding this comment.
LGTM
This was a massive undertaking. Thank you @cedrik-fuoco-adsk !
Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> # Conflicts: # src/lib/app/RvCommon/GLWindow.cpp
Contributor
Author
|
The last commit fixed the file conflict with the main branch. |
…small fixes) - VulkanView: restore explicit constructor and the showEvent parameter name. - nullptr instead of 0 in DesktopVideoDevice::open()/close() and the RvDocument view pointers. - VulkanWindow: constexpr kDefault; VK_API_VERSION_* instead of the deprecated VK_VERSION_* macros. - Named constants for the repeated 255.0f (MuUICommands) and 1000.0 (GLWindow::paintGL) literals. - RvDocument::setDisplayOutput(): switch instead of nested ternaries. - GL.h: drop top-level const from the twkGlPrintError declaration. - GLContextScope: 2026 copyright. - GLPixelBufferObjectPool: std::scoped_lock instead of unique_lock. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
Every method that overrides a base virtual is now marked override, including the destructor. Converting the whole class avoids -Winconsistent-missing-override. redraw, redrawImmediately, videoFormatFromData, makeCurrent and qtScreen stay virtual because DesktopVideoDevice introduces them. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
Both headers used #pragma once on main. Switching them to #ifndef guards was unrelated churn. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
FBOMap now holds std::unique_ptr<GLFBO>, so cloneForSource() and releaseFBOClones() no longer delete clones by hand. releaseFBOClones() still clears the map with the view context current, and close() still calls it first. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
VulkanDesktopVideoDevice.cpp and VulkanWindow.cpp now qualify std names explicitly. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
Extract collectDevices, findMatchingDevice (returns std::optional instead of a sentinel index), dropStaleOutputDevice, detachDisplayGroup and uniqueDisplayGroupName. Behaviour is unchanged. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
Contributor
Author
|
@eloisebrosseau All comments have been addressed! Thank you! |
- QTVulkanVideoDevice: add kFallbackPixelSize for the repeated 128 fallback size. - VulkanWindow: add kMsPerSecond for the repeated 1000.0 ms conversion factor. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
eloisebrosseau
left a comment
eloisebrosseau
left a comment
Contributor
There was a problem hiding this comment.
I think you might have missed this comment: #1427 (comment). Also, I see the last constant variables you added were named following Google C++ Style Guide which is not used widely in the codebase, and not even used everywhere in the changes on this branch. I know there seams to be more than one convention used in the codebase, for legacy reasons, but I think we should try to follow one convention when adding new code. I believe the mostly used convention is camelCase, but we can talk about it if you want.
GLContextScope.h, VulkanWindow.h and VulkanDesktopVideoDevice.h switch from #ifndef guards to #pragma once, as newer OpenRV headers do. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
Mutex and ScopedLock now match the FBOMap alias next to them. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
Follow the codebase convention for constexpr constants instead of the kName prefix: framesInFlight, msPerSecond, memoryHandleType, semaphoreHandleType, maxStaleSeconds, fallbackPixelSize, maxByteValue and defaultDepth. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
Contributor
Author
Not sure why it's not flagged as |
eloisebrosseau
approved these changes
Oct 13, 2026
cedrik-fuoco-adsk
merged commit Oct 13, 2026
32a8cee
into
AcademySoftwareFoundation:main
18 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat: SG-43947: Present through Vulkan for 10-bit output on Linux and Windows
Linked issues
Depends on #1319 (10-bit Vulkan presentation, Linux + Windows). The diff below is described relative
to that PR.
Summarize your change.
#1319 gives the main viewport a 10-bit Vulkan present path. This PR extends that path to the
presentation output (the second display), and makes the backend choice something RV can change
while it is running instead of fixing it at startup.
VulkanDesktopVideoDevicepresents the second displaythrough a Vulkan swapchain. Until now it went through the OpenGL
ScreenView, which capped it at8 bits even when the main view was already 10-bit.
devices are rebuilt onto the new backend, and the session's display groups, colour pipeline and
display profile survive the swap.
VulkanWindow(QWindow) embeddedby a
VulkanViewcontainer throughcreateWindowContainer(). This replaces feat: SG-43127: SG-43594: 10-bit Vulkan presentation (Linux + Windows) #1319'sWA_NativeWindow/WA_PaintOnScreenQWidgetand mirrorsGLWindow/GLView. It is whatmakes safe teardown and a passive, doc-less output window possible.
already existed on the GL path, but toggling presentation mode and swapping backends made them
reachable or consistently reproducible.
Describe the reason for the change.
With #1319 alone:
rebuildGLView()at 10 bits. On theaffected drivers that fails validation, shows a misleading "Display Configuration is Invalid"
dialog and resets the preference. The backend could only change after a restart.
went black.
vkDestroySwapchainKHR. Qt destroys theplatform surface before the view's destructor runs.
Describe what you have tested and on which operating system.
Tested on Linux and Windows
Add a list of changes, and note any that might need special attention during the review.
If possible, provide screenshots.