ImageProcessing-ElectronicPublications / scantailor-experimental

Scan Tailor Experimental is an interactive post-processing tool for scanned pages.
https://github.com/Tulon/scantailor/tree/experimental
GNU General Public License v3.0
34 stars 1 forks source link

Crash after clicking dewarp button #17

Closed mara004 closed 7 months ago

mara004 commented 9 months ago

On my sample, STEX always crashes when clicking the dewarp button ("distortion type -> curved lines") The same happened with my installs of the original Tulon STEX, and with ST Deviant.

Head of backtrace:

#0  0x00000000005f1bf7 in XSpline::linearCombinationFor(spfit::FittableSpline::LinearCoefficient*, int, double) const ()
#1  0x00000000005f21bf in XSpline::pointAtImpl(int, double) const ()
#2  0x00000000005f2c95 in XSpline::maybeAddMoreSamples(VirtualFunction3<void, QPointF, double, spfit::FittableSpline::SampleFlags>&, double, double, double, double, double, QPointF const&, double, QPointF const&) const ()

XSpline::maybeAddMoreSamples was then repeated countless times below.

mara004 commented 8 months ago

@mara004 . does your system crash with the fix from @noobie-iv : https://github.com/ImageProcessing-ElectronicPublications/scantailor-experimental/tree/fix_caching_factory ?

@zvezdochiot & co.: I'm afraid it still crashes (screencast attached). But nevertheless many thanks guys for your investigation, I'm glad you're at it!

2024-01-11 15-38-21.zip (zipped for GH)

mara004 commented 8 months ago

^ Ah, but that's the first bug (XSpline). I'll recompile in debug mode to check the second bug, which this fix is about (CachingFactory). Sorry...

mara004 commented 8 months ago

OK, I can confirm all works fine with a debug build on fix_caching_factory 🎉

May I ask if there have been any findings on the first bug yet? – I haven't followed all of this thread...

zvezdochiot commented 8 months ago

@mara004 . I can no reproduce the crash at the “Geometric Distortion” stage. We have to wait to see what @plzombie has to say about this, he will be able to reproduce your crash.

plzombie commented 8 months ago

@noobie-iv Я вот чего не понимаю во всём этом г-нокоде. Мы в лямбде downscaled_transform_orig_image используем замыкание на cached_transform_orig_image (теперь через ссылку). Потом создаём кэш cached_downscaled_transform_orig_image, который возвращает результат выполнения этой лямбды. А потом у нас идёт вот что:

    if (m_lastTab == TAB_PICTURE_ZONES)
    {
        // Because building a transformation of the original image takes
        // a noticeable amount of time, we only do it if we are sure we'll need it.
        // Otherwise it will get constructed on demand.
        cached_transform_orig_image();
        cached_downscaled_transform_orig_image();
    }

    return FilterResultPtr(
               new UiUpdater(
                   m_ptrFilter, accel_ops, m_ptrSettings, std::move(m_ptrDbg), params,
                   m_pageId, orig_image, out_img,
                   generator.origToOutputMapper(),
                   generator.outputToOrigMapper(),
                   automask_img, cached_transform_orig_image,
                   cached_downscaled_transform_orig_image,
                   despeckle_state, despeckle_visualization,
                   m_batchProcessing, m_debug
               )
           );

И если условие if (m_lastTab == TAB_PICTURE_ZONES) не выполнится, то и лямбда тоже не выполнится. Значит, она выполнится где-то позже. И в этот момент ссылка на объект cached_transform_orig_image (который у нас хранится в стэке) будет не валидна. То есть закрыв один баг мы породили другой (use after free). Или я чего-то не понимаю в работе лямбд, и в Си++ есть какая-то магия на этот случай?

zvezdochiot commented 8 months ago

@plzombie say:

то и лямбда тоже не выполнится.

Не пинайте дилетанта, я то в плюсах вообще только на ощупь, но в параметрах UiUpdater() явственно cached_transform_orig_image, cached_downscaled_transform_orig_image,. Это "слегка" отличается от "не выполнится". Но это ИМХО дилетанта.

