A Silent *BUG*

[Ceppo]

I have a strange bug in my filter and I was able to pinpoint the source function but I don't understand what's the problem. VS Project (https://www.mediafire.com/file/0fqalan7ylmjfge/InvertNeg.zip/file)

I will try to also copy here the main code parts so that maybe you don't have to download the project.

This is GetFrame:

PVideoFrame __stdcall InvertNeg::GetFrame(int m, IScriptEnvironment* env) {

int n = m / 2;
int order = child->GetParity(0) ? 1 : 0;

PVideoFrame dst = env->NewVideoFrame(vi);
PVideoFrame src = child->GetFrame(n, env);
PVideoFrame prv = child->GetFrame(n == 0 ? n : n - 1, env);
PVideoFrame nxt = child->GetFrame(n == vi.num_frames - 1 ? n : n + 1, env);

if ( m & 1 )
{
std::int64_t diffC = get_diff(src, src, mode, order, nt, sse);
std::int64_t diffN = get_diff(src, nxt, mode, !order, nt, sse);
field_match(dst, src, nxt, diffC, diffN, !order);
}
else
{
std::int64_t diffC = get_diff(src, src, mode, order, nt, sse);
std::int64_t diffP = get_diff(src, prv, mode, order, nt, sse);
field_match(dst, src, prv, diffC, diffP, order);
}

PVideoFrame met;
bool check = true;

if (m & 1)
{
if (thr < 255 || thr2 < 255)
{
PVideoFrame met = env->NewVideoFrame(vi);
check = calc_combed(dst, met, metric, !order, thr, thr2, src_width, src_height);
}
}
else
{
if (thr < 255 || thr2 < 255)
{
PVideoFrame met = env->NewVideoFrame(vi);
check = calc_combed(dst, met, metric, order, thr, thr2, src_width, src_height);
}
}

if (check && thr2 < 255 && edeint2)
{
PVideoFrame edtb = edeint2->GetFrame(m, env);
copy_frame(dst, edtb);
}
else if (thr < 255 && edeint)
{
if (is_combed(met, metric, order, 4))
{
PVideoFrame edta = edeint->GetFrame(m, env);
copy_frame(dst, edta);
}
}

return dst;
}


Now the bug shows up when this line is executed: if (is_combed(met, metric, order, 4))
The problem with it is the "met" clip as a PVideoframe argument. If I change "met" with another random clip like "src" the bug doesn't show up and all frames are detected as combed as expected.

Now I make another change in the calc_combed() lines and I replace "met" with "src" here as well. The bug shows up again so I'm guessing that calc_combed() does something evil. Here the function:

bool calc_combed(PVideoFrame& dst, PVideoFrame& met, int metric, int order, int thr, int thr2, int src_width, int src_height)
{
unsigned char* metp;
const unsigned char* dstp;
const unsigned char* dstpp;
const unsigned char* dstppp;
const unsigned char* dstpn;
const unsigned char* dstpnn;

int height;
int row_size;
int dst_pitch;
int met_pitch;

double sum = 0;
int p, x, y, c, c1, c2;
int planes[] = { PLANAR_Y, PLANAR_U, PLANAR_V };

if (metric == 0)
{
for (p = 0; p < 3; p++)
{
dstp = dst->GetReadPtr(planes[p]);
metp = met->GetWritePtr(planes[p]);

dst_pitch = dst->GetPitch(planes[p]);
met_pitch = met->GetPitch(planes[p]);

height = dst->GetHeight(planes[p]);
row_size = dst->GetRowSize(planes[p]);

dstp += dst_pitch * static_cast<int64_t>(2);
metp += met_pitch * static_cast<int64_t>(2);

dstpp = dstp - dst_pitch;
dstpn = dstp + dst_pitch;
dstppp = dstpp - dst_pitch;
dstpnn = dstpn + dst_pitch;

if (order == 1)
{
metp += met_pitch;
dstp += dst_pitch;
dstpp += dst_pitch;
dstpn += dst_pitch;
dstppp += dst_pitch;
dstpnn += dst_pitch;
}

for (y = 0; y < (height - 4); y += 2)
{
for (x = 0; x < row_size; x++)
{
c1 = (dstp[x] - dstpp[x]);
c2 = (dstp[x] - dstpn[x]);

c = std::abs((dstppp[x] + (dstp[x] << 2) + dstpnn[x]) - 3 * (dstpp[x] + dstpn[x]));
sum += c;

if ((c1 > thr && c2 > thr) || (c1 < -thr && c2 < -thr))
{
metp[x] = c > thr * 6 ? 1 : 0;
}
else
{
metp[x] = 0;
}
}
metp += met_pitch * static_cast<int64_t>(2);
dstp += dst_pitch * static_cast<int64_t>(2);
dstpp += dst_pitch * static_cast<int64_t>(2);
dstppp += dst_pitch * static_cast<int64_t>(2);
dstpn += dst_pitch * static_cast<int64_t>(2);
dstpnn += dst_pitch * static_cast<int64_t>(2);
}
}
}
else
{
for (p = 0; p < 3; p++)
{
dstp = dst->GetReadPtr(planes[p]);
metp = met->GetWritePtr(planes[p]);

dst_pitch = dst->GetPitch(planes[p]);
met_pitch = met->GetPitch(planes[p]);

height = dst->GetHeight(planes[p]);
row_size = dst->GetRowSize(planes[p]);

dstp += dst_pitch;
metp += met_pitch;

dstpp = dstp - dst_pitch;
dstpn = dstp + dst_pitch;

if (order == 0)
{
metp += met_pitch;
dstp += dst_pitch;
dstpp += dst_pitch;
dstpn += dst_pitch;
}

for (y = 0; y < (height - 2); y += 2)
{
for (x = 0; x < row_size; x++)
{
c = (dstpp[x] - dstp[x]) * (dstpn[x] - dstp[x]);
sum += c;

metp[x] = c > thr * thr ? 1 : 0;
}
metp += met_pitch * static_cast<int64_t>(2);
dstp += dst_pitch * static_cast<int64_t>(2);
dstpp += dst_pitch * static_cast<int64_t>(2);
dstpn += dst_pitch * static_cast<int64_t>(2);
}
}
}

sum /= (2.0 * static_cast<double>(src_width)
* static_cast<double>(metric == 0 ? src_height - 6 : src_height - 3));

return metric == 0 ? sum > static_cast<double>(thr2) * 6.0
: sum > static_cast<double>(thr2) * static_cast<double>(thr2);
}

Now for easy reading, I will explain what it does:
1) Calculates the combed values from "dst" pixels.
2) Keeps track of the combed values and returns a check on the average.
3) Stores in "met" pixels the result of the combed detection as 1 if combed and 0 if not for later use.

