Fix linear congruential random number generator

[stweil]

This reverts commit 2252936.

Fixes: #4146, #4148, #4270

[egorpugin]

But what is the reason of incorrect work?

If we check this table
https://en.wikipedia.org/wiki/Linear_congruential_generator#Parameters_in_common_use

we see our parameters from original code
MMIX by Donald Knuth | 264 | 6364136223846793005 | 1442695040888963407

Here we can find that
https://en.cppreference.com/w/cpp/numeric/random
std::minstd_rand is std::linear_congruential_engine<std::uint_fast32_t,                                48271, 0, 2147483647

Maybe my port was not presicely correct in terms of that std::minstd_rand is not exact what it was, I think we should try to make our code use std.

For example, using our_rand = std::linear_congruential_engine<std::uint_fast32_t, 6364136223846793005ULL, 1442695040888963407ULL, UINT64_MAX>

[egorpugin]

If new random was more correct, then old code has issues - in that place with delete after assert.

[marcreichman-pfi]

@stweil as i mentioned in those various tickets - this does remove the assertion failures and segfaults. I will point out, in most of these cases, compared to the original for a working variation on one of the bad images, and compared to my alternate assertion fix, the outputs are a bit different. I assume we can attribute that to the difference in random data?

[stweil]

Maybe my port was not presicely correct in terms of that std::minstd_rand is not exact what it was, I think we should try to make our code use std.

For example, using our_rand = std::linear_congruential_engine<std::uint_fast32_t, 6364136223846793005ULL, 1442695040888963407ULL, UINT64_MAX>

That's a good hint, thank you! I updated my pull request as suggested.

[marcreichman-pfi]

@stweil @egorpugin Coincidentally I tried that change locally as well. It also does not crash anymore, but the output is changed again a bit. Perhaps related to other things which were going on in this class at the time?

[egorpugin]

@stweil
Does it work with updated parameters like with old rand?

[egorpugin]

Also I'll put my analysis here.

Tried to reproduce this example on windows - no success (everything works).
#4148 (comment)

Is it specific STL issue? libstdc++ only? libc++?

---

Analysis

We have no issues with previous random implemenation.

So, cases are:

1. Code around assert + delete is incorrect and old random code is incorrect, so the issue stayed hidden.
2. Code around assert + delete is incorrect and new random is more correct, so it shows the problem around assert.
3. Code around assert + delete is correct and new random is incorrect - fix rand here.

What case do we have?

[stweil]

Does it work with updated parameters like with old rand?

Yes, the updated parameters are sufficient to fix the assertion and the heap issue.

[egorpugin]

Good, thank you.

I'm not well aware of tesseract testing, but I think it is worth to add some of those assert triggering images into the test suite.
For example, it should be a good test case for my changes with lists.

[stweil]

Yes, the updated parameters are sufficient to fix the assertion and the heap issue.

They fix these issues, but cause failures of recodebeam_test and intsimdmatrix_test. My previous revert seems to work better.

[egorpugin]

The problem is that 2,3 and 4 args to template become std::uint_fast32_t, but they should be kept as 64 bit.
std::linear_congruential_engine<std::uint_fast32_t, 6364136223846793005ULL, 1442695040888963407ULL, UINT64_MAX>

like this

std::linear_congruential_engine<std::uint_fast64_t, 6364136223846793005ULL, 1442695040888963407ULL, UINT64_MAX>

[stweil]

That does not fix the unittests. I hope I can fix it tomorrow.

[egorpugin]

It seems we cannot express
MMIX by Donald Knuth | 2^64 | 6364136223846793005 | 1442695040888963407
with std::linear_congruential_engine because of 2^64 modulus (we need 128 bit type for it to pass into template).
But 2^64 modulus is just a division by 1.

So the old code did exactly it plus converting a result into 32 bit int (taking first 31 bits).

    u *= 6364136223846793005ULL;
    u += 1442695040888963407ULL;
    return u >> 33;

But when I change my code to this snippet, the test does not work either.

[egorpugin]

It is even more clear now.

Our tests depends on random generator (default seed). Output is always different - unreliable.

Tesseract execution depends on random generator. Is random even seeded there?
If no, then tesseract code has issue around that assert + delete, it is unrelated to random generator.
Random only showed that there is an issue somewhere there.

[stweil]

I restored the old random generator (and the related unittest code) now. This fixes the unittests and the reported issues.

[egorpugin]

No, it does not fix the problem. Which is the problem around those lists, asserts and delete.
Only hides it behind unseeded rng.

[stweil]

I don't understand that. Do you have a test case which still triggers the assertion?

[egorpugin]

Code behavior depends on random generator.
We observe crashes on one generator and do not see them on other.

That means that the problem is not in rng, but in that code itself.
Old rng just hides the problem where changes in rng uncover it.

You can try to change initial seed (1 currently) to something else and try provided cases with it.

[stweil]

The initial seed 1 is not used, because the recognition sets a new seed in SetRandomSeed (not only once, but many times). And it calls TRand::SignedRand several thousand times.

[egorpugin]

The only place during recognition is https://github.com/tesseract-ocr/tesseract/blob/main/src/lstm/lstmrecognizer.h#L290
It sets seed based on iteration id. It means the numbers are the same during each run, they are not random (like based on time).

[egorpugin]

I cannot even call it 'random'. Not a fair one.
It is just a series of same numbers.

I can understand if they are time based so they change each run, but not like we have it now.

[stweil]

It's good that the series is not random, but pseudorandom, because otherwise some results would also be random (which is not desired).

And I think that I now know why commit 32fee19 still produced buggy code: the implementation of IntRand could return negative numbers which violated the specification. Fixing this also fixed the memory issues and the assertion.

[egorpugin]

I see. Indeed, return e() >> 33; part was missing.

[egorpugin]

It's good that the series is not random, but pseudorandom

It is not even pseudorandom. It is just a fixed series. Always the same. And it hides other code issues.

[marcreichman-pfi]

@stweil @egorpugin new data point, new segv after these changes: #4146 (comment)

Overall the stability is improved, but fwiw the image in that comment worked ok in 5.5.0 tag.

[stweil]

@marcreichman-pfi, I don't see such a regression, only a new error which you reported ("ELIST2_ITERATOR::add_before_stay_put:Error:Attempting to add an element with non nullptr links, to a list").