fix: deprecate ZM_COLOUR for AVPixelFormat (resubmit with planar/SHM fixes)#4788
Merged
Conversation
…dispatch" This reverts commit 60b6b37.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR resumes the repository-wide migration from legacy ZM_COLOUR_*/ZM_SUBPIX_ORDER_* handling to AVPixelFormat, with follow-up fixes for planar YUV handling, aligned image buffers, and shared-memory format consistency between capture and streaming processes. It sits in the media pipeline core, touching image manipulation, monitor SHM transport, multiple camera backends, and the monitor configuration UI.
Changes:
- Add central
AVPixelFormathelper/mapping utilities and wire cameras/FFmpeg code to prefer pixel-format-based dispatch over legacy colour/subpixel pairs. - Fix multiple
Imagecode paths for planar YUV and aligned linesizes, including copy, scale, JPEG encode/write, and related format bookkeeping. - Add SHM-side per-slot pixel-format tracking for monitor frames and expose the deprecation of the web
Colourssetting.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
web/skins/classic/views/monitor.php |
Adds deprecation text beside the monitor colours setting in the classic UI. |
web/lang/en_gb.php |
Adds the English translation string for the deprecation notice. |
tests/zm_pixformat.cpp |
Introduces Catch2 coverage for pixel-format mapping/helper functions. |
tests/CMakeLists.txt |
Registers the new pixformat test file in the test build. |
src/zm_rgb.h |
Marks legacy colour/subpixel constants as deprecated and adds YUV422 subpixel tags. |
src/zm_remote_camera_rtsp.cpp |
Switches RTSP camera constructor format setup to AVPixelFormat-family checks. |
src/zm_pixformat.h |
Adds central helper functions for colour/subpixel ↔ AVPixelFormat mapping and predicates. |
src/zm_mpeg.cpp |
Replaces manual FFmpeg pixel-format selection with the shared helper. |
src/zm_monitorstream.cpp |
Updates stream consumers to read SHM frames through the new format-sync helper. |
src/zm_monitor.h |
Declares new SHM read/write helpers for per-slot pixel-format consistency. |
src/zm_monitor.cpp |
Implements SHM pixel-format tracking, updates decode/capture paths, and adjusts monitor image handling. |
src/zm_local_camera.cpp |
Migrates local camera format negotiation/conversion decisions to AVPixelFormat logic. |
src/zm_libvnc_camera.cpp |
Migrates VNC camera target format setup to AVPixelFormat logic. |
src/zm_libvlc_camera.cpp |
Migrates libVLC camera target format setup to AVPixelFormat logic. |
src/zm_image.h |
Exposes PixFormat() and updates image pixel-format API comments. |
src/zm_image.cpp |
Fixes planar/aligned image operations and updates many image methods to use AVPixelFormat-aware dispatch. |
src/zm_ffmpeg_camera.cpp |
Migrates FFmpeg camera constructor format setup to AVPixelFormat logic. |
src/zm_ffmpeg.cpp |
Deprecates the old FFmpeg pixel-format helper in favor of the shared mapping helper. |
src/zm_camera.h |
Adds pixelFormat to Camera and marks legacy colour fields as deprecated. |
src/zm_camera.cpp |
Initializes camera line/image sizing from AVPixelFormat-aware FFmpeg helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Several Image methods were silently producing wrong output for planar YUV formats and for any format-width pair where the natural linesize isn't a multiple of 32. Each surfaced as a different visual artefact once the SHM started carrying YUV420P/RGB32/etc. through to the JPEG encoders without an upfront convert-to-RGB32 step: - Image::Assign(const Image &): sized new_size from image.height * image.linesize, which counts only the Y plane for planar layouts. Use image.size, which is populated from av_image_get_buffer_size and includes chroma. Without this an Assign'd YUV420P image left Cb/Cr at zero — solid green output via YCbCr-to-RGB later. - Image::AssignDirect(width,height,colours,...): computed new_buffer_size as W*H*p_colours and linesize as W*p_colours, both wrong for planar (1.5x undersize) and for non-32-aligned RGB widths (the actual buffer stride after sws_scale + av_image_fill_arrays with align=32 is FFALIGN(W*bpp, 32), not W*bpp). Use av_image_get_buffer_size and FFALIGN(av_image_get_linesize(...), 32) so the recorded size/linesize match the real buffer layout. Mismatched linesize produced the diagonal-shift artefact in RGB32 streams at scaled widths like 1094. - Image::WriteBuffer: same FFALIGN linesize fix; it was using the unaligned natural linesize too. - Image::Scale(new_width, new_height): scale_buffer was sized (new_W+1)*(new_H+1)*colours which undercounts planar formats. SWScale::Convert correctly checks the buffer against av_image_get_buffer_size and returns an error, but the caller ignored the return value and AssignDirect'd the empty/uninit buffer anyway. Size with av_image_get_buffer_size, check the Convert return, free and bail on failure. - Image::Scale(factor): the hand-rolled pixel-doubling/decimation loop treated the buffer as packed `colours`-bytes-per-pixel — scaled only the Y plane and dropped chroma. Drop the loop and delegate to Scale(new_width, new_height), which now uses sws_scale for all formats. - Image::EncodeJpeg + Image::WriteJpeg: format dispatch and scanline writer had no planar-YUV support. WriteJpeg's existing branch unpacked the buffer as packed YUYV 4:2:2 with offset scanline*W*2, which read W*H/2 bytes past the end of any YUV420P buffer — a latent crash that became reachable once image_buffer could carry YUV420P data, segfaulting from the Event thread on every event-frame write. Add a planar branch that uses av_image_fill_arrays for plane pointers and per-plane linesizes and feeds JCS_YCbCr scanlines built from the Y/U/V planes — works for both 4:2:0 (YUV420P/YUVJ420P) and 4:2:2 (YUV422P/YUVJ422P). - Image::Overlay: the warning fired on a benign GRAY8-on-YUV420P case (both report colours=1 due to the GRAY8/YUV420P alias collision in zm_rgb.h, but their imagePixFormat differs). Reframe the check so it only warns when imagePixFormat actually matches but the ZM (colours, subpixelorder) metadata diverges — i.e. a real format-tracking bug. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The deprecated Monitors.Colours column was driving the on-disk SHM image_buffer slot format end-to-end: zmc allocated each slot in camera->Colours()/SubpixelOrder() shape, zms attached and built its own per-process Image objects assuming the same shape. Anything that wrote a different format into a slot — i.e. anything that wasn't already RGB32 on a Colours=4 monitor — left zmc's local Image with the new format but zms reading the bytes as RGB32. The visible artefacts varied with the format pair (4 small images per row for YUV420P-into-RGB32, 3 tiles for the planar-as-RGB24 fallthrough, fully garbled colours for image_pixelformats overlapping alarm_image, etc.) but the underlying contract was just broken — the SHM had no representation of the per-slot format. Make the SHM transport any format Image supports without an upfront convert step: - Place image_pixelformats[] correctly. mem_size already reserved image_buffer_count*sizeof(AVPixelFormat) at the end of the SHM region, but the pointer was set to shared_images + image_buffer_count*image_size — overlapping the alarm_image region. zmc's writes scribbled into alarm_image; zms read alarm pixel data back as enum values. Move it to +2*image_buffer_count*image_size, after both image regions. - Add Monitor::WriteShmFrame(index, capture_image): no conversion, just Assign and record image_pixelformats[index] = capture_image->AVPixFormat(). Used by both the normal Decode path and the signal-loss path so they share the same cross-process format-consistency contract. - Add Monitor::ReadShmFrame(index): the read-side counterpart. Calls image_buffer[index]->AVPixFormat(image_pixelformats[index]) before returning so other-process Image objects interpret the SHM bytes correctly per-frame. zm_monitorstream's image_buffer[index] accesses go through this accessor. - image_buffer[i] / alarm_image: initial format is now just a placeholder (YUV420P), with allocation sized from camera->ImageSize() as the upper bound. The actual format carried in each slot is the one zmc wrote, recorded in image_pixelformats and adopted on read. This means the typical capture pipelines run with zero sws_scale calls in the SHM hot path: - H.264/H.265 sw decode -> YUV420P -> identity copy via av_image_copy in Image::Assign(AVFrame*). - LocalCamera/MJPEG -> RGB32 (DecodeJpeg target) -> SHM holds RGB32; zms reads RGB32 directly. - H.265 hwaccel -> NV12 -> falls through to YUV420P (NV12 isn't in zm_pixformat) — one convert at decode time, then passthrough through the rest of the pipeline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
connortechnology
force-pushed
the
fix/avpixelformat-respin
branch
from
d1d02b7 to
c8d8cc3
Compare
- Image::Assign: when source linesize > destination linesize, copy only the destination's row capacity per line; previously copied image.linesize bytes into rows of smaller linesize, overflowing the destination buffer on the last row. - Monitor::CheckSignal: dispatch the 1-byte-per-pixel branch via zm_bytes_per_pixel(pix_fmt) == 1 so YUV422P/J422P are sampled on the Y plane like GRAY8/YUV420P, instead of falling through and reporting "no signal". - Monitor::WriteShmFrame: record image_pixelformats[index] via capture_image->PixFormat() (canonical imagePixFormat) instead of AVPixFormat(), which re-derives from the deprecated (colours, subpixelorder) pair and could propagate stale metadata. - Monitor::connect: rewrite stale comment that claimed readers don't need per-slot pixformat adoption; readers MUST call ReadShmFrame() to adopt the actual format zmc wrote. - Camera::Camera: guard linesize/imagesize derivation against AV_PIX_FMT_NONE and negative returns from av_image_get_linesize / av_image_get_buffer_size, falling back to width * colours stride so unsigned wrap-around can't break SHM sizing. refs #4788 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rlay bounds - Image::Overlay: in the 1-byte-per-pixel branch, bound the loop by std::min(height*linesize, image.height*image.linesize) instead of `buffer + size`. For planar YUV destinations `size` includes chroma planes, while GRAY8 sources have only Y bytes; the old loop read past image.buffer and clobbered the destination's chroma. Restricting to the smaller of the two Y-plane spans keeps the overlay in the luma plane and never over-reads the source. - Monitor::ReadShmFrame: image_pixelformats[] lives in SHM and is written by another process, so its value must be treated as untrusted. Validate via zm_colours_from_pixformat() before passing into AVPixFormat(fmt) — an unsupported enum value would make av_image_get_buffer_size return negative and wrap into the Image's unsigned size/linesize members. Compare against PixFormat() (canonical imagePixFormat) instead of the deprecated AVPixFormat() getter. - Monitor::Capture signal-loss path: reorder SHM publishing so the slot bytes and per-slot shared_timestamps[index] are written first, then last_write_time is set from packet->timestamp (the fresh value, not the stale tv_sec the slot held from its previous occupant), and last_write_index is assigned last as the commit step. Readers gate on last_write_index, so all per-slot state must be visible before that final assignment. refs #4788 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Image::Assign(raw buffer overload): derive new_size from
av_image_get_buffer_size(pix_fmt, w, h, 32) instead of
width*height*colours, which undercounts planar formats (YUV420P/J420P/
YUV422P/J422P) and would silently truncate U/V planes. Update linesize
in the same branch so LineSize() consumers (JPEG encode, overlay) stay
in sync with the new format. Reject AV_PIX_FMT_NONE and negative
av_image_get_buffer_size returns up-front.
- Image::AVPixFormat(AVPixelFormat) setter: validate the requested
format via zm_colours_from_pixformat() and check both av_image_*
return values before mutating any state. On failure, leave
imagePixFormat/size/linesize untouched so callers can recover instead
of inheriting wrapped-unsigned junk.
- Image::Assign(const Image&) stride-mismatch branch: refuse planar
formats (YUV420P/J420P/YUV422P/J422P) explicitly. The per-row copy
only touches the Y plane; doing it silently on planar input leaves
U/V uninitialised in the destination and produces solid-green output
downstream. Caller must reconvert or reallocate.
- Monitor::connect: size SHM slots to an upper bound across every
supported AVPixelFormat (RGBA at w*h, align=32) instead of the
monitor's configured camera->ImageSize(). Prevents "Held buffer is
undersized" failures when the no-conversion pipeline transports a
format larger than the legacy DB Colours selection (e.g. YUV422P or
RGBA into a GRAY8-configured monitor). Store the chosen capacity on
the Monitor as shm_slot_size.
- Monitor::ReadShmFrame: validate image_pixelformats[index] both as a
recognised enum AND that its av_image_get_buffer_size fits
shm_slot_size before calling AVPixFormat(fmt). Treats a corrupted or
torn SHM value as ignorable instead of letting it overrun the held
slot. Single zm_colours_from_pixformat() call (no longer duplicated).
- web/.../monitor.php: inline fallback for
translate('DeprecatedColoursSetting') so non-en_gb locales render an
English string instead of the literal key name.
refs #4788
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
alarm_image's bytes live in SHM but its Image metadata (imagePixFormat/linesize/size) was per-process. Readers (zms) constructed alarm_image with a hardcoded YUV420P placeholder at attach time and there was no path for the writer (zmc/zma) to publish the actual format it wrote. The earlier comment pointed at image_pixelformats[alarm_index], but image_pixelformats[] only tracks the ring-buffer slots and no alarm_index exists. The result: when the writer assigned an RGB24/RGBA frame, reader streaming/encoding interpreted the bytes as YUV420P and produced garbage. Fix: - Allocate one extra AVPixelFormat slot at the end of SHM (alarm_image_pixelformat) and point a new Monitor member at it. - Add Monitor::WriteAlarmImage(const Image&) helper that mirrors WriteShmFrame's contract for the alarm slot: copy bytes, then publish the canonical PixFormat(). Replace the two direct alarm_image.Assign() call sites in Analyse() with WriteAlarmImage(). - Make Monitor::GetAlarmImage() sync alarm_image's format from *alarm_image_pixelformat before returning, with the same defensive validation ReadShmFrame uses (recognised enum + required size fits shm_slot_size, otherwise keep current format with a warning). - Initialise both image_pixelformats[] and alarm_image_pixelformat to AV_PIX_FMT_NONE in the CAPTURE-side memset path, since memset's 0 collides with AV_PIX_FMT_YUV420P rather than the NONE sentinel readers expect for "format not yet published". refs #4788 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 17 comments.
Comments suppressed due to low confidence (4)
src/zm_image.cpp:2406
- Image::Annotate's 1-byte-per-pixel branch steps between rows using
width, but this code now frequently uses an FFALIGN'dlinesize. For non-32-aligned widths (or any case where linesize!=width), annotations will be written at the wrong offsets. Use linesize as the row stride for pointer arithmetic.
if (zm_bytes_per_pixel(imagePixFormat) == 1) {
uint8 *ptr = &buffer[(y * width) + x0];
for (char c : line) {
for (uint64 cp_row : font_variant.GetCodepoint(c)) {
if (bg_colour != kRGBTransparent) {
src/zm_image.cpp:3181
- Deinterlace_Discard() RGB24 branch uses ((y*width)3) strides, which assumes tightly packed rows. With aligned
linesize, the start of each row is buffer + ylinesize. Uselinesizefor row starts.
} else if ( zm_is_rgb24(imagePixFormat) ) {
const uint8_t *psrc;
uint8_t *pdest;
for (unsigned int y = 0; y < (unsigned int)height; y += 2) {
psrc = buffer + ((y * width) * 3);
pdest = buffer + (((y+1) * width) * 3);
for (unsigned int x = 0; x < (unsigned int)width; x++) {
src/zm_image.cpp:3193
- Deinterlace_Discard() RGB32 branch uses width-based row strides (ywidth)<<2, which is incorrect when
linesizeis aligned/padded. Uselinesizefor row starts before casting to Rgb.
} else if ( zm_is_rgb32(imagePixFormat) ) {
const Rgb *psrc;
Rgb *pdest;
for (unsigned int y = 0; y < (unsigned int)height; y += 2) {
psrc = (Rgb*)(buffer + ((y * width) << 2));
pdest = (Rgb*)(buffer + (((y+1) * width) << 2));
for (unsigned int x = 0; x < (unsigned int)width; x++) {
src/zm_monitor.cpp:1881
- CheckSignal() indexes RGB32 pixels as
((const Rgb*)buffer)+index, which assumes tightly packed rows. When LineSize() includes padding, compute the pixel address using y*LineSize() and x (index%width).
} else {
/* Assume RGBA or BGRA */
if (RGBA_BGRA_ZEROALPHA(*(((const Rgb*)buffer)+index)) != RGBA_BGRA_ZEROALPHA(colour_val))
return true;
…afe logs (PR #4788) - Image::AssignDirect(const AVFrame*): both the invalidate() failure path and the success path overwrote buffer/buffertype without freeing any previously-owned buffer. If the Image currently owned a ZM_BUFTYPE_ZM/MALLOC/NEW allocation, that memory was leaked on every AssignDirect(frame) call. Added DumpImgBuffer() at the top of invalidate() and immediately before the success-path buffer reassignment; DumpBuffer is a no-op for DONTFREE buffers, so this is safe whether the Image previously owned its memory or wrapped a caller's buffer. - Switched five remaining av_get_pix_fmt_name() calls flagged by review to zm_get_pix_fmt_name(): * Monitor::GetAlarmImage warnings (zm_monitor.cpp:1432, 1438-1439) * Monitor::ReadShmFrame warnings (zm_monitor.cpp:3042, 3049-3050) * Camera ctor fallback Error (zm_camera.cpp:90) All can be reached with AV_PIX_FMT_NONE or unrecognised formats where av_get_pix_fmt_name returns nullptr, which is undefined when fed to %s. refs #4788 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The destination format for the same-format fast path was derived via the deprecated AVPixFormat() getter, which re-derives from the legacy (colours, subpixelorder) pair. If those fields ever drift out of sync with imagePixFormat — or hit the GRAY8/YUV420P alias collision — the fast path would compare against the wrong target and either fall through to sws_scale unnecessarily or, worse, av_image_copy the wrong layout. Use imagePixFormat directly and drop the manual av_get_pix_fmt_name nullptr dance in the Debug log in favour of the zm_get_pix_fmt_name wrapper. refs #4788 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
GetImage() and getSnapshot() both ran the bounds check `(size_t)index >= image_buffer.size()` before testing the sentinel `index == image_buffer_count`. When no frames have been written yet, shared_data->last_write_index equals image_buffer_count, the initial fallback assigns that to index, and image_buffer.size() also equals image_buffer_count (the vector is sized to image_buffer_count). So the bounds check fired first, logging the misleading "Image Buffer has not been allocated" and returning -1/nullptr instead of reaching the intended "no images in buffer" branch (return 0 / nullptr with a proper message). Reorder both functions to handle the sentinel after confirming image_buffer is non-empty, then do the bounds check, then the actual slot read. refs #4788 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…4788) The raw-buffer Assign overload previously validated buffer_size against av_image_get_buffer_size(..., align=32) and then memcpy'd `size` bytes flat. But callers feed it packed source buffers — Camera::ImageSize() is now derived with align=1 to describe device buffers (V4L2 mmap, raw RTP, etc.) — so for non-32-aligned widths: * the size check spuriously rejected valid packed source buffers, and * if relaxed, the flat memcpy would have read past the source. Validate the source size against the packed (align=1) layout and the destination size/allocation against the aligned (align=32) layout, then copy plane-by-plane via av_image_copy. av_image_fill_arrays gives us both layouts' per-plane pointers and strides, and av_image_copy walks each plane with its own src/dst linesize so per-row padding never over-reads the source. refs #4788 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ixdesc.h (PR #4788) - Image::Assign(const Image&): when the source linesize was *smaller* than the destination's (e.g. src came from a held-buffer ctor with a packed device stride while dst is FFALIGN'd to 32 from av_image_get_buffer_size), the function fell through to the flat memcpy of `size` bytes — over-reading the source by (linesize - image.linesize) * height bytes. Undefined behaviour; could crash or copy unrelated memory. Generalised the per-row branch to handle both directions: trigger whenever linesize != image.linesize and copy min(src, dst) bytes per row, so neither buffer is read or written past its capacity. Planar formats still refuse explicitly since per-row would lose chroma; the error message now reads "vs" instead of ">" to match the broader condition. - Added an explicit `#include <libavutil/pixdesc.h>` in zm_image.cpp. The recent Rotate/Flip rewrites call av_pix_fmt_desc_get / av_pix_fmt_count_planes / use AVPixFmtDescriptor, which previously only compiled thanks to transitive includes from zm_ffmpeg.h → libavutil/imgutils.h. Make the dependency explicit. refs #4788 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ct (PR #4788) The Image(const AVFrame*) constructor only initialised blend_buffer_ and blend_buffer_size_ before calling AssignDirect(frame). AssignDirect now calls DumpImgBuffer() on both the failure (invalidate) and success paths to release any previously-owned buffer — but in the freshly-constructed Image, buffer/buffertype/allocation are uninitialised, so DumpImgBuffer would read garbage and potentially free an invalid pointer. Use constructor delegation so Image() runs first (zero-initialises every member, sets buffertype = DONTFREE), making the subsequent DumpImgBuffer call a no-op on a known-empty Image. refs #4788 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…er (PR #4788) - Monitor::GetImage and Monitor::getSnapshot bounds-check branches were logging "Image Buffer has not been allocated", but by the time control reaches them the empty-vector check has already passed — the buffer IS allocated, the index is just out of range. Replaced with explicit "index N out of range (image_buffer.size() = M)" so debugging starts from the right place. - Image::WriteBuffer unsupported-format error only printed `colours`, but the mapping in zm_pixformat_from_colours depends on both `colours` and `subpixelorder`, and planar formats legitimately have colours=1. Print both fields so a bad (colours, subpixelorder) pair is immediately identifiable. refs #4788 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The y += 2 loop ran while `y < height`, but copied even row y into odd row y+1. For odd image heights, the final iteration had y == height-1 and wrote to row height — past the end of the image buffer in all three colour branches (GRAY8/YUV-Y plane, RGB24, RGB32). Stop one short of the last row (y < height - 1) so y+1 stays in bounds; the orphan last row in odd-height images is left untouched (consistent with the (even, odd) pairing this discard pass is doing). Also added a guard for height < 2 to keep the unsigned `height - 1` calculation from wrapping. refs #4788 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ic (PR #4788) - Image::Delta: pass delta8_bgr/rgb/argb/abgr/bgra/rgba/gray8 directly to delta_row() instead of dereferencing with *. They're already function pointers; the deref is redundant and would be undefined behaviour if any pointer were null (Initialise() sets them, but the explicit * obscures the intent and adds nothing). - Switched the Delta fallback Panic from av_get_pix_fmt_name() to the nullptr-safe zm_get_pix_fmt_name() wrapper. The Panic fires exactly when imagePixFormat is unrecognised, where the raw function would return nullptr. refs #4788 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…paths (PR #4788) - Image::Colourise sizing-failure Error: av_image_get_buffer_size / av_image_get_linesize return negative for AV_PIX_FMT_NONE and unrecognised formats, so this branch fires exactly when av_get_pix_fmt_name(p_req_pixfmt) can return nullptr. Switched to zm_get_pix_fmt_name. - Image::Deinterlace_4Field fallback Panic: same issue — fires when imagePixFormat is outside our dispatch set, where av_get_pix_fmt_name may return nullptr. Switched to zm_get_pix_fmt_name. refs #4788 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zm_get_pix_fmt_name() calls av_get_pix_fmt_name() which is declared in <libavutil/pixdesc.h>. The header previously got away with relying on transitive includes via zm_ffmpeg.h, but tests/zm_pixformat.cpp (and any other consumer that includes only zm_pixformat.h) would fail to compile. Add the explicit include so the header is self-contained. refs #4788 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The field was assigned in three branches of the constructor (always to the same value as Camera::pixelFormat) and never read anywhere. Unlike LocalCamera, FfmpegCamera doesn't run a sws_scale step at the Camera layer — libavcodec hands frames to the pipeline directly — so there's no separate capture-vs-image format distinction to track. Drop the redundant assignments and the member declaration. The canonical camera-side format is now exclusively Camera::pixelFormat (via Camera::PixelFormat()). refs #4788 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Jun 11, 2026
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
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.
Summary
Resubmits the AVPixelFormat-migration work that landed in PR #4742 and was reverted in #4786. This time with the bugs that motivated the revert addressed, plus the cross-process SHM format-consistency contract that the original PR didn't put in place.
Three commits, intended to be reviewed in order:
1. `Reapply "fix: replace ZM_COLOUR system with AVPixelFormat for format dispatch"`
Pure revert-of-revert of #4786. Restores the original PR's content unchanged so the diff against master shows what the AVPixelFormat migration actually does. Includes the two rounds of Copilot review feedback that landed inside the original PR.
2. `fix: planar YUV correctness + alignment in Image methods`
Once `Image`/`zm_pixformat` started carrying YUV420P/RGB32/etc. through the pipeline without an upfront convert-to-RGB32, several Image methods that were silently broken for planar layouts and for non-32-aligned linesizes started producing visible artefacts (or crashing). Each fix is independent of the SHM work in #3:
3. `feat: SHM cross-process format consistency via per-slot AVPixelFormat`
The deprecated `Monitors.Colours` column was driving the on-disk image_buffer slot format end-to-end. Anything that wrote a different format into a slot left zmc's local Image with the new format but zms reading the bytes as the original. This was the underlying bug class behind the merge-then-revert symptoms.
The whole point of the migration was to avoid the per-frame YUV→RGB sws_scale that the deprecated Colours=4 forced. With this commit the typical pipelines run zero converts in the SHM hot path:
Test plan
🤖 Generated with Claude Code