Skip to content

fix: replace ZM_COLOUR system with AVPixelFormat for format dispatch#4742

Merged
connortechnology merged 11 commits intomasterfrom
fix/deprecate-zm-colour-use-avpixelformat
May 2, 2026
Merged

fix: replace ZM_COLOUR system with AVPixelFormat for format dispatch#4742
connortechnology merged 11 commits intomasterfrom
fix/deprecate-zm-colour-use-avpixelformat

Conversation

@connortechnology
Copy link
Copy Markdown
Member

Summary

  • Replace legacy ZM_COLOUR_* / ZM_SUBPIX_ORDER_* integer pair with AVPixelFormat as the single source of truth for pixel format dispatch across the codebase
  • Add src/zm_pixformat.h with central format helpers (zm_pixformat_from_colours, zm_is_rgb32, zm_is_rgb24, zm_is_yuv420, zm_bytes_per_pixel, zm_db_colours_to_pixformat)
  • Migrate all ~100 format dispatch comparisons in zm_image.cpp, camera subclasses, zm_monitor.cpp, zm_mpeg.cpp, zm_ffmpeg.cpp from colours/subpixelorder checks to imagePixFormat/AVPixelFormat checks
  • Fix imagePixFormat sync bug in WriteBuffer, Assign, AssignDirect — these updated colours/subpixelorder without updating imagePixFormat, causing format misidentification (vertical lines and washed-out colors in live stream)
  • Fix DeColourise bug where imagePixFormat was not updated to AV_PIX_FMT_GRAY8
  • Add AVPixelFormat pixelFormat member + PixelFormat() accessor to Camera
  • Add PixFormat() accessor to Image
  • Deprecate GetFFMPEGPixelFormat(), delegate to zm_pixformat_from_colours()
  • Deprecate ZM_COLOUR_* and ZM_SUBPIX_ORDER_* constants in zm_rgb.h
  • Add deprecation notice on Monitor.Colours web UI dropdown
  • Add 13 Catch2 test cases (105 assertions) for format mapping helpers

Test plan

  • ctest — 105 pixformat assertions pass, no regressions
  • Full build clean (all targets: zmc, zms, zmu, zma, zm_rtsp_server)
  • V4L2 MJPEG camera with Colours=4 (RGB32): verify color output, no vertical lines
  • Monitor with Colours=1 (GRAY8): verify still works as grayscale
  • Event recording via h264 encoder: verify video output is correct

Fixes #4735

🤖 Generated with Claude Code

connortechnology and others added 2 commits April 18, 2026 11:42
…mat dispatch

ZM_COLOUR_GRAY8, ZM_COLOUR_YUV420P, and ZM_COLOUR_YUVJ420P were all
defined to 1, making format identification via colours ambiguous.
LocalCamera misidentified YUV420P as GRAY8, causing V4L2 MJPEG cameras
to decode to grayscale via expensive sws_scale conversion.

Replace the legacy ZM_COLOUR_*/ZM_SUBPIX_ORDER_* integer pair with
AVPixelFormat as the single source of truth for pixel format dispatch:

- Add src/zm_pixformat.h with central format helpers:
  zm_pixformat_from_colours, zm_colours_from_pixformat,
  zm_bytes_per_pixel, zm_db_colours_to_pixformat, zm_is_rgb32,
  zm_is_rgb24, zm_is_yuv420
- Add AVPixelFormat pixelFormat member + PixelFormat() accessor to Camera
- Add PixFormat() accessor to Image, delegate AVPixFormat methods
  to shared helpers
- Migrate all ~100 format dispatch comparisons in zm_image.cpp,
  zm_local_camera.cpp, zm_ffmpeg_camera.cpp, zm_remote_camera_rtsp.cpp,
  zm_libvlc_camera.cpp, zm_libvnc_camera.cpp, zm_monitor.cpp,
  zm_mpeg.cpp from colours/subpixelorder checks to imagePixFormat/
  AVPixelFormat checks
- Deprecate GetFFMPEGPixelFormat, delegate to zm_pixformat_from_colours
- Fix DeColourise bug: imagePixFormat was not updated to GRAY8
- Deprecate ZM_COLOUR_* and ZM_SUBPIX_ORDER_* constants in zm_rgb.h
- Add deprecation notice on Monitor.Colours web UI dropdown
- Add 13 Catch2 test cases (105 assertions) for format mapping helpers

