Find and use a system einspline
Needs RevisionPublic
Actions

Authored by pino on Dec 28 2016, 11:15 AM.

Details

Reviewers

dkazakov
rempt

Group Reviewers

Krita

Summary

Add a cmake module to look for the einspline library, using it if found
instead of the embedded copy of it (which is stil used otherwise).

Update README_PACKAGERS.md accordingly.

Test Plan

Try building krita with and without a system einspline, works in both
cases.

Diff Detail

Repository

R37 Krita

Branch

external-einspline

Lint

No Linters Available

Unit

No Unit Test Coverage

pino updated this revision to Diff 9410.Dec 28 2016, 11:15 AM

pino retitled this revision from to Find and use a system einspline.

pino updated this object.

pino edited the test plan for this revision. (Show Details)

pino added reviewers: Krita, rempt.

Hi Pino,

I'm not sure whether this will work correctly -- we did have to make some changes to the library that we couldn't upstream, because it was abandoned in 2008, starting with ad597172fde95af37d6002e72364415bfc9317cd , which fixes build issues but also a memory leak.

In D3831#71752, @rempt wrote:

we did have to make some changes to the library that we couldn't upstream, because it was abandoned in 2008

From what I see, there were commits upstream until 2011, and some of those commits fix some of the issues you had to fix in your copy (for example r429 and r442 in the upstream SVN).

which fixes build issues

Do you have more references to these? I just checked the einspline 0.9.2 source in Debian, and it builds fine with no patches (the only patches there touch the documentation).

also a memory leak.

The free/delete in destroy_NUBspline? IMHO it should be done rather in destroy_Bspline -- but that's something that applies to the upstream library as well, and IMHO if you could report the fix with my suggestion to the upstream bug tracker (on sourceforge), distros could backport it.

Also another chunk of the changes done in krita were the conversion of the sources to C++, as old MSVC (2010) did not support C99 properly; some of the changes associated to that conversion were reverted back.

So it looks to me that almost all the changes you need were to make the einspline code build as embedded within the krita sources, and thus that they are less of a concern when einspline is already built as distribution source. The memory leak is something I'd like to see also downstream. Is there anything else I'm missing?

Yes, I did quite a few fixes, including a memory leak and Windows builds. If you know anyone maintaining the library, I can try to recover those fixes. But we still have to maintain a copy of it for Windows and OSX builds I guess.

Well, if those fixes are in the upstream library, then we can simply use that -- make a 3rdparty module for it and build it that way. It would be cleaner -- because apparently we've already missed some development since we forked it.

There is one more problem actually. The original library uses SSE, and I would really like to avoid using this part. Vector instructions are updated very quickly in both CPUs and compilers, and it *will* cause problems. Using untested and unmaintained library with SSE would just mean that I will have to maintain this part, which I cannot do. Just physically.

If someone stands up and makes an official (not "patched by distributions") version of einspline with SSE chopped off, then it could work. Just dumb linking to an abandoned library is really a bad idea.

Btw, @pino, just running Krita for testing einspline is not enough. You should at least run the following tests and check them with memcheck:

KisBSplinesTest
KisCageTransformWorkerTest
KisLiquifyTransformWorkerTest
KisWarpTransformWorkerTest

In D3831#72004, @rempt wrote:

Well, if those fixes are in the upstream library, then we can simply use that

Most of them were done after the latest version (0.9.2) -- still, that's something packagers can backport to their local versions.

In D3831#72024, @dkazakov wrote:

There is one more problem actually. The original library uses SSE, and I would really like to avoid using this part. Vector instructions are updated very quickly in both CPUs and compilers, and it *will* cause problems.

Can you please expand more on the issues you ran into wrt SSE usage? After all, SSE extensions are already included in x86_64 processors, and software built on that architecture basically assumes it's there.

If someone stands up and makes an official (not "patched by distributions") version of einspline with SSE chopped off, then it could work. Just dumb linking to an abandoned library is really a bad idea.