plzombie commented 8 months ago

To resume all findings

OS Debian 11 Debian 12 Fedora 37 Windows
Compiler GCC 10.2.1 GCC 12.2 GCC 12.3.1 MSVC (Any)
Qt 5.15.2 5.15.8 5.15.10 5 or 6 (Any)
Bug with infinite loop in dewarping (segfault) No No Yes (Release only) No
Bug with crash in Output No Fixed Fixed No

@zvezdochiot

то и лямбда тоже не выполнится.

Не пинайте дилетанта, я то в плюсах вообще только на ощупюпь, но в параметрах UiUpdater() явственно cached_transform_orig_image, cached_downscaled_transform_orig_image,. Это "слегка" отличается от "не выполнится". Но это ИМХО дилетанта.

Здесь же просто cachingFactory передаётся в конструктор, а чтобы выполнилась лямбда, надо сделать cached_downscaled_transform_orig_image()

zvezdochiot commented 8 months ago

@plzombie say:

Release only

:question: Поподробнее с этого момента, а в Debug не падает чтоле? То есть имеется сношение с оптимизациями?

plzombie commented 8 months ago

@plzombie say:

Release only

❓ Поподробнее с этого момента, а в Debug не падает чтоле? То есть имеется сношение с оптимизациями?

Именно

zvezdochiot commented 8 months ago

@plzombie . Так флагами оптимизации поиграйся. Конечно не -O0, но может что то разумное можно поставить, да и делов то?

mara004 commented 8 months ago

@plzombie & co. Question for the lay's interest: Is there any explanation why it crashes on some platforms but works on others, although it still seems to come down to errors in ST code?

@zvezdochiot Yes, the debug build works, see above e.g. https://github.com/ImageProcessing-ElectronicPublications/scantailor-experimental/issues/17#issuecomment-1843750068, https://github.com/ImageProcessing-ElectronicPublications/scantailor-experimental/issues/17#issuecomment-1887353146 f.

zvezdochiot commented 8 months ago

@mara004 . Look: it works without compiler optimizations, but with optimizations it doesn’t work. Is this a code error?

mara004 commented 8 months ago

@mara004 . Look: it works without compiler optimizations, but with optimizations it doesn’t work. Is this a code error?

For the second first bug, yes, I see your point. But what about the first second bug?

zvezdochiot commented 8 months ago

@mara004 . So I’m talking about the first bug. The second one crashed on Debug. (order according to the table)

mara004 commented 8 months ago

Sorry, mixed up first and second 😅 I mean the other bug, the one which has been fixed (output stage).

plzombie commented 8 months ago

@plzombie . Так флагами оптимизации поиграйся. Конечно не -O0, но может что то разумное можно поставить, да и делов то?

Я совершенно без понятия как в CMAKE пробросить compiler-флаги вроде -O0, если там уже есть флаг -O2. Помогает только -D CMAKE_BUILD_TYPE=Debug

zvezdochiot commented 8 months ago

@plzombie , собственно, усё здесь: https://github.com/ImageProcessing-ElectronicPublications/scantailor-experimental/blob/326bb5ff5f45e618cf7d383c33918a628f5a8e17/cmake/SetDefaultGccFlags.cmake#L1-L148

Ты можешь даже впихнуть флаг -g и получить сбоящую версию с отладочной инфой.

noobie-iv commented 8 months ago

@plzombie

в Си++ есть какая-то магия на этот случай

Из того, что я пока вычитал по книжкам - лямбда без замыканий эквивалентна указателю на функцию, т.е. всегда валидна, потому что ее код не на стеке живет, а вместе со всем остальным исполняемым кодом, от начала и до конца работы программы. А лямбды с замыканиями - полноценные объекты, и код функции-то валиден, а вот данные помирают вместе с объектом, за такими лямбдами надо проверять время жизни. Про другие магии не читал.

Значит, она выполнится где-то позже

Внутри UiUpdater при конструировании сохраняется копия фабрики m_cachedTransformedOrigImage, так что FilterResultPtr вроде как в любом случае уходит наружу с копией. А вот почему сама лямбда без ссылки начинает глючить - пока не въехал.

