From 7e1532438564128568d9ceddfc27a17e2db65833 Mon Sep 17 00:00:00 2001 From: SteveGilvarry Date: Sat, 13 Jun 2026 10:19:09 +1000 Subject: [PATCH] =?UTF-8?q?fix:=20corrupted=20scaled/converted=20output=20?= =?UTF-8?q?for=20rotated=20monitors=20=E2=80=94=20SWScale=20guessed=20buff?= =?UTF-8?q?er=20alignment=20from=20width?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SWScale::Convert chose av_image_fill_arrays alignment by heuristic (width % 32 ? 1 : 32) for both buffers. Image buffers are always laid out align-32, so any width not divisible by 32 made Convert read luma rows 16 bytes short and chroma planes from packed offsets: diagonal shear plus garbage chroma. Rotating a monitor is what produces such widths (1280x720 ROTATE_270 -> 720 wide, 3840x2160 ROTATE_90 -> 2160 wide, both % 32 == 16), so every scaled view and every re-encode of a rotated monitor was corrupted while unrotated monitors (1280/2688/3840 all % 32 == 0) were untouched. The rotate/flip segfault fix exposed this: before it, rotated planar frames crashed zmc before reaching Scale. Alignment is a fact about how a buffer was laid out, not something derivable from dimensions. Convert now takes explicit in/out alignment: Image::Scale passes 32/32, the videostore encode path mirrors get_out_frame's allocation choice, libvnc passes 1 (packed VNC framebuffer) and 32 (Image WriteBuffer). Remove the unused SetDefaults/ConvertDefaults API rather than threading alignments through dead code. Tests: new Scale regression case on a 720x1280 YUV420P image (column-banded luma, uniform chroma) fails before the fix exactly as observed live (sheared rows, V plane reading 0) and passes after. Full suite 84/84. Co-Authored-By: Claude Fable 5 --- src/zm_image.cpp | 5 +- src/zm_libvnc_camera.cpp | 6 ++- src/zm_swscale.cpp | 102 ++++++++++++--------------------------- src/zm_swscale.h | 22 ++++----- src/zm_videostore.cpp | 7 ++- tests/zm_image.cpp | 47 ++++++++++++++++++ 6 files changed, 102 insertions(+), 87 deletions(-) diff --git a/src/zm_image.cpp b/src/zm_image.cpp index 3a5a7218f2f..66964c97c45 100644 --- a/src/zm_image.cpp +++ b/src/zm_image.cpp @@ -3359,8 +3359,11 @@ void Image::Scale(const unsigned int new_width, const unsigned int new_height) { SWScale swscale; swscale.init(); + // Both buffers use Image's align-32 layout: `buffer` is ours, and + // scale_buffer is adopted by AssignDirect below, which derives + // size/linesize with av_image_* at align=32. if (swscale.Convert(buffer, allocation, scale_buffer, scale_buffer_size, - format, format, width, height, new_width, new_height) < 0) { + format, format, width, height, new_width, new_height, 32, 32) < 0) { Error("Scale: sws_scale conversion failed (%ux%u %s -> %ux%u)", width, height, av_get_pix_fmt_name(format), new_width, new_height); DumpBuffer(scale_buffer, ZM_BUFTYPE_ZM); diff --git a/src/zm_libvnc_camera.cpp b/src/zm_libvnc_camera.cpp index 876ebd6db7a..4ffec93ec30 100644 --- a/src/zm_libvnc_camera.cpp +++ b/src/zm_libvnc_camera.cpp @@ -235,10 +235,11 @@ int VncCamera::Capture(std::shared_ptr &zm_packet) { width, height); + // The VNC framebuffer is packed rows (alignment 1); directbuffer is an + // Image buffer (WriteBuffer), which is always align-32. int rc = scale.Convert( mVncData.buffer, mRfb->si.framebufferWidth * mRfb->si.framebufferHeight * 4, - //SWScale::GetBufferSize(AV_PIX_FMT_RGBA, mRfb->si.framebufferWidth, mRfb->si.framebufferHeight), directbuffer, width * height * colours, AV_PIX_FMT_RGBA, @@ -246,7 +247,8 @@ int VncCamera::Capture(std::shared_ptr &zm_packet) { mRfb->si.framebufferWidth, mRfb->si.framebufferHeight, width, - height); + height, + 1, 32); return rc == 0 ? 1 : rc; } diff --git a/src/zm_swscale.cpp b/src/zm_swscale.cpp index 24303362cc0..e0f74c10233 100644 --- a/src/zm_swscale.cpp +++ b/src/zm_swscale.cpp @@ -23,10 +23,7 @@ #include "zm_logger.h" SWScale::SWScale() : - gotdefaults(false), - swscale_ctx(nullptr), - default_width(0), - default_height(0) { + swscale_ctx(nullptr) { Debug(4, "SWScale object created"); } @@ -56,23 +53,6 @@ SWScale::~SWScale() { Debug(4, "SWScale object destroyed"); } -int SWScale::SetDefaults( - enum _AVPIXELFORMAT in_pf, - enum _AVPIXELFORMAT out_pf, - unsigned int width, - unsigned int height) { - - /* Assign the defaults */ - default_input_pf = in_pf; - default_output_pf = out_pf; - default_width = width; - default_height = height; - - gotdefaults = true; - - return 0; -} - int SWScale::Convert( AVFrame *in_frame, AVFrame *out_frame @@ -109,11 +89,13 @@ int SWScale::Convert( unsigned int width, unsigned int height, unsigned int new_width, - unsigned int new_height + unsigned int new_height, + int in_alignment, + int out_alignment ) { - Debug(1, "Convert: in_buffer %p in_buffer_size %zu out_buffer %p size %zu width %d height %d width %d height %d %d %d", + Debug(1, "Convert: in_buffer %p in_buffer_size %zu out_buffer %p size %zu width %d height %d width %d height %d %d %d align %d/%d", in_buffer, in_buffer_size, out_buffer, out_buffer_size, width, height, new_width, new_height, - in_pf, out_pf); + in_pf, out_pf, in_alignment, out_alignment); /* Parameter checking */ if (in_buffer == nullptr) { Error("NULL Input buffer"); @@ -123,14 +105,14 @@ int SWScale::Convert( Error("NULL output buffer"); return -1; } - // if(in_pf == 0 || out_pf == 0) { - // Error("Invalid input or output pixel formats"); - // return -2; - // } if (!width || !height || !new_height || !new_width) { Error("Invalid width or height"); return -3; } + if (in_alignment <= 0 || out_alignment <= 0) { + Error("Invalid buffer alignment %d/%d", in_alignment, out_alignment); + return -4; + } in_pf = fix_deprecated_pix_fmt(in_pf); @@ -144,19 +126,19 @@ int SWScale::Convert( (out_pf)&0xff,((out_pf>>8)&0xff),((out_pf>>16)&0xff),((out_pf>>24)&0xff)); } - int alignment = width % 32 ? 1 : 32; - /* Check the buffer sizes */ - size_t needed_insize = GetBufferSize(in_pf, width, height); + /* Check the buffer sizes against the layout each alignment implies */ + size_t needed_insize = GetBufferSize(in_pf, width, height, in_alignment); if (needed_insize > in_buffer_size) { Warning( - "The input buffer size does not match the expected size for the input format. Required: %zu for %dx%d %d Available: %zu", + "The input buffer size does not match the expected size for the input format. Required: %zu for %dx%d %d align %d Available: %zu", needed_insize, width, height, in_pf, + in_alignment, in_buffer_size); } - size_t needed_outsize = GetBufferSize(out_pf, new_width, new_height); + size_t needed_outsize = GetBufferSize(out_pf, new_width, new_height, out_alignment); if (needed_outsize > out_buffer_size) { Error("The output buffer is undersized for the output format. Required: %zu Available: %zu", needed_outsize, @@ -174,22 +156,18 @@ int SWScale::Convert( return -6; } - /* - input_avframe->format = in_pf; - input_avframe->width = width; - input_avframe->height = height; - output_avframe->format = out_pf; - output_avframe->width = new_width; - output_avframe->height = new_height; - */ - /* Fill in the buffers */ + /* Fill in the buffers. The alignments describe how the caller's buffers + * are actually laid out — they are facts about the buffers, not tuning + * knobs. Guessing them from the dimensions (the old `width % 32 ? 1 : 32` + * heuristic) misread every align-32 Image whose width was not a multiple + * of 32, e.g. the 720- or 2160-wide output of a rotated monitor. */ if (av_image_fill_arrays(input_avframe->data, input_avframe->linesize, - (uint8_t*) in_buffer, in_pf, width, height, alignment) <= 0) { + (uint8_t*) in_buffer, in_pf, width, height, in_alignment) <= 0) { Error("Failed filling input frame with input buffer"); return -7; } if (av_image_fill_arrays(output_avframe->data, output_avframe->linesize, - out_buffer, out_pf, new_width, new_height, alignment) <= 0) { + out_buffer, out_pf, new_width, new_height, out_alignment) <= 0) { Error("Failed filling output frame with output buffer"); return -8; } @@ -214,8 +192,10 @@ int SWScale::Convert( enum _AVPIXELFORMAT in_pf, enum _AVPIXELFORMAT out_pf, unsigned int width, - unsigned int height) { - return Convert(in_buffer, in_buffer_size, out_buffer, out_buffer_size, in_pf, out_pf, width, height, width, height); + unsigned int height, + int in_alignment, + int out_alignment) { + return Convert(in_buffer, in_buffer_size, out_buffer, out_buffer_size, in_pf, out_pf, width, height, width, height, in_alignment, out_alignment); } int SWScale::Convert( @@ -225,7 +205,8 @@ int SWScale::Convert( enum _AVPIXELFORMAT in_pf, enum _AVPIXELFORMAT out_pf, unsigned int width, - unsigned int height) { + unsigned int height, + int out_alignment) { if ( img->Width() != width ) { Error("Source image width differs. Source: %d Output: %d", img->Width(), width); return -12; @@ -236,29 +217,10 @@ int SWScale::Convert( return -13; } - return Convert(img->Buffer(), img->Size(), out_buffer, out_buffer_size, in_pf, out_pf, width, height); -} - -int SWScale::ConvertDefaults(const Image* img, uint8_t* out_buffer, const size_t out_buffer_size) { - - if ( !gotdefaults ) { - Error("Defaults are not set"); - return -24; - } - - return Convert(img,out_buffer,out_buffer_size,default_input_pf,default_output_pf,default_width,default_height); -} - -int SWScale::ConvertDefaults(const uint8_t* in_buffer, const size_t in_buffer_size, uint8_t* out_buffer, const size_t out_buffer_size) { - - if ( !gotdefaults ) { - Error("Defaults are not set"); - return -24; - } - - return Convert(in_buffer,in_buffer_size,out_buffer,out_buffer_size,default_input_pf,default_output_pf,default_width,default_height); + // Image buffers are always laid out by av_image_* with align=32 + return Convert(img->Buffer(), img->Size(), out_buffer, out_buffer_size, in_pf, out_pf, width, height, 32, out_alignment); } -size_t SWScale::GetBufferSize(enum _AVPIXELFORMAT pf, unsigned int width, unsigned int height) { - return av_image_get_buffer_size(pf, width, height, 1); +size_t SWScale::GetBufferSize(enum _AVPIXELFORMAT pf, unsigned int width, unsigned int height, int alignment) { + return av_image_get_buffer_size(pf, width, height, alignment); } diff --git a/src/zm_swscale.h b/src/zm_swscale.h index 74ab8951d60..bf74f258eb7 100644 --- a/src/zm_swscale.h +++ b/src/zm_swscale.h @@ -12,24 +12,20 @@ class SWScale { SWScale(); ~SWScale(); bool init(); - int SetDefaults(enum _AVPIXELFORMAT in_pf, enum _AVPIXELFORMAT out_pf, unsigned int width, unsigned int height); - int ConvertDefaults(const Image* img, uint8_t* out_buffer, const size_t out_buffer_size); - int ConvertDefaults(const uint8_t* in_buffer, const size_t in_buffer_size, uint8_t* out_buffer, const size_t out_buffer_size); - int Convert( AVFrame *in_frame, AVFrame *out_frame ); - int Convert(const Image* img, uint8_t* out_buffer, const size_t out_buffer_size, enum _AVPIXELFORMAT in_pf, enum _AVPIXELFORMAT out_pf, unsigned int width, unsigned int height); - int Convert(const uint8_t* in_buffer, const size_t in_buffer_size, uint8_t* out_buffer, const size_t out_buffer_size, enum _AVPIXELFORMAT in_pf, enum _AVPIXELFORMAT out_pf, unsigned int width, unsigned int height); - int Convert(const uint8_t* in_buffer, const size_t in_buffer_size, uint8_t* out_buffer, const size_t out_buffer_size, enum _AVPIXELFORMAT in_pf, enum _AVPIXELFORMAT out_pf, unsigned int width, unsigned int height, unsigned int new_width, unsigned int new_height); - static size_t GetBufferSize(enum _AVPIXELFORMAT in_pf, unsigned int width, unsigned int height); + int Convert(AVFrame *in_frame, AVFrame *out_frame); + // A buffer's row alignment cannot be derived from its dimensions — the + // caller must state how each buffer is laid out. ZM Image buffers are + // always align-32 (av_image_* with align=32); raw device buffers (V4L2, + // VNC framebuffers) are packed rows (alignment 1). + int Convert(const Image* img, uint8_t* out_buffer, const size_t out_buffer_size, enum _AVPIXELFORMAT in_pf, enum _AVPIXELFORMAT out_pf, unsigned int width, unsigned int height, int out_alignment); + int Convert(const uint8_t* in_buffer, const size_t in_buffer_size, uint8_t* out_buffer, const size_t out_buffer_size, enum _AVPIXELFORMAT in_pf, enum _AVPIXELFORMAT out_pf, unsigned int width, unsigned int height, int in_alignment, int out_alignment); + int Convert(const uint8_t* in_buffer, const size_t in_buffer_size, uint8_t* out_buffer, const size_t out_buffer_size, enum _AVPIXELFORMAT in_pf, enum _AVPIXELFORMAT out_pf, unsigned int width, unsigned int height, unsigned int new_width, unsigned int new_height, int in_alignment, int out_alignment); + static size_t GetBufferSize(enum _AVPIXELFORMAT in_pf, unsigned int width, unsigned int height, int alignment); protected: - bool gotdefaults; struct SwsContext* swscale_ctx; av_frame_ptr input_avframe; av_frame_ptr output_avframe; - enum _AVPIXELFORMAT default_input_pf; - enum _AVPIXELFORMAT default_output_pf; - unsigned int default_width; - unsigned int default_height; }; #endif // ZM_SWSCALE_H diff --git a/src/zm_videostore.cpp b/src/zm_videostore.cpp index 4b4c239af50..e5435312de9 100644 --- a/src/zm_videostore.cpp +++ b/src/zm_videostore.cpp @@ -1153,6 +1153,10 @@ int VideoStore::writeVideoFramePacket(const std::shared_ptr zm_packet) zm_packet->get_out_frame(video_out_ctx->width, video_out_ctx->height, chosen_codec_data->sw_pix_fmt); av_frame_ref(frame.get(), zm_packet->out_frame.get()); + // The destination is out_frame's buffer, which get_out_frame laid + // out at alignment (width % 32 ? 1 : 32) — mirror that choice here + // so sws writes the layout the encoder will read via + // out_frame->linesize. swscale.Convert( zm_packet->image, frame->buf[0]->data, @@ -1160,7 +1164,8 @@ int VideoStore::writeVideoFramePacket(const std::shared_ptr zm_packet) zm_packet->image->AVPixFormat(), chosen_codec_data->sw_pix_fmt, video_out_ctx->width, - video_out_ctx->height + video_out_ctx->height, + (video_out_ctx->width % 32) ? 1 : 32 ); } } else if (zm_packet->in_frame) { diff --git a/tests/zm_image.cpp b/tests/zm_image.cpp index 3251628dd02..4405c5e4981 100644 --- a/tests/zm_image.cpp +++ b/tests/zm_image.cpp @@ -25,6 +25,7 @@ extern "C" { #include } +#include #include namespace { @@ -134,6 +135,52 @@ TEST_CASE("Image::Rotate YUV420P odd dimensions", "[image]") { REQUIRE(dst.data[1][(cw - 1) * dst.stride[1] + 0] == 211); } +// Regression: SWScale::Convert guessed each buffer's row alignment from +// `width % 32 ? 1 : 32`. Image buffers are ALWAYS laid out align-32 +// (av_image_fill_arrays/av_image_get_buffer_size with align=32), so for any +// width not divisible by 32 the converter read luma rows 16 bytes short and +// chroma planes from packed (wrong) offsets. Rotating a 1280x720 monitor +// yields exactly such a width (720 % 32 == 16): every Scale() of a rotated +// frame came out sheared with garbage chroma, while unrotated monitors +// (width % 32 == 0) were untouched. +TEST_CASE("Image::Scale YUV420P with non-32-multiple width", "[image]") { + bootstrap_image_config(); + const int w = 720, h = 1280; // rotated 1280x720, as ROTATE_90/270 produces + Image image(w, h, ZM_COLOUR_GRAY8, ZM_SUBPIX_ORDER_YUV420P); + REQUIRE(image.Buffer() != nullptr); + + // Column-banded luma (every row identical: left half 50, right half 200) + // and uniform chroma. Any per-row drift or plane-offset error shows up as + // rows that differ from each other or chroma that isn't the fill value. + Planes src = plane_view(image, AV_PIX_FMT_YUV420P, w, h); + for (int y = 0; y < h; y++) { + uint8_t *row = src.data[0] + static_cast(y) * src.stride[0]; + memset(row, 50, w / 2); + memset(row + w / 2, 200, w - w / 2); + } + memset(src.data[1], 100, static_cast(src.stride[1]) * (h / 2)); + memset(src.data[2], 160, static_cast(src.stride[2]) * (h / 2)); + + image.Scale(w / 2, h / 2); + REQUIRE(image.Width() == static_cast(w / 2)); + REQUIRE(image.Height() == static_cast(h / 2)); + + Planes dst = plane_view(image, AV_PIX_FMT_YUV420P, w / 2, h / 2); + // Identical input rows must produce identical output rows. Sample the + // middle of each band, away from the edge where bilinear blends. + for (int y : {0, h / 8, h / 4, h / 2 - 1}) { + const uint8_t *row = dst.data[0] + static_cast(y) * dst.stride[0]; + INFO("output luma row " << y); + CHECK(std::abs(static_cast(row[w / 8]) - 50) <= 4); + CHECK(std::abs(static_cast(row[w / 4 + w / 8]) - 200) <= 4); + } + for (int y : {0, h / 8, h / 4 - 1}) { + INFO("output chroma row " << y); + CHECK(std::abs(static_cast(dst.data[1][static_cast(y) * dst.stride[1] + w / 8]) - 100) <= 4); + CHECK(std::abs(static_cast(dst.data[2][static_cast(y) * dst.stride[2] + w / 8]) - 160) <= 4); + } +} + TEST_CASE("Image::Flip YUV420P", "[image]") { bootstrap_image_config(); const int w = 640, h = 480;