refs #4735

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
After migrating format dispatch from colours to imagePixFormat,
several methods that update colours/subpixelorder were not also
updating imagePixFormat, leaving it stale. This caused format
misidentification downstream — e.g. DecodeJpeg falling back to
RGB24 while imagePixFormat still claimed RGBA, producing vertical
lines and washed-out colors in the live stream.

Fix WriteBuffer, Assign(buffer), Assign(Image), AssignDirect(buffer),
and AssignDirect(AVFrame) to keep imagePixFormat in sync.

refs #4735

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 18, 2026 16:28
Copy link
Copy Markdown
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 modernizes ZoneMinder’s pixel-format handling by making AVPixelFormat the authoritative format identifier (replacing the legacy ZM_COLOUR_* + ZM_SUBPIX_ORDER_* pair) to fix long-standing format-collision issues and reduce fragile dispatch logic across cameras, images, monitors, and MPEG/FFmpeg components.

Changes:

  • Introduces src/zm_pixformat.h with centralized pixel-format mapping/predicates and migrates many call sites to dispatch on AVPixelFormat.
  • Fixes several Image format-sync issues by keeping imagePixFormat updated in buffer assignment/write paths and when de/colourising.
  • Adds Catch2 unit tests for pixformat mappings and adds a web UI deprecation notice for the legacy Monitor “Colours” dropdown.

Reviewed changes

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

Show a summary per file
File Description
web/skins/classic/views/monitor.php Adds deprecation notice for the legacy “Colours” UI setting.
tests/CMakeLists.txt Registers new pixformat test file in the test suite.
tests/zm_pixformat.cpp Adds unit coverage for format mapping helpers and collision regression.
src/zm_pixformat.h Adds centralized AVPixelFormat mapping helpers and predicates.
src/zm_rgb.h Marks legacy colour/subpixel constants as deprecated and documents DB compatibility.
src/zm_camera.h Adds AVPixelFormat pixelFormat member + accessor; deprecates legacy accessors.
src/zm_camera.cpp Initializes pixelFormat and uses it for stream codecpar format.
src/zm_ffmpeg.cpp Deprecates GetFFMPEGPixelFormat() and delegates to pixformat helper.
src/zm_mpeg.cpp Simplifies codec setup pixel-format selection via pixformat helper.
src/zm_image.h Adds PixFormat() accessor; deprecates AVPixFormat() getter.
src/zm_image.cpp Migrates dispatch to AVPixelFormat, fixes several sync issues, updates JPEG handling logic.
src/zm_monitor.cpp Updates image handling/dispatch to use PixFormat() and pixformat helpers.
src/zm_local_camera.cpp Updates palette/target format matching and auto-select logic to use AVPixelFormat helpers.
src/zm_remote_camera_rtsp.cpp Updates camera constructor format dispatch to use pixelFormat helpers.
src/zm_ffmpeg_camera.cpp Updates camera constructor format dispatch to use pixelFormat helpers.
src/zm_libvnc_camera.cpp Updates VNC camera format dispatch to use pixelFormat helpers.
src/zm_libvlc_camera.cpp Updates libVLC camera format dispatch and chroma selection via pixformat helpers.
Comments suppressed due to low confidence (2)