Probably is something stupid, but I passed already 2 days without luck. Also I tried to attach avspmod to visual studio debugger and when it gets to the point in which avspmod crashes it says that avisynth.dll throws an exception because of an error while trying to access an address.

The script lines that makes it crash is the following:

InvertNeg(edeint=bob().invert(),thr=6)

[videoh]

The two lines:

PVideoFrame met = env->NewVideoFrame(vi);

should be

met = env->NewVideoFrame(vi);

You are declaring met multiple times and then using the wrong one, because the met you make in the cited lines goes out of scope after you leave the blocks.

Didn't look at the rest.

[StainlessS]

I have not really tried to understand what is going on there, but this dont look right.


PVideoFrame __stdcall InvertNeg::GetFrame(int m, IScriptEnvironment* env) {

int n = m / 2;
int order = child->GetParity(0) ? 1 : 0;

PVideoFrame dst = env->NewVideoFrame(vi);
PVideoFrame src = child->GetFrame(n, env);
PVideoFrame prv = child->GetFrame(n == 0 ? n : n - 1, env);
PVideoFrame nxt = child->GetFrame(n == vi.num_frames - 1 ? n : n + 1, env);

if ( m & 1 )
{
std::int64_t diffC = get_diff(src, src, mode, order, nt, sse);
std::int64_t diffN = get_diff(src, nxt, mode, !order, nt, sse);
field_match(dst, src, nxt, diffC, diffN, !order);
}
else
{
std::int64_t diffC = get_diff(src, src, mode, order, nt, sse);
std::int64_t diffP = get_diff(src, prv, mode, order, nt, sse);
field_match(dst, src, prv, diffC, diffP, order);
}

PVideoFrame met;
bool check = true;

if (m & 1)
{
if (thr < 255 || thr2 < 255)
{
PVideoFrame met = env->NewVideoFrame(vi);
check = calc_combed(dst, met, metric, !order, thr, thr2, src_width, src_height);
}
}
else
{
if (thr < 255 || thr2 < 255)
{
PVideoFrame met = env->NewVideoFrame(vi);
check = calc_combed(dst, met, metric, order, thr, thr2, src_width, src_height);
}
}

if (check && thr2 < 255 && edeint2)
{
PVideoFrame edtb = edeint2->GetFrame(m, env);
copy_frame(dst, edtb);
}
else if (thr < 255 && edeint)
{
if (is_combed(met, metric, order, 4))
{
PVideoFrame edta = edeint->GetFrame(m, env);
copy_frame(dst, edta);
}
}

return dst;
}


EDIT: vh already said that.

[Ceppo]

Thanks, I fixed these lines as you guys said and it comes out that is_combed() as well was bugged, so the problem was a mix of bugs, but now I fixed them. I had the feeling it was something stupid and indeed it was. *2 days wasted for nothing*

Thank you, guys.

[videoh]

*2 days wasted for nothing* https://www.inc.com/christina-desmarais/6-ways-failing-is-necessary-for-success.html

[Boulder]

https://www.inc.com/christina-desmarais/6-ways-failing-is-necessary-for-success.html

True - nothing is wasted if you can learn from it. Next time you probably won't make the same mistake :)

[feisty2]

*2 days wasted for nothing*


that's why you should never declare a variable (without initialization), it's not a preferred practice in C++
the preferred way to create a variable is

auto variable = /*initialization expression*/;
// equivalent to type variable{ /*constructor arguments*/ } since C++17 because of guaranteed copy elision.

and you should always use auto to create a variable because it enforces an initialization.

[StainlessS]

True - nothing is wasted if you can learn from it. Next time you probably won't make the same mistake :)
Unfortunately, its the type of mistake that can be accidentaly repeated time after time, even when you know its wrong.

that's why you should never declare a variable (without initialization), it's not a preferred practice in C++
the preferred way to create a variable is

auto variable = /*initialization expression*/;
// equivalent to type variable{ /*constructor arguments*/ } since C++17 because of guaranteed copy elision.

and you should always use auto to create a variable because it enforces an initialization.

And how would that help, given that the variable was re-declared in a different scope.
Methinks that you just like to extol the virtues of 'auto'.

EDIT: Ceppo, I added an edit to your other thread, may be of use for debugging in future. [maybe you did not see it]
https://forum.doom9.org/showthread.php?p=1935134#post1935134

[feisty2]

and you should not re-declare the same variable in different scopes, this whole code block


PVideoFrame met;
bool check = true;

if (m & 1)
{
if (thr < 255 || thr2 < 255)
{
PVideoFrame met = env->NewVideoFrame(vi);
check = calc_combed(dst, met, metric, !order, thr, thr2, src_width, src_height);
}
}
else
{
if (thr < 255 || thr2 < 255)
{
PVideoFrame met = env->NewVideoFrame(vi);
check = calc_combed(dst, met, metric, order, thr, thr2, src_width, src_height);
}
}

if (check && thr2 < 255 && edeint2)
...


screams poor design and chaotic logic, it should be at least restructured to something like

auto check_combed = [&] {
if (auto met = env->NewVideoFrame(vi); thr < 255 || thr2 < 255)
return std::tuple{ calc_combed(dst, met, metric, m & 1 ? !order : order, thr, thr2, src_width, src_height), std::move(met) };
else
return std::tuple{ true, std::move(met) };
};

if (auto [check, met] = check_combed(); check && thr2 < 255 && edeint2)
...

[StainlessS]

and you should not re-declare the same variable in different scopes
Yep that was the accidental problem.

Very mystical your code snippet, I assume requires CPP v20 and does not compile with anything less.
I like portabilty, not having to have latest compiler just to make something work at all. [EDIT: or near total re-write if you wrote it]

EDIT:
screams poor design and chaotic logic
And I think is Ceppo's first plugin project (InvertNeg name is one of the SimpleSample demos [EDIT: examples] I think),
Ceppo is probably less interested in optimal design at this point.

[feisty2]

it's C++17 and it's supported by all mainstream compilers, GCC, Clang, MSVC, Intel C++, nvcc, you name it.


Ceppo is probably less interested in optimal design at this point.

unless the program structure is bad enough to hide errors that cost you days to debug.

[StainlessS]

OK, I stand corrected. I still use VS 2008 Express, no idea what version that is [dont tell me, I don't care :) ].

EDIT: I do have Community 2015, 2017, and 2019, I'll switch to one of those when I get my new machine setup properly,
but doubt if I'll be using 'auto' for a few years at least. [also, this ol' hound dog don't wanna learn any new tricks, I'm happy with Standard/ISO C]

[feisty2]

I'm happy with Standard/ISO C

really? (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2494.pdf)

[wonkey_monkey]

screams poor design and chaotic logic, it should be at least restructured to something like

auto check_combed = [&] {
if (auto met = env->NewVideoFrame(vi); thr < 255 || thr2 < 255)
return std::tuple{ calc_combed(dst, met, metric, m & 1 ? !order : order, thr, thr2, src_width, src_height), std::move(met) };
else
return std::tuple{ true, std::move(met) };
};

if (auto [check, met] = check_combed(); check && thr2 < 255 && edeint2)
...


What's wrong with


// declare met here too, if it's needed outside the scope below; original code not clear
bool check = true;

if (thr < 255 || thr2 < 255) {
auto met = env->NewVideoFrame(vi);
check = calc_combed(det, met, metric, m & 1 ? !order : order, thr, thr2, src_width, scr_height); // I'd be tempted to do "order ^ (m & 1)"
}

if (check && thr2 < 255 && edeint2) { ...


?

Seems rather more readable than messing around with tuples and lambdas (if that's what that was; not of all us are up to speed on these things).

[feisty2]

it's a different programming paradigm, mine is slightly more functional, so it avoids mutating state whenever and wherever possible. cross-scope mutations (like what you did to that "check" variable, you modified it in an "if" scope and read it in another "if" scope) are generally very error prone, and I don't want that.

function literals (lambdas) are EXTREMELY common in modern C++, get used to it.

[Ceppo]

Thank you guys for your opinions, as I said in the other post I started C++ after Christmas and I'm not very used to more complex but optimal ways to handle a problem. For the moment I will just try to solve the problem in the most basic way and build my skills along the way by buying more books etc. I will keep in mind your advices.

[StainlessS]

Yep Ceppo, you just stick to step by step learning, Sheldon Cooper [aka feisty] thinks that we should all be as brilliant as him,
but to most of us, he just seems to keep spouting gibberish :)