Fix tablet jitter
Closed, ResolvedPublic

Description

Xinput2 bug report: https://bugs.freedesktop.org/show_bug.cgi?id=92186

Qt bug report: https://bugreports.qt.io/browse/QTBUG-48151

Patch fixing jitter: https://codereview.qt-project.org/#/c/110334/


Workaround library (built/tested on Kubuntu 15.10)
https://dl.dropboxusercontent.com/u/16058153/Krita/libqxcb.so

cd /usr/lib/x86_64-linux-gnu/qt5/plugins/platforms
sudo mv libqxcb.so libqxcb.so.old
sudo cp ~/Downloads/libqxcb.so .

Prospect for a return to handling our own tablet events.

Windows:

Update Spent a little while trying to do a basic port, I ran into an unexpected situation: it seems like Qt creates an invisible phantom window to receive tablet events. I do not understand the implications of this, so it put me off. (See QWindowsTabletSupport::create()) I do not understand the implications of this, maybe it can be circumvented by creating a second tablet event window, or some other wintab calls. The window itself can be disabled with the configuration -no-feature-tabletevent, but that builds Qt without QTabletEvent support entirely.

is contained in qwindowstabletsupport.cpp, a file of about 500 lines, and which seems very similar to the Qt4 version code.
The main benefit here is to include our own support of badly behaved tablets, such as the Surface Pro 3. My opinion is that the best option would be to Option B for this is to simply throw a patch on top of Qt and distribute the patched .dll with Krita. There is no restriction to doing this. It also becomes possible to distribute upstream. The drawback is that Krita Windows developers would have to apply this patch themselves, or wait for our changes to be merged upstream.
The other option is to merge their new code with our previous Windows handling code, which is still included in the repo, just disabled. Maybe ecloud could weigh in on how much he cares about upstreaming any tablet fixes we make. If he would shepherd patches for us, that would make it less of a burden.

Linux:

Qt has "upgraded" to handling X calls exclusively through XCB. This means the existing customized code would have to be rewritten for XCB. The epicenter, where everything we know and love has been destroyed, is that QAbstractNativeEventFilter() now sends xcb events, not xlib. (Side note/rant: xcb-xinput is flagged upstream as unstable, buggy API. Unfortunately we have no choice but to live with it.) Compared to Windows, this is far messier and more difficult. However, there are some very severe tablet handling issues that we probably have no ability to fix without doing this

Now, on the one hand, it is quite possible to rewrite the old tablet handler code as xcb code. This is because, a vast majority of the time, an X call can be written equivalently as two xcb calls, like "xcb_request_info + xcb_wait_for_response." Most of the time, the arguments are basically unchanged. So even though xcb-xinput is undocumented, (buggy, unstable) API, most of the time all you have to do is look up the corresponding Xlib call to figure out what the arguments are doing.

The bigger problem is that the Qt internal API has changed. There is a huge delta between what Qt is actually doing, and the old code. There is one interesting consideration: the format of QTabletEvent has changed. Now, that's not necessarily an argument against porting our old code. The new QTabletEvent now looks a lot like the old KisTabletEvent, which means porting the old code would be a little easier.

The other option is to extract the new Qt XCB code and modify it. We can then forward-port the individual fixes contained in the Xlib version, if they are still necessary. It may be possible to extract this tablet handling code, then register our *own* handler, which steals all tablet XCB events from the Qt processing loop.

The main difficulty in porting is that tablet events are mixed in with *all* xcb events. The primary location where events are handled is qtcore/plugins/platforms/xcb/qxcbconnection_xi2.cpp, and the function xi2HandleTabletEvent. See in particular around 989, which points out where proximity events could be handled, but currently aren't.

My guess is that the is to go with the latter, that is, to disregard the old handler code and start from scratch by copying the current Qt handler code and then modifying it. However neither way sounds fun.

Regarding trying to restore KisTabletEvent: I don't think it brings any benefits. As far as I understood, the main justification for KisTabletEvent was to carry extended button masks, which QTablet now supports.

Currently the main limitation of QTabletEvent is that there is no way to reliably signal a hover event. I am not sure whether hover events themselves get sent reliably. If they are, perhaps we can just fall back on the old ProximityNotifier, and not worry about altering the event loop more than necessary. However, even if we want events themselves to carry hover information we don't necessarily require a new class: my first inclination is to try registering a new event type QEvent::TabletHover, and simply create QTabletEvents with type = TabletHover.

