Apply video source fourcc before fps by ColePBryan · Pull Request #3105 · roboflow/inference

ColePBryan · 2026-10-01T19:00:04Z

Description

V4L2 cameras such as the Logitech C920 open in YUYV, where 1080p is capped at 5 FPS. video_source_properties arrive as a JSON map with no guaranteed key order, so fps could be set (and clamped) before fourcc switched the device to MJPG, leaving the stream at 5 FPS even with fourcc set.

apply_capture_properties now applies fourcc first, other properties in their given order, then fps. It accepts fourcc as a four-character code (e.g. "MJPG", used exactly as given since codes like avc1 and Y16 are case- and space-sensitive) or its numeric value, skips invalid values with a warning, logs a property the capture rejects at debug level, and logs the effective format read back from the capture. The stream manager's VideoConfiguration and the stream_vision source-property type hints accept a string fourcc.

Type of change

Bug fix (non-breaking change which fixes an issue)

How has this change been tested, please provide a testcase or example of how you tested the change?

Unit tests for property ordering, string and numeric fourcc, verbatim four-character codes, rejected properties and invalid values; the stream_vision camera suite passes (223).
On a Jetson Orin Nano with a C920 at 1920x1080, the same ordering raised the camera from 5 FPS (YUYV) to 30 FPS (MJPG).

Any specific deployment considerations

None.

Docs

CHANGELOG entry added.

CLAassistant · 2026-10-01T19:00:13Z

All committers have signed the CLA.

github-actions · 2026-10-01T19:00:18Z

👋 Thanks for the pull request! Here is how automated Claude review works here, so you spend credits (and reviewer time) wisely.

🚦 This PR is marked Ready for review, so automated Claude review will run — and every pass spends real credits.

Warning

💸 The Claude reviewer bills in credits, not vibes

Automated review spins up a real agent that reads real code and spends real credits on every pass. It is glad to help — but it is not a rubber duck, a linter you poke in a loop, or a substitute for reading the contributing guide. Treat it like an expensive senior reviewer whose time you booked, and show up prepared.

Draft when unsure, Ready when you mean it:

🌱 Not sure the PR is in good shape yet? Keep it (or set it back) as a draft — drafts pause review, so you can push and iterate without burning credits on a moving target.
💪 Feel strong about the contents? Mark it Ready for review and the reviewer will take a look.

However you get there, arrive prepared:

🧱 Bring a SOLID, thorough PR. Point your local agent at our skills/ to tune it to our guidelines first — or, if you are one of those fabled carbon-based contributors, read them yourself. A half-baked diff costs exactly the same to review as a finished one.
✅ Resolve every comment before you re-request review. Re-requesting with threads still open means paying twice for the same conversation.
🔁 Do not use CI review as an inner loop for a local agent. The reviewer is not a step-by-step debugger — do the unfolding locally and arrive with the answer, not the search.
🙋 If something looks off, ask a human. One question to a maintainer is cheaper and faster than three rounds of agent re-review chasing a misread.

Reviews are not free. A draft costs nothing to review; a Ready PR is a promise that it is worth reviewing.

Prefer to skip automated review entirely? Add the skip-claude-review label.

alexnorell

The camera property ordering and both producer integrations pass all 28 focused tests at this head. Please remove the duplicate pass-through ordering case below; no real UVC camera was available for FPS negotiation validation.

alexnorell · 2026-10-01T19:43:22Z

+    assert "Ignoring invalid fourcc" in streamvision_caplog.text
+
+
+def test_properties_without_fourcc_and_fps_are_applied_in_given_order() -> None:


[P2] Remove this pass-through ordering case. test_other_properties_keep_their_given_order_between_fourcc_and_fps already checks height, brightness and width retain their input order while also exercising the camera-sensitive FOURCC/FPS reordering. This additional recording-fake assertion covers no separate camera negotiation outcome or failure mode.

Removed in 7791a79.

shntu

Reviewed with Claude and Codex. Codex found no regressions. Applying fourcc, then the other properties, then fps is the right V4L2 order, and both OpenCV-backed producers use the new helper.

Two small follow-ups are inline: one docstring and one input-validation gap. Neither is a blocker. A non-blocking note: the public InferencePipeline.init* and prepare_video_sources signatures, WebRTCVideoFrameProducer.initialize_source_properties, and inference_sdk's start_inference_pipeline_with_workflow are still typed Dict[str, float], so passing a string fourcc through the Python API is a type error even though it works at runtime.

