pinterf / mvtools

mvtools plugin for avisynth
155 stars 17 forks source link

WorkingArea.nMinCost is uninitialized #53

Closed magiblot closed 2 years ago

magiblot commented 2 years ago

Valgrind complains that an uninitialized value is being used when accessing workarea.nMinCost.

==197240== Conditional jump or move depends on uninitialised value(s)
==197240==    at 0x1E5E791C: CheckMV0<unsigned char> (PlaneOfBlocks.cpp:2771)
==197240==    by 0x1E5E791C: void PlaneOfBlocks::PseudoEPZSearch<unsigned char>(PlaneOfBlocks::WorkingArea&) (PlaneOfBlocks.cpp:1010)
==197240==    by 0x1E6042A5: void PlaneOfBlocks::search_mv_slice<unsigned char>(MTSlicer<PlaneOfBlocks, PlaneOfBlocks, 64>::TaskData&) (PlaneOfBlocks.cpp:3178)
==197240==    by 0x1E3F9623: AvstpWrapper::fallback_enqueue_task_ptr(avstp::TaskDispatcher*, void (*)(avstp::TaskDispatcher*, void*), void*) (AvstpWrapper.cpp:275)
==197240==    by 0x1E5CDF57: MTSlicer<PlaneOfBlocks, PlaneOfBlocks, 64>::start(int, PlaneOfBlocks&, void (PlaneOfBlocks::*)(MTSlicer<PlaneOfBlocks, PlaneOfBlocks, 64>::TaskData&), int) (MTSlicer.hpp:177)
==197240==    by 0x1E5CB2A5: PlaneOfBlocks::SearchMVs(MVFrame*, MVFrame*, SearchType, int, int, int, int, int, int, int*, VECTOR const*, short*, int, int*, int, int, int, int, int, bool, int*, bool, int) (PlaneOfBlocks.cpp:387)
==197240==    by 0x1E40733A: GroupOfPlanes::SearchMVs(MVGroupOfFrames*, MVGroupOfFrames*, SearchType, int, int, int, int, int, int, bool, int, int*, short*, int, int, int, int, int, bool, int*, bool, int) (GroupOfPlanes.cpp:158)
==197240==    by 0x1E45219B: MVAnalyse::GetFrame(int, IScriptEnvironment*) (MVAnalyse.cpp:635)
==197240==    by 0x1798A50B: MTGuard::GetFrame(int, IScriptEnvironment*) (MTGuard.cpp:182)
==197240==    by 0x179C3573: Cache::GetFrame(int, IScriptEnvironment*) (cache.cpp:222)
==197240==    by 0x179C3316: CacheGuard::GetFrame(int, IScriptEnvironment*) (cache.cpp:510)
==197240==    by 0x1E45D9CC: MVClip::GetFrame(int, IScriptEnvironment*) (MVClip.cpp:153)
==197240==    by 0x1E4647B9: MVDegrainX::GetFrame(int, IScriptEnvironment*) (MVDegrain3.cpp:619)

https://github.com/pinterf/mvtools/blob/d8bdff7e02c15a28dcc6e9ef2ebeaa9d16cc1f56/Sources/PlaneOfBlocks.cpp#L2771

