MakeWritable bug ?

[Fizick]

I try simple code:

#include "windows.h"
#include "avisynth.h"

class Wiener : public GenericVideoFilter {
public:
Wiener(PClip _child, IScriptEnvironment* env);
~Wiener();
PVideoFrame __stdcall GetFrame(int n, IScriptEnvironment* env);
};

Wiener::Wiener(PClip _child, IScriptEnvironment* env) :
GenericVideoFilter(_child) {
// child->SetCacheHints(CACHE_RANGE,1); // all is fine when enabled
}

Wiener::~Wiener() {
}

PVideoFrame __stdcall Wiener::GetFrame(int n, IScriptEnvironment* env) {
PVideoFrame src = child->GetFrame(n, env);
env->MakeWritable(&src);
if ( (n == 0) || (n >= (vi.num_frames -1)) )
return src; // if first or last frame, then no temporary filtration
PVideoFrame prev = child->GetFrame(n-1, env); // get previous frame
unsigned char* srcp = src->GetWritePtr();
srcp[0] = 0;
return src;
}

AVSValue __cdecl Create_Wiener(AVSValue args, void* user_data, IScriptEnvironment* env) {
return new Wiener(args[0].AsClip(), // the 0th parameter is the source clip
env);
}

extern "C" __declspec(dllexport) const char* __stdcall AvisynthPluginInit2(IScriptEnvironment* env) {
env->AddFunction("Wiener", "c", Create_Wiener, 0);
return "`Wiener' Wiener plugin";
}



I use a script:

blankclip()
loadplugin("wiener.dll")
converttoyv12()
separatefields()
wiener()


It produced the error:
Avisynth: caught an accsess violation at 0x013a13ab
attempting to write to 0x00000000

So, there is bad write pointer for src.

When i remove separeatefields, all is fine.
When i remove converttouv12(), all is fine.

When I enable cache>0, all is fine.
When I disable previous frame line, all is fine.

Avisynth 2.5.6RC1 and 2.55.

[IanB]

PVideoFrame __stdcall Wiener::GetFrame(int n, IScriptEnvironment* env) {
PVideoFrame src = child->GetFrame(n, env);
env->MakeWritable(&src);
...
PVideoFrame prev = child->GetFrame(n-1, env); // get previous frame
unsigned char* srcp = src->GetWritePtr();
srcp[0] = 0;
return src;
}I use a script:
blankclip()
loadplugin("wiener.dll")
converttoyv12()
separatefields()
wiener()Ouch! You are being stung by a very subtle race condition caused by SeparateFields(), Crop and YV12 SwapUV also expose this. To maximise throughput these function just do pointer arithmetic and return a new PVideoFrame pointing at the original VideoFrameBuffer. The performance gain is considerable, the alternative is to do a copy of the VFB. There was strong discussion in the 2.6 thread about the pros and cons of bit blitting frames versus pointer arithmetic.

Your code :-
1. Asks for a PVideoFrame
2. Checks to see if it is the only user with MakeWritable.
3. MakeWritable confirms this is currently true and does nothing.
4. Now, inadvertantly, ends up asking for a 2nd copy of the same VFB thru SeparateFields().
5. Now asks for the write pointer.
6. GetWritePtr() checks to see if you are the only user, finds you are NOT and return a null pointer.
7. BANG!

To avoid the race move you MakeWritable() to just before the GetWritePtr().

As a rule do all the GetFrame() calls together at the top of your code, this will correctly reflect the sharing state of all PVideoFrames.

Generally with Temporal style filters using NewVideoFrame() for the output can give improved performance by not whacking the cache copies of the input frames, which either get regenerated or copied needlessly when using in-place modifies.

IanB

[mg262]

6. GetWritePtr() checks to see if you are the only user, finds you are NOT and return a null pointer.
7. BANG!

How about having 6 throw a exception instead of returning a null pointer?

Edit: Or (in my opinion) even better, although it might be trickier to implement due to baked in code, have the MakeWritable return a pointer of type UniquePVideoFrame; as long as this object exists, it is illegal to try to alias the frame -- i.e., asking for another pointer to the same frame will throw an exception. (It will of course be destroyed at the end of the relevant function, but you could also have a release() member function.) Existing code will simply discard the UniquePVideoFrame and so behave as it always has.

[Fizick]

IanB,
thanks for answer and solution!

I can not put all GetFrame statements at the start of code, spme of them are in conditional blocks.
But pairing of MakeWritable and GetWritePtr together gives correct result.
But how it works? Consider one of correct code variant:


PVideoFrame __stdcall Wiener::GetFrame(int n, IScriptEnvironment* env) {
PVideoFrame src = child->GetFrame(n, env);
if ( (n == 0) || (n >= (vi.num_frames -1)) )
return src; // if first or last frame, then no temporary filtration
env->MakeWritable(&src);
unsigned char* srcp = src->GetWritePtr();
PVideoFrame prev = child->GetFrame(n-1, env); // get previous frame srcp[0] = 0;
return src;
}

1. n=0 (first field of very first frame)
it simply returned src (if ...) as a pointer
2. n=1 (second field of very first frame)
it can not make inplace edition ? So Avisynth makes a copy of VFB (whole, not one field only - correct?)
3. n=2 (first field of second frame).
Get write pointer for in-place edition.
4. n=3 (second field od second frame)
This VFB was in use at previous step, so Avisynth make a copy (of whole VFB).

and so on...

Last note: Yes, NewVideoframe is better for temporal. I use it now, but MakeWritable gives more simple code :) (without BitBlt for other planes)

[tsp]

easy solution: Increment the vfb.sequencecount in MakeWritable (because the VFB will be change else why call MakeWritable) this will force the cache to recreating the frame. This also fixes the problem with the latest fft3dfilter (because more than one thread is it possible that another thread request the frame between makewritable() and GetWritePtr()) and avisynthMT (that means the fix will go into avisynth 2.60 if not someone have anything against it.)

[Fizick]

tsp,
yes, the code above is a little simplfied fft3dfilter 1.7 code :)
But i have prepare 1.8 now.

[IanB]

How about having 6 throw a exception instead of returning a null pointer?Could do that, but runtime Avisynth exceptions are a might useless outside of the VirtualDub* sphere. Also this would kill any code using the NULL return as an IsNotWriteable flag.
Edit: Or (in my opinion) even better, although it might be trickier to implement due to baked in code, have the MakeWritable return a pointer of type UniquePVideoFrame; as long as this object exists, it is illegal to try to alias the frame -- i.e., asking for another pointer to the same frame will throw an exception. (It will of course be destroyed at the end of the relevant function, but you could also have a release() member function.) Existing code will simply discard the UniquePVideoFrame and so behave as it always has.This is getting towards David's 3.0 inititive, he handles this whole issue much more cleanly (by not having any MakeWriteable() ).


I can not put all GetFrame statements at the start of code, some of them are in conditional blocks.
But pairing of MakeWritable and GetWritePtr together gives correct result.
But how it works? Consider one of correct code variant:
PVideoFrame __stdcall Wiener::GetFrame(int n, IScri...nt* env) {
PVideoFrame src = child->GetFrame(n, env);
if ( (n == 0) || (n >= (vi.num_frames -1)) )
return src; // if first or last frame,
// then no temporal filtration
env->MakeWritable(&src);
unsigned char* srcp = src->GetWritePtr();
PVideoFrame prev = child->GetFrame(n-1, env); // prev frame
srcp[0] = 0;
return src;
}You still have a problem with this code. Yes you have avoided the NULL pointer, but you now have the potential to corrupt your copy of "prev", you might as well just have done a GetReadPtr() and used it to write with, the effect is the same, shoot yourself or electrocute yourself, you are still dead both ways. In your case you will get away with it because you ignore the data in the pitch-width space (the other field pixels) but it is generally bad practice.

As I said, you should do all the GetFrame() calls BEFORE you get any pointers and start manipulating any data. By all means put appropriate test where you need them, just don't get the pointers until you have got all the frames.
PVideoFrame __stdcall Wiener::GetFrame(int n, IScriptEnvironment* env) {
// Get all the frames
PVideoFrame src = child->GetFrame(n, env);
if ( (n == 0) || (n >= (vi.num_frames -1)) )
return src; // if first or last frame, then no temporary filtration

PVideoFrame prev = child->GetFrame(n-1, env);
if (moon_is_blue)
return prev;

PVideoFrame next = child->GetFrame(n+1, env);
if (saturn_is_rising)
return next;

// Now get pointers
env->MakeWritable(&src);
unsigned char* srcp = src->GetWritePtr();
unsigned char* nextp = src->GetReadPtr();
unsigned char* prevp = src->GetReadPtr();

// Now do magic
srcp[0] = 0;

// Return inplace modified frame
return src;
}

1. n=0 (first field of very first frame)
it simply returned src (if ...) as a pointer
2. n=1 (second field of very first frame)
it can not make inplace edition ? So Avisynth makes a copy of VFB (whole, not one field only - correct?)No! MakeWritable() only copies the active part of the frame to a completely new frame with a default pitch.
3. n=2 (first field of second frame).
Get write pointer for in-place edition.
4. n=3 (second field od second frame)
This VFB was in use at previous step, so Avisynth make a copy (of whole VFB).

and so on...Actually the order you are doing thing in potentially causes the GetFrame(n-1, env); to regenerate a new frame every time. You ask for frames 0 and 1 then modify frame 1, next cycle you ask for frames 2 and 1, but 1 was modified, hence must be regenerated. For best performance with in-place temporal algorithms you should be modifying the frame you won't ever need again. i.e. prev.

Thus ideally the access pattern would be ask for 1 and 0, modify 0, ask for 2 and 1, yes 1 was not modified last time so comes from cache, modify 1 now, ask for 3 and 2, etc. Of course this assumes a forward linear input request pattern, if some script has a Reverse() in it then the concept comes unstuck.

The 2.5.6 Cache has a heuristic detector for this and will lock VFB's when it detect this pattern so subsequent MakeWriteable() calls are forced to return a copy. It generally takes 3 such requests to kick in.

Using CACHE_RANGE mode, all AVS versions, has the cache hard protects its frames so subsequent MakeWriteable() calls are always forced to return a copy.

Last note: Yes, NewVideoframe is better for temporal. I use it now, but MakeWritable gives more simple code :) (without BitBlt for other planes)For maximum performance it is always best for the programmer explicitly control when and how frames are copied. Relying on hidden internal AVS blits could halve the speed of your filter. Remember blits are slow, on SSE2 (P4, hammer, etc) capable processors it is easy to write filters with twice the throughput of the current blit engine.


easy solution: Increment the vfb.sequencecount in MakeWritable (because the VFB will be change else why call MakeWritable) this will force the cache to recreating the frame.Yep! That would help. Also opens the possibility of more hints back into the cache.

IanB

[Fizick]

1. No! MakeWritable() only copies the active part of the frame to a completely new
frame with a default pitch.
May I ask for full clarity:
what is "active part"? active part of whole frame (size=frame height*width) or active part of this field (half frame height * width) ?

2. IMHO, current implementation of MakeWritable is not (plugun-writer)-friendly.
Often I do not know, what frame will be needed. For example i use internal frame (fft) cache in depan, fft3dfilter, etc. If I already have FFT(n-1), i do not need in frame n-1.
What is the cost of requesting of many videoframes, which will not be used in current step?

3. in latest fft3dfilter v1.8 I use dst=NewVideoFrame. :)

[mg262]

Since we are (I think) discussing efficiency, here is something I occasionally use that may be useful in this context:
PVideoFrame source = child->GetFrame(n, env);
PVideoFrame created = (source->IsWritable() ? 0 : (env->NewVideoFrame(vi)));
PVideoFrame &result = (source->IsWritable() ? source : created);

const unsigned char *otherrow = other->GetReadPtr();
unsigned char *resultrow = result->GetWritePtr();
const unsigned char *sourcerow = (source->IsWritable() ? resultrow : source->GetReadPtr());The idea is to write to the source if this is possible and otherwise to write to a new frame.

