From ec460089769f4faa196a4c253b569ef0c35faf72 Mon Sep 17 00:00:00 2001 From: Dan Stoza Date: Mon, 17 Dec 2018 15:35:09 -0800 Subject: [sf] Implement addSamplingListener Implements ISurfaceComposer::addSamplingListener, which allows a client to receive streaming median luma updates for a given region of the screen. Bug: 119639245 Test: Manual using SamplingDemo in libgui Test: Automated libgui_test in Ic85a97f475a3414a79d3719bbd0b2b648bbccfb0 Change-Id: Ic52359aeab884e734a806372be0eb4e327c45298 --- services/surfaceflinger/RegionSamplingThread.cpp | 252 +++++++++++++++++++++++ 1 file changed, 252 insertions(+) create mode 100644 services/surfaceflinger/RegionSamplingThread.cpp (limited to 'services/surfaceflinger/RegionSamplingThread.cpp') diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp new file mode 100644 index 0000000000..cbc7ada9b0 --- /dev/null +++ b/services/surfaceflinger/RegionSamplingThread.cpp @@ -0,0 +1,252 @@ +/* + * 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. + */ + +//#define LOG_NDEBUG 0 +#define ATRACE_TAG ATRACE_TAG_GRAPHICS +#undef LOG_TAG +#define LOG_TAG "RegionSamplingThread" + +#include "RegionSamplingThread.h" + +#include +#include + +#include "DisplayDevice.h" +#include "Layer.h" +#include "SurfaceFlinger.h" + +namespace android { + +template +struct SpHash { + size_t operator()(const sp& p) const { return std::hash()(p.get()); } +}; + +RegionSamplingThread::RegionSamplingThread(SurfaceFlinger& flinger) : mFlinger(flinger) { + std::lock_guard threadLock(mThreadMutex); + mThread = std::thread([this]() { threadMain(); }); + pthread_setname_np(mThread.native_handle(), "RegionSamplingThread"); +} + +RegionSamplingThread::~RegionSamplingThread() { + { + std::lock_guard lock(mMutex); + mRunning = false; + mCondition.notify_one(); + } + + std::lock_guard threadLock(mThreadMutex); + if (mThread.joinable()) { + mThread.join(); + } +} + +void RegionSamplingThread::addListener(const Rect& samplingArea, const sp& stopLayerHandle, + const sp& listener) { + wp stopLayer = stopLayerHandle != nullptr + ? static_cast(stopLayerHandle.get())->owner + : nullptr; + + sp asBinder = IInterface::asBinder(listener); + asBinder->linkToDeath(this); + std::lock_guard lock(mMutex); + mDescriptors.emplace(wp(asBinder), Descriptor{samplingArea, stopLayer, listener}); +} + +void RegionSamplingThread::removeListener(const sp& listener) { + std::lock_guard lock(mMutex); + mDescriptors.erase(wp(IInterface::asBinder(listener))); +} + +void RegionSamplingThread::sampleNow() { + std::lock_guard lock(mMutex); + mSampleRequested = true; + mCondition.notify_one(); +} + +void RegionSamplingThread::binderDied(const wp& who) { + std::lock_guard lock(mMutex); + mDescriptors.erase(who); +} + +namespace { +// Using Rec. 709 primaries +float getLuma(float r, float g, float b) { + constexpr auto rec709_red_primary = 0.2126f; + constexpr auto rec709_green_primary = 0.7152f; + constexpr auto rec709_blue_primary = 0.0722f; + return rec709_red_primary * r + rec709_green_primary * g + rec709_blue_primary * b; +} + +float sampleArea(const uint32_t* data, int32_t stride, const Rect& area) { + std::array brightnessBuckets = {}; + const int32_t majoritySampleNum = area.getWidth() * area.getHeight() / 2; + + for (int32_t row = area.top; row < area.bottom; ++row) { + const uint32_t* rowBase = data + row * stride; + for (int32_t column = area.left; column < area.right; ++column) { + uint32_t pixel = rowBase[column]; + const float r = (pixel & 0xFF) / 255.0f; + const float g = ((pixel >> 8) & 0xFF) / 255.0f; + const float b = ((pixel >> 16) & 0xFF) / 255.0f; + const uint8_t luma = std::round(getLuma(r, g, b) * 255.0f); + ++brightnessBuckets[luma]; + if (brightnessBuckets[luma] > majoritySampleNum) return luma / 255.0f; + } + } + + int32_t accumulated = 0; + size_t bucket = 0; + while (bucket++ < brightnessBuckets.size()) { + accumulated += brightnessBuckets[bucket]; + if (accumulated > majoritySampleNum) break; + } + + return bucket / 255.0f; +} +} // anonymous namespace + +std::vector sampleBuffer(const sp& buffer, const Point& leftTop, + const std::vector& descriptors) { + void* data_raw = nullptr; + buffer->lock(GRALLOC_USAGE_SW_READ_OFTEN, &data_raw); + std::shared_ptr data(reinterpret_cast(data_raw), + [&buffer](auto) { buffer->unlock(); }); + if (!data) return {}; + + const int32_t stride = buffer->getStride(); + std::vector lumas(descriptors.size()); + std::transform(descriptors.begin(), descriptors.end(), lumas.begin(), + [&](auto const& descriptor) { + return sampleArea(data.get(), stride, descriptor.area - leftTop); + }); + return lumas; +} + +void RegionSamplingThread::captureSample() { + ATRACE_CALL(); + + if (mDescriptors.empty()) { + return; + } + + std::vector descriptors; + Region sampleRegion; + for (const auto& [listener, descriptor] : mDescriptors) { + sampleRegion.orSelf(descriptor.area); + descriptors.emplace_back(descriptor); + } + + Rect sampledArea = sampleRegion.bounds(); + + sp device = mFlinger.getDefaultDisplayDevice(); + DisplayRenderArea renderArea(device, sampledArea, sampledArea.getWidth(), + sampledArea.getHeight(), ui::Dataspace::V0_SRGB, + ui::Transform::ROT_0); + + std::unordered_set, SpHash> listeners; + + auto traverseLayers = [&](const LayerVector::Visitor& visitor) { + bool stopLayerFound = false; + auto filterVisitor = [&](Layer* layer) { + // We don't want to capture any layers beyond the stop layer + if (stopLayerFound) return; + + // Likewise if we just found a stop layer, set the flag and abort + for (const auto& [area, stopLayer, listener] : descriptors) { + if (layer == stopLayer.promote().get()) { + stopLayerFound = true; + return; + } + } + + // Compute the layer's position on the screen + Rect bounds = Rect(layer->getBounds()); + ui::Transform transform = layer->getTransform(); + constexpr bool roundOutwards = true; + Rect transformed = transform.transform(bounds, roundOutwards); + + // If this layer doesn't intersect with the larger sampledArea, skip capturing it + Rect ignore; + if (!transformed.intersect(sampledArea, &ignore)) return; + + // If the layer doesn't intersect a sampling area, skip capturing it + bool intersectsAnyArea = false; + for (const auto& [area, stopLayer, listener] : descriptors) { + if (transformed.intersect(area, &ignore)) { + intersectsAnyArea = true; + listeners.insert(listener); + } + } + if (!intersectsAnyArea) return; + + ALOGV("Traversing [%s] [%d, %d, %d, %d]", layer->getName().string(), bounds.left, + bounds.top, bounds.right, bounds.bottom); + visitor(layer); + }; + mFlinger.traverseLayersInDisplay(device, filterVisitor); + }; + + const uint32_t usage = GRALLOC_USAGE_SW_READ_OFTEN | GRALLOC_USAGE_HW_RENDER; + sp buffer = + new GraphicBuffer(sampledArea.getWidth(), sampledArea.getHeight(), + PIXEL_FORMAT_RGBA_8888, 1, usage, "RegionSamplingThread"); + + // When calling into SF, we post a message into the SF message queue (so the + // screen capture runs on the main thread). This message blocks until the + // screenshot is actually captured, but before the capture occurs, the main + // thread may perform a normal refresh cycle. At the end of this cycle, it + // can request another sample (because layers changed), which triggers a + // call into sampleNow. When sampleNow attempts to grab the mutex, we can + // deadlock. + // + // To avoid this, we drop the mutex while we call into SF. + mMutex.unlock(); + mFlinger.captureScreenCore(renderArea, traverseLayers, buffer, false); + mMutex.lock(); + + std::vector activeDescriptors; + for (const auto& descriptor : descriptors) { + if (listeners.count(descriptor.listener) != 0) { + activeDescriptors.emplace_back(descriptor); + } + } + + ALOGV("Sampling %zu descriptors", activeDescriptors.size()); + std::vector lumas = sampleBuffer(buffer, sampledArea.leftTop(), activeDescriptors); + + if (lumas.size() != activeDescriptors.size()) { + return; + } + + for (size_t d = 0; d < activeDescriptors.size(); ++d) { + activeDescriptors[d].listener->onSampleCollected(lumas[d]); + } +} + +void RegionSamplingThread::threadMain() { + std::lock_guard lock(mMutex); + while (mRunning) { + if (mSampleRequested) { + mSampleRequested = false; + captureSample(); + } + mCondition.wait(mMutex, + [this]() REQUIRES(mMutex) { return mSampleRequested || !mRunning; }); + } +} + +} // namespace android -- cgit v1.2.3-59-g8ed1b From 7cbcc3710179fc47665a3e71d23661f1ee8b22f4 Mon Sep 17 00:00:00 2001 From: Kevin DuBois Date: Mon, 25 Feb 2019 14:53:28 -0800 Subject: SF: tidy comments for luma sampling. Tidying of const, comment and one internal fn name for Region Luma sampling. Test: libgui_test --gtest_filter="RegionSamp*" Change-Id: I46895c8ed5b2e2045d7ce7444085585b62b5ab6e --- services/surfaceflinger/RegionSamplingThread.cpp | 15 +++++++++------ services/surfaceflinger/RegionSamplingThread.h | 9 ++++++++- services/surfaceflinger/SurfaceFlinger.cpp | 10 +++++----- services/surfaceflinger/SurfaceFlinger.h | 4 ++-- 4 files changed, 24 insertions(+), 14 deletions(-) (limited to 'services/surfaceflinger/RegionSamplingThread.cpp') diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp index cbc7ada9b0..bd8548c8da 100644 --- a/services/surfaceflinger/RegionSamplingThread.cpp +++ b/services/surfaceflinger/RegionSamplingThread.cpp @@ -119,8 +119,9 @@ float sampleArea(const uint32_t* data, int32_t stride, const Rect& area) { } } // anonymous namespace -std::vector sampleBuffer(const sp& buffer, const Point& leftTop, - const std::vector& descriptors) { +std::vector RegionSamplingThread::sampleBuffer( + const sp& buffer, const Point& leftTop, + const std::vector& descriptors) { void* data_raw = nullptr; buffer->lock(GRALLOC_USAGE_SW_READ_OFTEN, &data_raw); std::shared_ptr data(reinterpret_cast(data_raw), @@ -150,7 +151,7 @@ void RegionSamplingThread::captureSample() { descriptors.emplace_back(descriptor); } - Rect sampledArea = sampleRegion.bounds(); + const Rect sampledArea = sampleRegion.bounds(); sp device = mFlinger.getDefaultDisplayDevice(); DisplayRenderArea renderArea(device, sampledArea, sampledArea.getWidth(), @@ -174,8 +175,8 @@ void RegionSamplingThread::captureSample() { } // Compute the layer's position on the screen - Rect bounds = Rect(layer->getBounds()); - ui::Transform transform = layer->getTransform(); + const Rect bounds = Rect(layer->getBounds()); + const ui::Transform transform = layer->getTransform(); constexpr bool roundOutwards = true; Rect transformed = transform.transform(bounds, roundOutwards); @@ -215,7 +216,7 @@ void RegionSamplingThread::captureSample() { // // To avoid this, we drop the mutex while we call into SF. mMutex.unlock(); - mFlinger.captureScreenCore(renderArea, traverseLayers, buffer, false); + mFlinger.captureScreenCommon(renderArea, traverseLayers, buffer, false); mMutex.lock(); std::vector activeDescriptors; @@ -229,6 +230,8 @@ void RegionSamplingThread::captureSample() { std::vector lumas = sampleBuffer(buffer, sampledArea.leftTop(), activeDescriptors); if (lumas.size() != activeDescriptors.size()) { + ALOGW("collected %zu median luma values for %zu descriptors", lumas.size(), + activeDescriptors.size()); return; } diff --git a/services/surfaceflinger/RegionSamplingThread.h b/services/surfaceflinger/RegionSamplingThread.h index bf59007465..ab06513be3 100644 --- a/services/surfaceflinger/RegionSamplingThread.h +++ b/services/surfaceflinger/RegionSamplingThread.h @@ -38,11 +38,16 @@ public: explicit RegionSamplingThread(SurfaceFlinger& flinger); ~RegionSamplingThread(); + // Add a listener to receive luma notifications. The luma reported via listener will + // report the median luma for the layers under the stopLayerHandle, in the samplingArea region. void addListener(const Rect& samplingArea, const sp& stopLayerHandle, const sp& listener); + // Remove the listener to stop receiving median luma notifications. void removeListener(const sp& listener); + // Instruct the thread to perform a median luma sampling on the layers. void sampleNow(); +private: struct Descriptor { Rect area = Rect::EMPTY_RECT; wp stopLayer; @@ -54,8 +59,10 @@ public: return std::hash()(p.unsafe_get()); } }; + std::vector sampleBuffer( + const sp& buffer, const Point& leftTop, + const std::vector& descriptors); -private: void binderDied(const wp& who) override; void captureSample() REQUIRES(mMutex); diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index df558e5e82..a59d45a181 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -5485,13 +5485,13 @@ status_t SurfaceFlinger::captureScreenCommon(RenderArea& renderArea, static_cast(reqPixelFormat), 1, usage, "screenshot"); - return captureScreenCore(renderArea, traverseLayers, *outBuffer, useIdentityTransform); + return captureScreenCommon(renderArea, traverseLayers, *outBuffer, useIdentityTransform); } -status_t SurfaceFlinger::captureScreenCore(RenderArea& renderArea, - TraverseLayersFunction traverseLayers, - const sp& buffer, - bool useIdentityTransform) { +status_t SurfaceFlinger::captureScreenCommon(RenderArea& renderArea, + TraverseLayersFunction traverseLayers, + const sp& buffer, + bool useIdentityTransform) { // This mutex protects syncFd and captureResult for communication of the return values from the // main thread back to this Binder thread std::mutex captureMutex; diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 31b4fb6232..deb78702ce 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -635,8 +635,8 @@ private: status_t captureScreenCommon(RenderArea& renderArea, TraverseLayersFunction traverseLayers, sp* outBuffer, const ui::PixelFormat reqPixelFormat, bool useIdentityTransform); - status_t captureScreenCore(RenderArea& renderArea, TraverseLayersFunction traverseLayers, - const sp& buffer, bool useIdentityTransform); + status_t captureScreenCommon(RenderArea& renderArea, TraverseLayersFunction traverseLayers, + const sp& buffer, bool useIdentityTransform); status_t captureScreenImplLocked(const RenderArea& renderArea, TraverseLayersFunction traverseLayers, ANativeWindowBuffer* buffer, bool useIdentityTransform, -- cgit v1.2.3-59-g8ed1b From 413287fa00fc9bbeb7246b88336c3f1a28cf922a Mon Sep 17 00:00:00 2001 From: Kevin DuBois Date: Mon, 25 Feb 2019 08:46:47 -0800 Subject: SF: rate-limit luma sampling Instead of sampling the luma regions every frame, introduce a rate limiting system to reduce load. Introduces a few tunables to control the rate, which is defaulted to 10Hz, when there is content being watched for luma. Test: manual systrace inspection, using SamplingDemo Test: libgui_test --gtest_filter="RegionSampling*" Test: atest CompositionSamplingListenerTest Fixes: 126747045 Change-Id: I7cae3e90fb405ba72dc2f276a88be48f1533a219 --- services/surfaceflinger/RegionSamplingThread.cpp | 185 ++++++++++++++++++++++- services/surfaceflinger/RegionSamplingThread.h | 46 +++++- services/surfaceflinger/Scheduler/Scheduler.cpp | 4 + services/surfaceflinger/Scheduler/Scheduler.h | 4 + services/surfaceflinger/SurfaceFlinger.cpp | 8 +- services/surfaceflinger/SurfaceFlinger.h | 6 +- 6 files changed, 240 insertions(+), 13 deletions(-) (limited to 'services/surfaceflinger/RegionSamplingThread.cpp') diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp index bd8548c8da..4f0b3bbbcf 100644 --- a/services/surfaceflinger/RegionSamplingThread.cpp +++ b/services/surfaceflinger/RegionSamplingThread.cpp @@ -21,27 +21,168 @@ #include "RegionSamplingThread.h" +#include #include #include +#include #include "DisplayDevice.h" #include "Layer.h" #include "SurfaceFlinger.h" namespace android { +using namespace std::chrono_literals; template struct SpHash { size_t operator()(const sp& p) const { return std::hash()(p.get()); } }; -RegionSamplingThread::RegionSamplingThread(SurfaceFlinger& flinger) : mFlinger(flinger) { - std::lock_guard threadLock(mThreadMutex); - mThread = std::thread([this]() { threadMain(); }); - pthread_setname_np(mThread.native_handle(), "RegionSamplingThread"); +constexpr auto lumaSamplingStepTag = "LumaSamplingStep"; +enum class samplingStep { + noWorkNeeded, + idleTimerWaiting, + waitForZeroPhase, + waitForSamplePhase, + sample +}; + +constexpr auto defaultRegionSamplingOffset = -3ms; +constexpr auto defaultRegionSamplingPeriod = 100ms; +constexpr auto defaultRegionSamplingTimerTimeout = 100ms; +// TODO: (b/127403193) duration to string conversion could probably be constexpr +template +inline std::string toNsString(std::chrono::duration t) { + return std::to_string(std::chrono::duration_cast(t).count()); +} + +RegionSamplingThread::EnvironmentTimingTunables::EnvironmentTimingTunables() { + char value[PROPERTY_VALUE_MAX] = {}; + + property_get("debug.sf.region_sampling_offset_ns", value, + toNsString(defaultRegionSamplingOffset).c_str()); + int const samplingOffsetNsRaw = atoi(value); + + property_get("debug.sf.region_sampling_period_ns", value, + toNsString(defaultRegionSamplingPeriod).c_str()); + int const samplingPeriodNsRaw = atoi(value); + + property_get("debug.sf.region_sampling_timer_timeout_ns", value, + toNsString(defaultRegionSamplingTimerTimeout).c_str()); + int const samplingTimerTimeoutNsRaw = atoi(value); + + if ((samplingPeriodNsRaw < 0) || (samplingTimerTimeoutNsRaw < 0)) { + ALOGW("User-specified sampling tuning options nonsensical. Using defaults"); + mSamplingOffset = defaultRegionSamplingOffset; + mSamplingPeriod = defaultRegionSamplingPeriod; + mSamplingTimerTimeout = defaultRegionSamplingTimerTimeout; + } else { + mSamplingOffset = std::chrono::nanoseconds(samplingOffsetNsRaw); + mSamplingPeriod = std::chrono::nanoseconds(samplingPeriodNsRaw); + mSamplingTimerTimeout = std::chrono::nanoseconds(samplingTimerTimeoutNsRaw); + } +} + +struct SamplingOffsetCallback : DispSync::Callback { + SamplingOffsetCallback(RegionSamplingThread& samplingThread, Scheduler& scheduler, + std::chrono::nanoseconds targetSamplingOffset) + : mRegionSamplingThread(samplingThread), + mScheduler(scheduler), + mTargetSamplingOffset(targetSamplingOffset) {} + + ~SamplingOffsetCallback() { stopVsyncListener(); } + + SamplingOffsetCallback(const SamplingOffsetCallback&) = delete; + SamplingOffsetCallback& operator=(const SamplingOffsetCallback&) = delete; + + void startVsyncListener() { + std::lock_guard lock(mMutex); + if (mVsyncListening) return; + + mPhaseIntervalSetting = Phase::ZERO; + mScheduler.withPrimaryDispSync([this](android::DispSync& sync) { + sync.addEventListener("SamplingThreadDispSyncListener", 0, this); + }); + mVsyncListening = true; + } + + void stopVsyncListener() { + std::lock_guard lock(mMutex); + stopVsyncListenerLocked(); + } + +private: + void stopVsyncListenerLocked() /*REQUIRES(mMutex)*/ { + if (!mVsyncListening) return; + + mScheduler.withPrimaryDispSync( + [this](android::DispSync& sync) { sync.removeEventListener(this); }); + mVsyncListening = false; + } + + void onDispSyncEvent(nsecs_t /* when */) final { + std::unique_lock lock(mMutex); + + if (mPhaseIntervalSetting == Phase::ZERO) { + ATRACE_INT(lumaSamplingStepTag, static_cast(samplingStep::waitForSamplePhase)); + mPhaseIntervalSetting = Phase::SAMPLING; + mScheduler.withPrimaryDispSync([this](android::DispSync& sync) { + sync.changePhaseOffset(this, mTargetSamplingOffset.count()); + }); + return; + } + + if (mPhaseIntervalSetting == Phase::SAMPLING) { + mPhaseIntervalSetting = Phase::ZERO; + mScheduler.withPrimaryDispSync( + [this](android::DispSync& sync) { sync.changePhaseOffset(this, 0); }); + stopVsyncListenerLocked(); + lock.unlock(); + mRegionSamplingThread.notifySamplingOffset(); + return; + } + } + + RegionSamplingThread& mRegionSamplingThread; + Scheduler& mScheduler; + const std::chrono::nanoseconds mTargetSamplingOffset; + mutable std::mutex mMutex; + enum class Phase { + ZERO, + SAMPLING + } mPhaseIntervalSetting /*GUARDED_BY(mMutex) macro doesnt work with unique_lock?*/ + = Phase::ZERO; + bool mVsyncListening /*GUARDED_BY(mMutex)*/ = false; +}; + +RegionSamplingThread::RegionSamplingThread(SurfaceFlinger& flinger, Scheduler& scheduler, + const TimingTunables& tunables) + : mFlinger(flinger), + mScheduler(scheduler), + mTunables(tunables), + mIdleTimer(std::chrono::duration_cast( + mTunables.mSamplingTimerTimeout), + [] {}, [this] { checkForStaleLuma(); }), + mPhaseCallback(std::make_unique(*this, mScheduler, + tunables.mSamplingOffset)), + lastSampleTime(0ns) { + { + std::lock_guard threadLock(mThreadMutex); + mThread = std::thread([this]() { threadMain(); }); + pthread_setname_np(mThread.native_handle(), "RegionSamplingThread"); + } + mIdleTimer.start(); } +RegionSamplingThread::RegionSamplingThread(SurfaceFlinger& flinger, Scheduler& scheduler) + : RegionSamplingThread(flinger, scheduler, + TimingTunables{defaultRegionSamplingOffset, + defaultRegionSamplingPeriod, + defaultRegionSamplingTimerTimeout}) {} + RegionSamplingThread::~RegionSamplingThread() { + mIdleTimer.stop(); + { std::lock_guard lock(mMutex); mRunning = false; @@ -71,8 +212,41 @@ void RegionSamplingThread::removeListener(const sp& lis mDescriptors.erase(wp(IInterface::asBinder(listener))); } -void RegionSamplingThread::sampleNow() { +void RegionSamplingThread::checkForStaleLuma() { + std::lock_guard lock(mMutex); + + if (mDiscardedFrames) { + ATRACE_INT(lumaSamplingStepTag, static_cast(samplingStep::waitForZeroPhase)); + mDiscardedFrames = false; + mPhaseCallback->startVsyncListener(); + } +} + +void RegionSamplingThread::notifyNewContent() { + doSample(); +} + +void RegionSamplingThread::notifySamplingOffset() { + doSample(); +} + +void RegionSamplingThread::doSample() { std::lock_guard lock(mMutex); + auto now = std::chrono::nanoseconds(systemTime(SYSTEM_TIME_MONOTONIC)); + if (lastSampleTime + mTunables.mSamplingPeriod > now) { + ATRACE_INT(lumaSamplingStepTag, static_cast(samplingStep::idleTimerWaiting)); + mDiscardedFrames = true; + return; + } + + ATRACE_INT(lumaSamplingStepTag, static_cast(samplingStep::sample)); + + mDiscardedFrames = false; + lastSampleTime = now; + + mIdleTimer.reset(); + mPhaseCallback->stopVsyncListener(); + mSampleRequested = true; mCondition.notify_one(); } @@ -238,6 +412,7 @@ void RegionSamplingThread::captureSample() { for (size_t d = 0; d < activeDescriptors.size(); ++d) { activeDescriptors[d].listener->onSampleCollected(lumas[d]); } + ATRACE_INT(lumaSamplingStepTag, static_cast(samplingStep::noWorkNeeded)); } void RegionSamplingThread::threadMain() { diff --git a/services/surfaceflinger/RegionSamplingThread.h b/services/surfaceflinger/RegionSamplingThread.h index ab06513be3..d4e57bfc7b 100644 --- a/services/surfaceflinger/RegionSamplingThread.h +++ b/services/surfaceflinger/RegionSamplingThread.h @@ -16,6 +16,7 @@ #pragma once +#include #include #include #include @@ -25,17 +26,42 @@ #include #include #include +#include "Scheduler/IdleTimer.h" namespace android { class GraphicBuffer; class IRegionSamplingListener; class Layer; +class Scheduler; class SurfaceFlinger; +struct SamplingOffsetCallback; class RegionSamplingThread : public IBinder::DeathRecipient { public: - explicit RegionSamplingThread(SurfaceFlinger& flinger); + struct TimingTunables { + // debug.sf.sampling_offset_ns + // When asynchronously collecting sample, the offset, from zero phase in the vsync timeline + // at which the sampling should start. + std::chrono::nanoseconds mSamplingOffset; + // debug.sf.sampling_period_ns + // This is the maximum amount of time the luma recieving client + // should have to wait for a new luma value after a frame is updated. The inverse of this is + // roughly the sampling rate. Sampling system rounds up sub-vsync sampling period to vsync + // period. + std::chrono::nanoseconds mSamplingPeriod; + // debug.sf.sampling_timer_timeout_ns + // This is the interval at which the luma sampling system will check that the luma clients + // have up to date information. It defaults to the mSamplingPeriod. + std::chrono::nanoseconds mSamplingTimerTimeout; + }; + struct EnvironmentTimingTunables : TimingTunables { + EnvironmentTimingTunables(); + }; + explicit RegionSamplingThread(SurfaceFlinger& flinger, Scheduler& scheduler, + const TimingTunables& tunables); + explicit RegionSamplingThread(SurfaceFlinger& flinger, Scheduler& scheduler); + ~RegionSamplingThread(); // Add a listener to receive luma notifications. The luma reported via listener will @@ -44,8 +70,13 @@ public: const sp& listener); // Remove the listener to stop receiving median luma notifications. void removeListener(const sp& listener); - // Instruct the thread to perform a median luma sampling on the layers. - void sampleNow(); + + // Notifies sampling engine that new content is available. This will trigger a sampling + // pass at some point in the future. + void notifyNewContent(); + + // Notifies the sampling engine that it has a good timing window in which to sample. + void notifySamplingOffset(); private: struct Descriptor { @@ -63,12 +94,19 @@ private: const sp& buffer, const Point& leftTop, const std::vector& descriptors); + void doSample(); void binderDied(const wp& who) override; + void checkForStaleLuma(); void captureSample() REQUIRES(mMutex); void threadMain(); SurfaceFlinger& mFlinger; + Scheduler& mScheduler; + const TimingTunables mTunables; + scheduler::IdleTimer mIdleTimer; + + std::unique_ptr const mPhaseCallback; std::mutex mThreadMutex; std::thread mThread GUARDED_BY(mThreadMutex); @@ -79,6 +117,8 @@ private: bool mSampleRequested GUARDED_BY(mMutex) = false; std::unordered_map, Descriptor, WpHash> mDescriptors GUARDED_BY(mMutex); + std::chrono::nanoseconds lastSampleTime GUARDED_BY(mMutex); + bool mDiscardedFrames GUARDED_BY(mMutex) = false; }; } // namespace android diff --git a/services/surfaceflinger/Scheduler/Scheduler.cpp b/services/surfaceflinger/Scheduler/Scheduler.cpp index 0ba6cf97e3..0063c8a202 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.cpp +++ b/services/surfaceflinger/Scheduler/Scheduler.cpp @@ -304,6 +304,10 @@ void Scheduler::addNativeWindowApi(int apiId) { mApiHistoryCounter = mApiHistoryCounter % scheduler::ARRAY_SIZE; } +void Scheduler::withPrimaryDispSync(std::function const& fn) { + fn(*mPrimaryDispSync); +} + void Scheduler::updateFpsBasedOnNativeWindowApi() { int mode; { diff --git a/services/surfaceflinger/Scheduler/Scheduler.h b/services/surfaceflinger/Scheduler/Scheduler.h index 2582c93d04..73896d5763 100644 --- a/services/surfaceflinger/Scheduler/Scheduler.h +++ b/services/surfaceflinger/Scheduler/Scheduler.h @@ -17,6 +17,7 @@ #pragma once #include +#include #include #include @@ -104,6 +105,9 @@ public: // Getter methods. EventThread* getEventThread(const sp& handle); + // Provides access to the DispSync object for the primary display. + void withPrimaryDispSync(std::function const& fn); + sp getEventConnection(const sp& handle); // Should be called when receiving a hotplug event. diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index dba1f8e03f..08a9eebafd 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -637,6 +637,10 @@ void SurfaceFlinger::init() { mVsyncModulator.setSchedulerAndHandles(mScheduler.get(), mAppConnectionHandle.get(), mSfConnectionHandle.get()); + mRegionSamplingThread = + new RegionSamplingThread(*this, *mScheduler, + RegionSamplingThread::EnvironmentTimingTunables()); + // Get a RenderEngine for the given display / config (can't fail) int32_t renderEngineFeature = 0; renderEngineFeature |= (useColorManagement ? @@ -2063,8 +2067,8 @@ void SurfaceFlinger::postComposition() mTransactionCompletedThread.addPresentFence(mPreviousPresentFence); mTransactionCompletedThread.sendCallbacks(); - if (mLumaSampling) { - mRegionSamplingThread->sampleNow(); + if (mLumaSampling && mRegionSamplingThread) { + mRegionSamplingThread->notifyNewContent(); } } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 8de1e97c5b..0c58de409f 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -1011,9 +1011,6 @@ private: TransactionCompletedThread mTransactionCompletedThread; - bool mLumaSampling = true; - sp mRegionSamplingThread = new RegionSamplingThread(*this); - // Restrict layers to use two buffers in their bufferqueues. bool mLayerTripleBufferingDisabled = false; @@ -1139,6 +1136,9 @@ private: bool mCheckPendingFence = false; /* ------------------------------------------------------------------------ */ + bool mLumaSampling = true; + sp mRegionSamplingThread; + sp mInputFlinger; InputWindowCommands mPendingInputWindowCommands GUARDED_BY(mStateLock); -- cgit v1.2.3-59-g8ed1b From 7355eb247aeca07d2e53901ab0feb1b2d76cd3f8 Mon Sep 17 00:00:00 2001 From: Alec Mouri Date: Tue, 5 Mar 2019 14:19:10 -0800 Subject: Add the notion of a last callback time into DispSync. If we do not track real callback times we run into two issues: * We can potentially fall into double-rate vsyncs due to error in thread wakeup latency. For instance, we only correct up to 1.5 ms when wakeup latency can be up to 5 ms in duration. To correct for this error, we need a sanity check that callback timings won't fall into double-rate vsyncs. * Registering and unregistering event listeners can occur rapidly within the same vsyncs, so that without any correction we can initially double-fire events. If we only rely on inferring the last event time, then we may end up skipping a frame, so we'll require the calling thread registering and unregistering listeners to best-effort cache the lastest callback time. Bug: 124383894 Test: systrace Change-Id: I7bc8f00502a3b798073a77c5ba4794c12cd14341 --- services/surfaceflinger/RegionSamplingThread.cpp | 8 ++-- services/surfaceflinger/Scheduler/DispSync.cpp | 48 +++++++++++++++++----- services/surfaceflinger/Scheduler/DispSync.h | 18 ++++++-- .../surfaceflinger/Scheduler/DispSyncSource.cpp | 6 ++- services/surfaceflinger/Scheduler/DispSyncSource.h | 1 + .../tests/unittests/mock/MockDispSync.h | 4 +- 6 files changed, 64 insertions(+), 21 deletions(-) (limited to 'services/surfaceflinger/RegionSamplingThread.cpp') diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp index 4f0b3bbbcf..718e996dae 100644 --- a/services/surfaceflinger/RegionSamplingThread.cpp +++ b/services/surfaceflinger/RegionSamplingThread.cpp @@ -101,7 +101,7 @@ struct SamplingOffsetCallback : DispSync::Callback { mPhaseIntervalSetting = Phase::ZERO; mScheduler.withPrimaryDispSync([this](android::DispSync& sync) { - sync.addEventListener("SamplingThreadDispSyncListener", 0, this); + sync.addEventListener("SamplingThreadDispSyncListener", 0, this, mLastCallbackTime); }); mVsyncListening = true; } @@ -115,8 +115,9 @@ private: void stopVsyncListenerLocked() /*REQUIRES(mMutex)*/ { if (!mVsyncListening) return; - mScheduler.withPrimaryDispSync( - [this](android::DispSync& sync) { sync.removeEventListener(this); }); + mScheduler.withPrimaryDispSync([this](android::DispSync& sync) { + sync.removeEventListener(this, &mLastCallbackTime); + }); mVsyncListening = false; } @@ -147,6 +148,7 @@ private: Scheduler& mScheduler; const std::chrono::nanoseconds mTargetSamplingOffset; mutable std::mutex mMutex; + nsecs_t mLastCallbackTime = 0; enum class Phase { ZERO, SAMPLING diff --git a/services/surfaceflinger/Scheduler/DispSync.cpp b/services/surfaceflinger/Scheduler/DispSync.cpp index 5b08aab69a..8cb48b13e4 100644 --- a/services/surfaceflinger/Scheduler/DispSync.cpp +++ b/services/surfaceflinger/Scheduler/DispSync.cpp @@ -185,7 +185,8 @@ public: return false; } - status_t addEventListener(const char* name, nsecs_t phase, DispSync::Callback* callback) { + status_t addEventListener(const char* name, nsecs_t phase, DispSync::Callback* callback, + nsecs_t lastCallbackTime) { if (mTraceDetailedInfo) ATRACE_CALL(); Mutex::Autolock lock(mMutex); @@ -211,7 +212,7 @@ public: const nsecs_t predictedReference = mReferenceTime + numPeriodsSinceReference * mPeriod; const nsecs_t phaseCorrection = mPhase + listener.mPhase; const nsecs_t predictedLastEventTime = predictedReference + phaseCorrection; - if (predictedLastEventTime > now) { + if (predictedLastEventTime >= now) { // Make sure that the last event time does not exceed the current time. // If it would, then back the last event time by a period. listener.mLastEventTime = predictedLastEventTime - mPeriod; @@ -222,6 +223,14 @@ public: listener.mLastEventTime = now + mPhase - mWakeupLatency; } + if (lastCallbackTime <= 0) { + // If there is no prior callback time, try to infer one based on the + // logical last event time. + listener.mLastCallbackTime = listener.mLastEventTime + mWakeupLatency; + } else { + listener.mLastCallbackTime = lastCallbackTime; + } + mEventListeners.push_back(listener); mCond.signal(); @@ -229,13 +238,14 @@ public: return NO_ERROR; } - status_t removeEventListener(DispSync::Callback* callback) { + status_t removeEventListener(DispSync::Callback* callback, nsecs_t* outLastCallback) { if (mTraceDetailedInfo) ATRACE_CALL(); Mutex::Autolock lock(mMutex); for (std::vector::iterator it = mEventListeners.begin(); it != mEventListeners.end(); ++it) { if (it->mCallback == callback) { + *outLastCallback = it->mLastCallbackTime; mEventListeners.erase(it); mCond.signal(); return NO_ERROR; @@ -277,6 +287,7 @@ private: const char* mName; nsecs_t mPhase; nsecs_t mLastEventTime; + nsecs_t mLastCallbackTime; DispSync::Callback* mCallback; }; @@ -301,6 +312,13 @@ private: return nextEventTime; } + // Sanity check that the duration is close enough in length to a period without + // falling into double-rate vsyncs. + bool isCloseToPeriod(nsecs_t duration) { + // Ratio of 3/5 is arbitrary, but it must be greater than 1/2. + return duration < (3 * mPeriod) / 5; + } + std::vector gatherCallbackInvocationsLocked(nsecs_t now) { if (mTraceDetailedInfo) ATRACE_CALL(); ALOGV("[%s] gatherCallbackInvocationsLocked @ %" PRId64, mName, ns2us(now)); @@ -312,12 +330,21 @@ private: nsecs_t t = computeListenerNextEventTimeLocked(eventListener, onePeriodAgo); if (t < now) { + if (isCloseToPeriod(now - eventListener.mLastCallbackTime)) { + eventListener.mLastEventTime = t; + eventListener.mLastCallbackTime = now; + ALOGV("[%s] [%s] Skipping event due to model error", mName, + eventListener.mName); + continue; + } CallbackInvocation ci; ci.mCallback = eventListener.mCallback; ci.mEventTime = t; - ALOGV("[%s] [%s] Preparing to fire", mName, eventListener.mName); + ALOGV("[%s] [%s] Preparing to fire, latency: %" PRId64, mName, eventListener.mName, + t - eventListener.mLastEventTime); callbackInvocations.push_back(ci); eventListener.mLastEventTime = t; + eventListener.mLastCallbackTime = now; } } @@ -364,7 +391,7 @@ private: // Check that it's been slightly more than half a period since the last // event so that we don't accidentally fall into double-rate vsyncs - if (t - listener.mLastEventTime < (3 * mPeriod / 5)) { + if (isCloseToPeriod(t - listener.mLastEventTime)) { t += mPeriod; ALOGV("[%s] Modifying t -> %" PRId64, mName, ns2us(t)); } @@ -445,7 +472,7 @@ void DispSync::init(bool hasSyncFramework, int64_t dispSyncPresentTimeOffset) { if (mTraceDetailedInfo && kEnableZeroPhaseTracer) { mZeroPhaseTracer = std::make_unique(); - addEventListener("ZeroPhaseTracer", 0, mZeroPhaseTracer.get()); + addEventListener("ZeroPhaseTracer", 0, mZeroPhaseTracer.get(), 0); } } @@ -539,9 +566,10 @@ bool DispSync::addResyncSample(nsecs_t timestamp) { void DispSync::endResync() {} -status_t DispSync::addEventListener(const char* name, nsecs_t phase, Callback* callback) { +status_t DispSync::addEventListener(const char* name, nsecs_t phase, Callback* callback, + nsecs_t lastCallbackTime) { Mutex::Autolock lock(mMutex); - return mThread->addEventListener(name, phase, callback); + return mThread->addEventListener(name, phase, callback, lastCallbackTime); } void DispSync::setRefreshSkipCount(int count) { @@ -551,9 +579,9 @@ void DispSync::setRefreshSkipCount(int count) { updateModelLocked(); } -status_t DispSync::removeEventListener(Callback* callback) { +status_t DispSync::removeEventListener(Callback* callback, nsecs_t* outLastCallbackTime) { Mutex::Autolock lock(mMutex); - return mThread->removeEventListener(callback); + return mThread->removeEventListener(callback, outLastCallbackTime); } status_t DispSync::changePhaseOffset(Callback* callback, nsecs_t phase) { diff --git a/services/surfaceflinger/Scheduler/DispSync.h b/services/surfaceflinger/Scheduler/DispSync.h index 6f3bd00992..de2b8749c7 100644 --- a/services/surfaceflinger/Scheduler/DispSync.h +++ b/services/surfaceflinger/Scheduler/DispSync.h @@ -54,8 +54,9 @@ public: virtual void setPeriod(nsecs_t period) = 0; virtual nsecs_t getPeriod() = 0; virtual void setRefreshSkipCount(int count) = 0; - virtual status_t addEventListener(const char* name, nsecs_t phase, Callback* callback) = 0; - virtual status_t removeEventListener(Callback* callback) = 0; + virtual status_t addEventListener(const char* name, nsecs_t phase, Callback* callback, + nsecs_t lastCallbackTime) = 0; + virtual status_t removeEventListener(Callback* callback, nsecs_t* outLastCallback) = 0; virtual status_t changePhaseOffset(Callback* callback, nsecs_t phase) = 0; virtual nsecs_t computeNextRefresh(int periodOffset) const = 0; virtual void setIgnorePresentFences(bool ignore) = 0; @@ -139,12 +140,21 @@ public: // given phase offset from the hardware vsync events. The callback is // called from a separate thread and it should return reasonably quickly // (i.e. within a few hundred microseconds). - status_t addEventListener(const char* name, nsecs_t phase, Callback* callback) override; + // If the callback was previously registered, and the last clock time the + // callback was invoked was known to the caller (e.g. via removeEventListener), + // then the caller may pass that through to lastCallbackTime, so that + // callbacks do not accidentally double-fire if they are unregistered and + // reregistered in rapid succession. + status_t addEventListener(const char* name, nsecs_t phase, Callback* callback, + nsecs_t lastCallbackTime) override; // removeEventListener removes an already-registered event callback. Once // this method returns that callback will no longer be called by the // DispSync object. - status_t removeEventListener(Callback* callback) override; + // outLastCallbackTime will contain the last time that the callback was invoked. + // If the caller wishes to reregister the same callback, they should pass the + // callback time back into lastCallbackTime (see addEventListener). + status_t removeEventListener(Callback* callback, nsecs_t* outLastCallbackTime) override; // changePhaseOffset changes the phase offset of an already-registered event callback. The // method will make sure that there is no skipping or double-firing on the listener per frame, diff --git a/services/surfaceflinger/Scheduler/DispSyncSource.cpp b/services/surfaceflinger/Scheduler/DispSyncSource.cpp index d848c976dd..6e89648bdd 100644 --- a/services/surfaceflinger/Scheduler/DispSyncSource.cpp +++ b/services/surfaceflinger/Scheduler/DispSyncSource.cpp @@ -40,13 +40,15 @@ void DispSyncSource::setVSyncEnabled(bool enable) { std::lock_guard lock(mVsyncMutex); if (enable) { status_t err = mDispSync->addEventListener(mName, mPhaseOffset, - static_cast(this)); + static_cast(this), + mLastCallbackTime); if (err != NO_ERROR) { ALOGE("error registering vsync callback: %s (%d)", strerror(-err), err); } // ATRACE_INT(mVsyncOnLabel.c_str(), 1); } else { - status_t err = mDispSync->removeEventListener(static_cast(this)); + status_t err = mDispSync->removeEventListener(static_cast(this), + &mLastCallbackTime); if (err != NO_ERROR) { ALOGE("error unregistering vsync callback: %s (%d)", strerror(-err), err); } diff --git a/services/surfaceflinger/Scheduler/DispSyncSource.h b/services/surfaceflinger/Scheduler/DispSyncSource.h index 5e3d181c4f..2858678108 100644 --- a/services/surfaceflinger/Scheduler/DispSyncSource.h +++ b/services/surfaceflinger/Scheduler/DispSyncSource.h @@ -45,6 +45,7 @@ private: const bool mTraceVsync; const std::string mVsyncOnLabel; const std::string mVsyncEventLabel; + nsecs_t mLastCallbackTime GUARDED_BY(mVsyncMutex) = 0; DispSync* mDispSync; diff --git a/services/surfaceflinger/tests/unittests/mock/MockDispSync.h b/services/surfaceflinger/tests/unittests/mock/MockDispSync.h index 9213ae5fdf..901cf157df 100644 --- a/services/surfaceflinger/tests/unittests/mock/MockDispSync.h +++ b/services/surfaceflinger/tests/unittests/mock/MockDispSync.h @@ -36,8 +36,8 @@ public: MOCK_METHOD1(setPeriod, void(nsecs_t)); MOCK_METHOD0(getPeriod, nsecs_t()); MOCK_METHOD1(setRefreshSkipCount, void(int)); - MOCK_METHOD3(addEventListener, status_t(const char*, nsecs_t, Callback*)); - MOCK_METHOD1(removeEventListener, status_t(Callback*)); + MOCK_METHOD4(addEventListener, status_t(const char*, nsecs_t, Callback*, nsecs_t)); + MOCK_METHOD2(removeEventListener, status_t(Callback*, nsecs_t*)); MOCK_METHOD2(changePhaseOffset, status_t(Callback*, nsecs_t)); MOCK_CONST_METHOD1(computeNextRefresh, nsecs_t(int)); MOCK_METHOD1(setIgnorePresentFences, void(bool)); -- cgit v1.2.3-59-g8ed1b From bb27bcd9c676f65937a2637d8c400d491d655f54 Mon Sep 17 00:00:00 2001 From: Kevin DuBois Date: Tue, 2 Apr 2019 14:34:35 -0700 Subject: 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 --- services/surfaceflinger/RegionSamplingThread.cpp | 4 +- services/surfaceflinger/RegionSamplingThread.h | 2 + services/surfaceflinger/tests/unittests/Android.bp | 1 + .../tests/unittests/RegionSamplingTest.cpp | 77 ++++++++++++++++++++++ 4 files changed, 82 insertions(+), 2 deletions(-) create mode 100644 services/surfaceflinger/tests/unittests/RegionSamplingTest.cpp (limited to 'services/surfaceflinger/RegionSamplingThread.cpp') diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp index 718e996dae..35f11fc494 100644 --- a/services/surfaceflinger/RegionSamplingThread.cpp +++ b/services/surfaceflinger/RegionSamplingThread.cpp @@ -266,6 +266,7 @@ float getLuma(float r, float g, float b) { 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 brightnessBuckets = {}; @@ -286,14 +287,13 @@ float sampleArea(const uint32_t* data, int32_t stride, const Rect& area) { 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 RegionSamplingThread::sampleBuffer( const sp& buffer, const Point& leftTop, diff --git a/services/surfaceflinger/RegionSamplingThread.h b/services/surfaceflinger/RegionSamplingThread.h index d4e57bfc7b..979642912b 100644 --- a/services/surfaceflinger/RegionSamplingThread.h +++ b/services/surfaceflinger/RegionSamplingThread.h @@ -37,6 +37,8 @@ class Scheduler; 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 12b41fd32c..d4eac88ba1 100644 --- a/services/surfaceflinger/tests/unittests/Android.bp +++ b/services/surfaceflinger/tests/unittests/Android.bp @@ -49,6 +49,7 @@ cc_test { "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 0000000000..51d6d7e1df --- /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 +#include +#include +#include + +#include "RegionSamplingThread.h" + +namespace android { + +struct RegionSamplingTest : testing::Test { +public: + static uint32_t constexpr kBlack = 0; + static uint32_t constexpr kWhite = std::numeric_limits::max(); + static int constexpr kWidth = 98; + static int constexpr kStride = 100; + static int constexpr kHeight = 29; + std::array 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::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 -- cgit v1.2.3-59-g8ed1b From 108b2c7d1cabbd1df12f0f20246ac1d5cce0d0be Mon Sep 17 00:00:00 2001 From: Robert Carr Date: Tue, 2 Apr 2019 16:32:58 -0700 Subject: SurfaceFlinger: Indicate whether we have captured secure layers. For purposes of the screen rotation animation the system server is allowed to capture secure (not protected) layers and trusted not to persist screenshots which may contain secure layers. However when displaying the screen rotation animation, the layer the screenshot is placed on will itself not be secure, so if we record the animation the recording will contain persisted versions of the secure content. Here we forward whether the screenshot contains secure content so that system server can do the right thing. Bug: 69703445 Test: Transaction_test#SetFlagsSecureEuidSystem Change-Id: I828cfe3faee3a0c84525f90b9df8b66e392bc240 --- libs/gui/ISurfaceComposer.cpp | 14 ++++++--- libs/gui/SurfaceComposerClient.cpp | 17 ++++++----- libs/gui/include/gui/ISurfaceComposer.h | 8 +++-- libs/gui/include/gui/SurfaceComposerClient.h | 3 +- libs/gui/tests/Surface_test.cpp | 8 +++-- services/surfaceflinger/RegionSamplingThread.cpp | 3 +- services/surfaceflinger/SurfaceFlinger.cpp | 34 +++++++++++++--------- services/surfaceflinger/SurfaceFlinger.h | 16 +++++----- services/surfaceflinger/tests/Transaction_test.cpp | 9 +++--- .../tests/unittests/TestableSurfaceFlinger.h | 11 +++---- 10 files changed, 74 insertions(+), 49 deletions(-) (limited to 'services/surfaceflinger/RegionSamplingThread.cpp') diff --git a/libs/gui/ISurfaceComposer.cpp b/libs/gui/ISurfaceComposer.cpp index 8d7baf34ae..a3165ddb9e 100644 --- a/libs/gui/ISurfaceComposer.cpp +++ b/libs/gui/ISurfaceComposer.cpp @@ -109,7 +109,7 @@ public: } virtual status_t captureScreen(const sp& display, sp* outBuffer, - const ui::Dataspace reqDataspace, + bool& outCapturedSecureLayers, const ui::Dataspace reqDataspace, const ui::PixelFormat reqPixelFormat, Rect sourceCrop, uint32_t reqWidth, uint32_t reqHeight, bool useIdentityTransform, ISurfaceComposer::Rotation rotation, bool captureSecureLayers) { @@ -137,6 +137,7 @@ public: *outBuffer = new GraphicBuffer(); reply.read(**outBuffer); + outCapturedSecureLayers = reply.readBool(); return result; } @@ -1027,12 +1028,17 @@ status_t BnSurfaceComposer::onTransact( int32_t rotation = data.readInt32(); bool captureSecureLayers = static_cast(data.readInt32()); - status_t res = captureScreen(display, &outBuffer, reqDataspace, reqPixelFormat, - sourceCrop, reqWidth, reqHeight, useIdentityTransform, - static_cast(rotation), captureSecureLayers); + bool capturedSecureLayers = false; + status_t res = captureScreen(display, &outBuffer, capturedSecureLayers, reqDataspace, + reqPixelFormat, sourceCrop, reqWidth, reqHeight, + useIdentityTransform, + static_cast(rotation), + captureSecureLayers); + reply->writeInt32(res); if (res == NO_ERROR) { reply->write(*outBuffer); + reply->writeBool(capturedSecureLayers); } return NO_ERROR; } diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp index 0e6170278f..611da89ef6 100644 --- a/libs/gui/SurfaceComposerClient.cpp +++ b/libs/gui/SurfaceComposerClient.cpp @@ -1542,13 +1542,15 @@ status_t SurfaceComposerClient::setDisplayBrightness(const sp& displayT status_t ScreenshotClient::capture(const sp& display, const ui::Dataspace reqDataSpace, const ui::PixelFormat reqPixelFormat, Rect sourceCrop, uint32_t reqWidth, uint32_t reqHeight, bool useIdentityTransform, - uint32_t rotation, bool captureSecureLayers, sp* outBuffer) { + uint32_t rotation, bool captureSecureLayers, + sp* outBuffer, bool& outCapturedSecureLayers) { sp s(ComposerService::getComposerService()); if (s == nullptr) return NO_INIT; - status_t ret = s->captureScreen(display, outBuffer, reqDataSpace, reqPixelFormat, sourceCrop, - reqWidth, reqHeight, useIdentityTransform, - static_cast(rotation), - captureSecureLayers); + status_t ret = + s->captureScreen(display, outBuffer, outCapturedSecureLayers, reqDataSpace, + reqPixelFormat, sourceCrop, reqWidth, reqHeight, useIdentityTransform, + static_cast(rotation), + captureSecureLayers); if (ret != NO_ERROR) { return ret; } @@ -1559,8 +1561,9 @@ status_t ScreenshotClient::capture(const sp& display, const ui::Dataspa const ui::PixelFormat reqPixelFormat, Rect sourceCrop, uint32_t reqWidth, uint32_t reqHeight, bool useIdentityTransform, uint32_t rotation, sp* outBuffer) { - return capture(display, reqDataSpace, reqPixelFormat, sourceCrop, reqWidth, - reqHeight, useIdentityTransform, rotation, false, outBuffer); + bool ignored; + return capture(display, reqDataSpace, reqPixelFormat, sourceCrop, reqWidth, reqHeight, + useIdentityTransform, rotation, false, outBuffer, ignored); } status_t ScreenshotClient::captureLayers(const sp& layerHandle, diff --git a/libs/gui/include/gui/ISurfaceComposer.h b/libs/gui/include/gui/ISurfaceComposer.h index 14d92bf04e..415b2d58ec 100644 --- a/libs/gui/include/gui/ISurfaceComposer.h +++ b/libs/gui/include/gui/ISurfaceComposer.h @@ -212,7 +212,7 @@ public: * it) around its center. */ virtual status_t captureScreen(const sp& display, sp* outBuffer, - const ui::Dataspace reqDataspace, + bool& outCapturedSecureLayers, const ui::Dataspace reqDataspace, const ui::PixelFormat reqPixelFormat, Rect sourceCrop, uint32_t reqWidth, uint32_t reqHeight, bool useIdentityTransform, Rotation rotation = eRotateNone, @@ -241,8 +241,10 @@ public: virtual status_t captureScreen(const sp& display, sp* outBuffer, Rect sourceCrop, uint32_t reqWidth, uint32_t reqHeight, bool useIdentityTransform, Rotation rotation = eRotateNone) { - return captureScreen(display, outBuffer, ui::Dataspace::V0_SRGB, ui::PixelFormat::RGBA_8888, - sourceCrop, reqWidth, reqHeight, useIdentityTransform, rotation); + bool outIgnored; + return captureScreen(display, outBuffer, outIgnored, ui::Dataspace::V0_SRGB, + ui::PixelFormat::RGBA_8888, sourceCrop, reqWidth, reqHeight, + useIdentityTransform, rotation); } template diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h index f64fb61ecb..9d344689df 100644 --- a/libs/gui/include/gui/SurfaceComposerClient.h +++ b/libs/gui/include/gui/SurfaceComposerClient.h @@ -509,7 +509,8 @@ public: static status_t capture(const sp& display, const ui::Dataspace reqDataSpace, const ui::PixelFormat reqPixelFormat, Rect sourceCrop, uint32_t reqWidth, uint32_t reqHeight, bool useIdentityTransform, - uint32_t rotation, bool captureSecureLayers, sp* outBuffer); + uint32_t rotation, bool captureSecureLayers, + sp* outBuffer, bool& outCapturedSecureLayers); static status_t capture(const sp& display, const ui::Dataspace reqDataSpace, const ui::PixelFormat reqPixelFormat, Rect sourceCrop, uint32_t reqWidth, uint32_t reqHeight, bool useIdentityTransform, diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp index fa2e97f1a8..d5bdd74a86 100644 --- a/libs/gui/tests/Surface_test.cpp +++ b/libs/gui/tests/Surface_test.cpp @@ -126,7 +126,7 @@ TEST_F(SurfaceTest, QueuesToWindowComposerIsTrueWhenPurgatorized) { } // This test probably doesn't belong here. -TEST_F(SurfaceTest, ScreenshotsOfProtectedBuffersSucceed) { +TEST_F(SurfaceTest, ScreenshotsOfProtectedBuffersDontSucceed) { sp anw(mSurface); // Verify the screenshot works with no protected buffers. @@ -136,8 +136,9 @@ TEST_F(SurfaceTest, ScreenshotsOfProtectedBuffersSucceed) { ASSERT_FALSE(display == nullptr); sp outBuffer; + bool ignored; ASSERT_EQ(NO_ERROR, - sf->captureScreen(display, &outBuffer, ui::Dataspace::V0_SRGB, + sf->captureScreen(display, &outBuffer, ignored, ui::Dataspace::V0_SRGB, ui::PixelFormat::RGBA_8888, Rect(), 64, 64, false)); ASSERT_EQ(NO_ERROR, native_window_api_connect(anw.get(), @@ -169,7 +170,7 @@ TEST_F(SurfaceTest, ScreenshotsOfProtectedBuffersSucceed) { ASSERT_EQ(NO_ERROR, anw->queueBuffer(anw.get(), buf, -1)); } ASSERT_EQ(NO_ERROR, - sf->captureScreen(display, &outBuffer, ui::Dataspace::V0_SRGB, + sf->captureScreen(display, &outBuffer, ignored, ui::Dataspace::V0_SRGB, ui::PixelFormat::RGBA_8888, Rect(), 64, 64, false)); } @@ -615,6 +616,7 @@ public: status_t setActiveColorMode(const sp& /*display*/, ColorMode /*colorMode*/) override { return NO_ERROR; } status_t captureScreen(const sp& /*display*/, sp* /*outBuffer*/, + bool& /* outCapturedSecureLayers */, const ui::Dataspace /*reqDataspace*/, const ui::PixelFormat /*reqPixelFormat*/, Rect /*sourceCrop*/, uint32_t /*reqWidth*/, uint32_t /*reqHeight*/, diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp index 35f11fc494..252ff0d611 100644 --- a/services/surfaceflinger/RegionSamplingThread.cpp +++ b/services/surfaceflinger/RegionSamplingThread.cpp @@ -392,7 +392,8 @@ void RegionSamplingThread::captureSample() { // // To avoid this, we drop the mutex while we call into SF. mMutex.unlock(); - mFlinger.captureScreenCommon(renderArea, traverseLayers, buffer, false); + bool ignored; + mFlinger.captureScreenCommon(renderArea, traverseLayers, buffer, false, ignored); mMutex.lock(); std::vector activeDescriptors; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index fccd910795..e2a134b637 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -5369,7 +5369,8 @@ private: }; status_t SurfaceFlinger::captureScreen(const sp& displayToken, - sp* outBuffer, const Dataspace reqDataspace, + sp* outBuffer, bool& outCapturedSecureLayers, + const Dataspace reqDataspace, const ui::PixelFormat reqPixelFormat, Rect sourceCrop, uint32_t reqWidth, uint32_t reqHeight, bool useIdentityTransform, @@ -5402,7 +5403,7 @@ status_t SurfaceFlinger::captureScreen(const sp& displayToken, auto traverseLayers = std::bind(&SurfaceFlinger::traverseLayersInDisplay, this, display, std::placeholders::_1); return captureScreenCommon(renderArea, traverseLayers, outBuffer, reqPixelFormat, - useIdentityTransform); + useIdentityTransform, outCapturedSecureLayers); } status_t SurfaceFlinger::captureLayers( @@ -5564,14 +5565,18 @@ status_t SurfaceFlinger::captureLayers( visitor(layer); }); }; - return captureScreenCommon(renderArea, traverseLayers, outBuffer, reqPixelFormat, false); + + bool outCapturedSecureLayers = false; + return captureScreenCommon(renderArea, traverseLayers, outBuffer, reqPixelFormat, false, + outCapturedSecureLayers); } status_t SurfaceFlinger::captureScreenCommon(RenderArea& renderArea, TraverseLayersFunction traverseLayers, sp* outBuffer, const ui::PixelFormat reqPixelFormat, - bool useIdentityTransform) { + bool useIdentityTransform, + bool& outCapturedSecureLayers) { ATRACE_CALL(); // TODO(b/116112787) Make buffer usage a parameter. @@ -5582,13 +5587,15 @@ status_t SurfaceFlinger::captureScreenCommon(RenderArea& renderArea, static_cast(reqPixelFormat), 1, usage, "screenshot"); - return captureScreenCommon(renderArea, traverseLayers, *outBuffer, useIdentityTransform); + return captureScreenCommon(renderArea, traverseLayers, *outBuffer, useIdentityTransform, + outCapturedSecureLayers); } status_t SurfaceFlinger::captureScreenCommon(RenderArea& renderArea, TraverseLayersFunction traverseLayers, const sp& buffer, - bool useIdentityTransform) { + bool useIdentityTransform, + bool& outCapturedSecureLayers) { // This mutex protects syncFd and captureResult for communication of the return values from the // main thread back to this Binder thread std::mutex captureMutex; @@ -5617,7 +5624,8 @@ status_t SurfaceFlinger::captureScreenCommon(RenderArea& renderArea, Mutex::Autolock _l(mStateLock); renderArea.render([&] { result = captureScreenImplLocked(renderArea, traverseLayers, buffer.get(), - useIdentityTransform, forSystem, &fd); + useIdentityTransform, forSystem, &fd, + outCapturedSecureLayers); }); } @@ -5757,21 +5765,19 @@ void SurfaceFlinger::renderScreenImplLocked(const RenderArea& renderArea, status_t SurfaceFlinger::captureScreenImplLocked(const RenderArea& renderArea, TraverseLayersFunction traverseLayers, ANativeWindowBuffer* buffer, - bool useIdentityTransform, - bool forSystem, - int* outSyncFd) { + bool useIdentityTransform, bool forSystem, + int* outSyncFd, bool& outCapturedSecureLayers) { ATRACE_CALL(); - bool secureLayerIsVisible = false; - traverseLayers([&](Layer* layer) { - secureLayerIsVisible = secureLayerIsVisible || (layer->isVisible() && layer->isSecure()); + outCapturedSecureLayers = + outCapturedSecureLayers || (layer->isVisible() && layer->isSecure()); }); // We allow the system server to take screenshots of secure layers for // use in situations like the Screen-rotation animation and place // the impetus on WindowManager to not persist them. - if (secureLayerIsVisible && !forSystem) { + if (outCapturedSecureLayers && !forSystem) { ALOGW("FB is protected: PERMISSION_DENIED"); return PERMISSION_DENIED; } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 7e8e836e6a..7914afc5ba 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -395,16 +395,17 @@ private: sp createDisplayEventConnection( ISurfaceComposer::VsyncSource vsyncSource = eVsyncSourceApp) override; status_t captureScreen(const sp& displayToken, sp* outBuffer, - const ui::Dataspace reqDataspace, const ui::PixelFormat reqPixelFormat, - Rect sourceCrop, uint32_t reqWidth, uint32_t reqHeight, - bool useIdentityTransform, ISurfaceComposer::Rotation rotation, - bool captureSecureLayers) override; + bool& outCapturedSecureLayers, const ui::Dataspace reqDataspace, + const ui::PixelFormat reqPixelFormat, Rect sourceCrop, + uint32_t reqWidth, uint32_t reqHeight, + bool useIdentityTransform, ISurfaceComposer::Rotation rotation, bool captureSecureLayers) override; status_t captureLayers( const sp& parentHandle, sp* outBuffer, const ui::Dataspace reqDataspace, const ui::PixelFormat reqPixelFormat, const Rect& sourceCrop, const std::unordered_set, ISurfaceComposer::SpHash>& exclude, float frameScale, bool childrenOnly) override; + status_t getDisplayStats(const sp& displayToken, DisplayStatInfo* stats) override; status_t getDisplayConfigs(const sp& displayToken, Vector* configs) override { @@ -640,13 +641,14 @@ private: int* outSyncFd); status_t captureScreenCommon(RenderArea& renderArea, TraverseLayersFunction traverseLayers, sp* outBuffer, const ui::PixelFormat reqPixelFormat, - bool useIdentityTransform); + bool useIdentityTransform, bool& outCapturedSecureLayers); status_t captureScreenCommon(RenderArea& renderArea, TraverseLayersFunction traverseLayers, - const sp& buffer, bool useIdentityTransform); + const sp& buffer, bool useIdentityTransform, + bool& outCapturedSecureLayers); status_t captureScreenImplLocked(const RenderArea& renderArea, TraverseLayersFunction traverseLayers, ANativeWindowBuffer* buffer, bool useIdentityTransform, - bool forSystem, int* outSyncFd); + bool forSystem, int* outSyncFd, bool& outCapturedSecureLayers); void traverseLayersInDisplay(const sp& display, const LayerVector::Visitor& visitor); diff --git a/services/surfaceflinger/tests/Transaction_test.cpp b/services/surfaceflinger/tests/Transaction_test.cpp index ba854e3f27..d03c25df70 100644 --- a/services/surfaceflinger/tests/Transaction_test.cpp +++ b/services/surfaceflinger/tests/Transaction_test.cpp @@ -1269,11 +1269,12 @@ TEST_F(LayerTransactionTest, SetFlagsSecureEUidSystem) { // Here we pass captureSecureLayers = true and since we are AID_SYSTEM we should be able // to receive them...we are expected to take care with the results. + bool outCapturedSecureLayers; ASSERT_EQ(NO_ERROR, - composer->captureScreen(mDisplay, &outBuffer, - ui::Dataspace::V0_SRGB, ui::PixelFormat::RGBA_8888, - Rect(), 0, 0, false, - ISurfaceComposer::eRotateNone, true)); + composer->captureScreen(mDisplay, &outBuffer, outCapturedSecureLayers, + ui::Dataspace::V0_SRGB, ui::PixelFormat::RGBA_8888, Rect(), 0, + 0, false, ISurfaceComposer::eRotateNone, true)); + ASSERT_EQ(true, outCapturedSecureLayers); ScreenCapture sc(outBuffer); sc.expectColor(Rect(0, 0, 32, 32), Color::RED); } diff --git a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h index 81235ba260..6bbcae3a9d 100644 --- a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h +++ b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h @@ -276,12 +276,13 @@ public: auto onMessageReceived(int32_t what) { return mFlinger->onMessageReceived(what); } - auto captureScreenImplLocked(const RenderArea& renderArea, - SurfaceFlinger::TraverseLayersFunction traverseLayers, - ANativeWindowBuffer* buffer, bool useIdentityTransform, - bool forSystem, int* outSyncFd) { + auto captureScreenImplLocked( + const RenderArea& renderArea, SurfaceFlinger::TraverseLayersFunction traverseLayers, + ANativeWindowBuffer* buffer, bool useIdentityTransform, bool forSystem, int* outSyncFd) { + bool ignored; return mFlinger->captureScreenImplLocked(renderArea, traverseLayers, buffer, - useIdentityTransform, forSystem, outSyncFd); + useIdentityTransform, forSystem, outSyncFd, + ignored); } auto traverseLayersInDisplay(const sp& display, -- cgit v1.2.3-59-g8ed1b From 4efd1f505ce5587bba57e79dfb54eb1daff59eec Mon Sep 17 00:00:00 2001 From: Kevin DuBois Date: Mon, 29 Apr 2019 10:09:43 -0700 Subject: sf: reuse luma sampling buffer when available Reuse the luma sampling buffer when available, saving a frequent buffer reallocation. Depending on driver refcounting behavior, this could have resulted in the buffer free on the SF main thread (and typically did on referenced device, costing about ~500us @10hz). Test: verify luma sampling work reduces from ~2ms -> 1.5ms Test: RegionSamplingTest in libgui Fixes: 131416627 Change-Id: I8e6d57ae25bd37ceec828c82796f0f4f8f45636e --- services/surfaceflinger/RegionSamplingThread.cpp | 19 +++++++++++++++---- services/surfaceflinger/RegionSamplingThread.h | 4 +++- 2 files changed, 18 insertions(+), 5 deletions(-) (limited to 'services/surfaceflinger/RegionSamplingThread.cpp') diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp index 252ff0d611..0d142675db 100644 --- a/services/surfaceflinger/RegionSamplingThread.cpp +++ b/services/surfaceflinger/RegionSamplingThread.cpp @@ -377,10 +377,15 @@ void RegionSamplingThread::captureSample() { mFlinger.traverseLayersInDisplay(device, filterVisitor); }; - const uint32_t usage = GRALLOC_USAGE_SW_READ_OFTEN | GRALLOC_USAGE_HW_RENDER; - sp buffer = - new GraphicBuffer(sampledArea.getWidth(), sampledArea.getHeight(), - PIXEL_FORMAT_RGBA_8888, 1, usage, "RegionSamplingThread"); + sp buffer = nullptr; + if (mCachedBuffer && mCachedBuffer->getWidth() == sampledArea.getWidth() && + mCachedBuffer->getHeight() == sampledArea.getHeight()) { + buffer = mCachedBuffer; + } else { + const uint32_t usage = GRALLOC_USAGE_SW_READ_OFTEN | GRALLOC_USAGE_HW_RENDER; + buffer = new GraphicBuffer(sampledArea.getWidth(), sampledArea.getHeight(), + PIXEL_FORMAT_RGBA_8888, 1, usage, "RegionSamplingThread"); + } // When calling into SF, we post a message into the SF message queue (so the // screen capture runs on the main thread). This message blocks until the @@ -415,6 +420,12 @@ void RegionSamplingThread::captureSample() { for (size_t d = 0; d < activeDescriptors.size(); ++d) { activeDescriptors[d].listener->onSampleCollected(lumas[d]); } + + // Extend the lifetime of mCachedBuffer from the previous frame to here to ensure that: + // 1) The region sampling thread is the last owner of the buffer, and the freeing of the buffer + // happens in this thread, as opposed to the main thread. + // 2) The listener(s) receive their notifications prior to freeing the buffer. + mCachedBuffer = buffer; ATRACE_INT(lumaSamplingStepTag, static_cast(samplingStep::noWorkNeeded)); } diff --git a/services/surfaceflinger/RegionSamplingThread.h b/services/surfaceflinger/RegionSamplingThread.h index 979642912b..72b20420ef 100644 --- a/services/surfaceflinger/RegionSamplingThread.h +++ b/services/surfaceflinger/RegionSamplingThread.h @@ -24,13 +24,13 @@ #include #include +#include #include #include #include "Scheduler/IdleTimer.h" namespace android { -class GraphicBuffer; class IRegionSamplingListener; class Layer; class Scheduler; @@ -121,6 +121,8 @@ private: std::unordered_map, Descriptor, WpHash> mDescriptors GUARDED_BY(mMutex); std::chrono::nanoseconds lastSampleTime GUARDED_BY(mMutex); bool mDiscardedFrames GUARDED_BY(mMutex) = false; + + sp mCachedBuffer GUARDED_BY(mMutex) = nullptr; }; } // namespace android -- cgit v1.2.3-59-g8ed1b From 26afc788434bf0f120db2fbd980d557f07bd1283 Mon Sep 17 00:00:00 2001 From: Kevin DuBois Date: Mon, 6 May 2019 16:46:45 -0700 Subject: sf: avoid lock on main thread during luma calc The luma calculation operation was previously done while holding a lock that was also used to protect the request to collect the next sample. When the main thread would request the next sample, it could stall waiting on the longer luma calculation to complete. This patch refactors the sampling request locking mechanisms so sampling request signalling is protected by one lock, and the members needed during the sampling thread operations are protected by another lock. Fixes: http://b/132110951 Test: collect traces during during problematic animation (agsa initial query for info) and verify problem went away Test: visually inspect jank during agsa continued conversation Test: libgui#RegionSamplingTest Test: monkey 6000 event inject Change-Id: I291d6bcb80d0588f2e1f3689bfdd4b3434132e90 --- services/surfaceflinger/RegionSamplingThread.cpp | 44 +++++++++--------------- services/surfaceflinger/RegionSamplingThread.h | 23 ++++++------- 2 files changed, 28 insertions(+), 39 deletions(-) (limited to 'services/surfaceflinger/RegionSamplingThread.cpp') diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp index 0d142675db..368426018b 100644 --- a/services/surfaceflinger/RegionSamplingThread.cpp +++ b/services/surfaceflinger/RegionSamplingThread.cpp @@ -168,11 +168,8 @@ RegionSamplingThread::RegionSamplingThread(SurfaceFlinger& flinger, Scheduler& s mPhaseCallback(std::make_unique(*this, mScheduler, tunables.mSamplingOffset)), lastSampleTime(0ns) { - { - std::lock_guard threadLock(mThreadMutex); - mThread = std::thread([this]() { threadMain(); }); - pthread_setname_np(mThread.native_handle(), "RegionSamplingThread"); - } + mThread = std::thread([this]() { threadMain(); }); + pthread_setname_np(mThread.native_handle(), "RegionSamplingThread"); mIdleTimer.start(); } @@ -186,12 +183,11 @@ RegionSamplingThread::~RegionSamplingThread() { mIdleTimer.stop(); { - std::lock_guard lock(mMutex); + std::lock_guard lock(mThreadControlMutex); mRunning = false; mCondition.notify_one(); } - std::lock_guard threadLock(mThreadMutex); if (mThread.joinable()) { mThread.join(); } @@ -205,17 +201,17 @@ void RegionSamplingThread::addListener(const Rect& samplingArea, const sp asBinder = IInterface::asBinder(listener); asBinder->linkToDeath(this); - std::lock_guard lock(mMutex); + std::lock_guard lock(mSamplingMutex); mDescriptors.emplace(wp(asBinder), Descriptor{samplingArea, stopLayer, listener}); } void RegionSamplingThread::removeListener(const sp& listener) { - std::lock_guard lock(mMutex); + std::lock_guard lock(mSamplingMutex); mDescriptors.erase(wp(IInterface::asBinder(listener))); } void RegionSamplingThread::checkForStaleLuma() { - std::lock_guard lock(mMutex); + std::lock_guard lock(mThreadControlMutex); if (mDiscardedFrames) { ATRACE_INT(lumaSamplingStepTag, static_cast(samplingStep::waitForZeroPhase)); @@ -233,7 +229,7 @@ void RegionSamplingThread::notifySamplingOffset() { } void RegionSamplingThread::doSample() { - std::lock_guard lock(mMutex); + std::lock_guard lock(mThreadControlMutex); auto now = std::chrono::nanoseconds(systemTime(SYSTEM_TIME_MONOTONIC)); if (lastSampleTime + mTunables.mSamplingPeriod > now) { ATRACE_INT(lumaSamplingStepTag, static_cast(samplingStep::idleTimerWaiting)); @@ -254,7 +250,7 @@ void RegionSamplingThread::doSample() { } void RegionSamplingThread::binderDied(const wp& who) { - std::lock_guard lock(mMutex); + std::lock_guard lock(mSamplingMutex); mDescriptors.erase(who); } @@ -315,6 +311,7 @@ std::vector RegionSamplingThread::sampleBuffer( void RegionSamplingThread::captureSample() { ATRACE_CALL(); + std::lock_guard lock(mSamplingMutex); if (mDescriptors.empty()) { return; @@ -387,19 +384,8 @@ void RegionSamplingThread::captureSample() { PIXEL_FORMAT_RGBA_8888, 1, usage, "RegionSamplingThread"); } - // When calling into SF, we post a message into the SF message queue (so the - // screen capture runs on the main thread). This message blocks until the - // screenshot is actually captured, but before the capture occurs, the main - // thread may perform a normal refresh cycle. At the end of this cycle, it - // can request another sample (because layers changed), which triggers a - // call into sampleNow. When sampleNow attempts to grab the mutex, we can - // deadlock. - // - // To avoid this, we drop the mutex while we call into SF. - mMutex.unlock(); bool ignored; mFlinger.captureScreenCommon(renderArea, traverseLayers, buffer, false, ignored); - mMutex.lock(); std::vector activeDescriptors; for (const auto& descriptor : descriptors) { @@ -429,15 +415,19 @@ void RegionSamplingThread::captureSample() { ATRACE_INT(lumaSamplingStepTag, static_cast(samplingStep::noWorkNeeded)); } -void RegionSamplingThread::threadMain() { - std::lock_guard lock(mMutex); +// NO_THREAD_SAFETY_ANALYSIS is because std::unique_lock presently lacks thread safety annotations. +void RegionSamplingThread::threadMain() NO_THREAD_SAFETY_ANALYSIS { + std::unique_lock lock(mThreadControlMutex); while (mRunning) { if (mSampleRequested) { mSampleRequested = false; + lock.unlock(); captureSample(); + lock.lock(); } - mCondition.wait(mMutex, - [this]() REQUIRES(mMutex) { return mSampleRequested || !mRunning; }); + mCondition.wait(lock, [this]() REQUIRES(mThreadControlMutex) { + return mSampleRequested || !mRunning; + }); } } diff --git a/services/surfaceflinger/RegionSamplingThread.h b/services/surfaceflinger/RegionSamplingThread.h index 72b20420ef..08134e6acd 100644 --- a/services/surfaceflinger/RegionSamplingThread.h +++ b/services/surfaceflinger/RegionSamplingThread.h @@ -100,7 +100,7 @@ private: void binderDied(const wp& who) override; void checkForStaleLuma(); - void captureSample() REQUIRES(mMutex); + void captureSample(); void threadMain(); SurfaceFlinger& mFlinger; @@ -110,19 +110,18 @@ private: std::unique_ptr const mPhaseCallback; - std::mutex mThreadMutex; - std::thread mThread GUARDED_BY(mThreadMutex); + std::thread mThread; - std::mutex mMutex; + std::mutex mThreadControlMutex; std::condition_variable_any mCondition; - bool mRunning GUARDED_BY(mMutex) = true; - bool mSampleRequested GUARDED_BY(mMutex) = false; - - std::unordered_map, Descriptor, WpHash> mDescriptors GUARDED_BY(mMutex); - std::chrono::nanoseconds lastSampleTime GUARDED_BY(mMutex); - bool mDiscardedFrames GUARDED_BY(mMutex) = false; - - sp mCachedBuffer GUARDED_BY(mMutex) = nullptr; + bool mRunning GUARDED_BY(mThreadControlMutex) = true; + bool mSampleRequested GUARDED_BY(mThreadControlMutex) = false; + bool mDiscardedFrames GUARDED_BY(mThreadControlMutex) = false; + std::chrono::nanoseconds lastSampleTime GUARDED_BY(mThreadControlMutex); + + std::mutex mSamplingMutex; + std::unordered_map, Descriptor, WpHash> mDescriptors GUARDED_BY(mSamplingMutex); + sp mCachedBuffer GUARDED_BY(mSamplingMutex) = nullptr; }; } // namespace android -- cgit v1.2.3-59-g8ed1b From b325c93b5ef70c3392dbde815b9867b01ab997f8 Mon Sep 17 00:00:00 2001 From: Kevin DuBois Date: Tue, 21 May 2019 08:34:09 -0700 Subject: SF: adapt region sampling to display orientation Pass in the orientation flags of SF to RenderEngine when conducting the sampling composition. This resulted mis-sampled areas, especially when the region was outside of the clip of the 0-degree rotated display. Bug: 132394665 Test: manual verification with 90, 270, 0 rotations Test: new tests in libsurfaceflinger_unittest#RegionSamplingTest.* Change-Id: I2869ef191572dbcc9170df8d3ed17414ab053ca4 --- services/surfaceflinger/RegionSamplingThread.cpp | 61 +++++++++++++++--- services/surfaceflinger/RegionSamplingThread.h | 5 +- .../tests/unittests/RegionSamplingTest.cpp | 72 ++++++++++++++++++++-- 3 files changed, 122 insertions(+), 16 deletions(-) (limited to 'services/surfaceflinger/RegionSamplingThread.cpp') diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp index 368426018b..7fa33f597c 100644 --- a/services/surfaceflinger/RegionSamplingThread.cpp +++ b/services/surfaceflinger/RegionSamplingThread.cpp @@ -26,6 +26,8 @@ #include #include +#include +#include #include "DisplayDevice.h" #include "Layer.h" #include "SurfaceFlinger.h" @@ -264,7 +266,22 @@ float getLuma(float r, float g, float b) { } } // anonymous namespace -float sampleArea(const uint32_t* data, int32_t stride, const Rect& area) { +float sampleArea(const uint32_t* data, int32_t width, int32_t height, int32_t stride, + uint32_t orientation, const Rect& sample_area) { + if (!sample_area.isValid() || (sample_area.getWidth() > width) || + (sample_area.getHeight() > height)) { + ALOGE("invalid sampling region requested"); + return 0.0f; + } + + // (b/133849373) ROT_90 screencap images produced upside down + auto area = sample_area; + if (orientation & ui::Transform::ROT_90) { + area.top = height - area.top; + area.bottom = height - area.bottom; + std::swap(area.top, area.bottom); + } + std::array brightnessBuckets = {}; const int32_t majoritySampleNum = area.getWidth() * area.getHeight() / 2; @@ -293,18 +310,21 @@ float sampleArea(const uint32_t* data, int32_t stride, const Rect& area) { std::vector RegionSamplingThread::sampleBuffer( const sp& buffer, const Point& leftTop, - const std::vector& descriptors) { + const std::vector& descriptors, uint32_t orientation) { void* data_raw = nullptr; buffer->lock(GRALLOC_USAGE_SW_READ_OFTEN, &data_raw); std::shared_ptr data(reinterpret_cast(data_raw), [&buffer](auto) { buffer->unlock(); }); if (!data) return {}; + const int32_t width = buffer->getWidth(); + const int32_t height = buffer->getHeight(); const int32_t stride = buffer->getStride(); std::vector lumas(descriptors.size()); std::transform(descriptors.begin(), descriptors.end(), lumas.begin(), [&](auto const& descriptor) { - return sampleArea(data.get(), stride, descriptor.area - leftTop); + return sampleArea(data.get(), width, height, stride, orientation, + descriptor.area - leftTop); }); return lumas; } @@ -317,6 +337,11 @@ void RegionSamplingThread::captureSample() { return; } + const auto device = mFlinger.getDefaultDisplayDevice(); + const auto display = device->getCompositionDisplay(); + const auto state = display->getState(); + const auto orientation = static_cast(state.orientation); + std::vector descriptors; Region sampleRegion; for (const auto& [listener, descriptor] : mDescriptors) { @@ -326,10 +351,28 @@ void RegionSamplingThread::captureSample() { const Rect sampledArea = sampleRegion.bounds(); - sp device = mFlinger.getDefaultDisplayDevice(); - DisplayRenderArea renderArea(device, sampledArea, sampledArea.getWidth(), - sampledArea.getHeight(), ui::Dataspace::V0_SRGB, - ui::Transform::ROT_0); + auto dx = 0; + auto dy = 0; + switch (orientation) { + case ui::Transform::ROT_90: + dx = device->getWidth(); + break; + case ui::Transform::ROT_180: + dx = device->getWidth(); + dy = device->getHeight(); + break; + case ui::Transform::ROT_270: + dy = device->getHeight(); + break; + default: + break; + } + + ui::Transform t(orientation); + auto screencapRegion = t.transform(sampleRegion); + screencapRegion = screencapRegion.translate(dx, dy); + DisplayRenderArea renderArea(device, screencapRegion.bounds(), sampledArea.getWidth(), + sampledArea.getHeight(), ui::Dataspace::V0_SRGB, orientation); std::unordered_set, SpHash> listeners; @@ -395,8 +438,8 @@ void RegionSamplingThread::captureSample() { } ALOGV("Sampling %zu descriptors", activeDescriptors.size()); - std::vector lumas = sampleBuffer(buffer, sampledArea.leftTop(), activeDescriptors); - + std::vector lumas = + sampleBuffer(buffer, sampledArea.leftTop(), activeDescriptors, orientation); if (lumas.size() != activeDescriptors.size()) { ALOGW("collected %zu median luma values for %zu descriptors", lumas.size(), activeDescriptors.size()); diff --git a/services/surfaceflinger/RegionSamplingThread.h b/services/surfaceflinger/RegionSamplingThread.h index 08134e6acd..3c6fcf3872 100644 --- a/services/surfaceflinger/RegionSamplingThread.h +++ b/services/surfaceflinger/RegionSamplingThread.h @@ -37,7 +37,8 @@ class Scheduler; class SurfaceFlinger; struct SamplingOffsetCallback; -float sampleArea(const uint32_t* data, int32_t stride, const Rect& area); +float sampleArea(const uint32_t* data, int32_t width, int32_t height, int32_t stride, + uint32_t orientation, const Rect& area); class RegionSamplingThread : public IBinder::DeathRecipient { public: @@ -94,7 +95,7 @@ private: }; std::vector sampleBuffer( const sp& buffer, const Point& leftTop, - const std::vector& descriptors); + const std::vector& descriptors, uint32_t orientation); void doSample(); void binderDied(const wp& who) override; diff --git a/services/surfaceflinger/tests/unittests/RegionSamplingTest.cpp b/services/surfaceflinger/tests/unittests/RegionSamplingTest.cpp index 51d6d7e1df..160f0412d9 100644 --- a/services/surfaceflinger/tests/unittests/RegionSamplingTest.cpp +++ b/services/surfaceflinger/tests/unittests/RegionSamplingTest.cpp @@ -17,6 +17,8 @@ #undef LOG_TAG #define LOG_TAG "RegionSamplingTest" +#include + #include #include #include @@ -33,18 +35,21 @@ public: static int constexpr kWidth = 98; static int constexpr kStride = 100; static int constexpr kHeight = 29; + static int constexpr kOrientation = ui::Transform::ROT_0; std::array 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)); + EXPECT_THAT(sampleArea(buffer.data(), kWidth, kHeight, kStride, kOrientation, 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)); + EXPECT_THAT(sampleArea(buffer.data(), kWidth, kHeight, kStride, kOrientation, whole_area), + testing::FloatEq(0.0f)); } TEST_F(RegionSamplingTest, calculate_mean_partial_region) { @@ -54,7 +59,8 @@ TEST_F(RegionSamplingTest, calculate_mean_partial_region) { 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)); + EXPECT_THAT(sampleArea(buffer.data(), kWidth, kHeight, kStride, kOrientation, partial_region), + testing::FloatEq(0.0f)); } TEST_F(RegionSamplingTest, calculate_mean_mixed_values) { @@ -63,15 +69,71 @@ TEST_F(RegionSamplingTest, calculate_mean_mixed_values) { n++; return pixel; }); - EXPECT_THAT(sampleArea(buffer.data(), kStride, whole_area), testing::FloatNear(0.083f, 0.01f)); + EXPECT_THAT(sampleArea(buffer.data(), kWidth, kHeight, kStride, kOrientation, 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), + EXPECT_THAT(sampleArea(buffer.data(), kWidth, kHeight, kStride, kOrientation, whole_area), testing::AnyOf(testing::FloatEq(1.0), testing::FloatEq(0.0f))); } +TEST_F(RegionSamplingTest, bounds_checking) { + std::generate(buffer.begin(), buffer.end(), + [n = 0]() mutable { return (n++ > (kStride * kHeight >> 1)) ? kBlack : kWhite; }); + + Rect invalid_region{0, 0, 4, kHeight + 1}; + EXPECT_THAT(sampleArea(buffer.data(), kWidth, kHeight, kStride, kOrientation, invalid_region), + testing::Eq(0.0)); + + invalid_region = Rect{0, 0, -4, kHeight}; + EXPECT_THAT(sampleArea(buffer.data(), kWidth, kHeight, kStride, kOrientation, invalid_region), + testing::Eq(0.0)); + + invalid_region = Rect{3, 0, 2, 0}; + EXPECT_THAT(sampleArea(buffer.data(), kWidth, kHeight, kStride, kOrientation, invalid_region), + testing::Eq(0.0)); + + invalid_region = Rect{0, 3, 0, 2}; + EXPECT_THAT(sampleArea(buffer.data(), kWidth, kHeight, kStride, kOrientation, invalid_region), + testing::Eq(0.0)); +} + +// workaround for b/133849373 +TEST_F(RegionSamplingTest, orientation_90) { + std::generate(buffer.begin(), buffer.end(), + [n = 0]() mutable { return (n++ > (kStride * kHeight >> 1)) ? kBlack : kWhite; }); + + Rect tl_region{0, 0, 4, 4}; + EXPECT_THAT(sampleArea(buffer.data(), kWidth, kHeight, kStride, ui::Transform::ROT_0, + tl_region), + testing::Eq(1.0)); + EXPECT_THAT(sampleArea(buffer.data(), kWidth, kHeight, kStride, ui::Transform::ROT_180, + tl_region), + testing::Eq(1.0)); + EXPECT_THAT(sampleArea(buffer.data(), kWidth, kHeight, kStride, ui::Transform::ROT_90, + tl_region), + testing::Eq(0.0)); + EXPECT_THAT(sampleArea(buffer.data(), kWidth, kHeight, kStride, ui::Transform::ROT_270, + tl_region), + testing::Eq(0.0)); + + Rect br_region{kWidth - 4, kHeight - 4, kWidth, kHeight}; + EXPECT_THAT(sampleArea(buffer.data(), kWidth, kHeight, kStride, ui::Transform::ROT_0, + br_region), + testing::Eq(0.0)); + EXPECT_THAT(sampleArea(buffer.data(), kWidth, kHeight, kStride, ui::Transform::ROT_180, + br_region), + testing::Eq(0.0)); + EXPECT_THAT(sampleArea(buffer.data(), kWidth, kHeight, kStride, ui::Transform::ROT_90, + br_region), + testing::Eq(1.0)); + EXPECT_THAT(sampleArea(buffer.data(), kWidth, kHeight, kStride, ui::Transform::ROT_270, + br_region), + testing::Eq(1.0)); +} + } // namespace android -- cgit v1.2.3-59-g8ed1b From 69162d0bbc444c66ad7cdf4ad5df26798fb2cc7a Mon Sep 17 00:00:00 2001 From: Kevin DuBois Date: Tue, 4 Jun 2019 20:22:43 -0700 Subject: SF: left/right flip luma calculation 90 deg rot During 90 degree screenshots, the gles produced images were actually rotated 180, whereas current code was written to accommodate flipped vertically only. Flip coordinates during luma calc horizontally to match the correct region the luma-listening client is expecting. Test: manual testing of content with different left/right luma Bug: 132394665 Change-Id: Ia3954cf97f57f5f563978f24391e66da23282d44 --- services/surfaceflinger/RegionSamplingThread.cpp | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'services/surfaceflinger/RegionSamplingThread.cpp') diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp index 7fa33f597c..66906e950c 100644 --- a/services/surfaceflinger/RegionSamplingThread.cpp +++ b/services/surfaceflinger/RegionSamplingThread.cpp @@ -280,6 +280,10 @@ float sampleArea(const uint32_t* data, int32_t width, int32_t height, int32_t st area.top = height - area.top; area.bottom = height - area.bottom; std::swap(area.top, area.bottom); + + area.left = width - area.left; + area.right = width - area.right; + std::swap(area.left, area.right); } std::array brightnessBuckets = {}; -- cgit v1.2.3-59-g8ed1b