==197240== Conditional jump or move depends on uninitialised value(s)
==197240==    at 0x1E5DDFC4: CheckMV<unsigned char> (PlaneOfBlocks.cpp:2819)
==197240==    by 0x1E5DDFC4: void PlaneOfBlocks::ExpandingSearch<unsigned char>(PlaneOfBlocks::WorkingArea&, int, int, int, int) (PlaneOfBlocks.cpp:1478)
==197240==    by 0x1E5E555B: void PlaneOfBlocks::Refine<unsigned char>(PlaneOfBlocks::WorkingArea&) (PlaneOfBlocks.cpp:858)
==197240==    by 0x1E5E7D1B: void PlaneOfBlocks::PseudoEPZSearch<unsigned char>(PlaneOfBlocks::WorkingArea&) (PlaneOfBlocks.cpp:1036)
==197240==    by 0x1E6042A5: void PlaneOfBlocks::search_mv_slice<unsigned char>(MTSlicer<PlaneOfBlocks, PlaneOfBlocks, 64>::TaskData&) (PlaneOfBlocks.cpp:3178)
==197240==    by 0x1E3F9623: AvstpWrapper::fallback_enqueue_task_ptr(avstp::TaskDispatcher*, void (*)(avstp::TaskDispatcher*, void*), void*) (AvstpWrapper.cpp:275)
==197240==    by 0x1E5CDF57: MTSlicer<PlaneOfBlocks, PlaneOfBlocks, 64>::start(int, PlaneOfBlocks&, void (PlaneOfBlocks::*)(MTSlicer<PlaneOfBlocks, PlaneOfBlocks, 64>::TaskData&), int) (MTSlicer.hpp:177)
==197240==    by 0x1E5CB2A5: PlaneOfBlocks::SearchMVs(MVFrame*, MVFrame*, SearchType, int, int, int, int, int, int, int*, VECTOR const*, short*, int, int*, int, int, int, int, int, bool, int*, bool, int) (PlaneOfBlocks.cpp:387)
==197240==    by 0x1E40733A: GroupOfPlanes::SearchMVs(MVGroupOfFrames*, MVGroupOfFrames*, SearchType, int, int, int, int, int, int, bool, int, int*, short*, int, int, int, int, int, bool, int*, bool, int) (GroupOfPlanes.cpp:158)
==197240==    by 0x1E45219B: MVAnalyse::GetFrame(int, IScriptEnvironment*) (MVAnalyse.cpp:635)
==197240==    by 0x1798A50B: MTGuard::GetFrame(int, IScriptEnvironment*) (MTGuard.cpp:182)
==197240==    by 0x179C3573: Cache::GetFrame(int, IScriptEnvironment*) (cache.cpp:222)
==197240==    by 0x179C3316: CacheGuard::GetFrame(int, IScriptEnvironment*) (cache.cpp:510)

https://github.com/pinterf/mvtools/blob/d8bdff7e02c15a28dcc6e9ef2ebeaa9d16cc1f56/Sources/PlaneOfBlocks.cpp#L2819

This member is indeed not initialized in WorkingArea::WorkingArea: https://github.com/pinterf/mvtools/blob/d8bdff7e02c15a28dcc6e9ef2ebeaa9d16cc1f56/Sources/PlaneOfBlocks.cpp#L3661-L3667

I'm not sure what the initial value for this member should be, so I leave the fix in your hands.

DTL2020 commented 2 years ago

It is good to show exact use case. Line 2771 - it is function PlaneOfBlocks::CheckMV0 called from many places. workarea is 'global' structure for current search thread. If call stack is PseudoEPZsearch - the mincost is set (conditionless/guaranteed) at least at https://github.com/pinterf/mvtools/blob/d8bdff7e02c15a28dcc6e9ef2ebeaa9d16cc1f56/Sources/PlaneOfBlocks.cpp#L934 at the start of PseudoEPZSearch after check zero-predictor before calling to CheckMV0. Same is for CheckMV in expanding search - it is in Refine() process at the end of PseudoEPZSearch.

pinterf commented 2 years ago

I suppose valgrind is complaining for real, so that at least the first comparison is done against a totally uninitialized value instead of workarea.nMinCost = verybigSAD + 1; I don't know whether it must be done outside the loop, once, before https://github.com/pinterf/mvtools/blob/mvtools-pfmod/Sources/PlaneOfBlocks.cpp#L3063

Or in each inner loop https://github.com/pinterf/mvtools/blob/mvtools-pfmod/Sources/PlaneOfBlocks.cpp#L3091

(And the same modification should be applied to the mv_recalc some 300 lines later)

DTL2020 commented 2 years ago

How it can be if CheckMV0 is called after checking zero-predictor at the beginning of PseudoEPZSearch with conditionless initializing mincost with its 'startup' value for zero-mv check of current block ?

pinterf commented 2 years ago

Yep, I see it now. Here. https://github.com/pinterf/mvtools/blob/mvtools-pfmod/Sources/PlaneOfBlocks.cpp#L934 Then I don't know.

magiblot commented 2 years ago

Sorry. After taking a closer look at it, I realized the warning is caused by cost because the uninitialized value comes from the video frame itself. I still don't know what exactly is causing this but I don't think it's mvtool's fault.

Excuse me for the trouble and thanks for the attention.