[PATCH v2 weston] libinput: normalize WL_CALIBRATION before passing it to libinput
Jonas Ådahl
jadahl at gmail.com
Tue Sep 9 23:25:13 PDT 2014
On Wed, Sep 10, 2014 at 09:34:52AM +1000, Peter Hutterer wrote:
> 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.
Sounds like the camera based ones seems like they need special support
in the compositor to do the post-boot calibration and as such would not
need any permanent calibration matrix at all. And yes it seems, as there
are already users of it, that a calibration matrix is indeed needed.
>
> 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.
We'd eventually fix it by deprecating and finally removing WL_CALIBRATION
though. Probably not for 1.6 though since it's a bit to close to release
for making such changes, but maybe for 1.7?
Jonas
>
> 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