Asd-g / AviSynth-vsTTempSmooth

A temporal smoothing filter.
GNU General Public License v3.0
10 stars 2 forks source link

Uninitialized memory #10

Closed Asd-g closed 1 year ago

Asd-g commented 1 year ago

https://github.com/Asd-g/AviSynth-vsTTempSmooth/blob/8bbb7948b201057488db8e1c161471e5d22c5850/src/vsTTempSmooth.cpp#L556

@DTL2020, here pIIRMemY is allocated but I cannot find where it is initialized. Am I missing something?

DTL2020 commented 1 year ago

It may not be good for classic programming to leave it non-initialized. But it is logically skipped for performance (also there is no known default value to initialize) - the memory of 'dissimilarity metric' pMemSum initialized with 'very bad metric value' at class constructor so with the very first pass of IIR-processing the sample memory IIRMemY/U/V always updated with current best sample value. So it does not require initializing.

At the very first frame processing after class construction https://github.com/Asd-g/AviSynth-vsTTempSmooth/blob/8bbb7948b201057488db8e1c161471e5d22c5850/src/vsTTempSmooth.cpp#L390

((wt_sum_minrow + pnew) > pMemSum[x]) - always FALSE as pMemSum initialized with 'very big value' so pMem is always updated (initialized at first frame).

To be more math correct - initializing of 'very bad metric': https://github.com/Asd-g/AviSynth-vsTTempSmooth/blob/8bbb7948b201057488db8e1c161471e5d22c5850/src/vsTTempSmooth.cpp#L548C1-L551C33

if (vi_src.ComponentSize() == 1) iMaxSum = 255 _maxr; else if (vi_src.ComponentSize() == 2) iMaxSum = 65535 _maxr;

May be more correct to set (255 + pnew + 1) _maxr for 8bit and (65535 + (pnew 255) + 1)* _maxr ? Because pnew was added some time after IIR-processing and it was missed from this calculation. Or simply set iMaxSum to MAX_INT ? The exact value is not critical - it only must be greater than max possible sum of row + pnew. So at the first frame all IIR memory will be updated with current frame values.

Also https://github.com/Asd-g/AviSynth-vsTTempSmooth/blob/8bbb7948b201057488db8e1c161471e5d22c5850/src/vsTTempSmooth.cpp#L564

2.0f for float max sum may be not always correct too: each max float difference may be about 1.0f or even some larger (what is the max and min float32 samples values in AVS+ ???) so total max sum of very bad case may be fMaxSum = (2.0f + pnew/256.0f) * _maxr . Or we need to select some 'very big float' - may be MAX_FLOAT standard define exist in C language (cross-platform too) ?

If 'good programming practice' requires memory always initialized - it may be set to zero.

Also the IIR-type processing with memory of old data also may have other not yet solved issues - may be at scenechange (first frame of new scene) we need to reset pMemSum to 'very bad value' again so all IIR-memory (best sum and best sample value) will be updated with first frame of new scene (as at the beginning of clip). Currently skipping reset of IIR memory at scenechange may cause some fault of IIR-algorithm (though protected from output too bad sample from previous scene from memory by y/u/vthresh if set not too high).

DTL2020 commented 1 year ago

This commit https://github.com/DTL2020/AviSynth-vsTTempSmooth/commit/13b38b36f36c185c5a6d3a3d6ed9bfee8e962634 should solve issue with max values to init MaxSum memory and also added zero memory of IIRMemY/U/V at constructor to keep programming fine.

Asd-g commented 1 year ago

Btw there is calloc = malloc + zero initialization.