Fixed Dup and Cnr2 for Avisynth 2.5 beta

[Guest]

I have created fixed versions of Dup() and MarcFD's Cnr2() that will run with Avisynth 2.5 beta. These versions should fix the crashing that has been encountered. Please let me know if you experience any crashing when using either of these two filters.

This post has Dup(); the next will have Cnr2().

[Guest]

Here is MarcFD's Cnr2() updated and fixed for Avisynth 2.5 beta.

[sh0dan]

Plugin package updated.

[scmccarthy]

I made some changes to Cnr2 also, if you don't mind.

1) Removed lastn, no longer used.
2) Replaced memcpy with BitBlt, following sh0dan's example.
3) Eliminated ~Cnr2(). Since AviSynth uses smart pointers, it is not needed.
4) Took Create() out of the Cnr2 class. Again, just because it is not nessesary.

Getting rid of ~Cnr2() is something sh0dan asked for and might be important.

Stephen

[Guest]

@scmccarthy

I am awaiting a response from marcFD about how he wants to handle further development on his filters. I'd prefer not to further fragment its ownership right away. I am delaying your attachment until I hear and until I run it through my test suite.

As for sh0dan's concern, I had already addressed it by removing the video frame destructor call. Also, where are the delete's for py and cy after you nuked the entire destructor? Anyway, let me hear from Marc (or not) and then we can decide the future of it.

I hope that is satisfactory for the moment.

[scmccarthy]

@neuron2

If you run it through your test suite yourself and release it yourself, you effectively avoid fragmenting it this time. Sh0dan did say this in the other thread:He also tries to deallocate the frame, when his filter is destroyed, which it shouldn't (it is already being automatically destroyed). Therefore some people will recieve access violations on unload. I think he is talking about ~Cnr2(). That is the sort of thing that needs to be included when creating a class, except with avisynth we are dealing with smart pointers, making it unnesessary and possibly dangerous.

Stephen

[sh0dan]

No - I was talking about the videoframe destruction he made. But I think Donald fixed that - the other changes seem nice. I seem to remember the inner loop being fairly unoptimized, but also that it was heavily memory-limited.

Edit: Looked through the source - it's as it should be now.

[scmccarthy]

Edit: Looked through the source - it's as it should be now. OK, I'll look again, but all the changes I made, I reapplied to neuron2's version. All of his changes are there.

Stephen

[scmccarthy]

By the way, I really am curious whether using destructors like ~Cnr2() is a good programming practice for avisynth plugins. I try to learn from other programmers techniques, so I look around and most plugin writers don't use them. On the other hand, most filter writers are probably not primarily C++ programmers. It seems to work the same with or without it.

Stephen

[Guest]

It depends (among other things) on what you are doing in your constructor. If you are allocating storage in your constructor, for example, you'll want to use the destructor to free it. That's why I pointed out the missing delete's in your changes to Cnr2. You had new's in the constructor but you removed the destructor that contained the corresponding delete's. That produces a memory leak.

Other examples are seizing and freeing semaphores, opening and closing files, etc. The destructor administers the last rites to your filter instantiation. :)

[scmccarthy]

@neuron

Thanks for the answer. What about my other changes? I think lastn does not do anything after you remove the condition for MakeWritable. And why is Create a method in the Cnr2 class? Is it better that way?

Stephen

[Guest]

You are correct about lastn. It is a superfluous declaration now. The Create can be a method or not; it is a matter of taste in our context. It was MarcFD's decision to do it that way and I saw no reason to change it.

For your amusement:

Top Ten Excuses for Memory Leaks

10. A little memory leak is no big thing.
9. Nobody will ever find out.
8. Complex programs always have memory leaks.
7. My Application Framework is too entangled.
6. The memory leak is in the operating system.
5. The memory leak is in my third party tools.
4. Product development cycles are too fast.
3. It's hard to get good programmers today.
2. Leaks and bugs are the price you pay for GUI.
1. Everything else leaks, why shouldn't mine?

[scmccarthy]

neuron2

I studied C++ before and remember now that I must pair each new with a delete. I am putting that back in. I hate memory leaks. I use the YV12Example plugin as my starting point. I don't want to depart from that programming style without good reason. The main idea is to cultivate a style that others can understand and learn from. Did you notice I declared the integers outside the for loop?

Stephen

[Bulletproof]

I think you might wanna look into eDeen also (one of Marc's filters), I think it's causing dup to act wacky, when I enable eDeen then Dup blends all the frames and the overlayed "show" text has values like "-J" and -1 under frame differences. I have 2 denoisers before dup and its not causing anything like that and the thresholds im using on eDeen are very low.

[scmccarthy]

Are you using the new dub 2.01?

[Bulletproof]

Yep.

[Guest]

Originally posted by Bulletproof
I think you might wanna look into eDeen also (one of Marc's filters), I think it's causing dup to act wacky, when I enable eDeen then Dup blends all the frames and the overlayed "show" text has values like "-J" and -1 under frame differences. I have 2 denoisers before dup and its not causing anything like that and the thresholds im using on eDeen are very low. Help me out by giving me the script that fails. :)

EDIT: Never mind, I've duplicated it.

[trbarry]

Maybe I was just doing something dumb but all my experiences with using destructors with my early Avisynth filters seemed to lead to crashes.

Maybe that's why you don't see as many of them in Avisynth filters.

- Tom

[Guest]

@Bulletproof

Regarding MarcFD's edeen() before Dup(). edeen() is failing to clear the multimedia state [__asm emms;] after doing its MMX work. That will crash the floating point operations of any following filters. Dup() is an innocent victim. MarcFD has not released the source code and is (so far) not responding to (my) emails, so I can't fix it where it should be fixed.

As a workaround in Dup() I now clear the state in my constructor. See the attachment in the next post. This should probably be a standard precaution in all filters using floating point.

[Guest]

Here is the workaround:

[scmccarthy]

@TOM

Failing to delete when it is needed will cause a memory leak. But referncing a deleted variable will cause a crash. So only put the delete operator in a method with a tilde in front of it.

Look at the Cnr2 code. It is a good example, because it is very small. Other than in the Create method, there are two variables created with the new operator. They are deleted in ~Cnr2. That is the only safe place to put the deletes.

Stephen

[timeToy]

Replacing the whole frame is very good for Anime, but what about a more flexible filter that do the exact same thing as DUP but work on a block basis.
This can allow to drastically improve compressibility in low motion or no motion parts of "real life" movies.

Thoughts ?

[Guest]

Originally posted by timeToy
Replacing the whole frame is very good for Anime, but what about a more flexible filter that do the exact same thing as DUP but work on a block basis.
This can allow to drastically improve compressibility in low motion or no motion parts of "real life" movies.

Thoughts ? Yes, that was suggested in the main Dup thread. I am not convinced because it effectively approaches a traditional temporal filter. I also worry that the blocking may become visible. But some experiments might be in order at least.

[sh0dan]

@neuron: On the blockbased approach. Several approaches could be used.

- Overlapping detection. Besides the ordinary detection there could be two additional tests offset by 16 pixels (one offset on x and one offset on y), so that slight movement from one block to another get's picked up.
So you get "changing blocks" in 16x16 squares, but you're always detecting on 32x32 blocks.

- Border triggering. If a block is found to be changing, it also triggers the four directly bordering blocks.

But I'm not sure how they'd perform in real-life - I guess there still would be stuff like a few blinking pixels that would become filtered out.

btw, I promised you an ISSE optimized basic detection loop - it's not forgotten. ;)

[Guest]

Thank you for your ideas, sh0dan. I've considered such things in other contexts (combing detection), but I'm not so much worried about the detection aspects as I am about the display ones. i.e., can it produce disjointed objects and other effects (they would be worse at higher thresholds). Maybe it would be fine; as I say experiements are in order. But would it give us anything more than a good temporal filter?

[sh0dan]

(Attachment is source ONLY!)

@neuron:

An optimized Dup-filter...

As promised I looked at your Dup filter.

I wrote an ISSE optimized detection filter, for detecting duplicates. It produces identical results - at least in my tests. The code is in #ifdefs, so you can make your own tests!

- Border issues: The filter does not handle borders 100% as your filter. It has to ignore borders, as it is only capable of processing mod 32 blocks. All borders get's a "0" written in the appropriate box.

- Bottom border: Are completely ignored - same reason as above.

- Blend: I added code for much faster blend. As I see it, your current algorithm is not correct.

It currently runs through all images, like this:

for (int p=0;p<frames_to_be_blended;p++) {
for (int xy=0;xy<all_pixels) {
dst = (dst+frame[p])>>1;
}
}

This will create an uneven distribution when there are more than 2 frames.

If frames=4, then
loop 1:
frame 1 = 50% represented in the final image.
frame 2 = 50% -""-
loop 2:
frame 1 = 25% represented in the final image.
frame 2 = 25% -""-
frame 3 = 50% -""-
loop 3:
frame 1 = 12% represented in the final image.
frame 2 = 12% -""-
frame 2 = 25% -""-
frame 3 = 50% -""-

etc. Am I missing something here?

Anyway, I created a blend routine - it blends one line of a frame. I haven't tested it, but most of the code is taken from temporalsoften, with slight modifications. It accumulates all the values and divides them at the end.

Edit: I guess there is also the matter of rounding in the current blend - even though a "+1" should solve that.

[Guest]

@sh0dan

Thank you for your valuable work and feedback on Dup.

The border issue is serious, I'm afraid. Without a solution, your comparison code can't be used as is. But I think I can use your code for interior blocks and then run my C algorithm on the border blocks.

Your point about the blending is quite valid. I will update Dup to incorporate your solution. Unfortunately I will not be able to do it until I get back from South India at the end of March. It's not broken, just suboptimal, so it can do until I get back. :)

Thank you again for your assistance.

[sh0dan]

- Adding border checks as C-code is of course trivial - It seems like a good thing that I actually used the same structure as you for storing.

- The blend code is a bit less trivial to implement - that's why I didn't do it. The loop has to be rewritten, but it might actually be nicer (the blend is pure MMX, so it could replace the current without any problems).

The implementation could be something like this:


byte** planes;
int* pitches;

for (int i=0;i<frames;i++) {
planes[i] = getReadPtr(plane);
pitches[i] = getPitch(plane);
}

for (int y=0;y<height;y++) {
mmx_blend_line(...);
for (int p=0;p<frames;p++) {
planes[p]+=pitches[p];
}
}


Repeat for all planes. It can also very well be used for YUY2 - if it is mod-4.

[sh0dan]

OK - I completed dup.

It has equally weighted MMX blend routines. Much faster now!
It detects changes in border sections properly.

I spend a good time testing and debugging, and it seems to both perform very well and (hopefully) being bugfree.

I also removed the memory allocation (of the blocks) from GetFrame, so that it's done on a per-filter basis and not on a per-frame basis.

You can still test the new detection routines by changing the define af the top. The #ifdefs should of course be replaced by "if (i_sse)" - this is the only change recessary before a release.

The blend routines have been completely replaced, since the old algorithm wasn't optimal, and it became a mess to have "#ifdefs".

[Guest]

Thank you. sh0dan. I will do a little testing and try to make a release before I leave for India. I will make us co-authors unless you have an objection. If so, do I use "sh0dan" or a real biological name?

[sh0dan]

Just use my real name. Must for amusement I found that having a fairly high thresholds give very surreal effects - just as temporalsoften with very high thresholds also do.

Oh - my indents/tabs are adjusted to 2,2 (I always use spaces) - I just saw how horrible it looked in another viewer - sorry I'll use tabs next time I play around with your code!

[Guest]

OK, attached is a beta based on the new sh0dan code. It improves the blend mode so that it equally weights all the frames involved in a blend and optimizes for speed (especially in the YV12 case).

I've limited time for testing due to my upcoming trip to India, so I am relying on YOU! Please post here if you find any errors. I'm including the source so sh0dan can fix it up in my absence. :)

Thank you once again, Klaus, for investing your time in improving Dup().