From ede50bebe77e373b9e28d9caa3db9ae3f7e3ba88 Mon Sep 17 00:00:00 2001 From: "wangyang (wysaid)" Date: Sat, 13 Dec 2025 11:49:53 +0800 Subject: [PATCH 1/7] fix: RGB24<->BGR24 3->3 channel conversion crash in AVX2 Issue Description: - When patchSize=10, the shuffle mask index exceeds the valid range (0-15) of _mm_shuffle_epi8. - At the 9th pixel, shuffleData[27] is accessed out of bounds, causing a program crash. Solution: 1. Change the patchSize for 3->3 channel conversion from 10 to 5. 2. Simplify the processing logic: - Before: Read twice (15+15 bytes), shuffle twice, complex memcpy concatenation. - Now: Read once (16 bytes), shuffle once, write directly. 3. Ensure the maximum shuffle mask index is 14 (5 pixels * 3 channels - 1), within the safe range. Technical Details: - _mm_shuffle_epi8 can only rearrange within a single 16-byte lane, index must be <= 15. - Now each operation processes 5 RGB pixels (15 bytes), reads/writes 16 bytes (SSE aligned). - The last 1 byte will be overwritten by subsequent operations, so correctness is not affected. Performance Impact: Slightly reduced (number of processed pixels from 10->5), but stability is ensured. Related Issue: #30 --- src/ccap_convert_avx2.cpp | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/ccap_convert_avx2.cpp b/src/ccap_convert_avx2.cpp index 24ab45b7..a7c19876 100644 --- a/src/ccap_convert_avx2.cpp +++ b/src/ccap_convert_avx2.cpp @@ -133,8 +133,8 @@ AVX2_TARGET void colorShuffle_avx2(const uint8_t* src, int srcStride, uint8_t* d } alignas(32) uint8_t shuffleData[32]; - constexpr uint32_t inputPatchSize = inputChannels == 4 ? 8 : 10; - constexpr uint32_t outputPatchSize = outputChannels == 4 ? 8 : 10; + constexpr uint32_t inputPatchSize = inputChannels == 4 ? 8 : (inputChannels == 3 && outputChannels == 3 ? 5 : 10); + constexpr uint32_t outputPatchSize = outputChannels == 4 ? 8 : (inputChannels == 3 && outputChannels == 3 ? 5 : 10); constexpr uint32_t patchSize = inputPatchSize < outputPatchSize ? inputPatchSize : outputPatchSize; for (int i = 0; i < patchSize; ++i) { @@ -218,17 +218,12 @@ AVX2_TARGET void colorShuffle_avx2(const uint8_t* src, int srcStride, uint8_t* d _mm_store_si128((__m128i*)remainBuffer, result_hi); // Temporarily store, 16 bytes memcpy(dstRow + x * outputChannels + 12, remainBuffer, 12); // Manual alignment, overwrite extra 4 bytes, fill remaining 12 bytes, exactly 24 bytes } else if constexpr (inputChannels == 3 && outputChannels == 3) { // 3 -> 3 - /// Split into 15 + 15, reading 30 bytes each time - __m128i pixels_lo = _mm_loadu_si128((__m128i*)(srcRow + x * inputChannels)); - __m128i pixels_hi = _mm_loadu_si128((__m128i*)(srcRow + x * inputChannels + 15)); + /// Process 5 pixels at a time (15 bytes), reading 16 bytes each time + __m128i pixels = _mm_loadu_si128((__m128i*)(srcRow + x * inputChannels)); - __m128i result_lo = _mm_shuffle_epi8(pixels_lo, shuffle128); // Only the first 15 bytes are useful - __m128i result_hi = _mm_shuffle_epi8(pixels_hi, shuffle128); // Only the first 15 bytes are useful + __m128i result = _mm_shuffle_epi8(pixels, shuffle128); // Only the first 15 bytes are useful - _mm_storeu_si128((__m128i*)(dstRow + x * outputChannels), result_lo); // Write 16 bytes, but only the first 15 bytes are useful - alignas(16) uint8_t remainBuffer[16]; - _mm_store_si128((__m128i*)remainBuffer, result_hi); // Temporarily store, 15 bytes - memcpy(dstRow + x * outputChannels + 15, remainBuffer, 15); // Manual alignment, overwrite extra 1 byte, fill remaining 15 bytes, exactly 30 bytes + _mm_storeu_si128((__m128i*)(dstRow + x * outputChannels), result); // Write 16 bytes, but only the first 15 bytes are useful } else { // 4 -> 4 __m256i pixels = _mm256_loadu_si256((const __m256i*)(srcRow + x * inputChannels)); __m256i result = _mm256_shuffle_epi8(pixels, shuffle256); From c360a97be49fcdb06521bf46ce5efde776bfd21c Mon Sep 17 00:00:00 2001 From: "wangyang (wysaid)" Date: Sat, 13 Dec 2025 12:07:10 +0800 Subject: [PATCH 2/7] fix: correct RGB24<->BGR24 function call and add design constraint validation Problem: 1. RGB24<->BGR24 conversion incorrectly used rgbaToBgra() instead of rgbToBgr() 2. Missing validation for critical design constraint: frame->data[0] must point to external memory (camera buffer) before conversion Fix: 1. Changed line 172: rgbaToBgra() -> rgbToBgr() for 3-channel RGB<->BGR conversion 2. Added assertions in all three inplaceConvertFrame*() functions to validate: - frame->allocator == nullptr OR - frame->data[0] != frame->allocator->data() 3. Enhanced header documentation explaining the design constraints: - Each VideoFrame should only be converted ONCE - Input data must be in external memory initially - After conversion, frame->data[0] points to allocator memory Technical details: - The assertion prevents use-after-free bugs by catching design violations early - Zero-copy optimization relies on this constraint: no memcpy needed since external memory remains valid during allocator->resize() - Assertions provide clear error messages for debugging Related: #30 --- src/ccap_convert_frame.cpp | 20 +++++++++++++++++++- src/ccap_convert_frame.h | 19 +++++++++++++++++-- 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/src/ccap_convert_frame.cpp b/src/ccap_convert_frame.cpp index d15cf388..8888fc18 100644 --- a/src/ccap_convert_frame.cpp +++ b/src/ccap_convert_frame.cpp @@ -19,6 +19,12 @@ bool inplaceConvertFrameYUV2RGBColor(VideoFrame* frame, PixelFormat toFormat, bo /// TODO: Fix toFormat here, only support YUV -> (BGR24/BGRA32). Simplify SDK design. Will improve later. + // ASSERTION: Ensure frame->data[0] points to EXTERNAL memory, not allocator->data() + // This validates the design constraint: VideoFrame should only be converted once + assert(frame->allocator == nullptr || frame->data[0] != frame->allocator->data() && + "DESIGN VIOLATION: frame->data[0] must point to external memory (e.g., camera buffer), not allocator memory. " + "Each VideoFrame should only be converted ONCE using inplaceConvertFrame*() functions."); + auto inputFormat = frame->pixelFormat; assert((inputFormat & kPixelFormatYUVColorBit) != 0 && (toFormat & kPixelFormatYUVColorBit) == 0); bool isInputNV12 = pixelFormatInclude(inputFormat, PixelFormat::NV12); @@ -122,6 +128,12 @@ bool inplaceConvertFrameYUV2RGBColor(VideoFrame* frame, PixelFormat toFormat, bo bool inplaceConvertFrameRGB(VideoFrame* frame, PixelFormat toFormat, bool verticalFlip) { // RGB(A) interconversion + + // ASSERTION: Ensure frame->data[0] points to EXTERNAL memory, not allocator->data() + // This validates the design constraint: VideoFrame should only be converted once + assert(frame->allocator == nullptr || frame->data[0] != frame->allocator->data() && + "DESIGN VIOLATION: frame->data[0] must point to external memory (e.g., camera buffer), not allocator memory. " + "Each VideoFrame should only be converted ONCE using inplaceConvertFrame*() functions."); uint8_t* inputBytes = frame->data[0]; int inputLineSize = frame->stride[0]; @@ -157,7 +169,7 @@ bool inplaceConvertFrameRGB(VideoFrame* frame, PixelFormat toFormat, bool vertic #endif } else // RGB <-> BGR { - rgbaToBgra(inputBytes, inputLineSize, outputBytes, newLineSize, frame->width, height); + rgbToBgr(inputBytes, inputLineSize, outputBytes, newLineSize, frame->width, height); } } else /// Different number of channels, only 4 channels <-> 3 channels { @@ -181,6 +193,12 @@ bool inplaceConvertFrameRGB(VideoFrame* frame, PixelFormat toFormat, bool vertic } inline bool inplaceConvertFrameImp(VideoFrame* frame, PixelFormat toFormat, bool verticalFlip) { + // ASSERTION: Ensure frame->data[0] points to EXTERNAL memory, not allocator->data() + // This validates the design constraint: VideoFrame should only be converted once + assert(frame->allocator == nullptr || frame->data[0] != frame->allocator->data() && + "DESIGN VIOLATION: frame->data[0] must point to external memory (e.g., camera buffer), not allocator memory. " + "Each VideoFrame should only be converted ONCE using inplaceConvertFrame*() functions."); + if (frame->pixelFormat == toFormat) { if (verticalFlip && (toFormat & kPixelFormatRGBColorBit)) { // flip upside down int srcStride = (int)frame->stride[0]; diff --git a/src/ccap_convert_frame.h b/src/ccap_convert_frame.h index aaf1744d..fa991b10 100644 --- a/src/ccap_convert_frame.h +++ b/src/ccap_convert_frame.h @@ -12,8 +12,23 @@ #include "ccap_def.h" -/// The methods here require that the data field of frame is not allocated with an allocator. -/// This method will use an allocator to allocate memory and convert to a new data format. +/// @brief Inplace frame conversion functions +/// +/// IMPORTANT CONSTRAINTS: +/// - These methods require that frame->data[0] points to EXTERNAL memory (e.g., camera buffer) +/// and NOT to frame->allocator->data() +/// - Each VideoFrame should only be converted ONCE using these functions +/// - The functions will allocate new memory via frame->allocator and update frame->data[0] +/// +/// TYPICAL USAGE: +/// 1. Capture frame from camera: frame->data[0] points to camera's buffer +/// 2. Call inplaceConvertFrame*() ONCE: converts and moves data to allocator +/// 3. After conversion: frame->data[0] == frame->allocator->data() +/// +/// VIOLATION will cause: +/// - Data corruption (reading freed memory) +/// - Assertion failure in debug builds +/// - Undefined behavior namespace ccap { From f912d7bb6c8b92391663b69fd5ceeaf63cfd6f03 Mon Sep 17 00:00:00 2001 From: "wangyang (wysaid)" Date: Sat, 13 Dec 2025 12:09:37 +0800 Subject: [PATCH 3/7] test: add comprehensive frame conversion unit tests Added 18 unit tests covering VideoFrame conversion functions (inplaceConvertFrame*): Test coverage: 1. Basic RGB/BGR conversions (RGB24<->BGR24, RGBA32<->BGRA32) 2. Cross-channel conversions (RGB24->BGRA32, BGRA32->RGB24, etc.) 3. Large frame stress tests (1280x720, 1920x1080) 4. Edge cases (odd widths, non-multiples of SIMD patch sizes) Test features: - Tests both CPU and AVX2 backends separately - Validates pixel-level correctness using various test patterns (gradient, random, checker) - Performance checks for large frames (should complete < 500ms for 720p, < 1000ms for 1080p) - Verifies AVX2 patchSize fix with full row scans - Tests alignment edge cases that previously caused crashes Implementation notes: - createTestFrame() properly simulates external memory (camera buffer) scenario - Complies with design constraint: frame->data[0] points to external memory initially - Uses static external_buffers vector to maintain memory validity during tests All 18 tests passing (CPU and AVX2 backends). Related: #30 --- tests/CMakeLists.txt | 8 + tests/test_frame_conversions.cpp | 474 +++++++++++++++++++++++++++++++ 2 files changed, 482 insertions(+) create mode 100644 tests/test_frame_conversions.cpp diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 6a95bb22..ed71a096 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -72,6 +72,7 @@ add_executable( test_color_conversions.cpp test_yuv_conversions.cpp test_platform_features.cpp + test_frame_conversions.cpp ) target_link_libraries( @@ -80,6 +81,13 @@ target_link_libraries( gtest_main ) +# Add src directory to include path for internal API testing +target_include_directories( + ccap_convert_test + PRIVATE + ${CMAKE_SOURCE_DIR}/src +) + # GUID definition verification test (Windows only) # This test verifies that locally defined GUIDs match strmiids.lib values if(WIN32) diff --git a/tests/test_frame_conversions.cpp b/tests/test_frame_conversions.cpp new file mode 100644 index 00000000..a5ade242 --- /dev/null +++ b/tests/test_frame_conversions.cpp @@ -0,0 +1,474 @@ +/** + * @file test_frame_conversions.cpp + * @brief Tests for VideoFrame conversion functions (inplaceConvertFrame and related APIs) + * @note These tests cover the high-level frame conversion API that uses PixelFormat enums + */ + +#include "ccap_convert_frame.h" +#include "ccap_core.h" +#include "test_utils.h" +#include "test_backend_manager.h" +#include +#include + +using namespace ccap_test; + +// Helper function to create a VideoFrame for testing +// Returns a frame with data pointing to EXTERNAL memory (simulating camera buffer) +// This matches the design constraint for inplaceConvertFrame*() functions +std::unique_ptr createTestFrame(int width, int height, ccap::PixelFormat pixelFormat) { + auto frame = std::make_unique(); + frame->width = width; + frame->height = height; + frame->pixelFormat = pixelFormat; + frame->orientation = ccap::FrameOrientation::TopToBottom; + + int channels = 3; + if (pixelFormat == ccap::PixelFormat::RGBA32 || pixelFormat == ccap::PixelFormat::BGRA32) { + channels = 4; + } + + frame->stride[0] = width * channels; + + // Create allocator but DON'T allocate memory yet + // This simulates the real-world scenario where frame->data[0] points to external memory + frame->allocator = std::make_shared(); + + // Allocate external buffer (simulating camera buffer) + // In real usage, this would be the camera's buffer + static std::vector> external_buffers; + external_buffers.push_back(std::vector(frame->stride[0] * height, 0xDD)); + frame->data[0] = external_buffers.back().data(); + + return frame; +} + +// ============ Frame RGB/BGR Conversion Tests ============ + +class FrameRGBBGRConversionTest : public BackendParameterizedTest { +protected: + void SetUp() override { + BackendParameterizedTest::SetUp(); + width_ = 64; + height_ = 64; + } + + int width_; + int height_; +}; + +TEST_P(FrameRGBBGRConversionTest, RGB24_To_BGR24_InplaceConversion) { + auto backend = GetParam(); + + auto frame = createTestFrame(width_, height_, ccap::PixelFormat::RGB24); + + // Fill with test pattern + TestImage test_pattern(width_, height_, 3); + test_pattern.fillGradient(); + memcpy(frame->data[0], test_pattern.data(), width_ * height_ * 3); + + // Store original data for verification + std::vector original_data(frame->data[0], frame->data[0] + width_ * height_ * 3); + + // Convert RGB24 -> BGR24 + bool success = ccap::inplaceConvertFrameRGB(frame.get(), ccap::PixelFormat::BGR24, false); + ASSERT_TRUE(success) << "RGB24 to BGR24 conversion failed, backend: " + << BackendTestManager::getBackendName(backend); + + EXPECT_EQ(frame->pixelFormat, ccap::PixelFormat::BGR24) + << "Frame pixel format not updated"; + + // Verify R and B channels are swapped + for (int y = 0; y < height_; ++y) { + for (int x = 0; x < width_; ++x) { + int offset = y * frame->stride[0] + x * 3; + EXPECT_EQ(original_data[offset + 0], frame->data[0][offset + 2]) + << "R->B swap failed at (" << x << "," << y << "), backend: " + << BackendTestManager::getBackendName(backend); + EXPECT_EQ(original_data[offset + 1], frame->data[0][offset + 1]) + << "G should remain unchanged at (" << x << "," << y << "), backend: " + << BackendTestManager::getBackendName(backend); + EXPECT_EQ(original_data[offset + 2], frame->data[0][offset + 0]) + << "B->R swap failed at (" << x << "," << y << "), backend: " + << BackendTestManager::getBackendName(backend); + } + } +} + +TEST_P(FrameRGBBGRConversionTest, BGR24_To_RGB24_InplaceConversion) { + auto backend = GetParam(); + + auto frame = createTestFrame(width_, height_, ccap::PixelFormat::BGR24); + + // Fill with test pattern + TestImage test_pattern(width_, height_, 3); + test_pattern.fillRandom(42); + memcpy(frame->data[0], test_pattern.data(), width_ * height_ * 3); + + // Store original data for verification + std::vector original_data(frame->data[0], frame->data[0] + width_ * height_ * 3); + + // Convert BGR24 -> RGB24 + bool success = ccap::inplaceConvertFrameRGB(frame.get(), ccap::PixelFormat::RGB24, false); + ASSERT_TRUE(success) << "BGR24 to RGB24 conversion failed, backend: " + << BackendTestManager::getBackendName(backend); + + EXPECT_EQ(frame->pixelFormat, ccap::PixelFormat::RGB24) + << "Frame pixel format not updated"; + + // Verify channels are swapped (should be symmetric operation) + for (int y = 0; y < height_; ++y) { + for (int x = 0; x < width_; ++x) { + int offset = y * frame->stride[0] + x * 3; + EXPECT_EQ(original_data[offset + 0], frame->data[0][offset + 2]) + << "Channel swap failed at (" << x << "," << y << "), backend: " + << BackendTestManager::getBackendName(backend); + EXPECT_EQ(original_data[offset + 1], frame->data[0][offset + 1]) + << "G should remain unchanged at (" << x << "," << y << "), backend: " + << BackendTestManager::getBackendName(backend); + EXPECT_EQ(original_data[offset + 2], frame->data[0][offset + 0]) + << "Channel swap failed at (" << x << "," << y << "), backend: " + << BackendTestManager::getBackendName(backend); + } + } +} + +TEST_P(FrameRGBBGRConversionTest, RGBA32_To_BGRA32_InplaceConversion) { + auto backend = GetParam(); + + auto frame = createTestFrame(width_, height_, ccap::PixelFormat::RGBA32); + + // Fill with test pattern + TestImage test_pattern(width_, height_, 4); + test_pattern.fillChecker(100, 200); + memcpy(frame->data[0], test_pattern.data(), width_ * height_ * 4); + + // Store original data for verification + std::vector original_data(frame->data[0], frame->data[0] + width_ * height_ * 4); + + // Convert RGBA32 -> BGRA32 + bool success = ccap::inplaceConvertFrameRGB(frame.get(), ccap::PixelFormat::BGRA32, false); + ASSERT_TRUE(success) << "RGBA32 to BGRA32 conversion failed, backend: " + << BackendTestManager::getBackendName(backend); + + EXPECT_EQ(frame->pixelFormat, ccap::PixelFormat::BGRA32) + << "Frame pixel format not updated"; + + // Verify R/B channels swapped, alpha preserved + for (int y = 0; y < height_; ++y) { + for (int x = 0; x < width_; ++x) { + int offset = y * frame->stride[0] + x * 4; + EXPECT_EQ(original_data[offset + 0], frame->data[0][offset + 2]) + << "R->B swap failed at (" << x << "," << y << ")"; + EXPECT_EQ(original_data[offset + 1], frame->data[0][offset + 1]) + << "G should remain unchanged at (" << x << "," << y << ")"; + EXPECT_EQ(original_data[offset + 2], frame->data[0][offset + 0]) + << "B->R swap failed at (" << x << "," << y << ")"; + EXPECT_EQ(original_data[offset + 3], frame->data[0][offset + 3]) + << "Alpha should remain unchanged at (" << x << "," << y << ")"; + } + } +} + +TEST_P(FrameRGBBGRConversionTest, RGB24_To_BGRA32_InplaceConversion) { + auto backend = GetParam(); + + auto frame = createTestFrame(width_, height_, ccap::PixelFormat::RGB24); + + // Fill with test pattern + TestImage test_pattern(width_, height_, 3); + test_pattern.fillGradient(); + memcpy(frame->data[0], test_pattern.data(), width_ * height_ * 3); + + // Store original data for verification + std::vector original_data(frame->data[0], frame->data[0] + width_ * height_ * 3); + + // Convert RGB24 -> BGRA32 + bool success = ccap::inplaceConvertFrameRGB(frame.get(), ccap::PixelFormat::BGRA32, false); + ASSERT_TRUE(success) << "RGB24 to BGRA32 conversion failed, backend: " + << BackendTestManager::getBackendName(backend); + + EXPECT_EQ(frame->pixelFormat, ccap::PixelFormat::BGRA32) + << "Frame pixel format not updated"; + + // Verify RGB->BGRA conversion (swap + add alpha) + for (int y = 0; y < height_; ++y) { + for (int x = 0; x < width_; ++x) { + int src_offset = y * test_pattern.stride() + x * 3; + int dst_offset = y * frame->stride[0] + x * 4; + + EXPECT_EQ(original_data[src_offset + 0], frame->data[0][dst_offset + 2]) + << "R->B conversion failed at (" << x << "," << y << ")"; + EXPECT_EQ(original_data[src_offset + 1], frame->data[0][dst_offset + 1]) + << "G should be preserved at (" << x << "," << y << ")"; + EXPECT_EQ(original_data[src_offset + 2], frame->data[0][dst_offset + 0]) + << "B->R conversion failed at (" << x << "," << y << ")"; + EXPECT_EQ(255, frame->data[0][dst_offset + 3]) + << "Alpha should be 255 at (" << x << "," << y << ")"; + } + } +} + +TEST_P(FrameRGBBGRConversionTest, BGRA32_To_RGB24_InplaceConversion) { + auto backend = GetParam(); + + auto frame = createTestFrame(width_, height_, ccap::PixelFormat::BGRA32); + + // Fill with test pattern + TestImage test_pattern(width_, height_, 4); + test_pattern.fillRandom(999); + memcpy(frame->data[0], test_pattern.data(), width_ * height_ * 4); + + // Store original data for verification + std::vector original_data(frame->data[0], frame->data[0] + width_ * height_ * 4); + + // Convert BGRA32 -> RGB24 + bool success = ccap::inplaceConvertFrameRGB(frame.get(), ccap::PixelFormat::RGB24, false); + ASSERT_TRUE(success) << "BGRA32 to RGB24 conversion failed, backend: " + << BackendTestManager::getBackendName(backend); + + EXPECT_EQ(frame->pixelFormat, ccap::PixelFormat::RGB24) + << "Frame pixel format not updated"; + + // Verify BGRA->RGB conversion (swap + remove alpha) + for (int y = 0; y < height_; ++y) { + for (int x = 0; x < width_; ++x) { + int src_offset = y * test_pattern.stride() + x * 4; + int dst_offset = y * frame->stride[0] + x * 3; + + EXPECT_EQ(original_data[src_offset + 0], frame->data[0][dst_offset + 2]) + << "B->R conversion failed at (" << x << "," << y << ")"; + EXPECT_EQ(original_data[src_offset + 1], frame->data[0][dst_offset + 1]) + << "G should be preserved at (" << x << "," << y << ")"; + EXPECT_EQ(original_data[src_offset + 2], frame->data[0][dst_offset + 0]) + << "R->B conversion failed at (" << x << "," << y << ")"; + } + } +} + +INSTANTIATE_BACKEND_TEST(FrameRGBBGRConversionTest); + +// ============ Large Frame Tests (Stress Testing) ============ + +class LargeFrameConversionTest : public BackendParameterizedTest { +protected: + void SetUp() override { + BackendParameterizedTest::SetUp(); + } +}; + +TEST_P(LargeFrameConversionTest, Large_1280x720_RGB24_To_BGR24) { + auto backend = GetParam(); + const int width = 1280; + const int height = 720; + + auto frame = createTestFrame(width, height, ccap::PixelFormat::RGB24); + + // Fill with gradient pattern + TestImage test_pattern(width, height, 3); + test_pattern.fillGradient(); + memcpy(frame->data[0], test_pattern.data(), width * height * 3); + + // Store original data for verification + std::vector original_data(frame->data[0], frame->data[0] + width * height * 3); + + // Convert RGB24 -> BGR24 + auto start = std::chrono::high_resolution_clock::now(); + bool success = ccap::inplaceConvertFrameRGB(frame.get(), ccap::PixelFormat::BGR24, false); + auto end = std::chrono::high_resolution_clock::now(); + auto duration = std::chrono::duration_cast(end - start); + + ASSERT_TRUE(success) << "1280x720 RGB24 to BGR24 conversion failed, backend: " + << BackendTestManager::getBackendName(backend); + + // Performance check - should complete in reasonable time + EXPECT_LT(duration.count(), 500) << "Conversion too slow for 1280x720, backend: " + << BackendTestManager::getBackendName(backend); + + // Verify correctness for multiple points across the image + // Check corners, center, and various positions to ensure AVX2 code works correctly + std::vector> test_points = { + {0, 0}, // Top-left corner + {width-1, 0}, // Top-right corner + {0, height-1}, // Bottom-left corner + {width-1, height-1}, // Bottom-right corner + {width/2, height/2}, // Center + {10, 10}, // Early in scan (before AVX2 alignment) + {width/3, height/3}, // Middle-ish area + {width*2/3, height*2/3}, // Another middle area + }; + + for (const auto& point : test_points) { + int x = point.first; + int y = point.second; + int offset = y * frame->stride[0] + x * 3; + + EXPECT_EQ(original_data[offset + 0], frame->data[0][offset + 2]) + << "R->B swap failed at (" << x << "," << y << "), backend: " + << BackendTestManager::getBackendName(backend); + EXPECT_EQ(original_data[offset + 1], frame->data[0][offset + 1]) + << "G unchanged failed at (" << x << "," << y << "), backend: " + << BackendTestManager::getBackendName(backend); + EXPECT_EQ(original_data[offset + 2], frame->data[0][offset + 0]) + << "B->R swap failed at (" << x << "," << y << "), backend: " + << BackendTestManager::getBackendName(backend); + } + + // Also verify a full row in the middle to catch any AVX2 patchSize issues + int mid_row = height / 2; + for (int x = 0; x < width; ++x) { + int offset = mid_row * frame->stride[0] + x * 3; + EXPECT_EQ(original_data[offset + 0], frame->data[0][offset + 2]) + << "Full row check: R->B swap failed at x=" << x << ", backend: " + << BackendTestManager::getBackendName(backend); + EXPECT_EQ(original_data[offset + 1], frame->data[0][offset + 1]) + << "Full row check: G unchanged failed at x=" << x << ", backend: " + << BackendTestManager::getBackendName(backend); + EXPECT_EQ(original_data[offset + 2], frame->data[0][offset + 0]) + << "Full row check: B->R swap failed at x=" << x << ", backend: " + << BackendTestManager::getBackendName(backend); + } +} + +TEST_P(LargeFrameConversionTest, Large_1920x1080_BGR24_To_RGB24) { + auto backend = GetParam(); + const int width = 1920; + const int height = 1080; + + auto frame = createTestFrame(width, height, ccap::PixelFormat::BGR24); + + // Fill with random pattern + TestImage test_pattern(width, height, 3); + test_pattern.fillRandom(12345); + memcpy(frame->data[0], test_pattern.data(), width * height * 3); + + // Store original data for verification + std::vector original_data(frame->data[0], frame->data[0] + width * height * 3); + + // Convert BGR24 -> RGB24 + auto start = std::chrono::high_resolution_clock::now(); + bool success = ccap::inplaceConvertFrameRGB(frame.get(), ccap::PixelFormat::RGB24, false); + auto end = std::chrono::high_resolution_clock::now(); + auto duration = std::chrono::duration_cast(end - start); + + ASSERT_TRUE(success) << "1920x1080 BGR24 to RGB24 conversion failed, backend: " + << BackendTestManager::getBackendName(backend); + + // Performance check + EXPECT_LT(duration.count(), 1000) << "Conversion too slow for 1920x1080, backend: " + << BackendTestManager::getBackendName(backend); + + // Verify correctness for sample points + std::vector> test_points = { + {0, 0}, {width-1, 0}, {0, height-1}, {width-1, height-1}, + {width/2, height/2}, {15, 15}, {width/3, height/3} + }; + + for (const auto& point : test_points) { + int x = point.first; + int y = point.second; + int offset = y * frame->stride[0] + x * 3; + + EXPECT_EQ(original_data[offset + 0], frame->data[0][offset + 2]) + << "Channel swap failed at (" << x << "," << y << ")"; + EXPECT_EQ(original_data[offset + 1], frame->data[0][offset + 1]) + << "G unchanged failed at (" << x << "," << y << ")"; + EXPECT_EQ(original_data[offset + 2], frame->data[0][offset + 0]) + << "Channel swap failed at (" << x << "," << y << ")"; + } +} + +INSTANTIATE_BACKEND_TEST(LargeFrameConversionTest); + +// ============ Edge Case Tests ============ + +class FrameConversionEdgeCaseTest : public BackendParameterizedTest { +protected: + void SetUp() override { + BackendParameterizedTest::SetUp(); + } +}; + +TEST_P(FrameConversionEdgeCaseTest, Odd_Width_RGB24_To_BGR24) { + auto backend = GetParam(); + + // Odd widths can cause alignment issues with SIMD code + std::vector odd_widths = {1, 3, 5, 7, 11, 13, 17, 31, 63, 127}; + + for (int width : odd_widths) { + int height = 8; + auto frame = createTestFrame(width, height, ccap::PixelFormat::RGB24); + + TestImage test_pattern(width, height, 3); + test_pattern.fillRandom(width); // Different seed for each width + memcpy(frame->data[0], test_pattern.data(), width * height * 3); + + // Save original stride before conversion (might change after conversion due to alignment) + int original_stride = frame->stride[0]; + std::vector original_data(frame->data[0], frame->data[0] + original_stride * height); + + bool success = ccap::inplaceConvertFrameRGB(frame.get(), ccap::PixelFormat::BGR24, false); + ASSERT_TRUE(success) << "Conversion failed for width=" << width << ", backend: " + << BackendTestManager::getBackendName(backend); + + // Verify all pixels + for (int y = 0; y < height; ++y) { + for (int x = 0; x < width; ++x) { + // Use original stride for original_data, new stride for frame->data + int original_offset = y * original_stride + x * 3; + int new_offset = y * frame->stride[0] + x * 3; + EXPECT_EQ(original_data[original_offset + 0], frame->data[0][new_offset + 2]) + << "Failed at width=" << width << ", pos=(" << x << "," << y << ")"; + EXPECT_EQ(original_data[original_offset + 1], frame->data[0][new_offset + 1]) + << "Failed at width=" << width << ", pos=(" << x << "," << y << ")"; + EXPECT_EQ(original_data[original_offset + 2], frame->data[0][new_offset + 0]) + << "Failed at width=" << width << ", pos=(" << x << "," << y << ")"; + } + } + } +} + +TEST_P(FrameConversionEdgeCaseTest, Non_Multiple_Of_Patch_Size_Widths) { + auto backend = GetParam(); + + // Test widths that are not multiples of common SIMD patch sizes (5, 8, 10, 16) + // These should trigger the scalar fallback code path + std::vector test_widths = { + 6, 9, 14, 18, 23, 33, 47, 62, 77, 99, + 100, 127, 255, 319, 639, 641, 1279, 1281 + }; + + for (int width : test_widths) { + int height = 4; + auto frame = createTestFrame(width, height, ccap::PixelFormat::RGB24); + + TestImage test_pattern(width, height, 3); + test_pattern.fillGradient(); + memcpy(frame->data[0], test_pattern.data(), width * height * 3); + + // Save original stride before conversion + int original_stride = frame->stride[0]; + std::vector original_data(frame->data[0], frame->data[0] + original_stride * height); + + bool success = ccap::inplaceConvertFrameRGB(frame.get(), ccap::PixelFormat::BGR24, false); + ASSERT_TRUE(success) << "Conversion failed for width=" << width << ", backend: " + << BackendTestManager::getBackendName(backend); + + // Verify last few pixels (most likely to have issues) + for (int x = std::max(0, width - 10); x < width; ++x) { + for (int y = 0; y < height; ++y) { + // Use original stride for original_data, new stride for frame->data + int original_offset = y * original_stride + x * 3; + int new_offset = y * frame->stride[0] + x * 3; + EXPECT_EQ(original_data[original_offset + 0], frame->data[0][new_offset + 2]) + << "Failed at width=" << width << ", pos=(" << x << "," << y << ")"; + EXPECT_EQ(original_data[original_offset + 1], frame->data[0][new_offset + 1]) + << "Failed at width=" << width << ", pos=(" << x << "," << y << ")"; + EXPECT_EQ(original_data[original_offset + 2], frame->data[0][new_offset + 0]) + << "Failed at width=" << width << ", pos=(" << x << "," << y << ")"; + } + } + } +} + +INSTANTIATE_BACKEND_TEST(FrameConversionEdgeCaseTest); From bed9a0e89191b269b4e2f6edc0b32eb2f187731f Mon Sep 17 00:00:00 2001 From: "wangyang (wysaid)" Date: Sat, 13 Dec 2025 12:21:56 +0800 Subject: [PATCH 4/7] Fix: Conditionally export inplaceConvertFrame* functions for testing only These internal conversion functions are only needed for unit testing and should not be part of the public API in production builds. - Added CCAP_TEST_EXPORT macro that expands to CCAP_EXPORT only when CCAP_BUILD_TESTS is ON - In production builds (CCAP_BUILD_TESTS=OFF), these functions remain internal - Fixes LNK2019 unresolved external symbol error in shared library test builds - Ensures cleaner API surface in production releases --- src/ccap_convert_frame.h | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/ccap_convert_frame.h b/src/ccap_convert_frame.h index fa991b10..cb803bae 100644 --- a/src/ccap_convert_frame.h +++ b/src/ccap_convert_frame.h @@ -30,11 +30,18 @@ /// - Assertion failure in debug builds /// - Undefined behavior +// Export internal functions only when building tests +#ifdef CCAP_BUILD_TESTS + #define CCAP_TEST_EXPORT CCAP_EXPORT +#else + #define CCAP_TEST_EXPORT +#endif + namespace ccap { -bool inplaceConvertFrame(VideoFrame* frame, PixelFormat toFormat, bool verticalFlip); -bool inplaceConvertFrameRGB(VideoFrame* frame, PixelFormat toFormat, bool verticalFlip); -bool inplaceConvertFrameYUV2RGBColor(VideoFrame* frame, PixelFormat toFormat, bool verticalFlip); +CCAP_TEST_EXPORT bool inplaceConvertFrame(VideoFrame* frame, PixelFormat toFormat, bool verticalFlip); +CCAP_TEST_EXPORT bool inplaceConvertFrameRGB(VideoFrame* frame, PixelFormat toFormat, bool verticalFlip); +CCAP_TEST_EXPORT bool inplaceConvertFrameYUV2RGBColor(VideoFrame* frame, PixelFormat toFormat, bool verticalFlip); } // namespace ccap From abf30597d4b5bc9130b5d0e19f2d8bda05416280 Mon Sep 17 00:00:00 2001 From: "wangyang (wysaid)" Date: Sat, 13 Dec 2025 19:26:47 +0800 Subject: [PATCH 5/7] Fix AVX2 SIMD boundary overread in colorShuffle_avx2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit fixes memory overread issues in AVX2-optimized RGB/BGR color conversions that could cause crashes at certain image widths (e.g., 1280). Problem: - 3->3 conversion (RGB↔BGR): Reads 16 bytes but only needs 15, causing 1-byte overread at boundaries - 3->4 conversion (RGB→RGBA): Second read extends to offset+27, causing 4-byte overread at boundaries Root cause: SIMD intrinsics (_mm_loadu_si128) read fixed byte amounts that can exceed actual pixel data at image boundaries. Solution: Calculate proper loop boundary for each conversion type: - 3->4 conversions: patchSize + 2 (needs 2 extra pixels margin) - 3->3 conversions: patchSize + 1 (needs 1 extra pixel margin) - 4->4 and 4->3: patchSize (already safe) Testing: Added 78 comprehensive test cases covering: - Issue #30 exact scenario (1280x720) - Critical boundary widths (1272-1280, 1915-1920) - Small/tiny widths (1-10 pixels) - Non-contiguous memory (with stride padding) All tests pass on both CPU and AVX2 backends. Fixes #30 --- src/ccap_convert_avx2.cpp | 12 +- tests/CMakeLists.txt | 1 + tests/test_boundary_conditions.cpp | 426 +++++++++++++++++++++++++++++ 3 files changed, 438 insertions(+), 1 deletion(-) create mode 100644 tests/test_boundary_conditions.cpp diff --git a/src/ccap_convert_avx2.cpp b/src/ccap_convert_avx2.cpp index a7c19876..c3016362 100644 --- a/src/ccap_convert_avx2.cpp +++ b/src/ccap_convert_avx2.cpp @@ -181,11 +181,21 @@ AVX2_TARGET void colorShuffle_avx2(const uint8_t* src, int srcStride, uint8_t* d shuffle128 = _mm_load_si128((__m128i*)shuffleData); } + // Different cases require different boundary conditions to avoid reading beyond allocated memory: + // - 3->4: reads 16 bytes from x*3+12, needs x*3+27 < width*3, i.e., x+9 < width + // - 3->3: reads 16 bytes from x*3, needs x*3+15 < width*3, i.e., x+5 < width + // - 4->3: reads 16 bytes from x*4+16, needs x*4+31 < width*4, i.e., x+8 <= width + // - 4->4: reads 32 bytes from x*4, needs x*4+31 < width*4, i.e., x+8 <= width + constexpr uint32_t loopBoundary = (inputChannels == 3 && outputChannels == 4) ? (patchSize + 2) : + (inputChannels == 3 && outputChannels == 3) ? (patchSize + 1) : + patchSize; + for (int y = 0; y < height; ++y) { const uint8_t* srcRow = src + y * srcStride; uint8_t* dstRow = dst + y * dstStride; uint32_t x = 0; - while (x + patchSize <= (uint32_t)width) { + + while (x + loopBoundary <= (uint32_t)width) { // _mm256_shuffle_epi8 can’t move these bytes across 16-byte lanes of the vector. // @see issue if constexpr (outputChannels == 4 && inputChannels == 3) { // 3 -> 4, need to split channels diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index ed71a096..13ac45ad 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -73,6 +73,7 @@ add_executable( test_yuv_conversions.cpp test_platform_features.cpp test_frame_conversions.cpp + test_boundary_conditions.cpp ) target_link_libraries( diff --git a/tests/test_boundary_conditions.cpp b/tests/test_boundary_conditions.cpp new file mode 100644 index 00000000..6fd7ffab --- /dev/null +++ b/tests/test_boundary_conditions.cpp @@ -0,0 +1,426 @@ +/** + * @file test_boundary_conditions.cpp + * @brief Tests for SIMD boundary conditions (Issue #30) + * + * This test suite verifies that SIMD implementations correctly handle + * edge cases where memory reads could extend beyond allocated buffers. + * + * Critical test cases: + * - 3->3 channel conversions (RGB<->BGR) at boundary widths + * - 3->4 channel conversions (RGB->RGBA) at boundary widths + * - Various image widths that trigger different SIMD path endings + * + * @see https://github.com/wysaid/CameraCapture/issues/30 + */ + +#include "ccap_convert.h" +#include "test_utils.h" +#include "test_backend_manager.h" +#include +#include + +using namespace ccap_test; + +/** + * @brief Test fixture for boundary condition tests + * + * Tests various critical widths that could trigger SIMD boundary issues: + * - Standard resolutions: 1280, 1920 + * - Near-boundary widths: 1275-1280, 1915-1920 + * - Edge cases that align differently with SIMD vector sizes + */ +class BoundaryConditionTest : public BackendParameterizedTest { +protected: + void SetUp() override { + BackendParameterizedTest::SetUp(); + } + + /** + * @brief Test RGB to BGR conversion at a specific width + * + * This tests the 3->3 channel conversion which: + * - Processes 5 pixels per iteration (AVX2) + * - Reads 16 bytes each time (overshooting by 1 byte) + * - Previously crashed at width=1280 when x=1275 + */ + void testRgbToBgrAtWidth(int width) { + auto backend = GetParam(); + const int height = 2; // Keep height small, we're testing width boundaries + + TestImage src(width, height, 3); + TestImage dst(width, height, 3); + + // Fill with recognizable pattern + for (int y = 0; y < height; ++y) { + for (int x = 0; x < width; ++x) { + uint8_t* pixel = src.data() + y * src.stride() + x * 3; + pixel[0] = (uint8_t)((x + 0) % 256); // R + pixel[1] = (uint8_t)((x + 1) % 256); // G + pixel[2] = (uint8_t)((x + 2) % 256); // B + } + } + + // This should not crash with the fix + ccap::rgbToBgr(src.data(), src.stride(), + dst.data(), dst.stride(), + width, height); + + // Verify conversion correctness at various positions + std::vector testPositions = {0, 1, width/2, width-2, width-1}; + for (int x : testPositions) { + if (x >= width) continue; + + for (int y = 0; y < height; ++y) { + const uint8_t* srcPixel = src.data() + y * src.stride() + x * 3; + const uint8_t* dstPixel = dst.data() + y * dst.stride() + x * 3; + + EXPECT_EQ(srcPixel[0], dstPixel[2]) + << "R->B mismatch at (" << x << "," << y << ") width=" << width + << " backend: " << BackendTestManager::getBackendName(backend); + EXPECT_EQ(srcPixel[1], dstPixel[1]) + << "G->G mismatch at (" << x << "," << y << ") width=" << width + << " backend: " << BackendTestManager::getBackendName(backend); + EXPECT_EQ(srcPixel[2], dstPixel[0]) + << "B->R mismatch at (" << x << "," << y << ") width=" << width + << " backend: " << BackendTestManager::getBackendName(backend); + } + } + } + + /** + * @brief Test RGB to RGBA conversion at a specific width + * + * This tests the 3->4 channel conversion which: + * - Processes 8 pixels per iteration (AVX2) + * - Reads up to 28 bytes (16 bytes from offset 12) + * - Previously would crash at width=1280 when x=1272 + */ + void testRgbToRgbaAtWidth(int width) { + auto backend = GetParam(); + const int height = 2; + + TestImage src(width, height, 3); + TestImage dst(width, height, 4); + + // Fill with pattern + for (int y = 0; y < height; ++y) { + for (int x = 0; x < width; ++x) { + uint8_t* pixel = src.data() + y * src.stride() + x * 3; + pixel[0] = (uint8_t)((x * 3 + 0) % 256); + pixel[1] = (uint8_t)((x * 3 + 1) % 256); + pixel[2] = (uint8_t)((x * 3 + 2) % 256); + } + } + + // This should not crash with the fix + ccap::rgbToRgba(src.data(), src.stride(), + dst.data(), dst.stride(), + width, height); + + // Verify conversion + std::vector testPositions = {0, 1, width/2, width-2, width-1}; + for (int x : testPositions) { + if (x >= width) continue; + + for (int y = 0; y < height; ++y) { + const uint8_t* srcPixel = src.data() + y * src.stride() + x * 3; + const uint8_t* dstPixel = dst.data() + y * dst.stride() + x * 4; + + EXPECT_EQ(srcPixel[0], dstPixel[0]) + << "R mismatch at (" << x << "," << y << ") width=" << width + << " backend: " << BackendTestManager::getBackendName(backend); + EXPECT_EQ(srcPixel[1], dstPixel[1]) + << "G mismatch at (" << x << "," << y << ") width=" << width + << " backend: " << BackendTestManager::getBackendName(backend); + EXPECT_EQ(srcPixel[2], dstPixel[2]) + << "B mismatch at (" << x << "," << y << ") width=" << width + << " backend: " << BackendTestManager::getBackendName(backend); + EXPECT_EQ(255, dstPixel[3]) + << "A mismatch at (" << x << "," << y << ") width=" << width + << " backend: " << BackendTestManager::getBackendName(backend); + } + } + } + + /** + * @brief Test BGR to RGB conversion (same as RGB to BGR) + */ + void testBgrToRgbAtWidth(int width) { + auto backend = GetParam(); + const int height = 2; + + TestImage src(width, height, 3); + TestImage dst(width, height, 3); + + src.fillRandom(12345); + + ccap::bgrToRgb(src.data(), src.stride(), + dst.data(), dst.stride(), + width, height); + + // Spot check + for (int x : {0, width-1}) { + const uint8_t* srcPixel = src.data() + x * 3; + const uint8_t* dstPixel = dst.data() + x * 3; + EXPECT_EQ(srcPixel[0], dstPixel[2]) << "width=" << width; + EXPECT_EQ(srcPixel[2], dstPixel[0]) << "width=" << width; + } + } + + /** + * @brief Test BGR to BGRA conversion + */ + void testBgrToBgraAtWidth(int width) { + auto backend = GetParam(); + const int height = 2; + + TestImage src(width, height, 3); + TestImage dst(width, height, 4); + + src.fillRandom(54321); + + ccap::bgrToBgra(src.data(), src.stride(), + dst.data(), dst.stride(), + width, height); + + // Verify alpha and spot check colors + for (int x : {0, width/2, width-1}) { + const uint8_t* srcPixel = src.data() + x * 3; + const uint8_t* dstPixel = dst.data() + x * 4; + EXPECT_EQ(srcPixel[0], dstPixel[0]) << "width=" << width; + EXPECT_EQ(srcPixel[1], dstPixel[1]) << "width=" << width; + EXPECT_EQ(srcPixel[2], dstPixel[2]) << "width=" << width; + EXPECT_EQ(255, dstPixel[3]) << "width=" << width; + } + } +}; + +// Test the exact case reported in issue #30 +TEST_P(BoundaryConditionTest, Issue30_1280x720_RGB_to_BGR) { + testRgbToBgrAtWidth(1280); +} + +TEST_P(BoundaryConditionTest, Issue30_1280x720_BGR_to_RGB) { + testBgrToRgbAtWidth(1280); +} + +// Test 3->4 conversion that also had boundary issues +TEST_P(BoundaryConditionTest, Issue30_1280x720_RGB_to_RGBA) { + testRgbToRgbaAtWidth(1280); +} + +TEST_P(BoundaryConditionTest, Issue30_1280x720_BGR_to_BGRA) { + testBgrToBgraAtWidth(1280); +} + +// Test 1920x1080 (Full HD) +TEST_P(BoundaryConditionTest, FullHD_1920_RGB_to_BGR) { + testRgbToBgrAtWidth(1920); +} + +TEST_P(BoundaryConditionTest, FullHD_1920_RGB_to_RGBA) { + testRgbToRgbaAtWidth(1920); +} + +// Test critical boundary widths for 3->3 conversion (processes 5 pixels at a time) +// These widths are chosen to trigger different remainder cases in SIMD loops +TEST_P(BoundaryConditionTest, Boundary_Width_1275_RGB_to_BGR) { + testRgbToBgrAtWidth(1275); // Should be last full SIMD iteration for old buggy code +} + +TEST_P(BoundaryConditionTest, Boundary_Width_1276_RGB_to_BGR) { + testRgbToBgrAtWidth(1276); +} + +TEST_P(BoundaryConditionTest, Boundary_Width_1277_RGB_to_BGR) { + testRgbToBgrAtWidth(1277); +} + +TEST_P(BoundaryConditionTest, Boundary_Width_1278_RGB_to_BGR) { + testRgbToBgrAtWidth(1278); +} + +TEST_P(BoundaryConditionTest, Boundary_Width_1279_RGB_to_BGR) { + testRgbToBgrAtWidth(1279); +} + +// Test critical boundary widths for 3->4 conversion (processes 8 pixels at a time) +TEST_P(BoundaryConditionTest, Boundary_Width_1272_RGB_to_RGBA) { + testRgbToRgbaAtWidth(1272); // Would crash with old buggy code +} + +TEST_P(BoundaryConditionTest, Boundary_Width_1273_RGB_to_RGBA) { + testRgbToRgbaAtWidth(1273); +} + +TEST_P(BoundaryConditionTest, Boundary_Width_1274_RGB_to_RGBA) { + testRgbToRgbaAtWidth(1274); +} + +TEST_P(BoundaryConditionTest, Boundary_Width_1278_RGB_to_RGBA) { + testRgbToRgbaAtWidth(1278); +} + +TEST_P(BoundaryConditionTest, Boundary_Width_1279_RGB_to_RGBA) { + testRgbToRgbaAtWidth(1279); +} + +// Test widths around 1920 (Full HD) +TEST_P(BoundaryConditionTest, Boundary_Width_1915_RGB_to_BGR) { + testRgbToBgrAtWidth(1915); +} + +TEST_P(BoundaryConditionTest, Boundary_Width_1916_RGB_to_BGR) { + testRgbToBgrAtWidth(1916); +} + +TEST_P(BoundaryConditionTest, Boundary_Width_1917_RGB_to_BGR) { + testRgbToBgrAtWidth(1917); +} + +TEST_P(BoundaryConditionTest, Boundary_Width_1918_RGB_to_BGR) { + testRgbToBgrAtWidth(1918); +} + +TEST_P(BoundaryConditionTest, Boundary_Width_1919_RGB_to_BGR) { + testRgbToBgrAtWidth(1919); +} + +TEST_P(BoundaryConditionTest, Boundary_Width_1912_RGB_to_RGBA) { + testRgbToRgbaAtWidth(1912); // 1920-8, critical for 3->4 +} + +TEST_P(BoundaryConditionTest, Boundary_Width_1918_RGB_to_RGBA) { + testRgbToRgbaAtWidth(1918); +} + +TEST_P(BoundaryConditionTest, Boundary_Width_1919_RGB_to_RGBA) { + testRgbToRgbaAtWidth(1919); +} + +// Test some odd widths to ensure robustness +TEST_P(BoundaryConditionTest, OddWidth_1281_RGB_to_BGR) { + testRgbToBgrAtWidth(1281); +} + +TEST_P(BoundaryConditionTest, OddWidth_1283_RGB_to_BGR) { + testRgbToBgrAtWidth(1283); +} + +TEST_P(BoundaryConditionTest, OddWidth_1285_RGB_to_RGBA) { + testRgbToRgbaAtWidth(1285); +} + +TEST_P(BoundaryConditionTest, OddWidth_1287_RGB_to_RGBA) { + testRgbToRgbaAtWidth(1287); +} + +// Small widths to test edge cases +TEST_P(BoundaryConditionTest, SmallWidth_5_RGB_to_BGR) { + testRgbToBgrAtWidth(5); // Exactly one SIMD iteration for 3->3 +} + +TEST_P(BoundaryConditionTest, SmallWidth_6_RGB_to_BGR) { + testRgbToBgrAtWidth(6); // One pixel past SIMD +} + +TEST_P(BoundaryConditionTest, SmallWidth_8_RGB_to_RGBA) { + testRgbToRgbaAtWidth(8); // Exactly one SIMD iteration for 3->4 +} + +TEST_P(BoundaryConditionTest, SmallWidth_9_RGB_to_RGBA) { + testRgbToRgbaAtWidth(9); // One pixel past SIMD +} + +TEST_P(BoundaryConditionTest, SmallWidth_10_RGB_to_RGBA) { + testRgbToRgbaAtWidth(10); // Two pixels past SIMD +} + +// Very small widths (should use scalar fallback entirely) +TEST_P(BoundaryConditionTest, TinyWidth_1_RGB_to_BGR) { + testRgbToBgrAtWidth(1); +} + +TEST_P(BoundaryConditionTest, TinyWidth_2_RGB_to_BGR) { + testRgbToBgrAtWidth(2); +} + +TEST_P(BoundaryConditionTest, TinyWidth_3_RGB_to_BGR) { + testRgbToBgrAtWidth(3); +} + +TEST_P(BoundaryConditionTest, TinyWidth_4_RGB_to_BGR) { + testRgbToBgrAtWidth(4); +} + +// Test with stride != width * channels (non-contiguous memory) +TEST_P(BoundaryConditionTest, NonContiguous_1280_RGB_to_BGR) { + auto backend = GetParam(); + const int width = 1280; + const int height = 2; + const int srcStride = width * 3 + 32; // Add padding + const int dstStride = width * 3 + 16; // Different padding + + std::vector srcBuffer(srcStride * height); + std::vector dstBuffer(dstStride * height); + + // Fill source + for (int y = 0; y < height; ++y) { + for (int x = 0; x < width; ++x) { + uint8_t* pixel = srcBuffer.data() + y * srcStride + x * 3; + pixel[0] = (uint8_t)((x + 0) % 256); + pixel[1] = (uint8_t)((x + 1) % 256); + pixel[2] = (uint8_t)((x + 2) % 256); + } + } + + // Convert + ccap::rgbToBgr(srcBuffer.data(), srcStride, + dstBuffer.data(), dstStride, + width, height); + + // Verify + for (int y = 0; y < height; ++y) { + for (int x : {0, width-1}) { + const uint8_t* srcPixel = srcBuffer.data() + y * srcStride + x * 3; + const uint8_t* dstPixel = dstBuffer.data() + y * dstStride + x * 3; + EXPECT_EQ(srcPixel[0], dstPixel[2]) << "x=" << x << " y=" << y; + EXPECT_EQ(srcPixel[2], dstPixel[0]) << "x=" << x << " y=" << y; + } + } +} + +TEST_P(BoundaryConditionTest, NonContiguous_1280_RGB_to_RGBA) { + auto backend = GetParam(); + const int width = 1280; + const int height = 2; + const int srcStride = width * 3 + 64; + const int dstStride = width * 4 + 32; + + std::vector srcBuffer(srcStride * height); + std::vector dstBuffer(dstStride * height); + + // Fill and convert + for (int y = 0; y < height; ++y) { + for (int x = 0; x < width; ++x) { + uint8_t* pixel = srcBuffer.data() + y * srcStride + x * 3; + pixel[0] = (uint8_t)(x % 256); + pixel[1] = (uint8_t)((x + 1) % 256); + pixel[2] = (uint8_t)((x + 2) % 256); + } + } + + ccap::rgbToRgba(srcBuffer.data(), srcStride, + dstBuffer.data(), dstStride, + width, height); + + // Verify alpha and some pixels + for (int y = 0; y < height; ++y) { + for (int x : {0, width/2, width-1}) { + const uint8_t* dstPixel = dstBuffer.data() + y * dstStride + x * 4; + EXPECT_EQ(255, dstPixel[3]) << "Alpha at x=" << x << " y=" << y; + } + } +} + +INSTANTIATE_BACKEND_TEST(BoundaryConditionTest); From 640328866b3504d9e89b12993b000846daf612a9 Mon Sep 17 00:00:00 2001 From: LeeGoDamn Date: Sat, 13 Dec 2025 21:15:49 +0800 Subject: [PATCH 6/7] Enable AddressSanitizer in CI/CD workflows and fix test boundary issues (#36) - Enable ASAN by default for functional tests in run_tests.sh - Add --no-sanitize and --sanitize-all options for flexibility - Update Linux and macOS workflows to use ASAN in unit tests - Fix heap-buffer-overflow in boundary condition tests for tiny widths - Add platform detection to disable ASAN on Windows automatically ASAN helps catch memory errors like Issue #30's buffer overflow early in development, improving code quality and preventing crashes. Co-authored-by: wangyang (wysaid) --- .github/workflows/linux-build.yml | 5 ++ .github/workflows/macos-build.yml | 1 + scripts/run_tests.sh | 124 ++++++++++++++++++++++++++--- tests/test_boundary_conditions.cpp | 20 ++++- 4 files changed, 136 insertions(+), 14 deletions(-) diff --git a/.github/workflows/linux-build.yml b/.github/workflows/linux-build.yml index 1e19e7a5..a5c12c24 100644 --- a/.github/workflows/linux-build.yml +++ b/.github/workflows/linux-build.yml @@ -140,6 +140,7 @@ jobs: if [ "${{ matrix.library_type }}" = "shared" ]; then export LD_LIBRARY_PATH="$PWD/../build/${{ matrix.build_type }}-${{ matrix.library_type }}:$LD_LIBRARY_PATH" fi + # Run tests with AddressSanitizer enabled by default (improves memory error detection) ./run_tests.sh --functional --exit-when-failed - name: Upload artifacts @@ -239,6 +240,8 @@ jobs: if [ "${{ matrix.library_type }}" = "shared" ]; then export LD_LIBRARY_PATH="$PWD/../build/${{ matrix.build_type }}-${{ matrix.library_type }}:$LD_LIBRARY_PATH" fi + # Run tests with AddressSanitizer enabled by default + # This helps catch memory errors early in CI ./run_tests.sh --functional --exit-when-failed - name: Upload artifacts @@ -448,6 +451,8 @@ jobs: if [ "${{ matrix.library_type }}" = "shared" ]; then export LD_LIBRARY_PATH="$PWD/../build/${{ matrix.build_type }}-${{ matrix.library_type }}:$LD_LIBRARY_PATH" fi + # Run tests with AddressSanitizer enabled by default + # This helps catch memory errors early in CI ./run_tests.sh --functional --exit-when-failed - name: Upload artifacts diff --git a/.github/workflows/macos-build.yml b/.github/workflows/macos-build.yml index 05589730..e65d4766 100644 --- a/.github/workflows/macos-build.yml +++ b/.github/workflows/macos-build.yml @@ -105,6 +105,7 @@ jobs: if [ "${{ matrix.library_type }}" = "shared" ]; then export DYLD_LIBRARY_PATH="$PWD/../build/${{ matrix.config }}-${{ matrix.library_type }}:$DYLD_LIBRARY_PATH" fi + # Run tests with AddressSanitizer enabled by default (improves memory error detection) ./run_tests.sh --functional --skip-build else # Set library path for shared library tests diff --git a/scripts/run_tests.sh b/scripts/run_tests.sh index 55c3f996..a3178aab 100755 --- a/scripts/run_tests.sh +++ b/scripts/run_tests.sh @@ -8,12 +8,19 @@ # - Windows (MSVC): Uses single build directory, configs specified during build # - Linux/Mac: Uses separate build/Debug and build/Release directories # +# Memory Sanitizer (ASAN): +# - Functional tests: ENABLED by default (can be disabled with --no-sanitize) +# - Performance tests: DISABLED by default (can be enabled with --sanitize-all) +# - ASAN helps detect memory errors like buffer overflows, use-after-free, etc. +# # Usage: -# ./run_tests.sh # Run all tests -# ./run_tests.sh --functional # Run only functional tests -# ./run_tests.sh --performance # Run only performance tests +# ./run_tests.sh # Run all tests (functional with ASAN, performance without) +# ./run_tests.sh --functional # Run only functional tests (with ASAN) +# ./run_tests.sh --performance # Run only performance tests (without ASAN) # ./run_tests.sh --avx2 # Run only AVX2 performance tests # ./run_tests.sh --shuffle # Run only tests with names containing 'shuffle' (case-insensitive) in functional tests +# ./run_tests.sh --no-sanitize # Disable ASAN for all tests +# ./run_tests.sh --sanitize-all # Enable ASAN for all tests (including performance) # ./run_tests.sh --help # Show help set -e # Exit on any error @@ -70,6 +77,9 @@ EXIT_WHEN_FAILED=false GTEST_FAIL_FAST_PARAM="" FILTER="" SHUFFLE_ONLY=false +# Memory sanitizer flags: auto-detect based on test type, can be overridden +SANITIZE_FUNCTIONAL="auto" # Default: enabled for functional tests +SANITIZE_PERFORMANCE="auto" # Default: disabled for performance tests while [[ $# -gt 0 ]]; do case $1 in @@ -105,19 +115,37 @@ while [[ $# -gt 0 ]]; do GTEST_FAIL_FAST_PARAM="--gtest_fail_fast" shift ;; + --no-sanitize) + SANITIZE_FUNCTIONAL="no" + SANITIZE_PERFORMANCE="no" + shift + ;; + --sanitize-all) + SANITIZE_FUNCTIONAL="yes" + SANITIZE_PERFORMANCE="yes" + shift + ;; --help) echo "CCAP Unit Tests Runner" echo "" echo "Usage:" - echo " $0 # Run all tests" - echo " $0 --functional # Run only functional tests (Debug mode)" - echo " $0 --performance # Run only performance tests (Release mode)" + echo " $0 # Run all tests (functional with ASAN, performance without)" + echo " $0 --functional # Run only functional tests (Debug mode, with ASAN)" + echo " $0 --performance # Run only performance tests (Release mode, without ASAN)" echo " $0 --avx2 # Run only AVX2 performance tests (Release mode)" echo " $0 --shuffle # Run only tests whose names contain '*shuffle*' or '*Shuffle*' in functional tests" echo " $0 --skip-build # Skip build step, run tests only" echo " $0 --exit-when-failed # Stop at first test failure (gtest fail fast mode)" + echo " $0 --no-sanitize # Disable AddressSanitizer (ASAN) for all tests" + echo " $0 --sanitize-all # Enable AddressSanitizer (ASAN) for all tests (including performance)" echo " $0 --help # Show this help" echo "" + echo "Memory Sanitizer (ASAN):" + echo " - Functional tests: ENABLED by default (detects memory errors)" + echo " - Performance tests: DISABLED by default (would affect performance measurements)" + echo " - Use --no-sanitize to disable ASAN completely" + echo " - Use --sanitize-all to enable ASAN for performance tests too" + echo "" echo "Note: Performance tests are automatically run in Release mode for accurate results" exit 0 ;; @@ -162,6 +190,48 @@ fi TEST_RESULT=0 PERF_RESULT=0 +# Determine ASAN usage +# Functional tests: default enabled, Performance tests: default disabled +USE_ASAN_FUNCTIONAL=false +USE_ASAN_PERFORMANCE=false + +if [ "$SANITIZE_FUNCTIONAL" = "auto" ]; then + USE_ASAN_FUNCTIONAL=true # Default: enable ASAN for functional tests +elif [ "$SANITIZE_FUNCTIONAL" = "yes" ]; then + USE_ASAN_FUNCTIONAL=true +fi + +if [ "$SANITIZE_PERFORMANCE" = "auto" ]; then + USE_ASAN_PERFORMANCE=false # Default: disable ASAN for performance tests +elif [ "$SANITIZE_PERFORMANCE" = "yes" ]; then + USE_ASAN_PERFORMANCE=true +fi + +# Function to check if ASAN is supported +function checkAsanSupport() { + # ASAN is well supported on Linux and macOS with GCC/Clang + # Windows MSVC support is limited and not used here + if isWindows; then + return 1 # Disable ASAN on Windows for now + fi + return 0 +} + +# Check ASAN support +ASAN_SUPPORTED=false +if checkAsanSupport; then + ASAN_SUPPORTED=true +fi + +# Disable ASAN if not supported +if [ "$ASAN_SUPPORTED" = false ]; then + if [ "$USE_ASAN_FUNCTIONAL" = true ] || [ "$USE_ASAN_PERFORMANCE" = true ]; then + echo -e "${YELLOW}⚠ AddressSanitizer not supported on this platform, disabling ASAN${NC}" + USE_ASAN_FUNCTIONAL=false + USE_ASAN_PERFORMANCE=false + fi +fi + # Build Debug version for functional tests if [ "$RUN_FUNCTIONAL" = true ]; then echo "" @@ -170,15 +240,24 @@ if [ "$RUN_FUNCTIONAL" = true ]; then echo -e "${BLUE}Skipping build, using existing Debug binaries${NC}" else echo -e "${BLUE}Building Debug version (for functional tests)${NC}" + if [ "$USE_ASAN_FUNCTIONAL" = true ]; then + echo -e "${GREEN}🛡️ AddressSanitizer (ASAN) ENABLED for memory error detection${NC}" + fi fi echo -e "${PURPLE}===============================================${NC}" if [ "$SKIP_BUILD" = false ]; then + # Prepare ASAN flags if enabled + ASAN_FLAGS="" + if [ "$USE_ASAN_FUNCTIONAL" = true ]; then + ASAN_FLAGS="-DCMAKE_CXX_FLAGS=\"-fsanitize=address -g\" -DCMAKE_C_FLAGS=\"-fsanitize=address -g\" -DCMAKE_EXE_LINKER_FLAGS=\"-fsanitize=address\" -DCMAKE_SHARED_LINKER_FLAGS=\"-fsanitize=address\"" + fi + if isWindows; then # Windows MSVC: use single build directory, specify config during build cd build echo -e "${BLUE}Configuring CMake (Windows MSVC)...${NC}" - cmake .. -DCCAP_BUILD_TESTS=ON -DCMAKE_POLICY_VERSION_MINIMUM=3.5 + eval cmake .. -DCCAP_BUILD_TESTS=ON -DCMAKE_POLICY_VERSION_MINIMUM=3.5 $ASAN_FLAGS echo -e "${BLUE}Building Debug project...${NC}" cmake --build . --config Debug --parallel $(detectCores) @@ -191,7 +270,7 @@ if [ "$RUN_FUNCTIONAL" = true ]; then # Linux/Mac: use separate Debug directory cd build/Debug echo -e "${BLUE}Configuring CMake (Debug)...${NC}" - cmake ../.. -DCMAKE_BUILD_TYPE=Debug -DCCAP_BUILD_TESTS=ON -DCMAKE_POLICY_VERSION_MINIMUM=3.5 + eval cmake ../.. -DCMAKE_BUILD_TYPE=Debug -DCCAP_BUILD_TESTS=ON -DCMAKE_POLICY_VERSION_MINIMUM=3.5 $ASAN_FLAGS echo -e "${BLUE}Building Debug project...${NC}" cmake --build . --config Debug --parallel $(detectCores) @@ -211,17 +290,27 @@ if [ "$RUN_PERFORMANCE" = true ]; then echo -e "${BLUE}Skipping build, using existing Release binaries${NC}" else echo -e "${BLUE}Building Release version (for performance tests)${NC}" + if [ "$USE_ASAN_PERFORMANCE" = true ]; then + echo -e "${GREEN}🛡️ AddressSanitizer (ASAN) ENABLED${NC}" + echo -e "${YELLOW}⚠ Note: ASAN affects performance measurements${NC}" + fi fi echo -e "${PURPLE}===============================================${NC}" if [ "$SKIP_BUILD" = false ]; then + # Prepare ASAN flags if enabled + ASAN_FLAGS="" + if [ "$USE_ASAN_PERFORMANCE" = true ]; then + ASAN_FLAGS="-DCMAKE_CXX_FLAGS=\"-fsanitize=address -g\" -DCMAKE_C_FLAGS=\"-fsanitize=address -g\" -DCMAKE_EXE_LINKER_FLAGS=\"-fsanitize=address\" -DCMAKE_SHARED_LINKER_FLAGS=\"-fsanitize=address\"" + fi + if isWindows; then # Windows MSVC: use single build directory, specify config during build cd build # Only configure if not already configured if [ ! -f "CMakeCache.txt" ]; then echo -e "${BLUE}Configuring CMake (Windows MSVC)...${NC}" - cmake .. -DCCAP_BUILD_TESTS=ON -DCMAKE_POLICY_VERSION_MINIMUM=3.5 + eval cmake .. -DCCAP_BUILD_TESTS=ON -DCMAKE_POLICY_VERSION_MINIMUM=3.5 $ASAN_FLAGS fi echo -e "${BLUE}Building Release project...${NC}" @@ -234,7 +323,7 @@ if [ "$RUN_PERFORMANCE" = true ]; then # Linux/Mac: use separate Release directory cd build/Release echo -e "${BLUE}Configuring CMake (Release)...${NC}" - cmake ../.. -DCMAKE_BUILD_TYPE=Release -DCCAP_BUILD_TESTS=ON -DCMAKE_POLICY_VERSION_MINIMUM=3.5 + eval cmake ../.. -DCMAKE_BUILD_TYPE=Release -DCCAP_BUILD_TESTS=ON -DCMAKE_POLICY_VERSION_MINIMUM=3.5 $ASAN_FLAGS echo -e "${BLUE}Building Release project...${NC}" cmake --build . --config Release --parallel $(detectCores) @@ -265,6 +354,14 @@ if [ "$RUN_FUNCTIONAL" = true ]; then if [ -f "$TEST_EXECUTABLE" ]; then echo -e "${YELLOW}Running functional tests in Debug mode...${NC}" + # Set ASAN options if enabled + if [ "$USE_ASAN_FUNCTIONAL" = true ]; then + echo -e "${GREEN}🛡️ Running with AddressSanitizer enabled${NC}" + # Disable memory leak detection in ASAN (can cause false positives in tests) + # Enable detailed error reporting + export ASAN_OPTIONS="detect_leaks=0:halt_on_error=1:allocator_may_return_null=1" + fi + if [ "$SHUFFLE_ONLY" = true ]; then echo -e "${BLUE}Filtering tests to names containing '*shuffle*' or '*Shuffle*'...${NC}" "$TEST_EXECUTABLE" --gtest_filter='*shuffle*:*Shuffle*:*SHUFFLE*' $GTEST_FAIL_FAST_PARAM --gtest_output=xml:test_results_debug.xml @@ -326,6 +423,13 @@ if [ "$RUN_PERFORMANCE" = true ]; then echo -e "${YELLOW}Running performance benchmarks in Release mode...${NC}" echo -e "${BLUE}Note: Release mode provides accurate performance measurements${NC}" + # Set ASAN options if enabled + if [ "$USE_ASAN_PERFORMANCE" = true ]; then + echo -e "${GREEN}🛡️ Running with AddressSanitizer enabled${NC}" + echo -e "${YELLOW}⚠ Performance results may be affected by ASAN overhead${NC}" + export ASAN_OPTIONS="detect_leaks=0:halt_on_error=1:allocator_may_return_null=1" + fi + if [ -n "$FILTER" ]; then echo -e "${BLUE}Filter: $FILTER${NC}" "$PERF_EXECUTABLE" $FILTER $GTEST_FAIL_FAST_PARAM --gtest_output=xml:build/performance_results_release.xml diff --git a/tests/test_boundary_conditions.cpp b/tests/test_boundary_conditions.cpp index 6fd7ffab..cdef2aee 100644 --- a/tests/test_boundary_conditions.cpp +++ b/tests/test_boundary_conditions.cpp @@ -66,9 +66,15 @@ class BoundaryConditionTest : public BackendParameterizedTest { width, height); // Verify conversion correctness at various positions - std::vector testPositions = {0, 1, width/2, width-2, width-1}; + std::vector testPositions; + testPositions.push_back(0); + if (width > 1) testPositions.push_back(1); + if (width > 2) testPositions.push_back(width/2); + if (width > 2) testPositions.push_back(width-2); + if (width > 1) testPositions.push_back(width-1); + for (int x : testPositions) { - if (x >= width) continue; + if (x < 0 || x >= width) continue; for (int y = 0; y < height; ++y) { const uint8_t* srcPixel = src.data() + y * src.stride() + x * 3; @@ -118,9 +124,15 @@ class BoundaryConditionTest : public BackendParameterizedTest { width, height); // Verify conversion - std::vector testPositions = {0, 1, width/2, width-2, width-1}; + std::vector testPositions; + testPositions.push_back(0); + if (width > 1) testPositions.push_back(1); + if (width > 2) testPositions.push_back(width/2); + if (width > 2) testPositions.push_back(width-2); + if (width > 1) testPositions.push_back(width-1); + for (int x : testPositions) { - if (x >= width) continue; + if (x < 0 || x >= width) continue; for (int y = 0; y < height; ++y) { const uint8_t* srcPixel = src.data() + y * src.stride() + x * 3; From 78242ca472a26ca57557ea0ea7b744865543edc9 Mon Sep 17 00:00:00 2001 From: "wangyang (wysaid)" Date: Sat, 13 Dec 2025 21:22:02 +0800 Subject: [PATCH 7/7] Fix ASAN library dependency in CI workflows - Install libasan6 for Ubuntu builds (gcc and clang) - Install libasan for Fedora builds - This fixes the linker error: 'cannot find libasan.so.8.0.0' - Ensures AddressSanitizer can be properly enabled in functional tests --- .github/workflows/linux-build.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/linux-build.yml b/.github/workflows/linux-build.yml index a5c12c24..4cf30fd0 100644 --- a/.github/workflows/linux-build.yml +++ b/.github/workflows/linux-build.yml @@ -24,7 +24,7 @@ jobs: - name: Install base dependencies run: | sudo apt-get update - sudo apt-get install -y cmake build-essential gcc + sudo apt-get install -y cmake build-essential gcc libasan6 - name: Install GLFW (for Release builds only) if: matrix.build_type == 'Release' @@ -176,7 +176,7 @@ jobs: - name: Install dependencies run: | sudo apt-get update - sudo apt-get install -y cmake build-essential clang libglfw3-dev + sudo apt-get install -y cmake build-essential clang libglfw3-dev libasan6 - name: Setup compiler run: | @@ -397,7 +397,7 @@ jobs: - name: Install dependencies run: | - dnf install -y cmake gcc-c++ make + dnf install -y cmake gcc-c++ make libasan - name: Configure CMake run: |