Прикол еще в том, что у меня полноценные отладочные сборки не запускаются - они вылетают с битой памятью прямо при запуске STEX; видимо, в программе еще какие-то баги имеются. Единственный рабочий отладочный вариант - RelWithDebInfo, но в нем оптимизатор уже половину всего поудалял, и внутрь шаблонов входить невозможно, так что я не могу посмотреть, как у меня фабрика работает. Разве что пытаться собрать полную версию Qt, и в Creator-е тестить, но это опять лишняя неделя-другая возни в поисках рабочего метода сборки.

Кстати, поставил Фёдора-37 на виртуалку - там не могу поймать глюк с вылетом на деварпе. Видмо, дополнительно версии компилятора/Qt/хз-чего-еще надо подбирать.

во всём этом г-нокоде

Вот у меня в STD никакого г-на нет. У меня деварпнутая картинка при каждом запросе пересчитывается заново. К релизу надо, по идее, г-на отсюда зачерпнуть, и туда намазать, а то после STEX работать с тормозными аналогами - одно расстройство 😄.

В оригинальном ST хитрая оптимизация: главное окно хранит всего лишь Id с путями к исходным файлам, а на каждой стадии просто пересчитываются матрицы поворота и контуры подрезки для показа, и так по цепочке до самого вывода, это быстро. А когда появился STEX, понадобилось по дороге подменять изображение на деварпнутое, и видимо, чтобы не переделывать всю логику обработки, в трансформацию, кроме "просто матриц", засунули еще и генерацию изображения по запросу. Вот тут лямбды как раз ее и вызывают - orig_image_transform->materialize(). А чтобы не тормозить лишний раз, этот мать-его-рилайз и завернули в кешкину фабрику. Подозреваю, что по-честному надо сгенерированное однажды изображение сохранять на диск, и дальше всегда использовать его, а не пересчитывать каждый раз заново, но это как раз переделка логики, на ней-то я в STD и застрял.

zvezdochiot commented 7 months ago

@noobie-iv say:

отладочные сборки не запускаются - они вылетают с битой памятью прямо при запуске STEX; видимо, в программе еще какие-то баги имеются.

Это ещё чо за нах?! Такого точно не должно быть. Что то точно не так.

noobie-iv commented 7 months ago

@zvezdochiot

Это ещё чо за нах

Починил уже, надо было и Qt в отладочном режиме собрать; видимо, она за памятью в какие-то runtime-библиотеки лазит, и при несовпадении конфигураций с основным c++ вылетает. У меня с начала экспериментов только релизная Qt стояла, слишком уж много отладочная весит. Так что исходная "правильная" версия для сверки в отладчике у меня теперь есть.

Осталось Фёдора поломать, чтобы баг с вылетом на деварпе поймать, иначе его не отладить. Как вариант, можно просто в эту самую maybeAddMoreSamples приделать принудительный счетчик на 5-10 уровней вложенности. Она же рекурсивная, и судя по первому сообщению, входит то ли в бесконечную рекурсию, то ли просто в слишком глубокую.

plzombie commented 7 months ago

So, about flags. I checked GCC 12 Release notes and it says that -O2 now works as -O2 -ftree-vectorize -fvect-cost-model=very-cheap. Information about -fvect-cost-model=very-cheap can be found here. So I tried to compile with -O1 and everything works fine. When I tried to compile with -O1 -ftree-vectorize -fvect-cost-model=very-cheap and everything also works fine. So at least it's not about vectorization.

So, as a temporary solution you can replace -O2 with -O1 or -O1 -ftree-vectorize -fvect-cost-model=very-cheap in https://github.com/ImageProcessing-ElectronicPublications/scantailor-experimental/blob/main/cmake/SetDefaultGccFlags.cmake

TODO: Get somewhere a full list of changes for -O2 flag. Also the second bug could be introduced with a new flag -ffold-simple-inlines which is enabled by default, need to investigate this.

