From ae48f8dd618da1bf60dae1e907bc72b6fae04f73 Mon Sep 17 00:00:00 2001 From: Sida Zhu Date: Tue, 25 Aug 2026 06:13:51 +0800 Subject: [PATCH] Fix same-width vertical point upsampling ScalePlane's same-width vertical fast path used the endpoint-oriented FixedDiv1 step for all upsampling. That stepping is appropriate for bilinear interpolation, but not for point sampling, where source rows should cover equally sized destination cells. Use the same centered FixedDiv stepping as the generic point sampler. Allow the point path to select the final source row while passing a zero interpolation stride so row kernels cannot form a pointer beyond it. Add 8-bit and 16-bit point regression coverage and verify that bilinear upsampling remains unchanged. Bug: 537818862 Change-Id: I7bee21b85913b31c146ecf7fe9f5f08608a8a17d Reviewed-on: https://chromium-review.googlesource.com/c/libyuv/libyuv/+/8264449 Reviewed-by: Wan-Teh Chang Commit-Queue: Wan-Teh Chang Reviewed-by: Frank Barchard --- AUTHORS | 1 + source/scale.cc | 25 +++++-- source/scale_common.cc | 40 +++++++++--- unit_test/scale_plane_test.cc | 120 ++++++++++++++++++++++++++++++++++ 4 files changed, 169 insertions(+), 17 deletions(-) diff --git a/AUTHORS b/AUTHORS index b272adfd2..42959c1d8 100644 --- a/AUTHORS +++ b/AUTHORS @@ -5,3 +5,4 @@ Google Inc. Ho Cheung Ivan Pavlotskiy +Sida Zhu diff --git a/source/scale.cc b/source/scale.cc index 29424ae71..6dc768918 100644 --- a/source/scale.cc +++ b/source/scale.cc @@ -1966,12 +1966,19 @@ int ScalePlane(const uint8_t* src, int dy = 0; int y = 0; // When scaling down, use the center 2 rows to filter. - // When scaling up, last row of destination uses the last 2 source rows. if (dst_height <= src_height) { dy = FixedDiv(src_height, dst_height); y = CENTERSTART(dy, -32768); // Subtract 0.5 (32768) to center filter. } else if (src_height > 1 && dst_height > 1) { - dy = FixedDiv1(src_height, dst_height); + // Point sampling uses centered source/destination stepping, matching + // ScaleSlope. Bilinear scaling maps the source endpoints. + if (filtering == kFilterNone) { + dy = FixedDiv(src_height, dst_height); + y = CENTERSTART(dy, 0); + } else { + // When scaling up, last row of destination uses the last 2 source rows. + dy = FixedDiv1(src_height, dst_height); + } } // Arbitrary scale vertically, but unscaled horizontally. ScalePlaneVertical(src_height, dst_width, dst_height, src_stride, @@ -2074,15 +2081,19 @@ int ScalePlane_16(const uint16_t* src, int dy = 0; int y = 0; // When scaling down, use the center 2 rows to filter. - // When scaling up, last row of destination uses the last 2 source rows. if (dst_height <= src_height) { dy = FixedDiv(src_height, dst_height); y = CENTERSTART(dy, -32768); // Subtract 0.5 (32768) to center filter. - // When scaling up, ensure the last row of destination uses the last - // source. Avoid divide by zero for dst_height but will do no scaling - // later. } else if (src_height > 1 && dst_height > 1) { - dy = FixedDiv1(src_height, dst_height); + // Point sampling uses centered source/destination stepping, matching + // ScaleSlope. Bilinear scaling maps the source endpoints. + if (filtering == kFilterNone) { + dy = FixedDiv(src_height, dst_height); + y = CENTERSTART(dy, 0); + } else { + // When scaling up, last row of destination uses the last 2 source rows. + dy = FixedDiv1(src_height, dst_height); + } } // Arbitrary scale vertically, but unscaled horizontally. ScalePlaneVertical_16(src_height, dst_width, dst_height, src_stride, diff --git a/source/scale_common.cc b/source/scale_common.cc index d0b0edd95..325e72319 100644 --- a/source/scale_common.cc +++ b/source/scale_common.cc @@ -1474,8 +1474,18 @@ void ScalePlaneVertical(int src_height, void (*InterpolateRow)(uint8_t* dst_argb, const uint8_t* src_argb, ptrdiff_t src_stride, int dst_width, int source_y_fraction) = InterpolateRow_C; - const int64_t max_y = - (src_height > 1) ? (((int64_t)src_height - 1) << 16) - 1 : 0; + const bool interpolate = filtering == kFilterBilinear; + int64_t max_y = 0; + if (src_height > 1) { + max_y = ((int64_t)src_height - 1) << 16; + if (interpolate) { + --max_y; + } + } + // Without vertical interpolation, InterpolateRow() copies one row. A zero + // stride keeps it from forming a second source row pointer past the end when + // y is clamped to the last row. + const ptrdiff_t interpolate_stride = interpolate ? src_stride : 0; int64_t y64 = y; int j; assert(bpp >= 1 && bpp <= 4); @@ -1530,9 +1540,9 @@ void ScalePlaneVertical(int src_height, y64 = max_y; } yi = (int)(y64 >> 16); - yf = filtering ? (int)((y64 >> 8) & 255) : 0; - InterpolateRow(dst_argb, src_argb + yi * (ptrdiff_t)src_stride, src_stride, - dst_width_bytes, yf); + yf = interpolate ? (int)((y64 >> 8) & 255) : 0; + InterpolateRow(dst_argb, src_argb + yi * (ptrdiff_t)src_stride, + interpolate_stride, dst_width_bytes, yf); dst_argb += dst_stride; y64 += dy; } @@ -1555,8 +1565,18 @@ void ScalePlaneVertical_16(int src_height, void (*InterpolateRow)(uint16_t* dst_argb, const uint16_t* src_argb, ptrdiff_t src_stride, int dst_width, int source_y_fraction) = InterpolateRow_16_C; - const int64_t max_y = - (src_height > 1) ? (((int64_t)src_height - 1) << 16) - 1 : 0; + const bool interpolate = filtering == kFilterBilinear; + int64_t max_y = 0; + if (src_height > 1) { + max_y = ((int64_t)src_height - 1) << 16; + if (interpolate) { + --max_y; + } + } + // Without vertical interpolation, InterpolateRow() copies one row. A zero + // stride keeps it from forming a second source row pointer past the end when + // y is clamped to the last row. + const ptrdiff_t interpolate_stride = interpolate ? src_stride : 0; int64_t y64 = y; int j; assert(wpp >= 1 && wpp <= 2); @@ -1600,9 +1620,9 @@ void ScalePlaneVertical_16(int src_height, y64 = max_y; } yi = (int)(y64 >> 16); - yf = filtering ? (int)((y64 >> 8) & 255) : 0; - InterpolateRow(dst_argb, src_argb + yi * (ptrdiff_t)src_stride, src_stride, - dst_width_words, yf); + yf = interpolate ? (int)((y64 >> 8) & 255) : 0; + InterpolateRow(dst_argb, src_argb + yi * (ptrdiff_t)src_stride, + interpolate_stride, dst_width_words, yf); dst_argb += dst_stride; y64 += dy; } diff --git a/unit_test/scale_plane_test.cc b/unit_test/scale_plane_test.cc index b04dda10c..ab521ebcf 100644 --- a/unit_test/scale_plane_test.cc +++ b/unit_test/scale_plane_test.cc @@ -570,6 +570,126 @@ TEST_F(LibYUVScaleTest, PlaneTest1_16_Box) { free_aligned_buffer_page_end(orig_pixels_alloc); } +TEST_F(LibYUVScaleTest, ScalePlaneVerticalPointUp) { + const int kWidth = 32; + const uint8_t kSentinel = 0xFF; + const uint8_t expected[] = {10, 10, 20, 30, 30}; + + // An extra sentinel row exposes accidental interpolation past the last valid + // row. 3->5 is the uneven ratio that used to drop the last source row. + align_buffer_page_end(src, kWidth * 4); + align_buffer_page_end(dst, kWidth * 5); + memset(src, kSentinel, kWidth * 4); + memset(dst, kSentinel, kWidth * 5); + memset(src, 10, kWidth); + memset(src + kWidth, 20, kWidth); + memset(src + 2 * kWidth, 30, kWidth); + + ASSERT_EQ(0, ScalePlane(src, kWidth, kWidth, 3, dst, kWidth, kWidth, 5, + kFilterNone)); + for (int y = 0; y < 5; ++y) { + for (int x = 0; x < kWidth; ++x) { + EXPECT_EQ(expected[y], dst[y * kWidth + x]); + } + } + + free_aligned_buffer_page_end(dst); + free_aligned_buffer_page_end(src); +} + +TEST_F(LibYUVScaleTest, ScalePlaneVerticalPointUp_16) { + const int kWidth = 32; + const uint16_t kSentinel = 0xFFFF; + const uint16_t expected[] = {10, 10, 20, 30, 30}; + + align_buffer_page_end(src_alloc, kWidth * 4 * 2); + align_buffer_page_end(dst_alloc, kWidth * 5 * 2); + uint16_t* src = reinterpret_cast(src_alloc); + uint16_t* dst = reinterpret_cast(dst_alloc); + for (int i = 0; i < kWidth * 4; ++i) { + src[i] = kSentinel; + } + for (int i = 0; i < kWidth * 5; ++i) { + dst[i] = kSentinel; + } + for (int x = 0; x < kWidth; ++x) { + src[x] = 10; + src[kWidth + x] = 20; + src[2 * kWidth + x] = 30; + } + + ASSERT_EQ(0, ScalePlane_16(src, kWidth, kWidth, 3, dst, kWidth, kWidth, 5, + kFilterNone)); + for (int y = 0; y < 5; ++y) { + for (int x = 0; x < kWidth; ++x) { + EXPECT_EQ(expected[y], dst[y * kWidth + x]); + } + } + + free_aligned_buffer_page_end(dst_alloc); + free_aligned_buffer_page_end(src_alloc); +} + +TEST_F(LibYUVScaleTest, ScalePlaneVerticalBilinearUp) { + const int kWidth = 32; + const uint8_t kSentinel = 0xFF; + // Same-width 2x upsampling still uses FixedDiv1. With y = 0 the 8.8 + // fractions are 0, 85, 170, 255. InterpolateRow_C: + // (a * (256 - yf) + b * yf + 128) >> 8. + // Nearest 2x would duplicate rows as {10, 10, 200, 200}. + const uint8_t expected[] = {10, 73, 136, 199}; + + align_buffer_page_end(src, kWidth * 3); + align_buffer_page_end(dst, kWidth * 4); + memset(src, kSentinel, kWidth * 3); + memset(dst, kSentinel, kWidth * 4); + memset(src, 10, kWidth); + memset(src + kWidth, 200, kWidth); + + ASSERT_EQ(0, ScalePlane(src, kWidth, kWidth, 2, dst, kWidth, kWidth, 4, + kFilterBilinear)); + for (int y = 0; y < 4; ++y) { + for (int x = 0; x < kWidth; ++x) { + EXPECT_EQ(expected[y], dst[y * kWidth + x]); + } + } + + free_aligned_buffer_page_end(dst); + free_aligned_buffer_page_end(src); +} + +TEST_F(LibYUVScaleTest, ScalePlaneVerticalBilinearUp_16) { + const int kWidth = 32; + const uint16_t kSentinel = 0xFFFF; + const uint16_t expected[] = {10, 73, 136, 199}; + + align_buffer_page_end(src_alloc, kWidth * 3 * 2); + align_buffer_page_end(dst_alloc, kWidth * 4 * 2); + uint16_t* src = reinterpret_cast(src_alloc); + uint16_t* dst = reinterpret_cast(dst_alloc); + for (int i = 0; i < kWidth * 3; ++i) { + src[i] = kSentinel; + } + for (int i = 0; i < kWidth * 4; ++i) { + dst[i] = kSentinel; + } + for (int x = 0; x < kWidth; ++x) { + src[x] = 10; + src[kWidth + x] = 200; + } + + ASSERT_EQ(0, ScalePlane_16(src, kWidth, kWidth, 2, dst, kWidth, kWidth, 4, + kFilterBilinear)); + for (int y = 0; y < 4; ++y) { + for (int x = 0; x < kWidth; ++x) { + EXPECT_EQ(expected[y], dst[y * kWidth + x]); + } + } + + free_aligned_buffer_page_end(dst_alloc); + free_aligned_buffer_page_end(src_alloc); +} + // POC: int * int overflow in ScalePlaneVertical (scale_common.cc). // // `yi * src_stride` is evaluated as int * int. When the product exceeds