feat: generic video driver - #981
Open
bennyz wants to merge 2 commits into
Open
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds a shared video driver package with HTTP/MJPEG support, snapshot and state APIs, local streaming, documentation, tests, and package registration. The ustreamer driver now uses the shared video interfaces and client functionality. ChangesVideo Driver
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant VideoClient
participant LocalServer
participant JumpstarterTunnel
participant HttpVideo
participant Camera
Operator->>VideoClient: run video stream command
VideoClient->>LocalServer: register snapshot and stream routes
LocalServer->>JumpstarterTunnel: request stream path
JumpstarterTunnel->>HttpVideo: open camera connection
HttpVideo->>Camera: connect over HTTP or HTTPS
Camera-->>HttpVideo: return MJPEG bytes
HttpVideo-->>JumpstarterTunnel: forward stream data
JumpstarterTunnel-->>LocalServer: proxy stream data
LocalServer-->>Operator: serve local video stream
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@python/packages/jumpstarter-driver-video/jumpstarter_driver_video/client_test.py`:
- Around line 128-176: Add a local end-to-end test for the video streaming flow
that starts the actual aiohttp server on an ephemeral loopback port and uses an
HTTP client to request /snapshot and /stream. Reuse the existing client setup
and route behavior from test_stream_command_registers_routes_and_starts_server,
but replace direct handler invocation and mocked run_video_server with real
server startup, then assert the snapshot payload/content type and proxied stream
response over the network.
In `@python/packages/jumpstarter-driver-video/jumpstarter_driver_video/client.py`:
- Around line 117-121: Update the upstream response-reading loop in the client
request handler around buf, tunnel.receive(), and header_part so it enforces a
maximum header size and catches an upstream EndOfStream before constructing the
local response. Raise web.HTTPBadGateway for either an incomplete header or a
header exceeding the limit, while preserving normal parsing for valid responses.
- Around line 121-125: Update the response setup near _parse_content_type so it
parses the upstream HTTP status line from header_part and assigns that status to
web.StreamResponse before response.prepare(request). If the source status line
is missing or invalid, return a 502 response instead of forwarding the stream;
preserve the upstream status and body for valid responses.
In `@python/packages/jumpstarter-driver-video/README.md`:
- Around line 79-81: Update the autoclass directive for HttpVideo to reference
the importable class path without constructor parentheses, using
jumpstarter_driver_video.driver.HttpVideo.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c18fa64-83cf-4eef-96ae-2fb0903886b8
⛔ Files ignored due to path filters (1)
python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
docs/source/reference/package-apis/drivers/index.mddocs/source/reference/package-apis/drivers/video.mdpython/packages/jumpstarter-all/pyproject.tomlpython/packages/jumpstarter-driver-ustreamer/jumpstarter_driver_ustreamer/client.pypython/packages/jumpstarter-driver-ustreamer/jumpstarter_driver_ustreamer/client_test.pypython/packages/jumpstarter-driver-ustreamer/jumpstarter_driver_ustreamer/common.pypython/packages/jumpstarter-driver-ustreamer/jumpstarter_driver_ustreamer/driver.pypython/packages/jumpstarter-driver-ustreamer/pyproject.tomlpython/packages/jumpstarter-driver-video/.gitignorepython/packages/jumpstarter-driver-video/README.mdpython/packages/jumpstarter-driver-video/examples/exporter.yamlpython/packages/jumpstarter-driver-video/jumpstarter_driver_video/__init__.pypython/packages/jumpstarter-driver-video/jumpstarter_driver_video/client.pypython/packages/jumpstarter-driver-video/jumpstarter_driver_video/client_test.pypython/packages/jumpstarter-driver-video/jumpstarter_driver_video/common.pypython/packages/jumpstarter-driver-video/jumpstarter_driver_video/driver.pypython/packages/jumpstarter-driver-video/jumpstarter_driver_video/driver_test.pypython/packages/jumpstarter-driver-video/pyproject.tomlpython/pyproject.toml
Comment on lines
+128
to
+176
| def test_stream_command_registers_routes_and_starts_server(): | ||
| client = _make_client() | ||
| client.call_async = AsyncMock(return_value=base64.b64encode(b"jpeg-data").decode("ascii")) | ||
| client.stream_path = MagicMock(return_value="/video.mjpg") | ||
|
|
||
| captured = {} | ||
| proxied_response = object() | ||
|
|
||
| with ( | ||
| patch( | ||
| "jumpstarter_driver_video.client.run_video_server", | ||
| side_effect=lambda client_arg, app, port, browser: captured.update( | ||
| {"client": client_arg, "app": app, "port": port, "browser": browser} | ||
| ), | ||
| ), | ||
| patch( | ||
| "jumpstarter_driver_video.client.proxy_mjpeg_stream", | ||
| new=AsyncMock(return_value=proxied_response), | ||
| ) as mock_proxy, | ||
| ): | ||
| result = CliRunner().invoke(client.cli(), ["stream", "--port", "1234", "--no-browser"]) | ||
|
|
||
| assert result.exit_code == 0 | ||
| assert captured["client"] is client | ||
| assert captured["port"] == 1234 | ||
| assert captured["browser"] is False | ||
|
|
||
| async def exercise_routes(): | ||
| app = captured["app"] | ||
| index_handler = _get_route_handler(app, "/") | ||
| snapshot_handler = _get_route_handler(app, "/snapshot") | ||
| stream_handler = _get_route_handler(app, "/stream") | ||
|
|
||
| index_response = await index_handler(object()) | ||
| assert index_response.text == LANDING_PAGE | ||
|
|
||
| snapshot_response = await snapshot_handler(object()) | ||
| assert snapshot_response.body == b"jpeg-data" | ||
| assert snapshot_response.content_type == "image/jpeg" | ||
|
|
||
| request = object() | ||
| response = await stream_handler(request) | ||
| assert response is proxied_response | ||
| mock_proxy.assert_awaited_once_with(client, request, "/video.mjpg") | ||
|
|
||
| anyio.run(exercise_routes) | ||
|
|
||
| client.call_async.assert_awaited_once_with("snapshot") | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a local server end-to-end test.
These tests invoke route handlers and mock run_video_server. They do not start the aiohttp server or connect an HTTP client.
Add a test that binds an ephemeral loopback port, requests /snapshot and /stream, and verifies the proxied response through the network stack. As per coding guidelines, “Provide comprehensive package test coverage, prioritizing end-to-end tests that start a server and client; use mocks when system tools, services, or platform compatibility make end-to-end testing impractical.”
Also applies to: 253-299
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@python/packages/jumpstarter-driver-video/jumpstarter_driver_video/client_test.py`
around lines 128 - 176, Add a local end-to-end test for the video streaming flow
that starts the actual aiohttp server on an ephemeral loopback port and uses an
HTTP client to request /snapshot and /stream. Reuse the existing client setup
and route behavior from test_stream_command_registers_routes_and_starts_server,
but replace direct handler invocation and mocked run_video_server with real
server startup, then assert the snapshot payload/content type and proxied stream
response over the network.
Source: Coding guidelines
bennyz
force-pushed
the
feat/video-interface
branch
2 times, most recently
from
August 7, 2026 18:26
029ec87 to
c8e8776
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@python/packages/jumpstarter-driver-video/jumpstarter_driver_video/client_test.py`:
- Around line 1-22: Update the import block in client_test.py to match Ruff’s
generated ordering, including grouping and ordering the aiohttp.web import with
the other third-party imports. Use make lint-fix to apply the formatting rather
than invoking Ruff directly.
- Around line 331-336: Remove the unused response initialization and invalid
status assignment before the StreamResponse patch in the affected test. Keep the
mock_sr setup unchanged, then run make pkg-ty-jumpstarter-driver-video to verify
type checking passes.
In `@python/packages/jumpstarter-driver-video/jumpstarter_driver_video/client.py`:
- Around line 66-76: Update the chunk-decoding loop in the video client’s tunnel
receive path to use bounded incremental buffering: cap chunk-size lines and
declared chunk sizes, reject invalid hexadecimal sizes, and require the
terminating CRLF before yielding data. Ensure each receive iteration enforces
the limits and raises an appropriate error instead of allowing unbounded buf
growth.
- Around line 135-136: Update the EndOfStream handler in the request method to
explicitly chain the HTTPBadGateway exception from the caught EndOfStream using
the appropriate cause syntax, preserving the existing response reason; then run
make lint-fix.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ac6653d-43a1-4c61-ae4f-2b98c78a9e58
📒 Files selected for processing (3)
python/packages/jumpstarter-driver-video/README.mdpython/packages/jumpstarter-driver-video/jumpstarter_driver_video/client.pypython/packages/jumpstarter-driver-video/jumpstarter_driver_video/client_test.py
Comment on lines
+66
to
+76
| while True: | ||
| while b"\r\n" not in buf: | ||
| buf += await tunnel.receive() | ||
| size_line, _, buf = buf.partition(b"\r\n") | ||
| size = int(size_line.split(b";")[0].strip(), 16) | ||
| if size == 0: | ||
| return | ||
| while len(buf) < size + 2: | ||
| buf += await tunnel.receive() | ||
| yield buf[:size] | ||
| buf = buf[size + 2 :] # drop the CRLF terminating the chunk |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound and validate chunked response framing.
A source can omit the chunk-size CRLF or declare a very large chunk. Both loops then grow buf without a limit before any data is forwarded.
Use a bounded incremental decoder. Reject oversized chunk-size lines, oversized chunks, invalid hexadecimal sizes, and missing chunk terminators. This prevents a faulty source from exhausting client memory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/packages/jumpstarter-driver-video/jumpstarter_driver_video/client.py`
around lines 66 - 76, Update the chunk-decoding loop in the video client’s
tunnel receive path to use bounded incremental buffering: cap chunk-size lines
and declared chunk sizes, reject invalid hexadecimal sizes, and require the
terminating CRLF before yielding data. Ensure each receive iteration enforces
the limits and raises an appropriate error instead of allowing unbounded buf
growth.
bennyz
force-pushed
the
feat/video-interface
branch
from
August 7, 2026 18:31
c8e8776 to
51f65ab
Compare
Video support was tied to uStreamer, which wraps a local device on the exporter host. A camera attached to the DUT itself, an ESP32 for example serving MJPEG over its WiFi interface has no device node on the exporter and so could not be exported at all. Add jumpstarter-driver-video, following the PowerInterface/FlasherInterface convention. VideoInterface defines the contract: snapshot(), state(), stream_path(), and a connect() bytestream carrying HTTP/MJPEG. VideoClient provides snapshots as PIL images, source state, the local MJPEG proxy server, and the `j video` CLI. HttpVideo implements the interface for sources reachable over HTTP: the exporter dials the camera, so clients reach DUTs on networks they cannot route to themselves. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
UStreamerClient was already source-agnostic apart from state(): its MJPEG proxy and CLI only spoke HTTP over the connect tunnel. Implement VideoInterface on the UStreamer driver and inherit VideoClient, so that code lives once in jumpstarter-driver-video rather than being duplicated by every video source. UStreamerState now extends VideoState, filling online/width/height/fps from ustreamer's own status document. Generic consumers can therefore read the common fields from a uStreamer source, while its richer detail stays available through the unchanged result field, and `j video state` keeps printing what it printed before. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
feat/video-interface
branch
from
August 7, 2026 18:38
51f65ab to
9f1b3fd
Compare
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
1 participant
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.
connecting a camera to a device like ESP32, would not show up as a /dev/ on the exporter, so we need to stream over TCP. Rather than copying a lot of code from ustreamer, create a generic video driver to make code sharing easier