From c5ae0dc56d828a85b8b609ca29923e58dea3b807 Mon Sep 17 00:00:00 2001 From: Siarhei Vishniakou Date: Wed, 10 Jul 2019 15:51:18 -0700 Subject: Improve input devices changed logs Currently, "reconfiguring input devices" log is hard to read, because it dumps the values as hex. That means, the reader has to go into code to understand what the actual changes are. Furthermore, if the defines are added/removed/modified between releases, it's even harder to debug. To improve this, convert to the actual values before printing. This will also help confirm that these are only ever invoked with a single change. Maybe we can change to enum class then. Bug: 137212522 Test: adb logcat | grep -i "input devices" Change-Id: I6b40d1c785df8a57c9e2619210906d326844d69d Before: InputReader: Reconfiguring input devices. changes=0x00000010 After: InputReader: Reconfiguring input devices, changes=KEYBOARD_LAYOUTS | --- services/inputflinger/InputReaderBase.cpp | 38 +++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) (limited to 'services/inputflinger/InputReaderBase.cpp') diff --git a/services/inputflinger/InputReaderBase.cpp b/services/inputflinger/InputReaderBase.cpp index f48a64551e..bc53cf52cc 100644 --- a/services/inputflinger/InputReaderBase.cpp +++ b/services/inputflinger/InputReaderBase.cpp @@ -49,6 +49,44 @@ bool InputReaderThread::threadLoop() { // --- InputReaderConfiguration --- +std::string InputReaderConfiguration::changesToString(uint32_t changes) { + if (changes == 0) { + return ""; + } + std::string result; + if (changes & CHANGE_POINTER_SPEED) { + result += "POINTER_SPEED | "; + } + if (changes & CHANGE_POINTER_GESTURE_ENABLEMENT) { + result += "POINTER_GESTURE_ENABLEMENT | "; + } + if (changes & CHANGE_DISPLAY_INFO) { + result += "DISPLAY_INFO | "; + } + if (changes & CHANGE_SHOW_TOUCHES) { + result += "SHOW_TOUCHES | "; + } + if (changes & CHANGE_KEYBOARD_LAYOUTS) { + result += "KEYBOARD_LAYOUTS | "; + } + if (changes & CHANGE_DEVICE_ALIAS) { + result += "DEVICE_ALIAS | "; + } + if (changes & CHANGE_TOUCH_AFFINE_TRANSFORMATION) { + result += "TOUCH_AFFINE_TRANSFORMATION | "; + } + if (changes & CHANGE_EXTERNAL_STYLUS_PRESENCE) { + result += "EXTERNAL_STYLUS_PRESENCE | "; + } + if (changes & CHANGE_ENABLED_STATE) { + result += "ENABLED_STATE | "; + } + if (changes & CHANGE_MUST_REOPEN) { + result += "MUST_REOPEN | "; + } + return result; +} + std::optional InputReaderConfiguration::getDisplayViewportByUniqueId( const std::string& uniqueDisplayId) const { if (uniqueDisplayId.empty()) { -- cgit v1.2.3-59-g8ed1b From 7d3847311e3d3955ea91facb0b83b824e16efdd4 Mon Sep 17 00:00:00 2001 From: Prabir Pradhan Date: Wed, 25 Sep 2019 15:07:32 -0700 Subject: getDisplayViewportByType doesn't count more than one viewport Bug: none Test: none Change-Id: I795f14bb1d7bd7f62a0d2a6eb301f3cced4f5109 --- services/inputflinger/InputReaderBase.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'services/inputflinger/InputReaderBase.cpp') diff --git a/services/inputflinger/InputReaderBase.cpp b/services/inputflinger/InputReaderBase.cpp index bc53cf52cc..0422d8342b 100644 --- a/services/inputflinger/InputReaderBase.cpp +++ b/services/inputflinger/InputReaderBase.cpp @@ -114,8 +114,10 @@ std::optional InputReaderConfiguration::getDisplayViewportByTyp std::optional result = std::nullopt; for (const DisplayViewport& currentViewport : mDisplays) { // Return the first match - if (currentViewport.type == type && !result) { - result = std::make_optional(currentViewport); + if (currentViewport.type == type) { + if (!result) { + result = std::make_optional(currentViewport); + } count++; } } -- cgit v1.2.3-59-g8ed1b From ba266f24e580de2d8339050b62fad7e66441f843 Mon Sep 17 00:00:00 2001 From: Prabir Pradhan Date: Tue, 3 Sep 2019 17:11:27 -0700 Subject: Let InputReader handle its own thread Previously, InputManager created and managed the InputReaderThread, which interacted with InputReader through the loopOnce() method in InputReaderInterface. We move the threading logic from InputManager to InputReader by removing the loopOnce() method from InputReaderInterface and adding a start() and stop() method in its place. Once InputReader is created, InputManager will call InputReaderInterface::start(), which creates and starts InputReader's own thread. InputManager can also call InputReaderInterface::stop() to halt further processing on InputReader's thread. Bug: 130819454 Test: atest inputflinger_tests Test: Touch input works on crosshatch Change-Id: Ic732436d4f00a831e317be1f16ac106a11652cff --- services/inputflinger/InputManager.cpp | 9 ++-- services/inputflinger/InputManager.h | 11 +++-- services/inputflinger/InputReaderBase.cpp | 14 ------- services/inputflinger/include/InputReaderBase.h | 48 +++++++++++----------- services/inputflinger/reader/InputReader.cpp | 47 ++++++++++++++++++++- services/inputflinger/reader/include/InputReader.h | 16 ++++++-- services/inputflinger/tests/InputReader_test.cpp | 44 ++++++++------------ 7 files changed, 108 insertions(+), 81 deletions(-) (limited to 'services/inputflinger/InputReaderBase.cpp') diff --git a/services/inputflinger/InputManager.cpp b/services/inputflinger/InputManager.cpp index 7d3067242a..34a04dfd95 100644 --- a/services/inputflinger/InputManager.cpp +++ b/services/inputflinger/InputManager.cpp @@ -46,7 +46,6 @@ InputManager::~InputManager() { } void InputManager::initialize() { - mReaderThread = new InputReaderThread(mReader); mDispatcherThread = new InputDispatcherThread(mDispatcher); } @@ -57,9 +56,9 @@ status_t InputManager::start() { return result; } - result = mReaderThread->run("InputReader", PRIORITY_URGENT_DISPLAY); + result = mReader->start(); if (result) { - ALOGE("Could not start InputReader thread due to error %d.", result); + ALOGE("Could not start InputReader due to error %d.", result); mDispatcherThread->requestExit(); return result; @@ -69,9 +68,9 @@ status_t InputManager::start() { } status_t InputManager::stop() { - status_t result = mReaderThread->requestExitAndWait(); + status_t result = mReader->stop(); if (result) { - ALOGW("Could not stop InputReader thread due to error %d.", result); + ALOGW("Could not stop InputReader due to error %d.", result); } result = mDispatcherThread->requestExitAndWait(); diff --git a/services/inputflinger/InputManager.h b/services/inputflinger/InputManager.h index 40f66d82f4..2a7ed0ff44 100644 --- a/services/inputflinger/InputManager.h +++ b/services/inputflinger/InputManager.h @@ -43,15 +43,15 @@ class InputDispatcherThread; /* * The input manager is the core of the system event processing. * - * The input manager uses two threads. + * The input manager has two components. * - * 1. The InputReaderThread (called "InputReader") reads and preprocesses raw input events, - * applies policy, and posts messages to a queue managed by the DispatcherThread. + * 1. The InputReader class starts a thread that reads and preprocesses raw input events, applies + * policy, and posts messages to a queue managed by the InputDispatcherThread. * 2. The InputDispatcherThread (called "InputDispatcher") thread waits for new events on the * queue and asynchronously dispatches them to applications. * - * By design, the InputReaderThread class and InputDispatcherThread class do not share any - * internal state. Moreover, all communication is done one way from the InputReaderThread + * By design, the InputReader class and InputDispatcherThread class do not share any + * internal state. Moreover, all communication is done one way from the InputReader * into the InputDispatcherThread and never the reverse. Both classes may interact with the * InputDispatchPolicy, however. * @@ -102,7 +102,6 @@ public: private: sp mReader; - sp mReaderThread; sp mClassifier; diff --git a/services/inputflinger/InputReaderBase.cpp b/services/inputflinger/InputReaderBase.cpp index 0422d8342b..2d6f2c1cc9 100644 --- a/services/inputflinger/InputReaderBase.cpp +++ b/services/inputflinger/InputReaderBase.cpp @@ -33,20 +33,6 @@ using android::base::StringPrintf; namespace android { -// --- InputReaderThread --- - -InputReaderThread::InputReaderThread(const sp& reader) : - Thread(/*canCallJava*/ true), mReader(reader) { -} - -InputReaderThread::~InputReaderThread() { -} - -bool InputReaderThread::threadLoop() { - mReader->loopOnce(); - return true; -} - // --- InputReaderConfiguration --- std::string InputReaderConfiguration::changesToString(uint32_t changes) { diff --git a/services/inputflinger/include/InputReaderBase.h b/services/inputflinger/include/InputReaderBase.h index 5d576b94f3..56c0a7356d 100644 --- a/services/inputflinger/include/InputReaderBase.h +++ b/services/inputflinger/include/InputReaderBase.h @@ -19,12 +19,12 @@ #include "PointerControllerInterface.h" +#include #include #include -#include #include #include -#include +#include #include #include @@ -44,7 +44,16 @@ namespace android { -/* Processes raw input events and sends cooked event data to an input listener. */ +// --- InputReaderInterface --- + +/* The interface for the InputReader shared library. + * + * Manages one or more threads that process raw input events and sends cooked event data to an + * input listener. + * + * The implementation must guarantee thread safety for this interface. However, since the input + * listener is NOT thread safe, all calls to the listener must happen from the same thread. + */ class InputReaderInterface : public virtual RefBase { protected: InputReaderInterface() { } @@ -56,18 +65,17 @@ public: * This method may be called on any thread (usually by the input manager). */ virtual void dump(std::string& dump) = 0; - /* Called by the heatbeat to ensures that the reader has not deadlocked. */ + /* Called by the heartbeat to ensures that the reader has not deadlocked. */ virtual void monitor() = 0; /* Returns true if the input device is enabled. */ virtual bool isInputDeviceEnabled(int32_t deviceId) = 0; - /* Runs a single iteration of the processing loop. - * Nominally reads and processes one incoming message from the EventHub. - * - * This method should be called on the input reader thread. - */ - virtual void loopOnce() = 0; + /* Makes the reader start processing events from the kernel. */ + virtual status_t start() = 0; + + /* Makes the reader stop processing any more events. */ + virtual status_t stop() = 0; /* Gets information about all input devices. * @@ -104,17 +112,7 @@ public: virtual bool canDispatchToDisplay(int32_t deviceId, int32_t displayId) = 0; }; -/* Reads raw events from the event hub and processes them, endlessly. */ -class InputReaderThread : public Thread { -public: - explicit InputReaderThread(const sp& reader); - virtual ~InputReaderThread(); - -private: - sp mReader; - - virtual bool threadLoop(); -}; +// --- InputReaderConfiguration --- /* * Input reader configuration. @@ -285,6 +283,8 @@ private: std::vector mDisplays; }; +// --- TouchAffineTransformation --- + struct TouchAffineTransformation { float x_scale; float x_ymix; @@ -307,6 +307,8 @@ struct TouchAffineTransformation { void applyTo(float& x, float& y) const; }; +// --- InputReaderPolicyInterface --- + /* * Input reader policy interface. * @@ -316,8 +318,8 @@ struct TouchAffineTransformation { * The actual implementation is partially supported by callbacks into the DVM * via JNI. This interface is also mocked in the unit tests. * - * These methods must NOT re-enter the input reader since they may be called while - * holding the input reader lock. + * These methods will NOT re-enter the input reader interface, so they may be called from + * any method in the input reader interface. */ class InputReaderPolicyInterface : public virtual RefBase { protected: diff --git a/services/inputflinger/reader/InputReader.cpp b/services/inputflinger/reader/InputReader.cpp index e57604cbe8..5d52bbc91f 100644 --- a/services/inputflinger/reader/InputReader.cpp +++ b/services/inputflinger/reader/InputReader.cpp @@ -38,16 +38,38 @@ #include #include +#include #include #include #include - +#include using android::base::StringPrintf; namespace android { +// --- InputReader::InputReaderThread --- + +/* Thread that reads raw events from the event hub and processes them, endlessly. */ +class InputReader::InputReaderThread : public Thread { +public: + explicit InputReaderThread(InputReader* reader) + : Thread(/* canCallJava */ true), mReader(reader) {} + + ~InputReaderThread() {} + +private: + InputReader* mReader; + + bool threadLoop() override { + mReader->loopOnce(); + return true; + } +}; + +// --- InputReader --- + InputReader::InputReader(std::shared_ptr eventHub, const sp& policy, const sp& listener) @@ -61,6 +83,7 @@ InputReader::InputReader(std::shared_ptr eventHub, mNextTimeout(LLONG_MAX), mConfigurationChangesToRefresh(0) { mQueuedListener = new QueuedInputListener(listener); + mThread = new InputReaderThread(this); { // acquire lock AutoMutex _l(mLock); @@ -76,6 +99,28 @@ InputReader::~InputReader() { } } +status_t InputReader::start() { + if (mThread->isRunning()) { + return ALREADY_EXISTS; + } + return mThread->run("InputReader", PRIORITY_URGENT_DISPLAY); +} + +status_t InputReader::stop() { + if (!mThread->isRunning()) { + return OK; + } + if (gettid() == mThread->getTid()) { + ALOGE("InputReader can only be stopped from outside of the InputReaderThread!"); + return INVALID_OPERATION; + } + // Directly calling requestExitAndWait() causes the thread to not exit + // if mEventHub is waiting for a long timeout. + mThread->requestExit(); + mEventHub->wake(); + return mThread->requestExitAndWait(); +} + void InputReader::loopOnce() { int32_t oldGeneration; int32_t timeoutMillis; diff --git a/services/inputflinger/reader/include/InputReader.h b/services/inputflinger/reader/include/InputReader.h index 7b4321ea82..3cf4535a03 100644 --- a/services/inputflinger/reader/include/InputReader.h +++ b/services/inputflinger/reader/include/InputReader.h @@ -38,12 +38,12 @@ struct StylusState; * that it sends to the input listener. Some functions of the input reader, such as early * event filtering in low power states, are controlled by a separate policy object. * - * The InputReader owns a collection of InputMappers. Most of the work it does happens - * on the input reader thread but the InputReader can receive queries from other system + * The InputReader owns a collection of InputMappers. InputReader starts its own thread, where + * most of the work happens, but the InputReader can receive queries from other system * components running on arbitrary threads. To keep things manageable, the InputReader * uses a single Mutex to guard its state. The Mutex may be held while calling into the * EventHub or the InputReaderPolicy but it is never held while calling into the - * InputListener. + * InputListener. All calls to InputListener must happen from InputReader's thread. */ class InputReader : public InputReaderInterface { public: @@ -55,7 +55,8 @@ public: virtual void dump(std::string& dump) override; virtual void monitor() override; - virtual void loopOnce() override; + virtual status_t start() override; + virtual status_t stop() override; virtual void getInputDevices(std::vector& outInputDevices) override; @@ -111,6 +112,9 @@ protected: friend class ContextImpl; private: + class InputReaderThread; + sp mThread; + Mutex mLock; Condition mReaderIsAliveCondition; @@ -133,6 +137,10 @@ private: KeyedVector mDevices; + // With each iteration of the loop, InputReader reads and processes one incoming message from + // the EventHub. + void loopOnce(); + // low-level input event decoding and device management void processEventsLocked(const RawEvent* rawEvents, size_t count); diff --git a/services/inputflinger/tests/InputReader_test.cpp b/services/inputflinger/tests/InputReader_test.cpp index 2b3257d132..59f275f58d 100644 --- a/services/inputflinger/tests/InputReader_test.cpp +++ b/services/inputflinger/tests/InputReader_test.cpp @@ -1133,12 +1133,8 @@ class InputReaderPolicyTest : public testing::Test { protected: sp mFakePolicy; - virtual void SetUp() { - mFakePolicy = new FakeInputReaderPolicy(); - } - virtual void TearDown() { - mFakePolicy.clear(); - } + virtual void SetUp() override { mFakePolicy = new FakeInputReaderPolicy(); } + virtual void TearDown() override { mFakePolicy.clear(); } }; /** @@ -1321,18 +1317,20 @@ protected: sp mFakeListener; sp mFakePolicy; std::shared_ptr mFakeEventHub; - sp mReader; + std::unique_ptr mReader; - virtual void SetUp() { + virtual void SetUp() override { mFakeEventHub = std::make_unique(); mFakePolicy = new FakeInputReaderPolicy(); mFakeListener = new TestInputListener(); - mReader = new InstrumentedInputReader(mFakeEventHub, mFakePolicy, mFakeListener); + mReader = std::make_unique(mFakeEventHub, mFakePolicy, + mFakeListener); + ASSERT_EQ(OK, mReader->start()); } - virtual void TearDown() { - mReader.clear(); + virtual void TearDown() override { + ASSERT_EQ(OK, mReader->stop()); mFakeListener.clear(); mFakePolicy.clear(); @@ -1346,8 +1344,6 @@ protected: mFakeEventHub->addConfigurationMap(deviceId, configuration); } mFakeEventHub->finishDeviceScan(); - mReader->loopOnce(); - mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakePolicy->assertInputDevicesChanged()); ASSERT_NO_FATAL_FAILURE(mFakeEventHub->assertQueueIsEmpty()); } @@ -1422,7 +1418,6 @@ TEST_F(InputReaderTest, WhenEnabledChanges_SendsDeviceResetNotification) { ASSERT_EQ(device->isEnabled(), true); disableDevice(deviceId, device); - mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); ASSERT_EQ(ARBITRARY_TIME, resetArgs.eventTime); @@ -1430,13 +1425,11 @@ TEST_F(InputReaderTest, WhenEnabledChanges_SendsDeviceResetNotification) { ASSERT_EQ(device->isEnabled(), false); disableDevice(deviceId, device); - mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasNotCalled()); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyConfigurationChangedWasNotCalled()); ASSERT_EQ(device->isEnabled(), false); enableDevice(deviceId, device); - mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); ASSERT_EQ(ARBITRARY_TIME, resetArgs.eventTime); ASSERT_EQ(deviceId, resetArgs.deviceId); @@ -1560,7 +1553,7 @@ TEST_F(InputReaderTest, MarkSupportedKeyCodes_ForwardsRequestsToMappers) { ASSERT_TRUE(flags[0] && flags[1] && !flags[2] && !flags[3]); } -TEST_F(InputReaderTest, LoopOnce_WhenDeviceScanFinished_SendsConfigurationChanged) { +TEST_F(InputReaderTest, WhenDeviceScanFinished_SendsConfigurationChanged) { addDevice(1, "ignored", INPUT_DEVICE_CLASS_KEYBOARD, nullptr); NotifyConfigurationChangedArgs args; @@ -1569,13 +1562,12 @@ TEST_F(InputReaderTest, LoopOnce_WhenDeviceScanFinished_SendsConfigurationChange ASSERT_EQ(ARBITRARY_TIME, args.eventTime); } -TEST_F(InputReaderTest, LoopOnce_ForwardsRawEventsToMappers) { +TEST_F(InputReaderTest, ForwardsRawEventsToMappers) { FakeInputMapper* mapper = nullptr; ASSERT_NO_FATAL_FAILURE(mapper = addDeviceWithFakeInputMapper(1, 0, "fake", INPUT_DEVICE_CLASS_KEYBOARD, AINPUT_SOURCE_KEYBOARD, nullptr)); mFakeEventHub->enqueueEvent(0, 1, EV_KEY, KEY_A, 1); - mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeEventHub->assertQueueIsEmpty()); RawEvent event; @@ -1602,19 +1594,16 @@ TEST_F(InputReaderTest, DeviceReset_IncrementsSequenceNumber) { uint32_t prevSequenceNum = resetArgs.sequenceNum; disableDevice(deviceId, device); - mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); ASSERT_TRUE(prevSequenceNum < resetArgs.sequenceNum); prevSequenceNum = resetArgs.sequenceNum; enableDevice(deviceId, device); - mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); ASSERT_TRUE(prevSequenceNum < resetArgs.sequenceNum); prevSequenceNum = resetArgs.sequenceNum; disableDevice(deviceId, device); - mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); ASSERT_TRUE(prevSequenceNum < resetArgs.sequenceNum); prevSequenceNum = resetArgs.sequenceNum; @@ -1642,7 +1631,6 @@ TEST_F(InputReaderTest, Device_CanDispatchToDisplay) { mFakePolicy->addDisplayViewport(SECONDARY_DISPLAY_ID, DISPLAY_WIDTH, DISPLAY_HEIGHT, DISPLAY_ORIENTATION_0, "local:1", hdmi1, ViewportType::VIEWPORT_EXTERNAL); mReader->requestRefreshConfiguration(InputReaderConfiguration::CHANGE_DISPLAY_INFO); - mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyConfigurationChangedWasCalled()); // Device should only dispatch to the specified display. @@ -1674,7 +1662,7 @@ protected: InputDevice* mDevice; - virtual void SetUp() { + virtual void SetUp() override { mFakeEventHub = std::make_unique(); mFakePolicy = new FakeInputReaderPolicy(); mFakeListener = new TestInputListener(); @@ -1688,7 +1676,7 @@ protected: DEVICE_CONTROLLER_NUMBER, identifier, DEVICE_CLASSES); } - virtual void TearDown() { + virtual void TearDown() override { delete mDevice; delete mFakeContext; @@ -1912,7 +1900,7 @@ protected: FakeInputReaderContext* mFakeContext; InputDevice* mDevice; - virtual void SetUp() { + virtual void SetUp() override { mFakeEventHub = std::make_unique(); mFakePolicy = new FakeInputReaderPolicy(); mFakeListener = new TestInputListener(); @@ -1926,7 +1914,7 @@ protected: mFakeEventHub->addDevice(mDevice->getId(), DEVICE_NAME, 0); } - virtual void TearDown() { + virtual void TearDown() override { delete mDevice; delete mFakeContext; mFakeListener.clear(); @@ -2589,7 +2577,7 @@ protected: sp mFakePointerController; - virtual void SetUp() { + virtual void SetUp() override { InputMapperTest::SetUp(); mFakePointerController = new FakePointerController(); -- cgit v1.2.3-59-g8ed1b From 95a4ed6e84e9b8845359f601050218fa28459f4b Mon Sep 17 00:00:00 2001 From: Siarhei Vishniakou Date: Fri, 1 Nov 2019 05:53:44 +0000 Subject: Revert "Let InputReader handle its own thread" This reverts commit ba266f24e580de2d8339050b62fad7e66441f843. Reason for revert: b/143735040 looks like this CL breaks input tests Change-Id: I8c19046935d2bdaf9df5df97b2eff43308065c72 --- services/inputflinger/InputManager.cpp | 9 ++-- services/inputflinger/InputManager.h | 11 ++--- services/inputflinger/InputReaderBase.cpp | 14 +++++++ services/inputflinger/include/InputReaderBase.h | 48 +++++++++++----------- services/inputflinger/reader/InputReader.cpp | 47 +-------------------- services/inputflinger/reader/include/InputReader.h | 16 ++------ services/inputflinger/tests/InputReader_test.cpp | 44 ++++++++++++-------- 7 files changed, 81 insertions(+), 108 deletions(-) (limited to 'services/inputflinger/InputReaderBase.cpp') diff --git a/services/inputflinger/InputManager.cpp b/services/inputflinger/InputManager.cpp index 34a04dfd95..7d3067242a 100644 --- a/services/inputflinger/InputManager.cpp +++ b/services/inputflinger/InputManager.cpp @@ -46,6 +46,7 @@ InputManager::~InputManager() { } void InputManager::initialize() { + mReaderThread = new InputReaderThread(mReader); mDispatcherThread = new InputDispatcherThread(mDispatcher); } @@ -56,9 +57,9 @@ status_t InputManager::start() { return result; } - result = mReader->start(); + result = mReaderThread->run("InputReader", PRIORITY_URGENT_DISPLAY); if (result) { - ALOGE("Could not start InputReader due to error %d.", result); + ALOGE("Could not start InputReader thread due to error %d.", result); mDispatcherThread->requestExit(); return result; @@ -68,9 +69,9 @@ status_t InputManager::start() { } status_t InputManager::stop() { - status_t result = mReader->stop(); + status_t result = mReaderThread->requestExitAndWait(); if (result) { - ALOGW("Could not stop InputReader due to error %d.", result); + ALOGW("Could not stop InputReader thread due to error %d.", result); } result = mDispatcherThread->requestExitAndWait(); diff --git a/services/inputflinger/InputManager.h b/services/inputflinger/InputManager.h index 2a7ed0ff44..40f66d82f4 100644 --- a/services/inputflinger/InputManager.h +++ b/services/inputflinger/InputManager.h @@ -43,15 +43,15 @@ class InputDispatcherThread; /* * The input manager is the core of the system event processing. * - * The input manager has two components. + * The input manager uses two threads. * - * 1. The InputReader class starts a thread that reads and preprocesses raw input events, applies - * policy, and posts messages to a queue managed by the InputDispatcherThread. + * 1. The InputReaderThread (called "InputReader") reads and preprocesses raw input events, + * applies policy, and posts messages to a queue managed by the DispatcherThread. * 2. The InputDispatcherThread (called "InputDispatcher") thread waits for new events on the * queue and asynchronously dispatches them to applications. * - * By design, the InputReader class and InputDispatcherThread class do not share any - * internal state. Moreover, all communication is done one way from the InputReader + * By design, the InputReaderThread class and InputDispatcherThread class do not share any + * internal state. Moreover, all communication is done one way from the InputReaderThread * into the InputDispatcherThread and never the reverse. Both classes may interact with the * InputDispatchPolicy, however. * @@ -102,6 +102,7 @@ public: private: sp mReader; + sp mReaderThread; sp mClassifier; diff --git a/services/inputflinger/InputReaderBase.cpp b/services/inputflinger/InputReaderBase.cpp index 2d6f2c1cc9..0422d8342b 100644 --- a/services/inputflinger/InputReaderBase.cpp +++ b/services/inputflinger/InputReaderBase.cpp @@ -33,6 +33,20 @@ using android::base::StringPrintf; namespace android { +// --- InputReaderThread --- + +InputReaderThread::InputReaderThread(const sp& reader) : + Thread(/*canCallJava*/ true), mReader(reader) { +} + +InputReaderThread::~InputReaderThread() { +} + +bool InputReaderThread::threadLoop() { + mReader->loopOnce(); + return true; +} + // --- InputReaderConfiguration --- std::string InputReaderConfiguration::changesToString(uint32_t changes) { diff --git a/services/inputflinger/include/InputReaderBase.h b/services/inputflinger/include/InputReaderBase.h index 56c0a7356d..5d576b94f3 100644 --- a/services/inputflinger/include/InputReaderBase.h +++ b/services/inputflinger/include/InputReaderBase.h @@ -19,12 +19,12 @@ #include "PointerControllerInterface.h" -#include #include #include +#include #include #include -#include +#include #include #include @@ -44,16 +44,7 @@ namespace android { -// --- InputReaderInterface --- - -/* The interface for the InputReader shared library. - * - * Manages one or more threads that process raw input events and sends cooked event data to an - * input listener. - * - * The implementation must guarantee thread safety for this interface. However, since the input - * listener is NOT thread safe, all calls to the listener must happen from the same thread. - */ +/* Processes raw input events and sends cooked event data to an input listener. */ class InputReaderInterface : public virtual RefBase { protected: InputReaderInterface() { } @@ -65,17 +56,18 @@ public: * This method may be called on any thread (usually by the input manager). */ virtual void dump(std::string& dump) = 0; - /* Called by the heartbeat to ensures that the reader has not deadlocked. */ + /* Called by the heatbeat to ensures that the reader has not deadlocked. */ virtual void monitor() = 0; /* Returns true if the input device is enabled. */ virtual bool isInputDeviceEnabled(int32_t deviceId) = 0; - /* Makes the reader start processing events from the kernel. */ - virtual status_t start() = 0; - - /* Makes the reader stop processing any more events. */ - virtual status_t stop() = 0; + /* Runs a single iteration of the processing loop. + * Nominally reads and processes one incoming message from the EventHub. + * + * This method should be called on the input reader thread. + */ + virtual void loopOnce() = 0; /* Gets information about all input devices. * @@ -112,7 +104,17 @@ public: virtual bool canDispatchToDisplay(int32_t deviceId, int32_t displayId) = 0; }; -// --- InputReaderConfiguration --- +/* Reads raw events from the event hub and processes them, endlessly. */ +class InputReaderThread : public Thread { +public: + explicit InputReaderThread(const sp& reader); + virtual ~InputReaderThread(); + +private: + sp mReader; + + virtual bool threadLoop(); +}; /* * Input reader configuration. @@ -283,8 +285,6 @@ private: std::vector mDisplays; }; -// --- TouchAffineTransformation --- - struct TouchAffineTransformation { float x_scale; float x_ymix; @@ -307,8 +307,6 @@ struct TouchAffineTransformation { void applyTo(float& x, float& y) const; }; -// --- InputReaderPolicyInterface --- - /* * Input reader policy interface. * @@ -318,8 +316,8 @@ struct TouchAffineTransformation { * The actual implementation is partially supported by callbacks into the DVM * via JNI. This interface is also mocked in the unit tests. * - * These methods will NOT re-enter the input reader interface, so they may be called from - * any method in the input reader interface. + * These methods must NOT re-enter the input reader since they may be called while + * holding the input reader lock. */ class InputReaderPolicyInterface : public virtual RefBase { protected: diff --git a/services/inputflinger/reader/InputReader.cpp b/services/inputflinger/reader/InputReader.cpp index 5d52bbc91f..e57604cbe8 100644 --- a/services/inputflinger/reader/InputReader.cpp +++ b/services/inputflinger/reader/InputReader.cpp @@ -38,38 +38,16 @@ #include #include -#include #include #include #include -#include + using android::base::StringPrintf; namespace android { -// --- InputReader::InputReaderThread --- - -/* Thread that reads raw events from the event hub and processes them, endlessly. */ -class InputReader::InputReaderThread : public Thread { -public: - explicit InputReaderThread(InputReader* reader) - : Thread(/* canCallJava */ true), mReader(reader) {} - - ~InputReaderThread() {} - -private: - InputReader* mReader; - - bool threadLoop() override { - mReader->loopOnce(); - return true; - } -}; - -// --- InputReader --- - InputReader::InputReader(std::shared_ptr eventHub, const sp& policy, const sp& listener) @@ -83,7 +61,6 @@ InputReader::InputReader(std::shared_ptr eventHub, mNextTimeout(LLONG_MAX), mConfigurationChangesToRefresh(0) { mQueuedListener = new QueuedInputListener(listener); - mThread = new InputReaderThread(this); { // acquire lock AutoMutex _l(mLock); @@ -99,28 +76,6 @@ InputReader::~InputReader() { } } -status_t InputReader::start() { - if (mThread->isRunning()) { - return ALREADY_EXISTS; - } - return mThread->run("InputReader", PRIORITY_URGENT_DISPLAY); -} - -status_t InputReader::stop() { - if (!mThread->isRunning()) { - return OK; - } - if (gettid() == mThread->getTid()) { - ALOGE("InputReader can only be stopped from outside of the InputReaderThread!"); - return INVALID_OPERATION; - } - // Directly calling requestExitAndWait() causes the thread to not exit - // if mEventHub is waiting for a long timeout. - mThread->requestExit(); - mEventHub->wake(); - return mThread->requestExitAndWait(); -} - void InputReader::loopOnce() { int32_t oldGeneration; int32_t timeoutMillis; diff --git a/services/inputflinger/reader/include/InputReader.h b/services/inputflinger/reader/include/InputReader.h index 3cf4535a03..7b4321ea82 100644 --- a/services/inputflinger/reader/include/InputReader.h +++ b/services/inputflinger/reader/include/InputReader.h @@ -38,12 +38,12 @@ struct StylusState; * that it sends to the input listener. Some functions of the input reader, such as early * event filtering in low power states, are controlled by a separate policy object. * - * The InputReader owns a collection of InputMappers. InputReader starts its own thread, where - * most of the work happens, but the InputReader can receive queries from other system + * The InputReader owns a collection of InputMappers. Most of the work it does happens + * on the input reader thread but the InputReader can receive queries from other system * components running on arbitrary threads. To keep things manageable, the InputReader * uses a single Mutex to guard its state. The Mutex may be held while calling into the * EventHub or the InputReaderPolicy but it is never held while calling into the - * InputListener. All calls to InputListener must happen from InputReader's thread. + * InputListener. */ class InputReader : public InputReaderInterface { public: @@ -55,8 +55,7 @@ public: virtual void dump(std::string& dump) override; virtual void monitor() override; - virtual status_t start() override; - virtual status_t stop() override; + virtual void loopOnce() override; virtual void getInputDevices(std::vector& outInputDevices) override; @@ -112,9 +111,6 @@ protected: friend class ContextImpl; private: - class InputReaderThread; - sp mThread; - Mutex mLock; Condition mReaderIsAliveCondition; @@ -137,10 +133,6 @@ private: KeyedVector mDevices; - // With each iteration of the loop, InputReader reads and processes one incoming message from - // the EventHub. - void loopOnce(); - // low-level input event decoding and device management void processEventsLocked(const RawEvent* rawEvents, size_t count); diff --git a/services/inputflinger/tests/InputReader_test.cpp b/services/inputflinger/tests/InputReader_test.cpp index 59f275f58d..2b3257d132 100644 --- a/services/inputflinger/tests/InputReader_test.cpp +++ b/services/inputflinger/tests/InputReader_test.cpp @@ -1133,8 +1133,12 @@ class InputReaderPolicyTest : public testing::Test { protected: sp mFakePolicy; - virtual void SetUp() override { mFakePolicy = new FakeInputReaderPolicy(); } - virtual void TearDown() override { mFakePolicy.clear(); } + virtual void SetUp() { + mFakePolicy = new FakeInputReaderPolicy(); + } + virtual void TearDown() { + mFakePolicy.clear(); + } }; /** @@ -1317,20 +1321,18 @@ protected: sp mFakeListener; sp mFakePolicy; std::shared_ptr mFakeEventHub; - std::unique_ptr mReader; + sp mReader; - virtual void SetUp() override { + virtual void SetUp() { mFakeEventHub = std::make_unique(); mFakePolicy = new FakeInputReaderPolicy(); mFakeListener = new TestInputListener(); - mReader = std::make_unique(mFakeEventHub, mFakePolicy, - mFakeListener); - ASSERT_EQ(OK, mReader->start()); + mReader = new InstrumentedInputReader(mFakeEventHub, mFakePolicy, mFakeListener); } - virtual void TearDown() override { - ASSERT_EQ(OK, mReader->stop()); + virtual void TearDown() { + mReader.clear(); mFakeListener.clear(); mFakePolicy.clear(); @@ -1344,6 +1346,8 @@ protected: mFakeEventHub->addConfigurationMap(deviceId, configuration); } mFakeEventHub->finishDeviceScan(); + mReader->loopOnce(); + mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakePolicy->assertInputDevicesChanged()); ASSERT_NO_FATAL_FAILURE(mFakeEventHub->assertQueueIsEmpty()); } @@ -1418,6 +1422,7 @@ TEST_F(InputReaderTest, WhenEnabledChanges_SendsDeviceResetNotification) { ASSERT_EQ(device->isEnabled(), true); disableDevice(deviceId, device); + mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); ASSERT_EQ(ARBITRARY_TIME, resetArgs.eventTime); @@ -1425,11 +1430,13 @@ TEST_F(InputReaderTest, WhenEnabledChanges_SendsDeviceResetNotification) { ASSERT_EQ(device->isEnabled(), false); disableDevice(deviceId, device); + mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasNotCalled()); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyConfigurationChangedWasNotCalled()); ASSERT_EQ(device->isEnabled(), false); enableDevice(deviceId, device); + mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); ASSERT_EQ(ARBITRARY_TIME, resetArgs.eventTime); ASSERT_EQ(deviceId, resetArgs.deviceId); @@ -1553,7 +1560,7 @@ TEST_F(InputReaderTest, MarkSupportedKeyCodes_ForwardsRequestsToMappers) { ASSERT_TRUE(flags[0] && flags[1] && !flags[2] && !flags[3]); } -TEST_F(InputReaderTest, WhenDeviceScanFinished_SendsConfigurationChanged) { +TEST_F(InputReaderTest, LoopOnce_WhenDeviceScanFinished_SendsConfigurationChanged) { addDevice(1, "ignored", INPUT_DEVICE_CLASS_KEYBOARD, nullptr); NotifyConfigurationChangedArgs args; @@ -1562,12 +1569,13 @@ TEST_F(InputReaderTest, WhenDeviceScanFinished_SendsConfigurationChanged) { ASSERT_EQ(ARBITRARY_TIME, args.eventTime); } -TEST_F(InputReaderTest, ForwardsRawEventsToMappers) { +TEST_F(InputReaderTest, LoopOnce_ForwardsRawEventsToMappers) { FakeInputMapper* mapper = nullptr; ASSERT_NO_FATAL_FAILURE(mapper = addDeviceWithFakeInputMapper(1, 0, "fake", INPUT_DEVICE_CLASS_KEYBOARD, AINPUT_SOURCE_KEYBOARD, nullptr)); mFakeEventHub->enqueueEvent(0, 1, EV_KEY, KEY_A, 1); + mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeEventHub->assertQueueIsEmpty()); RawEvent event; @@ -1594,16 +1602,19 @@ TEST_F(InputReaderTest, DeviceReset_IncrementsSequenceNumber) { uint32_t prevSequenceNum = resetArgs.sequenceNum; disableDevice(deviceId, device); + mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); ASSERT_TRUE(prevSequenceNum < resetArgs.sequenceNum); prevSequenceNum = resetArgs.sequenceNum; enableDevice(deviceId, device); + mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); ASSERT_TRUE(prevSequenceNum < resetArgs.sequenceNum); prevSequenceNum = resetArgs.sequenceNum; disableDevice(deviceId, device); + mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); ASSERT_TRUE(prevSequenceNum < resetArgs.sequenceNum); prevSequenceNum = resetArgs.sequenceNum; @@ -1631,6 +1642,7 @@ TEST_F(InputReaderTest, Device_CanDispatchToDisplay) { mFakePolicy->addDisplayViewport(SECONDARY_DISPLAY_ID, DISPLAY_WIDTH, DISPLAY_HEIGHT, DISPLAY_ORIENTATION_0, "local:1", hdmi1, ViewportType::VIEWPORT_EXTERNAL); mReader->requestRefreshConfiguration(InputReaderConfiguration::CHANGE_DISPLAY_INFO); + mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyConfigurationChangedWasCalled()); // Device should only dispatch to the specified display. @@ -1662,7 +1674,7 @@ protected: InputDevice* mDevice; - virtual void SetUp() override { + virtual void SetUp() { mFakeEventHub = std::make_unique(); mFakePolicy = new FakeInputReaderPolicy(); mFakeListener = new TestInputListener(); @@ -1676,7 +1688,7 @@ protected: DEVICE_CONTROLLER_NUMBER, identifier, DEVICE_CLASSES); } - virtual void TearDown() override { + virtual void TearDown() { delete mDevice; delete mFakeContext; @@ -1900,7 +1912,7 @@ protected: FakeInputReaderContext* mFakeContext; InputDevice* mDevice; - virtual void SetUp() override { + virtual void SetUp() { mFakeEventHub = std::make_unique(); mFakePolicy = new FakeInputReaderPolicy(); mFakeListener = new TestInputListener(); @@ -1914,7 +1926,7 @@ protected: mFakeEventHub->addDevice(mDevice->getId(), DEVICE_NAME, 0); } - virtual void TearDown() override { + virtual void TearDown() { delete mDevice; delete mFakeContext; mFakeListener.clear(); @@ -2577,7 +2589,7 @@ protected: sp mFakePointerController; - virtual void SetUp() override { + virtual void SetUp() { InputMapperTest::SetUp(); mFakePointerController = new FakePointerController(); -- cgit v1.2.3-59-g8ed1b From f15a8aa344946fb274b1db9f546d5fed746cae70 Mon Sep 17 00:00:00 2001 From: Prabir Pradhan Date: Tue, 5 Nov 2019 01:10:04 +0000 Subject: Reland "Let InputReader handle its own thread" This CL was first landed in Ic732436d4f00a831e317be1f16ac106a11652cff but was reverted due to flaky tests. The flaky tests were caused by races between the instrumented test classes and the InputReader class under test, which now runs in a new thread. In addition to re-landing the change, this CL fixes the flaky tests by changing the tests to eliminate the race condition. - InputReaderTest should send a configuration change request to InputReader every time a device is enabled or disabled, and the test should wait for notifyDeviceReset to be called on the input listener to ensure it was enabled/disabled successfully. Bug: 130819454 Test: atest inputflinger_tests Test: Touch input works on crosshatch Change-Id: I822d3c33384ebdc1bc850a40534e942a27a79ec9 --- services/inputflinger/InputManager.cpp | 9 ++- services/inputflinger/InputManager.h | 11 ++-- services/inputflinger/InputReaderBase.cpp | 14 ----- services/inputflinger/include/InputReaderBase.h | 48 ++++++++-------- services/inputflinger/reader/InputReader.cpp | 47 ++++++++++++++- services/inputflinger/reader/include/InputReader.h | 16 ++++-- services/inputflinger/tests/InputReader_test.cpp | 66 +++++++++------------- 7 files changed, 120 insertions(+), 91 deletions(-) (limited to 'services/inputflinger/InputReaderBase.cpp') diff --git a/services/inputflinger/InputManager.cpp b/services/inputflinger/InputManager.cpp index e7640dd6af..1043390f84 100644 --- a/services/inputflinger/InputManager.cpp +++ b/services/inputflinger/InputManager.cpp @@ -46,7 +46,6 @@ InputManager::~InputManager() { } void InputManager::initialize() { - mReaderThread = new InputReaderThread(mReader); mDispatcherThread = new InputDispatcherThread(mDispatcher); } @@ -57,9 +56,9 @@ status_t InputManager::start() { return result; } - result = mReaderThread->run("InputReader", PRIORITY_URGENT_DISPLAY); + result = mReader->start(); if (result) { - ALOGE("Could not start InputReader thread due to error %d.", result); + ALOGE("Could not start InputReader due to error %d.", result); mDispatcherThread->requestExit(); return result; @@ -69,9 +68,9 @@ status_t InputManager::start() { } status_t InputManager::stop() { - status_t result = mReaderThread->requestExitAndWait(); + status_t result = mReader->stop(); if (result) { - ALOGW("Could not stop InputReader thread due to error %d.", result); + ALOGW("Could not stop InputReader due to error %d.", result); } result = mDispatcherThread->requestExitAndWait(); diff --git a/services/inputflinger/InputManager.h b/services/inputflinger/InputManager.h index 40f66d82f4..2a7ed0ff44 100644 --- a/services/inputflinger/InputManager.h +++ b/services/inputflinger/InputManager.h @@ -43,15 +43,15 @@ class InputDispatcherThread; /* * The input manager is the core of the system event processing. * - * The input manager uses two threads. + * The input manager has two components. * - * 1. The InputReaderThread (called "InputReader") reads and preprocesses raw input events, - * applies policy, and posts messages to a queue managed by the DispatcherThread. + * 1. The InputReader class starts a thread that reads and preprocesses raw input events, applies + * policy, and posts messages to a queue managed by the InputDispatcherThread. * 2. The InputDispatcherThread (called "InputDispatcher") thread waits for new events on the * queue and asynchronously dispatches them to applications. * - * By design, the InputReaderThread class and InputDispatcherThread class do not share any - * internal state. Moreover, all communication is done one way from the InputReaderThread + * By design, the InputReader class and InputDispatcherThread class do not share any + * internal state. Moreover, all communication is done one way from the InputReader * into the InputDispatcherThread and never the reverse. Both classes may interact with the * InputDispatchPolicy, however. * @@ -102,7 +102,6 @@ public: private: sp mReader; - sp mReaderThread; sp mClassifier; diff --git a/services/inputflinger/InputReaderBase.cpp b/services/inputflinger/InputReaderBase.cpp index 0422d8342b..2d6f2c1cc9 100644 --- a/services/inputflinger/InputReaderBase.cpp +++ b/services/inputflinger/InputReaderBase.cpp @@ -33,20 +33,6 @@ using android::base::StringPrintf; namespace android { -// --- InputReaderThread --- - -InputReaderThread::InputReaderThread(const sp& reader) : - Thread(/*canCallJava*/ true), mReader(reader) { -} - -InputReaderThread::~InputReaderThread() { -} - -bool InputReaderThread::threadLoop() { - mReader->loopOnce(); - return true; -} - // --- InputReaderConfiguration --- std::string InputReaderConfiguration::changesToString(uint32_t changes) { diff --git a/services/inputflinger/include/InputReaderBase.h b/services/inputflinger/include/InputReaderBase.h index 5d576b94f3..56c0a7356d 100644 --- a/services/inputflinger/include/InputReaderBase.h +++ b/services/inputflinger/include/InputReaderBase.h @@ -19,12 +19,12 @@ #include "PointerControllerInterface.h" +#include #include #include -#include #include #include -#include +#include #include #include @@ -44,7 +44,16 @@ namespace android { -/* Processes raw input events and sends cooked event data to an input listener. */ +// --- InputReaderInterface --- + +/* The interface for the InputReader shared library. + * + * Manages one or more threads that process raw input events and sends cooked event data to an + * input listener. + * + * The implementation must guarantee thread safety for this interface. However, since the input + * listener is NOT thread safe, all calls to the listener must happen from the same thread. + */ class InputReaderInterface : public virtual RefBase { protected: InputReaderInterface() { } @@ -56,18 +65,17 @@ public: * This method may be called on any thread (usually by the input manager). */ virtual void dump(std::string& dump) = 0; - /* Called by the heatbeat to ensures that the reader has not deadlocked. */ + /* Called by the heartbeat to ensures that the reader has not deadlocked. */ virtual void monitor() = 0; /* Returns true if the input device is enabled. */ virtual bool isInputDeviceEnabled(int32_t deviceId) = 0; - /* Runs a single iteration of the processing loop. - * Nominally reads and processes one incoming message from the EventHub. - * - * This method should be called on the input reader thread. - */ - virtual void loopOnce() = 0; + /* Makes the reader start processing events from the kernel. */ + virtual status_t start() = 0; + + /* Makes the reader stop processing any more events. */ + virtual status_t stop() = 0; /* Gets information about all input devices. * @@ -104,17 +112,7 @@ public: virtual bool canDispatchToDisplay(int32_t deviceId, int32_t displayId) = 0; }; -/* Reads raw events from the event hub and processes them, endlessly. */ -class InputReaderThread : public Thread { -public: - explicit InputReaderThread(const sp& reader); - virtual ~InputReaderThread(); - -private: - sp mReader; - - virtual bool threadLoop(); -}; +// --- InputReaderConfiguration --- /* * Input reader configuration. @@ -285,6 +283,8 @@ private: std::vector mDisplays; }; +// --- TouchAffineTransformation --- + struct TouchAffineTransformation { float x_scale; float x_ymix; @@ -307,6 +307,8 @@ struct TouchAffineTransformation { void applyTo(float& x, float& y) const; }; +// --- InputReaderPolicyInterface --- + /* * Input reader policy interface. * @@ -316,8 +318,8 @@ struct TouchAffineTransformation { * The actual implementation is partially supported by callbacks into the DVM * via JNI. This interface is also mocked in the unit tests. * - * These methods must NOT re-enter the input reader since they may be called while - * holding the input reader lock. + * These methods will NOT re-enter the input reader interface, so they may be called from + * any method in the input reader interface. */ class InputReaderPolicyInterface : public virtual RefBase { protected: diff --git a/services/inputflinger/reader/InputReader.cpp b/services/inputflinger/reader/InputReader.cpp index e57604cbe8..5d52bbc91f 100644 --- a/services/inputflinger/reader/InputReader.cpp +++ b/services/inputflinger/reader/InputReader.cpp @@ -38,16 +38,38 @@ #include #include +#include #include #include #include - +#include using android::base::StringPrintf; namespace android { +// --- InputReader::InputReaderThread --- + +/* Thread that reads raw events from the event hub and processes them, endlessly. */ +class InputReader::InputReaderThread : public Thread { +public: + explicit InputReaderThread(InputReader* reader) + : Thread(/* canCallJava */ true), mReader(reader) {} + + ~InputReaderThread() {} + +private: + InputReader* mReader; + + bool threadLoop() override { + mReader->loopOnce(); + return true; + } +}; + +// --- InputReader --- + InputReader::InputReader(std::shared_ptr eventHub, const sp& policy, const sp& listener) @@ -61,6 +83,7 @@ InputReader::InputReader(std::shared_ptr eventHub, mNextTimeout(LLONG_MAX), mConfigurationChangesToRefresh(0) { mQueuedListener = new QueuedInputListener(listener); + mThread = new InputReaderThread(this); { // acquire lock AutoMutex _l(mLock); @@ -76,6 +99,28 @@ InputReader::~InputReader() { } } +status_t InputReader::start() { + if (mThread->isRunning()) { + return ALREADY_EXISTS; + } + return mThread->run("InputReader", PRIORITY_URGENT_DISPLAY); +} + +status_t InputReader::stop() { + if (!mThread->isRunning()) { + return OK; + } + if (gettid() == mThread->getTid()) { + ALOGE("InputReader can only be stopped from outside of the InputReaderThread!"); + return INVALID_OPERATION; + } + // Directly calling requestExitAndWait() causes the thread to not exit + // if mEventHub is waiting for a long timeout. + mThread->requestExit(); + mEventHub->wake(); + return mThread->requestExitAndWait(); +} + void InputReader::loopOnce() { int32_t oldGeneration; int32_t timeoutMillis; diff --git a/services/inputflinger/reader/include/InputReader.h b/services/inputflinger/reader/include/InputReader.h index 7b4321ea82..3cf4535a03 100644 --- a/services/inputflinger/reader/include/InputReader.h +++ b/services/inputflinger/reader/include/InputReader.h @@ -38,12 +38,12 @@ struct StylusState; * that it sends to the input listener. Some functions of the input reader, such as early * event filtering in low power states, are controlled by a separate policy object. * - * The InputReader owns a collection of InputMappers. Most of the work it does happens - * on the input reader thread but the InputReader can receive queries from other system + * The InputReader owns a collection of InputMappers. InputReader starts its own thread, where + * most of the work happens, but the InputReader can receive queries from other system * components running on arbitrary threads. To keep things manageable, the InputReader * uses a single Mutex to guard its state. The Mutex may be held while calling into the * EventHub or the InputReaderPolicy but it is never held while calling into the - * InputListener. + * InputListener. All calls to InputListener must happen from InputReader's thread. */ class InputReader : public InputReaderInterface { public: @@ -55,7 +55,8 @@ public: virtual void dump(std::string& dump) override; virtual void monitor() override; - virtual void loopOnce() override; + virtual status_t start() override; + virtual status_t stop() override; virtual void getInputDevices(std::vector& outInputDevices) override; @@ -111,6 +112,9 @@ protected: friend class ContextImpl; private: + class InputReaderThread; + sp mThread; + Mutex mLock; Condition mReaderIsAliveCondition; @@ -133,6 +137,10 @@ private: KeyedVector mDevices; + // With each iteration of the loop, InputReader reads and processes one incoming message from + // the EventHub. + void loopOnce(); + // low-level input event decoding and device management void processEventsLocked(const RawEvent* rawEvents, size_t count); diff --git a/services/inputflinger/tests/InputReader_test.cpp b/services/inputflinger/tests/InputReader_test.cpp index c1c912214a..8d4ab6afbd 100644 --- a/services/inputflinger/tests/InputReader_test.cpp +++ b/services/inputflinger/tests/InputReader_test.cpp @@ -1133,12 +1133,8 @@ class InputReaderPolicyTest : public testing::Test { protected: sp mFakePolicy; - virtual void SetUp() { - mFakePolicy = new FakeInputReaderPolicy(); - } - virtual void TearDown() { - mFakePolicy.clear(); - } + virtual void SetUp() override { mFakePolicy = new FakeInputReaderPolicy(); } + virtual void TearDown() override { mFakePolicy.clear(); } }; /** @@ -1321,18 +1317,20 @@ protected: sp mFakeListener; sp mFakePolicy; std::shared_ptr mFakeEventHub; - sp mReader; + std::unique_ptr mReader; - virtual void SetUp() { + virtual void SetUp() override { mFakeEventHub = std::make_unique(); mFakePolicy = new FakeInputReaderPolicy(); mFakeListener = new TestInputListener(); - mReader = new InstrumentedInputReader(mFakeEventHub, mFakePolicy, mFakeListener); + mReader = std::make_unique(mFakeEventHub, mFakePolicy, + mFakeListener); + ASSERT_EQ(OK, mReader->start()); } - virtual void TearDown() { - mReader.clear(); + virtual void TearDown() override { + ASSERT_EQ(OK, mReader->stop()); mFakeListener.clear(); mFakePolicy.clear(); @@ -1346,24 +1344,18 @@ protected: mFakeEventHub->addConfigurationMap(deviceId, configuration); } mFakeEventHub->finishDeviceScan(); - mReader->loopOnce(); - mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakePolicy->assertInputDevicesChanged()); ASSERT_NO_FATAL_FAILURE(mFakeEventHub->assertQueueIsEmpty()); } void disableDevice(int32_t deviceId, InputDevice* device) { mFakePolicy->addDisabledDevice(deviceId); - configureDevice(InputReaderConfiguration::CHANGE_ENABLED_STATE, device); + mReader->requestRefreshConfiguration(InputReaderConfiguration::CHANGE_ENABLED_STATE); } void enableDevice(int32_t deviceId, InputDevice* device) { mFakePolicy->removeDisabledDevice(deviceId); - configureDevice(InputReaderConfiguration::CHANGE_ENABLED_STATE, device); - } - - void configureDevice(uint32_t changes, InputDevice* device) { - device->configure(ARBITRARY_TIME, mFakePolicy->getReaderConfiguration(), changes); + mReader->requestRefreshConfiguration(InputReaderConfiguration::CHANGE_ENABLED_STATE); } FakeInputMapper* addDeviceWithFakeInputMapper(int32_t deviceId, int32_t controllerNumber, @@ -1417,28 +1409,22 @@ TEST_F(InputReaderTest, WhenEnabledChanges_SendsDeviceResetNotification) { NotifyDeviceResetArgs resetArgs; ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); - ASSERT_EQ(ARBITRARY_TIME, resetArgs.eventTime); ASSERT_EQ(deviceId, resetArgs.deviceId); ASSERT_EQ(device->isEnabled(), true); disableDevice(deviceId, device); - mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); - ASSERT_EQ(ARBITRARY_TIME, resetArgs.eventTime); ASSERT_EQ(deviceId, resetArgs.deviceId); ASSERT_EQ(device->isEnabled(), false); disableDevice(deviceId, device); - mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasNotCalled()); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyConfigurationChangedWasNotCalled()); ASSERT_EQ(device->isEnabled(), false); enableDevice(deviceId, device); - mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); - ASSERT_EQ(ARBITRARY_TIME, resetArgs.eventTime); ASSERT_EQ(deviceId, resetArgs.deviceId); ASSERT_EQ(device->isEnabled(), true); } @@ -1560,7 +1546,7 @@ TEST_F(InputReaderTest, MarkSupportedKeyCodes_ForwardsRequestsToMappers) { ASSERT_TRUE(flags[0] && flags[1] && !flags[2] && !flags[3]); } -TEST_F(InputReaderTest, LoopOnce_WhenDeviceScanFinished_SendsConfigurationChanged) { +TEST_F(InputReaderTest, WhenDeviceScanFinished_SendsConfigurationChanged) { addDevice(1, "ignored", INPUT_DEVICE_CLASS_KEYBOARD, nullptr); NotifyConfigurationChangedArgs args; @@ -1569,13 +1555,12 @@ TEST_F(InputReaderTest, LoopOnce_WhenDeviceScanFinished_SendsConfigurationChange ASSERT_EQ(ARBITRARY_TIME, args.eventTime); } -TEST_F(InputReaderTest, LoopOnce_ForwardsRawEventsToMappers) { +TEST_F(InputReaderTest, ForwardsRawEventsToMappers) { FakeInputMapper* mapper = nullptr; ASSERT_NO_FATAL_FAILURE(mapper = addDeviceWithFakeInputMapper(1, 0, "fake", INPUT_DEVICE_CLASS_KEYBOARD, AINPUT_SOURCE_KEYBOARD, nullptr)); mFakeEventHub->enqueueEvent(0, 1, EV_KEY, KEY_A, 1); - mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeEventHub->assertQueueIsEmpty()); RawEvent event; @@ -1602,19 +1587,16 @@ TEST_F(InputReaderTest, DeviceReset_IncrementsSequenceNumber) { uint32_t prevSequenceNum = resetArgs.sequenceNum; disableDevice(deviceId, device); - mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); ASSERT_TRUE(prevSequenceNum < resetArgs.sequenceNum); prevSequenceNum = resetArgs.sequenceNum; enableDevice(deviceId, device); - mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); ASSERT_TRUE(prevSequenceNum < resetArgs.sequenceNum); prevSequenceNum = resetArgs.sequenceNum; disableDevice(deviceId, device); - mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); ASSERT_TRUE(prevSequenceNum < resetArgs.sequenceNum); prevSequenceNum = resetArgs.sequenceNum; @@ -1629,7 +1611,6 @@ TEST_F(InputReaderTest, Device_CanDispatchToDisplay) { FakeInputMapper* mapper = new FakeInputMapper(device, AINPUT_SOURCE_TOUCHSCREEN); device->addMapper(mapper); mReader->setNextDevice(device); - addDevice(deviceId, "fake", deviceClass, nullptr); const uint8_t hdmi1 = 1; @@ -1637,13 +1618,20 @@ TEST_F(InputReaderTest, Device_CanDispatchToDisplay) { mFakePolicy->addInputPortAssociation(DEVICE_LOCATION, hdmi1); // Add default and second display. + mFakePolicy->clearViewports(); mFakePolicy->addDisplayViewport(DISPLAY_ID, DISPLAY_WIDTH, DISPLAY_HEIGHT, DISPLAY_ORIENTATION_0, "local:0", NO_PORT, ViewportType::VIEWPORT_INTERNAL); mFakePolicy->addDisplayViewport(SECONDARY_DISPLAY_ID, DISPLAY_WIDTH, DISPLAY_HEIGHT, DISPLAY_ORIENTATION_0, "local:1", hdmi1, ViewportType::VIEWPORT_EXTERNAL); mReader->requestRefreshConfiguration(InputReaderConfiguration::CHANGE_DISPLAY_INFO); - mReader->loopOnce(); + + // Add the device, and make sure all of the callbacks are triggered. + // The device is added after the input port associations are processed since + // we do not yet support dynamic device-to-display associations. + ASSERT_NO_FATAL_FAILURE(addDevice(deviceId, "fake", deviceClass, nullptr)); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyConfigurationChangedWasCalled()); + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled()); + ASSERT_NO_FATAL_FAILURE(mapper->assertConfigureWasCalled()); // Device should only dispatch to the specified display. ASSERT_EQ(deviceId, device->getId()); @@ -1652,6 +1640,8 @@ TEST_F(InputReaderTest, Device_CanDispatchToDisplay) { // Can't dispatch event from a disabled device. disableDevice(deviceId, device); + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled()); + ASSERT_NO_FATAL_FAILURE(mapper->assertConfigureWasCalled()); ASSERT_FALSE(mReader->canDispatchToDisplay(deviceId, SECONDARY_DISPLAY_ID)); } @@ -1674,7 +1664,7 @@ protected: InputDevice* mDevice; - virtual void SetUp() { + virtual void SetUp() override { mFakeEventHub = std::make_unique(); mFakePolicy = new FakeInputReaderPolicy(); mFakeListener = new TestInputListener(); @@ -1688,7 +1678,7 @@ protected: DEVICE_CONTROLLER_NUMBER, identifier, DEVICE_CLASSES); } - virtual void TearDown() { + virtual void TearDown() override { delete mDevice; delete mFakeContext; @@ -1912,7 +1902,7 @@ protected: FakeInputReaderContext* mFakeContext; InputDevice* mDevice; - virtual void SetUp() { + virtual void SetUp() override { mFakeEventHub = std::make_unique(); mFakePolicy = new FakeInputReaderPolicy(); mFakeListener = new TestInputListener(); @@ -1926,7 +1916,7 @@ protected: mFakeEventHub->addDevice(mDevice->getId(), DEVICE_NAME, 0); } - virtual void TearDown() { + virtual void TearDown() override { delete mDevice; delete mFakeContext; mFakeListener.clear(); @@ -2589,7 +2579,7 @@ protected: sp mFakePointerController; - virtual void SetUp() { + virtual void SetUp() override { InputMapperTest::SetUp(); mFakePointerController = new FakePointerController(); -- cgit v1.2.3-59-g8ed1b From 6cbc978d76d1c3b5d48f8018dc291bc987d5ff0f Mon Sep 17 00:00:00 2001 From: Siarhei Vishniakou Date: Fri, 15 Nov 2019 17:59:25 +0000 Subject: Revert "Reland "Let InputReader handle its own thread"" Reason for revert: flaky tests b/144546498 Bug: 144546498 Change-Id: Id5a4c8dc8e634b23b560dde6843b5a5860305e5f --- services/inputflinger/InputManager.cpp | 9 +-- services/inputflinger/InputManager.h | 11 ++-- services/inputflinger/InputReaderBase.cpp | 14 +++++ services/inputflinger/include/InputReaderBase.h | 48 ++++++++-------- services/inputflinger/reader/InputReader.cpp | 47 +-------------- services/inputflinger/reader/include/InputReader.h | 16 ++---- services/inputflinger/tests/InputReader_test.cpp | 66 +++++++++++++--------- 7 files changed, 91 insertions(+), 120 deletions(-) (limited to 'services/inputflinger/InputReaderBase.cpp') diff --git a/services/inputflinger/InputManager.cpp b/services/inputflinger/InputManager.cpp index 1043390f84..e7640dd6af 100644 --- a/services/inputflinger/InputManager.cpp +++ b/services/inputflinger/InputManager.cpp @@ -46,6 +46,7 @@ InputManager::~InputManager() { } void InputManager::initialize() { + mReaderThread = new InputReaderThread(mReader); mDispatcherThread = new InputDispatcherThread(mDispatcher); } @@ -56,9 +57,9 @@ status_t InputManager::start() { return result; } - result = mReader->start(); + result = mReaderThread->run("InputReader", PRIORITY_URGENT_DISPLAY); if (result) { - ALOGE("Could not start InputReader due to error %d.", result); + ALOGE("Could not start InputReader thread due to error %d.", result); mDispatcherThread->requestExit(); return result; @@ -68,9 +69,9 @@ status_t InputManager::start() { } status_t InputManager::stop() { - status_t result = mReader->stop(); + status_t result = mReaderThread->requestExitAndWait(); if (result) { - ALOGW("Could not stop InputReader due to error %d.", result); + ALOGW("Could not stop InputReader thread due to error %d.", result); } result = mDispatcherThread->requestExitAndWait(); diff --git a/services/inputflinger/InputManager.h b/services/inputflinger/InputManager.h index 2a7ed0ff44..40f66d82f4 100644 --- a/services/inputflinger/InputManager.h +++ b/services/inputflinger/InputManager.h @@ -43,15 +43,15 @@ class InputDispatcherThread; /* * The input manager is the core of the system event processing. * - * The input manager has two components. + * The input manager uses two threads. * - * 1. The InputReader class starts a thread that reads and preprocesses raw input events, applies - * policy, and posts messages to a queue managed by the InputDispatcherThread. + * 1. The InputReaderThread (called "InputReader") reads and preprocesses raw input events, + * applies policy, and posts messages to a queue managed by the DispatcherThread. * 2. The InputDispatcherThread (called "InputDispatcher") thread waits for new events on the * queue and asynchronously dispatches them to applications. * - * By design, the InputReader class and InputDispatcherThread class do not share any - * internal state. Moreover, all communication is done one way from the InputReader + * By design, the InputReaderThread class and InputDispatcherThread class do not share any + * internal state. Moreover, all communication is done one way from the InputReaderThread * into the InputDispatcherThread and never the reverse. Both classes may interact with the * InputDispatchPolicy, however. * @@ -102,6 +102,7 @@ public: private: sp mReader; + sp mReaderThread; sp mClassifier; diff --git a/services/inputflinger/InputReaderBase.cpp b/services/inputflinger/InputReaderBase.cpp index 2d6f2c1cc9..0422d8342b 100644 --- a/services/inputflinger/InputReaderBase.cpp +++ b/services/inputflinger/InputReaderBase.cpp @@ -33,6 +33,20 @@ using android::base::StringPrintf; namespace android { +// --- InputReaderThread --- + +InputReaderThread::InputReaderThread(const sp& reader) : + Thread(/*canCallJava*/ true), mReader(reader) { +} + +InputReaderThread::~InputReaderThread() { +} + +bool InputReaderThread::threadLoop() { + mReader->loopOnce(); + return true; +} + // --- InputReaderConfiguration --- std::string InputReaderConfiguration::changesToString(uint32_t changes) { diff --git a/services/inputflinger/include/InputReaderBase.h b/services/inputflinger/include/InputReaderBase.h index 56c0a7356d..5d576b94f3 100644 --- a/services/inputflinger/include/InputReaderBase.h +++ b/services/inputflinger/include/InputReaderBase.h @@ -19,12 +19,12 @@ #include "PointerControllerInterface.h" -#include #include #include +#include #include #include -#include +#include #include #include @@ -44,16 +44,7 @@ namespace android { -// --- InputReaderInterface --- - -/* The interface for the InputReader shared library. - * - * Manages one or more threads that process raw input events and sends cooked event data to an - * input listener. - * - * The implementation must guarantee thread safety for this interface. However, since the input - * listener is NOT thread safe, all calls to the listener must happen from the same thread. - */ +/* Processes raw input events and sends cooked event data to an input listener. */ class InputReaderInterface : public virtual RefBase { protected: InputReaderInterface() { } @@ -65,17 +56,18 @@ public: * This method may be called on any thread (usually by the input manager). */ virtual void dump(std::string& dump) = 0; - /* Called by the heartbeat to ensures that the reader has not deadlocked. */ + /* Called by the heatbeat to ensures that the reader has not deadlocked. */ virtual void monitor() = 0; /* Returns true if the input device is enabled. */ virtual bool isInputDeviceEnabled(int32_t deviceId) = 0; - /* Makes the reader start processing events from the kernel. */ - virtual status_t start() = 0; - - /* Makes the reader stop processing any more events. */ - virtual status_t stop() = 0; + /* Runs a single iteration of the processing loop. + * Nominally reads and processes one incoming message from the EventHub. + * + * This method should be called on the input reader thread. + */ + virtual void loopOnce() = 0; /* Gets information about all input devices. * @@ -112,7 +104,17 @@ public: virtual bool canDispatchToDisplay(int32_t deviceId, int32_t displayId) = 0; }; -// --- InputReaderConfiguration --- +/* Reads raw events from the event hub and processes them, endlessly. */ +class InputReaderThread : public Thread { +public: + explicit InputReaderThread(const sp& reader); + virtual ~InputReaderThread(); + +private: + sp mReader; + + virtual bool threadLoop(); +}; /* * Input reader configuration. @@ -283,8 +285,6 @@ private: std::vector mDisplays; }; -// --- TouchAffineTransformation --- - struct TouchAffineTransformation { float x_scale; float x_ymix; @@ -307,8 +307,6 @@ struct TouchAffineTransformation { void applyTo(float& x, float& y) const; }; -// --- InputReaderPolicyInterface --- - /* * Input reader policy interface. * @@ -318,8 +316,8 @@ struct TouchAffineTransformation { * The actual implementation is partially supported by callbacks into the DVM * via JNI. This interface is also mocked in the unit tests. * - * These methods will NOT re-enter the input reader interface, so they may be called from - * any method in the input reader interface. + * These methods must NOT re-enter the input reader since they may be called while + * holding the input reader lock. */ class InputReaderPolicyInterface : public virtual RefBase { protected: diff --git a/services/inputflinger/reader/InputReader.cpp b/services/inputflinger/reader/InputReader.cpp index 5d52bbc91f..e57604cbe8 100644 --- a/services/inputflinger/reader/InputReader.cpp +++ b/services/inputflinger/reader/InputReader.cpp @@ -38,38 +38,16 @@ #include #include -#include #include #include #include -#include + using android::base::StringPrintf; namespace android { -// --- InputReader::InputReaderThread --- - -/* Thread that reads raw events from the event hub and processes them, endlessly. */ -class InputReader::InputReaderThread : public Thread { -public: - explicit InputReaderThread(InputReader* reader) - : Thread(/* canCallJava */ true), mReader(reader) {} - - ~InputReaderThread() {} - -private: - InputReader* mReader; - - bool threadLoop() override { - mReader->loopOnce(); - return true; - } -}; - -// --- InputReader --- - InputReader::InputReader(std::shared_ptr eventHub, const sp& policy, const sp& listener) @@ -83,7 +61,6 @@ InputReader::InputReader(std::shared_ptr eventHub, mNextTimeout(LLONG_MAX), mConfigurationChangesToRefresh(0) { mQueuedListener = new QueuedInputListener(listener); - mThread = new InputReaderThread(this); { // acquire lock AutoMutex _l(mLock); @@ -99,28 +76,6 @@ InputReader::~InputReader() { } } -status_t InputReader::start() { - if (mThread->isRunning()) { - return ALREADY_EXISTS; - } - return mThread->run("InputReader", PRIORITY_URGENT_DISPLAY); -} - -status_t InputReader::stop() { - if (!mThread->isRunning()) { - return OK; - } - if (gettid() == mThread->getTid()) { - ALOGE("InputReader can only be stopped from outside of the InputReaderThread!"); - return INVALID_OPERATION; - } - // Directly calling requestExitAndWait() causes the thread to not exit - // if mEventHub is waiting for a long timeout. - mThread->requestExit(); - mEventHub->wake(); - return mThread->requestExitAndWait(); -} - void InputReader::loopOnce() { int32_t oldGeneration; int32_t timeoutMillis; diff --git a/services/inputflinger/reader/include/InputReader.h b/services/inputflinger/reader/include/InputReader.h index 3cf4535a03..7b4321ea82 100644 --- a/services/inputflinger/reader/include/InputReader.h +++ b/services/inputflinger/reader/include/InputReader.h @@ -38,12 +38,12 @@ struct StylusState; * that it sends to the input listener. Some functions of the input reader, such as early * event filtering in low power states, are controlled by a separate policy object. * - * The InputReader owns a collection of InputMappers. InputReader starts its own thread, where - * most of the work happens, but the InputReader can receive queries from other system + * The InputReader owns a collection of InputMappers. Most of the work it does happens + * on the input reader thread but the InputReader can receive queries from other system * components running on arbitrary threads. To keep things manageable, the InputReader * uses a single Mutex to guard its state. The Mutex may be held while calling into the * EventHub or the InputReaderPolicy but it is never held while calling into the - * InputListener. All calls to InputListener must happen from InputReader's thread. + * InputListener. */ class InputReader : public InputReaderInterface { public: @@ -55,8 +55,7 @@ public: virtual void dump(std::string& dump) override; virtual void monitor() override; - virtual status_t start() override; - virtual status_t stop() override; + virtual void loopOnce() override; virtual void getInputDevices(std::vector& outInputDevices) override; @@ -112,9 +111,6 @@ protected: friend class ContextImpl; private: - class InputReaderThread; - sp mThread; - Mutex mLock; Condition mReaderIsAliveCondition; @@ -137,10 +133,6 @@ private: KeyedVector mDevices; - // With each iteration of the loop, InputReader reads and processes one incoming message from - // the EventHub. - void loopOnce(); - // low-level input event decoding and device management void processEventsLocked(const RawEvent* rawEvents, size_t count); diff --git a/services/inputflinger/tests/InputReader_test.cpp b/services/inputflinger/tests/InputReader_test.cpp index 8d4ab6afbd..c1c912214a 100644 --- a/services/inputflinger/tests/InputReader_test.cpp +++ b/services/inputflinger/tests/InputReader_test.cpp @@ -1133,8 +1133,12 @@ class InputReaderPolicyTest : public testing::Test { protected: sp mFakePolicy; - virtual void SetUp() override { mFakePolicy = new FakeInputReaderPolicy(); } - virtual void TearDown() override { mFakePolicy.clear(); } + virtual void SetUp() { + mFakePolicy = new FakeInputReaderPolicy(); + } + virtual void TearDown() { + mFakePolicy.clear(); + } }; /** @@ -1317,20 +1321,18 @@ protected: sp mFakeListener; sp mFakePolicy; std::shared_ptr mFakeEventHub; - std::unique_ptr mReader; + sp mReader; - virtual void SetUp() override { + virtual void SetUp() { mFakeEventHub = std::make_unique(); mFakePolicy = new FakeInputReaderPolicy(); mFakeListener = new TestInputListener(); - mReader = std::make_unique(mFakeEventHub, mFakePolicy, - mFakeListener); - ASSERT_EQ(OK, mReader->start()); + mReader = new InstrumentedInputReader(mFakeEventHub, mFakePolicy, mFakeListener); } - virtual void TearDown() override { - ASSERT_EQ(OK, mReader->stop()); + virtual void TearDown() { + mReader.clear(); mFakeListener.clear(); mFakePolicy.clear(); @@ -1344,18 +1346,24 @@ protected: mFakeEventHub->addConfigurationMap(deviceId, configuration); } mFakeEventHub->finishDeviceScan(); + mReader->loopOnce(); + mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakePolicy->assertInputDevicesChanged()); ASSERT_NO_FATAL_FAILURE(mFakeEventHub->assertQueueIsEmpty()); } void disableDevice(int32_t deviceId, InputDevice* device) { mFakePolicy->addDisabledDevice(deviceId); - mReader->requestRefreshConfiguration(InputReaderConfiguration::CHANGE_ENABLED_STATE); + configureDevice(InputReaderConfiguration::CHANGE_ENABLED_STATE, device); } void enableDevice(int32_t deviceId, InputDevice* device) { mFakePolicy->removeDisabledDevice(deviceId); - mReader->requestRefreshConfiguration(InputReaderConfiguration::CHANGE_ENABLED_STATE); + configureDevice(InputReaderConfiguration::CHANGE_ENABLED_STATE, device); + } + + void configureDevice(uint32_t changes, InputDevice* device) { + device->configure(ARBITRARY_TIME, mFakePolicy->getReaderConfiguration(), changes); } FakeInputMapper* addDeviceWithFakeInputMapper(int32_t deviceId, int32_t controllerNumber, @@ -1409,22 +1417,28 @@ TEST_F(InputReaderTest, WhenEnabledChanges_SendsDeviceResetNotification) { NotifyDeviceResetArgs resetArgs; ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); + ASSERT_EQ(ARBITRARY_TIME, resetArgs.eventTime); ASSERT_EQ(deviceId, resetArgs.deviceId); ASSERT_EQ(device->isEnabled(), true); disableDevice(deviceId, device); + mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); + ASSERT_EQ(ARBITRARY_TIME, resetArgs.eventTime); ASSERT_EQ(deviceId, resetArgs.deviceId); ASSERT_EQ(device->isEnabled(), false); disableDevice(deviceId, device); + mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasNotCalled()); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyConfigurationChangedWasNotCalled()); ASSERT_EQ(device->isEnabled(), false); enableDevice(deviceId, device); + mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); + ASSERT_EQ(ARBITRARY_TIME, resetArgs.eventTime); ASSERT_EQ(deviceId, resetArgs.deviceId); ASSERT_EQ(device->isEnabled(), true); } @@ -1546,7 +1560,7 @@ TEST_F(InputReaderTest, MarkSupportedKeyCodes_ForwardsRequestsToMappers) { ASSERT_TRUE(flags[0] && flags[1] && !flags[2] && !flags[3]); } -TEST_F(InputReaderTest, WhenDeviceScanFinished_SendsConfigurationChanged) { +TEST_F(InputReaderTest, LoopOnce_WhenDeviceScanFinished_SendsConfigurationChanged) { addDevice(1, "ignored", INPUT_DEVICE_CLASS_KEYBOARD, nullptr); NotifyConfigurationChangedArgs args; @@ -1555,12 +1569,13 @@ TEST_F(InputReaderTest, WhenDeviceScanFinished_SendsConfigurationChanged) { ASSERT_EQ(ARBITRARY_TIME, args.eventTime); } -TEST_F(InputReaderTest, ForwardsRawEventsToMappers) { +TEST_F(InputReaderTest, LoopOnce_ForwardsRawEventsToMappers) { FakeInputMapper* mapper = nullptr; ASSERT_NO_FATAL_FAILURE(mapper = addDeviceWithFakeInputMapper(1, 0, "fake", INPUT_DEVICE_CLASS_KEYBOARD, AINPUT_SOURCE_KEYBOARD, nullptr)); mFakeEventHub->enqueueEvent(0, 1, EV_KEY, KEY_A, 1); + mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeEventHub->assertQueueIsEmpty()); RawEvent event; @@ -1587,16 +1602,19 @@ TEST_F(InputReaderTest, DeviceReset_IncrementsSequenceNumber) { uint32_t prevSequenceNum = resetArgs.sequenceNum; disableDevice(deviceId, device); + mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); ASSERT_TRUE(prevSequenceNum < resetArgs.sequenceNum); prevSequenceNum = resetArgs.sequenceNum; enableDevice(deviceId, device); + mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); ASSERT_TRUE(prevSequenceNum < resetArgs.sequenceNum); prevSequenceNum = resetArgs.sequenceNum; disableDevice(deviceId, device); + mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); ASSERT_TRUE(prevSequenceNum < resetArgs.sequenceNum); prevSequenceNum = resetArgs.sequenceNum; @@ -1611,6 +1629,7 @@ TEST_F(InputReaderTest, Device_CanDispatchToDisplay) { FakeInputMapper* mapper = new FakeInputMapper(device, AINPUT_SOURCE_TOUCHSCREEN); device->addMapper(mapper); mReader->setNextDevice(device); + addDevice(deviceId, "fake", deviceClass, nullptr); const uint8_t hdmi1 = 1; @@ -1618,20 +1637,13 @@ TEST_F(InputReaderTest, Device_CanDispatchToDisplay) { mFakePolicy->addInputPortAssociation(DEVICE_LOCATION, hdmi1); // Add default and second display. - mFakePolicy->clearViewports(); mFakePolicy->addDisplayViewport(DISPLAY_ID, DISPLAY_WIDTH, DISPLAY_HEIGHT, DISPLAY_ORIENTATION_0, "local:0", NO_PORT, ViewportType::VIEWPORT_INTERNAL); mFakePolicy->addDisplayViewport(SECONDARY_DISPLAY_ID, DISPLAY_WIDTH, DISPLAY_HEIGHT, DISPLAY_ORIENTATION_0, "local:1", hdmi1, ViewportType::VIEWPORT_EXTERNAL); mReader->requestRefreshConfiguration(InputReaderConfiguration::CHANGE_DISPLAY_INFO); - - // Add the device, and make sure all of the callbacks are triggered. - // The device is added after the input port associations are processed since - // we do not yet support dynamic device-to-display associations. - ASSERT_NO_FATAL_FAILURE(addDevice(deviceId, "fake", deviceClass, nullptr)); + mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyConfigurationChangedWasCalled()); - ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled()); - ASSERT_NO_FATAL_FAILURE(mapper->assertConfigureWasCalled()); // Device should only dispatch to the specified display. ASSERT_EQ(deviceId, device->getId()); @@ -1640,8 +1652,6 @@ TEST_F(InputReaderTest, Device_CanDispatchToDisplay) { // Can't dispatch event from a disabled device. disableDevice(deviceId, device); - ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled()); - ASSERT_NO_FATAL_FAILURE(mapper->assertConfigureWasCalled()); ASSERT_FALSE(mReader->canDispatchToDisplay(deviceId, SECONDARY_DISPLAY_ID)); } @@ -1664,7 +1674,7 @@ protected: InputDevice* mDevice; - virtual void SetUp() override { + virtual void SetUp() { mFakeEventHub = std::make_unique(); mFakePolicy = new FakeInputReaderPolicy(); mFakeListener = new TestInputListener(); @@ -1678,7 +1688,7 @@ protected: DEVICE_CONTROLLER_NUMBER, identifier, DEVICE_CLASSES); } - virtual void TearDown() override { + virtual void TearDown() { delete mDevice; delete mFakeContext; @@ -1902,7 +1912,7 @@ protected: FakeInputReaderContext* mFakeContext; InputDevice* mDevice; - virtual void SetUp() override { + virtual void SetUp() { mFakeEventHub = std::make_unique(); mFakePolicy = new FakeInputReaderPolicy(); mFakeListener = new TestInputListener(); @@ -1916,7 +1926,7 @@ protected: mFakeEventHub->addDevice(mDevice->getId(), DEVICE_NAME, 0); } - virtual void TearDown() override { + virtual void TearDown() { delete mDevice; delete mFakeContext; mFakeListener.clear(); @@ -2579,7 +2589,7 @@ protected: sp mFakePointerController; - virtual void SetUp() override { + virtual void SetUp() { InputMapperTest::SetUp(); mFakePointerController = new FakePointerController(); -- cgit v1.2.3-59-g8ed1b From 28efc19d395bca5c747d71d392dfbeccf42b00be Mon Sep 17 00:00:00 2001 From: Prabir Pradhan Date: Tue, 5 Nov 2019 01:10:04 +0000 Subject: Reland "Let InputReader handle its own thread" This CL was first landed in Ic732436d4f00a831e317be1f16ac106a11652cff but was reverted due to flaky tests. The flaky tests were caused by races between the instrumented test classes and the InputReader class under test, which now runs in a new thread. In addition to re-landing the change, this CL ensures that InputReader does not start its own thread in its unit tests, but instead keep using the loopOnce method that is exposed for testing. Bug: 130819454 Test: atest inputflinger_tests Test: Touch input works on crosshatch Change-Id: I5bbfc68b6be6745efc8b7106a9292ee3cb431c9c --- services/inputflinger/InputManager.cpp | 9 ++-- services/inputflinger/InputManager.h | 11 ++--- services/inputflinger/InputReaderBase.cpp | 14 ------ services/inputflinger/include/InputReaderBase.h | 48 ++++++++++---------- services/inputflinger/reader/InputReader.cpp | 47 ++++++++++++++++++- services/inputflinger/reader/include/InputReader.h | 16 +++++-- services/inputflinger/tests/InputReader_test.cpp | 53 +++++++++++----------- 7 files changed, 118 insertions(+), 80 deletions(-) (limited to 'services/inputflinger/InputReaderBase.cpp') diff --git a/services/inputflinger/InputManager.cpp b/services/inputflinger/InputManager.cpp index e7640dd6af..1043390f84 100644 --- a/services/inputflinger/InputManager.cpp +++ b/services/inputflinger/InputManager.cpp @@ -46,7 +46,6 @@ InputManager::~InputManager() { } void InputManager::initialize() { - mReaderThread = new InputReaderThread(mReader); mDispatcherThread = new InputDispatcherThread(mDispatcher); } @@ -57,9 +56,9 @@ status_t InputManager::start() { return result; } - result = mReaderThread->run("InputReader", PRIORITY_URGENT_DISPLAY); + result = mReader->start(); if (result) { - ALOGE("Could not start InputReader thread due to error %d.", result); + ALOGE("Could not start InputReader due to error %d.", result); mDispatcherThread->requestExit(); return result; @@ -69,9 +68,9 @@ status_t InputManager::start() { } status_t InputManager::stop() { - status_t result = mReaderThread->requestExitAndWait(); + status_t result = mReader->stop(); if (result) { - ALOGW("Could not stop InputReader thread due to error %d.", result); + ALOGW("Could not stop InputReader due to error %d.", result); } result = mDispatcherThread->requestExitAndWait(); diff --git a/services/inputflinger/InputManager.h b/services/inputflinger/InputManager.h index 40f66d82f4..2a7ed0ff44 100644 --- a/services/inputflinger/InputManager.h +++ b/services/inputflinger/InputManager.h @@ -43,15 +43,15 @@ class InputDispatcherThread; /* * The input manager is the core of the system event processing. * - * The input manager uses two threads. + * The input manager has two components. * - * 1. The InputReaderThread (called "InputReader") reads and preprocesses raw input events, - * applies policy, and posts messages to a queue managed by the DispatcherThread. + * 1. The InputReader class starts a thread that reads and preprocesses raw input events, applies + * policy, and posts messages to a queue managed by the InputDispatcherThread. * 2. The InputDispatcherThread (called "InputDispatcher") thread waits for new events on the * queue and asynchronously dispatches them to applications. * - * By design, the InputReaderThread class and InputDispatcherThread class do not share any - * internal state. Moreover, all communication is done one way from the InputReaderThread + * By design, the InputReader class and InputDispatcherThread class do not share any + * internal state. Moreover, all communication is done one way from the InputReader * into the InputDispatcherThread and never the reverse. Both classes may interact with the * InputDispatchPolicy, however. * @@ -102,7 +102,6 @@ public: private: sp mReader; - sp mReaderThread; sp mClassifier; diff --git a/services/inputflinger/InputReaderBase.cpp b/services/inputflinger/InputReaderBase.cpp index 0422d8342b..2d6f2c1cc9 100644 --- a/services/inputflinger/InputReaderBase.cpp +++ b/services/inputflinger/InputReaderBase.cpp @@ -33,20 +33,6 @@ using android::base::StringPrintf; namespace android { -// --- InputReaderThread --- - -InputReaderThread::InputReaderThread(const sp& reader) : - Thread(/*canCallJava*/ true), mReader(reader) { -} - -InputReaderThread::~InputReaderThread() { -} - -bool InputReaderThread::threadLoop() { - mReader->loopOnce(); - return true; -} - // --- InputReaderConfiguration --- std::string InputReaderConfiguration::changesToString(uint32_t changes) { diff --git a/services/inputflinger/include/InputReaderBase.h b/services/inputflinger/include/InputReaderBase.h index 5d576b94f3..56c0a7356d 100644 --- a/services/inputflinger/include/InputReaderBase.h +++ b/services/inputflinger/include/InputReaderBase.h @@ -19,12 +19,12 @@ #include "PointerControllerInterface.h" +#include #include #include -#include #include #include -#include +#include #include #include @@ -44,7 +44,16 @@ namespace android { -/* Processes raw input events and sends cooked event data to an input listener. */ +// --- InputReaderInterface --- + +/* The interface for the InputReader shared library. + * + * Manages one or more threads that process raw input events and sends cooked event data to an + * input listener. + * + * The implementation must guarantee thread safety for this interface. However, since the input + * listener is NOT thread safe, all calls to the listener must happen from the same thread. + */ class InputReaderInterface : public virtual RefBase { protected: InputReaderInterface() { } @@ -56,18 +65,17 @@ public: * This method may be called on any thread (usually by the input manager). */ virtual void dump(std::string& dump) = 0; - /* Called by the heatbeat to ensures that the reader has not deadlocked. */ + /* Called by the heartbeat to ensures that the reader has not deadlocked. */ virtual void monitor() = 0; /* Returns true if the input device is enabled. */ virtual bool isInputDeviceEnabled(int32_t deviceId) = 0; - /* Runs a single iteration of the processing loop. - * Nominally reads and processes one incoming message from the EventHub. - * - * This method should be called on the input reader thread. - */ - virtual void loopOnce() = 0; + /* Makes the reader start processing events from the kernel. */ + virtual status_t start() = 0; + + /* Makes the reader stop processing any more events. */ + virtual status_t stop() = 0; /* Gets information about all input devices. * @@ -104,17 +112,7 @@ public: virtual bool canDispatchToDisplay(int32_t deviceId, int32_t displayId) = 0; }; -/* Reads raw events from the event hub and processes them, endlessly. */ -class InputReaderThread : public Thread { -public: - explicit InputReaderThread(const sp& reader); - virtual ~InputReaderThread(); - -private: - sp mReader; - - virtual bool threadLoop(); -}; +// --- InputReaderConfiguration --- /* * Input reader configuration. @@ -285,6 +283,8 @@ private: std::vector mDisplays; }; +// --- TouchAffineTransformation --- + struct TouchAffineTransformation { float x_scale; float x_ymix; @@ -307,6 +307,8 @@ struct TouchAffineTransformation { void applyTo(float& x, float& y) const; }; +// --- InputReaderPolicyInterface --- + /* * Input reader policy interface. * @@ -316,8 +318,8 @@ struct TouchAffineTransformation { * The actual implementation is partially supported by callbacks into the DVM * via JNI. This interface is also mocked in the unit tests. * - * These methods must NOT re-enter the input reader since they may be called while - * holding the input reader lock. + * These methods will NOT re-enter the input reader interface, so they may be called from + * any method in the input reader interface. */ class InputReaderPolicyInterface : public virtual RefBase { protected: diff --git a/services/inputflinger/reader/InputReader.cpp b/services/inputflinger/reader/InputReader.cpp index 1c5adc3a85..05f0db1329 100644 --- a/services/inputflinger/reader/InputReader.cpp +++ b/services/inputflinger/reader/InputReader.cpp @@ -38,16 +38,38 @@ #include #include +#include #include #include #include - +#include using android::base::StringPrintf; namespace android { +// --- InputReader::InputReaderThread --- + +/* Thread that reads raw events from the event hub and processes them, endlessly. */ +class InputReader::InputReaderThread : public Thread { +public: + explicit InputReaderThread(InputReader* reader) + : Thread(/* canCallJava */ true), mReader(reader) {} + + ~InputReaderThread() {} + +private: + InputReader* mReader; + + bool threadLoop() override { + mReader->loopOnce(); + return true; + } +}; + +// --- InputReader --- + InputReader::InputReader(std::shared_ptr eventHub, const sp& policy, const sp& listener) @@ -61,6 +83,7 @@ InputReader::InputReader(std::shared_ptr eventHub, mNextTimeout(LLONG_MAX), mConfigurationChangesToRefresh(0) { mQueuedListener = new QueuedInputListener(listener); + mThread = new InputReaderThread(this); { // acquire lock AutoMutex _l(mLock); @@ -76,6 +99,28 @@ InputReader::~InputReader() { } } +status_t InputReader::start() { + if (mThread->isRunning()) { + return ALREADY_EXISTS; + } + return mThread->run("InputReader", PRIORITY_URGENT_DISPLAY); +} + +status_t InputReader::stop() { + if (!mThread->isRunning()) { + return OK; + } + if (gettid() == mThread->getTid()) { + ALOGE("InputReader can only be stopped from outside of the InputReaderThread!"); + return INVALID_OPERATION; + } + // Directly calling requestExitAndWait() causes the thread to not exit + // if mEventHub is waiting for a long timeout. + mThread->requestExit(); + mEventHub->wake(); + return mThread->requestExitAndWait(); +} + void InputReader::loopOnce() { int32_t oldGeneration; int32_t timeoutMillis; diff --git a/services/inputflinger/reader/include/InputReader.h b/services/inputflinger/reader/include/InputReader.h index 557eb3b7c4..502490601e 100644 --- a/services/inputflinger/reader/include/InputReader.h +++ b/services/inputflinger/reader/include/InputReader.h @@ -38,12 +38,12 @@ struct StylusState; * that it sends to the input listener. Some functions of the input reader, such as early * event filtering in low power states, are controlled by a separate policy object. * - * The InputReader owns a collection of InputMappers. Most of the work it does happens - * on the input reader thread but the InputReader can receive queries from other system + * The InputReader owns a collection of InputMappers. InputReader starts its own thread, where + * most of the work happens, but the InputReader can receive queries from other system * components running on arbitrary threads. To keep things manageable, the InputReader * uses a single Mutex to guard its state. The Mutex may be held while calling into the * EventHub or the InputReaderPolicy but it is never held while calling into the - * InputListener. + * InputListener. All calls to InputListener must happen from InputReader's thread. */ class InputReader : public InputReaderInterface { public: @@ -55,7 +55,8 @@ public: virtual void dump(std::string& dump) override; virtual void monitor() override; - virtual void loopOnce() override; + virtual status_t start() override; + virtual status_t stop() override; virtual void getInputDevices(std::vector& outInputDevices) override; @@ -86,6 +87,10 @@ protected: const InputDeviceIdentifier& identifier, uint32_t classes); + // With each iteration of the loop, InputReader reads and processes one incoming message from + // the EventHub. + void loopOnce(); + class ContextImpl : public InputReaderContext { InputReader* mReader; @@ -111,6 +116,9 @@ protected: friend class ContextImpl; private: + class InputReaderThread; + sp mThread; + Mutex mLock; Condition mReaderIsAliveCondition; diff --git a/services/inputflinger/tests/InputReader_test.cpp b/services/inputflinger/tests/InputReader_test.cpp index c1c912214a..d6624c9f5a 100644 --- a/services/inputflinger/tests/InputReader_test.cpp +++ b/services/inputflinger/tests/InputReader_test.cpp @@ -1113,6 +1113,9 @@ public: classes); } + // Make the protected loopOnce method accessible to tests. + using InputReader::loopOnce; + protected: virtual InputDevice* createDeviceLocked(int32_t deviceId, int32_t controllerNumber, const InputDeviceIdentifier& identifier, @@ -1133,12 +1136,8 @@ class InputReaderPolicyTest : public testing::Test { protected: sp mFakePolicy; - virtual void SetUp() { - mFakePolicy = new FakeInputReaderPolicy(); - } - virtual void TearDown() { - mFakePolicy.clear(); - } + virtual void SetUp() override { mFakePolicy = new FakeInputReaderPolicy(); } + virtual void TearDown() override { mFakePolicy.clear(); } }; /** @@ -1321,19 +1320,18 @@ protected: sp mFakeListener; sp mFakePolicy; std::shared_ptr mFakeEventHub; - sp mReader; + std::unique_ptr mReader; - virtual void SetUp() { + virtual void SetUp() override { mFakeEventHub = std::make_unique(); mFakePolicy = new FakeInputReaderPolicy(); mFakeListener = new TestInputListener(); - mReader = new InstrumentedInputReader(mFakeEventHub, mFakePolicy, mFakeListener); + mReader = std::make_unique(mFakeEventHub, mFakePolicy, + mFakeListener); } - virtual void TearDown() { - mReader.clear(); - + virtual void TearDown() override { mFakeListener.clear(); mFakePolicy.clear(); } @@ -1354,16 +1352,12 @@ protected: void disableDevice(int32_t deviceId, InputDevice* device) { mFakePolicy->addDisabledDevice(deviceId); - configureDevice(InputReaderConfiguration::CHANGE_ENABLED_STATE, device); + mReader->requestRefreshConfiguration(InputReaderConfiguration::CHANGE_ENABLED_STATE); } void enableDevice(int32_t deviceId, InputDevice* device) { mFakePolicy->removeDisabledDevice(deviceId); - configureDevice(InputReaderConfiguration::CHANGE_ENABLED_STATE, device); - } - - void configureDevice(uint32_t changes, InputDevice* device) { - device->configure(ARBITRARY_TIME, mFakePolicy->getReaderConfiguration(), changes); + mReader->requestRefreshConfiguration(InputReaderConfiguration::CHANGE_ENABLED_STATE); } FakeInputMapper* addDeviceWithFakeInputMapper(int32_t deviceId, int32_t controllerNumber, @@ -1417,7 +1411,6 @@ TEST_F(InputReaderTest, WhenEnabledChanges_SendsDeviceResetNotification) { NotifyDeviceResetArgs resetArgs; ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); - ASSERT_EQ(ARBITRARY_TIME, resetArgs.eventTime); ASSERT_EQ(deviceId, resetArgs.deviceId); ASSERT_EQ(device->isEnabled(), true); @@ -1425,7 +1418,6 @@ TEST_F(InputReaderTest, WhenEnabledChanges_SendsDeviceResetNotification) { mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); - ASSERT_EQ(ARBITRARY_TIME, resetArgs.eventTime); ASSERT_EQ(deviceId, resetArgs.deviceId); ASSERT_EQ(device->isEnabled(), false); @@ -1438,7 +1430,6 @@ TEST_F(InputReaderTest, WhenEnabledChanges_SendsDeviceResetNotification) { enableDevice(deviceId, device); mReader->loopOnce(); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled(&resetArgs)); - ASSERT_EQ(ARBITRARY_TIME, resetArgs.eventTime); ASSERT_EQ(deviceId, resetArgs.deviceId); ASSERT_EQ(device->isEnabled(), true); } @@ -1629,7 +1620,6 @@ TEST_F(InputReaderTest, Device_CanDispatchToDisplay) { FakeInputMapper* mapper = new FakeInputMapper(device, AINPUT_SOURCE_TOUCHSCREEN); device->addMapper(mapper); mReader->setNextDevice(device); - addDevice(deviceId, "fake", deviceClass, nullptr); const uint8_t hdmi1 = 1; @@ -1637,13 +1627,21 @@ TEST_F(InputReaderTest, Device_CanDispatchToDisplay) { mFakePolicy->addInputPortAssociation(DEVICE_LOCATION, hdmi1); // Add default and second display. + mFakePolicy->clearViewports(); mFakePolicy->addDisplayViewport(DISPLAY_ID, DISPLAY_WIDTH, DISPLAY_HEIGHT, DISPLAY_ORIENTATION_0, "local:0", NO_PORT, ViewportType::VIEWPORT_INTERNAL); mFakePolicy->addDisplayViewport(SECONDARY_DISPLAY_ID, DISPLAY_WIDTH, DISPLAY_HEIGHT, DISPLAY_ORIENTATION_0, "local:1", hdmi1, ViewportType::VIEWPORT_EXTERNAL); mReader->requestRefreshConfiguration(InputReaderConfiguration::CHANGE_DISPLAY_INFO); mReader->loopOnce(); + + // Add the device, and make sure all of the callbacks are triggered. + // The device is added after the input port associations are processed since + // we do not yet support dynamic device-to-display associations. + ASSERT_NO_FATAL_FAILURE(addDevice(deviceId, "fake", deviceClass, nullptr)); ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyConfigurationChangedWasCalled()); + ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyDeviceResetWasCalled()); + ASSERT_NO_FATAL_FAILURE(mapper->assertConfigureWasCalled()); // Device should only dispatch to the specified display. ASSERT_EQ(deviceId, device->getId()); @@ -1652,6 +1650,7 @@ TEST_F(InputReaderTest, Device_CanDispatchToDisplay) { // Can't dispatch event from a disabled device. disableDevice(deviceId, device); + mReader->loopOnce(); ASSERT_FALSE(mReader->canDispatchToDisplay(deviceId, SECONDARY_DISPLAY_ID)); } @@ -1674,7 +1673,7 @@ protected: InputDevice* mDevice; - virtual void SetUp() { + virtual void SetUp() override { mFakeEventHub = std::make_unique(); mFakePolicy = new FakeInputReaderPolicy(); mFakeListener = new TestInputListener(); @@ -1688,7 +1687,7 @@ protected: DEVICE_CONTROLLER_NUMBER, identifier, DEVICE_CLASSES); } - virtual void TearDown() { + virtual void TearDown() override { delete mDevice; delete mFakeContext; @@ -1912,7 +1911,7 @@ protected: FakeInputReaderContext* mFakeContext; InputDevice* mDevice; - virtual void SetUp() { + virtual void SetUp() override { mFakeEventHub = std::make_unique(); mFakePolicy = new FakeInputReaderPolicy(); mFakeListener = new TestInputListener(); @@ -1926,7 +1925,7 @@ protected: mFakeEventHub->addDevice(mDevice->getId(), DEVICE_NAME, 0); } - virtual void TearDown() { + virtual void TearDown() override { delete mDevice; delete mFakeContext; mFakeListener.clear(); @@ -2589,7 +2588,7 @@ protected: sp mFakePointerController; - virtual void SetUp() { + virtual void SetUp() override { InputMapperTest::SetUp(); mFakePointerController = new FakePointerController(); -- cgit v1.2.3-59-g8ed1b From 888a6a41edc5e4250076b7e36e4d626eb70edc71 Mon Sep 17 00:00:00 2001 From: Garfield Tan Date: Thu, 9 Jan 2020 11:39:16 -0800 Subject: Let InputReader set mouse cursor's display. InputReader was already responsible to bind device/event and display. It makes sense to further extend it to control the display where mouse cursor is shown. Bug: 146385350 Test: Mouse cursor shows up at expected display. Test: atest inputflinger_tests Change-Id: Ie7a9546550b70c8834462b06de929472196fe713 --- services/inputflinger/InputReaderBase.cpp | 12 ++++++++- services/inputflinger/include/InputReaderBase.h | 6 ++++- .../include/PointerControllerInterface.h | 4 +++ .../reader/mapper/CursorInputMapper.cpp | 22 +++++++++++++++- .../inputflinger/reader/mapper/CursorInputMapper.h | 3 ++- .../reader/mapper/TouchInputMapper.cpp | 17 ++++++++++-- services/inputflinger/tests/InputReader_test.cpp | 30 +++++++++++++++------- 7 files changed, 79 insertions(+), 15 deletions(-) (limited to 'services/inputflinger/InputReaderBase.cpp') diff --git a/services/inputflinger/InputReaderBase.cpp b/services/inputflinger/InputReaderBase.cpp index 2d6f2c1cc9..b2dadf8460 100644 --- a/services/inputflinger/InputReaderBase.cpp +++ b/services/inputflinger/InputReaderBase.cpp @@ -125,6 +125,16 @@ std::optional InputReaderConfiguration::getDisplayViewportByPor return std::nullopt; } +std::optional InputReaderConfiguration::getDisplayViewportById( + int32_t displayId) const { + for (const DisplayViewport& currentViewport : mDisplays) { + if (currentViewport.displayId == displayId) { + return std::make_optional(currentViewport); + } + } + return std::nullopt; +} + void InputReaderConfiguration::setDisplayViewports(const std::vector& viewports) { mDisplays = viewports; } @@ -151,4 +161,4 @@ void TouchAffineTransformation::applyTo(float& x, float& y) const { y = newY; } -} // namespace android \ No newline at end of file +} // namespace android diff --git a/services/inputflinger/include/InputReaderBase.h b/services/inputflinger/include/InputReaderBase.h index 56c0a7356d..f8d535152e 100644 --- a/services/inputflinger/include/InputReaderBase.h +++ b/services/inputflinger/include/InputReaderBase.h @@ -169,6 +169,9 @@ struct InputReaderConfiguration { // Used to determine which DisplayViewport should be tied to which InputDevice. std::unordered_map portAssociations; + // The suggested display ID to show the cursor. + int32_t defaultPointerDisplayId; + // Velocity control parameters for mouse pointer movements. VelocityControlParameters pointerVelocityControlParameters; @@ -273,6 +276,7 @@ struct InputReaderConfiguration { std::optional getDisplayViewportByUniqueId(const std::string& uniqueDisplayId) const; std::optional getDisplayViewportByPort(uint8_t physicalPort) const; + std::optional getDisplayViewportById(int32_t displayId) const; void setDisplayViewports(const std::vector& viewports); @@ -352,4 +356,4 @@ public: } // namespace android -#endif // _UI_INPUT_READER_COMMON_H \ No newline at end of file +#endif // _UI_INPUT_READER_COMMON_H diff --git a/services/inputflinger/include/PointerControllerInterface.h b/services/inputflinger/include/PointerControllerInterface.h index 0ff28e4174..194c66547f 100644 --- a/services/inputflinger/include/PointerControllerInterface.h +++ b/services/inputflinger/include/PointerControllerInterface.h @@ -17,6 +17,7 @@ #ifndef _INPUTFLINGER_POINTER_CONTROLLER_INTERFACE_H #define _INPUTFLINGER_POINTER_CONTROLLER_INTERFACE_H +#include #include #include #include @@ -101,6 +102,9 @@ public: /* Gets the id of the display where the pointer should be shown. */ virtual int32_t getDisplayId() const = 0; + + /* Sets the associated display of this pointer. Pointer should show on that display. */ + virtual void setDisplayViewport(const DisplayViewport& displayViewport) = 0; }; } // namespace android diff --git a/services/inputflinger/reader/mapper/CursorInputMapper.cpp b/services/inputflinger/reader/mapper/CursorInputMapper.cpp index f69138ea09..69a75bae2f 100644 --- a/services/inputflinger/reader/mapper/CursorInputMapper.cpp +++ b/services/inputflinger/reader/mapper/CursorInputMapper.cpp @@ -189,12 +189,32 @@ void CursorInputMapper::configure(nsecs_t when, const InputReaderConfiguration* // Update the PointerController if viewports changed. if (mParameters.mode == Parameters::MODE_POINTER) { - getPolicy()->obtainPointerController(getDeviceId()); + updatePointerControllerDisplayViewport(*config); } bumpGeneration(); } } +void CursorInputMapper::updatePointerControllerDisplayViewport( + const InputReaderConfiguration& config) { + std::optional viewport = + config.getDisplayViewportById(config.defaultPointerDisplayId); + if (!viewport) { + ALOGW("Can't find the designated viewport with ID %" PRId32 " to update cursor input " + "mapper. Fall back to default display", + config.defaultPointerDisplayId); + viewport = config.getDisplayViewportById(ADISPLAY_ID_DEFAULT); + } + + if (!viewport) { + ALOGE("Still can't find a viable viewport to update cursor input mapper. Skip setting it to" + " PointerController."); + return; + } + + mPointerController->setDisplayViewport(*viewport); +} + void CursorInputMapper::configureParameters() { mParameters.mode = Parameters::MODE_POINTER; String8 cursorModeString; diff --git a/services/inputflinger/reader/mapper/CursorInputMapper.h b/services/inputflinger/reader/mapper/CursorInputMapper.h index 77d122af0a..d56f9be045 100644 --- a/services/inputflinger/reader/mapper/CursorInputMapper.h +++ b/services/inputflinger/reader/mapper/CursorInputMapper.h @@ -117,8 +117,9 @@ private: void dumpParameters(std::string& dump); void sync(nsecs_t when); + void updatePointerControllerDisplayViewport(const InputReaderConfiguration& config); }; } // namespace android -#endif // _UI_INPUTREADER_CURSOR_INPUT_MAPPER_H \ No newline at end of file +#endif // _UI_INPUTREADER_CURSOR_INPUT_MAPPER_H diff --git a/services/inputflinger/reader/mapper/TouchInputMapper.cpp b/services/inputflinger/reader/mapper/TouchInputMapper.cpp index c80a2dc4d9..b66caca787 100644 --- a/services/inputflinger/reader/mapper/TouchInputMapper.cpp +++ b/services/inputflinger/reader/mapper/TouchInputMapper.cpp @@ -552,9 +552,10 @@ bool TouchInputMapper::hasExternalStylus() const { * Determine which DisplayViewport to use. * 1. If display port is specified, return the matching viewport. If matching viewport not * found, then return. - * 2. If a device has associated display, get the matching viewport by either unique id or by + * 2. Always use the suggested viewport from WindowManagerService for pointers. + * 3. If a device has associated display, get the matching viewport by either unique id or by * the display type (internal or external). - * 3. Otherwise, use a non-display viewport. + * 4. Otherwise, use a non-display viewport. */ std::optional TouchInputMapper::findViewport() { if (mParameters.hasAssociatedDisplay) { @@ -564,6 +565,17 @@ std::optional TouchInputMapper::findViewport() { return mDevice->getAssociatedViewport(); } + if (mDeviceMode == DEVICE_MODE_POINTER) { + std::optional viewport = + mConfig.getDisplayViewportById(mConfig.defaultPointerDisplayId); + if (viewport) { + return viewport; + } else { + ALOGW("Can't find designated display viewport with ID %" PRId32 " for pointers.", + mConfig.defaultPointerDisplayId); + } + } + // Check if uniqueDisplayId is specified in idc file. if (!mParameters.uniqueDisplayId.empty()) { return mConfig.getDisplayViewportByUniqueId(mParameters.uniqueDisplayId); @@ -749,6 +761,7 @@ void TouchInputMapper::configureSurface(nsecs_t when, bool* outResetNeeded) { (mDeviceMode == DEVICE_MODE_DIRECT && mConfig.showTouches)) { if (mPointerController == nullptr || viewportChanged) { mPointerController = getPolicy()->obtainPointerController(getDeviceId()); + mPointerController->setDisplayViewport(mViewport); } } else { mPointerController.clear(); diff --git a/services/inputflinger/tests/InputReader_test.cpp b/services/inputflinger/tests/InputReader_test.cpp index a9d7793a17..8ca7e4a893 100644 --- a/services/inputflinger/tests/InputReader_test.cpp +++ b/services/inputflinger/tests/InputReader_test.cpp @@ -90,10 +90,6 @@ public: mMaxY = maxY; } - void setDisplayId(int32_t displayId) { - mDisplayId = displayId; - } - virtual void setPosition(float x, float y) { mX = x; mY = y; @@ -116,6 +112,10 @@ public: return mDisplayId; } + virtual void setDisplayViewport(const DisplayViewport& viewport) { + mDisplayId = viewport.displayId; + } + const std::map>& getSpots() { return mSpotsByDisplay; } @@ -280,6 +280,10 @@ public: mConfig.showTouches = enabled; } + void setDefaultPointerDisplayId(int32_t pointerDisplayId) { + mConfig.defaultPointerDisplayId = pointerDisplayId; + } + private: DisplayViewport createDisplayViewport(int32_t displayId, int32_t width, int32_t height, int32_t orientation, const std::string& uniqueId, std::optional physicalPort, @@ -3432,12 +3436,18 @@ TEST_F(CursorInputMapperTest, Process_ShouldHandleDisplayId) { CursorInputMapper* mapper = new CursorInputMapper(mDevice); addMapperAndConfigure(mapper); - // Setup PointerController for second display. + // Setup for second display. constexpr int32_t SECOND_DISPLAY_ID = 1; + const std::string SECOND_DISPLAY_UNIQUE_ID = "local:1"; + mFakePolicy->addDisplayViewport(SECOND_DISPLAY_ID, 800, 480, DISPLAY_ORIENTATION_0, + SECOND_DISPLAY_UNIQUE_ID, NO_PORT, + ViewportType::VIEWPORT_EXTERNAL); + mFakePolicy->setDefaultPointerDisplayId(SECOND_DISPLAY_ID); + configureDevice(InputReaderConfiguration::CHANGE_DISPLAY_INFO); + mFakePointerController->setBounds(0, 0, 800 - 1, 480 - 1); mFakePointerController->setPosition(100, 200); mFakePointerController->setButtonState(0); - mFakePointerController->setDisplayId(SECOND_DISPLAY_ID); NotifyMotionArgs args; process(mapper, ARBITRARY_TIME, EV_REL, REL_X, 10); @@ -6539,14 +6549,16 @@ TEST_F(MultiTouchInputMapperTest, Viewports_Fallback) { } TEST_F(MultiTouchInputMapperTest, Process_Pointer_ShouldHandleDisplayId) { - // Setup PointerController for second display. + // Setup for second display. sp fakePointerController = new FakePointerController(); - fakePointerController->setBounds(0, 0, 800 - 1, 480 - 1); + fakePointerController->setBounds(0, 0, DISPLAY_WIDTH - 1, DISPLAY_HEIGHT - 1); fakePointerController->setPosition(100, 200); fakePointerController->setButtonState(0); - fakePointerController->setDisplayId(SECONDARY_DISPLAY_ID); mFakePolicy->setPointerController(mDevice->getId(), fakePointerController); + mFakePolicy->setDefaultPointerDisplayId(SECONDARY_DISPLAY_ID); + prepareSecondaryDisplay(ViewportType::VIEWPORT_EXTERNAL); + MultiTouchInputMapper* mapper = new MultiTouchInputMapper(mDevice); prepareDisplay(DISPLAY_ORIENTATION_0); prepareAxes(POSITION); -- cgit v1.2.3-59-g8ed1b