Video: numerous small enhancements and fixes - #417
Open
rmackay9 wants to merge 15 commits into
Open
Conversation
Photo mode's device ID comes from get_camera_caps.py's synthetic "CSI-<name>" still-device ID, while Video/Streaming mode's comes from gstcaps.py's real filesystem device path. The model-name extraction in sendCameraInformation() only knew how to clean up the path-style format (splitting on '/' and '@'), so Photo mode reported "CSI-imx415" while Video/Streaming mode reported the clean "imx415" for the same physical camera. Confirmed via live testing on an imx415.
…are_version
Was hardcoded to 0, which ArduPilot displays as "fw:0.0.0.0" - not
useful for identifying which Rpanion version a GCS is talking to.
Now encodes package.json's version (e.g. "0.12.0") using the packing
CAMERA_INFORMATION.firmware_version actually specifies: (Dev << 24) |
(Patch << 16) | (Minor << 8) | Major, i.e. Major is the *least*
significant byte. This is the opposite byte order to
AUTOPILOT_VERSION.flight_sw_version (major is most significant there,
already decoded elsewhere in mavManager.js) - confirmed against the
MAVLink spec doc comments for both fields rather than assumed, since
copying the other field's byte order would have been silently wrong.
Reads the version via require('../package.json') rather than
process.env.npm_package_version (only set when launched through npm,
not when run directly under systemd as in production - see
aboutInfo.js's existing fallback to shelling out to `dpkg -l` for the
same reason) or shelling out to dpkg - this works the same way in dev
and in the packaged/installed app without spawning a subprocess.
The "Video source IP Address" dropdown defaulted to loopback almost every time, for two compounding reasons: - scanInterfaces() never filters out the loopback interface, and it typically sorts first in os.networkInterfaces()'s enumeration. - video.jsx's default computation referenced this.state.ifaces[0] in the same setState() call that first populates ifaces - reading it there reads the stale pre-update state (still the constructor's empty array), not the freshly-fetched interface list, so the fallback to '127.0.0.1' fired on essentially every first load regardless of interface order. A remote GCS can never reach 127.0.0.1 on the companion computer, so this made VIDEO_STREAM_INFORMATION's advertised RTSP URI useless by default. Confirmed via live testing (GCS showed 127.0.0.1:8554 instead of the Pi's real LAN address). Fixed in two places: - video.jsx now computes the default from the freshly-fetched interface list directly, preferring the first non-loopback address. - videostream.js's sendVideoStreamInformation() now also refuses to advertise a loopback address even if one somehow ends up selected (e.g. a device with no other network interface configured yet), falling back to the first non-loopback address in deviceAddresses if one exists. This doesn't address multi-homed setups where different GCS's need different source addresses (VIDEO_STREAM_INFORMATION only has one uri per stream_id) - just stops the message from defaulting to something guaranteed unreachable.
Previously only one address was ever advertised - whichever matched the configured "MAVLink video source IP" (or the first non-loopback fallback added in the previous commit). A GCS connected on a different interface than whichever one Rpanion happened to pick had no way to discover a URI it could actually reach. RTSP now sends one VIDEO_STREAM_INFORMATION message per address in deviceAddresses, using distinct stream_id values (1..count) for the same underlying stream, ordered: the explicitly-selected address first (if non-loopback), then other non-loopback addresses, then loopback last as a final fallback. RTP is unaffected - it pushes to a single fixed destination (useUDPIP), so there's only ever one meaningful stream to advertise there. Known tradeoff: index.js's 'videostreaminfo' handler sends a COMMAND_ACK on every emission, so a GCS now receives `count` identical ACKs alongside the `count` stream messages for a single request. Redundant but harmless (same MAV_RESULT_ACCEPTED each time) - not worth restructuring the ack path for. Added a test covering the multi-address case; the existing single-address test is unaffected (a device with exactly one address still sends exactly one message, same as before).
Now that VIDEO_STREAM_INFORMATION is sent once per reachable address, every message used the same `name` (the raw videoSettings.device path) via the shared common-fields helper - so a GCS's stream picker would show several identical-looking entries with no way to tell which one to pick. VIDEO_STREAM_INFORMATION.name's spec purpose is literally "Stream name", so it's the right field to disambiguate with. Each stream's name is now "<model> (<ip>)", e.g. "imx415 (10.0.2.100)" and "imx415 (127.0.0.1)" for the same camera on two addresses. RTP mode (single destination, no disambiguation needed) just gets the plain model name. Extracted the model-name cleanup logic (already used for CAMERA_INFORMATION.model_name) into a shared extractModelName() helper rather than duplicating it, since the raw device path alone (e.g. /base/soc/i2c0mux/i2c@1/imx415@1a, ~34 chars) is already close to the name field's 31-usable-character limit before appending anything - needed the short form to leave room for the IP suffix.
Two related bugs, both stemming from MAVLink camera commands never checking whether the camera was actually in the right mode to do what was asked: 1. captureStillPhoto() sent SIGUSR1 to whatever process happened to be running (this.deviceStream), regardless of cameraMode. photovideo.py's SIGUSR1 handler does something different depending on which mode it was launched with - in 'photo' mode it takes a picture, but in 'video' mode it TOGGLES RECORDING. So an IMAGE_START_CAPTURE command received while Rpanion was in video mode wouldn't just fail to take a photo - it would actually start or stop a video recording instead. In streaming mode it likely did nothing (GStreamer has no SIGUSR1 handler), also silently. 2. VIDEO_START_CAPTURE/VIDEO_STOP_CAPTURE already checked cameraMode before calling toggleVideoRecording(), but sent camera_command_ack unconditionally right after regardless of whether that check passed - so a rejected command still told the GCS MAV_RESULT_ACCEPTED. Both now explicitly check cameraMode first and NACK with MAV_RESULT_TEMPORARILY_REJECTED (1) if it's wrong - "valid command, but not right now, retrying later should work" is the correct semantics per the MAVLink spec, since switching mode (manually for now, or automatically once camera mode auto-switching exists) and retrying would succeed. For VIDEO_START_CAPTURE/STOP_CAPTURE specifically, also distinguish "wrong mode" (reject) from "right mode but already in the requested state" (e.g. START received while already recording) - the latter still ACKs accepted, since the state the GCS wants already holds, no toggle needed. Added tests for the wrong-mode rejection paths (both captureStillPhoto directly and via onMavPacket's VIDEO_START_CAPTURE dispatch, which previously had no test coverage at all), and updated the existing #captureStillPhoto() test to set cameraMode = 'photo' explicitly, since it defaulted to 'streaming' and the new guard correctly rejects that.
Previously, MAVLink commands that needed a specific camera mode (capture a photo, record video, stream) just got rejected with MAV_RESULT_TEMPORARILY_REJECTED if Rpanion happened to be in a different mode - there was no way for an autopilot/GCS to actually get the camera into the right mode itself. Added switchCameraModeAndThen(): stops whatever's currently running, switches cameraMode, starts the target mode's process, and then runs the requested action. Starting a mode can take a while (spawning photovideo.py/video-server.py, camera library init), so it ACKs MAV_RESULT_IN_PROGRESS immediately if a switch is actually needed (avoiding a GCS command-ack timeout), then a final ACCEPTED/FAILED once the switch (and the action) actually completes. A simple modeSwitchInProgress guard rejects a second switch request that arrives while one is already underway, rather than letting them race against each other. Wired this into IMAGE_START_CAPTURE, DO_DIGICAM_CONTROL (both -> 'photo'), and VIDEO_START_CAPTURE (-> 'video'). VIDEO_STOP_CAPTURE deliberately does NOT auto-switch - if the camera isn't in video mode there's nothing recording to stop, so the requested state (not recording) already holds; it just ACKs accepted as a no-op rather than switching mode pointlessly. Also implemented two previously entirely-unhandled commands (fell through to sendUnsupportedAck before this): - SET_CAMERA_MODE (530): the MAVLink camera protocol's actual "switch mode" command. param2 is a CameraMode enum (IMAGE/VIDEO/ IMAGE_SURVEY) - maps IMAGE->'photo' and VIDEO->'video'; Rpanion has no equivalent of IMAGE_SURVEY. Streaming isn't reachable through this command, since the spec's CameraMode enum has no streaming value - the protocol treats streaming as orthogonal to capture mode, not a peer of it (see VIDEO_START_STREAMING below). - VIDEO_START_STREAMING/VIDEO_STOP_STREAMING (2502/2503): distinct from VIDEO_START/STOP_CAPTURE, which are about local recording. Mapped to switching to/from 'streaming' mode, matching Rpanion's current one-exclusive-mode-at-a-time architecture. STOP works like VIDEO_STOP_CAPTURE - no-op accept if not already streaming. Updated/added tests to match: the existing wrong-mode-rejection test now verifies the auto-switch sequence instead (stop -> start in new mode -> action, with IN_PROGRESS then final ack), and added a test for the concurrent-switch-rejected guard. captureStillPhoto()'s own mode check (from the previous commit) is unchanged and still matters as a safety net for the web UI's direct call path, which doesn't go through switchCameraModeAndThen.
Resolution options were listed in whichever order the camera's native sensor modes happened to be enumerated by picam2.sensor_modes, which has no relation to resolution size. Confirmed via live testing: the Resolution dropdown showed 1280x720, 640x480, 1920x1080, 1680x1050 - neither ascending nor descending. Sort every device's caps list once, right before the final JSON output, rather than at each of the several retDevices.append() call sites (Picamera2/CSI, generic v4l2 device provider, legacy Pi camera stack, Jetson, testsrc).
When entering video mode on page load, the saved FPS (videoData.selectedFps) was reused unconditionally, even though the device/resolution match just above it can fall back to a different device or capability than what was originally saved (if the saved one is no longer available). The fallback capability's FPS range/options can differ from what the saved FPS was actually valid for. Now only reuses the saved FPS if it's actually valid for the matched capability: within [1, fpsMax] for manual-entry capabilities, or a match against one of the fixed fps options otherwise. Compares fps option values as strings, since they arrive from the Python backend as strings (e.g. "30") while the saved FPS is a parsed number - verified this mismatch would otherwise silently fail every comparison in the fixed-options case. Falls back to the same default (fpsMax, or the first option, or 30) as before when the saved value isn't valid or wasn't set.
ArduPilot's AP_Camera_MAVLinkCamV2 gates whether it even sends commands like MAV_CMD_IMAGE_START_CAPTURE or MAV_CMD_VIDEO_START_CAPTURE on the CAMERA_CAP_FLAGS bits it previously received in CAMERA_INFORMATION. Since Rpanion was only ever reporting the single capability matching its current mode, ArduPilot would silently refuse to request a photo or video capture while Rpanion was in another mode (e.g. streaming) - so the auto mode-switching logic never even got a chance to run. Confirmed via journalctl: "Received MAVLink command to start image capture" never appeared while in streaming mode, even though the same command worked fine when already in photo mode. Now that Rpanion can auto-switch modes to service any request, report CAPTURE_IMAGE | CAPTURE_VIDEO | HAS_VIDEO_STREAM unconditionally.
Streaming and Video Recording are separate pipelines (video-server.py/ GStreamer vs photovideo.py/Picamera2) but shared a single persisted settings object (camera.videoSettings), so saving one mode's device/ resolution/fps/bitrate/rotation silently overwrote the other's - and a MAVLink-triggered switch into Video Recording mode (e.g. after VIDEO_START_CAPTURE while streaming) would record using whatever Streaming happened to be configured with, not a separately-tuned recording setting. Add a new persisted camera.videoRecordSettings object dedicated to Video Recording mode, independent of Streaming's camera.videoSettings. /api/camera/start now writes to the correct one depending on mode, and sendCameraInformation/getStreamingStatus/onMavPacket read from whichever object matches the current mode. Also fix the Resolution/FPS/Rotation/Bitrate/Media Destination dropdowns on the Photo and Video page getting reset to their first option whenever the Streaming/Video Recording mode radio button was toggled in the UI, even before saving - handleCameraModeChange now remembers each mode's selection for the session (seeded from the backend's own saved settings via new streamingSelection/videoRecordSelection fields on /api/videodevices) instead of resetting to the first device/cap.
… drift Video Recording mode only set a nominal FrameRate control, which auto-exposure can still stretch in low light, letting the actual capture rate drift below what was requested. The recording is also a raw elementary .h264 stream (no container/timestamps), so a player has no way to know the real playback rate unless it's embedded in-band. Pin FrameDurationLimits to a fixed duration so the capture rate can't drift, and pass framerate/enable_sps_framerate to H264Encoder so the actual rate is stamped into the stream's SPS/VUI metadata for players to read.
… mode startPhotoMode() and startVideoMode() both proactively announce CameraInformation/CameraSettings when the camera heartbeat is enabled, but startVideoStreaming() only sent VideoStreamInformation - a GCS/ autopilot that doesn't explicitly re-request CAMERA_INFORMATION via REQUEST_MESSAGE never saw it while Streaming (Rpanion's default mode) was active, only after switching to Photo mode. Pre-dates this session's work (from the original PR) but only became visible now that multi-mode MAVLink discovery is being tested more thoroughly.
handleVideoResChange() capped fpsSelected at Math.min(newFpsMax, 10) instead of just newFpsMax, so choosing a new resolution with a max FPS of 10 or higher always jumped the Framerate field down to 10 - inconsistent with the sibling handleVideoDeviceChange(), which correctly defaults to newFpsMax. Pre-dates this session's work (from the original PR).
switchCameraModeAndTakeAction() would fail an autopilot-triggered mode switch if the target mode had never been configured/saved via the web UI (videoSettings/videoRecordSettings/stillSettings still null) - cameraMode would already show the target mode on the page, but the camera never actually started, silently stuck until the user manually visited that mode's tab and clicked Start at least once. Add ensureDefaultSettingsForMode(), called right before startCamera() during a switch: if the target mode has no saved settings, populate one from the first available device/capability (persisted afterwards, same as a normal save) so the switch can actually succeed instead of requiring the user to have pre-configured every mode in advance.
rmackay9
force-pushed
the
mavcamera-fixes2
branch
from
July 31, 2026 04:49
e1ce988 to
f6d07b1
Compare
Contributor
Author
|
I've tested this and it seems to all work. Perhaps the most controversial change is the automatic switching between streaming, taking pictures and recording video when mavlink messages arrive. This means that a user could turn off their streaming video fairly easily by taking a picture and then it can be hard to get it going again. To resolve this slightly, I've [raised a PR to allow an AP RC auxiliary switch to turn on/off the video streaming](this AP flight code change to ). Still, if @stephendade doesn't like this automatic switch we could remove it. Ideally I would like to have simultaneous streaming, taking pictures and recording video so I could immediately move onto implementing that if you like. |
9 tasks
Owner
|
I'm getting this error when trying to enable the "Video Recording" mode: This is using a CSI camera, RasPi 5 with Bookworm. |
Contributor
Author
|
Hi @stephendade, OK, txs for the review, I'll have a look! |
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
2 participants
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.
This includes many small enhancements and fixes to the Video and Photo feature. This PR is built on top of PR #416 so it should be merged before this one. Once that's done we will rebase this one and the commit list will become much smaller
The code is mostly generated with Claude but I have tested it fairly thoroughly myself. I would like to do a bit more testing before we merge it.
The changes are: