Avisynth+

[ultim]

Avisynth Plugin Writing Tips #2: Parallel execution

Avisynth-MT and VapourSynth both support multithreading, and it is being implemented in Avisynth+ too. All of them require the same things from your plugin. Here is a list of what you as a plugin author can do to support execution on multiple threads. If you are the author of any Avisynth plugin, please update your filter according to these rules if needed. Doing so will make sure your plugin can execute seamlessly when multithreaded. Furthermore, following these rules will not only guarantee correct execution in multihtreaded environments, it will also provide optimal mulithreaded performance.

Short list for those on the run:
- Unless you have the slowest filter in the world, don't start threads in your plugin.
- Never use global or static variables. In addition, your filter class should only have read-only members which are initialized during construction.
- Don't reuse the IScriptEnvionment pointer between method executions.

And again, the same points with a bit of more explanation:
- In general, do not slice up your frame and start multiple threads on your own. Threading has its own performance overhead, and it is only worth doing it manually if your filter takes a lot of time to execute. And even if your filter is extremely slow (like fft3dfilter), you should try to optimize its single-threaded performance (by using SIMD instructions or choosing a more efficient algorithm) rather then manually threading it. Optimize for single-threaded performance, and as long as you follow the other rules below, you will get automatic and correct multithreading from Avisynth.
- Do not cache frames yourself. You might think it is efficient because you won't have to request/compute them in the next frame, but you are wrong, and there are several reasons why. First, if you write your own cache, you will have to introduce a global state to your filter, which means you will have to take care of synchronization between multiple threads too, which is not easy to do efficiently with caches. Second, keeping copies of past frames also means there will always be multiple references to them, thus Avisynth cannot pass them as write pointers to other filters, and will have to do an extra copy of it more often. And last but not least, Avisynth has a very extensive caching mechanism, and if you request the same frame multiple times (even when you need it for different frame requests), chances are you will get it for free anyway, so your own caching is just pure overhead.
- As a general extension to the previous rule, try not to keep any state between frames. In the optimal case your filter class should only have read-only members which are initialized during construction. Surely this is not always possible with every algorithm, but most times it is, and this is what you should strive for.
- As stated before, for best multithreading always try to implement algorithms which require no state between frames. Whenever this is violated, be sure to group reads and writes to the state (do not spread them), and guard them in critical sections (as few and as short as possible). For example, it is a good practice to copy all your writable class variables (in a critical section) at the start of each frame into local stack variables, compute the whole frame outside of the critical section (updating the local variables that captured the global state as needed), then write them back together at the end of your frame in another critical section. Do not request an automatic lock around your whole filter from Avisynth, because it will serialize your filter's execution.
- If you have class variables that must be writable in every frame, you will also have to keep in mind that Avisynth does not guarantee that frames will be processed in their natural order. Just a reminder.
- Only store variables in classes and in method stacks. Per-frame heap allocations should be avoided, because they can act as implicit synchronization points between threads. And most importantly, never store anything in static variables or in the (global) namespace scope. Read the previous sentence a few more times.
- Do not store the IScriptEnvironment pointer anywhere yourself (except locally on the stack), and never reuse those pointers outside of the methods where they were supplied to you. Not even between different executions of the same method! There is a reason why you get that pointer separately for each method, which is that it may be different every time, especially in multithreaded scenarios. If you reuse it, the consequences will be different between every implementation, but you can get anything from race conditions to program crashes.

[Robert Martens]

Originally Posted by ultim

Do not store the IScriptEnvironment pointer anywhere yourself (except locally on the stack), and never reuse those pointers outside of the methods where they were supplied to you. Not even between different executions of the same method! There is a reason why you get that pointer separately for each method, which is that it may be different every time, especially in multithreaded scenarios. If you reuse it, the consequences will be different between every implementation, but you can get anything from race conditions to program crashes.

I have to own up to being a dummy here and having done that, thinking the rule only applied to storing it as a global variable. I had made the--admittedly careless--assumption that storing the environment pointer as a non-static class variable would be safe since each instance of the host would have its own instance of my filter class. I take it I'm supposed to understand that multiple instances of a host application might try to access a single instance of my filter?

