[PATCH libevdev] Support direct ABS_MT_TRACKING_ID changes
Benjamin Tissoires
benjamin.tissoires at gmail.com
Mon Apr 16 17:55:14 UTC 2018
On Mon, Apr 16, 2018 at 7:07 AM, Peter Hutterer
<peter.hutterer at who-t.net> wrote:
> Due to some decade-old misunderstanding of how tracking IDs can change in the
> kernel, libevdev suppresses direct ABS_MT_TRACKING_ID changes. So the
> transition that is ok is M → -1 → N, but a direct M → N change would be
> suppressed. The cause is that most clients (at least libinput, evdev,
> synaptics) have code like this:
>
> if tracking_id == -1
> end_touch()
> else
> begin_new_touch()
>
> So a direct tracking ID change would cause new touches without terminating the
> old touches.
>
> To provide a gentle switch add a new flag for libevdev_next_event() to allow
> for direct tracking ID changes. When provided, libevdev doesn't filter those
> direct changes and clients have to be able to cope with them directly.
>
> This does not apply to a duplicate -1 ABS_MT_TRACKING_ID but that would be a
> real kernel bug.
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
Acked-by: Benjamin Tissoires <benjamin.tissoires at gmail.com>
Cheers,
Benjamin
> ---
> libevdev/libevdev.c | 16 +++--
> libevdev/libevdev.h | 76 +++++++++++++++++++-
> test/test-libevdev-events.c | 166 +++++++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 251 insertions(+), 7 deletions(-)
>
> diff --git a/libevdev/libevdev.c b/libevdev/libevdev.c
> index c6ec114..903abba 100644
> --- a/libevdev/libevdev.c
> +++ b/libevdev/libevdev.c
> @@ -979,6 +979,7 @@ update_state(struct libevdev *dev, const struct input_event *e)
> */
> static inline enum event_filter_status
> sanitize_event(const struct libevdev *dev,
> + bool allow_tracking_id_change,
> struct input_event *ev,
> enum SyncState sync_state)
> {
> @@ -1004,7 +1005,8 @@ sanitize_event(const struct libevdev *dev,
> ((ev->value == -1 &&
> *slot_value(dev, dev->current_slot, ABS_MT_TRACKING_ID) == -1) ||
> (ev->value != -1 &&
> - *slot_value(dev, dev->current_slot, ABS_MT_TRACKING_ID) != -1)))) {
> + *slot_value(dev, dev->current_slot, ABS_MT_TRACKING_ID) != -1 &&
> + !allow_tracking_id_change)))) {
> log_bug(dev, "Device \"%s\" received a double tracking ID %d in slot %d.\n",
> dev->name, ev->value, dev->current_slot);
> return EVENT_FILTER_DISCARD;
> @@ -1021,7 +1023,9 @@ libevdev_next_event(struct libevdev *dev, unsigned int flags, struct input_event
> const unsigned int valid_flags = LIBEVDEV_READ_FLAG_NORMAL |
> LIBEVDEV_READ_FLAG_SYNC |
> LIBEVDEV_READ_FLAG_FORCE_SYNC |
> - LIBEVDEV_READ_FLAG_BLOCKING;
> + LIBEVDEV_READ_FLAG_BLOCKING |
> + LIBEVDEV_READ_FLAG_ALLOW_TRACKING_ID_SWITCH;
> + bool allow_tracking_id_change = !!(flags & LIBEVDEV_READ_FLAG_ALLOW_TRACKING_ID_SWITCH);
>
> if (!dev->initialized) {
> log_bug(dev, "device not initialized. call libevdev_set_fd() first\n");
> @@ -1054,7 +1058,8 @@ libevdev_next_event(struct libevdev *dev, unsigned int flags, struct input_event
> of the device too */
> while (queue_shift(dev, &e) == 0) {
> dev->queue_nsync--;
> - if (sanitize_event(dev, &e, dev->sync_state) != EVENT_FILTER_DISCARD)
> + if (sanitize_event(dev, allow_tracking_id_change,
> + &e, dev->sync_state) != EVENT_FILTER_DISCARD)
> update_state(dev, &e);
> }
>
> @@ -1084,7 +1089,8 @@ libevdev_next_event(struct libevdev *dev, unsigned int flags, struct input_event
> if (queue_shift(dev, ev) != 0)
> return -EAGAIN;
>
> - filter_status = sanitize_event(dev, ev, dev->sync_state);
> + filter_status = sanitize_event(dev, allow_tracking_id_change,
> + ev, dev->sync_state);
> if (filter_status != EVENT_FILTER_DISCARD)
> update_state(dev, ev);
>
> @@ -1283,7 +1289,7 @@ libevdev_set_event_value(struct libevdev *dev, unsigned int type, unsigned int c
> e.code = code;
> e.value = value;
>
> - if (sanitize_event(dev, &e, SYNC_NONE) != EVENT_FILTER_NONE)
> + if (sanitize_event(dev, false, &e, SYNC_NONE) != EVENT_FILTER_NONE)
> return -1;
>
> switch(type) {
> diff --git a/libevdev/libevdev.h b/libevdev/libevdev.h
> index b36d3b4..4863989 100644
> --- a/libevdev/libevdev.h
> +++ b/libevdev/libevdev.h
> @@ -428,6 +428,74 @@ extern "C" {
> * @endcode
> */
>
> +/**
> + * @page tracking_id_changes ABS_MT_TRACKING_ID changes
> + *
> + * By default, libevdev expects an `ABS_MT_TRACKING_ID` of -1 before the start
> + * of a new touchpoint.
> + *
> + * An event sequence may look like this:
> + *
> + * @code
> + * EV_ABS ABS_MT_SLOT 0
> + * EV_ABS ABS_MT_TRACKING_ID 45
> + * EV_ABS ABS_MT_POSITION_X 100
> + * EV_ABS ABS_MT_POSITION_Y 80
> + * EV_SYN SYN_REPORT 0
> + * ------------------------
> + * EV_ABS ABS_MT_TRACKING_ID -1
> + * EV_SYN SYN_REPORT 0
> + * ------------------------
> + * EV_ABS ABS_MT_TRACKING_ID 46
> + * EV_ABS ABS_MT_POSITION_X 200
> + * EV_ABS ABS_MT_POSITION_Y 80
> + * EV_SYN SYN_REPORT 0
> + * ------------------------
> + * @endcode
> + *
> + * However, in some cases, the kernel may change the tracking ID directly,
> + * without changing it to -1 first. An event sequence may look like
> + * this:
> + *
> + * @code
> + * EV_ABS ABS_MT_TRACKING_ID 30
> + * EV_ABS ABS_MT_POSITION_X 100
> + * EV_ABS ABS_MT_POSITION_Y 80
> + * EV_SYN SYN_REPORT 0
> + * ------------------------
> + * EV_ABS ABS_MT_TRACKING_ID 31
> + * EV_ABS ABS_MT_POSITION_X 200
> + * EV_ABS ABS_MT_POSITION_Y 80
> + * EV_SYN SYN_REPORT 0
> + * ------------------------
> + * @endcode
> + *
> + * Direct tracking ID changes are mainly caused when the kernel lost track
> + * of the touch point and cannot guarantee that the new touch point is the
> + * same as the one before. In some cases, it cannot be determined whether a
> + * finger moved a large distance quickly, or whether a finger was relased
> + * and a new finger set down within the same hardware frame.
> + *
> + * Unfortunately, direct tracking ID changes require client support and
> + * until April 2018, none of the most widely distributed libevdev clients
> + * expected direct tracking ID changes (libinput, xf86-input-evdev,
> + * xf86-input-synaptics). libevdev 1.5.x and earlier always filtered out
> + * these changes, effectively suppressing the `ABS_MT_TRACKING_ID` event
> + * unless it was preceeded by one with value -1.
> + *
> + * The message logged for the example above would be:
> + * <blockquote >
> + * Device "foo" received a double tracking ID 31 in slot 0.
> + * </blockquote>
> + *
> + * As of libevdev 1.6.0, the @ref
> + * LIBEVDEV_READ_FLAG_ALLOW_TRACKING_ID_SWITCH flag may be provided to
> + * libevdev_next_event(). When present, libevdev will **not** supress direct
> + * tracking ID changes and forward them to the client. It is the client's
> + * responsibility to keep track of direct changes vs touch changes
> + * terminated with a tracking ID of -1.
> + */
> +
> /**
> * @page backwardscompatibility Compatibility and Behavior across kernel versions
> *
> @@ -756,7 +824,13 @@ enum libevdev_read_flag {
> LIBEVDEV_READ_FLAG_NORMAL = 2, /**< Process data in normal mode */
> LIBEVDEV_READ_FLAG_FORCE_SYNC = 4, /**< Pretend the next event is a SYN_DROPPED and
> require the caller to sync */
> - LIBEVDEV_READ_FLAG_BLOCKING = 8 /**< The fd is not in O_NONBLOCK and a read may block */
> + LIBEVDEV_READ_FLAG_BLOCKING = 8, /**< The fd is not in O_NONBLOCK and a read may block */
> + /**
> + * Allow for direct tracking ID changes, see @ref tracking_id_changes.
> + * All newly written clients should support this behavior and
> + * provide this flag.
> + */
> + LIBEVDEV_READ_FLAG_ALLOW_TRACKING_ID_SWITCH = 16
> };
>
> /**
> diff --git a/test/test-libevdev-events.c b/test/test-libevdev-events.c
> index dd2face..19602ec 100644
> --- a/test/test-libevdev-events.c
> +++ b/test/test-libevdev-events.c
> @@ -81,7 +81,7 @@ START_TEST(test_next_event_invalid_fd)
> -1);
>
> /* invalid (missing) flag */
> - rc = libevdev_next_event(dev, 0x10, &ev);
> + rc = libevdev_next_event(dev, 0x20, &ev);
> ck_assert_int_eq(rc, -EINVAL);
>
> /* set an invalid fd */
> @@ -1882,6 +1882,168 @@ START_TEST(test_mt_tracking_id_discard_neg_1)
> }
> END_TEST
>
> +START_TEST(test_mt_tracking_id_keep)
> +{
> + struct uinput_device* uidev;
> + struct libevdev *dev;
> + int rc;
> + struct input_event ev;
> + struct input_absinfo abs[6];
> + unsigned int flags = LIBEVDEV_READ_FLAG_NORMAL |
> + LIBEVDEV_READ_FLAG_ALLOW_TRACKING_ID_SWITCH;
> +
> + memset(abs, 0, sizeof(abs));
> + abs[0].value = ABS_X;
> + abs[0].maximum = 1000;
> + abs[1].value = ABS_MT_POSITION_X;
> + abs[1].maximum = 1000;
> +
> + abs[2].value = ABS_Y;
> + abs[2].maximum = 1000;
> + abs[3].value = ABS_MT_POSITION_Y;
> + abs[3].maximum = 1000;
> +
> + abs[4].value = ABS_MT_SLOT;
> + abs[4].maximum = 10;
> + abs[5].value = ABS_MT_TRACKING_ID;
> + abs[5].maximum = 500;
> +
> + test_create_abs_device(&uidev, &dev,
> + 6, abs,
> + EV_SYN, SYN_REPORT,
> + -1);
> +
> + uinput_device_event(uidev, EV_ABS, ABS_MT_SLOT, 1);
> + uinput_device_event(uidev, EV_ABS, ABS_MT_TRACKING_ID, 1);
> + uinput_device_event(uidev, EV_SYN, SYN_REPORT, 0);
> +
> + /* second tracking ID on same slot */
> + uinput_device_event(uidev, EV_ABS, ABS_MT_TRACKING_ID, 2);
> + uinput_device_event(uidev, EV_SYN, SYN_REPORT, 0);
> +
> + rc = libevdev_next_event(dev, flags, &ev);
> + ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
> + ck_assert_int_eq(ev.type, EV_ABS);
> + ck_assert_int_eq(ev.code, ABS_MT_SLOT);
> + ck_assert_int_eq(ev.value, 1);
> +
> + rc = libevdev_next_event(dev, flags, &ev);
> + ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
> + ck_assert_int_eq(ev.type, EV_ABS);
> + ck_assert_int_eq(ev.code, ABS_MT_TRACKING_ID);
> + ck_assert_int_eq(ev.value, 1);
> +
> + rc = libevdev_next_event(dev, flags, &ev);
> + ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
> + ck_assert_int_eq(ev.type, EV_SYN);
> + ck_assert_int_eq(ev.code, SYN_REPORT);
> + ck_assert_int_eq(ev.value, 0);
> +
> + rc = libevdev_next_event(dev, flags, &ev);
> + ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
> + ck_assert_int_eq(ev.type, EV_ABS);
> + ck_assert_int_eq(ev.code, ABS_MT_TRACKING_ID);
> + ck_assert_int_eq(ev.value, 2);
> +
> + rc = libevdev_next_event(dev, flags, &ev);
> + ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
> + ck_assert_int_eq(ev.type, EV_SYN);
> + ck_assert_int_eq(ev.code, SYN_REPORT);
> + ck_assert_int_eq(ev.value, 0);
> +
> + rc = libevdev_next_event(dev, flags, &ev);
> + ck_assert_int_eq(rc, -EAGAIN);
> +
> + uinput_device_free(uidev);
> + libevdev_free(dev);
> +}
> +END_TEST
> +
> +START_TEST(test_mt_tracking_id_nokeep_neg_1)
> +{
> + struct uinput_device* uidev;
> + struct libevdev *dev;
> + int rc;
> + struct input_event ev;
> + struct input_absinfo abs[6];
> + int pipefd[2];
> + struct input_event events[] = {
> + { .type = EV_ABS, .code = ABS_MT_TRACKING_ID, .value = -1 },
> + { .type = EV_SYN, .code = SYN_REPORT, .value = 0 },
> + };
> + unsigned int flags = LIBEVDEV_READ_FLAG_NORMAL |
> + LIBEVDEV_READ_FLAG_ALLOW_TRACKING_ID_SWITCH;
> +
> + rc = pipe2(pipefd, O_NONBLOCK);
> + ck_assert_int_eq(rc, 0);
> +
> + memset(abs, 0, sizeof(abs));
> + abs[0].value = ABS_X;
> + abs[0].maximum = 1000;
> + abs[1].value = ABS_MT_POSITION_X;
> + abs[1].maximum = 1000;
> +
> + abs[2].value = ABS_Y;
> + abs[2].maximum = 1000;
> + abs[3].value = ABS_MT_POSITION_Y;
> + abs[3].maximum = 1000;
> +
> + abs[4].value = ABS_MT_SLOT;
> + abs[4].maximum = 10;
> + abs[5].value = ABS_MT_TRACKING_ID;
> + abs[5].maximum = 500;
> +
> + test_create_abs_device(&uidev, &dev,
> + 6, abs,
> + EV_SYN, SYN_REPORT,
> + -1);
> + uinput_device_event(uidev, EV_ABS, ABS_MT_SLOT, 1);
> + uinput_device_event(uidev, EV_ABS, ABS_MT_TRACKING_ID, 1);
> + uinput_device_event(uidev, EV_SYN, SYN_REPORT, 0);
> +
> + while (libevdev_next_event(dev, flags, &ev) != -EAGAIN)
> + ;
> +
> + libevdev_set_log_function(test_logfunc_ignore_error, NULL);
> +
> + /* two -1 tracking ids, need to use the pipe here, the kernel will
> + filter it otherwise */
> + libevdev_change_fd(dev, pipefd[0]);
> +
> + rc = write(pipefd[1], events, sizeof(events));
> + ck_assert_int_eq(rc, sizeof(events));
> + rc = write(pipefd[1], events, sizeof(events));
> + ck_assert_int_eq(rc, sizeof(events));
> +
> + rc = libevdev_next_event(dev, flags, &ev);
> + ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
> + ck_assert_int_eq(ev.type, EV_ABS);
> + ck_assert_int_eq(ev.code, ABS_MT_TRACKING_ID);
> + ck_assert_int_eq(ev.value, -1);
> + rc = libevdev_next_event(dev, flags, &ev);
> + ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
> + ck_assert_int_eq(ev.type, EV_SYN);
> + ck_assert_int_eq(ev.code, SYN_REPORT);
> + ck_assert_int_eq(ev.value, 0);
> +
> + /* expect second tracking ID discarded */
> +
> + rc = libevdev_next_event(dev, flags, &ev);
> + ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
> + ck_assert_int_eq(ev.type, EV_SYN);
> + ck_assert_int_eq(ev.code, SYN_REPORT);
> + ck_assert_int_eq(ev.value, 0);
> +
> + rc = libevdev_next_event(dev, flags, &ev);
> + ck_assert_int_eq(rc, -EAGAIN);
> +
> + libevdev_set_log_function(test_logfunc_abort_on_error, NULL);
> +
> + uinput_device_free(uidev);
> + libevdev_free(dev);
> +}
> +END_TEST
> +
> START_TEST(test_ev_rep_values)
> {
> struct uinput_device* uidev;
> @@ -2183,6 +2345,8 @@ libevdev_events(void)
> tcase_add_test(tc, test_mt_slot_ranges_invalid);
> tcase_add_test(tc, test_mt_tracking_id_discard);
> tcase_add_test(tc, test_mt_tracking_id_discard_neg_1);
> + tcase_add_test(tc, test_mt_tracking_id_keep);
> + tcase_add_test(tc, test_mt_tracking_id_nokeep_neg_1);
> tcase_add_test(tc, test_ev_rep_values);
> suite_add_tcase(s, tc);
>
> --
> 2.14.3
>
> _______________________________________________
> Input-tools mailing list
> Input-tools at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/input-tools
More information about the Input-tools
mailing list