Add tilt support to DFT processor #70
Conversation
src/daemon/dft.cpp
Outdated
| yt *= ctx.config.height / (ctx.config.dft_tilt_distance * 10); | ||
|
|
||
| auto azm = std::max(0., std::fmod(std::atan2(-yt, xt) / M_PI + 2, 2)) * 18000; | ||
| auto alt = (.5 - std::acos(std::min(1., std::hypot(xt, yt))) / M_PI) * 18000; |
There was a problem hiding this comment.
Nitpick: I would prefer if these floats were fully typed out (0.5 instead of .5). Makes things a bit easier to read IMHO.
There was a problem hiding this comment.
I just realized I can change acos to asin, then the .5 isn't even needed. I'll change 0. and 1. to 0.0 and 1.0 though.
There was a problem hiding this comment.
Maybe i'm wrong but if config.width/height is now in cm, then is no need to multiply by 10, and the correct formula is:
xt *= ctx.config.width / ctx.config.dft_tilt_distance;
There was a problem hiding this comment.
dft_tilt_distance is in mm. Should I change it to cm for consistency?
There was a problem hiding this comment.
Sorry i thought that dft_tilt_distance is in cm. My bad.
There was a problem hiding this comment.
dft_tilt_distance is in mm. Should I change it to cm for consistency?
I think that would be a good idea.
There was a problem hiding this comment.
I'm somewhat wondering whether we shouldn't change things to mm everywhere... could avoid some floating-point inaccuracies if mm is the finest scale we're working with. On the other hand, I think everywhere the constants are applied we're already working with floating-point types, so I guess it doesn't make that much of a difference.
|
Thank you! The code looks good to me. |
|
I tried the current version without any modification. In the screencast: https://fileupload.ggc-project.de/download/e60b69cd7bf718e1/#djIuYh0HI0gQOa1YnZkFeQ , you can see the difference between iptsd with and without tilt support. Is there anything I could change to maybe reduce the noise or any data I could provide? |
|
That's kind of what I was afraid of. The problem is that |
|
I tried this patch with |
|
Just a heads up, I just pushed a large set of changes to the master branch. I didn't touch the DFT code, but two other changes impact this PR:
|
|
@lxdb @freak007 @StollD I think it would be better to keep the width/height in config, and only use the drm values if the user has not set them explicitly. I was also planning to add support for the metadata feature report (see linux-surface/intel-precise-touch#4 (comment)), which contains the exact width/height, and also whether axes should be inverted. Adding support for this would also be easier if width/height are kept in the config class. |
Yeah, it comes from the EDID, and there its rounded to centimeters
That report looks really nice. Kinda annoys me that there is so much shiny HID stuff on the newer devices, and my SB2 needs all these hacks :P Moving it back to the config seems like the better idea then. Althought I am not sure if we should keep the DRM values at all then. Having three possible sources seems like a bit of a mess, especially if we only need two (config and feature report) for surface devices. |
|
I think it's fine to have a fallback to the EDID values for the older devices. You could even get rid of the libdrm dependency by just reading the EDID directly: But since you still need the config files for older devices for InvertX/Y, maybe the fallback wouldn't be that useful. |
…} (e.g. when both arguments to pow are negative)
|
FWIW I finally found some time to test this myself. Nice work! Math seems to be correct, tilt does work (at least with my quick test in krita), but as others mentioned it's a bit noisy. But that's a common theme that we'll have to work out in pretty much every other part of IPTSd as well. |
| @@ -63,6 +63,9 @@ iptsd_dft_interpolate_position(const Context &ctx, const struct ipts_pen_dft_win | |||
| // find critical point of fitted parabola | |||
| f64 d = (x[0] - x[2]) / (2 * (x[0] - 2 * x[1] + x[2])); | |||
|
|
|||
| if (std::isnan(d)) | |||
There was a problem hiding this comment.
We are compiling iptsd with -ffast-math, which if I understand it correctly will make the compiler take some shortcuts, that can break NAN handling. So this might not actually be safe. Maybe its possible to catch the situation that would result in a NAN earlier?
(also I just noticed that I forgot to remove the NAN returns from iptsd_dft_interpolate_frequency, whoops)
There was a problem hiding this comment.
You're correct: -ffast-math implies -ffinite-math-only, so inf and nan cannot be relied upon. Even if you explicitly set a = std::nan somewhere, I think the compiler can choose to optimize out a std::isnan(a) due to -ffinite-math-only assuming there are only ever valid finite numbers.
There was a problem hiding this comment.
I've always thought that isnan should work even with -ffast-math/-ffinite-math-only, although I do know it is broken on older gccs.
Personally though, I think -ffast-math/-ffinite-math-only should never be used. The actual performance benefit tends to be minimal or non-existent. If there is a performance benefit, you can generally achieve it without -ffast-math by rearranging the offending expressions manually. NaNs and infinities are a core part of floating point math and pop up everywhere; if you really want to avoid them you will have to put validation everywhere (for essentially no real benefit).
There was a problem hiding this comment.
Personally though, I think -ffast-math/-ffinite-math-only should never be used. The actual performance benefit tends to be minimal or non-existent.
True, the benefits may not exceed the drawbacks, but the alternative at the moment is hand-optimization and I'm not sure if we're at that stage already. Ideally we'd have a benchmark or performance comparison for this. On the other hand, I'm wondering whether the check above should be an abs() check on the divisor instead.
There was a problem hiding this comment.
The most recent debate on -ffast-math/-ffinite-math-only and NaNs that I could find is from 2021 for clang/llvm, discussing whether isnan should be optimized out (which it seems it was at that point): https://lists.llvm.org/pipermail/llvm-dev/2021-September/152530.html. Not really sure what the conclusion of that was, but 2021 isn't that long ago so I think we should avoid isnan and the likes with -ffast-math.
There was a problem hiding this comment.
For what its worth, I would be fine with just removing -ffast-math. But idk how much impact it would have on perfomance.
There was a problem hiding this comment.
I'd guess for pen processing there shouldn't be much? The biggest impact will probably be in the touch processing. If there isn't any noticeable impact, we should probably disable that for now (or maybe only -ffinite-math-only?). I'd argue we're not really at the performance-optimization stage yet anyways.
There was a problem hiding this comment.
I did a very quick and dirty benchmark. Basically recorded ~1 minute of multitouch data, and ~1 min of pen data, and modified iptsd to parse it 100 times. I'm using gcc 11.3. Below is the user/sys time in seconds for various builds (I've listed sys because fast-math seems to have a small effect on it for some reason):
| benchmark | touch user | touch sys | pen user | pen sys |
|---|---|---|---|---|
| debugoptimized with fast-math | 33.646 | 15.626 | 13.604 | 25.386 |
| release with fast-math | 28.840 | 15.509 | 12.937 | 24.428 |
| release w/o fast-math | 30.395 | 16.124 | 13.068 | 24.250 |
| release w/o fast-math, with LTO | 21.133 | 16.112 | 12.224 | 24.283 |
So the optimization options don't really seem to affect the DFT processing. The touch processing becomes only about 5% slower without -ffast-math. And the userspace code becomes about 30% faster if you enable LTO (b_lto=true). So IMO it's fine to disable fast-math and probably a good idea to enable LTO.
There was a problem hiding this comment.
Thank you! I also did some benchmarks, and they pretty much show the same results as yours, so I pushed the changes.
There was a problem hiding this comment.
Great, thanks. I think this can be merged now.
…on correction by default
|
Was about to ask if you think this can be merged, only to see that you already answered that. Thank you! |
See quo#3 (comment).
The basic idea is that there is a secondary transmitter about 6mm behind the primary transmitter in the pen. Tilt is calculated from the difference in positions of the signals from the two transmitters. I assume the distance between the transmitters is 6mm for all pens, but it's possible that the distance is encoded in the unknown binary signals somehow. Note that tilt data is only transmitted when the pen is touching the screen, not while hovering.
Additionally, if the distance between the tip of the pen and the primary transmitter is known, the tilt data can be used to calculate the actual tip position. I've guessed this distance to be 2mm based on behavior of my non-tilt pen, and added a correction for it, but the distance may be slightly different for each pen. Since tilt is not transmitted while hovering, with this correction the cursor will shift slightly when the pen starts or stops touching the screen, which may be undesirable.
Since I don't have a pen with tilt support myself, someone who does (@qzed?) should test these changes before they are merged. (I hope I got the azimuth/altitude calculations right, but it's very possible the azimuth will need to be mirrored or rotated further.)
For the tip position correction, please check if the 2mm value works, or if you need to adjust this up or down. Also, please compare the behavior with the tip distance set to 0, especially with things like slow diagonal movements. Is the additional noise from the tilt data (that is added to the position when tip distance is nonzero) noticeable?