Not suggesting that, of course. BTW, did you try contacting the original authors? (I'm not finger-pointing here, just want to know more information about the upstream status of einspline).

Btw, @pino, just running Krita for testing einspline is not enough. You should at least run the following tests and check them with memcheck:

KisBSplinesTest
KisCageTransformWorkerTest
KisLiquifyTransformWorkerTest
KisWarpTransformWorkerTest

KisCageTransformWorkerTest is already marked as broken, so I didn't test it. Regarding the other ones, indeed applying the simple memory leak patch to the system copy makes valgrind happier -- follows the version I tested:

P64 einspline memleak patch

1	--- a/src/nubspline_create.c
2	+++ b/src/nubspline_create.c
3	@@ -1084,5 +1084,6 @@ destroy_NUBspline(Bspline *spline)
4	destroy_NUBasis (((NUBspline_3d*)spline)->z_basis);
5	break;
6	}
7	+ free (spline);
8	}
9

If it's OK for you, I can report that upstream, and ask for a backport in Debian.

Sorry to poke on this -- would it be possible to have more feedback on this, especially on some blocks (like "no SEE")?

Hi, @pino!

To get this patch in, we should ensure the following:

Every distribution under active maintenance (not only Debian, but including Ubuntu 14.04) has at least two patches applied:
- memory leaks fixes
- SSE-features disabled

The upstreamed library builds fine on Linux, OSX and Windows.

If there is a volunteer who does it (and de-facto becomes a maintainer of the library), then we can accept the patch. Otherwise, this patch will just push me to become a maintainer of the library, which I cannot do physically, I'm sorry.

As a transitive solution, you can also implement some clever system for recognition of the system library version and whether it was built with SSE and something. That can also work. Right now this patch just breaks Krita on all non-Debian systems, which do not have your leak patch.

This revision now requires changes to proceed.Apr 6 2017, 10:53 AM

@dkazakov
OK -- can you please explain why SSE is problematic and must be disabled?
Also, I provided above one memory leak -- are there any other?

In D3831#100208, @pino wrote:

@dkazakov
OK -- can you please explain why SSE is problematic and must be disabled?

Eispline build system uses all vector features available in build host cpu by default. That creates non-portable binaries that will crash with SIGILL on older CPUs.
SSE behaves differently on a different CPUs (not speaking about the need to build architecture-specific binaries for every arch). E.g. AVX1 binaries work about 10 times slower than non-vectorized code on all some of AMD A* CPUs. We even have an option to disable vectorization on such CPUs in the runtime. Vectorized code should be tested on all available consumer CPUs and noone did it during the last 6 years.
Vector instructions depend on alignment. Every unaligned access causes either a severe performance degradation or SIGBUS that terminates the application. So every single bug in abandoned and unmaintained code will terminate Krita and cause a data loss for the users.
I could build neither stable version nor svn checkout of einspline, so I cannot even run its unittests. Looking into GCC messages it seems like sse intrinsics are used incorrectly. They are not a part of the standard, therefore their meaning can change with time.

Also, I provided above one memory leak -- are there any other?

Here are all the commits we did to the einspline code, please check if you'd like to port anything upstream:

2014-09-10 14:51:46 +0400 ad59717 Added implementation of 1D- and 2D- interpolation b-splines
2014-11-24 11:38:28 +0400 50492d2 Rename Einspline files into .cpp
2014-11-24 15:41:46 +0400 c29c1d1 Ported einspline into C++
2014-11-24 20:16:43 +0400 13d19d9 Fix compilation on Arch
2015-03-05 10:28:54 +0100 e54cca9 Fix unused-but-set variables in einspline
2015-03-06 10:08:19 +0100 6b7249d Fix warning, use free instead of delete on void *
2015-03-07 00:40:34 +0100 629a5da Move einspline subfolder into a 3rdparty dir
2015-03-11 10:10:45 +0100 9244151 msvc 2015 build fix
2015-07-07 21:27:00 -0400 9f3e35d Declare float types for einspline constants
2016-01-25 18:19:54 +0100 b46898a Move kritaimage to libs
2016-05-26 19:14:49 +0200 80a0d2f Fix: create Bspline coefs data consistenly in C-style