Skip to content

fix(recording): let a webcam's MJPEG mode win when it is faster or sharper - #1129

Merged
EtienneLescot merged 1 commit into
mainfrom
fix/webcam-compressed-mode-rank
Oct 12, 2026
Merged

EtienneLescot merged 1 commit into
mainfrom
fix/webcam-compressed-mode-rank

Conversation

@EtienneLescot

@EtienneLescot EtienneLescot commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

A Logitech C920 is driven at YUY2 800x448 on every preset because chooseWebcamFormat drops all compressed modes as soon as one uncompressed mode exists. The camera only reaches 1080p at 30 fps in MJPEG; its YUY2 1080p mode runs at 5 fps.

  • Compressed modes now compete like any other. Useful frame rate first, then pixels. An uncompressed mode only wins a tie. The C920 now lands on MJPEG 1920x1080@30 for the 1080p, 1440p and 2160p presets.
  • Guard: a decode cost ceiling of 1080p at 30. A compressed mode is out when width × height × its own frame rate is above that, unless the camera offers nothing cheaper. A BRIO therefore keeps its uncompressed 1080p30 rather than decoding MJPEG 4K.

Measured on a Ryzen 7 5800X, decoding MJPEG to NV12 with the helper's own source-reader setup (ReadSample per frame, unpaced):

Size avg p95
1920x1080 4.9 ms 5.3 ms
2560x1440 9.2 ms 11.4 ms
3840x2160 19.7 ms 22.7 ms

At 30 fps the frame interval is 33 ms. 1080p takes 15% of it, and 4K takes 60% on an idle desktop.

Related issue

Refs #1112

Type of change

  • Bug fix

Release impact

  • Patch

Desktop impact

  • Windows

Testing

  • webcam_format_test covers the C920, BRIO and StreamCam lists on all three presets, plus the tie-break and the decode ceiling. Built with MSVC and run: all pass. On main it fails on the three C920 cases (expected 1920x1080@30, got 800x448@30).
  • Real Logitech StreamCam, console probe with no window and nothing saved. MJPEG 1080p30 → NV12 holds 30.0 fps (ReadSample 33.3 ms, paced by the camera). On Windows 11 the Frame Server already decodes it, because it lists an NV12 copy of every MJPEG mode. The chooser still picks NV12 1920x1080@30 on that camera, as before.
  • The decode table therefore comes from synthetic MJPEG clips: the camera has no 4K mode, and its Frame Server decodes MJPEG itself. The clips' frames are about 1.6x larger than the StreamCam's real ones.
  • Not run: a real C920, or a slower CPU.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Improvements
    • Webcam format selection now weighs resolution, frame rate, and the processing cost of compressed video. It prefers uncompressed formats when picture size and frame rate are tied, and favors lower-cost usable options over compressed modes above the supported cost limit.
    • High-cost compressed formats remain available when no lower-cost usable mode exists. When frame rates tie, selection also considers which mode better meets the requested frame rate.

…arper

chooseWebcamFormat excluded every compressed mode as soon as a camera
offered one uncompressed mode. A Logitech C920 only reaches 1080p at 30
in MJPEG (its YUY2 1080p runs at 5 fps), so it was driven at YUY2
800x448 on every preset.

Compressed modes now compete on useful frame rate and pixels like any
other; an uncompressed mode only breaks a tie. What stays out is a
compressed mode whose decode costs more than 1080p at 30, counted at the
rate the camera runs. Decoding MJPEG to NV12 through the helper's own
source-reader setup on a Ryzen 7 5800X takes 4.9 ms a frame at 1080p,
9.2 ms at 1440p and 19.7 ms at 4K (p95 22.7), so a BRIO keeps its
uncompressed 1080p30 rather than decode 4K.

Refs #1112
@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 20d57ec1-cb48-479b-9d49-256cc3f9704a

📥 Commits

Reviewing files that changed from the base of the PR and between 2a70bf7 and 181da38.


📒 Files selected for processing (3)
  • electron/native/wgc-capture/src/webcam_format.cpp
  • electron/native/wgc-capture/src/webcam_format.h
  • electron/native/wgc-capture/src/webcam_format_test.cpp

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



📝 Walkthrough

Walkthrough

The webcam format selector now applies a decoded-pixel-per-second limit to compressed modes and updates candidate filtering and tie-breaking. Tests cover format selection for several camera mode lists and decode-cost cases.

Changes

Webcam format selection

Layer / File(s) Summary
Decode-cost filtering and ranking
electron/native/wgc-capture/src/webcam_format.cpp, electron/native/wgc-capture/src/webcam_format.h
Compressed modes are decodable when their pixel count multiplied by frame rate is at most 1920×1080×30. The selector filters candidates based on whether any usable mode meets this limit. It prefers uncompressed modes when candidates tie on pixel count.
Camera format-selection tests
electron/native/wgc-capture/src/webcam_format_test.cpp
Tests cover BRIO, C920, and StreamCam selections, uncompressed tie preference, and cases where costly compressed modes are selected only when no cheaper usable mode is available.

Priority: ⬇️ Low

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

Change: Bug fix


Merge Risk: ⚪ Minimal · up to 181da

The change selects better webcam modes, such as MJPEG 1080p30 for the C920. The capture path already enables the conversion needed for the new selections, so no merge-blocking risk was found. Testing on real C920 hardware and slower CPUs was not performed.

Pre-merge checks | Passed 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check Passed The title clearly describes the main change: allowing a webcam MJPEG mode to win when it provides higher frame rate or resolution. It is concise and related to the implementation.
Description check Passed The description includes the required summary, related issue, change type, release impact, desktop impact, and detailed testing information. It also documents limitations and untested conditions.
Docstring Coverage Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR

🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@EtienneLescot
EtienneLescot merged commit 9c55596 into main Oct 12, 2026
19 checks passed
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.

1 participant