[2/2 v2] winex11: Use XIAllDevices to select for XI_RawMotion

[Carlos Garnacho]

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 at gmail.com> wrote:
> On 18 November 2016 at 12:43, Carlos Garnacho <carlosg at 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