Skip to content

Conversation

@tfrere
Copy link
Contributor

@tfrere tfrere commented Nov 14, 2025

  • Add camera router with /stream endpoint
  • Support both simulation (UDP) and real robot (hardware camera)
  • Share single camera instance between all clients
  • Optimize with 640x480 resolution and 80% JPEG quality
  • Ensure Mujoco rendering loop starts in headless mode for UDP stream

- Add camera router with /stream endpoint
- Support both simulation (UDP) and real robot (hardware camera)
- Share single camera instance between all clients
- Optimize with 640x480 resolution and 80% JPEG quality
- Ensure Mujoco rendering loop starts in headless mode for UDP stream
Copilot finished reviewing on behalf of tfrere November 14, 2025 21:14
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new /api/camera/stream endpoint for MJPEG video streaming, supporting both simulation (via UDP) and real robot (hardware camera) modes. The implementation uses a shared camera instance pattern to serve multiple clients efficiently.

Key changes:

  • Modified Mujoco backend to always start the rendering loop regardless of headless mode, ensuring UDP stream availability for simulation
  • Added new camera router with streaming endpoint using OpenCV camera backend
  • Registered the camera router in the FastAPI application

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.

File Description
src/reachy_mini/daemon/backend/mujoco/backend.py Unindented rendering thread initialization to run in both headless and non-headless modes, enabling UDP camera stream in simulation
src/reachy_mini/daemon/app/routers/camera.py New router implementing MJPEG streaming with shared camera instance, automatic mode detection (sim vs hardware), and frame optimization
src/reachy_mini/daemon/app/main.py Added camera router to the API router to expose the /api/camera/stream endpoint

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 41 to 48
"""Stream camera feed as MJPEG."""
async def _stream():
is_sim = daemon.status().simulation_enabled and isinstance(backend, MujocoBackend)
cam = await _get_shared_camera(is_sim)
try:
while True:
f = cam.read()
if f is not None:
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Missing error handling for camera read failures. If cam.read() returns None (line 42-43), the frame is simply skipped. While this prevents crashes, it means clients could receive no frames for extended periods without any indication of an error. Consider:

  1. Adding a counter for consecutive None reads and raising an error after a threshold
  2. Logging warnings when frames are dropped
  3. Breaking the loop and returning an error response if the camera becomes permanently unavailable

This would provide better diagnostics when camera issues occur.

Copilot uses AI. Check for mistakes.
_shared_camera.close()
_shared_camera = None
_camera_refs = 0

Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Missing space after equals sign in type annotation. Python style guide (PEP 8) requires spaces around the equals sign in type-annotated default values.

Should be: daemon: Daemon = Depends(get_daemon)

Copilot uses AI. Check for mistakes.
Comment on lines +13 to +14
from ..dependencies import get_backend, get_daemon

Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Global state management without thread safety. The _shared_camera and _camera_refs variables are accessed and modified by multiple concurrent requests without synchronization (e.g., locks). This can lead to race conditions where:

  1. Multiple requests could simultaneously check _shared_camera is None and create multiple camera instances
  2. The reference count could be corrupted due to concurrent increments/decrements
  3. The camera could be closed while another request is still using it

Consider using asyncio.Lock or threading.Lock to protect these shared variables.

Copilot uses AI. Check for mistakes.
Comment on lines 24 to 30
_shared_camera.open(udp_camera="udp://@127.0.0.1:5005" if is_sim else None)
_camera_refs += 1
return _shared_camera

def _release_camera():
global _shared_camera, _camera_refs
_camera_refs -= 1
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Resource leak with reference counting. The reference counting mechanism is flawed because _camera_refs is incremented in _get_shared_camera() but only decremented in the finally block of _stream(). If a client disconnects unexpectedly or the stream generator is never consumed, the finally block may not execute, leading to a leaked reference that prevents the camera from being closed.

Additionally, the current design means the camera is only released when ALL clients disconnect. If you want to share the camera among concurrent clients, the cleanup should happen when the individual stream ends, not when all streams end. Consider using a context manager or a more robust reference counting mechanism with proper cleanup guarantees.

Copilot uses AI. Check for mistakes.
Comment on lines 19 to 44

async def _get_shared_camera(is_sim: bool):
global _shared_camera, _camera_refs
if _shared_camera is None:
_shared_camera = OpenCVCamera(log_level="WARNING", resolution=CameraResolution.R1280x720)
_shared_camera.open(udp_camera="udp://@127.0.0.1:5005" if is_sim else None)
_camera_refs += 1
return _shared_camera

def _release_camera():
global _shared_camera, _camera_refs
_camera_refs -= 1
if _camera_refs <= 0 and _shared_camera is not None:
_shared_camera.close()
_shared_camera = None
_camera_refs = 0

@router.get("/stream")
async def stream_camera(
backend=Depends(get_backend),
daemon: Daemon=Depends(get_daemon),
) -> StreamingResponse:
"""Stream camera feed as MJPEG."""
async def _stream():
is_sim = daemon.status().simulation_enabled and isinstance(backend, MujocoBackend)
cam = await _get_shared_camera(is_sim)
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Resolution mismatch between camera initialization and usage. The camera is initialized with CameraResolution.R1280x720 (1280x720) on line 19, but the frames are immediately resized to 640x480 on line 44. This wastes bandwidth and processing:

  1. For hardware cameras, you're requesting 1280x720 from the camera and then downscaling
  2. For UDP cameras, you're receiving 1280x720 frames over UDP and then downscaling

Consider initializing the camera with a resolution closer to your target output size, or use CameraResolution.R640x480 if it exists. The PR description mentions "640x480 resolution" as an optimization, but this isn't achieved at the camera level.

Copilot uses AI. Check for mistakes.
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.

2 participants