Repository navigation
Conversation
Previous review (2026-10-07)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head Thanks Randy. The photo pipeline and JPEG path look solid and the tests pass, but two things in the sensor-mode switch need fixing before merge.
Smaller things: on failure paths the synchronous capture can block the main loop for over 4 s (1 s drain plus 3 s still timeout), longer than ArduPilot's 1 s mount-health window. Worth shorter timeouts or a README note. The web shutter's ACK loop (mt11-web.cpp:3577) also needs an overall deadline, because 1 Hz heartbeats keep resetting the 10 s receive timeout. None of this was run on CM4 hardware, so how often the race actually hits is unconfirmed. |
Photos are copies of the next camera frame, taken without interrupting streaming or recording, at the 1920x1080 video resolution. They work from the GCS (MAV_CMD_IMAGE_START_CAPTURE, including interval capture) and the web Photos page, and are saved with the shared JPEG metadata (EXIF and GPS) under the capture root. The pipeline maps the camera buffers read-only and copies a requested frame in libcamera's completion callback, with DMA-BUF cache syncs around the read. jpeg.cpp converts the frame from limited-range Rec.709 to the full-range BT.601 that JPEG viewers assume, then encodes the planar 4:2:0 data with libjpeg(-turbo) at quality 90. libjpeg's default error handler exits, so failures return an error instead. The web page's shutter sends MAV_CMD_IMAGE_START_CAPTURE over its MAVLink connection on targets with MAVLink web control and photos; other targets keep the SIYI request unchanged and build identically. Build with libjpeg-dev. tests/test_rpi_jpeg checks the colour conversion by decoding red, green, blue, white, grey and black back to RGB; it needs libjpeg, so it runs in CI and on the Pi rather than in "make test". Tested on the CM4: 13 photos over MAVLink, acknowledged in 62-120 ms; a 15 s RTSP recording during eight photos had 457 frames and no gap over 50 ms; the web page's capture button saves photos and lists them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
b332121 to
760ed8f
Compare
|
I've updated this branch to resolve most of the items raised in the AI review. The testing has mostly still only been done by AI but I will do testing myself in the near future. |
Previous review (2026-10-08)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head Thanks Randy, this round fixes most of the earlier review. AWB is re-enabled on every video start, the normal-path decrement now follows requeue() under video_lock, the failure stalls are shorter and documented, and the web shutter has a real deadline. This was reviewed at 760ed8f; the earlier review was of b332121, which has since been force-pushed. One blocking problem is left, in the new encoder-reset fallback (pipeline.cpp:743). STREAMOFF/STREAMON plus setting encoder_queued to 0 doesn't account for a DQBUF the encoder thread has already completed (pipeline.cpp:322) but not yet requeued and decremented.
Two independent host models, using your function bodies unchanged, reproduce both cases. It needs an encoder stall plus bad timing, so it is probably rare on a CM4 (not measured), but that stall is exactly what this path is for. A lock held across DQBUF, requeue and decrement that the reset also takes (or stopping the encoder thread around the reset) would close it. Checking the STREAMOFF/STREAMON return values would help too. Smaller: reopen_camera() on every resume (pipeline.cpp:803) restarts the IPA from cold. On libcamera 0.7 the first ~6 frames come back flagged FrameStartup rather than dropped. request_complete() encodes them and report_exposure() (pipeline.cpp:243) stores their exposure for the next still lock. Skipping buffers whose metadata status isn't FrameSuccess, behind a version check, would keep them out of the video and the photo lock. At 1 photo/s there should still be about 9 frames per gap by the README timings, so this is mostly a visible blip after each large photo, not a confirmed exposure bug. Hardware wasn't available for this review, so colour, timing and how often the race actually hits are unconfirmed here. |
Add PHOTO_RES ([capture] resolution in camera.ini, on the web Parameters page and in the camera definition) with three choices on the IMX477: - video: a 1920x1080 video frame, without interrupting video (default) - 2028x1520: the 2x2 binned sensor mode, with the full field of view - 4056x3040: the full sensor resolution The larger sizes switch the sensor mode. Video frames stop going to the H.264 encoder, which returns every camera buffer before they are freed; the camera then runs a still configuration in JPEG colour (sYCC) with exposure, gain and colour gains locked to the last video frame, takes the first frame whose exposure matches, and returns to video with a key frame. The encoder keeps running, so RTSP sessions and recordings continue across the gap. Video restarts even if the still capture fails, and each phase is timed in the log. The parameter and its web translations are compiled only for targets defining APCAM_HAVE_PHOTO_RESOLUTION, so other targets are unchanged. apcam/config.h includes target.h when APCAM_TARGET is defined, for the conditional ca_config member; target-independent code such as the camera FTP service still builds without a target. Tested on the CM4 with libcamera 0.7 (and compiled against 0.2): - 4056x3040: pause 20-32 ms, capture 305-368 ms, resume 112-144 ms; video gap up to 0.51 s; acknowledged after about 0.7 s; 2.0 MB JPEG - 2028x1520: video gap 0.31 s; acknowledged after 0.3 s - photos at all three sizes match the video's brightness and colour - 20 photos at 4056x3040 by one-per-second interval capture while recording and streaming; all succeeded, the recording decodes, and camera (CMA) memory, process memory, threads and file descriptors stay flat - brightness and white balance mode changes apply immediately after a photo (daylight to incandescent changes the blue/red ratio 0.80 to 1.06, as on a fresh start) Recordings skip the paused frames, so they are shorter than real time by the pauses. Capture is synchronous, so MAVLink waits up to about 0.7 s (about 2 s if a capture fails). This also included changes to close the encoder reset race, skip start-up frames The encoder thread now dequeues, requeues and decrements encoder_queued under video_lock, and the still capture's encoder reset takes the same lock, so a buffer dequeued but not yet requeued can no longer straddle the reset. The STREAMOFF/STREAMON results are checked and logged. On libcamera 0.7 and later, frames the IPA does not report as FrameSuccess after a restart are returned to the camera instead of being encoded or used for the next photo's exposure lock. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
a780a9e to
f92c3e6
Compare
|
Ok, my AI has had another go and resolving the AI reviews, thanks! |
|
Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head Thanks Randy. This round closes the blocking reset race from the last review. The encoder thread now does DQBUF, requeue and decrement as one step under video_lock (pipeline.cpp:328), and the reset takes the same lock (pipeline.cpp:749). Host models built from your function bodies show no stale requeues or negative counts with the new ordering, while the old ordering reproduces both. This review covers f92c3e6; the previous one was of 760ed8f, which has since been force-pushed away. Two non-blocking things are left:
The JPEG and image-control/CADDX tests pass, and pipeline.cpp compiles against libcamera 0.2.0, 0.5.2 and 0.7.2 headers. No hardware was available, so colour, timing, how visible the start-up frames are and real encoder-reset behaviour remain unconfirmed. |
3 participants
This follow-up to #45 and photo support to an RPI-CM4 ("rpi_libcam_caddx") with a CSI/MIPI camera (an IMX477 shown below)
Detailed changes:
photos are either taken from the streamed video (e.g. lower resolution but no interruption to the video stream) or taken directly from the camera (e.g. higher resolution but video stream pauses). Which occurs is controlled by a new PHOTO_RES parameter exposed via the xml and on the web page
PHOTO_RESphotos are saved as JPEGs with the shared EXIF/GPS metadata under
/var/lib/ap_cameragimbal/capture.Known limitations:
Human testing performed:
AI Testing performed: