From 1fe29202f552aa5d412fe2b114c55d00577f0baa Mon Sep 17 00:00:00 2001 From: Isaac Connor Date: Fri, 22 May 2026 19:35:50 -0400 Subject: [PATCH] fix: address PR #4788 review comments on AVPixelFormat migration - 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) --- src/zm_camera.cpp | 24 ++++++++++++++++++++++-- src/zm_image.cpp | 10 +++++++--- src/zm_monitor.cpp | 31 ++++++++++++++++++++----------- 3 files changed, 49 insertions(+), 16 deletions(-) diff --git a/src/zm_camera.cpp b/src/zm_camera.cpp index 7c3ee460b..c5e55cee2 100644 --- a/src/zm_camera.cpp +++ b/src/zm_camera.cpp @@ -64,9 +64,29 @@ Camera::Camera( mLastAudioDTS(AV_NOPTS_VALUE), bytes(0), mIsPrimed(false) { - linesize = FFALIGN(av_image_get_linesize(pixelFormat, width, 0), 32); + // Guard against an unknown (colours, subpixelorder) pair that produced + // pixelFormat == AV_PIX_FMT_NONE, or any case where av_image_get_* + // returns a negative size — assigning those to unsigned would wrap to a + // huge value and break SHM sizing and downstream allocations. pixels = width * height; - imagesize = av_image_get_buffer_size(pixelFormat, width, height, 32); + if (pixelFormat == AV_PIX_FMT_NONE) { + Error("Camera: unknown pixel format from colours=%u subpixelorder=%u; falling back to width*colours stride", + p_colours, p_subpixelorder); + linesize = width * colours; + imagesize = static_cast(height) * linesize; + } else { + int raw_linesize = av_image_get_linesize(pixelFormat, width, 0); + int raw_imagesize = av_image_get_buffer_size(pixelFormat, width, height, 32); + if (raw_linesize < 0 || raw_imagesize < 0) { + Error("Camera: av_image_get_* returned %d/%d for pixelFormat=%s; falling back", + raw_linesize, raw_imagesize, av_get_pix_fmt_name(pixelFormat)); + linesize = width * colours; + imagesize = static_cast(height) * linesize; + } else { + linesize = FFALIGN(raw_linesize, 32); + imagesize = static_cast(raw_imagesize); + } + } Debug(2, "New camera id: %d width: %d line size: %d height: %d colours: %d subpixelorder: %d capture: %d, size: %llu", monitor->Id(), width, linesize, height, colours, subpixelorder, capture, imagesize); diff --git a/src/zm_image.cpp b/src/zm_image.cpp index 8a7e8fd18..0511ff075 100644 --- a/src/zm_image.cpp +++ b/src/zm_image.cpp @@ -1003,15 +1003,19 @@ void Image::Assign(const Image &image) { if ( image.buffer != buffer ) { if (image.linesize > linesize) { + // Source has more per-row padding than the destination can hold. Copy + // only the destination's row capacity per line to avoid writing past + // the destination buffer on the last row. Source padding bytes beyond + // the common row width are discarded. // This branch is only reached when dimensions and colours/subpixelorder - // match but linesize disagrees, which is an oddly-shaped Image. Copy the - // Y/primary plane line by line; planar chroma planes are not handled + // match but linesize disagrees, which is an oddly-shaped Image. Only + // the Y/primary plane is copied; planar chroma planes are not handled // here. The common path is the flat copy below. Debug(1, "Must copy line by line due to different line size %d != %d", image.linesize, linesize); uint8_t *src_ptr = image.buffer; uint8_t *dst_ptr = buffer; for (unsigned int i=0; i< image.height; i++) { - (*fptr_imgbufcpy)(dst_ptr, src_ptr, image.linesize); + (*fptr_imgbufcpy)(dst_ptr, src_ptr, linesize); src_ptr += image.linesize; dst_ptr += linesize; } diff --git a/src/zm_monitor.cpp b/src/zm_monitor.cpp index ed4d20515..7cf3b550b 100644 --- a/src/zm_monitor.cpp +++ b/src/zm_monitor.cpp @@ -1240,11 +1240,14 @@ bool Monitor::connect() { Error("Shared data not initialised by capture daemon for monitor %s", name.c_str()); return false; } - // No zms-side per-slot pixformat adoption needed: image_buffer was - // constructed above with the same hardcoded YUV420P that zmc uses, so - // both processes interpret the SHM bytes the same way without consulting - // the image_pixelformats[] array (which still records the actual format - // zmc wrote, kept for diagnostic visibility). + // image_buffer[] is constructed above with a hardcoded YUV420P placeholder + // format because we don't yet know which AVPixelFormat zmc has written into + // each SHM slot. Readers (zms, zma, etc.) MUST call ReadShmFrame() before + // interpreting a slot's bytes — it adopts the per-slot AVPixelFormat from + // image_pixelformats[] (written by zmc in WriteShmFrame) and updates the + // Image's imagePixFormat, colours, subpixelorder, size and linesize + // together. Direct image_buffer[i] access without ReadShmFrame() will + // mis-interpret any slot whose actual format differs from the placeholder. // We set these here because otherwise the first fps calc is meaningless last_fps_time = std::chrono::system_clock::now(); @@ -1787,7 +1790,9 @@ bool Monitor::CheckSignal(const Image *image) { } } - if (pix_fmt == AV_PIX_FMT_GRAY8 || zm_is_yuv420(pix_fmt)) { + if (zm_bytes_per_pixel(pix_fmt) == 1) { + // GRAY8 plus all planar YUV variants we transport (420P/J420P/422P/J422P). + // For planar formats the Y plane is the first byte at each pixel index. if (*(buffer+index) != grayscale_val) return true; @@ -2860,12 +2865,16 @@ bool Monitor::setupConvertContext(const AVFrame *input_frame, const Image *image void Monitor::WriteShmFrame(unsigned int index, Image *capture_image) { // No conversion at the SHM-write side. zmc records the format the bytes // were actually written in via image_pixelformats[index]; consumers in - // other processes (zms etc.) sync image_buffer[index]->AVPixFormat() from - // that array before reading via ReadShmFrame, so the SHM transports any - // format Image can represent without a sws_scale step. This is the - // central no-conversion promise of the AVPixelFormat migration. + // other processes (zms etc.) sync image_buffer[index] from that array + // before reading via ReadShmFrame, so the SHM transports any format + // Image can represent without a sws_scale step. This is the central + // no-conversion promise of the AVPixelFormat migration. + // + // Record imagePixFormat directly via PixFormat() — AVPixFormat() re-derives + // from the deprecated (colours, subpixelorder) pair and could propagate + // stale metadata. image_buffer[index]->Assign(*capture_image); - image_pixelformats[index] = capture_image->AVPixFormat(); + image_pixelformats[index] = capture_image->PixFormat(); } Image *Monitor::ReadShmFrame(unsigned int index) {