Skip to content

Add OBS_content_takeScreenshot: save a PNG of a canvas' program output - #1786

Merged
summeroff merged 8 commits into
stagingfrom
screenshot-output
Sep 30, 2026
Merged

summeroff merged 8 commits into
stagingfrom
screenshot-output

Conversation

@au-sf

@au-sf au-sf commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds OBS_content_takeScreenshot so Desktop can offer OBS's "Screenshot Output": a PNG of a canvas' program output at base resolution, named by the user's Filename Formatting, never overwriting.

NodeObs.OBS_content_takeScreenshot(video: IVideo | IVideo[], directory, filenameFormat, noSpace?)
  : Promise<IScreenshotResult | IScreenshotResult[]>
  • Non-blocking. The IPC call returns at once. A tick callback renders and stages the canvas on the graphics thread in one frame and maps it in the next; an encoder thread writes the PNG with libobs' gs_save_png_file (32.1.1sl12). The promise resolves when the file exists.
  • One or all canvases. An array captures every canvas from the same frame; names get a 1920x1080 suffix.
  • Creates subfolders named by the format. Errors reject the promise: no active video, timeout, write failure, more than 4 in flight.

The first version rendered from the IPC thread, which could deadlock with the graphics thread (graphics lock vs mixes_mutex) and stalled all IPC while encoding.

Tests

  • electron-mocha ... test_osn_video.ts: 14/14 on Windows x64 (9 screenshot tests: PNG size/IHDR, (2) dedupe, noSpace, subfolder, multi-canvas, returns before the file exists, argument errors)
  • clang-format 18.1.3 clean; tsc clean
  • Not run: macOS, Desktop end to end

Asana Link

https://app.asana.com/0/1207748235152481/1216010475168255

@CLAassistant

CLAassistant commented Sep 26, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

au-sf and others added 2 commits September 29, 2026 12:31
Mirrors OBS Studio's "Screenshot Output": renders the main mix of the given
video context at base resolution, stages it, and writes
"Screenshot <Filename Formatting>.png" into the given directory, appending
" (2)", " (3)"... (or "_2"... with noSpace) when the name is taken.
PNG encoding uses the vendored public-domain stb_image_write.h; file I/O goes
through os_fopen so UTF-8 paths work on Windows. Exposed on NodeObs as
OBS_content_takeScreenshot(video, directory, filenameFormat, noSpace?) and
covered by tests in test_osn_video.ts.
libobs 32.1.1sl12 exports a PNG writer backed by its FFmpeg, so the vendored stb_image_write is no longer needed.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
summeroff and others added 2 commits September 29, 2026 14:35
The old handler rendered and encoded synchronously on the IPC thread while
holding the graphics lock, stalling the whole app for the GPU readback. A
ScreenshotManager now stages each canvas over two ticks (queue the copy, map
it once it has landed) and encodes PNGs on a dedicated thread, so
OBS_content_takeScreenshot returns a promise instead of blocking, and can
capture several canvases from the same frame in one call.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…lling

Unqueried jobs kept their slot forever, so a partial failure or a reload mid-shot eventually made every screenshot fail as busy. Also register the tick callback outside the manager lock, since ticks run under libobs' draw_callbacks_mutex.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@summeroff

Copy link
Copy Markdown
Contributor

@au-sf heads-up: I rebased this onto staging and pushed two commits on top of yours; the description is updated to match.

Why the redesign: the synchronous version could deadlock. The IPC thread took the graphics lock, and obs_render_texture → obs_video_mix_get then took mixes_mutex. The graphics thread takes the same two locks in the opposite order in output_frames() (mixes_mutex held while each mix enters graphics). lib-streamlabs-ipc also serves one call at a time, so the ~100 ms encode stalled every other Desktop call, not just the caller.

Now:

  • A tick callback renders and stages on the graphics thread (the OBS ScreenshotObj approach), then maps on the next tick.
  • An encoder thread writes the PNG with libobs' gs_save_png_file (no stb).
  • The JS call returns a Promise and accepts one IVideo or an array.

On the earlier review questions:

  • Capturing the Display surface would miss the goal: it's window-sized, draws the HUD, shows the preview in studio mode, and needs a Display to exist.
  • The canvas mix texture is rendered every frame whether or not any Display exists.
  • The render context and mode are no longer touched.

desktop#6231 will need to await the call; I'll push that there once this is released.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Unresolved path containment, connection races, and encoder lifecycle risks in the new capture pipeline require human review.

Review effort: Balanced
Findings: 3 High severity · 3 Medium severity

Open (6)
What changed in this PR

This PR adds a promise-based API for saving a canvas’s program output as a PNG, so Desktop can offer OBS’s “Screenshot Output” feature.

Changes:

  • Capture one or more canvases through a graphics tick and encode PNGs on a background thread.
  • Add screenshot IPC calls, Node bindings, and TypeScript result types.
  • Add screenshot tests and include the new server files in the build.