plzombie commented 7 months ago

Also if compile with -O3 it doesn't crash but generate this interesting result. As we can see, preview image is normal but resulting image differs from canonical results (but still looks fine). изображение

zvezdochiot commented 7 months ago

@plzombie say:

you can replace -O2 with -O1

После замены у себя никаких тормазнутостей и прочей чепухи не наблюдал. Ежели ты потверждаешь прекращение падения в Fedora с флагом -O1, то на этом и порешим. Ежели есть какие то ньюансы в смене флага в винде, то внесение изменений и закрытие бага оставлю на тебя и @noobie-iv . Протестить то мне нечем. Такие вот дела.

trufanov-nok commented 7 months ago

Я в пол глаза просмотрел тред, и не факт что уловил суть, но могу поделиться своими наблюдениями - что если есть разница между сборками Debug/Release или O2/O1 , то стоит собрать проект в QtCreator в Debug режиме и через Menu/Tool (или как его) выполнить анализ в Valgrind на предмет утечек памяти. В debug сборке неинициализированные указатели могут принудительно ставиться в NULL, тогда как в release оставаться мусором в угоду производительности. Valgrind должен такую чепуху находить.

plzombie commented 7 months ago

@trufanov-nok @noobie-iv So I created another branch for testing Dewarping with debug input. Correct build has 6MB of logs while crashing build has 22MB

plzombie commented 7 months ago

dewarp-MSVC2019.txt

plzombie commented 7 months ago

dewarp-gcc.12.3.1-O1.txt dewarp-gcc.12.3.1-O2.txt So here are files. At least we can compare them and see the difference As we can see, middle point have different coordinates in all 3 cases. Especially for -O2 flag.

plzombie commented 7 months ago

So we can just debug QPointF XSpline::pointAt(double t) const and try to get the same results for all compilers

zvezdochiot commented 7 months ago

@plzombie Так-так-так... Вот это стоит посмотреть повнимательней. Момент таков: при выборе автоматического деварпинга (а это основа) после сохранения проекта и его переоткрытия также получались слегка отличающиеся параметры трансформации, что приводило к необходимости пересчёта всех последующих этапов. Это вызывало недоумение у многих. Но вполне возможно, что причина именно в том, на что ты указываешь. Ежели порешать, то решится не одна проблема, а сразу несколько.

@plzombie , @noobie-iv . Посмотрел я на этот XSpline. Хоспаде, с каким бы удовольствием и откровенной радостью я заменил бы все эти навороты на сплайн Акимы с продольной осью, соединяющей углы сетки. Ну почему выбрали именно BSpline?!

noobie-iv commented 7 months ago

@plzombie, @zvezdochiot

Удалось Фёдора заломать. Поставил ту же версию 37-1.7, но с другого iso. Плюс компиляторы с библиотеками ставил не с командной строки, а из GUI, тыкая наугад в одну из нескольких версий из списка. Теперь и у меня релизная сборка падает, а отладочная работает.

Причина падения - провал maybeAddMoreSamples в бесконечную рекурсию. Добавил счетчик глубины - вылет происходит где-то за 30'000 вложений, с параметрами prev_t=0.49999999... и next_t=0.5. Чтобы поймать начало входа в рекурсию, надо в точке останова поставить условие "num_segments==4", тогда после щелчка на деварп сразу попадаем сюда.

Уже сразу на входе видно, что виновата не эта функция, просто ей передают битый сплайн из 5 точек, у которого первые 4 точки совпадают, тогда как в отладочной версии все точки разные. При этом и в Winsows, и в Fedora работающие версии получают одинаковые сплайны на обработку (плюс-минус где-то в последних знаках). Впервые этот битый сплайн получается внутри Task::processWarpDistortion, после вызова model_builder.tryBuildModel(), дальше внутрь еще не долез.

Заодно релиз работает правильно не только с "-O1", но и с "-fsanitize=undefined", это первый попавшийся совет из гугла. Возможно, мы где-то поймали UB.

zvezdochiot commented 7 months ago