shntu · 2026-10-01T19:44:24Z

+    """Set capture properties on ``stream`` with ``fourcc`` first and ``fps`` last.
+
+    Properties other than ``fourcc`` and ``fps`` keep their given order.
+    ``fourcc`` may be a four-character code (``"MJPG"``, ``"mjpg"``) or its


[Claude] Since 85cc12e dropped .upper(), "mjpg" is encoded as the FOURCC mjpg, not MJPG. V4L2 pixel formats are case-sensitive, so the driver rejects it and stays on YUYV. That rejection is logged only at DEBUG, so it is invisible at the default level. Suggest removing "mjpg" from the docstring, or saying explicitly that codes are case-sensitive.

[Claude] Rechecked at 468ce8b: the force-push rebased the same two commits onto main without changing them, so this is still open.

Fixed in 7791a79. The docstring says codes are case-sensitive, and a fourcc the camera rejects is now logged at WARNING.

shntu · 2026-10-01T19:44:24Z

+                        f"Video source property {property_id!r} must be a "
+                        f"number, got {property_value!r}."
+                    )
+                validated[property_id] = property_value


[Claude] This accepts any string as fourcc, so "MJPEG" or "MJ" return 200 from the API and are then dropped on the device with a warning. Validating with _parse_fourcc here (and reusing FOURCC_PROPERTY) would give callers a 422 and keep a single definition of a valid code. It would also catch strings like "²²²²": str.isdigit() is True for them, but int() raises, so at the moment they crash source startup instead of being skipped. Alternatively, guard that with code.isascii() and code.isdigit() in _parse_fourcc.

[Claude] Rechecked at 468ce8b: the force-push rebased the same two commits onto main without changing them, so this is still open.

Fixed in 7791a79. VideoConfiguration validates fourcc with the shared parser, so invalid values get a 422, and non-ASCII digits like "²²²²" are rejected instead of crashing. In 02c4b1f the parser moved to an OpenCV-free streamvision.camera.fourcc, so the entities stay light.

V4L2 cameras such as the Logitech C920 open in YUYV, where 1080p is capped at 5 fps. Properties arrive from a JSON map with no guaranteed key order, so fps could be set (and clamped) before fourcc switched the device to MJPG, leaving the stream at 5 fps. Apply fourcc first, other properties in their given order, then fps. Accept fourcc as a four-character code (e.g. "MJPG") as well as its numeric value, skip invalid values with a warning, and log the effective format read back from the capture. The stream manager VideoConfiguration now accepts a string fourcc while still coercing all other properties to float.

strip() and upper() broke valid codes: "Y16 " ends in a space and avc1 or pRAA are case-sensitive. A four-character value is now used exactly as given; other strings are trimmed. A property the capture doesn't accept is logged at debug level, and the source-property type hints in stream_vision accept string values such as a fourcc code.

Expose parse_fourcc and reuse it in VideoConfiguration so an invalid fourcc is a validation error instead of being silently dropped on the device. Only ASCII digits are parsed as a numeric code, so values like "²²²²" are rejected rather than crashing source startup. A fourcc the device rejects is now logged at WARNING; the docstring states codes are case-sensitive. Drop a duplicate ordering test.

InferencePipeline init methods, prepare_video_sources, the WebRTC producer and the SDK's start_inference_pipeline_with_workflow accept a string fourcc at runtime, so widen their annotations to match.

Validating fourcc in the stream manager's entities imported capture_properties, which imports cv2, and broke the isolation check that entities import without OpenCV. Move parse_fourcc and FOURCC_PROPERTY to streamvision.camera.fourcc, packing the code as cv2.VideoWriter_fourcc does (tested against it); capture_properties re-exports both.

alexnorell

The Unicode validation and lightweight-import fixes are verified: all 54 camera tests pass, and a fresh client/entities import loads none of the forbidden heavy modules. The previous pass-through duplicate is removed. Please consolidate the two remaining direct-helper duplicates below while retaining the entity/API boundary tests. Current package CI is still being rechecked after the import fix.

alexnorell · 2026-10-01T20:29:21Z

+        None,
+    ],
+)
+def test_parse_fourcc_returns_none_for_invalid_values(fourcc: Any) -> None:


[P2] Move the unique Arabic-digit case into test_invalid_fourcc_is_skipped_with_a_warning, then remove this direct-parser table. Its other values are already exercised through apply_capture_properties, which checks the same parser result plus the warning and continued property application. Keep the entity rejection tests: they verify a separate API validation outcome.

Done in 59bd775.

alexnorell · 2026-10-01T20:29:21Z

+
+
+@pytest.mark.parametrize("code", ["MJPG", "YUYV", "avc1", "Y16 ", "pRAA"])
+def test_fourcc_matches_opencv_packing(code: str) -> None:


[P2] Remove this duplicate packing test. The existing capture tests already compare MJPG, avc1, Y16-with-space, and pRAA against the same cv2.VideoWriter_fourcc oracle through the production apply path. YUYV is another ordinary four-byte ASCII sample, adding no distinct packing branch or failure mode. The existing fresh-import isolation probe covers the reason for moving the parser.

Removed in 59bd775.

The public-contract tests freeze InferencePipeline.init* signatures and docstrings, so restore them; the widened hints stay on the helpers outside that contract. The request-entities import check lists the modules entities may load, which now include streamvision.camera.fourcc.

alexnorell

The latest commit restores the intentional legacy signature contract and accounts for the new lightweight FOURCC module. The camera implementation is unchanged from the 54 passing focused tests. The import-format CI failure remains in unchanged code, alongside the two existing test-cleanup findings.

alexnorell · 2026-10-01T20:54:39Z

+import cv2
+
+from streamvision.camera.fourcc import FOURCC_PROPERTY, parse_fourcc
+


[P2] Format this import group with the repository isort configuration. The code-quality job failed on this file alone (Imports are incorrectly sorted and/or formatted), and this file is unchanged by the latest commit, so the deterministic failure remains. Run the configured isort on the file and rerun the gate: https://github.com/roboflow/inference/actions/runs/36921392196/job/110568057950.

Fixed in 59bd775.

Apply the repository isort configuration to capture_properties. Move the non-ASCII-digit case into the skipped-with-warning table and drop the direct parser table and the packing test, which the capture-path tests already cover.

github-actions · 2026-10-01T22:00:25Z

🤖 Claude review started at commit 59bd7753ba57bbff7f94df4a67ea8668a27b1519.

New commits are not auto-reviewed. Add the claude-review label to request a re-review — the label is consumed when the review starts, so just add it again next time.

github-actions · 2026-10-01T22:06:31Z

Review summary

Skills: review-sdk, review-topic-backward-compat-and-versioning, review-topic-test-hygiene

No dedicated surface skill covers stream_vision/ — generic review plus topic skills only.

What I verified against the code (zero-trust, static trace — I cannot run tests):

Ordering fix (capture_properties.apply_capture_properties): fourcc is applied first, fps last, other properties keep their given order regardless of input dict order — the actual regression this PR targets. Both OpenCV-backed producers (CV2VideoFrameProducer, GStreamerRtspVideoFrameProducer) route through the helper.
FOURCC parsing (camera/fourcc.parse_fourcc): case-sensitive 4-char codes used verbatim (avc1, Y16 ), numeric/numeric-string forms accepted, and the code.isascii() and code.isdigit() guard correctly rejects non-ASCII digits (²²²², Arabic-Indic digits) that str.isdigit() would otherwise pass into int() — no startup crash. _encode_fourcc matches cv2.VideoWriter_fourcc packing.
API validation (VideoConfiguration.validate_video_source_properties): invalid fourcc now fails with a ValidationError (422) instead of being silently dropped on the device; other properties still coerce to float. Backward compatible — previously-valid numeric payloads behave identically.
Type widening (Dict[str, float] → Dict[str, Union[float, str]]) across the camera producers, prepare_video_sources, and the SDK's start_inference_pipeline_with_workflow is additive/backward compatible; Union is in scope in every touched file (no NameError). No async twin exists for the SDK method, so no sibling to widen.
Lightweight-import contract preserved: fourcc.py imports only math/typing (no cv2), camera/__init__.py is empty, and the updated test_request_entities_do_not_import_the_decoder_webrtc_or_pipeline keeps cv2 out of the stream-manager entities import path.
Tests + CHANGELOG: new behavior is covered by stream_vision/tests/unit_tests/camera/test_capture_properties.py, which runs in unit_tests_streamvision_x86.yml; the ## Unreleased → ### Fixed entry is present in stream_vision/CHANGELOG.md.

Prior automated-review threads (docstring case-sensitivity; validation gap / non-ASCII-digit crash) are resolved in the current code; the remaining prior notes were test-dedup NITs already trimmed.

Maintainer note (non-blocking, does not gate): the streamvision package has a functional change and will need a release-time version bump in stream_vision/pyproject.toml + the requirements/requirements.streamvision.txt pin at publish time — contributor action is not required.

Reviewed at HEAD: 59bd775

github-actions · 2026-10-01T22:06:35Z

😎 PR passes the vibe-check and trust-me-bro verification.

github-actions · 2026-10-01T22:07:33Z

Maintainer review discussion: Slack thread.

Final approval and merge remain in GitHub.

alexnorell

The FOURCC-before-FPS ordering, invalid-format handling and lightweight parser import are verified, and the redundant tests are removed. All 41 focused camera tests, pinned isort, independent tensor integration runs for Python 3.10/3.12, and current required PR checks pass. Older optional OCR-proxy failures are external HTTP 500s; physical USB-camera negotiation was not exercised.

dkosowski87 · 2026-10-01T23:07:04Z

[P2] Preserve previously accepted numeric FOURCC representations during validation

At 59bd7753, VideoConfiguration.validate_video_source_properties calls parse_fourcc(property_value) before numeric coercion. This rejects values that the previous Dict[str, float] field accepted, so existing configurations can fail validation after upgrading.

I reproduced this against the actual base (23a01eb3) and PR models with Pydantic 2.11.10:

VideoConfiguration(
    type="VideoConfiguration",
    video_reference=0,
    video_source_properties={"fourcc": "1196444237.0"},
)

Before: accepted and converted to 1196444237.0 (MJPG). After: ValidationError. The same regression occurs for "1.196444237e9", "+1196444237", and, for Python callers, numpy.int64(1196444237). Plain integer/float values and digit-only numeric strings still work.

Please preserve numeric coercion for valid integral FOURCC values while retaining support for case-sensitive four-character codes and rejection of genuinely invalid values. Regression coverage should include the previously accepted numeric representations above.

This is distinct from the earlier discussion about rejecting malformed codes such as "MJPEG" and non-ASCII digits. The four compatibility probes pass the base-model assertion and fail on the PR model; the 39 isolated helper/validation cases from the PR's test file pass.

Validation ran parse_fourcc before numeric coercion, and parse_fourcc only read plain digit strings, so values the previous Dict[str, float] field accepted now failed: "1196444237.0", "1.196444237e9", "+1196444237", and numpy integers from Python callers. Read strings with float() (ASCII only, so non-ASCII digits stay rejected) and any numbers.Real value, keeping finite, non-negative integral results; non-numeric four-character strings are still case-sensitive codes.

ColePBryan · 2026-10-01T23:11:21Z

@dkosowski87 Thanks, good catch. Fixed in 4032c22: parse_fourcc now reads strings with float() (ASCII only, so non-ASCII digits stay rejected) and accepts any numbers.Real value, keeping finite, non-negative integral results. So "1196444237.0", "1.196444237e9", "+1196444237" and numpy.int64(1196444237) validate again, and those four are now regression cases in test_video_configuration_accepts_valid_fourcc. Non-numeric four-character strings are still case-sensitive codes; MJPEG, ²²²², nan, -1 and 1.5 are still rejected.

alexnorell

One reproduced input-validation regression remains: oversized camera-property integers escape as server errors instead of validation errors. See the inline reproduction.

alexnorell · 2026-10-01T23:35:03Z

+                )
+            try:
+                validated[property_id] = float(property_value)
+            except (TypeError, ValueError):


[P2] Keep oversized numeric inputs as validation errors. A valid JSON payload containing video_source_properties={"width": 10**400} raises an uncaught OverflowError here; the same value for fourcc raises earlier in parse_fourcc at its float(value) conversion. I reproduced the actual base and current DTOs through FastAPI: both properties return HTTP 422 at base 23a01eb3, but HTTP 500 at this head (the 45 focused tests pass). Handle OverflowError in both numeric conversion paths and add these inputs to the existing invalid-value cases so malformed requests retain a structured validation response.

Fixed in a069ad9. OverflowError is now handled in both numeric paths, so 10**400 / -10**400 for width or fourcc return a 422 again. Non-fourcc properties are now validated with pydantic's float (TypeAdapter(float)), so they match the field's original float semantics exactly (this also rejects "١٢" and bytearray(b"12") again). A numeric fourcc goes through the same float validation before the FOURCC check, and parse_fourcc guards its float conversions and returns None, so the device-side apply path logs "Ignoring invalid fourcc" instead of raising. Integral b"12" / Decimal fourcc values are accepted.

New test cases: 10**400 and -(10**400) in the invalid VideoConfiguration cases for both fps and fourcc; "١٢" and bytearray(b"12") in the rejected non-fourcc cases; bytes/Decimal/Fraction forms in the accepted fourcc cases; 10**400 in the invalid fourcc skipped-with-warning table.

An integer too large for a float, such as 10**400, raised OverflowError from the video_source_properties validator (and from parse_fourcc for fourcc), so the request failed with a 500 instead of a 422. Validate non-fourcc properties with pydantic's float adapter so they keep the field's float semantics, including rejecting oversized integers, non-ASCII digits and bytearrays. Validate a numeric fourcc the same way before checking that it is a valid code, and guard parse_fourcc's float conversions so it returns None for any value it cannot convert. This also accepts integral bytes and Decimal fourcc values.

alexnorell reviewed Oct 1, 2026

View reviewed changes

shntu reviewed Oct 1, 2026

View reviewed changes

ColePBryan added 2 commits October 1, 2026 12:51

ColePBryan force-pushed the bryan/video-source-fourcc-first branch from 85cc12e to 468ce8b Compare October 1, 2026 19:51

ColePBryan added 3 commits October 1, 2026 13:13

Type video source properties as float or str in public signatures

b7f09d2

InferencePipeline init methods, prepare_video_sources, the WebRTC producer and the SDK's start_inference_pipeline_with_workflow accept a string fourcc at runtime, so widen their annotations to match.

alexnorell reviewed Oct 1, 2026

View reviewed changes

ColePBryan requested review from alexnorell and shntu October 1, 2026 21:13

ColePBryan marked this pull request as ready for review October 1, 2026 21:59

ColePBryan requested review from PawelPeczek-Roboflow, dkosowski87, grzegorz-roboflow, hansent, probicheaux, rafel-roboflow and yeldarby as code owners October 1, 2026 21:59

alexnorell previously approved these changes Oct 1, 2026

View reviewed changes

ColePBryan dismissed alexnorell’s stale review via 4032c22 October 1, 2026 23:11

ColePBryan added 2 commits October 1, 2026 16:12

Describe the numeric fourcc test cases by what they cover

b07beeb

Simplify the FOURCC integral check and say what it enforces

ac41ee8

alexnorell requested changes Oct 1, 2026

View reviewed changes

		assert "Ignoring invalid fourcc" in streamvision_caplog.text


		def test_properties_without_fourcc_and_fps_are_applied_in_given_order() -> None:



		@pytest.mark.parametrize("code", ["MJPG", "YUYV", "avc1", "Y16 ", "pRAA"])
		def test_fourcc_matches_opencv_packing(code: str) -> None:

		import cv2

		from streamvision.camera.fourcc import FOURCC_PROPERTY, parse_fourcc

Conversation

ColePBryan commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How has this change been tested, please provide a testcase or example of how you tested the change?

Any specific deployment considerations

Docs

Uh oh!

CLAassistant commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💸 The Claude reviewer bills in credits, not vibes

Uh oh!

alexnorell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shntu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexnorell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexnorell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 1, 2026

Uh oh!

github-actions Bot commented Oct 1, 2026

Uh oh!

github-actions Bot commented Oct 1, 2026

Uh oh!

github-actions Bot commented Oct 1, 2026

Uh oh!

alexnorell left a comment

Choose a reason for hiding this comment

Uh oh!

dkosowski87 commented Oct 1, 2026

Uh oh!

ColePBryan commented Oct 1, 2026

Uh oh!

alexnorell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

ColePBryan commented Oct 1, 2026 •

edited

Loading

CLAassistant commented Oct 1, 2026 •

edited

Loading

github-actions Bot commented Oct 1, 2026 •

edited

Loading