Conversation
…t layout and IDE-style stats
There was a problem hiding this comment.
Pull request overview
This PR migrates the Windows desktop client UI from SDL3/ImGui to Slint, updates the build/CI pipeline to fetch and build Slint (including Rust as a prerequisite), and reorganizes WebRTC “test” media helper code into a runtime media_support/ module.
Changes:
- Replace the SDL3/ImGui UI with a Slint UI (new
.slintcomponents +SlintAppbackend) and wire it intomain. - Update rendering pipeline integration to push frames into Slint images (new
SlintVideoRendererbridge + enhancedVideoRendererframe pacing/scaling). - Update build + CI scripts to fetch Slint via CMake
FetchContent, cache dependencies, and require a Rust toolchain.
Reviewed changes
Copilot reviewed 32 out of 35 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
ui/*.slint |
New Slint UI components (window layout, video stage, sidebar, overlays, logs). |
src/ui/backend/slint_app.cc / include/ui/slint_app.h |
New Slint UI backend replacing SDL app; binds controller callbacks and drives UI state. |
src/ui/backend/slint_video_renderer.cc / include/ui/slint_video_renderer.h |
Bridge that consumes VideoRenderer frames and updates Slint Image properties on the event loop. |
src/videorenderer.cc / include/videorenderer.h |
Adds output scaling, frame-rate limiting, and a “frame available” callback for UI redraw triggering. |
src/webrtcengine.cc |
Adjusts COM initialization strategy for Windows UI thread requirements. |
src/webrtcengine_media_pipeline.cc |
Updates includes to the new media_support/* paths. |
src/media_support/* + include/media_support/* |
Moves former WebRTC test helpers into runtime “media_support” module and updates includes. |
src/main.cc |
Switches app entrypoint from SdlApp to SlintApp. |
CMakeLists.txt / cmake/FetchSlint.cmake |
Fetches and configures Slint from source, links Slint::Slint, and adds slint_target_sources. |
build.cmd |
Adds Rust toolchain checks and improves stale-cache detection; passes Slint tag into CMake. |
.github/workflows/build.yml |
Installs Rust, adds Rust caching, and expands third-party caching key/scope for Slint. |
cmake/ui_paths.h.in |
Adds a template header (currently not wired up elsewhere). |
README.md |
Documents Slint UI migration and Rust toolchain prerequisite. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| SlintApp::~SlintApp() { | ||
| shutting_down_.store(true); | ||
| StopTimers(); |
There was a problem hiding this comment.
SlintApp::~SlintApp() stops the VideoRenderer instances but never detaches them from SlintVideoRenderer. Since VideoRenderer::Stop() currently does not clear frame_available_callback_, any in-flight OnFrame() call could still invoke a callback that references video_renderer_bridge_ during/after destruction. DetachLocalRenderer()/DetachRemoteRenderer() should be called (or Stop() should clear the callback) to make shutdown robust against use-after-free races.
| StopTimers(); | |
| StopTimers(); | |
| if (video_renderer_bridge_) { | |
| video_renderer_bridge_->DetachLocalRenderer(); | |
| video_renderer_bridge_->DetachRemoteRenderer(); | |
| } |
| @@ -33,15 +53,19 @@ void VideoRenderer::Stop() { | |||
| } | |||
|
|
|||
| latest_frame_ = Frame{}; | |||
| standby_frame_ = Frame{}; | |||
| latest_frame_.frame_id = next_frame_id_++; | |||
| frame_dirty_ = true; | |||
| last_frame_time_ = std::chrono::steady_clock::time_point{}; | |||
| } | |||
There was a problem hiding this comment.
VideoRenderer::Stop() removes the sink and clears frame state, but it leaves frame_available_callback_ intact. If the renderer is stopped during shutdown while another thread is still delivering/finishing an OnFrame() call, the callback can still fire and call back into UI code that may already be tearing down. Consider clearing frame_available_callback_ inside Stop() (under the mutex) to avoid use-after-free risks during shutdown.
| set "RUST_VERSION=" | ||
| for /f "tokens=2 delims= " %%I in ('rustc --version') do ( | ||
| set "RUST_VERSION=%%I" | ||
| goto rust_version_found | ||
| ) | ||
|
|
||
| :rust_version_found | ||
| if not defined RUST_VERSION ( | ||
| echo [build.cmd] Failed to parse the installed rustc version. | ||
| exit /b 1 | ||
| ) | ||
|
|
||
| for /f "tokens=1,2 delims=." %%I in ("%RUST_VERSION%") do ( | ||
| set /a RUST_VERSION_MAJOR=%%I | ||
| set /a RUST_VERSION_MINOR=%%J | ||
| ) | ||
|
|
||
| if %RUST_VERSION_MAJOR% GTR 1 exit /b 0 | ||
| if %RUST_VERSION_MAJOR% EQU 1 if %RUST_VERSION_MINOR% GEQ 88 exit /b 0 | ||
|
|
||
| echo [build.cmd] Rust %RUST_VERSION% is too old. Install Rust 1.88 or newer before building the Slint UI. |
There was a problem hiding this comment.
The Rust version parsing assumes rustc --version is strictly numeric (e.g. 1.88.0). For toolchains like 1.88.0-nightly (or other suffixes), set /a RUST_VERSION_MINOR=%%J will fail because %%J is not a pure integer. Consider stripping any non-numeric suffix before the numeric comparison, or using rustc -Vv and parsing release: instead.
| #ifndef GENERATED_UI_PATHS_H_GUARD | ||
| #define GENERATED_UI_PATHS_H_GUARD | ||
|
|
||
| #define PEERCONNECTION_UI_DIR "@PEERCONNECTION_UI_DIR@" | ||
|
|
||
| #endif // GENERATED_UI_PATHS_H_GUARD |
There was a problem hiding this comment.
This template header is added but never referenced or generated anywhere in the repo (no configure_file() usage and no references to PEERCONNECTION_UI_DIR). If it’s intended to provide runtime UI paths, wire it into CMake (configure + install/copy) and include it from the relevant code; otherwise consider removing it to avoid dead build artifacts.
| #ifndef GENERATED_UI_PATHS_H_GUARD | |
| #define GENERATED_UI_PATHS_H_GUARD | |
| #define PEERCONNECTION_UI_DIR "@PEERCONNECTION_UI_DIR@" | |
| #endif // GENERATED_UI_PATHS_H_GUARD | |
| /* Deprecated header template. | |
| * | |
| * This file was previously intended to be used as a CMake template | |
| * (e.g. via configure_file) to provide a PEERCONNECTION_UI_DIR macro, | |
| * but it is not currently wired into the build or referenced by code. | |
| * | |
| * It is intentionally left empty to avoid maintaining a dead build | |
| * artifact while preserving the file path for any external tooling | |
| * that might still reference it. | |
| */ |
This pull request migrates the project’s UI from SDL3/ImGui to Slint, updates the build system to fetch and integrate Slint, and reorganizes media support/test code for clarity. It also introduces Rust as a build prerequisite (required by Slint), updates documentation, and improves dependency caching and validation in both CMake and CI/CD workflows.
UI Migration and Refactoring:
CMakeLists.txt,README.md, renaming/movingsdl_app.*toui/slint_app.*, etc.). [1] [2] [3] [4]Build System and Dependency Management:
FetchContent, disables unnecessary Slint features, and integrates Slint into the build process (cmake/FetchSlint.cmake,CMakeLists.txt). [1] [2] [3]build.cmdto require Rust (version 1.88+), checks for its presence, and ensures the correct toolchain is used for building Slint. [1] [2] [3]Project Structure and Code Organization:
test/directory intosrc/media_support/andinclude/media_support/for better organization and clarity, updating all includes accordingly. [1] [2] [3] [4] [5]Documentation and Developer Experience:
README.mdto reflect the new UI stack (Slint), the Rust requirement, and improved build notes regarding Slint source fetching. [1] [2] [3]Build and Caching Improvements:
These changes modernize the UI stack, improve build reliability, and make the codebase easier to maintain and extend.