@noobie-iv . Так-так-так... Я уже "ломал хребет" подобному, только с обратной стороны, когда точки распределены ровно по линии на равном расстоянии. Порешал добавлением нового ограничения по разности значений до 1e-5, при невыполнении return. Только без выяснения причин UB такое решение будет даже не половиной дела. Как и предположил @trufanov-nok , где то в вычислениях явно мусор из неинициализированной переменной участвует.

zvezdochiot commented 7 months ago

@plzombie , @noobie-iv по отладочной информации в ветке debug-dewarp изначально получается какое то невразумительное значение:

max_sqdist_between_samples 179769313486231570814527423731704356798070567525844996598917476803157260780028538760589558632766878171540458953514382464234321326889464182768467546703537516986049910576551282076245490090389328944075868508455133942304583236903222948165808559332123348274797826204144723168738177180919299881250404026184124858368,000000

в то время как при повторной обработке во многих местах (но не во всех) вполне нормальное:

max_sqdist_between_samples 100,000000

Никто не пыталься распарсит, на кой нужно такое значение? И не приводит ли оно к ошибкам округления?

noobie-iv commented 7 months ago

@zvezdochiot

невразумительное значение:

Оно там вообще выставляется в NumericTraits::max(), т.е. 1.7e308 в коде при запуске. Не натыкался, когда успевает понижаться. А потерю точности только одну ловил - 4 совпадающие точки в сплайне, из-за которых pointAt() насчитывает левые координаты средней точки, которая и не дает сработать условиям прерывания рекурсии.

Проблема еще в том, что дебаг с релизом не совсем одинаковые координаты дают, поэтому не удается пошагово сравнить, начиная с какого места они расходятся. А анализируются там 30 строк по 50 точек, что в ручном режиме сверять поточечную обработку сплайнов малость сложновато, я к концу второй строки забываю с чего начал.

Пробовал сегодя с valgrind поиграть. В релизе есть множественные сообщения об обращение к неициализированным переменным, которых нет в отладке.

Снимок

По протоколу все они вызываются обращением к памяти, выделяемой под BinaryImage. Если в тамошний оператор выделения памяти вписать принудительную инициализацию через memset, сообщения пропадают.

void*
BinaryImage::SharedData::operator new(size_t, NumWords const num_words)
{
    SharedData* sd = 0;
    void* addr = malloc(((char*)&sd->m_data[0] - (char*)sd) + num_words.numWords * 4);
    if (!addr)
    {
        throw std::bad_alloc();
    }
    memset(addr, 0, ((char*)&sd->m_data[0] - (char*)sd) + num_words.numWords * 4);
    return addr;
}

Остаются еще какие-то подозрения на утечки памяти где-то внутри Qt, Boost и т.п., но это явно не они, их даже QTCreator по умолчанию фильтрами прячет.

Главное - после принудительной иницализации по-прежнему дебаг работает, а релиз падает, только уже с чистыми протоколами. Автоматическая халява не удалась.

zvezdochiot commented 7 months ago

@noobie-iv , ну так поставь заглушку в maybeAddMoreSamples() по соседству с уже имеющейся prev_next_sqdist:

    double delta_t = (prev_t > next_t) ? (prev_t - next_t) : (next_t - prev_t);

    if (delta_t < 1e-6)
    {
        return;
    }

Чтобы порвать рекурсию и посмотреть, что там с этими странными сплайнами в результате получается.

PS: Меня волнует, почему при первичном и повторном проходе значения разные? При этом требуется строго перезапуск STEX. Без перезапуска ничего не меняется.

noobie-iv commented 7 months ago

@zvezdochiot Результат с заглушкой:

Заглушка

Заглушка-2

zvezdochiot commented 7 months ago

@noobie-iv . Ну а теперь (коли как то но работает) надо бы выяснить, чего это точки расставляются в одну? С чего это вдруг они кучкуются? И почему на правьюшке они не кучкуются, а на самом изображении кучкуются?

noobie-iv commented 7 months ago

