From 0cde5911bccf265932739f2428e0c8aa4d3359c1 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Fri, 26 Jun 2020 20:25:34 +0100 Subject: [PATCH] Move PointerController from sp to shared_ptr Bug: 160010896 Test: atest PointerController_test, manual usage Change-Id: I4e665d00c56b44c9c1a4ea8cb27ffd10ade3315b --- libs/input/PointerController.cpp | 95 +++++++++++-------- libs/input/PointerController.h | 66 ++++++++----- libs/input/tests/PointerController_test.cpp | 4 +- ...droid_server_input_InputManagerService.cpp | 19 ++-- 4 files changed, 106 insertions(+), 78 deletions(-) diff --git a/libs/input/PointerController.cpp b/libs/input/PointerController.cpp index 3b494e9129dba..f74989488491e 100644 --- a/libs/input/PointerController.cpp +++ b/libs/input/PointerController.cpp @@ -24,31 +24,10 @@ #include +#include + namespace android { -// --- WeakLooperCallback --- - -class WeakLooperCallback: public LooperCallback { -protected: - virtual ~WeakLooperCallback() { } - -public: - explicit WeakLooperCallback(const wp& callback) : - mCallback(callback) { - } - - virtual int handleEvent(int fd, int events, void* data) { - sp callback = mCallback.promote(); - if (callback != NULL) { - return callback->handleEvent(fd, events, data); - } - return 0; // the client is gone, remove the callback - } - -private: - wp mCallback; -}; - // --- PointerController --- // Time to wait before starting the fade when the pointer is inactive. @@ -64,21 +43,42 @@ static const nsecs_t POINTER_FADE_DURATION = 500 * 1000000LL; // 500 ms // The number of events to be read at once for DisplayEventReceiver. static const int EVENT_BUFFER_SIZE = 100; -// --- PointerController --- +std::shared_ptr PointerController::create( + const sp& policy, const sp& looper, + const sp& spriteController) { + std::shared_ptr controller = std::shared_ptr( + new PointerController(policy, looper, spriteController)); -PointerController::PointerController(const sp& policy, - const sp& looper, const sp& spriteController) : - mPolicy(policy), mLooper(looper), mSpriteController(spriteController) { - mHandler = new WeakMessageHandler(this); - mCallback = new WeakLooperCallback(this); + /* + * Now we need to hook up the constructed PointerController object to its callbacks. + * + * This must be executed after the constructor but before any other methods on PointerController + * in order to ensure that the fully constructed object is visible on the Looper thread, since + * that may be a different thread than where the PointerController is initially constructed. + * + * Unfortunately, this cannot be done as part of the constructor since we need to hand out + * weak_ptr's which themselves cannot be constructed until there's at least one shared_ptr. + */ - if (mDisplayEventReceiver.initCheck() == NO_ERROR) { - mLooper->addFd(mDisplayEventReceiver.getFd(), Looper::POLL_CALLBACK, - Looper::EVENT_INPUT, mCallback, nullptr); + controller->mHandler->pointerController = controller; + controller->mCallback->pointerController = controller; + if (controller->mDisplayEventReceiver.initCheck() == NO_ERROR) { + controller->mLooper->addFd(controller->mDisplayEventReceiver.getFd(), Looper::POLL_CALLBACK, + Looper::EVENT_INPUT, controller->mCallback, nullptr); } else { ALOGE("Failed to initialize DisplayEventReceiver."); } + return controller; +} +PointerController::PointerController(const sp& policy, + const sp& looper, + const sp& spriteController) + : mPolicy(policy), + mLooper(looper), + mSpriteController(spriteController), + mHandler(new MessageHandler()), + mCallback(new LooperCallback()) { AutoMutex _l(mLock); mLocked.animationPending = false; @@ -480,24 +480,35 @@ void PointerController::setCustomPointerIcon(const SpriteIcon& icon) { updatePointerLocked(); } -void PointerController::handleMessage(const Message& message) { +void PointerController::MessageHandler::handleMessage(const Message& message) { + std::shared_ptr controller = pointerController.lock(); + + if (controller == nullptr) { + ALOGE("PointerController instance was released before processing message: what=%d", + message.what); + return; + } switch (message.what) { case MSG_INACTIVITY_TIMEOUT: - doInactivityTimeout(); + controller->doInactivityTimeout(); break; } } -int PointerController::handleEvent(int /* fd */, int events, void* /* data */) { +int PointerController::LooperCallback::handleEvent(int /* fd */, int events, void* /* data */) { + std::shared_ptr controller = pointerController.lock(); + if (controller == nullptr) { + ALOGW("PointerController instance was released with pending callbacks. events=0x%x", + events); + return 0; // Remove the callback, the PointerController is gone anyways + } if (events & (Looper::EVENT_ERROR | Looper::EVENT_HANGUP)) { - ALOGE("Display event receiver pipe was closed or an error occurred. " - "events=0x%x", events); + ALOGE("Display event receiver pipe was closed or an error occurred. events=0x%x", events); return 0; // remove the callback } if (!(events & Looper::EVENT_INPUT)) { - ALOGW("Received spurious callback for unhandled poll event. " - "events=0x%x", events); + ALOGW("Received spurious callback for unhandled poll event. events=0x%x", events); return 1; // keep the callback } @@ -505,7 +516,7 @@ int PointerController::handleEvent(int /* fd */, int events, void* /* data */) { ssize_t n; nsecs_t timestamp; DisplayEventReceiver::Event buf[EVENT_BUFFER_SIZE]; - while ((n = mDisplayEventReceiver.getEvents(buf, EVENT_BUFFER_SIZE)) > 0) { + while ((n = controller->mDisplayEventReceiver.getEvents(buf, EVENT_BUFFER_SIZE)) > 0) { for (size_t i = 0; i < static_cast(n); ++i) { if (buf[i].header.type == DisplayEventReceiver::DISPLAY_EVENT_VSYNC) { timestamp = buf[i].header.timestamp; @@ -514,7 +525,7 @@ int PointerController::handleEvent(int /* fd */, int events, void* /* data */) { } } if (gotVsync) { - doAnimate(timestamp); + controller->doAnimate(timestamp); } return 1; // keep the callback } @@ -731,7 +742,7 @@ PointerController::Spot* PointerController::removeFirstFadingSpotLocked(std::vec return spot; } } - return NULL; + return nullptr; } void PointerController::releaseSpotLocked(Spot* spot) { diff --git a/libs/input/PointerController.h b/libs/input/PointerController.h index ebc622bae3026..ff5631b4d2074 100644 --- a/libs/input/PointerController.h +++ b/libs/input/PointerController.h @@ -17,19 +17,20 @@ #ifndef _UI_POINTER_CONTROLLER_H #define _UI_POINTER_CONTROLLER_H -#include "SpriteController.h" - -#include -#include - -#include +#include +#include #include #include -#include +#include #include -#include #include -#include +#include + +#include +#include +#include + +#include "SpriteController.h" namespace android { @@ -70,25 +71,22 @@ public: virtual int32_t getCustomPointerIconId() = 0; }; - /* * Tracks pointer movements and draws the pointer sprite to a surface. * * Handles pointer acceleration and animation. */ -class PointerController : public PointerControllerInterface, public MessageHandler, - public LooperCallback { -protected: - virtual ~PointerController(); - +class PointerController : public PointerControllerInterface { public: + static std::shared_ptr create( + const sp& policy, const sp& looper, + const sp& spriteController); enum InactivityTimeout { INACTIVITY_TIMEOUT_NORMAL = 0, INACTIVITY_TIMEOUT_SHORT = 1, }; - PointerController(const sp& policy, - const sp& looper, const sp& spriteController); + virtual ~PointerController(); virtual bool getBounds(float* outMinX, float* outMinY, float* outMaxX, float* outMaxY) const; @@ -113,8 +111,8 @@ public: void reloadPointerResources(); private: - static const size_t MAX_RECYCLED_SPRITES = 12; - static const size_t MAX_SPOTS = 12; + static constexpr size_t MAX_RECYCLED_SPRITES = 12; + static constexpr size_t MAX_SPOTS = 12; enum { MSG_INACTIVITY_TIMEOUT, @@ -130,8 +128,13 @@ private: float x, y; inline Spot(uint32_t id, const sp& sprite) - : id(id), sprite(sprite), alpha(1.0f), scale(1.0f), - x(0.0f), y(0.0f), lastIcon(NULL) { } + : id(id), + sprite(sprite), + alpha(1.0f), + scale(1.0f), + x(0.0f), + y(0.0f), + lastIcon(nullptr) {} void updateSprite(const SpriteIcon* icon, float x, float y, int32_t displayId); @@ -139,12 +142,24 @@ private: const SpriteIcon* lastIcon; }; + class MessageHandler : public virtual android::MessageHandler { + public: + void handleMessage(const Message& message) override; + std::weak_ptr pointerController; + }; + + class LooperCallback : public virtual android::LooperCallback { + public: + int handleEvent(int fd, int events, void* data) override; + std::weak_ptr pointerController; + }; + mutable Mutex mLock; sp mPolicy; sp mLooper; sp mSpriteController; - sp mHandler; + sp mHandler; sp mCallback; DisplayEventReceiver mDisplayEventReceiver; @@ -181,14 +196,15 @@ private: int32_t buttonState; std::map> spotsByDisplay; - std::vector > recycledSprites; + std::vector> recycledSprites; } mLocked GUARDED_BY(mLock); + PointerController(const sp& policy, const sp& looper, + const sp& spriteController); + bool getBoundsLocked(float* outMinX, float* outMinY, float* outMaxX, float* outMaxY) const; void setPositionLocked(float x, float y); - void handleMessage(const Message& message); - int handleEvent(int fd, int events, void* data); void doAnimate(nsecs_t timestamp); bool doFadingAnimationLocked(nsecs_t timestamp); bool doBitmapAnimationLocked(nsecs_t timestamp); diff --git a/libs/input/tests/PointerController_test.cpp b/libs/input/tests/PointerController_test.cpp index a15742671dc79..1704436c6f3a7 100644 --- a/libs/input/tests/PointerController_test.cpp +++ b/libs/input/tests/PointerController_test.cpp @@ -136,7 +136,7 @@ protected: sp mPointerSprite; sp mPolicy; sp mSpriteController; - sp mPointerController; + std::shared_ptr mPointerController; private: void loopThread(); @@ -160,7 +160,7 @@ PointerControllerTest::PointerControllerTest() : mPointerSprite(new NiceMock obtainPointerController(int32_t deviceId); + virtual std::shared_ptr obtainPointerController(int32_t deviceId); virtual void notifyInputDevicesChanged(const std::vector& inputDevices); virtual sp getKeyboardLayoutOverlay(const InputDeviceIdentifier& identifier); virtual std::string getDeviceAlias(const InputDeviceIdentifier& identifier); @@ -299,7 +299,7 @@ private: sp spriteController; // Pointer controller singleton, created and destroyed as needed. - wp pointerController; + std::weak_ptr pointerController; // Input devices to be disabled std::set disabledInputDevices; @@ -544,15 +544,16 @@ void NativeInputManager::getReaderConfiguration(InputReaderConfiguration* outCon } // release lock } -sp NativeInputManager::obtainPointerController(int32_t /* deviceId */) { +std::shared_ptr NativeInputManager::obtainPointerController( + int32_t /* deviceId */) { ATRACE_CALL(); AutoMutex _l(mLock); - sp controller = mLocked.pointerController.promote(); + std::shared_ptr controller = mLocked.pointerController.lock(); if (controller == nullptr) { ensureSpriteControllerLocked(); - controller = new PointerController(this, mLooper, mLocked.spriteController); + controller = PointerController::create(this, mLooper, mLocked.spriteController); mLocked.pointerController = controller; updateInactivityTimeoutLocked(); } @@ -803,7 +804,7 @@ void NativeInputManager::setSystemUiVisibility(int32_t visibility) { } void NativeInputManager::updateInactivityTimeoutLocked() REQUIRES(mLock) { - sp controller = mLocked.pointerController.promote(); + std::shared_ptr controller = mLocked.pointerController.lock(); if (controller == nullptr) { return; } @@ -891,7 +892,7 @@ void NativeInputManager::reloadCalibration() { void NativeInputManager::setPointerIconType(int32_t iconId) { AutoMutex _l(mLock); - sp controller = mLocked.pointerController.promote(); + std::shared_ptr controller = mLocked.pointerController.lock(); if (controller != nullptr) { controller->updatePointerIcon(iconId); } @@ -899,7 +900,7 @@ void NativeInputManager::setPointerIconType(int32_t iconId) { void NativeInputManager::reloadPointerIcons() { AutoMutex _l(mLock); - sp controller = mLocked.pointerController.promote(); + std::shared_ptr controller = mLocked.pointerController.lock(); if (controller != nullptr) { controller->reloadPointerResources(); } @@ -907,7 +908,7 @@ void NativeInputManager::reloadPointerIcons() { void NativeInputManager::setCustomPointerIcon(const SpriteIcon& icon) { AutoMutex _l(mLock); - sp controller = mLocked.pointerController.promote(); + std::shared_ptr controller = mLocked.pointerController.lock(); if (controller != nullptr) { controller->setCustomPointerIcon(icon); }