[IanB]

1. No! MakeWritable() only copies the active part of the frame to a completely new frame with a default pitch.
May I ask for full clarity:
what is "active part"? active part of whole frame (size=frame height*width) or active part of this field (half frame height * width) ?BitBlt(dst->GetWritePtr(), dst->GetPitch(),
src->GetReadPtr(), src->GetPitch(),
src->GetRowSize(), src->GetHeight());Src is whatever the filter chain does to make the frame to that point. e.g. SeparateFields halves the height and doubles the pitch in the PVideoFrames it processes, it also bumps the origin 1 line for bottom fields.

2. IMHO, current implementation of MakeWritable is not (plugin-writer)-friendly.No argument here ;) Within the group I work with, it's what we call a Cobol programmerism.Often I do not know, what frame will be needed. For example i use internal frame (fft) cache in depan, fft3dfilter, etc. If I already have FFT(n-1), i do not need in frame n-1.
What is the cost of requesting of many videoframes, which will not be used in current step?They all have to be rendered at least once. Hopefully the cache will be able to hold onto them until you request them again. If it cannot then they will need to be re-rendered.

Generally it is assumed that a GetFrame() implementation only calls source GetFrame() methods needed to satisfy the current GetFrame() request. But it is not set in concrete. The API is very flexible and if a filter class wants to implement a class level array of PVideoFrames to privately cache frames, who am I to say you cannot. It might not be very wise to lock down 500 PVideoFrames representing a 720x576 rgb32 image, this represents 800Mb of memory, so it might not work very well on a 512Mb machine and will cetainly need an appropriate SetMemoryMax() to enlarge the VFB pool enough to cope on hardware that is large enough.

Both BlankClip and ColorBars allocate and paint a single class global PVideoFrame to hold the prototype frame in the constructor and returns that one frame on each and every GetFrame() call so there is precident. Frames allocated this way come out of the global VFB pool so do reduce the cache size.

IanB

[Fizick]

IanB,
Thanks for long answer,
it will be useful for many plugin-writers.
But i am still do not understand nothing :)
(what i did not know previously ;)
1. about my first question.
of course i know, that
SeparateFields halves the height and doubles the pitch in the PVideoFrames it processes ...
So, in this case Avisynth (makewritable) will copy source frame, which is only one field of original (not field-separated) frame ?
Previously i thinked, that Avisynth will copy whole original frame.
But how Avisynth will be weave two fields, if it have only one field copied?
May be it is a stupid question.

2. About my second question. I say about my own intermnal FFT cache (not Avisynth)
I try be more specific. let pseudocode is:

PVideoframe src = getframe(n)

if (fft of frame n exist in fftcache)
complex * FFT0 = getFFT_FromCache(n)
else
FFT0 = makefft(src); and put FFT0 to fftcache

if (fft of frame n-1 exist in fftcache)
complex * FFT1 = getFFT_FromCache(n-1)
else
{
PVideoframe prev= getframe(n-1);
FFT1 = makefft(prev);
put FFT1 to fftcache
}

if (fft of frame n+1 exist in fftcache)
complex * FFT2 = getFFT_FromCache(n+1)
else
{
PVideoframe next =getframe(n+1);
FFT2 = makefft(next);
put FFT2 to fftcache
}


fftout=DoSuperFiltration(fft0,fff1,fft2)
RevertFFT(fftout, src)
return src

So, only needed frames are requested.
(I omit code to reorganize the FFTcache)

Must I put all Getframes() at the top? But I will not use some PVideoframes at current step!

In DepanStabilize i use very large number of frames.

[IanB]

1. about my first question.
of course i know, that
SeparateFields halves the height and doubles the pitch in the PVideoFrames it processes ...
So, in this case Avisynth (makewritable) will copy source frame, which is only one field of original (not field-separated) frame ?
Previously i thinked, that Avisynth will copy whole original frame.
But how Avisynth will be weave two fields, if it have only one field copied?
May be it is a stupid question.Short answer, Weave() does 2 consecutive GetFrame calls 1 for the top field and another for the bottom field. The 2 resultant PVideoFrames are BitBlt'ed into the output. Any extra data (other field) is mutually ignored. If the 2 PVideoFrames share a VFB it is simply not relevant.