Кто знает, что это за на? Внутри XSpline::linearCombinationFor() вычисляются коэффициенты A[0]...A[3]. Вычисляются на основе структуры TensionDerivedParams. Сама структура в дебаге/релизе одинакова. А коэффициенты получаются разные. И даже после нормализации "A /= A.sum();" релизные коэффициенты получаются битые - на них нормализация не действует.

Релиз:

Rel

Дебаг:

Deb

Коэффициенты создаются вызовами GBlendFunc().vlaue(), но им внутрь в релизе заглянуть нельзя, они оптимизированы. При этом начальная и конечная точки сплайна, вычисленные через такие битые коэффициенты, в дебаге и релизе совпадают. А вот первая вычисленная mid_pt(pointAt(mid_t)) уже дает различия при одинаковых исходных.

Это QTCreator умеет в отладчике показывать глюкавые значения, и ему нельзя верить?

noobie-iv commented 7 months ago

@plzombie @zvezdochiot

Еще интересно: вылеты прекращаются, если повесить флаг компиляции только на один файл - со сплайном: добавить в src/src/math/CMakeLists.txt строчку:

set_source_files_properties(XSpline.cpp PROPERTIES COMPILE_FLAGS -O1)

Остается выследить конкретную функцию через #pragma, а потом устроить ей темную.

noobie-iv commented 7 months ago

@plzombie @zvezdochiot

Собственно, вот виновники вылетов (XSpline.cpp) - GBlendFunc и HBlendFunc. Достаточно собрать с O1 только их.

Можно обернуть в pragma их объявления:

#pragma GCC push_options
#pragma GCC optimize ("-O1")

class XSpline::GBlendFunc
{
...
};

class XSpline::HBlendFunc
{
...
};

#pragma GCC pop_options

А можно обернуть в pragma парочку их пользователей:

#pragma GCC push_options
#pragma GCC optimize ("-O1")

XSpline::linearCombinationFor(LinearCoefficient* coeffs, int segment, double t) const
{
...
}

#pragma GCC pop_options

...

#pragma GCC push_options
#pragma GCC optimize ("-O1")

XSpline::decomposedDerivsImpl(int const segment, double const t) const
{
...
}

#pragma GCC pop_options

В обоих случаях вылеты прекращаются. Кто объяснит, где тут смеяться?

plzombie commented 7 months ago

То есть у нас есть ошибки в GBlendFunc и HBlendFunc, которые вызывают изначальную ошибку. А дальше вызывается pointAt, где вероятнее из-за неправильных линейных коэффициентов (спасибо первым двум функциям) вычисляется неправильная средняя точка. Всё это накладывается, и появляется бесконечная рекурсия

zvezdochiot commented 7 months ago

@noobie-iv , что даст строгое использование внутренней переменной?:

/*========================== GBlendFunc ==========================*/

XSpline::GBlendFunc::GBlendFunc(double q, double p)
    :   m_c1(q), // See formula 20 in [18].
      m_c2(2.0 * q),
      m_c3(10.0 - 12.0 * q - p),
      m_c4(2.0 * p + 14.0 * q - 15.0),
      m_c5(6.0 - 5.0 * q - p)
{
}

double
XSpline::GBlendFunc::value(double u) const
{
    double const u2 = u * u;
    double const u3 = u2 * u;
    double const u4 = u3 * u;
    double const u5 = u4 * u;
    double const r = m_c1 * u + m_c2 * u2 + m_c3 * u3 + m_c4 * u4 + m_c5 * u5;

    return r;
}

double
XSpline::GBlendFunc::firstDerivative(double u) const
{
    double const u2 = u * u;
    double const u3 = u2 * u;
    double const u4 = u3 * u;
    double const r = m_c1 + 2.0 * m_c2 * u + 3.0 * m_c3 * u2 + 4.0 * m_c4 * u3 + 5.0 * m_c5 * u4;

    return r;
}

double
XSpline::GBlendFunc::secondDerivative(double u) const
{
    double const u2 = u * u;
    double const u3 = u2 * u;
    double const r = 2.0 * m_c2 + 6.0 * m_c3 * u + 12.0 * m_c4 * u2 + 20.0 * m_c5 * u3;

    return r;
}