Only the slightest bit more consideration put into my problem and I've eliminated what I thought was the need to store the pointer, so it's no big deal for me to bring my code into compliance (luckily I work so slowly I haven't released a version with this problem included), but for the sake of curiosity I thought I'd ask.

[Myrsloik]

Originally Posted by Robert Martens

I have to own up to being a dummy here and having done that, thinking the rule only applied to storing it as a global variable. I had made the--admittedly careless--assumption that storing the environment pointer as a non-static class variable would be safe since each instance of the host would have its own instance of my filter class. I take it I'm supposed to understand that multiple instances of a host application might try to access a single instance of my filter?

Only the slightest bit more consideration put into my problem and I've eliminated what I thought was the need to store the pointer, so it's no big deal for me to bring my code into compliance (luckily I work so slowly I haven't released a version with this problem included), but for the sake of curiosity I thought I'd ask.

Always pass the environment pointer as an argument to your functions. It's a simple rule. Because Avisynth isn't multithreaded you'll never have two threads accessing your filter at once. However it's still a very bad practice. (not sure exactly what the MT side does to get around it)

Note that this issue affects everyone trying to use Avisynth plugins. For example RemoveGrain commits the biggest sin of all. It stores the environment pointer as a global variable when AvisynthPluginInit2 is called. This makes VapourSynth, which creates fake Avisynth environments only when needed, crash. A lot. Since the environment pointer will be different between plugin instances and registering functions.

I'd like to add another filter writing tip myself.

ONLY call AddFunction in AvisynthPluginInit2. Do not throw errors. DON'T DO ANYTHING ELSE THERE. Do initialization when one of your functions is called the first time.

It's the biggest reason for autoloading "breaking" in avisynth and a pile of other issues.

[ultim]

Originally Posted by Robert Martens

I take it I'm supposed to understand that multiple instances of a host application might try to access a single instance of my filter?

Almost. Only one host application will use your filter instance, but that single host application (the Avisynth library to be precise) can use different IScriptEnvironment objects depending on which thread your filter instance is running on. Avisynth-MT, for example, uses special IScriptEnvironments instances to implement thread-local storage (TLS), and I'm implementing something similar in Avs+ too. Apparently, VapourSynth makes use of fake IScriptEnvironments objects too. The thing about TLS objects are, well, that they are local to the thread, so if the given multithreading implementation (for example Avs-MT mode 1) creates just one filter instance for all threads, your filter will get different IScriptEnvironment pointers based on the thread it is running on. Because you cannot know in your filter what threading implementation of Avisynth is in use, it is best not to store the environment pointer between calls at all, and only then will you be safe in every case.

Originally Posted by Myrsloik

Because Avisynth isn't multithreaded you'll never have two threads accessing your filter at once.

Nonononono, forget what Myrsloik said here. This statement can be very misleading, because it is only true for some versions of Avisynth (like classic non-threaded version). The point of this discussion is to let plugins prepare for multithreaded versions (and/or multithreading modes). An important point of my tips is that if you can, you SHOULD assume that multiple threads will be accessing your filter at once, as it really can happen, even with existing implementations.

Originally Posted by Myrsloik

... in AvisynthPluginInit2. Do not throw errors.

A little bit off-topic (as far as multithreading issues go), but for best practices about exceptions, look at this tip.

[Robert Martens]

Thanks for the detail, that clears everything up! I've already eliminated the broken code and pushed the fix, now I just need to sort out one spot in my test executable where I'm catching an AvisynthError to grab its error message and I should be done with your list of Don'ts for the time being.

[Stereodude]

While I don't have anything technical to add, I would like to thank ultim and others for their efforts to modernize AVIsynth by dragging it kicking and screaming into this decade.

[ajp_anton]

If there's room for feature requests, can we skip some arguments when calling functions like this:
some_function(a,b,,d,e)
if I don't want to change the default value for the third argument, and don't want to explicitly use the names of the 4th and 5th arguments.

[Gavino]

Originally Posted by ajp_anton

If there's room for feature requests, can we skip some arguments when calling functions like this:
some_function(a,b,,d,e)

You can get that effect now (more or less) by using (from v2.60):

some_function(a,b,Undefined(),d,e)

or if you prefer

global _ = Undefined()
...
some_function(a,b,_,d,e)

I don't like the idea of allowing some_function(a,b,,d,e) because a user could easily put an extra comma by mistake and not realise he wasn't getting what he expected.

[l33tmeatwad]

Came across something interesting when testing things....if you have fft3dfilter (version 2.1.1) in a script twice and the bw & bh settings in one is 3x or more than the other then there is a green and red bar at the top. This odd glitch only happens in AviSynth+ and does not show up when using AviSynth 2.6 a5. Figured this was worth mentioning, I haven't tested this on another machine yet.

[cwk]

I also get the green bar when using the fft3dfilter, although I see it with just a single placement of the tool within a script. I am working around it for the time being by merging chroma back in at the end. I'll admit this means you can't use fft to filter your chroma.

[TurboPascal7]

Please post the minimal script you can reproduce the issue with (I couldn't). Also, what colorspace and what resolution you're working with?

[cwk]

I can see it with the following script

avisource("1_fields.avi")
converttoyv12()
fft3dfilter()

The source file is huffyuv, 688 x 476. I also see the lines if I resize to 704x480 or 720x480. fps is 59.940, if that matters.

[TurboPascal7]

Reproduced, thank you.
----
Okay, it's a bug in env->BitBlt.

fft3d calls it in CoverbufToPlanarPlane

for (h=0; h<dst_height; h++)
{
    env->BitBlt(dstp, dst_pitch, coverbuf1, coverpitch, dst_width, 1); // copy pure frame size only
    dstp += dst_pitch;
    coverbuf1 += coverpitch;
}

That in BitBlt, passing the height == 1 condition, goes to

#ifdef X86_32
if (GetCPUFlags() & CPUF_INTEGER_SSE)
    memcpy_amd(dstp, srcp, src_pitch*height);  // SSE
else
#endif
    memcpy(dstp, srcp, src_pitch*height);      // fallback
}

which in any case ends up broken because you actually have to copy row_size bytes instead of src_pitch bytes in this case.

So either we could remove the height == 1 check, replace src_pitch with row_size (they're equal when height != 1) or fix fft3d to call BitBlt once for the whole frame instead of every line.

Original Avisynth actually does it right:

if (height == 1 || (src_pitch == dst_pitch && dst_pitch == row_size)) {
    memcpy_amd(dstp, srcp, row_size*height);
} else {
    asm_BitBlt_ISSE(dstp,dst_pitch,srcp,src_pitch,row_size,height);
}

[ultim]

Originally Posted by TurboPascal7

Okay, it's a bug in env->BitBlt.

You might be right. Actually I'm greatly annoyed by Avisynth's BitBlt, IMHO it is a messed up implementation, but when I "fixed" it last time, it broke some plugins doing interleaved copy of non-interleaved frames. Even today I still don't know if the ability to do such copies is intentional, or is it an accident that plugins started to rely upon. Anyway, I changed "row_size*height" to "src_pitch*height" because I thought it is much more logical, thinking they are the same anyway because of the condition row_size==dst_pitch==src_pitch that guards it. Here is where I made the error: I accidently overlooked that this last condition need not be true if height==1. Fix in next release. And thx for looking into this for me.

edit: added clarification of what plugins got broken.

[Guest]

"row_size==dst_pitch==src_pitch"

Are you sure about that? I was under the impression that a Crop can change that. It's always been a common error with Avisynth plugins to assume that they are the same.

[TurboPascal7]

Originally Posted by neuron2

"row_size==dst_pitch==src_pitch"

Are you sure about that? I was under the impression that a Crop can change that. It's always been a common error with Avisynth plugins to assume that they are the same.

This is irrelevant. It's simply a condition. If it isn't true - the code will go the other way, that is all.

[Guest]

Thanks for the lesson on coding but as a condition "row_size==dst_pitch==src_pitch" is nonsense.

[ultim]

Originally Posted by neuron2

"row_size==dst_pitch==src_pitch"

Are you sure about that? I was under the impression that a Crop can change that. It's always been a common error with Avisynth plugins to assume that they are the same.

Of course that equality needs not be true, not just because of crop, also because of alignment. But the piece of code in question here is guarded by an "if" to be able to take a more optimal path. It does not actually require that row_size==dst_pitch==src_pitch, if that is unmet, it will take a slower path.

Edit: I do find it a nonsense that the code is therefor optimized for the minority of the cases, which is (only) part of the reason I hate the BitBlt implementation. Fixing that though is not trivial due to existing plugins relying on the current implementation.

[TurboPascal7]

neuron2, no problems, don't hesitate to ask. By the way, this condition is also present in the original avs core. So you're a bit late with the nonsense part.

[Guest]

LOL. I don't actually need any help or guidance from you in SW design or coding. The stated condition was nonsense in the post that I responded to. Let it go, friend, or take it to PM.