Skip to content

perf(desktop): stop decoding snapshots twice for accessibility - #19221

Closed
AksharP5 wants to merge 1 commit into
pingdotgg:mainfrom
AksharP5:perf/snapshot-accessibility-dimensions
Closed

AksharP5 wants to merge 1 commit into
pingdotgg:mainfrom
AksharP5:perf/snapshot-accessibility-dimensions

Conversation

@AksharP5

Copy link
Copy Markdown
Contributor

Problem

Snapshot accessibility decoded the entire captured PNG again just to read its dimensions. On Linux with Electron 44.4.5, that cost 3.77 ms for a 1440×900 PNG and 10.45 ms for a 2560×1600 PNG, synchronously on the desktop process.

Closes #19220

Change

Reuse the existing shared image-header reader. Capture backends already validate or encode the PNG; the logical-bounds fallback remains when dimensions are unavailable. Tests check persisted accessibility dimensions across GNOME, Niri, KDE, and Hyprland when captured pixels differ from logical window bounds, plus fallback behavior.

Scope and approval

This is a small, focused removal of proven redundant work under the obvious-bug exception in CONTRIBUTING.md. It changes no capture workflow, settings, contracts, or visual behavior. Searches found no matching issue or active PR; #16728 and #19193 concern different screenshot paths.

Verification

  • Compared the actual original and replacement function bodies in Electron 44.4.5 on Linux x64 with synthetic editor-pattern PNGs. Fifteen warmed decode samples per image; dimensions matched, and header lookup was below 0.001 ms. Empty, invalid, and truncated-signature inputs kept the same rounded fallback. The linked issue includes input sizes, environment, methodology, and a reproduction script.
  • vp test run apps/desktop/src/snapShot/DesktopSnapShot.test.ts packages/shared/src/imageDimensions.test.ts: 123 passed, including an additional run using Electron's Node 24.21.0. The initial run used system Node 26.11.1.
  • Scoped vp lint, vp fmt --check, desktop tsc-rs --noEmit, and git diff --check passed. Typecheck emitted only two existing suggestions in unrelated tests.
  • Independent GPT-6-Astra review at high reasoning effort found no blocking issues.

The timings isolate dimension lookup, not total capture latency or idle CPU. Live compositor captures and native macOS/Windows execution were not tested; their PNG producers and Electron's scale defaults were inspected. There is no UI change requiring before/after screenshots.

Implemented with GPT-6.1-Sol in the Codex harness. Reviewed by GPT-6-Astra at high reasoning effort.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Oct 14, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 061681a

Macroscope's review found this PR approvable — This is a small, isolated performance fix that replaces redundant PNG decoding with header-only dimension lookup while preserving the existing fallback path. Targeted tests cover both real PNG dimensions and invalid screenshots, with no product-default, schema, or deployment changes.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 14, 2026

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: c2acdbf3-054b-42de-9873-36503c689cce
📥 Commits

Reviewing files that changed from the base of the PR and between dff750b and 061681a.

📒 Files selected for processing: 2
  • apps/desktop/src/snapShot/DesktopSnapShot.test.ts
  • apps/desktop/src/snapShot/DesktopSnapShot.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

The snapshot accessibility path now reads PNG dimensions from the image header instead of decoding the PNG to get its size. Tests cover valid screenshots across four Wayland backends and an invalid GNOME screenshot.

Changes: Snapshot Dimension Lookup

Layer / File(s) Summary
Read PNG dimensions and validate fallback
apps/desktop/src/snapShot/DesktopSnapShot.ts, apps/desktop/src/snapShot/DesktopSnapShot.test.ts
snapShotImageSize uses readImageDimensions and retains its rounded, minimum-1 fallback. Wayland tests check accessibility image size and root bounds against the selected screenshot dimensions. They cover valid PNGs and an invalid-image case.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor · Severity of issue fixed: Low

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 06168

The suspected Windows image-encoding failure is not reachable, and no concrete user-impacting merge risk is established.

### Architecture Summary

Architecture risk: 🔵 Low · up to 06168

The change affects 1 system.

Changed systems: apps/desktop

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — apps/desktop (service) was modified; 2 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in apps/desktop/src/snapShot/DesktopSnapShot.test.ts: The parameterized test now exercises four Wayland backends with valid PNG screenshots and GNOME with an invalid screenshot, replacing the prior backend-only cases.
  • observed — Modified behavior in apps/desktop/src/snapShot/DesktopSnapShot.test.ts: The test chooses 1400×1040 image dimensions and a PNG when the screenshot is valid; for the invalid screenshot case, it uses the window bounds and three arbitrary bytes. The Linux capture mock now receives the selected image and window.
  • observed — Modified behavior in apps/desktop/src/snapShot/DesktopSnapShot.test.ts: The expected accessibility metadata image size now uses the selected screenshot dimensions instead of fixed width 700 and the window height.
  • observed — Modified behavior in apps/desktop/src/snapShot/DesktopSnapShot.test.ts: The expected accessibility root bounds now span the selected image dimensions rather than the fixed window width and height.
Pre-merge checks: Passed 4
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: removing redundant PNG decoding for desktop snapshot accessibility.
Description check Passed The description includes all required sections. It explains the problem, implementation, scope and approval rationale, focused verification, limitations, and tool usage. The verification details are s…
Linked Issues check Passed Issue #19220 requires header-based dimension lookup and preservation of the logical-bounds fallback. DesktopSnapShot.ts now calls the shared readImageDimensions helper instead of decoding the PNG …
Out of Scope Changes check Passed The diff changes only DesktopSnapShot.ts and its focused accessibility tests. The implementation removes redundant PNG decoding, and the tests verify the required dimensions and fallback behavior. N…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@maria-rcks

Copy link
Copy Markdown
Collaborator

Note

Written by gpt-6.1-sol on behalf of Maria

Thanks for investigating snapshot overhead. Right now we are only taking focused bug fixes from outside contributors under our limited review capacity policy in Read this first. This PR removes a redundant PNG decode, but the supplied timings isolate dimension lookup and do not demonstrate a defect in the actual capture workflow. Please keep any reproducible capture delay in the linked issue, add measurements through that workflow, and agree on a focused fix with maintainers before requesting reconsideration.

@maria-rcks maria-rcks closed this Oct 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: snapshot accessibility decodes PNGs again just to read their dimensions

2 participants