/*========================== HBlendFunc ==========================*/

XSpline::HBlendFunc::HBlendFunc(double q)
    :   m_c1(q), // See formula 20 in [18].
      m_c2(2.0 * q),
      m_c4(-2.0 * q),
      m_c5(-q)
{
}

double
XSpline::HBlendFunc::value(double u) const
{
    double const u2 = u * u;
    double const u3 = u2 * u;
    double const u4 = u3 * u;
    double const u5 = u4 * u;
    double const r = m_c1 * u + m_c2 * u2 + m_c4 * u4 + m_c5 * u5;

    return r;
}

double
XSpline::HBlendFunc::firstDerivative(double u) const
{
    double const u2 = u * u;
    double const u3 = u2 * u;
    double const u4 = u3 * u;
    double const r = m_c1 + 2.0 * m_c2 * u + 4.0 * m_c4 * u3 + 5.0 * m_c5 * u4;

    return r;
}

double
XSpline::HBlendFunc::secondDerivative(double u) const
{
    double const u2 = u * u;
    double const u3 = u2 * u;
    double const r = 2.0 * m_c2 + 12.0 * m_c4 * u2 + 20.0 * m_c5 * u3;

    return r;
}

PS: я бы вообще пользовал бы вложенное умножение, без всяких там u2,u3,...

plzombie commented 7 months ago

Может попробовать везде volatile double const поставить? Возможно компилятор как-раз оптимизирует подставляя переменные

trufanov-nok commented 7 months ago

Я не смог пока воспроизвести проблему... А если в https://github.com/ImageProcessing-ElectronicPublications/scantailor-experimental/blob/main/src/imageproc/BinaryImage.cpp#L1335 malloc на calloc заменить, лучше не станет?

plzombie commented 7 months ago

Я не смог пока воспроизвести проблему... А если в https://github.com/ImageProcessing-ElectronicPublications/scantailor-experimental/blob/main/src/imageproc/BinaryImage.cpp#L1335 malloc на calloc заменить, лучше не станет?

Нет. Более того, в calloc нет защиты от переполнения, а под виндой это просто malloc+memset

trufanov-nok commented 7 months ago

Ну это понятно. зато память в нулях, а не undefined. Это похоже единственное на что Valgrind у меня ругался - на то что в некоторых ситуациях идет обращением к этой памяти, а там фигня. При изменении размеров страницы при dewarp'е такое в теории могло бы происходить по краям.

plzombie commented 7 months ago

Тогда лучше memset добавить

noobie-iv commented 7 months ago

@zvezdochiot

что даст строгое использование внутренней переменной

Ничего не дает, так же вылетает. При просмотре в отладчике внутрь релизных GBlendFunc в принципе нельзя войти. Похоже, там оптимизирована каждая строка вида:

A[1] = GBlendFunc(tdp.q[1], tdp.p[1]).value((t - tdp.T1p) / (tdp.t1 - tdp.T1p));

да так, что вообще не создается объект GBlendFunc, а сразу подставляются вычисления. Или вообще не подставляются, хз. Везде сплошные сообщения типа "optimized out", и строчки просто проскакиваются, а в локальной переменной A[] то никаких изменений не происходит, то мусор появлятеся. Дальше надо ассемблер смотреть.

@plzombie

попробовать везде volatile double const

Не помогает. Даже если добавить volatile еще и ко всем m_* обеих функций.

@plzombie

Тогда лучше memset добавить

Уже пробовал:

void*
BinaryImage::SharedData::operator new(size_t, NumWords const num_words)
{
    SharedData* sd = 0;
    void* addr = malloc(((char*)&sd->m_data[0] - (char*)sd) + num_words.numWords * 4);
    if (!addr)
    {
        throw std::bad_alloc();
    }
    memset(addr, 0, ((char*)&sd->m_data[0] - (char*)sd) + num_words.numWords * 4);
    return addr;
}

Получается чистый отчет valgrind, но вылеты остаются.

@trufanov-nok