rempt created this task.Aug 10 2015, 7:23 AM
rempt updated the task description. (Show Details)
rempt raised the priority of this task from to High.
rempt added a project: Krita.
rempt moved this task to Krita Frameworks on the Krita board.
rempt added a subscriber: rempt.
rempt added a comment.Aug 10 2015, 1:54 PM

Tablet support on Qt5

  • Qt5 uses XInput2, which is different to how we used to precess the events before.
  • It might happen that our hacks are not needed to Qt5 anymore and it works fine now

We have two ways forward:

  • Recover our own hack. It is dirty, but will not disturb users much.
    1. Ensure we cut off the Qt's internal flow of events. Since it uses different kind of API, it much happen that both Krita and Qt get the duplicated events.
    2. Initialize XInput properly. Afair, XInput required explicit subscribing for custor tablet events (see evdevEventsActivationWorkaround(), it should be called for any tablet now, not only 'evdev; ones).
  • Switch to the main Qt5 events flow. Not sure how much time it will take, but we should at least plan this.
rempt added a comment.Aug 10 2015, 1:56 PM

What's the easiest way to disable our event hacks? Just this:

diff --git a/krita/main.cc b/krita/main.cc
index 3112558..ff77dce 100644

  • a/krita/main.cc

+++ b/krita/main.cc
@@ -156,17 +156,6 @@ extern "C" int main(int argc, char **argv)

    }
}

-#if defined Q_OS_WIN

  • KisTabletSupportWin tabletSupportWin;
  • tabletSupportWin.init();
  • app.installNativeEventFilter(&tabletSupportWin);

-#elif defined HAVE_X11

  • KisTabletSupportX11 tabletSupportX11;
  • tabletSupportX11.init();
  • app.installNativeEventFilter(&tabletSupportX11);

-#endif

  • - #if defined HAVE_OPENGL KisOpenGL::initialize(); #endif

Doesn't do it. Calling evdevEventsActivationWorkaround() unconditionally also doesn't seem to do anything:

diff --git a/krita/ui/input/wintab/kis_tablet_support_x11.cpp b/krita/ui/input/wintab/kis_tablet_support_x11.cpp
index ee95e0a..8d06395 100644

  • a/krita/ui/input/wintab/kis_tablet_support_x11.cpp

