From 755e319d6a656dc92bd4f2b486d8f5a44b0e7350 Mon Sep 17 00:00:00 2001 From: Lloyd Pique Date: Wed, 31 Jan 2018 16:46:15 -0800 Subject: SF: Cleanup EventControlThread Primarily the goal was to eliminate the use of RefBase in various forms from EventControlThread. 1) SurfaceFlinger only needs a std::unique_ptr<> and not an android::sp<> to own the created instance. 2) Convert from android::Thread to std::thread, along with using std::mutex and std::condition_variable to keep consistency. 3) The code only needs a reference to a function to call, rather than a reference to all of SurfaceFlinger. This removes an unnecessary full dependency. 4) Switch the header to #pragma once. 5) Added Clang thread annotations and enabled the corresponding warning. 6) Simplified the thread function to eliminate unnecessary locals and indentation. 7) Added proper thread shutdown handling (invoked by dtor). Bug: None Test: Verified event control thread still works on Pixel XL Change-Id: I2d5621b0cbbfb9e0f8c5831ccfc94704c95a4a55 --- services/surfaceflinger/EventControlThread.h | 38 ++++++++++++++++------------ 1 file changed, 22 insertions(+), 16 deletions(-) (limited to 'services/surfaceflinger/EventControlThread.h') diff --git a/services/surfaceflinger/EventControlThread.h b/services/surfaceflinger/EventControlThread.h index 1b1ef75b9d..321fb79831 100644 --- a/services/surfaceflinger/EventControlThread.h +++ b/services/surfaceflinger/EventControlThread.h @@ -14,35 +14,41 @@ * limitations under the License. */ -#ifndef ANDROID_EVENTCONTROLTHREAD_H -#define ANDROID_EVENTCONTROLTHREAD_H +#pragma once -#include +#include +#include +#include +#include +#include -#include -#include +#include namespace android { class SurfaceFlinger; -class EventControlThread: public Thread { +class EventControlThread { public: + using SetVSyncEnabledFunction = std::function; - explicit EventControlThread(const sp& flinger); - virtual ~EventControlThread() {} + explicit EventControlThread(SetVSyncEnabledFunction function); + ~EventControlThread(); void setVsyncEnabled(bool enabled); - virtual bool threadLoop(); private: - sp mFlinger; - bool mVsyncEnabled; + void threadMain(); - Mutex mMutex; - Condition mCond; -}; + std::mutex mMutex; + std::condition_variable mCondition; + + const SetVSyncEnabledFunction mSetVSyncEnabled; + bool mVsyncEnabled GUARDED_BY(mMutex) = false; + bool mKeepRunning GUARDED_BY(mMutex) = true; -} + // Must be last so that everything is initialized before the thread starts. + std::thread mThread{&EventControlThread::threadMain, this}; +}; -#endif // ANDROID_EVENTCONTROLTHREAD_H +} // namespace android -- cgit v1.2.3-59-g8ed1b From 0c3a88319136a8ce0e7050ef5695610a986ce900 Mon Sep 17 00:00:00 2001 From: Lloyd Pique Date: Mon, 22 Jan 2018 17:31:47 -0800 Subject: SF: Separate EventControlThread into interface and impl Test: Builds Bug: 74827900 Change-Id: Ib79503860bf9409cc71d98e2e845ffaff114fbb1 (cherry picked from commit 379adc10ebe94eec8c7754d262c6184fbfb3f0a3) --- services/surfaceflinger/EventControlThread.cpp | 5 +++++ services/surfaceflinger/EventControlThread.h | 15 +++++++++++++-- services/surfaceflinger/SurfaceFlinger.cpp | 5 ++--- 3 files changed, 20 insertions(+), 5 deletions(-) (limited to 'services/surfaceflinger/EventControlThread.h') diff --git a/services/surfaceflinger/EventControlThread.cpp b/services/surfaceflinger/EventControlThread.cpp index ac54059360..fb6cff5705 100644 --- a/services/surfaceflinger/EventControlThread.cpp +++ b/services/surfaceflinger/EventControlThread.cpp @@ -26,6 +26,10 @@ namespace android { +EventControlThread::~EventControlThread() = default; + +namespace impl { + EventControlThread::EventControlThread(EventControlThread::SetVSyncEnabledFunction function) : mSetVSyncEnabled(function) { pthread_setname_np(mThread.native_handle(), "EventControlThread"); @@ -67,4 +71,5 @@ void EventControlThread::threadMain() NO_THREAD_SAFETY_ANALYSIS { } } +} // namespace impl } // namespace android diff --git a/services/surfaceflinger/EventControlThread.h b/services/surfaceflinger/EventControlThread.h index 321fb79831..9be4e7cd8e 100644 --- a/services/surfaceflinger/EventControlThread.h +++ b/services/surfaceflinger/EventControlThread.h @@ -16,8 +16,8 @@ #pragma once -#include #include +#include #include #include #include @@ -29,13 +29,23 @@ namespace android { class SurfaceFlinger; class EventControlThread { +public: + virtual ~EventControlThread(); + + virtual void setVsyncEnabled(bool enabled) = 0; +}; + +namespace impl { + +class EventControlThread final : public android::EventControlThread { public: using SetVSyncEnabledFunction = std::function; explicit EventControlThread(SetVSyncEnabledFunction function); ~EventControlThread(); - void setVsyncEnabled(bool enabled); + // EventControlThread implementation + void setVsyncEnabled(bool enabled) override; private: void threadMain(); @@ -51,4 +61,5 @@ private: std::thread mThread{&EventControlThread::threadMain, this}; }; +} // namespace impl } // namespace android diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 932b92e653..cdf126e9e2 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -639,9 +639,8 @@ void SurfaceFlinger::init() { } } - mEventControlThread = std::make_unique([this](bool enabled) { - setVsyncEnabled(HWC_DISPLAY_PRIMARY, enabled); - }); + mEventControlThread = std::make_unique( + [this](bool enabled) { setVsyncEnabled(HWC_DISPLAY_PRIMARY, enabled); }); // initialize our drawing state mDrawingState = mCurrentState; -- cgit v1.2.3-59-g8ed1b From 117510d0c16a73b76540f9b4e104df1bc8ac2ee6 Mon Sep 17 00:00:00 2001 From: Lloyd Pique Date: Fri, 9 Mar 2018 18:52:56 -0800 Subject: SF: Test coverage for EventControlThread Add a unit test to cover EventControlThreadTest.cpp Test: atest libsurfaceflinger_unittest Bug: 74827900 Change-Id: I6618561de9b2023586fa2f5236090beef08caf32 --- services/surfaceflinger/EventControlThread.h | 2 - services/surfaceflinger/tests/unittests/Android.bp | 1 + .../tests/unittests/EventControlThreadTest.cpp | 119 +++++++++++++++++++++ 3 files changed, 120 insertions(+), 2 deletions(-) create mode 100644 services/surfaceflinger/tests/unittests/EventControlThreadTest.cpp (limited to 'services/surfaceflinger/EventControlThread.h') diff --git a/services/surfaceflinger/EventControlThread.h b/services/surfaceflinger/EventControlThread.h index 9be4e7cd8e..cafae53400 100644 --- a/services/surfaceflinger/EventControlThread.h +++ b/services/surfaceflinger/EventControlThread.h @@ -26,8 +26,6 @@ namespace android { -class SurfaceFlinger; - class EventControlThread { public: virtual ~EventControlThread(); diff --git a/services/surfaceflinger/tests/unittests/Android.bp b/services/surfaceflinger/tests/unittests/Android.bp index 3e9f6a425c..39761dd9ad 100644 --- a/services/surfaceflinger/tests/unittests/Android.bp +++ b/services/surfaceflinger/tests/unittests/Android.bp @@ -20,6 +20,7 @@ cc_test { srcs: [ ":libsurfaceflinger_sources", "DisplayTransactionTest.cpp", + "EventControlThreadTest.cpp", "EventThreadTest.cpp", "mock/DisplayHardware/MockComposer.cpp", "mock/DisplayHardware/MockDisplaySurface.cpp", diff --git a/services/surfaceflinger/tests/unittests/EventControlThreadTest.cpp b/services/surfaceflinger/tests/unittests/EventControlThreadTest.cpp new file mode 100644 index 0000000000..b34645463f --- /dev/null +++ b/services/surfaceflinger/tests/unittests/EventControlThreadTest.cpp @@ -0,0 +1,119 @@ +/* + * Copyright (C) 2018 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 "LibSurfaceFlingerUnittests" + +#include +#include + +#include + +#include "AsyncCallRecorder.h" +#include "EventControlThread.h" + +namespace android { +namespace { + +using namespace std::chrono_literals; +using testing::_; + +class EventControlThreadTest : public testing::Test { +protected: + EventControlThreadTest(); + ~EventControlThreadTest() override; + + void createThread(); + + void expectVSyncEnableCallbackCalled(bool enable); + + AsyncCallRecorder mVSyncSetEnabledCallRecorder; + + std::unique_ptr mThread; +}; + +EventControlThreadTest::EventControlThreadTest() { + const ::testing::TestInfo* const test_info = + ::testing::UnitTest::GetInstance()->current_test_info(); + ALOGD("**** Setting up for %s.%s\n", test_info->test_case_name(), test_info->name()); +} + +EventControlThreadTest::~EventControlThreadTest() { + const ::testing::TestInfo* const test_info = + ::testing::UnitTest::GetInstance()->current_test_info(); + ALOGD("**** Tearing down after %s.%s\n", test_info->test_case_name(), test_info->name()); +} + +void EventControlThreadTest::createThread() { + mThread = std::make_unique( + mVSyncSetEnabledCallRecorder.getInvocable()); +} + +void EventControlThreadTest::expectVSyncEnableCallbackCalled(bool expectedEnabled) { + auto args = mVSyncSetEnabledCallRecorder.waitForCall(); + ASSERT_TRUE(args.has_value()); + EXPECT_EQ(std::get<0>(args.value()), expectedEnabled); +} + +/* ------------------------------------------------------------------------ + * Test cases + */ + +TEST_F(EventControlThreadTest, signalsVSyncDisabledOnStartup) { + createThread(); + + // On thread start, there should be an automatic explicit call to disable + // vsyncs + expectVSyncEnableCallbackCalled(false); +} + +TEST_F(EventControlThreadTest, signalsVSyncDisabledOnce) { + createThread(); + expectVSyncEnableCallbackCalled(false); + + mThread->setVsyncEnabled(false); + + EXPECT_FALSE(mVSyncSetEnabledCallRecorder.waitForUnexpectedCall().has_value()); +} + +TEST_F(EventControlThreadTest, signalsVSyncEnabledThenDisabled) { + createThread(); + expectVSyncEnableCallbackCalled(false); + + mThread->setVsyncEnabled(true); + + expectVSyncEnableCallbackCalled(true); + + mThread->setVsyncEnabled(false); + + expectVSyncEnableCallbackCalled(false); +} + +TEST_F(EventControlThreadTest, signalsVSyncEnabledOnce) { + createThread(); + expectVSyncEnableCallbackCalled(false); + + mThread->setVsyncEnabled(true); + + expectVSyncEnableCallbackCalled(true); + + mThread->setVsyncEnabled(true); + + EXPECT_FALSE(mVSyncSetEnabledCallRecorder.waitForUnexpectedCall().has_value()); +} + +} // namespace +} // namespace android -- cgit v1.2.3-59-g8ed1b