Я не смог пока воспроизвести проблему

Ловится почему-то только на Fedora-37. Ubuntu 18/20, Debian 10/12 - нормально работают.

Причем, когда я ставил Live-ISO, вылетов не было. Поставил Everything-ISO - вылеты начались.

Я играю с VirtualBox/Win10x64. Ставлю Fedora-Everything-netinst-x86_64-37-1.7.iso в режиме "xfce" без дополнительных программ. Ставлю софт:

# Установка Qt (заодно установит cmake, gcc и т.п.)
    sudo dnf -y install qt5-qtbase-devel
    sudo dnf -y install qt5-designer
    sudo dnf -y install qt5-linguist
    sudo dnf -y install qt-creator

# Установка библиотек:
    sudo dnf -y install boost-devel
    sudo dnf -y install eigen3
    sudo dnf -y install zlib-devel
    sudo dnf -y install libjpeg-turbo-devel
    sudo dnf -y install libpng-devel
    sudo dnf -y install libtiff-devel

# Установка valgrind:
    sudo dnf -y install valgrind

# Установка gdb:
    sudo dnf -y install gdb

# Установка git:
    sudo dnf -y install git

# Установка mc:
    sudo dnf -y install mc

# Установка Double Commander:
    sudo dnf -y install doublecmd-qt

# Установка firefox:
    sudo dnf -y install firefox

# Установка VSCode
    sudo rpm --import https://packages.microsoft.com/keys/microsoft.asc
    sudo sh -c 'echo -e "[code]\nname=Visual Studio Code\nbaseurl=https://packages.microsoft.com/yumrepos/vscode\nenabled=1\ngpgcheck=1\ngpgkey=https://packages.microsoft.com/keys/microsoft.asc" > /etc/yum.repos.d/vscode.repo'
    dnf check-update
    sudo dnf -y install code

# Установка прав доступа к общим папкам VirtualBox:
    sudo usermod -aG vboxsf user
    sudo shutdown -r now
noobie-iv commented 7 months ago

@plzombie

попробовать везде volatile double const поставить

Однако, в отладчике после этого вход в функции GBlendFunc становится доступен, и коэффициенты вроде становятся правильными. По крайней мере раньше уже на первых же вызовах насчитывалась ерунда, а теперь в том десятке-другом, которые мне хватило терпения сверить, коэффициенты совпадают. Но в свободном полете вылеты продолжаются.

Раньше в момент вылета точки сплайна this->m_controlPoints были явно битые (совпадали первые 4), а теперь они, как и положено, разные, но все равно отличаются от рабочего дебажного варианта. Так что часть глюков volatile снял, но часть где-то еще остается.

Может, вообще все переменные в программе volatile сделать 🥲?

noobie-iv commented 7 months ago

@plzombie @zvezdochiot

Похоже, один-единственный volatile лечит вылеты. Пришлось убрать Vec4d, он сопротивлялся. По идее, надо это изменение в какой-нибудь #if завернуть:

https://github.com/ImageProcessing-ElectronicPublications/scantailor-experimental/tree/fix_issue_17

Win10x64 Debug/Release, Fedora-3.7-1.7 Debug/Release - OK.

И что все это значит?

P.S. Кто знает еще какие-нибудь прикольные ключевые слова, чтобы дописать куда-нибудь? Заодно и остальные баги починим 😄.

zvezdochiot commented 7 months ago

@mara004 . Need confirmation of STEX functionality in https://github.com/ImageProcessing-ElectronicPublications/scantailor-experimental/tree/fix_issue_17

@noobie-iv . А просто выделение A.sum() в отдельную переменную в int XSpline::linearCombinationFor(LinearCoefficient* coeffs, int segment, double t) const, как это сделано в XSpline::DecomposedDerivs XSpline::decomposedDerivsImpl(int const segment, double const t) const ничего не даёт?:

    Vec4d A;
...
    double const sum = A.sum();
    A /= sum;

И почему в void XSpline::maybeAddMoreSamples() один sink(mid_pt, mid_t, flags); когда напрашивается 2?