From 9b0d9534c75dc4d40edbf13938aa09c3eec737a1 Mon Sep 17 00:00:00 2001 From: Shuzhen Wang Date: Tue, 14 Apr 2020 17:14:20 -0700 Subject: [PATCH] Camera: Fix off-by-1 error for scaling crop region Make sure the scaling of coordinates based on zoomRatio is symmetric relative to the center of the image. And use the center of the pixel for calculation. Test: testZoomRatio, testDigitalZoom, cameraservice_test Test: vendor testing Bug: 153959755 Change-Id: I966bd31bde5afd50462a018590c7a4fefc3eaac6 --- .../device3/ZoomRatioMapper.cpp | 81 +++++++++++-------- .../device3/ZoomRatioMapper.h | 8 +- .../libcameraservice/tests/ZoomRatioTest.cpp | 81 ++++++++++--------- 3 files changed, 89 insertions(+), 81 deletions(-) diff --git a/services/camera/libcameraservice/device3/ZoomRatioMapper.cpp b/services/camera/libcameraservice/device3/ZoomRatioMapper.cpp index 84da45a069..a87de7742c 100644 --- a/services/camera/libcameraservice/device3/ZoomRatioMapper.cpp +++ b/services/camera/libcameraservice/device3/ZoomRatioMapper.cpp @@ -243,10 +243,15 @@ status_t ZoomRatioMapper::separateZoomFromCropLocked(CameraMetadata* metadata, b if (weight == 0) { continue; } - // Top-left is inclusively clamped - scaleCoordinates(entry.data.i32 + j, 1, zoomRatio, ClampInclusive); - // Bottom-right is exclusively clamped - scaleCoordinates(entry.data.i32 + j + 2, 1, zoomRatio, ClampExclusive); + // Top left (inclusive) + scaleCoordinates(entry.data.i32 + j, 1, zoomRatio, true /*clamp*/); + // Bottom right (exclusive): Use adjacent inclusive pixel to + // calculate. + entry.data.i32[j+2] -= 1; + entry.data.i32[j+3] -= 1; + scaleCoordinates(entry.data.i32 + j + 2, 1, zoomRatio, true /*clamp*/); + entry.data.i32[j+2] += 1; + entry.data.i32[j+3] += 1; } } @@ -258,7 +263,7 @@ status_t ZoomRatioMapper::separateZoomFromCropLocked(CameraMetadata* metadata, b if (isResult) { for (auto pts : kResultPointsToCorrectNoClamp) { entry = metadata->find(pts); - scaleCoordinates(entry.data.i32, entry.count / 2, zoomRatio, ClampOff); + scaleCoordinates(entry.data.i32, entry.count / 2, zoomRatio, false /*clamp*/); } } @@ -282,10 +287,15 @@ status_t ZoomRatioMapper::combineZoomAndCropLocked(CameraMetadata* metadata, boo if (weight == 0) { continue; } - // Top-left is inclusively clamped - scaleCoordinates(entry.data.i32 + j, 1, 1.0 / zoomRatio, ClampInclusive); - // Bottom-right is exclusively clamped - scaleCoordinates(entry.data.i32 + j + 2, 1, 1.0 / zoomRatio, ClampExclusive); + // Top-left (inclusive) + scaleCoordinates(entry.data.i32 + j, 1, 1.0 / zoomRatio, true /*clamp*/); + // Bottom-right (exclusive): Use adjacent inclusive pixel to + // calculate. + entry.data.i32[j+2] -= 1; + entry.data.i32[j+3] -= 1; + scaleCoordinates(entry.data.i32 + j + 2, 1, 1.0 / zoomRatio, true /*clamp*/); + entry.data.i32[j+2] += 1; + entry.data.i32[j+3] += 1; } } for (auto rect : kRectsToCorrect) { @@ -295,7 +305,7 @@ status_t ZoomRatioMapper::combineZoomAndCropLocked(CameraMetadata* metadata, boo if (isResult) { for (auto pts : kResultPointsToCorrectNoClamp) { entry = metadata->find(pts); - scaleCoordinates(entry.data.i32, entry.count / 2, 1.0 / zoomRatio, ClampOff); + scaleCoordinates(entry.data.i32, entry.count / 2, 1.0 / zoomRatio, false /*clamp*/); } } @@ -309,28 +319,31 @@ status_t ZoomRatioMapper::combineZoomAndCropLocked(CameraMetadata* metadata, boo } void ZoomRatioMapper::scaleCoordinates(int32_t* coordPairs, int coordCount, - float scaleRatio, ClampMode clamp) { + float scaleRatio, bool clamp) { + // A pixel's coordinate is represented by the position of its top-left corner. + // To avoid the rounding error, we use the coordinate for the center of the + // pixel instead: + // 1. First shift the coordinate system half pixel both horizontally and + // vertically, so that [x, y] is the center of the pixel, not the top-left corner. + // 2. Do zoom operation to scale the coordinate relative to the center of + // the active array (shifted by 0.5 pixel as well). + // 3. Shift the coordinate system back by directly using the pixel center + // coordinate. for (int i = 0; i < coordCount * 2; i += 2) { float x = coordPairs[i]; float y = coordPairs[i + 1]; - float xCentered = x - mArrayWidth / 2; - float yCentered = y - mArrayHeight / 2; + float xCentered = x - (mArrayWidth - 2) / 2; + float yCentered = y - (mArrayHeight - 2) / 2; float scaledX = xCentered * scaleRatio; float scaledY = yCentered * scaleRatio; - scaledX += mArrayWidth / 2; - scaledY += mArrayHeight / 2; + scaledX += (mArrayWidth - 2) / 2; + scaledY += (mArrayHeight - 2) / 2; + coordPairs[i] = static_cast(std::round(scaledX)); + coordPairs[i+1] = static_cast(std::round(scaledY)); // Clamp to within activeArray/preCorrectionActiveArray - coordPairs[i] = static_cast(scaledX); - coordPairs[i+1] = static_cast(scaledY); - if (clamp != ClampOff) { - int32_t right, bottom; - if (clamp == ClampInclusive) { - right = mArrayWidth - 1; - bottom = mArrayHeight - 1; - } else { - right = mArrayWidth; - bottom = mArrayHeight; - } + if (clamp) { + int32_t right = mArrayWidth - 1; + int32_t bottom = mArrayHeight - 1; coordPairs[i] = std::min(right, std::max(0, coordPairs[i])); coordPairs[i+1] = @@ -343,25 +356,25 @@ void ZoomRatioMapper::scaleCoordinates(int32_t* coordPairs, int coordCount, void ZoomRatioMapper::scaleRects(int32_t* rects, int rectCount, float scaleRatio) { for (int i = 0; i < rectCount * 4; i += 4) { - // Map from (l, t, width, height) to (l, t, r, b). - // [l, t] is inclusive, and [r, b] is exclusive. + // Map from (l, t, width, height) to (l, t, l+width-1, t+height-1), + // where both top-left and bottom-right are inclusive. int32_t coords[4] = { rects[i], rects[i + 1], - rects[i] + rects[i + 2], - rects[i + 1] + rects[i + 3] + rects[i] + rects[i + 2] - 1, + rects[i + 1] + rects[i + 3] - 1 }; // top-left - scaleCoordinates(coords, 1, scaleRatio, ClampInclusive); + scaleCoordinates(coords, 1, scaleRatio, true /*clamp*/); // bottom-right - scaleCoordinates(coords+2, 1, scaleRatio, ClampExclusive); + scaleCoordinates(coords+2, 1, scaleRatio, true /*clamp*/); // Map back to (l, t, width, height) rects[i] = coords[0]; rects[i + 1] = coords[1]; - rects[i + 2] = coords[2] - coords[0]; - rects[i + 3] = coords[3] - coords[1]; + rects[i + 2] = coords[2] - coords[0] + 1; + rects[i + 3] = coords[3] - coords[1] + 1; } } diff --git a/services/camera/libcameraservice/device3/ZoomRatioMapper.h b/services/camera/libcameraservice/device3/ZoomRatioMapper.h index aa3d9139ac..698f87fb22 100644 --- a/services/camera/libcameraservice/device3/ZoomRatioMapper.h +++ b/services/camera/libcameraservice/device3/ZoomRatioMapper.h @@ -65,14 +65,8 @@ class ZoomRatioMapper : private CoordinateMapper { status_t updateCaptureResult(CameraMetadata *request, bool requestedZoomRatioIs1); public: // Visible for testing. Do not use concurently. - enum ClampMode { - ClampOff, - ClampInclusive, - ClampExclusive, - }; - void scaleCoordinates(int32_t* coordPairs, int coordCount, - float scaleRatio, ClampMode clamp); + float scaleRatio, bool clamp); bool isValid() { return mIsValid; } private: diff --git a/services/camera/libcameraservice/tests/ZoomRatioTest.cpp b/services/camera/libcameraservice/tests/ZoomRatioTest.cpp index 300da099a3..4e949916b8 100644 --- a/services/camera/libcameraservice/tests/ZoomRatioTest.cpp +++ b/services/camera/libcameraservice/tests/ZoomRatioTest.cpp @@ -171,35 +171,35 @@ void subScaleCoordinatesTest(bool usePreCorrectArray) { std::array originalCoords = { 0, 0, // top-left - width, 0, // top-right - 0, height, // bottom-left - width, height, // bottom-right - width / 2, height / 2, // center - width / 4, height / 4, // top-left after 2x - width / 3, height * 2 / 3, // bottom-left after 3x zoom - width * 7 / 8, height / 2, // middle-right after 1.33x zoom + width - 1, 0, // top-right + 0, height - 1, // bottom-left + width - 1, height - 1, // bottom-right + (width - 1) / 2, (height - 1) / 2, // center + (width - 1) / 4, (height - 1) / 4, // top-left after 2x + (width - 1) / 3, (height - 1) * 2 / 3, // bottom-left after 3x zoom + (width - 1) * 7 / 8, (height - 1) / 2, // middle-right after 1.33x zoom }; // Verify 1.0x zoom doesn't change the coordinates auto coords = originalCoords; - mapper.scaleCoordinates(coords.data(), coords.size()/2, 1.0f, ZoomRatioMapper::ClampOff); + mapper.scaleCoordinates(coords.data(), coords.size()/2, 1.0f, false /*clamp*/); for (size_t i = 0; i < coords.size(); i++) { EXPECT_EQ(coords[i], originalCoords[i]); } // Verify 2.0x zoom work as expected (no clamping) std::array expected2xCoords = { - - width / 2.0f, - height / 2.0f,// top-left - width * 3 / 2.0f, - height / 2.0f, // top-right - - width / 2.0f, height * 3 / 2.0f, // bottom-left - width * 3 / 2.0f, height * 3 / 2.0f, // bottom-right - width / 2.0f, height / 2.0f, // center + - (width - 1) / 2.0f, - (height - 1) / 2.0f,// top-left + (width - 1) * 3 / 2.0f, - (height - 1) / 2.0f, // top-right + - (width - 1) / 2.0f, (height - 1) * 3 / 2.0f, // bottom-left + (width - 1) * 3 / 2.0f, (height - 1) * 3 / 2.0f, // bottom-right + (width - 1) / 2.0f, (height - 1) / 2.0f, // center 0, 0, // top-left after 2x - width / 6.0f, height - height / 6.0f, // bottom-left after 3x zoom - width + width / 4.0f, height / 2.0f, // middle-right after 1.33x zoom + (width - 1) / 6.0f, (height - 1) * 5.0f / 6.0f, // bottom-left after 3x zoom + (width - 1) * 5.0f / 4.0f, (height - 1) / 2.0f, // middle-right after 1.33x zoom }; coords = originalCoords; - mapper.scaleCoordinates(coords.data(), coords.size()/2, 2.0f, ZoomRatioMapper::ClampOff); + mapper.scaleCoordinates(coords.data(), coords.size()/2, 2.0f, false /*clamp*/); for (size_t i = 0; i < coords.size(); i++) { EXPECT_LE(std::abs(coords[i] - expected2xCoords[i]), kMaxAllowedPixelError); } @@ -207,16 +207,16 @@ void subScaleCoordinatesTest(bool usePreCorrectArray) { // Verify 2.0x zoom work as expected (with inclusive clamping) std::array expected2xCoordsClampedInc = { 0, 0, // top-left - static_cast(width) - 1, 0, // top-right - 0, static_cast(height) - 1, // bottom-left - static_cast(width) - 1, static_cast(height) - 1, // bottom-right - width / 2.0f, height / 2.0f, // center + width - 1.0f, 0, // top-right + 0, height - 1.0f, // bottom-left + width - 1.0f, height - 1.0f, // bottom-right + (width - 1) / 2.0f, (height - 1) / 2.0f, // center 0, 0, // top-left after 2x - width / 6.0f, height - height / 6.0f , // bottom-left after 3x zoom - static_cast(width) - 1, height / 2.0f, // middle-right after 1.33x zoom + (width - 1) / 6.0f, (height - 1) * 5.0f / 6.0f , // bottom-left after 3x zoom + width - 1.0f, (height - 1) / 2.0f, // middle-right after 1.33x zoom }; coords = originalCoords; - mapper.scaleCoordinates(coords.data(), coords.size()/2, 2.0f, ZoomRatioMapper::ClampInclusive); + mapper.scaleCoordinates(coords.data(), coords.size()/2, 2.0f, true /*clamp*/); for (size_t i = 0; i < coords.size(); i++) { EXPECT_LE(std::abs(coords[i] - expected2xCoordsClampedInc[i]), kMaxAllowedPixelError); } @@ -224,33 +224,33 @@ void subScaleCoordinatesTest(bool usePreCorrectArray) { // Verify 2.0x zoom work as expected (with exclusive clamping) std::array expected2xCoordsClampedExc = { 0, 0, // top-left - static_cast(width), 0, // top-right - 0, static_cast(height), // bottom-left - static_cast(width), static_cast(height), // bottom-right + width - 1.0f, 0, // top-right + 0, height - 1.0f, // bottom-left + width - 1.0f, height - 1.0f, // bottom-right width / 2.0f, height / 2.0f, // center 0, 0, // top-left after 2x - width / 6.0f, height - height / 6.0f , // bottom-left after 3x zoom - static_cast(width), height / 2.0f, // middle-right after 1.33x zoom + (width - 1) / 6.0f, (height - 1) * 5.0f / 6.0f , // bottom-left after 3x zoom + width - 1.0f, height / 2.0f, // middle-right after 1.33x zoom }; coords = originalCoords; - mapper.scaleCoordinates(coords.data(), coords.size()/2, 2.0f, ZoomRatioMapper::ClampExclusive); + mapper.scaleCoordinates(coords.data(), coords.size()/2, 2.0f, true /*clamp*/); for (size_t i = 0; i < coords.size(); i++) { EXPECT_LE(std::abs(coords[i] - expected2xCoordsClampedExc[i]), kMaxAllowedPixelError); } // Verify 0.33x zoom work as expected std::array expectedZoomOutCoords = { - width / 3.0f, height / 3.0f, // top-left - width * 2 / 3.0f, height / 3.0f, // top-right - width / 3.0f, height * 2 / 3.0f, // bottom-left - width * 2 / 3.0f, height * 2 / 3.0f, // bottom-right - width / 2.0f, height / 2.0f, // center - width * 5 / 12.0f, height * 5 / 12.0f, // top-left after 2x - width * 4 / 9.0f, height * 5 / 9.0f, // bottom-left after 3x zoom-in - width * 5 / 8.0f, height / 2.0f, // middle-right after 1.33x zoom-in + (width - 1) / 3.0f, (height - 1) / 3.0f, // top-left + (width - 1) * 2 / 3.0f, (height - 1) / 3.0f, // top-right + (width - 1) / 3.0f, (height - 1) * 2 / 3.0f, // bottom-left + (width - 1) * 2 / 3.0f, (height - 1) * 2 / 3.0f, // bottom-right + (width - 1) / 2.0f, (height - 1) / 2.0f, // center + (width - 1) * 5 / 12.0f, (height - 1) * 5 / 12.0f, // top-left after 2x + (width - 1) * 4 / 9.0f, (height - 1) * 5 / 9.0f, // bottom-left after 3x zoom-in + (width - 1) * 5 / 8.0f, (height - 1) / 2.0f, // middle-right after 1.33x zoom-in }; coords = originalCoords; - mapper.scaleCoordinates(coords.data(), coords.size()/2, 1.0f/3, ZoomRatioMapper::ClampOff); + mapper.scaleCoordinates(coords.data(), coords.size()/2, 1.0f/3, false /*clamp*/); for (size_t i = 0; i < coords.size(); i++) { EXPECT_LE(std::abs(coords[i] - expectedZoomOutCoords[i]), kMaxAllowedPixelError); } @@ -323,7 +323,8 @@ void subCropOverZoomRangeTest(bool usePreCorrectArray) { entry = metadata.find(ANDROID_SCALER_CROP_REGION); ASSERT_EQ(entry.count, 4U); for (int i = 0; i < 4; i++) { - EXPECT_EQ(entry.data.i32[i], testDefaultCropSize[index][i]); + EXPECT_LE(std::abs(entry.data.i32[i] - testDefaultCropSize[index][i]), + kMaxAllowedPixelError); } entry = metadata.find(ANDROID_CONTROL_ZOOM_RATIO); EXPECT_NEAR(entry.data.f[0], 2.0f, kMaxAllowedRatioError); @@ -335,7 +336,7 @@ void subCropOverZoomRangeTest(bool usePreCorrectArray) { entry = metadata.find(ANDROID_SCALER_CROP_REGION); ASSERT_EQ(entry.count, 4U); for (int i = 0; i < 4; i++) { - EXPECT_EQ(entry.data.i32[i], test2xCropRegion[index][i]); + EXPECT_LE(std::abs(entry.data.i32[i] - test2xCropRegion[index][i]), kMaxAllowedPixelError); } // Letter boxing crop region, zoomRatio is 1.0