[PATCH v2 weston] libinput: normalize WL_CALIBRATION before passing it to libinput

[Peter Hutterer]

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.

> 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