File Description
tests/​osn-tests/​src/​test_osn_video.ts Adds screenshot API tests.
obs-studio-server/​source/​osn-screenshot.hpp Declares screenshot jobs and manager.
obs-studio-server/​source/​osn-screenshot.cpp Implements capture, naming, and encoding.
obs-studio-server/​source/​nodeobs_content.h Declares screenshot IPC handlers.
obs-studio-server/​source/​nodeobs_common.cpp Registers and implements screenshot IPC calls.
obs-studio-server/​source/​nodeobs_api.cpp Shuts down the screenshot manager.
obs-studio-server/​CMakeLists.txt Includes the new server files.
obs-studio-client/​source/​nodeobs_display.hpp Declares the Node binding.
obs-studio-client/​source/​nodeobs_display.cpp Submits jobs and resolves screenshot promises.
js/​module.ts Documents and types the screenshot API.
js/​module.d.ts Exposes the generated TypeScript declarations.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread obs-studio-client/source/nodeobs_display.cpp Outdated
Comment thread obs-studio-server/source/osn-screenshot.cpp
Comment thread obs-studio-server/source/osn-screenshot.cpp
Comment thread obs-studio-server/source/osn-screenshot.cpp Outdated
Comment thread obs-studio-server/source/osn-screenshot.cpp
Comment thread obs-studio-server/source/osn-screenshot.cpp Outdated
Keeps the client connection on the JS thread, counts queued encodes toward the job cap, and stops holding the job mutex across GPU work so a slow frame cannot stall IPC polls or shutdown.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Canvas removal can overlap capture, and the native graphics and threading paths need human validation, including on macOS.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (6)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Bound graphics-thread staging copy work

obs-studio-server/​source/​osn-screenshot.cpp:419

This loop maps and copies every staged canvas on the graphics tick before returning, including rewriting each pixel's alpha. Four 3840×2160 screenshots copy about 127 MiB in one tick, so PNG encoding off-thread does not prevent a noticeable render stall. Consider staging all canvases together but spreading readback/copy work across ticks, or another design that bounds work on the render-critical thread.

Comment thread obs-studio-server/source/osn-screenshot.cpp
Comment thread obs-studio-client/source/nodeobs_display.cpp Outdated
Comment thread obs-studio-client/source/nodeobs_display.cpp Outdated
Comment thread obs-studio-server/source/osn-screenshot.cpp
Unwrap doesn't check class, so another wrapped OSN object could be read as a Video. The per-pixel alpha pass now runs on the encoder thread to shorten the graphics tick.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@summeroff

Copy link
Copy Markdown
Contributor

Re Copilot "Bound graphics-thread staging copy work": 9483a6a moves the per-pixel alpha rewrite to the encoder thread; the graphics tick now only maps and memcpys rows.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The graphics and encoder lifecycle needs human review, and capture correctness and integration remain unverified.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (4)

In code that hasn't changed since last review

Medium severity Avoid blocking one libuv thread per screenshot request

obs-studio-client/​source/​nodeobs_display.cpp:447

Each screenshot request starts its own AsyncWorker, which holds a libuv pool thread while it synchronously polls and sleeps here. Four concurrent single-canvas requests can therefore occupy all four default pool threads for up to the server's 10-second timeout. Unrelated asynchronous file or crypto operations may stall even though the JavaScript event loop remains free. Poll all outstanding jobs from one worker (or use a nonblocking polling mechanism) rather than parking a pool thread for each request.

Medium severity Verify captured PNG pixels from known-color sources

tests/​osn-tests/​src/​test_osn_video.ts:257

The test checks the PNG header and dimensions, but the canvas has no source attached and no test checks the saved pixels. A blank or stale frame would pass all the screenshot tests, even though the API promises to capture program output. Render a known-color source, decode the PNG, and verify its pixel values; use distinct colors to check both canvases in the array case.

Medium severity Test atomic batch rejection when exceeding job limit

tests/​osn-tests/​src/​test_osn_video.ts:359

The four-job limit is testable without timing five separate calls. Passing five entries in one array (even five references to this valid context) makes Submit reject the batch before reserving any jobs. Replace this note with a test that checks rejection, no files created, and a successful subsequent single-canvas capture; the documented limit and atomic rejection are otherwise untested.

Low severity Document graphics-thread capture latency accurately

js/​module.ts:2508

This says capture runs off the graphics thread and cannot block rendering, but RunTick renders, stages, maps, and copies pixels on the graphics thread (osn-screenshot.cpp:342-425). Only PNG encoding runs on a separate thread. Describe the two graphics ticks and background encoding so callers have an accurate latency expectation.

The doc misdescribed where capture and encoding run, and the 4-job cap rejection had only a comment explaining why it was untested.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@summeroff

Copy link
Copy Markdown
Contributor

Re Copilot's latest review (05f5354):

  • Doc now describes the two graphics ticks and background encoding accurately.
  • Added a test for the 4-job limit: a 5-entry batch rejects, writes nothing, and a following capture succeeds.
  • Pixel-verification test and the per-request libuv thread are left as follow-ups.

A bare "busy" didn't tell callers about the 4-job cap, and server failures surface as promise rejections, not synchronous throws.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@summeroff
summeroff merged commit 92ac217 into staging Sep 30, 2026
20 checks passed
summeroff added a commit that referenced this pull request Sep 30, 2026
#1786 and 3fde7d7 each added these imports and landed back to back, so the integration tests fail to load with "Identifier 'fs' has already been declared" on staging.

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants