GNOME Bugzilla – Bug 709600
Screen mapping incorrect when Cintiq connected to tablet PC
Last modified: 2014-02-14 01:39:40 UTC
g-s-d tries to determine the correct screen mapping by binding tablets to any display with a "WAC" EDID. This makes sense for Cinitq devices, but not for tablet PCs. If a Cintiq is connected to a tablet PC, the heuristic causes the ISD digitizer to map to the Cintiq instead of remaining on the integrated screen.
Created attachment 256674 [details] [review] Fix reported bug The attached patch fixes this bug by changing the heuristic so that ISD digitizers only map to integrated displays and external digitizers search for a "WAC" EDID. (note: not yet compile-tested; my GNOME dev environment is months out of date...)
Created attachment 256675 [details] [review] wacom: Map ISD digitizers only to integrated displays ISD digitisers are built into the system that they're being used with and as such should only ever be mapped to an integrated display. The old logic preferentially maps to a display with the "WAC" EDID, which will be an issue if a Cintiq is connected to a tablet PC.
This one keeps the fallback, if the cintiq screen is off (but the digitizer still on).
Created attachment 256676 [details] [review] wacom: Map ISD digitizers only to integrated displays ISD digitisers are built into the system that they're being used with and as such should only ever be mapped to an integrated display. The old logic preferentially maps to a display with the "WAC" EDID, which will be an issue if a Cintiq is connected to a tablet PC.
Removes the unnecessary 2nd call to 'find_output_by_edid' in attachment 256675 [details] [review].
Created attachment 256681 [details] [review] wacom: Map ISD digitizers only to integrated displays ISD digitisers are built into the system that they're being used with and as such should only ever be mapped to an integrated display. The old logic preferentially maps to a display with the "WAC" EDID, which will be an issue if a Cintiq is connected to a tablet PC.
With the fallback, really. I shouldn't update patches at 2AM.
Any news on testing this?
Afraid not. Its been near the bottom of my to-do list, even moreso with jhbuild generally disliking me. I'll see if I can find some time to wrangle with the build system and get you a test result.
Comment on attachment 256681 [details] [review] wacom: Map ISD digitizers only to integrated displays I've tricked g-s-d underneath to trigger this with my x220t and my not-really-a-cintiq second monitor, the patch indeed works for the case. Although I would suggest taking this out of find_output_by_heuristic() and putting it into find_output() itself, as it's not really an heuristic anymore, plus it makes the "mapping tablet to heuristically-found display" warning go from frequent to gone in this laptop :)
Created attachment 263150 [details] [review] wacom: Map ISD digitizers only to integrated displays ISD digitisers are built into the system that they're being used with and as such should only ever be mapped to an integrated display. The old logic preferentially maps to a display with the "WAC" EDID, which will be an issue if a Cintiq is connected to a tablet PC.
Created attachment 263151 [details] [review] wacom: Attempt to match EDID product with screen-tablet wacom devices Before sorting out to the heuristic methods; if a wacom device is a screen tablet, look for a display with a product name equal to the device name's. This does work with a Cintiq 12WX, so it should be safe to assume this happens too on devices with sane EDIDs.
I like the idea behind the second patch, but as its written I don't think its doing anything useful. The EDID product names appear to be very generic and wouldn't match in find_output_by_edid using the logic you have in place. My 12WX at least has the EDID product name "Cintiq", rather than the needed "Cintiq 12WX". EDID product names for other Wacom displays I have handy seem to be similarly generic: Cintiq 24HDT: "Cintiq" Cintiq 21UX: "WACOM" Cintiq 12WX "Cintiq" DTU-2231: N/A
Hmm, thanks for that list, I find it very funky that the 12WX I have here has a more specific EDID product name... Seeing the situation, I think there's several levels of confidence if we continue going this way: - not-so-heuristic checks like the one in your patch - an exact match on product name is next (still prone to failure if 2 identical tablets are there, but...) - a product name substring match comes after - just matching vendor name comes last I would propose doing the following: - per-device, fetch all possible matching outputs, sorted by confidence - traverse through devices from most to least confidence gotten. - pick the output(s) with most confidence * if there's one, take it as good * if there's more than one with the same degree, pick one and warn * always set a hint on outputs so we know which ones already have an in-display tablet assigned in order to ignore these on later matches - if there was no match, check for any second output, as any in-display tablet left here is unlikely to be on the primary monitor. Also warn. - if there's no second monitor, then pick primary output and warn With something like this, it could still break if there's 2 too similar outputs (24HDT and 12WX, or 21UX and DTU-2231), for that I guess we could also keep a gsettings key for edid like in other devices, so there's a way to mean it... I'll start working on this
Created attachment 268200 [details] [review] common: Add GsdDeviceMapper This is a per-screen object that's to be used across plugins, it on one hand keeps track of all outputs and their position/rotation, and on the other hand it is offered by plugins GdkDevices to handle, so those devices are mapped to the corresponding device. The primary source to know the corresponding output is the configuration, if a device has a configured display in GSettings that's currently present, that will be the one the device maps to. When a device has no settings, or an unconfigured display, multiple things are checked in order to find out a good guess. - System-integrated devices go to the builtin output - Several EDID checks are done for Wacom devices with a display built-in. - Already attached devices are checked on outputs, so a single output doesn't hoard multiple pen/eraser/touch devices. If an output is found on some or other way, the device will be mapped to that output, and follow it if the display configuration changes.
Created attachment 268201 [details] [review] xrandr: Use GsdDeviceMap to rotate touscreens The Xrandr module was previously in charge of rotating touchscreens, although it did so in a limited number of cases (eg. only through fn-F7, not through the g-c-c panel), and didn't do anything to map touchscreens to the right output if multiple monitors are present, so touchscreens would be left mapped to the whole span of monitors. Fix this by using GsdDeviceMapper, so touchscreen mapping is managed by that object. The Xrandr module still gets to manage what/whether touchscreens get added to the mapper.
Created attachment 268202 [details] [review] wacom: Use GsdDeviceMapper Offload device mapping to this object, that has more ellaborate methods to find out a compelling candidate to map an input device to.
Created attachment 268456 [details] [review] common: Add GsdDeviceMapper This is a per-screen object that's to be used across plugins, it on one hand keeps track of all outputs and their position/rotation, and on the other hand it is offered by plugins GdkDevices to handle, so those devices are mapped to the corresponding device. The primary source to know the corresponding output is the configuration, if a device has a configured display in GSettings that's currently present, that will be the one the device maps to. When a device has no settings, or an unconfigured display, multiple things are checked in order to find out a good guess. - System-integrated devices go to the builtin output - Several EDID checks are done for Wacom devices with a display built-in. - Already attached devices are checked on outputs, so a single output doesn't hoard multiple pen/eraser/touch devices. If an output is found on some or other way, the device will be mapped to that output, and follow it if the display configuration changes.
Created attachment 268457 [details] [review] wacom: Use GsdDeviceMapper Offload device mapping to this object, that has more ellaborate methods to find out a compelling candidate to map an input device to.
Created attachment 268679 [details] [review] wacom: Use GsdDeviceMapper Offload device mapping to this object, that has more ellaborate methods to find out a compelling candidate to map an input device to.
I think I should explain the changes a bit more in detail, GsdDeviceMapper is a per-screen object, it will know directly about all outputs, but will need explicit calls to gsd_device_mapper_add_input() to handle an input device. When a device is handled by the device mapper, the output is decided as described in comment #14, which from extensive testing is proving robust here. It is worth noting that all output mapping and left/right handed mode in tablets are unified through the "Coordinate Transformation Matrix" property in XI(2) devices, other properties like "Wacom Rotation" or "Evdev Axis Inversion" are skipped (this might bring issues in testing, if those properties keep the older values from a previous g-s-d instance). The changes in the wacom and xrandr modules ensure pretty much every pen/eraser/pad/touch-on-tablet/touchscreen goes through GsdDeviceMapper, so this also fixes other mapping bugs, especially on touchscreens.
Created attachment 268685 [details] [review] common: Add GsdDeviceMapper This is a per-screen object that's to be used across plugins, it on one hand keeps track of all outputs and their position/rotation, and on the other hand it is offered by plugins GdkDevices to handle, so those devices are mapped to the corresponding device. The primary source to know the corresponding output is the configuration, if a device has a configured display in GSettings that's currently present, that will be the one the device maps to. When a device has no settings, or an unconfigured display, multiple things are checked in order to find out a good guess. - System-integrated devices go to the builtin output - Several EDID checks are done for Wacom devices with a display built-in. - Already attached devices are checked on outputs, so a single output doesn't hoard multiple pen/eraser/touch devices. If an output is found on some or other way, the device will be mapped to that output, and follow it if the display configuration changes.
Review of attachment 268685 [details] [review]: I can't really parse the first paragraph of the commit message, could you reword it? Also, when you use "device" I'd rather your said "GdkDevice" or "input device" depending on whether you're talking about the physical device, or the XInput/GdkDevice instances (eg. a Wacom tablet is a single device, but multiple GdkDevices). "If an output is found on some or other way" That should be "If an output is found one way or another", but you could probably reword that to "If an output is found for that GdkDevice". The code is missing some comments in general. ::: plugins/common/gsd-device-mapper.c @@ +33,3 @@ + +enum { + INPUT_IS_SYSTEM_INTEGRATED = 1 << 0, Can you add some comments/examples for each type of device listed here? @@ +39,3 @@ + INPUT_IS_ERASER = 1 << 4, + INPUT_IS_PAD = 1 << 5 +}; Name the flags here please. @@ +101,3 @@ + +enum { + PROP_SCREEN = 1 I prefer having a PROP_0 as the first item. @@ +111,3 @@ +static guint signals[N_SIGNALS] = { 0 }; + +#define NUM_ELEMS_MATRIX 9 Define that before and use it in the declaration of the rotation_matrices[]. @@ +255,3 @@ + + if (monitor_num == gdk_screen_get_monitor_at_point (mapper->screen, + x, y)) No need for a linefeed here, we have wide screens. @@ +267,3 @@ + MappingHelper *helper; + + helper = g_new0 (MappingHelper, 1); Does it make sense to use slices for those? @@ +325,3 @@ + gchar **split; + + name = gdk_device_get_name (input->device); A few comments about what you're doing in this function wouldn't go amiss. @@ +402,3 @@ + gsize nvalues; + + old = g_settings_get_value (settings, KEY_DISPLAY); Why not g_settings_get_strv()? ::: plugins/common/gsd-device-mapper.h @@ +39,3 @@ + +GType gsd_device_mapper_get_type (void) G_GNUC_CONST; +GsdDeviceMapper * gsd_device_mapper_get (GdkScreen *screen); We only support a single GdkScreen (with multiple monitors though), so a call that gets the GsdDeviceMapper for the default screen is probably best. ::: plugins/common/gsd-input-helper.c @@ +578,3 @@ + +const char * +xdevice_get_tool_type (int deviceid) get_wacom_tool_type() rather. ::: plugins/common/gsd-input-helper.h @@ +83,3 @@ +const char * xdevice_get_tool_type (int deviceid); + Why the extra linefeeds?
Review of attachment 268201 [details] [review]: "GsdDeviceMapper" in the commit subject. Rest looks good!
Review of attachment 268679 [details] [review]: "elaborate". I'm not sure candidate screens get to be more "compelling" but rather they have "better heuristics". Looks good otherwise.
Created attachment 268862 [details] [review] common: Add GsdDeviceMapper This is an object attached to the default screen, meant to be used across several g-s-d plugins. On one hand, it keeps track of monitors and their layout/rotation/mode. On the other hand, g-s-d plugins tell GsdDeviceMapper which GdkDevices will be handled by this object, so a monitor is chosen for it applying what we know about the input device. The primary source to know the corresponding monitor is the configuration, if an input device has a configured display in GSettings, and the monitor is currently present, that will be the one the input device will map to. When an input device has no settings, or an unconfigured monitor, multiple things are checked in order to find out a good guess for the GdkDevice. - System-integrated input devices go to the built-in output - Several EDID checks are done for Wacom devices with a built-in screen - Sanity checks are done on outputs with screen-integrated input devices, if a monitor already has a GdkDevice with similar capabilities (eg. a system-integrated pen), this output will be punted, so there is no accumulation of input devices. If an output is found on some or other way, the GdkDevice will be mapped to that output, and follow it if the display configuration changes, or device left/right handedness changes.
Created attachment 268863 [details] [review] xrandr: Use GsdDeviceMapper to rotate touscreens The Xrandr module was previously in charge of rotating touchscreens, although it did so in a limited number of cases (eg. only through fn-F7, not through the g-c-c panel), and didn't do anything to map touchscreens to the right output if multiple monitors are present, so touchscreens would be left mapped to the whole span of monitors. Fix this by using GsdDeviceMapper, so touchscreen mapping is managed by that object. The Xrandr module still gets to manage what/whether touchscreens get added to the mapper.
Created attachment 268864 [details] [review] wacom: Use GsdDeviceMapper Offload device mapping to this object, that has more elaborate heuristics to find out the (most suitable) monitor to map an input device to.
Review of attachment 268863 [details] [review]: Looks good.
Review of attachment 268864 [details] [review]: Every call to: g_object_get (device, "gdk-device", &gdk_device, NULL); is leaking the device. It's defined as an object, so needs to be unref'ed.
Review of attachment 268862 [details] [review]: > so a monitor is chosen for it applying what we know about the input device. I don't understand that. Rest looks good. ::: plugins/common/gsd-device-mapper.c @@ +48,3 @@ + INPUT_IS_ERASER = 1 << 4, /* eraser device, either touchscreen or tablet */ + INPUT_IS_PAD = 1 << 5 /* pad device, most usually in tablets */ +}; Those enums/flags are still missing names. @@ +56,3 @@ + PRIO_EDID_MATCH_VENDOR, /* EDID vendor match, eg. "WAC" for Wacom */ + N_OUTPUT_PRIORITIES +}; Ditto. @@ +274,3 @@ + MappingHelper *helper; + + helper = g_new0 (MappingHelper, 1); g_slice? @@ +466,3 @@ + } + + ptr = (guessed) ? &input->guessed_output : &input->output; No need for brackets here, and you're already doing that a couple of lines before. @@ +700,3 @@ + if ((info->input->capabilities & + (INPUT_IS_SYSTEM_INTEGRATED | INPUT_IS_SCREEN_INTEGRATED)) && + output_has_input_type (output, info->input->capabilities)) Could you split this into multiple conditions? if (!(info->input->capabilities & (INPUT_IS...)) continue; ::: plugins/common/gsd-input-helper.c @@ +578,3 @@ + +const char * +get_wacom_tool_type (int deviceid) I meant adding "wacom" in the middle of the function name, not truncating it :) Sorry that wasn't clear.
Thanks for the comments Bastien, I actually forgot to reply on some bits of the first round too (although most was applied) (In reply to comment #23) > @@ +267,3 @@ > + MappingHelper *helper; > + > + helper = g_new0 (MappingHelper, 1); > > Does it make sense to use slices for those? I didn't do this in the end... I don't think there's much benefit as this struct isn't going to be very often, nor very long in memory, and there would be once instance top at any given time. If this happened more often maybe could be worth to have this stack allocated OTOH. (In reply to comment #30) > Review of attachment 268864 [details] [review]: > > Every call to: > g_object_get (device, "gdk-device", &gdk_device, NULL); > is leaking the device. It's defined as an object, so needs to be unref'ed. AFAICS the GsdWacomDevice property has a pointer type: g_object_class_install_property (object_class, PROP_GDK_DEVICE, g_param_spec_pointer ("gdk-device", "gdk-device", "gdk-device", G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY)); Could be worth changing that to being a GParamSpecObject property, although not getting an extra ref was convenient here :).
Created attachment 268914 [details] [review] common: Add GsdDeviceMapper This is an object attached to the default screen, meant to be used across several g-s-d plugins. On one hand, it keeps track of monitors and their layout/rotation/mode. On the other hand, g-s-d plugins tell GsdDeviceMapper which GdkDevices will be handled by this object, so a monitor is chosen to map the GdkDevice to, applying what we know about the input device. The primary source to know the corresponding monitor is the configuration, if an input device has a configured display in GSettings, and the monitor is currently present, that will be the one the input device will map to. When an input device has no settings, or an unconfigured monitor, multiple things are checked in order to find out a good guess for the GdkDevice. - System-integrated input devices go to the built-in output - Several EDID checks are done for Wacom devices with a built-in screen - Sanity checks are done on outputs with screen-integrated input devices, if a monitor already has a GdkDevice with similar capabilities (eg. a system-integrated pen), this output will be punted, so there is no accumulation of input devices. If an output is found on some or other way, the GdkDevice will be mapped to that output, and follow it if the display configuration changes, or device left/right handedness changes.
(In reply to comment #31) > Review of attachment 268862 [details] [review]: > > > so a monitor is chosen for it applying what we know about the input device. > > I don't understand that. > > Rest looks good. > > ::: plugins/common/gsd-device-mapper.c > @@ +48,3 @@ > + INPUT_IS_ERASER = 1 << 4, /* eraser device, either touchscreen > or tablet */ > + INPUT_IS_PAD = 1 << 5 /* pad device, most usually in tablets */ > +}; > > Those enums/flags are still missing names. I added a typedef for those, and they are now "GSD_" prefixed too, event though it's all still internal to gsd-device-mapper.c > @@ +466,3 @@ > + } > + > + ptr = (guessed) ? &input->guessed_output : &input->output; > > No need for brackets here, and you're already doing that a couple of lines > before. Good catch, that line is just removed now. > > @@ +700,3 @@ > + if ((info->input->capabilities & > + (INPUT_IS_SYSTEM_INTEGRATED | INPUT_IS_SCREEN_INTEGRATED)) && > + output_has_input_type (output, info->input->capabilities)) > > Could you split this into multiple conditions? Note this is if (a && b), I've nested the ifs and moved the comment within, so it's hopefully clearer. > > > I meant adding "wacom" in the middle of the function name, not truncating it :) > Sorry that wasn't clear. oh :), changed that
Review of attachment 268914 [details] [review]: Looks good!
Attachment 268863 [details] pushed as 8e488c9 - xrandr: Use GsdDeviceMapper to rotate touscreens Attachment 268864 [details] pushed as 0810de0 - wacom: Use GsdDeviceMapper Attachment 268914 [details] pushed as bf2f0d5 - common: Add GsdDeviceMapper
Carlos, you mentioned g-c-c in your commit. Do these commits fix this related bug? https://bugzilla.gnome.org/show_bug.cgi?id=723930
*** Bug 723930 has been marked as a duplicate of this bug. ***
(In reply to comment #37) > Carlos, you mentioned g-c-c in your commit. Do these commits fix this related > bug? > https://bugzilla.gnome.org/show_bug.cgi?id=723930 Indeed it was, thanks for noticing that one
*** Bug 720722 has been marked as a duplicate of this bug. ***
*** Bug 702952 has been marked as a duplicate of this bug. ***
*** Bug 624880 has been marked as a duplicate of this bug. ***
*** Bug 652247 has been marked as a duplicate of this bug. ***