src/zm_image.cpp:1004

  • HighlightEdges computes destination pointers using this->linesize (source stride). If the source grayscale image came from an AVFrame with padding (linesize > width), then phigh will advance too far and can write past high_buff. Use the destination image stride (high_image->LineSize()) or compute offsets from width/bytes-per-pixel instead of reusing the source linesize.
    for ( unsigned int y = lo_y; y <= hi_y; y++ ) {
      const uint8_t* p = buffer + (y * linesize) + lo_x;
      uint8_t* phigh = high_buff + (((y * linesize) + lo_x) * 3);
      for ( unsigned int x = lo_x; x <= hi_x; x++, p++, phigh += 3 ) {
        bool edge = false;
        if ( *p ) {
          edge = (x > 0 && !*(p-1)) || (x < (width-1) && !*(p+1)) || (y > 0 && !*(p-width)) || (y < (height-1) && !*(p+width));
#if 0
          if ( !edge && x > 0 && !*(p-1) ) edge = true;
          if ( !edge && x < (width-1) && !*(p+1) ) edge = true;
          if ( !edge && y > 0 && !*(p-width) ) edge = true;
          if ( !edge && y < (height-1) && !*(p+width) ) edge = true;
#endif
        }
        if ( edge ) {
          RED_PTR_RGBA(phigh) = RED_VAL_RGBA(colour);
          GREEN_PTR_RGBA(phigh) = GREEN_VAL_RGBA(colour);
          BLUE_PTR_RGBA(phigh) = BLUE_VAL_RGBA(colour);
        }
      }
    }
  } else if ( zm_is_rgb32(p_pixfmt) ) {
    for ( unsigned int y = lo_y; y <= hi_y; y++ ) {
      const uint8_t* p = buffer + (y * linesize) + lo_x;
      Rgb* phigh = (Rgb*)(high_buff + (((y * linesize) + lo_x) * 4));
      for ( unsigned int x = lo_x; x <= hi_x; x++, p++, phigh++ ) {

src/zm_image.cpp:1399

  • The YUV420/J420 JPEG path assumes a packed 4:2:2 (YUYV) layout (e.g., offset uses width*2 and reads bytes as Y U Y V). But the predicate now triggers for AV_PIX_FMT_YUV420P/YUVJ420P, which are planar 4:2:0 formats with different layout/stride; this can read out of bounds and/or encode incorrect colors. Either switch this branch to the correct packed pixel format (e.g., AV_PIX_FMT_YUYV422) or rework it to read true planar YUV420P data (using proper plane pointers/strides).
    } else if (zm_is_yuv420(imagePixFormat)) {
      cinfo->in_color_space = JCS_YCbCr;
    } else {
      /* Assume RGB */
      /*
      #ifdef JCS_EXTENSIONS
      cinfo->out_color_space = JCS_EXT_RGB;
      #else
      cinfo->out_color_space = JCS_RGB;
      #endif
      */
      cinfo->in_color_space = JCS_RGB;
    }
  }  // end format dispatch

  jpeg_set_defaults(cinfo);
  jpeg_set_quality(cinfo, quality, FALSE);
  cinfo->dct_method = JDCT_FASTEST;

  jpeg_start_compress(cinfo, TRUE);
  if (config.add_jpeg_comments && !annotation_.empty()) {
    jpeg_write_marker(cinfo, JPEG_COM, reinterpret_cast<const JOCTET *>(annotation_.c_str()), annotation_.size());
  }
  // If we have a non-zero time (meaning a parameter was passed in), then form a simple exif segment with that time as DateTimeOriginal and SubsecTimeOriginal
  // No timestamp just leave off the exif section.
  if (timestamp.time_since_epoch() > Seconds(0)) {
#define EXIFTIMES_MS_OFFSET 0x36   // three decimal digits for milliseconds
#define EXIFTIMES_MS_LEN  0x03
#define EXIFTIMES_OFFSET  0x3E   // 19 characters format '2015:07:21 13:14:45' not including quotes
#define EXIFTIMES_LEN     0x13   // = 19
#define EXIF_CODE       0xE1

    // This is a lot of stuff to allocate on the stack.  Recommend char *timebuf[64];
    char timebuf[64], msbuf[64];

    tm timestamp_tm = {};
    time_t timestamp_t = std::chrono::system_clock::to_time_t(timestamp);
    strftime(timebuf, sizeof timebuf, "%Y:%m:%d %H:%M:%S", localtime_r(&timestamp_t, &timestamp_tm));
    Seconds ts_sec = std::chrono::duration_cast<Seconds>(timestamp.time_since_epoch());
    Microseconds ts_usec = std::chrono::duration_cast<Microseconds>(timestamp.time_since_epoch() - ts_sec);
    // we only use milliseconds because that's all defined in exif, but this is the whole microseconds because we have it
    snprintf(msbuf, sizeof msbuf, "%06d", static_cast<int32>(ts_usec.count()));

    unsigned char exiftimes[82] = {
      0x45, 0x78, 0x69, 0x66, 0x00, 0x00, 0x49, 0x49, 0x2A, 0x00, 0x08, 0x00, 0x00, 0x00, 0x01, 0x00,
      0x69, 0x87, 0x04, 0x00, 0x01, 0x00, 0x00, 0x00, 0x1A, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
      0x02, 0x00, 0x03, 0x90, 0x02, 0x00, 0x14, 0x00, 0x00, 0x00, 0x38, 0x00, 0x00, 0x00, 0x91, 0x92,
      0x02, 0x00, 0x04, 0x00, 0x00, 0x00, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x00, 0x00, 0xff, 0xff,
      0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
      0xff, 0x00
    };
    memcpy(&exiftimes[EXIFTIMES_OFFSET], timebuf, EXIFTIMES_LEN);
    memcpy(&exiftimes[EXIFTIMES_MS_OFFSET], msbuf, EXIFTIMES_MS_LEN);
    jpeg_write_marker(cinfo, EXIF_CODE, (const JOCTET *) exiftimes, sizeof(exiftimes));
  }

  if (zm_is_yuv420(imagePixFormat)) {
    std::vector<uint8_t> tmprowbuf(width * 3);
    JSAMPROW row_pointer = &tmprowbuf[0];  /* pointer to a single row */
    while (cinfo->next_scanline < cinfo->image_height) {
      unsigned i, j;
      unsigned offset = cinfo->next_scanline * cinfo->image_width * 2; //offset to the correct row
      for (i = 0, j = 0; i < cinfo->image_width * 2; i += 4, j += 6) { //input strides by 4 bytes, output strides by 6 (2 pixels)
        tmprowbuf[j + 0] = buffer[offset + i + 0]; // Y (unique to this pixel)
        tmprowbuf[j + 1] = buffer[offset + i + 1]; // U (shared between pixels)
        tmprowbuf[j + 2] = buffer[offset + i + 3]; // V (shared between pixels)
        tmprowbuf[j + 3] = buffer[offset + i + 2]; // Y (unique to this pixel)
        tmprowbuf[j + 4] = buffer[offset + i + 1]; // U (shared between pixels)
        tmprowbuf[j + 5] = buffer[offset + i + 3]; // V (shared between pixels)
      }

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

Comment thread src/zm_local_camera.cpp Outdated
conversion_type = 0;
} else if (palette == V4L2_PIX_FMT_RGB24 && zm_is_rgb24(pixelFormat)) {
conversion_type = 0;
subpixelorder = ZM_SUBPIX_ORDER_BGR;
Copy link

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

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

For V4L2_PIX_FMT_RGB24, this sets subpixelorder to BGR and then derives pixelFormat from that, which will treat an RGB24 capture buffer as BGR24 (red/blue swapped). This should use ZM_SUBPIX_ORDER_RGB (and resulting AV_PIX_FMT_RGB24) when the capture palette is RGB24.

Suggested change
subpixelorder = ZM_SUBPIX_ORDER_BGR;
subpixelorder = ZM_SUBPIX_ORDER_RGB;

Copilot uses AI. Check for mistakes.
Comment thread src/zm_image.cpp Outdated
break;
}
imagePixFormat = static_cast<AVPixelFormat>(frame->format);
zm_colours_from_pixformat(imagePixFormat, colours, subpixelorder);
Copy link

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

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

zm_colours_from_pixformat() can return false for unrecognised AVPixelFormat values, leaving colours/subpixelorder unchanged. That can make Image state inconsistent with imagePixFormat (and with the assigned frame). Please handle the false case explicitly (e.g., log and set imagePixFormat=AV_PIX_FMT_NONE plus a safe colours/subpixelorder), or restrict AssignDirect() to supported formats.

Suggested change
zm_colours_from_pixformat(imagePixFormat, colours, subpixelorder);
if ( !zm_colours_from_pixformat(imagePixFormat, colours, subpixelorder) ) {
Error("AssignDirect called with unsupported pixel format: %d", frame->format);
imagePixFormat = AV_PIX_FMT_NONE;
colours = 0;
subpixelorder = 0;
}

Copilot uses AI. Check for mistakes.
connortechnology and others added 8 commits April 18, 2026 18:38
MJPEG decoder outputs AV_PIX_FMT_YUVJ422P which was not handled by
zm_colours_from_pixformat, causing "Unknown pixelformat 13 yuvj422p"
errors and leaving imagePixFormat stale — resulting in random image
corruption.

Add YUV422P and YUVJ422P cases to zm_colours_from_pixformat and
zm_bytes_per_pixel in zm_pixformat.h.

refs #4735

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…RAY8-only

The migration from colours==ZM_COLOUR_GRAY8 to imagePixFormat==GRAY8
broke YUV420P handling: the old check matched both GRAY8 and YUV420P
(both had colours==1) but the new check only matched GRAY8.

For pixel-rendering operations (Annotate, Fill, Outline, DrawLine,
Rotate, Flip, Delta, MaskPrivacy, Deinterlace, Overlay) that work on
the Y-plane of any 1-byte-per-pixel format, replace the GRAY8-only
check with zm_bytes_per_pixel(imagePixFormat)==1. This covers GRAY8,
YUV420P, YUVJ420P, YUV422P, and YUVJ422P.

Format-identification sites (JPEG encoding, Colourise, DeColourise)
that genuinely distinguish grayscale from YUV are left unchanged.

refs #4735

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…d YUV420P

linesize and imagesize were still hardcoded to AV_PIX_FMT_YUV420P
sizing despite colours/pixelFormat being set correctly from DB.
For RGBA monitors, shared memory slots were 3.7x too small, causing
buffer overflow and intermittent corruption in the live stream.

Use the Camera's pixelFormat member (set from p_colours/p_subpixelorder)
for linesize and imagesize computation.

refs #4735

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The decoder was creating packet->image using camera->Colours() from the
DB (typically RGBA), forcing an expensive conversion from the decoded
frame's native format (typically YUV420P). This was wasteful — the
camera already decoded to a usable format — and caused 3x larger shared
memory frames, worsening the live-view race condition.

Use the in_frame's native pixel format directly. Only fall back to the
DB format if the native format is unrecognised. For a typical h264
camera producing YUV420P, this eliminates the YUV420P->RGBA conversion
entirely and keeps frames at 5.5MB instead of 14.7MB (for 2560x1440).

refs #4735

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add YUV422P/YUVJ422P subpixelorder constants and proper mapping in
  zm_pixformat.h so MJPEG cameras keep full chroma through the pipeline
- Fix decoder to only passthrough formats Image handles with full color
- Fix libjpeg EncodeJpeg/WriteJpeg: use zm_bytes_per_pixel==1 instead
  of GRAY8-only check so planar YUV images encode as grayscale JPEG
  (fixes "split in 3" artifact on inactive monitor placeholder)
- Fix sendTextFrame: use GRAY8 (not YUV420P) for text-on-black frames

refs #4735

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use av_image_copy instead of sws_scale when source and dest format +
dimensions match. Eliminates unnecessary per-frame pixel processing
for passthrough formats (e.g. YUVJ422P from MJPEG cameras).

refs #4735

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two issues flagged by Copilot review on the AVPixelFormat-migration PR,
plus one build fix that was needed to land them:

1. zm_local_camera.cpp set subpixelorder to BGR for V4L2_PIX_FMT_RGB24
   captures. V4L2_PIX_FMT_RGB24 is byte-order R,G,B in memory and is
   mapped to AV_PIX_FMT_RGB24 by getFfPixFormatFromV4lPalette earlier
   in the same file, so the matching ZM subpixel order is RGB. Setting
   BGR meant red and blue were swapped in the captured image whenever
   a V4L2 camera was configured with the RGB24 palette. Long-standing
   bug — preserved unchanged through the AVPixelFormat migration —
   now fixed to ZM_SUBPIX_ORDER_RGB.

2. Image::AssignDirect(const AVFrame*) called zm_colours_from_pixformat
   without checking the return value, leaving colours/subpixelorder at
   their previous values for any unsupported AVPixelFormat. Wrap the
   call and on failure put the Image into an explicit invalid state
   (AV_PIX_FMT_NONE plus zeroed colours/subpixelorder) so the
   inconsistency surfaces immediately instead of producing wrong-format
   reads downstream.

3. Drop the u_buffer = ... / v_buffer = ... assignments inside
   Image::Assign()'s identity-copy path. Those members exist on the
   ai_server lineage but not on master, so the PR branch did not
   compile against master as-is. av_image_copy reads the planes
   directly out of temp_frame->data, so the assignments were not
   load-bearing — they look like leftover state-tracking that didn't
   survive the upstreaming. Comment notes why the lines were removed.

Tests pass: 76 cases, 778 assertions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Image::Overlay() warned when (colours == image.colours &&
subpixelorder != image.subpixelorder), which made sense when
(colours, subpixelorder) was the canonical format identifier.

Now that imagePixFormat is canonical, the check produces false
positives in a normal code path: zm_monitor.cpp's analysis pass calls
analysis_image->Overlay(*(zone.AlarmImage())) where the destination is
YUV420P (colours=1 via the GRAY8/YUV420P=1 alias collision in
zm_rgb.h, subpixelorder=ZM_SUBPIX_ORDER_YUV420P=11) and the source is
the zone's GRAY8 alarm mask (colours=1, subpixelorder=NONE=2). The
overlay dispatch below already handles this correctly via
zm_bytes_per_pixel(imagePixFormat) == 1 on both sides — only the Y
plane of the dest is touched, leaving chroma untouched, which is
exactly the intent. The warning was just noise.

Reframe the check around imagePixFormat: warn only when the
AVPixelFormat actually matches but the ZM (colours, subpixelorder)
metadata diverges, which would indicate a real format-tracking bug.
The new message also names the AVPixelFormat for context, instead of
two opaque integers.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
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

Copilot reviewed 17 out of 17 changed files in this pull request and generated 8 comments.


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

Comment thread web/skins/classic/views/monitor.php Outdated
<li class="TargetColorspace">
<label><?php echo translate('TargetColorspace') ?></label>
<?php echo htmlSelect('newMonitor[Colours]', $Colours, $monitor->Colours()) ?>
<small class="text-muted">(Deprecated - will be auto-detected in a future release)</small>
Comment thread tests/zm_pixformat.cpp
Comment on lines +22 to +33
TEST_CASE("zm_pixformat_from_colours: GRAY8 family", "[pixformat]") {
REQUIRE(zm_pixformat_from_colours(ZM_COLOUR_GRAY8, ZM_SUBPIX_ORDER_NONE) == AV_PIX_FMT_GRAY8);
REQUIRE(zm_pixformat_from_colours(ZM_COLOUR_GRAY8, ZM_SUBPIX_ORDER_YUV420P) == AV_PIX_FMT_YUV420P);
REQUIRE(zm_pixformat_from_colours(ZM_COLOUR_GRAY8, ZM_SUBPIX_ORDER_YUVJ420P) == AV_PIX_FMT_YUVJ420P);
}

TEST_CASE("zm_pixformat_from_colours: RGB24 family", "[pixformat]") {
REQUIRE(zm_pixformat_from_colours(ZM_COLOUR_RGB24, ZM_SUBPIX_ORDER_RGB) == AV_PIX_FMT_RGB24);
REQUIRE(zm_pixformat_from_colours(ZM_COLOUR_RGB24, ZM_SUBPIX_ORDER_BGR) == AV_PIX_FMT_BGR24);
}

TEST_CASE("zm_pixformat_from_colours: RGB32 family", "[pixformat]") {
Comment thread src/zm_image.cpp
Comment on lines 751 to 757
colours = p_colours;
linesize = p_width * p_colours;
subpixelorder = p_subpixelorder;
imagePixFormat = p_pixfmt;
pixels = height*width;
size = newsize;
} // end if need to re-alloc buffer
Comment thread src/zm_image.cpp Outdated
@@ -742,18 +765,19 @@ void Image::AssignDirect(const AVFrame *frame) {
buffer = frame->data[0];
linesize = frame->linesize[0];
allocation = size = av_image_get_buffer_size(static_cast<AVPixelFormat>(frame->format), frame->width, frame->height, 32);
Comment thread src/zm_image.cpp
frame->format, frame->width, frame->height);
imagePixFormat = AV_PIX_FMT_NONE;
colours = 0;
subpixelorder = 0;
Comment thread src/zm_image.cpp Outdated
Comment on lines +373 to +374
Debug(4, "Same format %s %dx%d, using av_image_copy",
av_get_pix_fmt_name(format), width, height);
Comment thread src/zm_monitor.cpp Outdated
Comment on lines +3087 to +3090
Debug(1, "Using native frame format %s", av_get_pix_fmt_name(native_fmt));
} else {
Debug(1, "Converting %s to yuv420p for pipeline", av_get_pix_fmt_name(native_fmt));
native_colours = ZM_COLOUR_GRAY8;
Comment thread src/zm_monitor.cpp
Comment on lines +3080 to +3081
|| native_fmt == AV_PIX_FMT_YUV422P
|| native_fmt == AV_PIX_FMT_YUVJ422P
Eight Copilot comments from the second review pass:

1. monitor.php (#3): "Deprecated - will be auto-detected..." note next
   to TargetColorspace bypassed translate(). Added DeprecatedColoursSetting
   key to en_gb and routed the deprecation note through translate() so it
   localises with the rest of the form.

2. tests/zm_pixformat.cpp (#4): zm_colours_from_pixformat / round-trip
   tests didn't cover the new YUV422P/YUVJ422P entries (added by
   02e6be6). Added explicit assertions in both test cases — bumps
   pixformat coverage from 105 to 115 assertions.

3. zm_image.cpp WriteBuffer (#5): linesize and size were derived from
   p_width * p_colours, which undercounts planar YUV* (where p_colours=1
   via the GRAY8 alias collision but actual buffer needs ~1.5x/2x for
   chroma). Use av_image_get_buffer_size and av_image_get_linesize for
   the AVPixelFormat instead, with bail-out on either failing.

4. zm_image.cpp AssignDirect (#6, #7): av_image_get_buffer_size returns
   int and can be negative; assigning that into unsigned size/allocation
   wrapped to a huge value. Check the return first, treat negative as the
   same "unsupported format" failure as zm_colours_from_pixformat
   returning false, and reset size/allocation/linesize/pixels to 0
   (alongside imagePixFormat=NONE/colours=0/subpixelorder=0) so the
   Image is left in a single coherent invalid state instead of partially
   stale.

5. zm_image.cpp Assign (#8): av_get_pix_fmt_name(format) can return
   nullptr (e.g. AV_PIX_FMT_NONE / unknown); passing that into
   Debug(..., "%s", ...) would segfault. Capture once with a fallback
   string before logging.

6. zm_monitor.cpp Capture path (#9): same nullptr issue with two Debug
   calls — capture native_fmt_name once with fallback.

7. zm_monitor.cpp can_passthrough comment (#10): comment claimed
   YUVJ422P would be converted to YUV420P because Image drops chroma,
   but can_passthrough now allows YUV422P/YUVJ422P passthrough since
   02e6be6 added 4:2:2 support. Updated the comment to describe the
   current behavior (full 4:2:0 + 4:2:2 planar passthrough plus GRAY8
   and RGB24/32) so the code and the rationale agree.

Tests: 76 cases, 788 assertions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@connortechnology connortechnology merged commit 81519fb into master May 2, 2026
8 of 9 checks passed
connortechnology added a commit to connortechnology/ZoneMinder that referenced this pull request May 4, 2026
…ignDirect default

Two issues flagged by review on the AVPixelFormat-migration PR:

1. zm_local_camera.cpp set subpixelorder to BGR for V4L2_PIX_FMT_RGB24
   captures. V4L2_PIX_FMT_RGB24 is byte-order R,G,B in memory and is
   mapped to AV_PIX_FMT_RGB24 by getFfPixFormatFromV4lPalette earlier
   in the same file, so the matching ZM subpixel order is RGB. Setting
   BGR meant red and blue were swapped in the captured image whenever
   a V4L2 camera was configured with the RGB24 palette. Long-standing
   bug — preserved unchanged through the AVPixelFormat migration —
   now fixed to ZM_SUBPIX_ORDER_RGB.

2. Image::AssignDirect(const AVFrame*) had a switch covering only
   AV_PIX_FMT_RGBA and AV_PIX_FMT_YUV420P/YUVJ420P, with a default
   that just emitted Warning() and left colours/subpixelorder at
   their previous values. That left the Image inconsistent with the
   imagePixFormat assigned a few lines above and with the buffer
   pointer the function had just adopted. Replace the switch with a
   call to zm_colours_from_pixformat() (already covers all formats
   the project uses) and on failure put the Image into an explicit
   invalid state — AV_PIX_FMT_NONE plus zeroed colours/subpixelorder
   — instead of silently keeping stale fields.

Tests still green (76 cases, 778 assertions).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

ZM_COLOUR_YUV420P collides with ZM_COLOUR_GRAY8, causing V4L2 cameras to decode as grayscale

2 participants