On 18 November 2016 at 12:43, Carlos Garnacho carlosg@gnome.org wrote:
Change the strategy used to get raw motion from slave devices. Instead of selecting for XI2 events for every slave device individually, do it for XIAllDevices, and store the current device's relative X/Y valuators so they can be quickly looked up in the XI_RawMotion events received.
I think this makes sense, and in my (limited) testing this correctly handles the case of attaching a new input device. I do have a few minor comments though:
+static void update_relative_valuators(XIAnyClassInfo **valuators,
int n_valuators)
This should probably just be "static void update_relative_valuators( XIAnyClassInfo **valuators, int n_valuators )"
- free( thread_data->x_rel_valuator );
- free( thread_data->y_rel_valuator );
...
thread_data->x_rel_valuator = malloc( sizeof(XIValuatorClassInfo) );memcpy ( thread_data->x_rel_valuator, class, sizeof(XIValuatorClassInfo) );
We generally avoid malloc()/free() in Wine code and use HeapAlloc()/HeapFree() instead. The main consideration has to do with the way address space is allocated for Win32 applications, effectively making malloc() space a bit more scarce than HeapAlloc() space. We'd probably prefer the sizeof as "sizeof(*thread_data->x_rel_valuator)", or perhaps "sizeof(*class)".
I'm not sure how much we should worry about this in practice, but when multiple slaves are attached to the master, and they're not completely idle, update_relative_valuators() can end up getting called quite a bit. It doesn't seem hard to at least do the allocation only once, and only update the data in update_relative_valuators(). Perhaps it's also fine the way it is though.
- XISetMask( mask_bits, XI_ButtonPress );
This was added intentionally (see also https://bugs.winehq.org/show_bug.cgi?id=27522#c18). Perhaps it's no longer needed, but that would need to be a separate patch.
- pointer_info = pXIQueryDevice( data->display, data->xi2_core_pointer, &count );
- update_relative_valuators( pointer_info->classes, pointer_info->num_classes );
- pXIFreeDeviceInfo( pointer_info );
...
- pXISelectEvents( data->display, DefaultRootWindow( data->display ), &mask, 1 );
I'm not too concerned about it, but can this race? I.e. switching slaves between when update_relative_valuators() is called above and when XISelectEvents() takes effect. Applies to querying the devices list as well.
- if (thread_data->xi2_current_slave == 0)
We'd typically write that "if (!thread_data->xi2_current_slave)", although I don't think anyone would be overly concerned about it.
- for (i = 0; i <= max ( x_rel->number, y_rel->number ); i++)
- {
+»······if (!XIMaskIsSet( event->valuators.mask, i )) continue; +»······val = *values++; +»······if (i == x_rel->number) +»······{
input.u.mi.dx = dx = val;+»······ if (x_rel->min < x_rel->max)
input.u.mi.dx = val * (virtual_rect.right - virtual_rect.left)/ (x_rel->max - x_rel->min);+»······} +»······if (i == y_rel->number) +»······{ +»······ input.u.mi.dy = dy = val; +»······ if (y_rel->min < y_rel->max)
You're using tabs for indentation here.
Hi Henri,
Sorry it took a while to get back to this patch.
On Mon, Nov 21, 2016 at 2:08 PM, Henri Verbeet hverbeet@gmail.com wrote:
On 18 November 2016 at 12:43, Carlos Garnacho carlosg@gnome.org wrote:
Change the strategy used to get raw motion from slave devices. Instead of selecting for XI2 events for every slave device individually, do it for XIAllDevices, and store the current device's relative X/Y valuators so they can be quickly looked up in the XI_RawMotion events received.
I think this makes sense, and in my (limited) testing this correctly handles the case of attaching a new input device. I do have a few minor comments though:
+static void update_relative_valuators(XIAnyClassInfo **valuators,
int n_valuators)This should probably just be "static void update_relative_valuators( XIAnyClassInfo **valuators, int n_valuators )"
- free( thread_data->x_rel_valuator );
- free( thread_data->y_rel_valuator );
...
thread_data->x_rel_valuator = malloc( sizeof(XIValuatorClassInfo) );memcpy ( thread_data->x_rel_valuator, class, sizeof(XIValuatorClassInfo) );We generally avoid malloc()/free() in Wine code and use HeapAlloc()/HeapFree() instead. The main consideration has to do with the way address space is allocated for Win32 applications, effectively making malloc() space a bit more scarce than HeapAlloc() space. We'd probably prefer the sizeof as "sizeof(*thread_data->x_rel_valuator)", or perhaps "sizeof(*class)".
I'm not sure how much we should worry about this in practice, but when multiple slaves are attached to the master, and they're not completely idle, update_relative_valuators() can end up getting called quite a bit. It doesn't seem hard to at least do the allocation only once, and only update the data in update_relative_valuators(). Perhaps it's also fine the way it is though.
Right, if the user interacts simultaneously with two devices, an XI_DeviceChanged event is expected to be sent each time a device takes over the VCP, at a rate determined by the devices themselves.
So this is indeed not a place for heavy operations, but I would be surprised if memory allocation overhead itself gets that much noticeable (this rate being usually hertzs or kilohertzs). Anyhow the data we want from XIValuatorClassInfo is simple enough to just store it in fixed structs, I've gone that way in the v3 patch.
- XISetMask( mask_bits, XI_ButtonPress );
This was added intentionally (see also https://bugs.winehq.org/show_bug.cgi?id=27522#c18). Perhaps it's no longer needed, but that would need to be a separate patch.
Oh I see. I tried to reproduce that on top of my patch to no avail, I've anyway added it back in v3, seems harmless enough and just in case this is related to certain Xserver versions.
- pointer_info = pXIQueryDevice( data->display, data->xi2_core_pointer, &count );
- update_relative_valuators( pointer_info->classes, pointer_info->num_classes );
- pXIFreeDeviceInfo( pointer_info );
...
- pXISelectEvents( data->display, DefaultRootWindow( data->display ), &mask, 1 );
I'm not too concerned about it, but can this race? I.e. switching slaves between when update_relative_valuators() is called above and when XISelectEvents() takes effect. Applies to querying the devices list as well.
Right, there is this brief window, I've inverted the event selection and device querying steps, that should ensure there's at least an XI_DeviceChanged event on the wire even if the device query is instantly outdated.
Cheers, Carlos
-
Carlos Garnacho -
Henri Verbeet