Long Detailed (Boring) Answer
PVideoFrames are smart Struct's they contain a pointer to a VideoFrameBuffer (VFB) and parameters how to address and access the video information stored there. Paramters like origin, height, width and pitch. They also have replacment C++ operator overload code which manages the use count of the VFB and the PVideoFrame itself.

Take the simple piece of script:-...
SeparateFields()
Weave()Now the code for SeparateFields() does a GetFrame(n/2) which returns a PVideoFrame which describes the full interlaced frame, i.e. both fields. It then uses the SubFrame method to return a new PVideoFrame using the same VFB but with different parameters (half height, double pitch). On the next call, this time for the bottom field, GetFrame(n/2) returns a PVideoFrame describing the same full frame as before AND with the same VFB as before from the cache. Again a call to the Subframe method and a new returned PVideoFrame (origin+=pitch, half height, double pitch).

Now the code for Weave() does 2 GetFrame calls, 2*n & 2*n+1. It then does NewVideoFrame and BitBlt's the pixel contents from the 2 PVideoFrames into the output. It does not actually know or care that both PVideoFrames shared the same VFB but with differing origins.

Now complicate the script 1 level:-...
SeparateFields()
InPlaceFilter()
Weave()In this case the 1st GetFrame call from Weave() goes thru InPlaceFilter() which in turn does a GetFrame call to SeparateFields(). It does a MakeWritable on the returned PVideoFrame, which does nothing because it currently is the only user of the contained VFB. The contained VFB is now MODIFIED! The 2nd GetFrame call from Weave() does the same thing, BUT the resultant GetFrame call in SeparateFields() has to return a freshly generated version of the frame, because the Cached copy was modified and hence expired. Thus the 2 PVideoFrames have different VFB's.

However this is WRONG! The cache is intelligent. It notices the VFB being modified and protects it by incrementing the inuse count. What actually happens on the 1st GetFrame path is that MakeWritable() is forced to make a copy of the PVideoFrame complete with a new VFB, containing only the top field data. On the 2nd GetFrame path SeparateFields() gets the original VFB from the cache, it is NOT protected this time, MakeWritable() does nothing, this path is the only user, the 1st path took a copy and released its use. Weave() now BitBlt's the pixel contents for the 2 PVideoFrames into the output. Again the 2 PVideoFrames have different VFB's, but for a different reason.

For a fuller understanding build a Debug avisynth.dll and step thru the execution of the GetFrame chain. This description really only scratches the surface and omits much complexity for easier explanation.


2. About my second question. I say about my own internal FFT cache (not Avisynth)
I try be more specific. let pseudocode is:
...
Must I put all Getframes() at the top? But I will not use some PVideoframes at current step!

In DepanStabilize i use very large number of frames.No, you do not HAVE to put all the GetFrame() calls at the top. I was just offering some generic advise. Doing so helps avoid coding race conditions when using the horrible MakeWritable(). If your algorithm codes better with GetFrame() in the middle, do so. Just be aware that Smart Pointers can sometimes be Smart Alec pointers. Be carefull. ;)


Also why is your 1st block not like this?if (fft of frame n exist in fftcache)
complex * FFT0 = getFFT_FromCache(n)
else
{
PVideoframe src = getframe(n)
FFT0 = makefft(src);
put FFT0 to fftcache
}And you ending block not like thisfftout=DoSuperFiltration(fft0,fff1,fft2)
PVideoFrame dst=env->NewVideoFrame(vi);
RevertFFT(fftout, dst)
return dstDoes RevertFFT() make some really good use of in-place frame manipulation?
If this is true then your code is correct, please ignore this ramble.

IanB

[Fizick]

IanB, Thanks!
Now I understand the main from your short answer (two bitblt).
This description really only scratches the surface and omits much complexity for easier explanation.
I will often return to this easy reading. :)
(And probably not me only)