SF: fix off-by one in luma mean calculation
There was an off by one error in luma mean calculation that could
lead to segfault. Flaw was figured out in writing the 5 attached
unit tests for the function. Condition would be obscure in the wild,
but an exactly half-white, half-another-color in the sampled region
could present condition.
Fixes: 129858549
Test: libsurfaceflinger_unittest (5 new tests)
Change-Id: I920cd9cac15122178ec9258e33c9bc35b1bb9357
diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp
index 718e996..35f11fc 100644
--- a/services/surfaceflinger/RegionSamplingThread.cpp
+++ b/services/surfaceflinger/RegionSamplingThread.cpp
@@ -266,6 +266,7 @@
constexpr auto rec709_blue_primary = 0.0722f;
return rec709_red_primary * r + rec709_green_primary * g + rec709_blue_primary * b;
}
+} // anonymous namespace
float sampleArea(const uint32_t* data, int32_t stride, const Rect& area) {
std::array<int32_t, 256> brightnessBuckets = {};
@@ -286,14 +287,13 @@
int32_t accumulated = 0;
size_t bucket = 0;
- while (bucket++ < brightnessBuckets.size()) {
+ for (; bucket < brightnessBuckets.size(); bucket++) {
accumulated += brightnessBuckets[bucket];
if (accumulated > majoritySampleNum) break;
}
return bucket / 255.0f;
}
-} // anonymous namespace
std::vector<float> RegionSamplingThread::sampleBuffer(
const sp<GraphicBuffer>& buffer, const Point& leftTop,
diff --git a/services/surfaceflinger/RegionSamplingThread.h b/services/surfaceflinger/RegionSamplingThread.h
index d4e57bf..9796429 100644
--- a/services/surfaceflinger/RegionSamplingThread.h
+++ b/services/surfaceflinger/RegionSamplingThread.h
@@ -37,6 +37,8 @@
class SurfaceFlinger;
struct SamplingOffsetCallback;
+float sampleArea(const uint32_t* data, int32_t stride, const Rect& area);
+
class RegionSamplingThread : public IBinder::DeathRecipient {
public:
struct TimingTunables {
diff --git a/services/surfaceflinger/tests/unittests/Android.bp b/services/surfaceflinger/tests/unittests/Android.bp
index 12b41fd..d4eac88 100644
--- a/services/surfaceflinger/tests/unittests/Android.bp
+++ b/services/surfaceflinger/tests/unittests/Android.bp
@@ -49,6 +49,7 @@
"SchedulerUtilsTest.cpp",
"RefreshRateConfigsTest.cpp",
"RefreshRateStatsTest.cpp",
+ "RegionSamplingTest.cpp",
"TimeStatsTest.cpp",
"mock/DisplayHardware/MockComposer.cpp",
"mock/DisplayHardware/MockDisplay.cpp",
diff --git a/services/surfaceflinger/tests/unittests/RegionSamplingTest.cpp b/services/surfaceflinger/tests/unittests/RegionSamplingTest.cpp
new file mode 100644
index 0000000..51d6d7e
--- /dev/null
+++ b/services/surfaceflinger/tests/unittests/RegionSamplingTest.cpp
@@ -0,0 +1,77 @@
+/*
+ * Copyright 2019 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#undef LOG_TAG
+#define LOG_TAG "RegionSamplingTest"
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+#include <array>
+#include <limits>
+
+#include "RegionSamplingThread.h"
+
+namespace android {
+
+struct RegionSamplingTest : testing::Test {
+public:
+ static uint32_t constexpr kBlack = 0;
+ static uint32_t constexpr kWhite = std::numeric_limits<uint32_t>::max();
+ static int constexpr kWidth = 98;
+ static int constexpr kStride = 100;
+ static int constexpr kHeight = 29;
+ std::array<uint32_t, kHeight * kStride> buffer;
+ Rect const whole_area{0, 0, kWidth, kHeight};
+};
+
+TEST_F(RegionSamplingTest, calculate_mean_white) {
+ std::fill(buffer.begin(), buffer.end(), kWhite);
+ EXPECT_THAT(sampleArea(buffer.data(), kStride, whole_area), testing::FloatEq(1.0f));
+}
+
+TEST_F(RegionSamplingTest, calculate_mean_black) {
+ std::fill(buffer.begin(), buffer.end(), kBlack);
+ EXPECT_THAT(sampleArea(buffer.data(), kStride, whole_area), testing::FloatEq(0.0f));
+}
+
+TEST_F(RegionSamplingTest, calculate_mean_partial_region) {
+ auto const halfway_down = kHeight >> 1;
+ auto const half = halfway_down * kStride;
+ Rect const partial_region = {whole_area.left, whole_area.top, whole_area.right,
+ whole_area.top + halfway_down};
+ std::fill(buffer.begin(), buffer.begin() + half, 0);
+ std::fill(buffer.begin() + half, buffer.end(), kWhite);
+ EXPECT_THAT(sampleArea(buffer.data(), kStride, partial_region), testing::FloatEq(0.0f));
+}
+
+TEST_F(RegionSamplingTest, calculate_mean_mixed_values) {
+ std::generate(buffer.begin(), buffer.end(), [n = 0]() mutable {
+ uint32_t const pixel = (n % std::numeric_limits<uint8_t>::max()) << ((n % 3) * CHAR_BIT);
+ n++;
+ return pixel;
+ });
+ EXPECT_THAT(sampleArea(buffer.data(), kStride, whole_area), testing::FloatNear(0.083f, 0.01f));
+}
+
+TEST_F(RegionSamplingTest, bimodal_tiebreaker) {
+ std::generate(buffer.begin(), buffer.end(),
+ [n = 0]() mutable { return (n++ % 2) ? kBlack : kWhite; });
+ // presently there's no tiebreaking strategy in place, accept either of the means
+ EXPECT_THAT(sampleArea(buffer.data(), kStride, whole_area),
+ testing::AnyOf(testing::FloatEq(1.0), testing::FloatEq(0.0f)));
+}
+
+} // namespace android