+++ b/krita/ui/input/wintab/kis_tablet_support_x11.cpp
@@ -779,7 +779,7 @@ bool KisTabletSupportX11::nativeEventFilter(const QByteArray &/*eventType*/, voi

    return true;
}
  • if (kis_haveEvdevTablets && event->type == EnterNotify) {

+ if (/*kis_haveEvdevTablets && */event->type == EnterNotify) {

    evdevEventsActivationWorkaround((WId)event->xany.window);
}
rempt added a subscriber: dkazakov.Aug 10 2015, 1:56 PM

On Windows, I actually get close to reasonable behavior when commenting out the tabletSupportWin.

Ripping KisInputManager to shreds sounds very cathartic. I'm going to give it a shot.

Well, there are some problems with Qt's tablet support, especially when it comes to proximity events: https://bugreports.qt.io/browse/QTBUG-47702. So I'm not sure we can make do without our own code. Here's a bit of irc discussion from today with a Qt developer:

12:26:58 < dmitryK> ecloud: hi! seems like the idea of not sending tablet move events for hover mode has major

drawbacks.

12:27:30 < ecloud> yeah maybe we should... I'm not sure if there's some historical reason that we don't
12:28:07 < ecloud> for the Art Pen, it would anyway be nice to have continuous rotation, to be able to rotate the

cursor even when you are not pressing

12:28:43 < dmitryK> ecloud: the point is (probably for some drivers only) synthesized MousePress comes earlier

than a TabletPress, so basically you cannot start a stroke without a pressure==1.0 blob in the
beginning

12:30:24 < dmitryK> ecloud: and I'm not sure I can filter out these mouse press event by Proximity events. At

least in Qt 4.8 they might not come if the app was switched into while the sylus had already
been in proximity

12:32:27 < dmitryK> ecloud: hm... funny, it seems like in Qt5 EnterLeave proximity are coming even when the

application is minimized

12:34:53 < Animtim> ecloud: not only for Art pen, rotation can come from tilt from stylus that support it, r even

direction for any stylus..

12:35:38 < Animtim> (just my 2 cents, not sure of the discussion context)
12:35:39 < ecloud> dmitryK: well we have https://bugreports.qt.io/browse/QTBUG-47702
12:35:49 < ecloud> tablet events should come before mouse events
12:37:46 < ecloud> ok feel free to write up a couple more bugs: we need the tablet moves, and we need to stop

sending proximity when the window isn't visible, right?

12:38:15 < dmitryK> ecloud: well, proximity events are actually very useful
12:38:26 < dmitryK> ecloud: so I'd call it a 'feature'
12:38:28 < ecloud> even when minimized?
12:38:31 < ecloud> ok
12:38:33 < dmitryK> ecloud: yes
12:38:37 -!- Matthi [~Matthi@212.23.140.110] has joined Krita
12:40:00 < dmitryK> ecloud: well, it is disputable of course, but at least it allows us not to add workarounds

that try to recover tablet state at Enter/Leave and FocusIn/Out

Note from dmitry on irc: on windows, it should be enough to disable the windows workaround part in main.cc.

abrahams added a comment.EditedAug 21 2015, 8:46 AM

Before this can be solved, we need to really be sure: should we use our own handlers?

Pros:

  • Handler is working pretty well on Windows now.
  • We can ensure the flow of events is sane.
  • We get to look over every last bit of incoming data, e.g. high resolution location.
  • We do not have to wait for Qt to fix bugs for us.
  • The existing workaround D256 is not a perfect solution, we will have to do more work either way.
  • Qt does not support non-Wacom tablets, and if our workarounds can't be merged upstream, we will have to write event handlers regardless.

Cons:

  • Taking on the huge burden of cross-platform event handling, which should really be Qt's job.
  • Linux handler would need to be rewritten for XCB. Unfortunately, the code is nearly as complex as XCB's documentation. rimshot
  • Small binary incompatibility risks from linking against wintab/xinput/xcb/whatever OSX uses/etc.
  • If we are discovering errors it is better for the Qt ecosystem to push as much as possible upstream.
abrahams updated the task description. (Show Details)EditedAug 21 2015, 9:54 AM

Turns out the Qt bug was not a Qt bug but a Wacom settings bug! It decided to apply a weird screen mapping just to Krita when I imported my settings file for the Windows 10 upgrade.

rempt added a comment.Aug 25 2015, 9:42 AM

I think we should push the tablet patches as we have them now, and declare 3.0. After the next 2.9 release, feature development should move to frameworks, too.

I wrote a patch D270 that takes a different route from Dmitry's. Instead of adding more event transformation logic to KisInputManager it tries to remove as much of it as possible. The patch works well in Windows but it's still not perfect in Linux.

Well tentative tablet repair is now on master: it broke up into 11 patches, counting the file split. The first few patches are more like unambiguous improvements (things like defining a tablet debug logging category) and the later ones include heavier changes.

rempt added a comment.Sep 5 2015, 7:06 AM

With the testing branch, the results are much better, though every line seems to start with a full-pressure blob, which looks like it might be a mouse event. Tablet log attached.

For me the offset is fixed, I get no blob(but I do have a lower version of qt than boud), and the wobbliness is much better, but not as smooth as on 2.9.

Chopping off the first mouse press event should be very straightforward - I can't see the tablet log, could you try posting it again?

I'm getting wobbly choppy lines too. I performed the following experiment. I took an ordinary canvas and zoomed it all the way to the smallest setting, and then tried to draw a tiny circle on my canvas. Then I booted into Windows and did the same thing. Same hardware, both built from the current master branch.

The result on Linux is like this:


On Windows, it is like this:

So my interpretation is that something is going wrong in the Linux xorg/qt stack. My best guess is that the RawSample setting is not working correctly. When I edit RawSample I notice no difference. http://doswa.com/2010/01/01/wacom-jitter-fix-for-linux.html

Turning on the smoother should give similar results to a nonzero RawSample setting. Should we ask Qt about this? Linux-wacom?

rempt added a comment.Sep 7 2015, 7:39 AM

Let's see if this works:

That upload worked for me. Looks like luckily it is in fact just a timing issue. I just pushed a patch that will snatch the first mouse button press. The master branch is not compiling for me right this minute but it worked fine on my local branch.

Here is a "smoking gun" regarding the tablet driver regression.

  • In Calligra 2.9, configuring the RawSample parameter in the driver will make the tablet jittery or smoother.
  • In Calligra 3.0, configuring the RawSample parameter in the driver does nothing.
abrahams updated the task description. (Show Details)Sep 7 2015, 6:44 PM

Copied the info in this discussion into a Qt bug report, link in description.

Here's an update from the bug report. We have identified the issue in Qt, and I'm able to get a nice smooth drawing with the patch. I am not sure how dissemination of the fix will occur.

rempt added a comment.Sep 12 2015, 7:59 AM

Okay! The usual thing is to carry the patch in our git repo. If we detect a version of Qt that older than the first released version of Qt with the patch, we'll let cmake warn about it. Distributions will apply the patch, and we do the same for the builds we ship for Windows and OSX. We used to do that for Words for a long time, too.

And when there is a shipped version of Qt with the patch, we can even update the minimum Qt version to that.

abrahams updated the task description. (Show Details)Sep 12 2015, 7:30 PM

Awesome. I added a link to the patch in the description. I'm happy to help with the packaging stuff if you'd like, but I have literally no experience so I'd need guidance. It would be very cool if we could get this into Kubuntu 15.10 and Fedora 23.

rempt added a comment.Sep 14 2015, 7:32 AM

Just checking: the patch applies against Qt 5.5 and Qt 5.6 and we're aiming for inclusion by default in 5.7?

abrahams added a comment.EditedSep 14 2015, 7:25 PM

Yes, it applies against 5.5 and 5.6 beta. As for inclusion I was optimistically hoping for something sooner like 5.5.1 and 5.6.0 - the patch is really a bug fix more than anything.

There is an unhappy twist to this story. Apparently the but is not in Qt, instead it is in Xorg/Xinput2. The Qt patch which fixes the error is not a patch to faulty behavior in Qt, but rather it is a workaround to avoid the buggy Xinput2 scaling. That means:

  1. A complete solution will have to happen in Xorg,
  2. The patch in Qt will have to be made optional,
  3. There may be unforeseen problems with circumventing Xorg's scaling code e.g. with multiple monitors, DPI, and so on.

Link to the bug at freedesktop is in the description.

abrahams updated the task description. (Show Details)Oct 5 2015, 8:18 PM
rempt added a comment.Oct 6 2015, 7:12 AM

Okay... Let's see where this gets to in the coming month or so.

Okay, I have created a patched build on Ubuntu 15.10 using apt-get source and rebuilding the package with the patch. On my machine, replacing the existing xcb library with the new one will fix the tablet issue. The new library is several megabytes because it includes debugging info.

https://dl.dropboxusercontent.com/u/16058153/Krita/libqxcb.so

cd /usr/lib/x86_64-linux-gnu/qt5/plugins/platforms
sudo mv libqxcb.so libqxcb.so.old
sudo cp ~/Downloads/libqxcb.so .

Please don't come after me if your Intuos bursts into flame, all of your data is deleted, or your DVD tray starts devouring orphans.

abrahams updated the task description. (Show Details)Oct 8 2015, 2:14 AM
abrahams lowered the priority of this task from High to Low.Oct 18 2015, 5:17 AM

I checked in on the bug report on freedesktop, Peter Hutterer confirmed that the issue originates in Xorg. Since there's likely nothing to do here but sit and wait, let's call this low priority.

well, there's still two other issues:

  1. The proximity-states don't work. This means that the tilt-cursor doesn't work when hovering, nor does the dab-rotation indicator for the tilt and rotation sensors.
  1. There's still that weird thing where the stroke just randomly jumps to a corner of the image. This is completely random, so difficult to track, but probably frequent enough to annoy users.

Alright, this task got pretty derailed with the jitter issue, so we can reorganize.

abrahams renamed this task from Fix tablet handling to Fix tablet jitter.Oct 20 2015, 2:39 AM
abrahams updated the task description. (Show Details)Nov 6 2015, 10:53 PM
abrahams updated the task description. (Show Details)Nov 6 2015, 10:58 PM
abrahams raised the priority of this task from Low to Normal.
abrahams updated the task description. (Show Details)Nov 10 2015, 4:37 AM
abrahams updated the task description. (Show Details)Nov 10 2015, 4:43 AM
abrahams closed this task as Resolved.Dec 17 2015, 7:35 PM
abrahams claimed this task.