From e37f8346c62551dac1919fad4cd60a707d27e6a2 Mon Sep 17 00:00:00 2001 From: Paul Ramirez Date: Wed, 28 Aug 2024 18:42:21 +0000 Subject: Add multiple device resampling support to InputConsumerNoResampling with tests Added multiple device resampling support to InputConsumerNoResampling with unit tests to ensure correctness Bug: 297226446 Flag: EXEMPT refactor Test: TEST=libinput_tests; m $TEST && $ANDROID_HOST_OUT/nativetest64/$TEST/$TEST --gtest_filter="InputConsumerTest*" Change-Id: I45528a89e0b60f46b0095078356382ed701b191b --- libs/input/InputConsumerNoResampling.cpp | 65 +++++++++++++++++++++----------- 1 file changed, 42 insertions(+), 23 deletions(-) (limited to 'libs/input/InputConsumerNoResampling.cpp') diff --git a/libs/input/InputConsumerNoResampling.cpp b/libs/input/InputConsumerNoResampling.cpp index cdbc1869c3..ce8bb43a76 100644 --- a/libs/input/InputConsumerNoResampling.cpp +++ b/libs/input/InputConsumerNoResampling.cpp @@ -17,8 +17,6 @@ #define LOG_TAG "InputConsumerNoResampling" #define ATRACE_TAG ATRACE_TAG_INPUT -#include - #include #include @@ -39,6 +37,8 @@ namespace { using std::chrono::nanoseconds; +using android::base::Result; + /** * Log debug messages relating to the consumer end of the transport channel. * Enable this via "adb shell setprop log.tag.InputTransportConsumer DEBUG" (requires restart) @@ -169,24 +169,18 @@ InputMessage createTimelineMessage(int32_t inputEventId, nsecs_t gpuCompletedTim msg.body.timeline.graphicsTimeline[GraphicsTimeline::PRESENT_TIME] = presentTime; return msg; } - -bool isPointerEvent(const MotionEvent& motionEvent) { - return (motionEvent.getSource() & AINPUT_SOURCE_CLASS_POINTER) == AINPUT_SOURCE_CLASS_POINTER; -} } // namespace -using android::base::Result; - // --- InputConsumerNoResampling --- -InputConsumerNoResampling::InputConsumerNoResampling(const std::shared_ptr& channel, - sp looper, - InputConsumerCallbacks& callbacks, - std::unique_ptr resampler) +InputConsumerNoResampling::InputConsumerNoResampling( + const std::shared_ptr& channel, sp looper, + InputConsumerCallbacks& callbacks, + std::function()> resamplerCreator) : mChannel{channel}, mLooper{looper}, mCallbacks{callbacks}, - mResampler{std::move(resampler)}, + mResamplerCreator{std::move(resamplerCreator)}, mFdEvents(0) { LOG_ALWAYS_FATAL_IF(mLooper == nullptr); mCallback = sp::make( @@ -319,7 +313,6 @@ void InputConsumerNoResampling::setFdEvents(int events) { } void InputConsumerNoResampling::handleMessages(std::vector&& messages) { - // TODO(b/297226446) : add resampling for (const InputMessage& msg : messages) { if (msg.header.type == InputMessage::Type::MOTION) { const int32_t action = msg.body.motion.action; @@ -329,12 +322,31 @@ void InputConsumerNoResampling::handleMessages(std::vector&& messa action == AMOTION_EVENT_ACTION_HOVER_MOVE) && (isFromSource(source, AINPUT_SOURCE_CLASS_POINTER) || isFromSource(source, AINPUT_SOURCE_CLASS_JOYSTICK)); + + const bool canResample = (mResamplerCreator != nullptr) && + (isFromSource(source, AINPUT_SOURCE_CLASS_POINTER)); + if (canResample) { + if (action == AMOTION_EVENT_ACTION_DOWN) { + if (std::unique_ptr resampler = mResamplerCreator(); + resampler != nullptr) { + const auto [_, inserted] = + mResamplers.insert(std::pair(deviceId, std::move(resampler))); + LOG_IF(WARNING, !inserted) << deviceId << "already exists in mResamplers"; + } + } + } + if (batchableEvent) { // add it to batch mBatches[deviceId].emplace(msg); } else { // consume all pending batches for this device immediately consumeBatchedInputEvents(deviceId, /*requestedFrameTime=*/std::nullopt); + if (canResample && + (action == AMOTION_EVENT_ACTION_UP || action == AMOTION_EVENT_ACTION_CANCEL)) { + LOG_IF(INFO, mResamplers.erase(deviceId) == 0) + << deviceId << "does not exist in mResamplers"; + } handleMessage(msg); } } else { @@ -456,8 +468,13 @@ InputConsumerNoResampling::createBatchedMotionEvent(const nsecs_t requestedFrame std::queue& messages) { std::unique_ptr motionEvent; std::optional firstSeqForBatch; - const nanoseconds resampleLatency = - (mResampler != nullptr) ? mResampler->getResampleLatency() : nanoseconds{0}; + + LOG_IF(FATAL, messages.empty()) << "messages queue is empty!"; + const DeviceId deviceId = messages.front().body.motion.deviceId; + const auto resampler = mResamplers.find(deviceId); + const nanoseconds resampleLatency = (resampler != mResamplers.cend()) + ? resampler->second->getResampleLatency() + : nanoseconds{0}; const nanoseconds adjustedFrameTime = nanoseconds{requestedFrameTime} - resampleLatency; while (!messages.empty() && @@ -474,15 +491,17 @@ InputConsumerNoResampling::createBatchedMotionEvent(const nsecs_t requestedFrame } messages.pop(); } + // Check if resampling should be performed. - if (motionEvent != nullptr && isPointerEvent(*motionEvent) && mResampler != nullptr) { - InputMessage* futureSample = nullptr; - if (!messages.empty()) { - futureSample = &messages.front(); - } - mResampler->resampleMotionEvent(nanoseconds{requestedFrameTime}, *motionEvent, - futureSample); + InputMessage* futureSample = nullptr; + if (!messages.empty()) { + futureSample = &messages.front(); } + if ((motionEvent != nullptr) && (resampler != mResamplers.cend())) { + resampler->second->resampleMotionEvent(nanoseconds{requestedFrameTime}, *motionEvent, + futureSample); + } + return std::make_pair(std::move(motionEvent), firstSeqForBatch); } -- cgit v1.2.3-59-g8ed1b From bc72309b1463a77c4548e0f9596d7a7a5dd75db0 Mon Sep 17 00:00:00 2001 From: Siarhei Vishniakou Date: Tue, 8 Oct 2024 15:41:51 -0700 Subject: Do not invoke callbacks when InputConsumer is destroyed Before this CL, the InputConsumer would try to finish all of the pending events that haven't been received by the app by forcefully consuming everything in the destructor. Unfortunately, this leads to the following crash: the callbacks (app) is storing the consumer inside unique_ptr. When the destructor of consumer runs, the unique_ptr will first assign the raw pointer to nullptr and then run the destructor. Next, inside the destructor, the callbacks are invoked, and the app may try to finish an input event that it receives. However, to finish, app needs to call back into the consumer, which is being destroyed. Since the only way the app can talk to the consumer is via unique_ptr, that pointer has already been set to nullptr, and this causes an NPE. To fix this, we will no longer be invoking the callbacks when the consumer destructor is invoked (this is done in this CL). The logic is as follows: - All events that have already been delivered to the app should still be finished by the app (this piece is debatable. we can fix this later if this becomes a problem). - Events that are batched, and not yet delivered to the app, will be automatically finished by the consumer, without sending them to the app. Since the app is destroying the consumer, it's unlikely that it cares about handling these events, anyways. If we want to finish _all_ pending messages (the messages that app hasn't yet called "finish" on), then we should probably iterate through mConsumeTimes, which is the only struct that keeps track of _all_ events that have been sent to the app, and not yet finished. However, to do this, we would need to rename this data structure, since it will no longer be solely used for keeping track of consume times, but also for "unprocessed events" that were read from the channel. Bug: 332613662 Flag: EXEMPT refactor Test: TEST=libinput_tests; m $TEST && $ANDROID_HOST_OUT/nativetest64/$TEST/$TEST Change-Id: I9b0128291f6197f099cb9e3c305f9717c0198c90 --- libs/input/InputConsumerNoResampling.cpp | 18 +++++++- libs/input/tests/InputConsumer_test.cpp | 77 +++++++++++++++++++++++++------- 2 files changed, 79 insertions(+), 16 deletions(-) (limited to 'libs/input/InputConsumerNoResampling.cpp') diff --git a/libs/input/InputConsumerNoResampling.cpp b/libs/input/InputConsumerNoResampling.cpp index ce8bb43a76..9665de799f 100644 --- a/libs/input/InputConsumerNoResampling.cpp +++ b/libs/input/InputConsumerNoResampling.cpp @@ -193,13 +193,29 @@ InputConsumerNoResampling::InputConsumerNoResampling( InputConsumerNoResampling::~InputConsumerNoResampling() { ensureCalledOnLooperThread(__func__); - consumeBatchedInputEvents(/*requestedFrameTime=*/std::nullopt); while (!mOutboundQueue.empty()) { processOutboundEvents(); // This is our last chance to ack the events. If we don't ack them here, we will get an ANR, // so keep trying to send the events as long as they are present in the queue. } + setFdEvents(0); + // If there are any remaining unread batches, send an ack for them and don't deliver + // them to callbacks. + for (auto& [_, batches] : mBatches) { + while (!batches.empty()) { + finishInputEvent(batches.front().header.seq, /*handled=*/false); + batches.pop(); + } + } + // However, it is still up to the app to finish any events that have already been delivered + // to the callbacks. If we wanted to change that behaviour and auto-finish all unfinished events + // that were already sent to callbacks, we could potentially loop through "mConsumeTimes" + // instead. We can't use "mBatchedSequenceNumbers" for this purpose, because it only contains + // batchable (i.e., ACTION_MOVE) events that were sent to the callbacks. + const size_t unfinishedEvents = mConsumeTimes.size(); + LOG_IF(INFO, unfinishedEvents != 0) + << getName() << " has " << unfinishedEvents << " unfinished event(s)"; } int InputConsumerNoResampling::handleReceiveCallback(int events) { diff --git a/libs/input/tests/InputConsumer_test.cpp b/libs/input/tests/InputConsumer_test.cpp index f899e2529f..6a3bbe5b36 100644 --- a/libs/input/tests/InputConsumer_test.cpp +++ b/libs/input/tests/InputConsumer_test.cpp @@ -112,6 +112,9 @@ protected: std::queue> mDragEvents; std::queue> mTouchModeEvents; + // Whether or not to automatically call "finish" whenever a motion event is received. + bool mShouldFinishMotions{true}; + private: uint32_t mLastSeq{0}; size_t mOnBatchedInputEventPendingInvocationCount{0}; @@ -119,11 +122,13 @@ private: // InputConsumerCallbacks interface void onKeyEvent(std::unique_ptr event, uint32_t seq) override { mKeyEvents.push(std::move(event)); - mConsumer->finishInputEvent(seq, true); + mConsumer->finishInputEvent(seq, /*handled=*/true); } void onMotionEvent(std::unique_ptr event, uint32_t seq) override { mMotionEvents.push(std::move(event)); - mConsumer->finishInputEvent(seq, true); + if (mShouldFinishMotions) { + mConsumer->finishInputEvent(seq, /*handled=*/true); + } } void onBatchedInputEventPending(int32_t pendingBatchSource) override { if (!mConsumer->probablyHasInput()) { @@ -133,19 +138,19 @@ private: }; void onFocusEvent(std::unique_ptr event, uint32_t seq) override { mFocusEvents.push(std::move(event)); - mConsumer->finishInputEvent(seq, true); + mConsumer->finishInputEvent(seq, /*handled=*/true); }; void onCaptureEvent(std::unique_ptr event, uint32_t seq) override { mCaptureEvents.push(std::move(event)); - mConsumer->finishInputEvent(seq, true); + mConsumer->finishInputEvent(seq, /*handled=*/true); }; void onDragEvent(std::unique_ptr event, uint32_t seq) override { mDragEvents.push(std::move(event)); - mConsumer->finishInputEvent(seq, true); + mConsumer->finishInputEvent(seq, /*handled=*/true); } void onTouchModeEvent(std::unique_ptr event, uint32_t seq) override { mTouchModeEvents.push(std::move(event)); - mConsumer->finishInputEvent(seq, true); + mConsumer->finishInputEvent(seq, /*handled=*/true); }; }; @@ -231,16 +236,58 @@ TEST_F(InputConsumerTest, LastBatchedSampleIsLessThanResampleTime) { EXPECT_LT(moveMotionEvent->getHistoricalEventTime(numSamples - 2), moveMotionEvent->getEventTime()); - // Consume all remaining events before ending the test. Otherwise, the smart pointer that owns - // consumer is set to null before destroying consumer. This leads to a member function call on a - // null object. - // TODO(b/332613662): Remove this workaround. - mConsumer->consumeBatchedInputEvents(std::nullopt); + mClientTestChannel->assertFinishMessage(/*seq=*/0, /*handled=*/true); + mClientTestChannel->assertFinishMessage(/*seq=*/1, /*handled=*/true); + mClientTestChannel->assertFinishMessage(/*seq=*/2, /*handled=*/true); + // The event with seq=3 remains unconsumed, and therefore finish will not be called for it until + // after the consumer is destroyed. + mConsumer.reset(); + mClientTestChannel->assertFinishMessage(/*seq=*/3, /*handled=*/false); + mClientTestChannel->assertNoSentMessages(); +} + +/** + * During normal operation, the user of InputConsumer (callbacks) is expected to call "finish" + * for each input event received in InputConsumerCallbacks. + * If the InputConsumer is destroyed, the events that were already sent to the callbacks will not + * be finished automatically. + */ +TEST_F(InputConsumerTest, UnhandledEventsNotFinishedInDestructor) { + mClientTestChannel->enqueueMessage( + InputMessageBuilder{InputMessage::Type::MOTION, /*seq=*/0}.action(ACTION_DOWN).build()); + mClientTestChannel->enqueueMessage( + InputMessageBuilder{InputMessage::Type::MOTION, /*seq=*/1}.action(ACTION_MOVE).build()); + mShouldFinishMotions = false; + invokeLooperCallback(); + assertOnBatchedInputEventPendingWasCalled(); + assertReceivedMotionEvent(WithMotionAction(ACTION_DOWN)); + mClientTestChannel->assertNoSentMessages(); + // The "finishInputEvent" was not called by the InputConsumerCallbacks. + // Now, destroy the consumer and check that the "finish" was not called automatically for the + // DOWN event, but was called for the undelivered MOVE event. + mConsumer.reset(); + mClientTestChannel->assertFinishMessage(/*seq=*/1, /*handled=*/false); + mClientTestChannel->assertNoSentMessages(); +} - mClientTestChannel->assertFinishMessage(/*seq=*/0, true); - mClientTestChannel->assertFinishMessage(/*seq=*/1, true); - mClientTestChannel->assertFinishMessage(/*seq=*/2, true); - mClientTestChannel->assertFinishMessage(/*seq=*/3, true); +/** + * Send an event to the InputConsumer, but do not invoke "consumeBatchedInputEvents", thus leaving + * the input event unconsumed by the callbacks. Ensure that no crash occurs when the consumer is + * destroyed. + * This test is similar to the one above, but here we are calling "finish" + * automatically for any event received in the callbacks. + */ +TEST_F(InputConsumerTest, UnconsumedEventDoesNotCauseACrash) { + mClientTestChannel->enqueueMessage( + InputMessageBuilder{InputMessage::Type::MOTION, /*seq=*/0}.action(ACTION_DOWN).build()); + invokeLooperCallback(); + assertReceivedMotionEvent(WithMotionAction(ACTION_DOWN)); + mClientTestChannel->assertFinishMessage(/*seq=*/0, /*handled=*/true); + mClientTestChannel->enqueueMessage( + InputMessageBuilder{InputMessage::Type::MOTION, /*seq=*/1}.action(ACTION_MOVE).build()); + invokeLooperCallback(); + mConsumer.reset(); + mClientTestChannel->assertFinishMessage(/*seq=*/1, /*handled=*/false); } TEST_F(InputConsumerTest, BatchedEventsMultiDeviceConsumption) { -- cgit v1.2.3-59-g8ed1b From 362fa2273d66fae4af1ec38cac32c0ad22e1acf5 Mon Sep 17 00:00:00 2001 From: Siarhei Vishniakou Date: Thu, 17 Oct 2024 14:19:52 -0700 Subject: Remove callback at the end of consumer destructor It turns out that 'finishInputEvent' was adding the fd back to the looper. This caused an NPE because we were calling 'finishInputEvent' at the end of the consumer destructor, thus leaving the callback in the looper. The looper would hit an NPE whenever the fd signaled after that. To fix this, we move the call to setFdEvents(0) to the very end of the destructor. This way, we can be sure that the last thing that executes is the removal of the fd from the Looper. There is also a possibility that fd is signaled during the destructor execution. Theoretically, this should be okay because the fd callbacks are processed on the same thread as the one where destructor runs. Therefore, the signals would only be processed after the destructor has completed, which means the callback would be removed before it gets the chance to execute. Bug: 332613662 Test: TEST=libinput_tests; m $TEST && $ANDROID_HOST_OUT/nativetest64/$TEST/$TEST --gtest_filter="*InputConsumerTest*" Test: TEST=libutils_test; m $TEST && $ANDROID_HOST_OUT/nativetest64/$TEST/$TEST Flag: EXEMPT bugfix Change-Id: If5ac7a8eaf96e842d5d8e44008b9c1bff74e674e --- include/input/InputConsumerNoResampling.h | 2 +- libs/input/InputConsumerNoResampling.cpp | 17 +++++++++------ libs/input/tests/InputConsumer_test.cpp | 36 ++++++++++++++++++++++++++++--- 3 files changed, 44 insertions(+), 11 deletions(-) (limited to 'libs/input/InputConsumerNoResampling.cpp') diff --git a/include/input/InputConsumerNoResampling.h b/include/input/InputConsumerNoResampling.h index 228347d818..2d1714c70a 100644 --- a/include/input/InputConsumerNoResampling.h +++ b/include/input/InputConsumerNoResampling.h @@ -141,7 +141,7 @@ private: } private: - std::function mCallback; + const std::function mCallback; }; sp mCallback; /** diff --git a/libs/input/InputConsumerNoResampling.cpp b/libs/input/InputConsumerNoResampling.cpp index 9665de799f..4cb6bf7bb4 100644 --- a/libs/input/InputConsumerNoResampling.cpp +++ b/libs/input/InputConsumerNoResampling.cpp @@ -193,13 +193,6 @@ InputConsumerNoResampling::InputConsumerNoResampling( InputConsumerNoResampling::~InputConsumerNoResampling() { ensureCalledOnLooperThread(__func__); - while (!mOutboundQueue.empty()) { - processOutboundEvents(); - // This is our last chance to ack the events. If we don't ack them here, we will get an ANR, - // so keep trying to send the events as long as they are present in the queue. - } - - setFdEvents(0); // If there are any remaining unread batches, send an ack for them and don't deliver // them to callbacks. for (auto& [_, batches] : mBatches) { @@ -208,6 +201,12 @@ InputConsumerNoResampling::~InputConsumerNoResampling() { batches.pop(); } } + + while (!mOutboundQueue.empty()) { + processOutboundEvents(); + // This is our last chance to ack the events. If we don't ack them here, we will get an ANR, + // so keep trying to send the events as long as they are present in the queue. + } // However, it is still up to the app to finish any events that have already been delivered // to the callbacks. If we wanted to change that behaviour and auto-finish all unfinished events // that were already sent to callbacks, we could potentially loop through "mConsumeTimes" @@ -216,6 +215,10 @@ InputConsumerNoResampling::~InputConsumerNoResampling() { const size_t unfinishedEvents = mConsumeTimes.size(); LOG_IF(INFO, unfinishedEvents != 0) << getName() << " has " << unfinishedEvents << " unfinished event(s)"; + // Remove the fd from epoll, so that Looper does not call 'handleReceiveCallback' anymore. + // This must be done at the end of the destructor; otherwise, some of the other functions may + // call 'setFdEvents' as a side-effect, thus adding the fd back to the epoll set of the looper. + setFdEvents(0); } int InputConsumerNoResampling::handleReceiveCallback(int events) { diff --git a/libs/input/tests/InputConsumer_test.cpp b/libs/input/tests/InputConsumer_test.cpp index 6a3bbe5b36..b32c2cb655 100644 --- a/libs/input/tests/InputConsumer_test.cpp +++ b/libs/input/tests/InputConsumer_test.cpp @@ -70,11 +70,20 @@ protected: []() { return std::make_unique(); }); } - void invokeLooperCallback() const { + bool invokeLooperCallback() const { sp callback; - ASSERT_TRUE(mLooper->getFdStateDebug(mClientTestChannel->getFd(), /*ident=*/nullptr, - /*events=*/nullptr, &callback, /*data=*/nullptr)); + const bool found = + mLooper->getFdStateDebug(mClientTestChannel->getFd(), /*ident=*/nullptr, + /*events=*/nullptr, &callback, /*data=*/nullptr); + if (!found) { + return false; + } + if (callback == nullptr) { + LOG(FATAL) << "Looper has the fd of interest, but the callback is null!"; + return false; + } callback->handleEvent(mClientTestChannel->getFd(), ALOOPER_EVENT_INPUT, /*data=*/nullptr); + return true; } void assertOnBatchedInputEventPendingWasCalled() { @@ -270,6 +279,27 @@ TEST_F(InputConsumerTest, UnhandledEventsNotFinishedInDestructor) { mClientTestChannel->assertNoSentMessages(); } +/** + * Check what happens when looper invokes callback after consumer has been destroyed. + * This reproduces a crash where the LooperEventCallback was added back to the Looper during + * destructor, thus allowing the looper callback to be invoked onto a null consumer object. + */ +TEST_F(InputConsumerTest, LooperCallbackInvokedAfterConsumerDestroyed) { + mClientTestChannel->enqueueMessage( + InputMessageBuilder{InputMessage::Type::MOTION, /*seq=*/0}.action(ACTION_DOWN).build()); + mClientTestChannel->enqueueMessage( + InputMessageBuilder{InputMessage::Type::MOTION, /*seq=*/1}.action(ACTION_MOVE).build()); + ASSERT_TRUE(invokeLooperCallback()); + assertOnBatchedInputEventPendingWasCalled(); + assertReceivedMotionEvent(WithMotionAction(ACTION_DOWN)); + mClientTestChannel->assertFinishMessage(/*seq=*/0, /*handled=*/true); + + // Now, destroy the consumer and invoke the looper callback again after it's been destroyed. + mConsumer.reset(); + mClientTestChannel->assertFinishMessage(/*seq=*/1, /*handled=*/false); + ASSERT_FALSE(invokeLooperCallback()); +} + /** * Send an event to the InputConsumer, but do not invoke "consumeBatchedInputEvents", thus leaving * the input event unconsumed by the callbacks. Ensure that no crash occurs when the consumer is -- cgit v1.2.3-59-g8ed1b From 37242c82244e56acf73458b83706d6b11e8b1289 Mon Sep 17 00:00:00 2001 From: Paul Ramirez Date: Fri, 18 Oct 2024 23:16:43 +0000 Subject: Do not resample when frameTime is not available LegacyResampler still resamples MotionEvents even when requestedFrameTime is std::nullopt. The problem is that if the parameter requestedFrameTime is std::nullopt, it is reassigned inside consumeBatchedInputEvents to std::numeric_limits::max(). In spite of using max() as a placeholder to indicate that everything should be consumed, it is still a valid time, and resampler uses it to resample MotionEvent. To fix this issue, having a valid requestedFrameTime should also be a condition to resample MotionEvents. Bug: 374375203 Flag: EXEMPT refactor Test: TEST=libinput_tests; m $TEST && $ANDROID_HOST_OUT/nativetest64/$TEST/$TEST Change-Id: I6e53bdd0a35d359da8b50f10dd4aad9bddc8aa3f --- include/input/InputConsumerNoResampling.h | 15 ++++++------ libs/input/InputConsumerNoResampling.cpp | 21 +++++++++-------- libs/input/tests/InputConsumerResampling_test.cpp | 28 +++++++++++++++++++++++ libs/input/tests/InputConsumer_test.cpp | 3 ++- 4 files changed, 50 insertions(+), 17 deletions(-) (limited to 'libs/input/InputConsumerNoResampling.cpp') diff --git a/include/input/InputConsumerNoResampling.h b/include/input/InputConsumerNoResampling.h index 228347d818..2e346bb7cd 100644 --- a/include/input/InputConsumerNoResampling.h +++ b/include/input/InputConsumerNoResampling.h @@ -211,16 +211,17 @@ private: * `consumeBatchedInputEvents`. */ std::map> mBatches; + /** - * Creates a MotionEvent by consuming samples from the provided queue. If one message has - * eventTime > adjustedFrameTime, all subsequent messages in the queue will be skipped. It is - * assumed that messages are queued in chronological order. In other words, only events that - * occurred prior to the adjustedFrameTime will be consumed. - * @param requestedFrameTime the time up to which to consume events. - * @param messages the queue of messages to consume from + * Creates a MotionEvent by consuming samples from the provided queue. Consumes all messages + * with eventTime <= requestedFrameTime - resampleLatency, where `resampleLatency` is latency + * introduced by the resampler. Assumes that messages are queued in chronological order. + * @param requestedFrameTime The time up to which consume messages, as given by the inequality + * above. If std::nullopt, everything in messages will be consumed. + * @param messages the queue of messages to consume from. */ std::pair, std::optional> createBatchedMotionEvent( - const nsecs_t requestedFrameTime, std::queue& messages); + const std::optional requestedFrameTime, std::queue& messages); /** * Consumes the batched input events, optionally allowing the caller to specify a device id diff --git a/libs/input/InputConsumerNoResampling.cpp b/libs/input/InputConsumerNoResampling.cpp index 9665de799f..d3653cfc45 100644 --- a/libs/input/InputConsumerNoResampling.cpp +++ b/libs/input/InputConsumerNoResampling.cpp @@ -357,7 +357,8 @@ void InputConsumerNoResampling::handleMessages(std::vector&& messa mBatches[deviceId].emplace(msg); } else { // consume all pending batches for this device immediately - consumeBatchedInputEvents(deviceId, /*requestedFrameTime=*/std::nullopt); + consumeBatchedInputEvents(deviceId, /*requestedFrameTime=*/ + std::numeric_limits::max()); if (canResample && (action == AMOTION_EVENT_ACTION_UP || action == AMOTION_EVENT_ACTION_CANCEL)) { LOG_IF(INFO, mResamplers.erase(deviceId) == 0) @@ -480,7 +481,7 @@ void InputConsumerNoResampling::handleMessage(const InputMessage& msg) const { } std::pair, std::optional> -InputConsumerNoResampling::createBatchedMotionEvent(const nsecs_t requestedFrameTime, +InputConsumerNoResampling::createBatchedMotionEvent(const std::optional requestedFrameTime, std::queue& messages) { std::unique_ptr motionEvent; std::optional firstSeqForBatch; @@ -491,7 +492,11 @@ InputConsumerNoResampling::createBatchedMotionEvent(const nsecs_t requestedFrame const nanoseconds resampleLatency = (resampler != mResamplers.cend()) ? resampler->second->getResampleLatency() : nanoseconds{0}; - const nanoseconds adjustedFrameTime = nanoseconds{requestedFrameTime} - resampleLatency; + // When batching is not enabled, we want to consume all events. That's equivalent to having an + // infinite requestedFrameTime. + const nanoseconds adjustedFrameTime = (requestedFrameTime.has_value()) + ? (nanoseconds{*requestedFrameTime} - resampleLatency) + : nanoseconds{std::numeric_limits::max()}; while (!messages.empty() && (messages.front().body.motion.eventTime <= adjustedFrameTime.count())) { @@ -513,8 +518,9 @@ InputConsumerNoResampling::createBatchedMotionEvent(const nsecs_t requestedFrame if (!messages.empty()) { futureSample = &messages.front(); } - if ((motionEvent != nullptr) && (resampler != mResamplers.cend())) { - resampler->second->resampleMotionEvent(nanoseconds{requestedFrameTime}, *motionEvent, + if ((motionEvent != nullptr) && (resampler != mResamplers.cend()) && + (requestedFrameTime.has_value())) { + resampler->second->resampleMotionEvent(nanoseconds{*requestedFrameTime}, *motionEvent, futureSample); } @@ -524,16 +530,13 @@ InputConsumerNoResampling::createBatchedMotionEvent(const nsecs_t requestedFrame bool InputConsumerNoResampling::consumeBatchedInputEvents( std::optional deviceId, std::optional requestedFrameTime) { ensureCalledOnLooperThread(__func__); - // When batching is not enabled, we want to consume all events. That's equivalent to having an - // infinite requestedFrameTime. - requestedFrameTime = requestedFrameTime.value_or(std::numeric_limits::max()); bool producedEvents = false; for (auto deviceIdIter = (deviceId.has_value()) ? (mBatches.find(*deviceId)) : (mBatches.begin()); deviceIdIter != mBatches.cend(); ++deviceIdIter) { std::queue& messages = deviceIdIter->second; - auto [motion, firstSeqForBatch] = createBatchedMotionEvent(*requestedFrameTime, messages); + auto [motion, firstSeqForBatch] = createBatchedMotionEvent(requestedFrameTime, messages); if (motion != nullptr) { LOG_ALWAYS_FATAL_IF(!firstSeqForBatch.has_value()); mCallbacks.onMotionEvent(std::move(motion), *firstSeqForBatch); diff --git a/libs/input/tests/InputConsumerResampling_test.cpp b/libs/input/tests/InputConsumerResampling_test.cpp index 883ca82fe0..a7feeeb301 100644 --- a/libs/input/tests/InputConsumerResampling_test.cpp +++ b/libs/input/tests/InputConsumerResampling_test.cpp @@ -566,4 +566,32 @@ TEST_F(InputConsumerResamplingTest, OldEventReceivedAfterResampleOccurs) { mClientTestChannel->assertFinishMessage(/*seq=*/4, /*handled=*/true); } +TEST_F(InputConsumerResamplingTest, DoNotResampleWhenFrameTimeIsNotAvailable) { + mClientTestChannel->enqueueMessage(nextPointerMessage( + {0ms, {Pointer{.id = 0, .x = 10.0f, .y = 20.0f}}, AMOTION_EVENT_ACTION_DOWN})); + + invokeLooperCallback(); + assertReceivedMotionEvent({InputEventEntry{0ms, + {Pointer{.id = 0, .x = 10.0f, .y = 20.0f}}, + AMOTION_EVENT_ACTION_DOWN}}); + + mClientTestChannel->enqueueMessage(nextPointerMessage( + {10ms, {Pointer{.id = 0, .x = 20.0f, .y = 30.0f}}, AMOTION_EVENT_ACTION_MOVE})); + mClientTestChannel->enqueueMessage(nextPointerMessage( + {20ms, {Pointer{.id = 0, .x = 30.0f, .y = 30.0f}}, AMOTION_EVENT_ACTION_MOVE})); + + invokeLooperCallback(); + mConsumer->consumeBatchedInputEvents(std::nullopt); + assertReceivedMotionEvent({InputEventEntry{10ms, + {Pointer{.id = 0, .x = 20.0f, .y = 30.0f}}, + AMOTION_EVENT_ACTION_MOVE}, + InputEventEntry{20ms, + {Pointer{.id = 0, .x = 30.0f, .y = 30.0f}}, + AMOTION_EVENT_ACTION_MOVE}}); + + mClientTestChannel->assertFinishMessage(/*seq=*/1, /*handled=*/true); + mClientTestChannel->assertFinishMessage(/*seq=*/2, /*handled=*/true); + mClientTestChannel->assertFinishMessage(/*seq=*/3, /*handled=*/true); +} + } // namespace android diff --git a/libs/input/tests/InputConsumer_test.cpp b/libs/input/tests/InputConsumer_test.cpp index 6a3bbe5b36..06e19bb644 100644 --- a/libs/input/tests/InputConsumer_test.cpp +++ b/libs/input/tests/InputConsumer_test.cpp @@ -194,7 +194,7 @@ TEST_F(InputConsumerTest, MessageStreamBatchedInMotionEvent) { std::unique_ptr moveMotionEvent = assertReceivedMotionEvent(WithMotionAction(ACTION_MOVE)); ASSERT_NE(moveMotionEvent, nullptr); - EXPECT_EQ(moveMotionEvent->getHistorySize() + 1, 3UL); + EXPECT_EQ(moveMotionEvent->getHistorySize() + 1, 2UL); mClientTestChannel->assertFinishMessage(/*seq=*/0, /*handled=*/true); mClientTestChannel->assertFinishMessage(/*seq=*/1, /*handled=*/true); @@ -443,4 +443,5 @@ TEST_F(InputConsumerTest, MultiDeviceResampling) { mClientTestChannel->assertFinishMessage(/*seq=*/8, /*handled=*/true); mClientTestChannel->assertFinishMessage(/*seq=*/10, /*handled=*/true); } + } // namespace android -- cgit v1.2.3-59-g8ed1b From eb63bbfd16ed9732be13d2ea6699edc6d7e4b545 Mon Sep 17 00:00:00 2001 From: Siarhei Vishniakou Date: Wed, 13 Nov 2024 15:13:29 -0800 Subject: Do not crash when server channel has closed If the publisher has been destroyed (or, equivalently, if the server channel was closed), the consumer should not crash. Instead, we should allow consumer to exit gracefully. In this CL, we specifically check for DEAD_OBJECT and drain the queue, printing all of the events that will never be delivered to the publisher for useful debugging purposes. Bug: 305165753 Test: TEST=libinput_tests; m $TEST && $ANDROID_HOST_OUT/nativetest64/$TEST/$TEST Flag: EXEMPT bugfix Change-Id: I00973ebb87e0f59bfd3c0f58edf7355b28988c15 --- libs/input/InputConsumerNoResampling.cpp | 15 +++++++ .../InputPublisherAndConsumerNoResampling_test.cpp | 48 +++++++++++++++------- 2 files changed, 48 insertions(+), 15 deletions(-) (limited to 'libs/input/InputConsumerNoResampling.cpp') diff --git a/libs/input/InputConsumerNoResampling.cpp b/libs/input/InputConsumerNoResampling.cpp index 2c0f77a814..cd8582182a 100644 --- a/libs/input/InputConsumerNoResampling.cpp +++ b/libs/input/InputConsumerNoResampling.cpp @@ -169,6 +169,12 @@ InputMessage createTimelineMessage(int32_t inputEventId, nsecs_t gpuCompletedTim msg.body.timeline.graphicsTimeline[GraphicsTimeline::PRESENT_TIME] = presentTime; return msg; } + +std::ostream& operator<<(std::ostream& out, const InputMessage& msg) { + out << ftl::enum_string(msg.header.type); + return out; +} + } // namespace // --- InputConsumerNoResampling --- @@ -272,6 +278,15 @@ void InputConsumerNoResampling::processOutboundEvents() { return; // try again later } + if (result == DEAD_OBJECT) { + // If there's no one to receive events in the channel, there's no point in sending them. + // Drop all outbound events. + LOG(INFO) << "Channel " << mChannel->getName() << " died. Dropping outbound event " + << outboundMsg; + mOutboundQueue.pop(); + setFdEvents(0); + continue; + } // Some other error. Give up LOG(FATAL) << "Failed to send outbound event on channel '" << mChannel->getName() << "'. status=" << statusToString(result) << "(" << result << ")"; diff --git a/libs/input/tests/InputPublisherAndConsumerNoResampling_test.cpp b/libs/input/tests/InputPublisherAndConsumerNoResampling_test.cpp index 39bb841c0a..1dadae98e4 100644 --- a/libs/input/tests/InputPublisherAndConsumerNoResampling_test.cpp +++ b/libs/input/tests/InputPublisherAndConsumerNoResampling_test.cpp @@ -319,6 +319,8 @@ protected: protected: // Interaction with the looper thread + void blockLooper(); + void unblockLooper(); enum class LooperMessage : int { CALL_PROBABLY_HAS_INPUT, CREATE_CONSUMER, @@ -389,6 +391,26 @@ private: }; }; +void InputPublisherAndConsumerNoResamplingTest::blockLooper() { + { + std::scoped_lock l(mLock); + mLooperMayProceed = false; + } + sendMessage(LooperMessage::BLOCK_LOOPER); + { + std::unique_lock l(mLock); + mNotifyLooperWaiting.wait(l, [this] { return mLooperIsBlocked; }); + } +} + +void InputPublisherAndConsumerNoResamplingTest::unblockLooper() { + { + std::scoped_lock l(mLock); + mLooperMayProceed = true; + } + mNotifyLooperMayProceed.notify_all(); +} + void InputPublisherAndConsumerNoResamplingTest::sendMessage(LooperMessage message) { Message msg{ftl::to_underlying(message)}; mLooper->sendMessage(mMessageHandler, msg); @@ -600,15 +622,7 @@ void InputPublisherAndConsumerNoResamplingTest::publishAndConsumeSinglePointerMu std::queue publishedSequenceNumbers; // Block Looper to increase the chance of batching events - { - std::scoped_lock l(mLock); - mLooperMayProceed = false; - } - sendMessage(LooperMessage::BLOCK_LOOPER); - { - std::unique_lock l(mLock); - mNotifyLooperWaiting.wait(l, [this] { return mLooperIsBlocked; }); - } + blockLooper(); uint32_t firstSampleId; for (size_t i = 0; i < nSamples; ++i) { @@ -629,12 +643,7 @@ void InputPublisherAndConsumerNoResamplingTest::publishAndConsumeSinglePointerMu std::vector singleSampledMotionEvents; - // Unblock Looper - { - std::scoped_lock l(mLock); - mLooperMayProceed = true; - } - mNotifyLooperMayProceed.notify_all(); + unblockLooper(); // We have no control over the socket behavior, so the consumer can receive // the motion as a batched event, or as a sequence of multiple single-sample MotionEvents (or a @@ -809,6 +818,15 @@ void InputPublisherAndConsumerNoResamplingTest::publishAndConsumeTouchModeEvent( verifyFinishedSignal(*mPublisher, seq, publishTime); } +/** + * If the publisher has died, consumer should not crash when trying to send an outgoing message. + */ +TEST_F(InputPublisherAndConsumerNoResamplingTest, ConsumerWritesAfterPublisherDies) { + mPublisher.reset(); // The publisher has died + mReportTimelineArgs.emplace(/*inputEventId=*/10, /*gpuCompletedTime=*/20, /*presentTime=*/30); + sendMessage(LooperMessage::CALL_REPORT_TIMELINE); +} + TEST_F(InputPublisherAndConsumerNoResamplingTest, SendTimeline) { const int32_t inputEventId = 20; const nsecs_t gpuCompletedTime = 30; -- cgit v1.2.3-59-g8ed1b