[PATCH v2 weston] libinput: normalize WL_CALIBRATION before passing it to libinput
Peter Hutterer
peter.hutterer at who-t.net
Tue Sep 9 16:34:52 PDT 2014
On Tue, Sep 09, 2014 at 10:42:26PM +0200, Jonas Ådahl wrote:
> On Tue, Sep 09, 2014 at 01:15:42PM +1000, Peter Hutterer wrote:
> > On Mon, Sep 08, 2014 at 08:16:07PM +0200, Jonas Ådahl wrote:
> > > On Fri, Sep 05, 2014 at 11:25:25AM +1000, Peter Hutterer wrote:
> > > > WL_CALIBRATION, introduced in weston-1.1, requires the translation component
> > > > of the calibration matrix to be in screen coordinates. libinput does not have
> > > > access to this and it's not a very generic way to do this anyway. So with
> > > > the libinput backend, WL_CALIBRATION support is currently broken (#82742).
> > > > This cannot be fixed in libinput without changing its API for this specific
> > > > use-case.
> > > >
> > > > This patch lets weston take care of WL_CALIBRATION. It takes the original
> > > > format and normalizes it before passing it to libinput. This way libinput
> > > > still does the coordinate transformation, weston just needs to provide the
> > > > initial configuration.
> > > >
> > > > Note that this needs an updated libinput, otherwise libinput will try to
> > > > transform coordinates as well.
> > >
> > > Should we also deprecate the WL_CALIBRATION udev parameter and warn
> > > about when using it, as well as having weston-calibrator output the
> > > normalized calibration matrix?
> >
> > tbh, I'd rather leave it as-is. I'm not a big fan of the configuration
> > back-channel in libinput, I'd prefer this to be set by the compositor as
> > required. with WL_CALIBRATION as-is that works fine, having a weston tool
> > spit out a udev configuration that is then read by libinput to give weston
> > calibrated values seems a bit all over the place to me.
>
> Are you saying you want to get rid of LIBINPUT_CALIBRATION_MATRIX,
> keep having the compositors read the udev configuration unconditionally?
> At least we should not have both LIBINPUT_CALIBRATION_MATRIX *and*
> WL_CALIBRATION around forever.
yeah, good point.
> Regarding where to put it, if a calibration tool outputs data to be read
> by libinput, it makes more sense to move it there. Whether its good or
> not to have a back-channel (i.e. udev configuration reading), I'm not
> sure what I think is better. Calibration is not really configuration
> that you change, its something that is always system wide, either
> bundled by the OEM or set up once and never touched again, so it's not
> really that bad to have it set silently IMHO.
There are two kinds of calibrations, one is for 'wrongly' mounted screens
and usually requires some simple rotation matrix. The other one is for
on-the-fly calibration for touchscreens that are just off by a bit. Old
camera-based ones suffered from that more than new devices. Those
touchscreens sometimes need re-calibration after each boot, but they are
reasonably rare these days. IIRC Win8 specs require that the touchscreens
work without calibration anyway, so most common devices don't need this at
all but I wouldn't be suprised if use cases like IVI see more of the odd
ones though.
As much as I dislike the backchannel for calibration configuration there is
a need for it to be permanently assigned to a device. so
LIBINPUT_CALIBRATION_MATRIX should stay. Though we should document it
properly and make it part of the API, and provide examples enough that a
user knows what values to put in for the various basic calibrations
(rotated, flipped, upside-down). I'll get patches on the list for that in a
tick.
> On a side note, there is still a problem with the current format of
> WL_CALIBRATION and that is that it's translation part is still in pixels,
> and outputs don't necessarily always have the same resolution, which
> would make the calibration off when not using the resolution used when
> calibrating.
yes. without breaking existing use-cases we can't really change that
though.
Cheers,
Peter
> > > We could also move weston-calibrator to
> > > libinput, but then we'd need to port it to something else than toytoolkit
> > > (or by not using a toolkit), which naturally is less trivial. Keeping
> > > weston-calibrator that outputs the non-normalized matrix, at least after
> > > the release, is probably not a good idea.
> > >
> > > In anyway, this patch can be considered
> > > Reviewed-by: Jonas Ådahl <jadahl at gmail.com> with some minor style nits
> > > below.
> >
> > thanks for the review, updated patch is on the list.
> >
> > Cheers,
> > Peter
> > >
> > >
> > > Jonas
> > >
> > > > ---
> > > > Changes to v1:
> > > > - fix copy/paste error assigning height to the screen width
> > > >
> > > > https://bugs.freedesktop.org/show_bug.cgi?id=82742 has positive test results
> > > > so we don't break exisint setups with WL_CALIBRATION set.
> > > >
> > > > Can be merged once libinput 0.6 is released.
> > > >
> > > > configure.ac | 2 +-
> > > > src/libinput-device.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > 2 files changed, 88 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/configure.ac b/configure.ac
> > > > index bc5c88a..fb19fb2 100644
> > > > --- a/configure.ac
> > > > +++ b/configure.ac
> > > > @@ -160,7 +160,7 @@ AC_ARG_ENABLE(libinput-backend, [ --disable-libinput-backend],,
> > > > AM_CONDITIONAL([ENABLE_LIBINPUT_BACKEND], [test x$enable_libinput_backend = xyes])
> > > > if test x$enable_libinput_backend = xyes; then
> > > > AC_DEFINE([BUILD_LIBINPUT_BACKEND], [1], [Build the libinput input device backend])
> > > > - PKG_CHECK_MODULES(LIBINPUT_BACKEND, [libinput >= 0.5.0])
> > > > + PKG_CHECK_MODULES(LIBINPUT_BACKEND, [libinput >= 0.6.0])
> > > > fi
> > > >
> > > >
> > > > diff --git a/src/libinput-device.c b/src/libinput-device.c
> > > > index 6e50eeb..fded34c 100644
> > > > --- a/src/libinput-device.c
> > > > +++ b/src/libinput-device.c
> > > > @@ -282,6 +282,90 @@ notify_output_destroy(struct wl_listener *listener, void *data)
> > > > }
> > > > }
> > > >
> > > > +/**
> > > > + * The WL_CALIBRATION property requires a pixel-specific matrix to be
> > > > + * applied after scaling device coordinates to screen coordinates. libinput
> > > > + * can't do that, so we need to convert the calibration to the normalized
> > > > + * format libinput expects.
> > > > + */
> > > > +static void
> > > > +evdev_device_set_calibration(struct evdev_device *device)
> > > > +{
> > > > + struct udev *udev;
> > > > + struct udev_device *udev_device = NULL;
> > > > + const char *sysname = libinput_device_get_sysname(device->device);
> > > > + const char *calibration_values;
> > > > + uint32_t width, height;
> > > > + float calibration[6];
> > > > + enum libinput_config_status status;
> > > > +
> > > > + if (!device->output)
> > > > + return;
> > > > +
> > > > + width = device->output->width;
> > > > + height = device->output->height;
> > > > + if (width == 0 || height == 0)
> > > > + return;
> > > > +
> > > > + /* If libinput has a pre-set calibration matrix, don't override it */
> > > > + if (!libinput_device_config_calibration_has_matrix(device->device) ||
> > > > + libinput_device_config_calibration_get_default_matrix(
> > > > + device->device,
> > > > + calibration) != 0)
> > > > + return;
> > > > +
> > > > + udev = udev_new();
> > > > + if (!udev)
> > > > + return;
> > > > +
> > > > + udev_device = udev_device_new_from_subsystem_sysname(udev,
> > > > + "input",
> > > > + sysname);
> > > > + if (!udev_device)
> > > > + goto out;
> > > > +
> > > > + calibration_values =
> > > > + udev_device_get_property_value(udev_device,
> > > > + "WL_CALIBRATION");
> > >
> > > These two lines should be indented.
> > >
> > > > +
> > > > + if (!calibration_values || sscanf(calibration_values,
> > > > + "%f %f %f %f %f %f",
> > > > + &calibration[0],
> > > > + &calibration[1],
> > > > + &calibration[2],
> > > > + &calibration[3],
> > > > + &calibration[4],
> > > > + &calibration[5]) != 6)
> > > > + goto out;
> > > > +
> > > > + weston_log("Applying calibration: %f %f %f %f %f %f "
> > > > + "(normalized %f %f)\n",
> > > > + calibration[0],
> > > > + calibration[1],
> > > > + calibration[2],
> > > > + calibration[3],
> > > > + calibration[4],
> > > > + calibration[5],
> > > > + calibration[2]/width,
> > > > + calibration[5]/height);
> > >
> > > There should be spaces between the binary operators.
> > >
> > > > +
> > > > + /* normalize to a format libinput can use. There is a chance of
> > > > + this being wrong if the width/height don't match the device
> > > > + width/height but I'm not sure how to fix that */
> > > > + calibration[2] /= width;
> > > > + calibration[5] /= height;
> > > > +
> > > > + status = libinput_device_config_calibration_set_matrix(device->device,
> > > > + calibration);
> > > > + if (status != LIBINPUT_CONFIG_STATUS_SUCCESS)
> > > > + weston_log("Failed to apply calibration.\n");
> > > > +
> > > > +out:
> > > > + if (udev_device)
> > > > + udev_device_unref(udev_device);
> > > > + udev_unref(udev);
> > > > +}
> > > > +
> > > > void
> > > > evdev_device_set_output(struct evdev_device *device,
> > > > struct weston_output *output)
> > > > @@ -295,6 +379,7 @@ evdev_device_set_output(struct evdev_device *device,
> > > > device->output_destroy_listener.notify = notify_output_destroy;
> > > > wl_signal_add(&output->destroy_signal,
> > > > &device->output_destroy_listener);
> > > > + evdev_device_set_calibration(device);
> > > > }
> > > >
> > > > static void
> > > > @@ -318,6 +403,8 @@ configure_device(struct evdev_device *device)
> > > > libinput_device_config_tap_set_enabled(device->device,
> > > > enable_tap);
> > > > }
> > > > +
> > > > + evdev_device_set_calibration(device);
> > > > }
> > > >
> > > > struct evdev_device *
> > > > --
> > > > 1.9.3
> > > >
> > > > _______________________________________________
> > > > wayland-devel mailing list
> > > > wayland-devel at lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list