Repository navigation
Skip obs_set_video_info when a canvas' requested settings are unchanged - #1788
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Failed applies can leave stale cached settings, and the test does not verify that reinitialization was skipped.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Optimizes repeated video-context updates by skipping unchanged libobs reinitialization.
Changes:
- Caches successfully applied settings per canvas.
- Refreshes video levels on skipped updates.
- Adds repeated-update coverage.
| File | Description |
|---|---|
obs-studio-server/source/osn-video.cpp |
Adds video-setting comparison and caching. |
tests/osn-tests/src/test_osn_video.ts |
Tests repeated settings assignments. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
libobs frees video for every canvas on a failed set/remove/reset, so a per-canvas record could skip the re-apply needed to recover. Tests now assert the skip path via the server log. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
|
Review follow-up (pushed in c2ad4d3):
|
Comment on lines
+353
to
+358
| // libobs video state is process-wide, so a failed call can leave every canvas torn down. | ||
| void osn::Video::InvalidateAppliedVideoInfo() | ||
| { | ||
| std::lock_guard<std::mutex> lock(lastAppliedVideoMutex); | ||
| lastAppliedVideo.clear(); | ||
| } |
Contributor
There was a problem hiding this comment.
Fixed in 6081996: doResetVideoContext now invalidates the applied video info after obs_set_video_info (success or failure) and in the catch path.
doResetVideoContext calls obs_set_video_info directly, so a failed reset left the skip-unchanged cache stale and a later re-apply of the old settings could be skipped while video was torn down. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
summeroff
approved these changes
Sep 30, 2026
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
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.



Summary
osn::Video::SetVideoContextpasses every request straight toobs_set_video_info, and libobs does not short-circuit: each call stops the graphics thread, frees the canvas' video and re-initialises it ([VIDEO_CANVAS] wait obs_graphics_thread to stop,obs_free_video), about 130 ms per call on an M4 Max, whether or not a single value changed. Streamlabs Desktop re-applies unchanged settings more than it means to: at startup it pushed each display's settings twice (fixed on the app side in streamlabs/desktop#6235), andupdateObsSettings/refreshVideoSettingspush the current values on every settings write. This change makes an identical re-apply a no-op inside OSN, so every caller benefits and the app-side fix becomes belt and braces.What it does
Server (
osn-video.cpp).SetVideoContextbuilds the fullobs_video_infoexactly as before, including theoutput_format/colorspace/rangeit reads frombasic.ini, and then compares it with the settings last applied successfully to that canvas. If every field matches (graphics_module, fps num/den/type, base and output size, output format, colorspace, range, scale type, adapter, gpu_conversion), it logs[VIDEO_CANVAS] Set video context skipped for 0x…: settings unchanged, refreshes the SDR/HDR video levels from config as the success path does, and returnsOkwithout callingobs_set_video_info,autoOptimizer::CancelActiveSession()orstopConnectingStreamingOutputs().The "last applied" record is OSN's own (
static std::unordered_map<const obs_video_info *, obs_video_info>behind a mutex), written only afterobs_set_video_inforeturnsOBS_VIDEO_SUCCESSand erased inRemoveVideoContexton both paths that free the canvas id. Nothing is read from libobs'initializedflag, so a freshly created canvas always gets its first real apply even if the request happens to equal the defaults, a canvas whose apply failed retries next time, and a change made throughbasic.ini(colour format, colour space, range) still shows up as a difference because those fields are part of the comparison. The levels refresh on the skip path keepsSdrWhiteLevel/HdrNominalPeakLeveledits taking effect exactly when they did before.Test (
tests/osn-tests/src/test_osn_video.ts). "Re-applying identical video settings is accepted and later changes still apply": sets a context, re-applies the same object and an equal copy, reads back unchanged, then applies a real change and reads that back. In the server log the two re-applies produce theskippedline and the third call goes through.Files changed:
obs-studio-server/source/osn-video.cpp,tests/osn-tests/src/test_osn_video.tsWhat I checked for anything relying on the old behaviour
OBS_API_initAPI's initialobs_reset_video,OBS_service::doResetVideoContext(nodeobs_service.cpp, which callsobs_set_video_infoonbase_canvasdirectly) andsaveVideoSettings'obs_reset_video.nodeobs_common.cpp):OnDeviceLostis a no-op andOnDeviceRebuiltonly resizes displays; neither re-applies video settings to force a reset.videosetter, i.e. this handler:migrateSettings,loadLegacySettings,establishVideoContext,updateObsSettings, the vertical sync insettings-v2/video.ts. None is a deliberate "re-apply to reset"; Desktop's only recovery path (videoContextError) destroys and recreates the canvas, which is a fresh canvas and therefore always applies.SetLegacySettingsonly writesbasic.iniand is unaffected.Performance Implications
Net reduction: an identical re-apply now costs a field comparison instead of a ~130 ms pipeline rebuild. Nothing changes for a request that differs in any field. Memory is one
obs_video_infoper live canvas.Verification
Unix Makefiles,RelWithDebInfo, Electron 29.3.1 headers) fromstaging(71c4c03a).osn-videosuite under electron-mocha: 6 passing, including the new test; the server log for the run shows exactly twoSet video context skippedlines.node_modules/obs-studio-nodeand the app built from plainmaster(which still double-pushes at startup): the OBS server log shows, per canvas, the firstSet video contextapplied and the secondskipped … settings unchangedwithin 1 ms, for both the horizontal and vertical canvas; the app initialised normally (2.6 s cold) and the settings OBS reported matched the app's state. With the app built from Apply a Display's Video Settings to Its OBS Context Once When Establishing It desktop#6235 instead, no skip fires, as expected.clang-format(repo style) on the touched file changed only the new code.graphics_moduleis compared as a string so the D3D11/OpenGL names both work.Notes
Two other things the same server logs showed, not addressed here: on a restart after an unclean exit,
mac-avcapture-legacy's module load blocks for ~5 s on macOS (Desktop stopped creatingav_capture_inputsources in 2025-10 and migrates old ones, soobs_add_disabled_module("mac-avcapture-legacy")beforeobs_load_all_modules2looks possible, pending a product check); andOBS_settings_getVideoDeviceson macOS still enumerates through a throwawaymacos_avcapturesource (~140 ms at startup) where the audio lists moved togetDevices()in #1691.