From c4c39551c442101a46ee73f66912d29495a398a1 Mon Sep 17 00:00:00 2001 From: Denis Brockus Date: Fri, 10 Mar 2023 13:56:12 -0700 Subject: lookahead_filter_interpreter: use std::list Using std::list directly instead of using a pointer std::list with the extra code to manage a free_list_ BUG=b:271591258 TEST=USE="coverage" FEATURES="test noclean" emerge-brya chromeos-base/gestures Change-Id: I5d086d4826fef83105ce71d37c8002422c8f5ad2 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/gestures/+/4334918 Reviewed-by: Kenneth Albanowski Code-Coverage: Henry Barnor Commit-Queue: Denis Brockus Reviewed-by: Harry Cutts Code-Coverage: Denis Brockus Auto-Submit: Denis Brockus Tested-by: Denis Brockus Reviewed-by: Henry Barnor --- include/lookahead_filter_interpreter.h | 19 +-- src/lookahead_filter_interpreter.cc | 179 +++++++++++---------------- src/lookahead_filter_interpreter_unittest.cc | 80 ++++++------ 3 files changed, 122 insertions(+), 156 deletions(-) diff --git a/include/lookahead_filter_interpreter.h b/include/lookahead_filter_interpreter.h index 31aefe4..890919f 100644 --- a/include/lookahead_filter_interpreter.h +++ b/include/lookahead_filter_interpreter.h @@ -6,7 +6,7 @@ #include #include #include -#include +#include #include // For FRIEND_TEST @@ -65,10 +65,7 @@ class LookaheadFilterInterpreter : public FilterInterpreter { std::map output_ids_; // input tracking ids -> output stime_t due_; - bool completed_; - - QState* next_; - QState* prev_; + bool completed_ = false; }; void LogVectors(); @@ -111,13 +108,17 @@ class LookaheadFilterInterpreter : public FilterInterpreter { stime_t ExtraVariableDelay() const; - class QStateList : public std::list { + typedef std::optional< + std::reference_wrapper< + LookaheadFilterInterpreter::QState + > + > OptionalRefQState; + + class QStateList : public std::list { public: - QState* at(int offset); + OptionalRefQState at(int offset); } queue_; - std::vector free_list_; - // The last id assigned to a contact (part of drumroll suppression) short last_id_; diff --git a/src/lookahead_filter_interpreter.cc b/src/lookahead_filter_interpreter.cc index f60b0f5..730becf 100644 --- a/src/lookahead_filter_interpreter.cc +++ b/src/lookahead_filter_interpreter.cc @@ -19,12 +19,9 @@ namespace { static const stime_t kMaxDelay = 0.09; // 90ms } -static const size_t kMaxQNodes = 16; - LookaheadFilterInterpreter::LookaheadFilterInterpreter( PropRegistry* prop_reg, Interpreter* next, Tracer* tracer) : FilterInterpreter(NULL, next, tracer, false), - free_list_(kMaxQNodes), last_id_(0), max_fingers_per_hwstate_(0), interpreter_due_(-1.0), last_interpreted_time_(-1.0), min_nonsuppress_speed_(prop_reg, "Input Queue Min Nonsuppression Speed", @@ -48,43 +45,32 @@ LookaheadFilterInterpreter::LookaheadFilterInterpreter( void LookaheadFilterInterpreter::SyncInterpretImpl(HardwareState* hwstate, stime_t* timeout) { - // Push back into queue - if (free_list_.empty()) { - Err("Can't accept new hwstate b/c we're out of nodes!"); - Err("Now: %f, interpreter_due_ %f", hwstate->timestamp, interpreter_due_); - Err("Dump of queue:"); - for (const auto& element : queue_) - Err("Due: %f%s", element->due_, element->completed_ ? " (c)" : ""); - return; - } - QState* node = free_list_.back(); - free_list_.pop_back(); - node->set_state(*hwstate); + // Keep track of where the last node is in the current queue_ + auto const queue_was_not_empty = !queue_.empty(); + QState* old_back_node = queue_was_not_empty ? &queue_.back() : nullptr; + // Allocate and initialize a new node on the end of the queue_ + auto& new_node = queue_.emplace_back(hwprops_->max_finger_cnt); + new_node.set_state(*hwstate); double delay = max(0.0, min(kMaxDelay, min_delay_.val_)); - node->due_ = hwstate->timestamp + delay; - node->completed_ = false; - if (queue_.empty()) - node->output_ids_.clear(); - else - node->output_ids_ = queue_.back()->output_ids_; - // At this point, if ExtraVariableDelay() > 0, queue_.back()->due_ may have - // ExtraVariableDelay() applied, but node->due_ does not, yet. - if (!queue_.empty() && - (queue_.back()->due_ - node->due_ > ExtraVariableDelay())) { + new_node.due_ = hwstate->timestamp + delay; + if (queue_was_not_empty) + new_node.output_ids_ = old_back_node->output_ids_; + // At this point, if ExtraVariableDelay() > 0, old_back_node.due_ may have + // ExtraVariableDelay() applied, but new_node.due_ does not, yet. + if (queue_was_not_empty && + (old_back_node->due_ - new_node.due_ > ExtraVariableDelay())) { Err("Clock changed backwards. Flushing queue."); stime_t next_timeout = NO_DEADLINE; auto q_node_iter = queue_.begin(); do { - if (!(*q_node_iter)->completed_) - next_->SyncInterpret(&(*q_node_iter)->state_, &next_timeout); + if (!(*q_node_iter).completed_) + next_->SyncInterpret(&(*q_node_iter).state_, &next_timeout); ++q_node_iter; - free_list_.push_back(queue_.front()); queue_.pop_front(); - } while (!queue_.empty()); + } while (queue_.size() > 1); interpreter_due_ = -1.0; last_interpreted_time_ = -1.0; } - queue_.push_back(node); AssignTrackingIds(); AttemptInterpolation(); UpdateInterpreterDue(interpreter_due_ < 0.0 ? @@ -93,10 +79,10 @@ void LookaheadFilterInterpreter::SyncInterpretImpl(HardwareState* hwstate, HandleTimerImpl(hwstate->timestamp, timeout); // Copy finger flags for upstream filters. - QState* q_node = queue_.front(); - if (q_node->state_.SameFingersAs(*hwstate)) { + QState& q_node = queue_.front(); + if (q_node.state_.SameFingersAs(*hwstate)) { for (size_t i = 0; i < hwstate->finger_cnt; i++) { - hwstate->fingers[i].flags = q_node->state_.fingers[i].flags; + hwstate->fingers[i].flags = q_node.state_.fingers[i].flags; } } } @@ -148,27 +134,27 @@ void LookaheadFilterInterpreter::AssignTrackingIds() { // Always reassign trackingID on the very first hwstate so that // the next hwstate can inherit the trackingID mapping. if (queue_.size() == 1) { - QState* tail = queue_.back(); - HardwareState* hs = &tail->state_; + QState& tail = queue_.back(); + HardwareState* hs = &tail.state_; for (size_t i = 0; i < hs->finger_cnt; i++) { FingerState* fs = &hs->fingers[i]; - tail->output_ids_[fs->tracking_id] = NextTrackingId(); - fs->tracking_id = tail->output_ids_[fs->tracking_id]; + tail.output_ids_[fs->tracking_id] = NextTrackingId(); + fs->tracking_id = tail.output_ids_[fs->tracking_id]; } if (hs->finger_cnt > 0) - tail->due_ += ExtraVariableDelay(); + tail.due_ += ExtraVariableDelay(); } return; } - auto tail = queue_.at(-1); - HardwareState* hs = &tail->state_; - QState* prev_qs = queue_.size() < 2 ? NULL : queue_.at(-2); + auto& tail = queue_.at(-1)->get(); + HardwareState* hs = &tail.state_; + QState* prev_qs = queue_.size() < 2 ? NULL : &(queue_.at(-2)->get()); HardwareState* prev_hs = prev_qs ? &prev_qs->state_ : NULL; - QState* prev2_qs = queue_.size() < 3 ? NULL : queue_.at(-3); + QState* prev2_qs = queue_.size() < 3 ? NULL : &(queue_.at(-3)->get()); HardwareState* prev2_hs = prev2_qs ? &prev2_qs->state_ : NULL; - RemoveMissingIdsFromMap(&tail->output_ids_, *hs); + RemoveMissingIdsFromMap(&tail.output_ids_, *hs); float dt = prev_hs ? hs->timestamp - prev_hs->timestamp : 1.0; float prev_dt = prev_hs && prev2_hs ? prev_hs->timestamp - prev2_hs->timestamp : 1.0; @@ -191,11 +177,11 @@ void LookaheadFilterInterpreter::AssignTrackingIds() { FingerState* fs = &hs->fingers[i]; const short old_id = fs->tracking_id; bool new_finger = false; - if (!MapContainsKey(tail->output_ids_, fs->tracking_id)) { - tail->output_ids_[fs->tracking_id] = NextTrackingId(); + if (!MapContainsKey(tail.output_ids_, fs->tracking_id)) { + tail.output_ids_[fs->tracking_id] = NextTrackingId(); new_finger_present = new_finger = true; } - fs->tracking_id = tail->output_ids_[fs->tracking_id]; + fs->tracking_id = tail.output_ids_[fs->tracking_id]; if (new_finger) continue; if (!prev_hs) { @@ -244,8 +230,8 @@ void LookaheadFilterInterpreter::AssignTrackingIds() { if (prev_qs->output_ids_[old_id] != prev2_qs->output_ids_[old_id]) { prev_qs->output_ids_[old_id] = prev2_qs->output_ids_[old_id]; prev_fs->tracking_id = prev_qs->output_ids_[old_id]; - tail->output_ids_[old_id] = prev2_qs->output_ids_[old_id]; - fs->tracking_id = tail->output_ids_[old_id]; + tail.output_ids_[old_id] = prev2_qs->output_ids_[old_id]; + fs->tracking_id = tail.output_ids_[old_id]; continue; } } @@ -274,7 +260,7 @@ void LookaheadFilterInterpreter::AssignTrackingIds() { continue; } separated_fingers.insert(old_id); - SeparateFinger(tail, fs, old_id); + SeparateFinger(&tail, fs, old_id); // Separating fingers shouldn't tap. fs->flags |= GESTURES_FINGER_NO_TAP; // Try to also flag the previous frame, if we didn't execute it yet @@ -303,10 +289,10 @@ void LookaheadFilterInterpreter::AssignTrackingIds() { Err("How is input ID missing from prev state? %d", input_id); continue; } - short new_bad_output_id = tail->output_ids_[input_id]; + short new_bad_output_id = tail.output_ids_[input_id]; short prev_output_id = prev_qs->output_ids_[input_id]; - tail->output_ids_[input_id] = prev_output_id; - FingerState* fs = tail->state_.GetFingerState(new_bad_output_id); + tail.output_ids_[input_id] = prev_output_id; + FingerState* fs = tail.state_.GetFingerState(new_bad_output_id); if (!fs) { Err("Can't find finger state."); continue; @@ -321,7 +307,7 @@ void LookaheadFilterInterpreter::AssignTrackingIds() { LiftoffJumpStarting(*hs, *prev_hs, *prev2_hs))) { // Possibly add some extra delay to correct, incase this separation // shouldn't have occurred or if the finger may be lifting from the pad. - tail->due_ += ExtraVariableDelay(); + tail.due_ += ExtraVariableDelay(); } } @@ -361,12 +347,12 @@ void LookaheadFilterInterpreter::TapDownOccurringGesture(stime_t now) { return; if (queue_.size() < 2) return; // Not enough data to know - HardwareState& hs = queue_.back()->state_; - if (queue_.back()->state_.timestamp != now) + HardwareState& hs = queue_.back().state_; + if (queue_.back().state_.timestamp != now) return; // We didn't push a new hardware state now // See if latest hwstate has finger that previous doesn't. // Get the state_ of back->prev - HardwareState& prev_hs = queue_.at(-2)->state_; + HardwareState& prev_hs = queue_.at(-2)->get().state_; if (hs.finger_cnt > prev_hs.finger_cnt) { // Finger was added. ProduceGesture(Gesture(kGestureFling, prev_hs.timestamp, hs.timestamp, @@ -402,33 +388,25 @@ short LookaheadFilterInterpreter::NextTrackingId() { void LookaheadFilterInterpreter::AttemptInterpolation() { if (queue_.size() < 2) return; - QState* new_node = queue_.at(-1); - QState* prev = queue_.at(-2); - if (new_node->state_.timestamp - prev->state_.timestamp < + QState& new_node = queue_.at(-1)->get(); + QState& prev = queue_.at(-2)->get(); + if (new_node.state_.timestamp - prev.state_.timestamp < split_min_period_.val_) { return; // Nodes came in too quickly to need interpolation } - if (!prev->state_.SameFingersAs(new_node->state_)) - return; - if (free_list_.empty()) { - Err("out of nodes?"); + if (!prev.state_.SameFingersAs(new_node.state_)) return; - } - QState* node = free_list_.back(); - free_list_.pop_back(); - node->state_.fingers = node->fs_.get(); - node->completed_ = false; - Interpolate(prev->state_, new_node->state_, &node->state_); + auto node = QState(hwprops_->max_finger_cnt); + node.state_.fingers = node.fs_.get(); + node.completed_ = false; + Interpolate(prev.state_, new_node.state_, &node.state_); double delay = max(0.0, min(kMaxDelay, min_delay_.val_)); - node->due_ = node->state_.timestamp + delay; + node.due_ = node.state_.timestamp + delay; - if (node->state_.timestamp <= last_interpreted_time_) { - // Time wouldn't seem monotonically increasing w/ this new event, so - // discard it. - free_list_.push_back(node); - } else { - queue_.insert(--queue_.end(), node); + // Make sure time seems monotonically increasing w/ this new event + if (node.state_.timestamp > last_interpreted_time_) { + queue_.insert(--queue_.end(), std::move(node)); } } @@ -450,10 +428,10 @@ void LookaheadFilterInterpreter::HandleTimerImpl(stime_t now, if (queue_.empty()) break; // Get next uncompleted and overdue hwstate - auto node = queue_.back(); - for (auto state : queue_) { - if (!state->completed_) { - node = state; + auto node = &queue_.back(); + for (auto& elem : queue_) { + if (!elem.completed_) { + node = &elem; break; } } @@ -482,8 +460,7 @@ void LookaheadFilterInterpreter::HandleTimerImpl(stime_t now, next_->SyncInterpret(&hs_copy, &next_timeout); // Clear previously completed nodes, but keep at least two nodes. - while (queue_.size() > 2 && queue_.front()->completed_) { - free_list_.push_back(queue_.front()); + while (queue_.size() > 2 && queue_.front().completed_) { queue_.pop_front(); } @@ -502,7 +479,7 @@ void LookaheadFilterInterpreter::HandleTimerImpl(stime_t now, } void LookaheadFilterInterpreter::ConsumeGesture(const Gesture& gesture) { - QState* node = queue_.front(); + QState& node = queue_.front(); float distance_sq = 0.0; // Slow movements should potentially be suppressed @@ -530,8 +507,8 @@ void LookaheadFilterInterpreter::ConsumeGesture(const Gesture& gesture) { } // Speed is slow. Suppress if fingers have changed. for (auto iter = ++queue_.begin(); iter != queue_.end(); ++iter) { - if (!node->state_.SameFingersAs((*iter)->state_) || - (node->state_.buttons_down != (*iter)->state_.buttons_down)) + if (!node.state_.SameFingersAs((*iter).state_) || + (node.state_.buttons_down != (*iter).state_.buttons_down)) return; // suppress } @@ -546,10 +523,10 @@ void LookaheadFilterInterpreter::UpdateInterpreterDue( // timeout, so we use -DBL_MAX as the invalid value. stime_t next_hwstate_timeout = -DBL_MAX; // Scan queue_ to find when next hwstate is due. - for (auto elem : queue_) { - if (elem->completed_) + for (auto& elem : queue_) { + if (elem.completed_) continue; - next_hwstate_timeout = elem->due_ - now; + next_hwstate_timeout = elem.due_ - now; break; } @@ -571,11 +548,6 @@ void LookaheadFilterInterpreter::Initialize( GestureConsumer* consumer) { FilterInterpreter::Initialize(hwprops, NULL, mprops, consumer); queue_.clear(); - free_list_.clear(); - for (size_t i = 0; i < kMaxQNodes; ++i) { - QState* node = new QState(hwprops_->max_finger_cnt); - free_list_.push_back(node); - } } stime_t LookaheadFilterInterpreter::ExtraVariableDelay() const { @@ -583,13 +555,13 @@ stime_t LookaheadFilterInterpreter::ExtraVariableDelay() const { } LookaheadFilterInterpreter::QState::QState() - : max_fingers_(0), completed_(false), next_(NULL), prev_(NULL) { + : max_fingers_(0) { fs_.reset(); state_.fingers = NULL; } LookaheadFilterInterpreter::QState::QState(unsigned short max_fingers) - : max_fingers_(max_fingers), completed_(false), next_(NULL), prev_(NULL) { + : max_fingers_(max_fingers) { fs_.reset(new FingerState[max_fingers]); state_.fingers = fs_.get(); } @@ -615,25 +587,24 @@ void LookaheadFilterInterpreter::QState::set_state( state_.msc_timestamp = new_state.msc_timestamp; } -LookaheadFilterInterpreter::QState* +LookaheadFilterInterpreter::OptionalRefQState LookaheadFilterInterpreter::QStateList::at(int offset) { // Traverse to the appropriate offset if (offset < 0) { // negative offset is from end to begin - auto count = size(); - for (auto iter = --end(); count > 0; --count, --iter) { + for (auto iter = rbegin(); iter != rend(); ++iter) { if (++offset == 0) - return *iter; + return LookaheadFilterInterpreter::OptionalRefQState{*iter}; } } else { // positive offset is from begin to end - for (auto elem : *this) { - if (offset-- == 0) - return elem; + for (auto iter = begin(); iter != end(); ++iter, --offset) { + if (offset == 0) + return LookaheadFilterInterpreter::OptionalRefQState{*iter}; } } - // invalid offset returns NULL - return nullptr; + // Invalid offset + return std::nullopt; } } // namespace gestures diff --git a/src/lookahead_filter_interpreter_unittest.cc b/src/lookahead_filter_interpreter_unittest.cc index 1c84eb4..e4d02a9 100644 --- a/src/lookahead_filter_interpreter_unittest.cc +++ b/src/lookahead_filter_interpreter_unittest.cc @@ -175,7 +175,7 @@ TEST(LookaheadFilterInterpreterTest, SimpleTest) { if (newtimeout < 0.0) EXPECT_DOUBLE_EQ(NO_DEADLINE, newtimeout); if (i == 5) { - EXPECT_EQ(reinterpret_cast(NULL), out); + EXPECT_EQ(nullptr, out); } else { // Expect movement ASSERT_TRUE(out); @@ -389,19 +389,19 @@ TEST(LookaheadFilterInterpreterTest, SpuriousCallbackTest) { stime_t timeout = NO_DEADLINE; Gesture* out = wrapper.SyncInterpret(&hs, &timeout); - EXPECT_EQ(reinterpret_cast(NULL), out); + EXPECT_EQ(nullptr, out); EXPECT_FLOAT_EQ(interpreter->min_delay_.val_, timeout); out = wrapper.HandleTimer(hs.timestamp + interpreter->min_delay_.val_, &timeout); - EXPECT_EQ(reinterpret_cast(NULL), out); + EXPECT_EQ(nullptr, out); EXPECT_FLOAT_EQ(1.0, timeout); out = wrapper.HandleTimer(hs.timestamp + interpreter->min_delay_.val_ + 0.25, &timeout); - EXPECT_EQ(reinterpret_cast(NULL), out); + EXPECT_EQ(nullptr, out); EXPECT_FLOAT_EQ(0.75, timeout); } @@ -640,20 +640,20 @@ TEST(LookaheadFilterInterpreterTest, InterpolationOverdueTest) { stime_t timeout = NO_DEADLINE; Gesture* out = wrapper.SyncInterpret(&hs[0], &timeout); - EXPECT_EQ(reinterpret_cast(NULL), out); + EXPECT_EQ(nullptr, out); EXPECT_FLOAT_EQ(timeout, interpreter->min_delay_.val_); stime_t now = hs[0].timestamp + timeout; timeout = NO_DEADLINE; out = wrapper.HandleTimer(now, &timeout); - ASSERT_NE(reinterpret_cast(NULL), out); + ASSERT_NE(nullptr, out); EXPECT_EQ(kGestureTypeMove, out->type); EXPECT_EQ(1, out->details.move.dy); EXPECT_DOUBLE_EQ(timeout, 0.700); timeout = NO_DEADLINE; out = wrapper.SyncInterpret(&hs[1], &timeout); - ASSERT_NE(reinterpret_cast(NULL), out); + ASSERT_NE(nullptr, out); EXPECT_EQ(kGestureTypeMove, out->type); EXPECT_EQ(2, out->details.move.dy); EXPECT_GE(timeout, 0.0); @@ -793,17 +793,17 @@ TEST(LookaheadFilterInterpreterTest, QuickMoveTest) { // Pushing the first event wrapper.SyncInterpret(&hs[0], &timeout); EXPECT_EQ(queue.size(), 1); - EXPECT_EQ(queue.back()->fs_[0].tracking_id, 1); + EXPECT_EQ(queue.back().fs_[0].tracking_id, 1); // Expecting Drumroll detected and ID reassigned 1 -> 2. wrapper.SyncInterpret(&hs[1], &timeout); EXPECT_EQ(queue.size(), 2); - EXPECT_EQ(queue.back()->fs_[0].tracking_id, 2); + EXPECT_EQ(queue.back().fs_[0].tracking_id, 2); // Expecting Drumroll detected and ID reassigned 1 -> 3. wrapper.SyncInterpret(&hs[2], &timeout); EXPECT_EQ(queue.size(), 3); - EXPECT_EQ(queue.back()->fs_[0].tracking_id, 3); + EXPECT_EQ(queue.back().fs_[0].tracking_id, 3); // Removing the touch. wrapper.SyncInterpret(&hs[3], &timeout); @@ -813,17 +813,17 @@ TEST(LookaheadFilterInterpreterTest, QuickMoveTest) { // New finger tracking ID assigned 2 - > 4. wrapper.SyncInterpret(&hs[4], &timeout); EXPECT_EQ(queue.size(), 2); - EXPECT_EQ(queue.back()->fs_[0].tracking_id, 4); + EXPECT_EQ(queue.back().fs_[0].tracking_id, 4); // Expecting Drumroll detected and ID reassigned 2 -> 5. wrapper.SyncInterpret(&hs[5], &timeout); - EXPECT_EQ(queue.back()->fs_[0].tracking_id, 5); + EXPECT_EQ(queue.back().fs_[0].tracking_id, 5); // Expecting Quick movement detected and ID correction 5 -> 4. wrapper.SyncInterpret(&hs[6], &timeout); - EXPECT_EQ(interpreter->queue_.at(-1)->fs_[0].tracking_id, 4); - EXPECT_EQ(interpreter->queue_.at(-2)->fs_[0].tracking_id, 4); - EXPECT_EQ(interpreter->queue_.at(-3)->fs_[0].tracking_id, 4); + EXPECT_EQ(interpreter->queue_.at(-1)->get().fs_[0].tracking_id, 4); + EXPECT_EQ(interpreter->queue_.at(-2)->get().fs_[0].tracking_id, 4); + EXPECT_EQ(interpreter->queue_.at(-3)->get().fs_[0].tracking_id, 4); } struct QuickSwipeTestInputs { @@ -1266,17 +1266,19 @@ TEST(LookaheadFilterInterpreterTest, SemiMtNoTrackingIdAssignmentTest) { const auto& queue = interpreter->queue_; wrapper.SyncInterpret(&hs[0], &timeout); - EXPECT_EQ(queue.back()->fs_[0].tracking_id, 20); + EXPECT_EQ(queue.back().fs_[0].tracking_id, 20); // Test if the fingers in queue have the same tracking ids from input. for (size_t i = 1; i < arraysize(hs); i++) { wrapper.SyncInterpret(&hs[i], &timeout); - EXPECT_EQ(queue.back()->fs_[0].tracking_id, 20); // the same input id - EXPECT_EQ(queue.back()->fs_[1].tracking_id, 21); + EXPECT_EQ(queue.back().fs_[0].tracking_id, 20); // the same input id + EXPECT_EQ(queue.back().fs_[1].tracking_id, 21); } } TEST(LookaheadFilterInterpreterTest, QStateListTest) { + const size_t kMaxQNodes = 4; + LookaheadFilterInterpreterTestInterpreter* base_interpreter = NULL; std::unique_ptr interpreter; @@ -1300,42 +1302,34 @@ TEST(LookaheadFilterInterpreterTest, QStateListTest) { wrapper.Reset(interpreter.get()); auto& queue = interpreter->queue_; - auto& free_list = interpreter->free_list_; - // Release all of the QStates - while (!queue.empty()) { - free_list.push_back(queue.front()); - queue.pop_front(); - } + EXPECT_TRUE(queue.empty()); EXPECT_EQ(queue.size(), 0); - EXPECT_EQ(free_list.size(), 16); - EXPECT_EQ(queue.at(-1), nullptr); - EXPECT_EQ(queue.at(queue.size()), nullptr); + // Use the default constructor one time... just for coverage + queue.push_front(LookaheadFilterInterpreter::QState()); - // fill up the QStates - while (!free_list.empty()) { - queue.push_front(free_list.back()); - free_list.pop_back(); + for (size_t i{1}; i < kMaxQNodes; ++i) { + queue.push_front( + LookaheadFilterInterpreter::QState(hwprops.max_finger_cnt)); } - EXPECT_EQ(queue.size(), 16); - EXPECT_EQ(free_list.size(), 0); - - EXPECT_NE(queue.at(-1), nullptr); - EXPECT_EQ(queue.at(-1), queue.at(queue.size() - 1)); - - EXPECT_EQ(queue.at(queue.size()), nullptr); + EXPECT_EQ(queue.size(), kMaxQNodes); + EXPECT_EQ(&(queue.at(-1)->get()), &(queue.at(queue.size() - 1)->get())); - for (int i = 0; i < 16; ++i) { - for (int j = 0; j < 16; ++j) { + for (size_t i = 0; i < kMaxQNodes; ++i) { + for (size_t j = 0; j < kMaxQNodes; ++j) { if (i == j) { - EXPECT_EQ(queue.at(i), queue.at(j)); + EXPECT_EQ(&(queue.at(i)->get()), &(queue.at(j)->get())); } else { - EXPECT_NE(queue.at(i), queue.at(j)); + EXPECT_NE(&(queue.at(i)->get()), &(queue.at(j)->get())); } } } - LookaheadFilterInterpreter::QState qs; + int qnode_count = queue.size(); + EXPECT_EQ(queue.at(qnode_count), std::nullopt); + EXPECT_NE(queue.at(qnode_count - 1), std::nullopt); + EXPECT_EQ(queue.at(-qnode_count - 1), std::nullopt); + EXPECT_NE(queue.at(-qnode_count), std::nullopt); } } // namespace gestures -- cgit v1.2.3 From 852c64501bd7a6162d02f48f0faac7fef5080718 Mon Sep 17 00:00:00 2001 From: Denis Brockus Date: Mon, 20 Mar 2023 10:25:52 -0600 Subject: Add ListAt to the gesture utilities BUG=b:271591258 TEST=USE="coverage" FEATURES="test noclean" emerge-brya chromeos-base/gestures Change-Id: Ie95a888ddccb16255db75f3e99436896ef367016 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/gestures/+/4352449 Code-Coverage: Harry Cutts Auto-Submit: Denis Brockus Commit-Queue: Harry Cutts Reviewed-by: Harry Cutts Tested-by: Denis Brockus --- include/util.h | 22 ++++++++++++++++++++++ src/util_unittest.cc | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/include/util.h b/include/util.h index 2e87a7b..9feb2d5 100644 --- a/include/util.h +++ b/include/util.h @@ -5,7 +5,9 @@ #ifndef GESTURES_UTIL_H_ #define GESTURES_UTIL_H_ +#include #include +#include #include #include @@ -105,6 +107,26 @@ inline bool SetContainsValue(const Set& the_set, return the_set.find(elt) != the_set.end(); } +template +R ListAt(L& the_list, int offset) { + // Traverse to the appropriate offset + if (offset < 0) { + // negative offset is from end to begin + for (auto iter = the_list.rbegin(); iter != the_list.rend(); ++iter) { + if (++offset == 0) + return R(*iter); + } + } else { + // positive offset is from begin to end + for (auto iter = the_list.begin(); iter != the_list.end(); ++iter) { + if (offset-- == 0) + return R(*iter); + } + } + // Invalid offset + return std::nullopt; +} + } // namespace gestures #endif // GESTURES_UTIL_H_ diff --git a/src/util_unittest.cc b/src/util_unittest.cc index 5d0a435..9784d70 100644 --- a/src/util_unittest.cc +++ b/src/util_unittest.cc @@ -21,4 +21,44 @@ TEST(UtilTest, DistSqTest) { EXPECT_FLOAT_EQ(DistSqXY(fs[0], 4, 6), 25); } +TEST(UtilTest, ListAtTest) { + const int kMaxElements = 3; + struct element { + int x; + }; + + typedef std::list ElemListType; + typedef std::optional> OptionalRefElem; + + struct ElemList : public ElemListType { + OptionalRefElem at(int offset) { + return ListAt(*this, offset); + } + } list; + + for (auto i = 0; i < kMaxElements; ++i) { + auto& elem = list.emplace_back(); + elem.x = i; + } + EXPECT_EQ(list.at(-1)->get().x, list.at(list.size() - 1)->get().x); + + for (auto i = 0; i < kMaxElements; ++i) { + for (auto j = 0; j < kMaxElements; ++j) { + if (i == j) { + EXPECT_EQ(list.at(i)->get().x, list.at(j)->get().x); + EXPECT_EQ(&(list.at(i)->get()), &(list.at(j)->get())); + } else { + EXPECT_NE(list.at(i)->get().x, list.at(j)->get().x); + EXPECT_NE(&(list.at(i)->get()), &(list.at(j)->get())); + } + } + } + + int list_count = list.size(); + EXPECT_EQ(list.at(list_count), std::nullopt); + EXPECT_NE(list.at(list_count - 1), std::nullopt); + EXPECT_EQ(list.at(-list_count - 1), std::nullopt); + EXPECT_NE(list.at(-list_count), std::nullopt); +} + } // namespace gestures -- cgit v1.2.3 From 1e3890f8c3637a4d61f60d660462e0fd9a29f716 Mon Sep 17 00:00:00 2001 From: Denis Brockus Date: Mon, 20 Mar 2023 10:26:49 -0600 Subject: Move Lookahead list.at() processing to common utilities BUG=b:271591258 TEST=USE="coverage" FEATURES="test noclean" emerge-brya chromeos-base/gestures Change-Id: Ief01adbe704e69704efad3af241b7a1b88d3e220 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/gestures/+/4352450 Reviewed-by: Harry Cutts Auto-Submit: Denis Brockus Tested-by: Denis Brockus Code-Coverage: Harry Cutts Commit-Queue: Harry Cutts --- include/lookahead_filter_interpreter.h | 16 ++++---- src/lookahead_filter_interpreter.cc | 20 ---------- src/lookahead_filter_interpreter_unittest.cc | 56 ---------------------------- 3 files changed, 7 insertions(+), 85 deletions(-) diff --git a/include/lookahead_filter_interpreter.h b/include/lookahead_filter_interpreter.h index 890919f..d3d4077 100644 --- a/include/lookahead_filter_interpreter.h +++ b/include/lookahead_filter_interpreter.h @@ -15,6 +15,7 @@ #include "include/gestures.h" #include "include/prop_registry.h" #include "include/tracer.h" +#include "include/util.h" #ifndef GESTURES_LOOKAHEAD_FILTER_INTERPRETER_H_ #define GESTURES_LOOKAHEAD_FILTER_INTERPRETER_H_ @@ -34,7 +35,6 @@ class LookaheadFilterInterpreter : public FilterInterpreter { FRIEND_TEST(LookaheadFilterInterpreterTest, SimpleTest); FRIEND_TEST(LookaheadFilterInterpreterTest, SpuriousCallbackTest); FRIEND_TEST(LookaheadFilterInterpreterTest, VariableDelayTest); - FRIEND_TEST(LookaheadFilterInterpreterTest, QStateListTest); public: LookaheadFilterInterpreter(PropRegistry* prop_reg, Interpreter* next, @@ -108,15 +108,13 @@ class LookaheadFilterInterpreter : public FilterInterpreter { stime_t ExtraVariableDelay() const; - typedef std::optional< - std::reference_wrapper< - LookaheadFilterInterpreter::QState - > - > OptionalRefQState; + typedef std::list QStateListType; + typedef std::optional> OptionalRefQState; - class QStateList : public std::list { - public: - OptionalRefQState at(int offset); + struct QStateList : public QStateListType { + OptionalRefQState at(int offset) { + return ListAt(*this, offset); + } } queue_; // The last id assigned to a contact (part of drumroll suppression) diff --git a/src/lookahead_filter_interpreter.cc b/src/lookahead_filter_interpreter.cc index 730becf..69156f5 100644 --- a/src/lookahead_filter_interpreter.cc +++ b/src/lookahead_filter_interpreter.cc @@ -587,24 +587,4 @@ void LookaheadFilterInterpreter::QState::set_state( state_.msc_timestamp = new_state.msc_timestamp; } -LookaheadFilterInterpreter::OptionalRefQState -LookaheadFilterInterpreter::QStateList::at(int offset) { - // Traverse to the appropriate offset - if (offset < 0) { - // negative offset is from end to begin - for (auto iter = rbegin(); iter != rend(); ++iter) { - if (++offset == 0) - return LookaheadFilterInterpreter::OptionalRefQState{*iter}; - } - } else { - // positive offset is from begin to end - for (auto iter = begin(); iter != end(); ++iter, --offset) { - if (offset == 0) - return LookaheadFilterInterpreter::OptionalRefQState{*iter}; - } - } - // Invalid offset - return std::nullopt; -} - } // namespace gestures diff --git a/src/lookahead_filter_interpreter_unittest.cc b/src/lookahead_filter_interpreter_unittest.cc index e4d02a9..ee99a0f 100644 --- a/src/lookahead_filter_interpreter_unittest.cc +++ b/src/lookahead_filter_interpreter_unittest.cc @@ -1276,60 +1276,4 @@ TEST(LookaheadFilterInterpreterTest, SemiMtNoTrackingIdAssignmentTest) { } } -TEST(LookaheadFilterInterpreterTest, QStateListTest) { - const size_t kMaxQNodes = 4; - - LookaheadFilterInterpreterTestInterpreter* base_interpreter = NULL; - std::unique_ptr interpreter; - - HardwareProperties hwprops = { - 0, 0, 100, 100, // left, top, right, bottom - 1, // x res (pixels/mm) - 1, // y res (pixels/mm) - 25, 25, // scrn DPI X, Y - -1, // orientation minimum - 2, // orientation maximum - 2, 5, // max fingers, max_touch - 1, 1, 0, // t5r2, semi, button pad - 0, 0, // has wheel, vertical wheel is high resolution - 0, // haptic pad - }; - TestInterpreterWrapper wrapper(interpreter.get(), &hwprops); - - base_interpreter = new LookaheadFilterInterpreterTestInterpreter; - interpreter.reset(new LookaheadFilterInterpreter( - NULL, base_interpreter, NULL)); - wrapper.Reset(interpreter.get()); - - auto& queue = interpreter->queue_; - - EXPECT_TRUE(queue.empty()); - EXPECT_EQ(queue.size(), 0); - - // Use the default constructor one time... just for coverage - queue.push_front(LookaheadFilterInterpreter::QState()); - - for (size_t i{1}; i < kMaxQNodes; ++i) { - queue.push_front( - LookaheadFilterInterpreter::QState(hwprops.max_finger_cnt)); - } - EXPECT_EQ(queue.size(), kMaxQNodes); - EXPECT_EQ(&(queue.at(-1)->get()), &(queue.at(queue.size() - 1)->get())); - - for (size_t i = 0; i < kMaxQNodes; ++i) { - for (size_t j = 0; j < kMaxQNodes; ++j) { - if (i == j) { - EXPECT_EQ(&(queue.at(i)->get()), &(queue.at(j)->get())); - } else { - EXPECT_NE(&(queue.at(i)->get()), &(queue.at(j)->get())); - } - } - } - - int qnode_count = queue.size(); - EXPECT_EQ(queue.at(qnode_count), std::nullopt); - EXPECT_NE(queue.at(qnode_count - 1), std::nullopt); - EXPECT_EQ(queue.at(-qnode_count - 1), std::nullopt); - EXPECT_NE(queue.at(-qnode_count), std::nullopt); -} } // namespace gestures -- cgit v1.2.3 From 1737b251d7e22532158941caa45c9b11fdc5e99c Mon Sep 17 00:00:00 2001 From: Denis Brockus Date: Fri, 17 Mar 2023 11:18:41 -0600 Subject: metrics_filter_interpreter: remove the use of MemoryManagedList Remove all references of MemoryManager Remove all references of MemoryManagedList Follow the mechanism in LookaheadFilterInterpreter crrev/c/4334918 BUG=b:271591258 TEST=USE="coverage" FEATURES="test noclean" emerge-brya chromeos-base/gestures Change-Id: Ia2ca7ef7db0d44f880b969a6e0babf92c508f244 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/gestures/+/4348019 Commit-Queue: Harry Cutts Auto-Submit: Denis Brockus Tested-by: Denis Brockus Reviewed-by: Harry Cutts Code-Coverage: Harry Cutts --- include/metrics_filter_interpreter.h | 41 +++++++++----------- src/metrics_filter_interpreter.cc | 74 +++++++++++------------------------- 2 files changed, 41 insertions(+), 74 deletions(-) diff --git a/include/metrics_filter_interpreter.h b/include/metrics_filter_interpreter.h index 60d5898..b58aabf 100644 --- a/include/metrics_filter_interpreter.h +++ b/include/metrics_filter_interpreter.h @@ -2,16 +2,17 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include #include // for FRIEND_TEST +#include +#include +#include #include "include/filter_interpreter.h" #include "include/finger_metrics.h" #include "include/gestures.h" -#include "include/list.h" -#include "include/memory_manager.h" #include "include/prop_registry.h" #include "include/tracer.h" +#include "include/util.h" #ifndef GESTURES_METRICS_FILTER_INTERPRETER_H_ #define GESTURES_METRICS_FILTER_INTERPRETER_H_ @@ -43,31 +44,29 @@ class MetricsFilterInterpreter : public FilterInterpreter { template struct State { State() {} - State(const DataType& fs, const HardwareState& hwstate) { - Init(fs, hwstate); - } - - void Init(const DataType& fs, const HardwareState& hwstate) { - timestamp = hwstate.timestamp; - data = fs; - } + State(const DataType& fs, const HardwareState& hwstate) + : timestamp(hwstate.timestamp), data(fs) {} static size_t MaxHistorySize() { return kHistorySize; } stime_t timestamp; DataType data; - State* next_; - State* prev_; }; // struct for one finger's data of one frame. typedef State MState; - typedef MemoryManagedList FingerHistory; + typedef std::list MStateListType; + typedef std::optional> OptionalRefMState; + + struct FingerHistory : public MStateListType { + OptionalRefMState at(int offset) { + return ListAt(*this, offset); + } + }; // Push the new data into the buffer. - template - void AddNewStateToBuffer(MemoryManagedList* history, - const DataType& data, + void AddNewStateToBuffer(FingerHistory& history, + const FingerState& data, const HardwareState& hwstate); // Update the class with new finger data, check if there is any interesting @@ -75,7 +74,7 @@ class MetricsFilterInterpreter : public FilterInterpreter { void UpdateFingerState(const HardwareState& hwstate); // Detect the noisy ground pattern and send GestureMetrics - bool DetectNoisyGround(const FingerHistory* history); + bool DetectNoisyGround(FingerHistory& history); // Update the class with new mouse movement data. void UpdateMouseMovementState(const HardwareState& hwstate); @@ -83,12 +82,8 @@ class MetricsFilterInterpreter : public FilterInterpreter { // Compute interested statistics for the mouse history, send GestureMetrics. void ReportMouseStatistics(); - // memory managers to prevent malloc during interrupt calls - MemoryManager mstate_mm_; - MemoryManager history_mm_; - // A map to store each finger's past data - typedef std::map FingerHistoryMap; + typedef std::map FingerHistoryMap; FingerHistoryMap histories_; // Device class (e.g. touchpad, mouse). diff --git a/src/metrics_filter_interpreter.cc b/src/metrics_filter_interpreter.cc index de6ca33..39c9766 100644 --- a/src/metrics_filter_interpreter.cc +++ b/src/metrics_filter_interpreter.cc @@ -22,8 +22,6 @@ MetricsFilterInterpreter::MetricsFilterInterpreter( Tracer* tracer, GestureInterpreterDeviceClass devclass) : FilterInterpreter(NULL, next, tracer, false), - mstate_mm_(kMaxFingers * MState::MaxHistorySize()), - history_mm_(kMaxFingers), devclass_(devclass), mouse_movement_session_index_(0), mouse_movement_current_session_length(0), @@ -44,7 +42,7 @@ MetricsFilterInterpreter::MetricsFilterInterpreter( } void MetricsFilterInterpreter::SyncInterpretImpl(HardwareState* hwstate, - stime_t* timeout) { + stime_t* timeout) { if (devclass_ == GESTURES_DEVCLASS_TOUCHPAD) { // Right now, we only want to update finger states for built-in touchpads // because all the generated metrics gestures would be put under each @@ -66,22 +64,16 @@ void MetricsFilterInterpreter::SyncInterpretImpl(HardwareState* hwstate, next_->SyncInterpret(hwstate, timeout); } -template void MetricsFilterInterpreter::AddNewStateToBuffer( - MemoryManagedList* history, - const DataType& data, + FingerHistory& history, + const FingerState& data, const HardwareState& hwstate) { // The history buffer is already full, pop one - if (history->size() == StateType::MaxHistorySize()) - history->DeleteFront(); + if (history.size() == MState::MaxHistorySize()) + history.pop_front(); // Push the new finger state to the back of buffer - StateType* current = history->PushNewEltBack(); - if (!current) { - Err("MetricsFilterInterpreter buffer out of space"); - return; - } - current->Init(data, hwstate); + (void)history.emplace_back(data, hwstate); } void MetricsFilterInterpreter::UpdateMouseMovementState( @@ -138,68 +130,48 @@ void MetricsFilterInterpreter::ReportMouseStatistics() { void MetricsFilterInterpreter::UpdateFingerState( const HardwareState& hwstate) { - FingerHistoryMap removed; - RemoveMissingIdsFromMap(&histories_, hwstate, &removed); - for (FingerHistoryMap::const_iterator it = - removed.begin(); it != removed.end(); ++it) { - it->second->clear(); - history_mm_.Free(it->second); -} + RemoveMissingIdsFromMap(&histories_, hwstate); FingerState *fs = hwstate.fingers; for (short i = 0; i < hwstate.finger_cnt; i++) { - FingerHistory* hp; - // Update the map if the contact is new if (!MapContainsKey(histories_, fs[i].tracking_id)) { - hp = history_mm_.Allocate(); - if (!hp) { - Err("FingerHistory out of space"); - continue; - } - hp->Init(&mstate_mm_); - histories_[fs[i].tracking_id] = hp; - } else { - hp = histories_[fs[i].tracking_id]; + histories_[fs[i].tracking_id] = FingerHistory{}; } + auto& href = histories_[fs[i].tracking_id]; // Check if the finger history contains interesting patterns - AddNewStateToBuffer(hp, fs[i], hwstate); - DetectNoisyGround(hp); + AddNewStateToBuffer(href, fs[i], hwstate); + DetectNoisyGround(href); } } -bool MetricsFilterInterpreter::DetectNoisyGround( - const FingerHistory* history) { - auto iter = history->end(); - MState* current = *(--iter); - size_t n_samples = history->size(); +bool MetricsFilterInterpreter::DetectNoisyGround(FingerHistory& history) { // Noise pattern takes 3 samples - if (n_samples < 3) + if (history.size() < 3) return false; - MState* past_1 = *(--iter); - MState* past_2 = *(--iter); + auto current = history.at(-1)->get(); + auto past_1 = history.at(-2)->get(); + auto past_2 = history.at(-3)->get(); // Noise pattern needs to happen in a short period of time - if(current->timestamp - past_2->timestamp > - noisy_ground_time_threshold_.val_) { + if (current.timestamp - past_2.timestamp > noisy_ground_time_threshold_.val_) return false; - } // vec[when][x,y] float vec[2][2]; - vec[0][0] = current->data.position_x - past_1->data.position_x; - vec[0][1] = current->data.position_y - past_1->data.position_y; - vec[1][0] = past_1->data.position_x - past_2->data.position_x; - vec[1][1] = past_1->data.position_y - past_2->data.position_y; + vec[0][0] = current.data.position_x - past_1.data.position_x; + vec[0][1] = current.data.position_y - past_1.data.position_y; + vec[1][0] = past_1.data.position_x - past_2.data.position_x; + vec[1][1] = past_1.data.position_y - past_2.data.position_y; const float thr = noisy_ground_distance_threshold_.val_; // We dictate the noise pattern as two consecutive big moves in // opposite directions in either X or Y for (size_t i = 0; i < arraysize(vec[0]); i++) if ((vec[0][i] < -thr && vec[1][i] > thr) || (vec[0][i] > thr && vec[1][i] < -thr)) { - ProduceGesture(Gesture(kGestureMetrics, past_2->timestamp, - current->timestamp, kGestureMetricsTypeNoisyGround, + ProduceGesture(Gesture(kGestureMetrics, past_2.timestamp, + current.timestamp, kGestureMetricsTypeNoisyGround, vec[0][i], vec[1][i])); return true; } -- cgit v1.2.3 From 930aa64c667122525a3e3cdaf9f0d03518a73c39 Mon Sep 17 00:00:00 2001 From: Gwendal Grignou Date: Wed, 1 Mar 2023 13:51:15 -0800 Subject: tools: Use json instead of simplejson python we are using have json built-in. Use it to not depend on simplejson package. BUG=b:187794810 TEST=Run unit test, but fails at: "/mnt/host/source/src/platform/mttools/mtreplay/replay: error while loading shared libraries: libevdev_hollow.so.0: cannot open shared object file: No such file or directory" Change-Id: I27f034d10e24814f0d2e1ef3fdb15d19a25d5a6c Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/gestures/+/4300256 Code-Coverage: Gwendal Grignou Reviewed-by: Jack Rosenthal Commit-Queue: Gwendal Grignou Reviewed-by: Sean O'Brien Tested-by: Gwendal Grignou Code-Coverage: Sean O'Brien --- tools/tplog.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/tplog.py b/tools/tplog.py index 3ede9b7..7df13f8 100755 --- a/tools/tplog.py +++ b/tools/tplog.py @@ -8,9 +8,9 @@ import getopt +import json import logging import os -import simplejson as json import sys from operator import ge, lt -- cgit v1.2.3 From d434c1bc882f2627f37327f3805bc3cfbc2df37f Mon Sep 17 00:00:00 2001 From: Denis Brockus Date: Mon, 20 Mar 2023 14:01:39 -0600 Subject: trend_classifying_filter_interpreter: remove MemoryManagedList Remove all references of MemoryManager Remove all references of MemoryManagedList Follow the mechanism in LookaheadFilterInterpreter crrev/c/4334918 BUG=b:271591258 TEST=USE="coverage" FEATURES="test noclean" emerge-brya chromeos-base/gestures Change-Id: Ie3774ec0596e96571617b2775ced719b639dd536 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/gestures/+/4357692 Code-Coverage: Henry Barnor Auto-Submit: Denis Brockus Commit-Queue: Denis Brockus Reviewed-by: Harry Cutts Tested-by: Denis Brockus --- include/trend_classifying_filter_interpreter.h | 26 ++++++----- src/trend_classifying_filter_interpreter.cc | 64 +++++++++----------------- 2 files changed, 35 insertions(+), 55 deletions(-) diff --git a/include/trend_classifying_filter_interpreter.h b/include/trend_classifying_filter_interpreter.h index b2e8436..1de25cd 100644 --- a/include/trend_classifying_filter_interpreter.h +++ b/include/trend_classifying_filter_interpreter.h @@ -2,17 +2,18 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include // for FRIEND_TEST +#include #include +#include #include -#include // for FRIEND_TEST #include "include/filter_interpreter.h" #include "include/finger_metrics.h" #include "include/gestures.h" -#include "include/list.h" -#include "include/memory_manager.h" #include "include/prop_registry.h" #include "include/tracer.h" +#include "include/util.h" #ifndef GESTURES_TREND_CLASSIFYING_FILTER_INTERPRETER_H_ #define GESTURES_TREND_CLASSIFYING_FILTER_INTERPRETER_H_ @@ -151,12 +152,17 @@ private: GESTURES_FINGER_TREND_DEC_TOUCH_MAJOR }; return flags[idx]; } + }; - KState* next_; - KState* prev_; + typedef std::list KStateListType; + typedef std::optional> OptionalRefKState; + + struct FingerHistory : public KStateListType { + OptionalRefKState at(int offset) { + return ListAt(*this, offset); + } }; - typedef MemoryManagedList FingerHistory; // Trend types for internal use enum TrendType { @@ -169,7 +175,7 @@ private: void UpdateFingerState(const HardwareState& hwstate); // Push new finger data into the buffer and update values - void AddNewStateToBuffer(FingerHistory* history, const FingerState& fs); + void AddNewStateToBuffer(FingerHistory& history, const FingerState& fs); // Assess statistical significance with a classic two-tail hypothesis test TrendType RunKTTest(const KState::KAxis* current, const size_t n_samples); @@ -220,13 +226,9 @@ private: const unsigned flag_decreasing, unsigned* flags); - // memory managers to prevent malloc during interrupt calls - MemoryManager kstate_mm_; - MemoryManager history_mm_; - // A map to store each finger's past coordinates and calculation // intermediates - typedef std::map FingerHistoryMap; + typedef std::map FingerHistoryMap; FingerHistoryMap histories_; // Flag to turn on/off the trend classifying filter diff --git a/src/trend_classifying_filter_interpreter.cc b/src/trend_classifying_filter_interpreter.cc index d6ddd21..2bfa156 100644 --- a/src/trend_classifying_filter_interpreter.cc +++ b/src/trend_classifying_filter_interpreter.cc @@ -30,8 +30,6 @@ namespace gestures { TrendClassifyingFilterInterpreter::TrendClassifyingFilterInterpreter( PropRegistry* prop_reg, Interpreter* next, Tracer* tracer) : FilterInterpreter(NULL, next, tracer, false), - kstate_mm_(kMaxFingers * kNumOfSamples), - history_mm_(kMaxFingers), trend_classifying_filter_enable_( prop_reg, "Trend Classifying Filter Enabled", true), second_order_enable_( @@ -72,37 +70,32 @@ void TrendClassifyingFilterInterpreter::InterpretTestResult( } void TrendClassifyingFilterInterpreter::AddNewStateToBuffer( - FingerHistory* history, const FingerState& fs) { + FingerHistory& history, const FingerState& fs) { // The history buffer is already full, pop one - if (history->size() == static_cast(num_of_samples_.val_)) - history->DeleteFront(); + if (history.size() == static_cast(num_of_samples_.val_)) + history.pop_front(); // Push the new finger state to the back of buffer - KState* previous_end = history->back(); - KState* current = history->PushNewEltBack(); - if (!current) { - Err("KState buffer out of space"); - return; - } - current->Init(fs); - if (history->size() == 1) + auto& current = history.emplace_back(fs); + if (history.size() == 1) return; + auto& previous_end = history.at(-2)->get(); - current->DxAxis()->val = current->XAxis()->val - previous_end->XAxis()->val; - current->DyAxis()->val = current->YAxis()->val - previous_end->YAxis()->val; + current.DxAxis()->val = current.XAxis()->val - previous_end.XAxis()->val; + current.DyAxis()->val = current.YAxis()->val - previous_end.YAxis()->val; // Update the nodes already in the buffer and compute the Kendall score/ // variance along the way. Complexity is O(|buffer|) per finger. int tie_n2[KState::n_axes_] = { 0, 0, 0, 0, 0, 0 }; int tie_n3[KState::n_axes_] = { 0, 0, 0, 0, 0, 0 }; - for (auto it = history->begin(); it != history->end(); ++it) + for (auto it = history.begin(); it != history.end(); ++it) for (size_t i = 0; i < KState::n_axes_; i++) - if (it != history->begin() || !KState::IsDelta(i)) { - UpdateKTValuePair(&(*it)->axes_[i], ¤t->axes_[i], + if (it != history.begin() || !KState::IsDelta(i)) { + UpdateKTValuePair(&it->axes_[i], ¤t.axes_[i], &tie_n2[i], &tie_n3[i]); } - size_t n_samples = history->size(); + size_t n_samples = history.size(); for (size_t i = 0; i < KState::n_axes_; i++) { - current->axes_[i].var = ComputeKTVariance(tie_n2[i], tie_n3[i], + current.axes_[i].var = ComputeKTVariance(tie_n2[i], tie_n3[i], KState::IsDelta(i) ? n_samples - 1 : n_samples); } } @@ -132,38 +125,23 @@ TrendClassifyingFilterInterpreter::RunKTTest(const KState::KAxis* current, void TrendClassifyingFilterInterpreter::UpdateFingerState( const HardwareState& hwstate) { - FingerHistoryMap removed; - RemoveMissingIdsFromMap(&histories_, hwstate, &removed); - for (FingerHistoryMap::const_iterator it = - removed.begin(); it != removed.end(); ++it) { - it->second->clear(); - history_mm_.Free(it->second); - } + RemoveMissingIdsFromMap(&histories_, hwstate); - FingerState *fs = hwstate.fingers; + FingerState* fs = hwstate.fingers; for (short i = 0; i < hwstate.finger_cnt; i++) { - FingerHistory* hp; - // Update the map if the contact is new if (!MapContainsKey(histories_, fs[i].tracking_id)) { - hp = history_mm_.Allocate(); - if (!hp) { - Err("FingerHistory out of space"); - continue; - } - hp->Init(&kstate_mm_); - histories_[fs[i].tracking_id] = hp; - } else { - hp = histories_[fs[i].tracking_id]; + histories_[fs[i].tracking_id] = FingerHistory{}; } + auto& history = histories_[fs[i].tracking_id]; // Check if the score demonstrates statistical significance - AddNewStateToBuffer(hp, fs[i]); - KState* current = hp->back(); - size_t n_samples = hp->size(); + AddNewStateToBuffer(history, fs[i]); + const auto& current = history.back(); + const size_t n_samples = history.size(); for (size_t idx = 0; idx < KState::n_axes_; idx++) if (second_order_enable_.val_ || !KState::IsDelta(idx)) { - TrendType result = RunKTTest(¤t->axes_[idx], + TrendType result = RunKTTest(¤t.axes_[idx], KState::IsDelta(idx) ? n_samples - 1 : n_samples); InterpretTestResult(result, KState::IncFlag(idx), KState::DecFlag(idx), &(fs[i].flags)); -- cgit v1.2.3 From 1dcfdc359ab94ad03b9ee4bd6e1cb88f1ef2a009 Mon Sep 17 00:00:00 2001 From: Denis Brockus Date: Mon, 20 Mar 2023 14:02:41 -0600 Subject: Removal cleanup of MemoryManagedList Remove MemoryManager Remove MemoryManagedList Remove utils RemoveMissingIdsFromMap that passes removed map BUG=b:271591258 TEST=USE="coverage" FEATURES="test noclean" emerge-brya chromeos-base/gestures Change-Id: I30686b6e353327066ddc4c8ef496fd51ef6a36dc Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/gestures/+/4357693 Auto-Submit: Denis Brockus Tested-by: Denis Brockus Reviewed-by: Harry Cutts Commit-Queue: Denis Brockus Code-Coverage: Henry Barnor --- include/list.h | 70 ------------------------------------------ include/memory_manager.h | 79 ------------------------------------------------ include/util.h | 24 ++++----------- 3 files changed, 6 insertions(+), 167 deletions(-) delete mode 100644 include/list.h delete mode 100644 include/memory_manager.h diff --git a/include/list.h b/include/list.h deleted file mode 100644 index c822e7a..0000000 --- a/include/list.h +++ /dev/null @@ -1,70 +0,0 @@ -// Copyright 2011 The ChromiumOS Authors -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef GESTURES_LIST_H__ -#define GESTURES_LIST_H__ - -#include -#include "include/logging.h" -#include "include/memory_manager.h" - -namespace gestures { - -template -class MemoryManagedList : public std::list { - public: - MemoryManagedList() { }; - ~MemoryManagedList() { clear(); } - - void Init(MemoryManager* memory_manager) { - memory_manager_ = memory_manager; - } - - Elt* PushNewEltBack() { - AssertWithReturnValue(memory_manager_, NULL); - Elt* elt = NewElt(); - AssertWithReturnValue(elt, NULL); - this->push_back(elt); - return elt; - } - - Elt* PushNewEltFront() { - AssertWithReturnValue(memory_manager_, NULL); - Elt* elt = NewElt(); - AssertWithReturnValue(elt, NULL); - this->push_front(elt); - return elt; - } - - void DeleteFront() { - AssertWithReturn(memory_manager_); - memory_manager_->Free(this->front()); - this->pop_front(); - } - - void DeleteBack() { - AssertWithReturn(memory_manager_); - memory_manager_->Free(this->pop_back()); - } - - void clear() { - while (!this->empty()) - DeleteFront(); - } - - private: - MemoryManager* memory_manager_; - - Elt* NewElt() { - Elt* elt = memory_manager_->Allocate(); - AssertWithReturnValue(elt, NULL); - elt->next_ = elt->prev_ = NULL; - return elt; - } -}; - - -} // namespace gestures - -#endif // GESTURES_LIST_H__ diff --git a/include/memory_manager.h b/include/memory_manager.h deleted file mode 100644 index 26e096a..0000000 --- a/include/memory_manager.h +++ /dev/null @@ -1,79 +0,0 @@ -// Copyright 2011 The ChromiumOS Authors -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef GESTURES_MEMORY_MANAGER_H_ -#define GESTURES_MEMORY_MANAGER_H_ - -#include - -#include "include/logging.h" -#include "include/macros.h" - -namespace gestures { - -// A simple memory manager class that pre-allocates a size of -// buffer and garbage collects freed variables. It is not intended -// to be used directly. User classes should inherit the -// MemoryManaged wrapper class and use the provided Allocate/Free -// interface instead (see below). - -template -class MemoryManager { - public: - explicit MemoryManager(size_t size) : buf_(new T[size]), - free_slots_(new T *[size]), used_mark_(new bool[size]()), - max_size_(size), head_(size) { - for (size_t i = 0; i < max_size_; i++) - free_slots_[i] = buf_.get() + i; - } - - size_t Size() const { return max_size_ - head_; } - size_t MaxSize() const { return max_size_; } - - T* Allocate() { - if (!head_) { - Err("MemoryManager::Allocate: out of space"); - return NULL; - } - - T* ptr = free_slots_[--head_]; - used_mark_[ptr - buf_.get()] = true; - return ptr; - } - - bool Free(T* ptr) { - // Check for invalid pointer and double-free - if (ptr < buf_.get() || ptr >= buf_.get() + max_size_) { - Err("MemoryManager::Free: pointer out of bounds"); - return false; - } - size_t offset_in_bytes = reinterpret_cast(ptr) - - reinterpret_cast(buf_.get()); - if (offset_in_bytes % sizeof(T)) { - Err("MemoryManager::Free: unaligned pointer"); - return false; - } - size_t offset = ptr - buf_.get(); - if (!used_mark_[offset]) { - Err("MemoryManager::Free: double-free"); - return false; - } - - free_slots_[head_++] = ptr; - used_mark_[offset] = false; - return true; - } - - private: - std::unique_ptr buf_; - std::unique_ptr free_slots_; - std::unique_ptr used_mark_; - size_t max_size_; - size_t head_; - DISALLOW_COPY_AND_ASSIGN(MemoryManager); -}; - -} // namespace gestures - -#endif // MEMORY_MANAGER_H_ diff --git a/include/util.h b/include/util.h index 9feb2d5..0e1d89a 100644 --- a/include/util.h +++ b/include/util.h @@ -62,29 +62,17 @@ bool MapContainsKey(const Map& the_map, const Key& the_key) { return the_map.find(the_key) != the_map.end(); } -// Removes any ids from the map that are not finger ids in hs. -// This implementation supports returning removed elements for -// further processing. -template -void RemoveMissingIdsFromMap(std::map* the_map, - const HardwareState& hs, - std::map* removed) { - removed->clear(); - for (typename std::map::const_iterator it = - the_map->begin(); it != the_map->end(); ++it) - if (!hs.GetFingerState((*it).first)) - (*removed)[it->first] = it->second; - for (typename std::map::const_iterator it = - removed->begin(); it != removed->end(); ++it) - the_map->erase(it->first); -} - // Removes any ids from the map that are not finger ids in hs. template void RemoveMissingIdsFromMap(std::map* the_map, const HardwareState& hs) { std::map removed; - RemoveMissingIdsFromMap(the_map, hs, &removed); + for (const auto& [key, value] : *the_map) { + if (!hs.GetFingerState(key)) + removed[key] = value; + } + for (const auto& [key, value] : removed) + the_map->erase(key); } // Removes any ids from the set that are not finger ids in hs. -- cgit v1.2.3 From e5d5bde929391e62e82261f29bd68ec39f02a01e Mon Sep 17 00:00:00 2001 From: Denis Brockus Date: Tue, 21 Mar 2023 10:24:48 -0600 Subject: Remove std::optional> from .at() There was a desire to move to use either a pointer or a normal reference. The original .at() returned a normal reference but did not allow for offsets that were out of bounds. I went back to this original and used an abort for the invalid offset. I added a death test in util_unittest to verify that was working. BUG=b:271591258 TEST=USE="coverage" FEATURES="test noclean" emerge-brya chromeos-base/gestures Change-Id: Ie6a6716f23d5c8516dab7801098e7f4d60b00bee Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/gestures/+/4357694 Reviewed-by: Harry Cutts Code-Coverage: Henry Barnor Auto-Submit: Denis Brockus Tested-by: Denis Brockus Commit-Queue: Denis Brockus --- include/lookahead_filter_interpreter.h | 10 ++--- include/metrics_filter_interpreter.h | 9 ++--- include/trend_classifying_filter_interpreter.h | 11 ++---- include/util.h | 11 +++--- src/lookahead_filter_interpreter.cc | 12 +++--- src/lookahead_filter_interpreter_unittest.cc | 6 +-- src/metrics_filter_interpreter.cc | 6 +-- src/trend_classifying_filter_interpreter.cc | 2 +- src/util_unittest.cc | 53 ++++++++++++++++++-------- 9 files changed, 64 insertions(+), 56 deletions(-) diff --git a/include/lookahead_filter_interpreter.h b/include/lookahead_filter_interpreter.h index d3d4077..c37f170 100644 --- a/include/lookahead_filter_interpreter.h +++ b/include/lookahead_filter_interpreter.h @@ -6,7 +6,6 @@ #include #include #include -#include #include // For FRIEND_TEST @@ -108,12 +107,9 @@ class LookaheadFilterInterpreter : public FilterInterpreter { stime_t ExtraVariableDelay() const; - typedef std::list QStateListType; - typedef std::optional> OptionalRefQState; - - struct QStateList : public QStateListType { - OptionalRefQState at(int offset) { - return ListAt(*this, offset); + struct QStateList : public std::list { + QState& at(int offset) { + return ListAt(*this, offset); } } queue_; diff --git a/include/metrics_filter_interpreter.h b/include/metrics_filter_interpreter.h index b58aabf..9dc1978 100644 --- a/include/metrics_filter_interpreter.h +++ b/include/metrics_filter_interpreter.h @@ -5,7 +5,6 @@ #include // for FRIEND_TEST #include #include -#include #include "include/filter_interpreter.h" #include "include/finger_metrics.h" @@ -55,12 +54,10 @@ class MetricsFilterInterpreter : public FilterInterpreter { // struct for one finger's data of one frame. typedef State MState; - typedef std::list MStateListType; - typedef std::optional> OptionalRefMState; - struct FingerHistory : public MStateListType { - OptionalRefMState at(int offset) { - return ListAt(*this, offset); + struct FingerHistory : public std::list { + MState& at(int offset) { + return ListAt(*this, offset); } }; diff --git a/include/trend_classifying_filter_interpreter.h b/include/trend_classifying_filter_interpreter.h index 1de25cd..6c05073 100644 --- a/include/trend_classifying_filter_interpreter.h +++ b/include/trend_classifying_filter_interpreter.h @@ -5,7 +5,6 @@ #include // for FRIEND_TEST #include #include -#include #include #include "include/filter_interpreter.h" @@ -154,16 +153,12 @@ private: } }; - typedef std::list KStateListType; - typedef std::optional> OptionalRefKState; - - struct FingerHistory : public KStateListType { - OptionalRefKState at(int offset) { - return ListAt(*this, offset); + struct FingerHistory : public std::list { + KState& at(int offset) { + return ListAt(*this, offset); } }; - // Trend types for internal use enum TrendType { TREND_NONE, diff --git a/include/util.h b/include/util.h index 0e1d89a..afaf61a 100644 --- a/include/util.h +++ b/include/util.h @@ -7,7 +7,6 @@ #include #include -#include #include #include @@ -95,24 +94,24 @@ inline bool SetContainsValue(const Set& the_set, return the_set.find(elt) != the_set.end(); } -template -R ListAt(L& the_list, int offset) { +template +Elem& ListAt(std::list& the_list, int offset) { // Traverse to the appropriate offset if (offset < 0) { // negative offset is from end to begin for (auto iter = the_list.rbegin(); iter != the_list.rend(); ++iter) { if (++offset == 0) - return R(*iter); + return *iter; } } else { // positive offset is from begin to end for (auto iter = the_list.begin(); iter != the_list.end(); ++iter) { if (offset-- == 0) - return R(*iter); + return *iter; } } // Invalid offset - return std::nullopt; + abort(); } } // namespace gestures diff --git a/src/lookahead_filter_interpreter.cc b/src/lookahead_filter_interpreter.cc index 69156f5..fea2c47 100644 --- a/src/lookahead_filter_interpreter.cc +++ b/src/lookahead_filter_interpreter.cc @@ -147,11 +147,11 @@ void LookaheadFilterInterpreter::AssignTrackingIds() { return; } - auto& tail = queue_.at(-1)->get(); + auto& tail = queue_.at(-1); HardwareState* hs = &tail.state_; - QState* prev_qs = queue_.size() < 2 ? NULL : &(queue_.at(-2)->get()); + QState* prev_qs = queue_.size() < 2 ? NULL : &(queue_.at(-2)); HardwareState* prev_hs = prev_qs ? &prev_qs->state_ : NULL; - QState* prev2_qs = queue_.size() < 3 ? NULL : &(queue_.at(-3)->get()); + QState* prev2_qs = queue_.size() < 3 ? NULL : &(queue_.at(-3)); HardwareState* prev2_hs = prev2_qs ? &prev2_qs->state_ : NULL; RemoveMissingIdsFromMap(&tail.output_ids_, *hs); @@ -352,7 +352,7 @@ void LookaheadFilterInterpreter::TapDownOccurringGesture(stime_t now) { return; // We didn't push a new hardware state now // See if latest hwstate has finger that previous doesn't. // Get the state_ of back->prev - HardwareState& prev_hs = queue_.at(-2)->get().state_; + HardwareState& prev_hs = queue_.at(-2).state_; if (hs.finger_cnt > prev_hs.finger_cnt) { // Finger was added. ProduceGesture(Gesture(kGestureFling, prev_hs.timestamp, hs.timestamp, @@ -388,8 +388,8 @@ short LookaheadFilterInterpreter::NextTrackingId() { void LookaheadFilterInterpreter::AttemptInterpolation() { if (queue_.size() < 2) return; - QState& new_node = queue_.at(-1)->get(); - QState& prev = queue_.at(-2)->get(); + QState& new_node = queue_.at(-1); + QState& prev = queue_.at(-2); if (new_node.state_.timestamp - prev.state_.timestamp < split_min_period_.val_) { return; // Nodes came in too quickly to need interpolation diff --git a/src/lookahead_filter_interpreter_unittest.cc b/src/lookahead_filter_interpreter_unittest.cc index ee99a0f..1345daf 100644 --- a/src/lookahead_filter_interpreter_unittest.cc +++ b/src/lookahead_filter_interpreter_unittest.cc @@ -821,9 +821,9 @@ TEST(LookaheadFilterInterpreterTest, QuickMoveTest) { // Expecting Quick movement detected and ID correction 5 -> 4. wrapper.SyncInterpret(&hs[6], &timeout); - EXPECT_EQ(interpreter->queue_.at(-1)->get().fs_[0].tracking_id, 4); - EXPECT_EQ(interpreter->queue_.at(-2)->get().fs_[0].tracking_id, 4); - EXPECT_EQ(interpreter->queue_.at(-3)->get().fs_[0].tracking_id, 4); + EXPECT_EQ(interpreter->queue_.at(-1).fs_[0].tracking_id, 4); + EXPECT_EQ(interpreter->queue_.at(-2).fs_[0].tracking_id, 4); + EXPECT_EQ(interpreter->queue_.at(-3).fs_[0].tracking_id, 4); } struct QuickSwipeTestInputs { diff --git a/src/metrics_filter_interpreter.cc b/src/metrics_filter_interpreter.cc index 39c9766..a19b814 100644 --- a/src/metrics_filter_interpreter.cc +++ b/src/metrics_filter_interpreter.cc @@ -151,9 +151,9 @@ bool MetricsFilterInterpreter::DetectNoisyGround(FingerHistory& history) { if (history.size() < 3) return false; - auto current = history.at(-1)->get(); - auto past_1 = history.at(-2)->get(); - auto past_2 = history.at(-3)->get(); + auto current = history.at(-1); + auto past_1 = history.at(-2); + auto past_2 = history.at(-3); // Noise pattern needs to happen in a short period of time if (current.timestamp - past_2.timestamp > noisy_ground_time_threshold_.val_) return false; diff --git a/src/trend_classifying_filter_interpreter.cc b/src/trend_classifying_filter_interpreter.cc index 2bfa156..714cb68 100644 --- a/src/trend_classifying_filter_interpreter.cc +++ b/src/trend_classifying_filter_interpreter.cc @@ -79,7 +79,7 @@ void TrendClassifyingFilterInterpreter::AddNewStateToBuffer( auto& current = history.emplace_back(fs); if (history.size() == 1) return; - auto& previous_end = history.at(-2)->get(); + auto& previous_end = history.at(-2); current.DxAxis()->val = current.XAxis()->val - previous_end.XAxis()->val; current.DyAxis()->val = current.YAxis()->val - previous_end.YAxis()->val; diff --git a/src/util_unittest.cc b/src/util_unittest.cc index 9784d70..9d6bd40 100644 --- a/src/util_unittest.cc +++ b/src/util_unittest.cc @@ -27,12 +27,9 @@ TEST(UtilTest, ListAtTest) { int x; }; - typedef std::list ElemListType; - typedef std::optional> OptionalRefElem; - - struct ElemList : public ElemListType { - OptionalRefElem at(int offset) { - return ListAt(*this, offset); + struct ElemList : public std::list { + element& at(int offset) { + return ListAt(*this, offset); } } list; @@ -40,25 +37,49 @@ TEST(UtilTest, ListAtTest) { auto& elem = list.emplace_back(); elem.x = i; } - EXPECT_EQ(list.at(-1)->get().x, list.at(list.size() - 1)->get().x); + EXPECT_EQ(list.at(-1).x, list.at(list.size() - 1).x); for (auto i = 0; i < kMaxElements; ++i) { for (auto j = 0; j < kMaxElements; ++j) { if (i == j) { - EXPECT_EQ(list.at(i)->get().x, list.at(j)->get().x); - EXPECT_EQ(&(list.at(i)->get()), &(list.at(j)->get())); + EXPECT_EQ(list.at(i).x, list.at(j).x); + EXPECT_EQ(&(list.at(i)), &(list.at(j))); } else { - EXPECT_NE(list.at(i)->get().x, list.at(j)->get().x); - EXPECT_NE(&(list.at(i)->get()), &(list.at(j)->get())); + EXPECT_NE(list.at(i).x, list.at(j).x); + EXPECT_NE(&(list.at(i)), &(list.at(j))); } } } +} + +TEST(UtilTest, ListAtDeathForwardTest) { + const int kMaxElements = 3; + + struct IntList : public std::list { + int& at(int offset) { + return ListAt(*this, offset); + } + } list; + + for (auto i = 0; i < kMaxElements; ++i) { + list.emplace_back(i); + } + EXPECT_DEATH(list.at(kMaxElements+1), ""); +} - int list_count = list.size(); - EXPECT_EQ(list.at(list_count), std::nullopt); - EXPECT_NE(list.at(list_count - 1), std::nullopt); - EXPECT_EQ(list.at(-list_count - 1), std::nullopt); - EXPECT_NE(list.at(-list_count), std::nullopt); +TEST(UtilTest, ListAtDeathBackwardTest) { + const int kMaxElements = 3; + + struct IntList : public std::list { + int& at(int offset) { + return ListAt(*this, offset); + } + } list; + + for (auto i = 0; i < kMaxElements; ++i) { + list.emplace_back(i); + } + EXPECT_DEATH(list.at(-(kMaxElements+1)), ""); } } // namespace gestures -- cgit v1.2.3 From 54cf5df0ee02c0573b302410c871f80673d55a2c Mon Sep 17 00:00:00 2001 From: Denis Brockus Date: Wed, 22 Mar 2023 13:47:37 -0600 Subject: Move list with .at into generic in util.h Instead of repeating making a class that inherits from std::list, make this gestures::List a single point of truth for this code. This is still just a std::list but adds .at() to this class. BUG=b:271591258 TEST=USE="coverage" FEATURES="test noclean" emerge-brya chromeos-base/gestures Change-Id: Id612a6501d5caabea8195b8a14c76c28dcaced5a Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/gestures/+/4361866 Code-Coverage: Henry Barnor Reviewed-by: Harry Cutts Reviewed-by: Sean O'Brien Auto-Submit: Denis Brockus Tested-by: Denis Brockus Commit-Queue: Denis Brockus --- include/lookahead_filter_interpreter.h | 6 +---- include/metrics_filter_interpreter.h | 7 +----- include/trend_classifying_filter_interpreter.h | 6 +---- include/util.h | 35 ++++++++++++++------------ src/util_unittest.cc | 20 +++------------ 5 files changed, 25 insertions(+), 49 deletions(-) diff --git a/include/lookahead_filter_interpreter.h b/include/lookahead_filter_interpreter.h index c37f170..ba2f92b 100644 --- a/include/lookahead_filter_interpreter.h +++ b/include/lookahead_filter_interpreter.h @@ -107,11 +107,7 @@ class LookaheadFilterInterpreter : public FilterInterpreter { stime_t ExtraVariableDelay() const; - struct QStateList : public std::list { - QState& at(int offset) { - return ListAt(*this, offset); - } - } queue_; + List queue_; // The last id assigned to a contact (part of drumroll suppression) short last_id_; diff --git a/include/metrics_filter_interpreter.h b/include/metrics_filter_interpreter.h index 9dc1978..816f849 100644 --- a/include/metrics_filter_interpreter.h +++ b/include/metrics_filter_interpreter.h @@ -54,12 +54,7 @@ class MetricsFilterInterpreter : public FilterInterpreter { // struct for one finger's data of one frame. typedef State MState; - - struct FingerHistory : public std::list { - MState& at(int offset) { - return ListAt(*this, offset); - } - }; + typedef List FingerHistory; // Push the new data into the buffer. void AddNewStateToBuffer(FingerHistory& history, diff --git a/include/trend_classifying_filter_interpreter.h b/include/trend_classifying_filter_interpreter.h index 6c05073..049c5e0 100644 --- a/include/trend_classifying_filter_interpreter.h +++ b/include/trend_classifying_filter_interpreter.h @@ -153,11 +153,7 @@ private: } }; - struct FingerHistory : public std::list { - KState& at(int offset) { - return ListAt(*this, offset); - } - }; + typedef List FingerHistory; // Trend types for internal use enum TrendType { diff --git a/include/util.h b/include/util.h index afaf61a..f0c2a39 100644 --- a/include/util.h +++ b/include/util.h @@ -95,24 +95,27 @@ inline bool SetContainsValue(const Set& the_set, } template -Elem& ListAt(std::list& the_list, int offset) { - // Traverse to the appropriate offset - if (offset < 0) { - // negative offset is from end to begin - for (auto iter = the_list.rbegin(); iter != the_list.rend(); ++iter) { - if (++offset == 0) - return *iter; - } - } else { - // positive offset is from begin to end - for (auto iter = the_list.begin(); iter != the_list.end(); ++iter) { - if (offset-- == 0) - return *iter; +class List : public std::list { +public: + Elem& at(int offset) { + // Traverse to the appropriate offset + if (offset < 0) { + // negative offset is from end to begin + for (auto iter = this->rbegin(); iter != this->rend(); ++iter) { + if (++offset == 0) + return *iter; + } + } else { + // positive offset is from begin to end + for (auto iter = this->begin(); iter != this->end(); ++iter) { + if (offset-- == 0) + return *iter; + } } + // Invalid offset + abort(); } - // Invalid offset - abort(); -} +}; } // namespace gestures diff --git a/src/util_unittest.cc b/src/util_unittest.cc index 9d6bd40..f19fe1f 100644 --- a/src/util_unittest.cc +++ b/src/util_unittest.cc @@ -27,11 +27,7 @@ TEST(UtilTest, ListAtTest) { int x; }; - struct ElemList : public std::list { - element& at(int offset) { - return ListAt(*this, offset); - } - } list; + List list; for (auto i = 0; i < kMaxElements; ++i) { auto& elem = list.emplace_back(); @@ -53,14 +49,9 @@ TEST(UtilTest, ListAtTest) { } TEST(UtilTest, ListAtDeathForwardTest) { + List list; const int kMaxElements = 3; - struct IntList : public std::list { - int& at(int offset) { - return ListAt(*this, offset); - } - } list; - for (auto i = 0; i < kMaxElements; ++i) { list.emplace_back(i); } @@ -68,14 +59,9 @@ TEST(UtilTest, ListAtDeathForwardTest) { } TEST(UtilTest, ListAtDeathBackwardTest) { + List list; const int kMaxElements = 3; - struct IntList : public std::list { - int& at(int offset) { - return ListAt(*this, offset); - } - } list; - for (auto i = 0; i < kMaxElements; ++i) { list.emplace_back(i); } -- cgit v1.2.3 From 352cb8fa8a80066fa0ea68ec202fae53f305aaca Mon Sep 17 00:00:00 2001 From: Denis Brockus Date: Thu, 23 Mar 2023 13:57:14 -0600 Subject: Cleanup *_filter_interpreter.h include list Missed a nit in crrev/c/4361866 to remove the includes that are not being directly accessed. BUG=b:271591258 TEST=USE="coverage" FEATURES="test noclean" emerge-brya chromeos-base/gestures Change-Id: I0be861b846acb60c2f263db5456e87d6fc288f9c Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/gestures/+/4367481 Reviewed-by: Harry Cutts Commit-Queue: Harry Cutts Tested-by: Denis Brockus Code-Coverage: Harry Cutts Auto-Submit: Denis Brockus --- include/lookahead_filter_interpreter.h | 1 - include/metrics_filter_interpreter.h | 1 - include/trend_classifying_filter_interpreter.h | 2 -- 3 files changed, 4 deletions(-) diff --git a/include/lookahead_filter_interpreter.h b/include/lookahead_filter_interpreter.h index ba2f92b..2658daa 100644 --- a/include/lookahead_filter_interpreter.h +++ b/include/lookahead_filter_interpreter.h @@ -3,7 +3,6 @@ // found in the LICENSE file. #include -#include #include #include diff --git a/include/metrics_filter_interpreter.h b/include/metrics_filter_interpreter.h index 816f849..518a47a 100644 --- a/include/metrics_filter_interpreter.h +++ b/include/metrics_filter_interpreter.h @@ -3,7 +3,6 @@ // found in the LICENSE file. #include // for FRIEND_TEST -#include #include #include "include/filter_interpreter.h" diff --git a/include/trend_classifying_filter_interpreter.h b/include/trend_classifying_filter_interpreter.h index 049c5e0..92ae3d2 100644 --- a/include/trend_classifying_filter_interpreter.h +++ b/include/trend_classifying_filter_interpreter.h @@ -3,9 +3,7 @@ // found in the LICENSE file. #include // for FRIEND_TEST -#include #include -#include #include "include/filter_interpreter.h" #include "include/finger_metrics.h" -- cgit v1.2.3 From b6ae21cf67acbc76254be3677264b0ea472fce09 Mon Sep 17 00:00:00 2001 From: Harry Cutts Date: Wed, 22 Mar 2023 14:34:34 +0000 Subject: Document how raised touches should be reported It wasn't immediately clear before whether a raised touch should be reported one last time with tracking_id = -1 in its FingerState, or just dropped from the fingers array altogether. It turns out to be the latter [0], so add some comments to make that clear. Also move the information from a comment that had somehow been separated from the FingerState struct, while we're at it. [0]: https://crsrc.org/c/ui/events/ozone/evdev/libgestures_glue/gesture_interpreter_libevdev_cros.cc;l=199;drc=3e1a26c44c024d97dc9a4c09bbc6a2365398ca2c BUG=None TEST=emerge-volteer chromeos-base/gestures Change-Id: I8325a1b9bfedaa88c9a5d15a85d1b7d1b3498e59 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/gestures/+/4360158 Tested-by: Harry Cutts Auto-Submit: Harry Cutts Reviewed-by: Torsha Banerjee Code-Coverage: Torsha Banerjee Reviewed-by: Sean O'Brien Commit-Queue: Harry Cutts --- include/gestures.h | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/include/gestures.h b/include/gestures.h index f21d531..f3005a0 100644 --- a/include/gestures.h +++ b/include/gestures.h @@ -106,15 +106,6 @@ struct HardwareProperties { #endif // __cplusplus }; -// position is the (x,y) cartesian coord of the finger on the trackpad. -// touch_major/minor are the large/small radii of the ellipse of the touching -// finger. width_major/minor are also radii, but of the finger itself, -// including a little bit that isn't touching. So, width* is generally a tad -// larger than touch*. -// tracking_id: If a finger is the same physical finger across two -// consecutive frames, it must have the same tracking id; if it's a different -// finger, it may (should) have a different tracking id. - // Warp: If a finger has the 'warp' flag set for an axis, it means that while // the finger may have moved, it should not cause any motion in that direction. // This may occur is some situations where we thought a finger was in one place, @@ -168,16 +159,27 @@ struct HardwareProperties { #define GESTURES_FINGER_WARP_Y (GESTURES_FINGER_WARP_Y_NON_MOVE | \ GESTURES_FINGER_WARP_Y_MOVE) -// Describes a single contact on a touch surface. Unless otherwise noted, the -// fields have the same meaning as the equivalent ABS_MT_... axis in the Linux -// evdev protocol. +// Describes a single contact on a touch surface. Generally, the fields have the +// same meaning as the equivalent ABS_MT_... axis in the Linux evdev protocol. struct FingerState { + // The large and small radii of the ellipse of the finger touching the pad. float touch_major, touch_minor; + + // The large and small radii of the ellipse of the finger, including parts + // that are hovering over the pad but not quite touching. Devices normally + // don't report these values, in which case they should be left at 0. float width_major, width_minor; float pressure; float orientation; + float position_x; float position_y; + + // A number that identifies a single physical finger across consecutive + // frames. If a finger is the same physical finger across two consecutive + // frames, it must have the same tracking ID; if it's a different finger, it + // should have a different tracking ID. It should be ≥ 0 (see documentation + // comment for HardwareState::fingers). short tracking_id; // A bit field of flags that are used internally by the library. (See the @@ -229,8 +231,13 @@ struct HardwareState { unsigned short finger_cnt; // The number of fingers touching the pad, which may be more than finger_cnt. unsigned short touch_cnt; - // A pointer to finger_cnt FingerState structs representing the contacts - // currently being tracked. + // A pointer to an array of FingerState structs with finger_cnt entries, + // representing the contacts currently being tracked. The order in which + // FingerStates appear need not be stable between HardwareStates — only the + // tracking ID is used to track individual contacts over time. Accordingly, + // when a finger is lifted from the pad (and therefore its ABS_MT_TRACKING_ID + // becomes -1), the client should simply stop including it in this array, + // rather than including a final FingerState for it. struct FingerState* fingers; // Mouse axes, which have the same meanings as the Linux evdev axes of the -- cgit v1.2.3