Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tilt support to DFT processor #70

Merged
merged 5 commits into from
Aug 30, 2022
Merged

Add tilt support to DFT processor #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
05b85e5
Add tilt support to DFT processor
quo Aug 20, 2022
850cd45
DFT tilt: config.width/height is now in cm
quo Aug 24, 2022
0f08b6f
DFT tilt: simplify azimuth/altitude calculations
quo Aug 27, 2022
d7cab17
DFT: make sure iptsd_dft_interpolate_position never returns {true,NAN…
quo Aug 27, 2022
e2ecfa0
DFT tilt: change distance config values to cm, and disable tip positi…
quo Aug 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/config/config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,15 @@ static int parse_conf(void *user, const char *c_section, const char *c_name, con
if (section == "DFT" && name == "FreqMinMag")
config->dft_freq_min_mag = std::stoi(value);

if (section == "DFT" && name == "TiltMinMag")
config->dft_tilt_min_mag = std::stoi(value);

if (section == "DFT" && name == "TiltDistance")
config->dft_tilt_distance = std::stof(value);

if (section == "DFT" && name == "TipDistance")
config->dft_tip_distance = std::stof(value);

return 1;
}

Expand Down
3 changes: 3 additions & 0 deletions src/config/config.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ class Config {
f32 dft_position_exp = -0.7;
u16 dft_button_min_mag = 1000;
u16 dft_freq_min_mag = 10000;
u16 dft_tilt_min_mag = 10000;
f32 dft_tilt_distance = 0.6;
f32 dft_tip_distance = 0;

public:
Config(i16 vendor, i16 product);
Expand Down
46 changes: 45 additions & 1 deletion src/daemon/dft.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Member

@StollD StollD Aug 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@StollD StollD Aug 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what its worth, I would be fine with just removing -ffast-math. But idk how much impact it would have on perfomance.

Copy link
Member

@qzed qzed Aug 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I also did some benchmarks, and they pretty much show the same results as yours, so I pushed the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks. I think this can be merged now.

return std::tuple {false, 0};

return std::tuple {true, row.first + maxi + std::clamp(d, mind, maxd)};
}

Expand Down Expand Up @@ -129,7 +132,7 @@ static f64 iptsd_dft_interpolate_frequency(const Context &ctx, const ipts::DftWi
static void iptsd_dft_handle_position(Context &ctx, const ipts::DftWindow &dft,
ipts::StylusData &stylus)
{
if (dft.rows <= 0) {
if (dft.rows <= 1) {
iptsd_dft_lift(stylus);
return;
}
Expand Down Expand Up @@ -160,6 +163,47 @@ static void iptsd_dft_handle_position(Context &ctx, const ipts::DftWindow &dft,
if (ctx.config.invert_y)
y = 1 - y;

if (dft.x[1].magnitude > ctx.config.dft_tilt_min_mag &&
dft.y[1].magnitude > ctx.config.dft_tilt_min_mag) {

// calculate tilt angle from relative position of secondary transmitter

auto [pxt, xt] = iptsd_dft_interpolate_position(ctx, dft.x[1]);
auto [pyt, yt] = iptsd_dft_interpolate_position(ctx, dft.y[1]);

if (pxt && pyt) {

xt /= dft.dim.width - 1;
yt /= dft.dim.height - 1;

if (ctx.config.invert_x)
xt = 1 - xt;

if (ctx.config.invert_y)
yt = 1 - yt;

xt -= x;
yt -= y;

if (ctx.config.dft_tip_distance) {
// correct tip position using tilt data
auto r = ctx.config.dft_tip_distance / ctx.config.dft_tilt_distance;
x -= xt * r;
y -= yt * r;
}

xt *= ctx.config.width / ctx.config.dft_tilt_distance;
yt *= ctx.config.height / ctx.config.dft_tilt_distance;

auto azm = std::fmod(std::atan2(-yt, xt) / M_PI + 2, 2) * 18000;
auto alt = std::asin(std::min(1.0, std::hypot(xt, yt))) / M_PI * 18000;
stylus.azimuth = gsl::narrow<u16>(std::round(azm));
stylus.altitude = gsl::narrow<u16>(std::round(alt));

}

}

x = std::round(std::clamp(x, 0.0, 1.0) * IPTS_MAX_X);
y = std::round(std::clamp(y, 0.0, 1.0) * IPTS_MAX_Y);

Expand Down