fix: deprecate ZM_COLOUR for AVPixelFormat (resubmit with planar/SHM fixes)#4788
Open
connortechnology wants to merge 3 commits intomasterfrom
Open
fix: deprecate ZM_COLOUR for AVPixelFormat (resubmit with planar/SHM fixes)#4788connortechnology wants to merge 3 commits intomasterfrom
connortechnology wants to merge 3 commits intomasterfrom
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.
Comment on lines
+995
to
+997
| // it. Use this from zms / zma / event paths instead of touching | ||
| // image_buffer[index] directly. | ||
| Image *ReadShmFrame(unsigned int index); |
| } | ||
|
|
||
| if (colours == ZM_COLOUR_GRAY8) { | ||
| if (pix_fmt == AV_PIX_FMT_GRAY8 || zm_is_yuv420(pix_fmt)) { |
| } | ||
|
|
||
| if ( zone_image->Colours() == ZM_COLOUR_GRAY8 ) { | ||
| if (zone_image->PixFormat() == AV_PIX_FMT_GRAY8) { |
Comment on lines
+2475
to
+2482
| if ( imagePixFormat != AV_PIX_FMT_GRAY8) { | ||
| Warning("Target image is already colourised, colours: %u",colours); | ||
| return; | ||
| } | ||
|
|
||
| if ( p_reqcolours == ZM_COLOUR_RGB32 ) { | ||
| AVPixelFormat p_req_pixfmt = zm_pixformat_from_colours(p_reqcolours, p_reqsubpixelorder); | ||
|
|
||
| if ( zm_is_rgb32(p_req_pixfmt) ) { |
Comment on lines
+1135
to
+1138
| // alarm_image follows the same per-slot format convention. Initial format | ||
| // is a placeholder; consumers should AVPixFormat()-sync from | ||
| // image_pixelformats[alarm_index] when reading via GetAlarmImage(). | ||
| alarm_image.AssignDirect(width, height, ZM_COLOUR_YUV420P, ZM_SUBPIX_ORDER_YUV420P, |
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
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