(Bug) Run-time functions can produce wrong results

[Gavino]

I have discovered a subtle and potentially nasty bug in the operation of the run-time (aka conditional) environment. Under certain circumstances, a run-time function can produce the wrong result because it is looking at the wrong frame of its input clip.

Specifically, if:

1. The input clip to a run-time filter is derived from an earlier run-time filter, and
2. There is an intervening filter in the chain that changes frame numbers (eg Trim), and
3. The (later) run-time script makes more than one run-time function call

then all its calls to run-time functions after the first one will operate on the wrong frame.

As you can see, a combination of factors need to be present. However, the bug is potentially nasty because when it bites, you get no error or any indication that something has gone wrong; the script runs apparently OK but produces erroneous results. And it can appear or disappear depending on statements in a completely different part of the script.

Here's an example reduced to its simplest form:

# 100 frames black + 100 white, to give predictable luma
BlankClip(100, pixel_type="YV12")+BlankClip(100, pixel_type="YV12", color=color_white)
FrameEvaluate("#anything you like, even nothing")
  # ... <- arbitrary filters can appear here...
Trim(50, 149)
  # ... <- ... and again here
ScriptClip("""
  a1 = AverageLuma() # OK
  a2 = AverageLuma() # gives wrong value
  Subtitle("a1="+string(a1)+" a2="+string(a2)) # shows a1 != a2
""")

Analysis: The source of the problem is that the evaluation of the first run-time function causes the variable current_frame to be overwritten by its value from the earlier script. You can see this in action if you change the example to do

ScriptClip("""
  f1 = current_frame
  a1 = AverageLuma()
  f2 = current_frame # now wrong, = f1+50
  Subtitle("f1="+string(f1)+" f2="+string(f2))
""")

What happens is:

1. When the ScriptClip's GetFrame is called for frame n, it sets current_frame to n, then evaluates its associated script.
2. This causes the first run-time function call to be evaluated, which in turn requires frame n of its input to be fetched. This passes up the filter chain in the usual way, but because of the Trim, will request a different frame number (in this case n+50) from the earlier run-time filter (here FrameEvaluate).
3. In a similar way to step 1, FrameEvaluate's GetFrame sets current_frame to n+50, and carries on with its business. Ultimately the correct frame is fetched from the source clip and we unwind back to step 2 whereupon the run-time function correctly calculates its result.
4. However, current_frame now has the value n+50, so any subsequent run-time function calls in the run-time script will be evaluated using the wrong frame number.

Solution: The correct solution is for the GetFrame of each run-time filter to save any existing value of current_frame before assigning to it, and then restore the initial value when it exits (just like it does with last, and for the same reason). That way the correct value is preserved when returning to an enclosing GetFrame as in the example given.

(If some aspects of this seem strangely familiar, you may have seen the recent thread about problems using run-time functions inside user-defined functions. Thinking further about that problem led me to deduce the existence of this bug, which I was then able to reproduce.)

[gzarkadas]

@Gavino,

I see that you had time to test it seriously, congratulations .

I have verified the existence of this behavior (running avisynth 2.57) with the following script (a slight modification of your template):

# 100 frames black + 100 white, to give predictable luma
BlankClip(100, pixel_type="YV12")+BlankClip(100, pixel_type="YV12", color=color_white)
ScriptClip("SubTitle(String(current_frame), y=10)")
Trim(50, 149)
ScriptClip("""
  f1 = current_frame
  a1 = AverageLuma() # OK
  f2 = current_frame
  a2 = AverageLuma() # gives wrong value
  f3 = current_frame
  SubTitle("a1="+string(a1)+" a2="+string(a2), y=30) # shows a1 != a2
  SubTitle(String(f1)+","+String(f2)+","+String(f3), y=50)
""")

The problem occures because Trim is placed between the two runtime scripts and because runtime functions compute their values dynamically requesting frames at runtime.

The screenshots for frames 0, 49, 50 of final clip are attached.

The play of frames shows up at the result as following:
1. ScriptClip #2 requests a frame (0, 49, 50)
2. f1 is set to current_frame (0, 49, 50)
3. Calculation of a1 requests a frame from Trim (0, 49, 50)
4. Trim requests a frame from ScriptClip #1 (50, 99, 100)
5. ScriptClip #1 delivers the frame and prints its number on it (50, 99, 100)
6. f2 is now the value of last assignment to current frame (50, 99, 50). Last value is 50 because frame #100 has already been requested (at the linear, forward scan that I have applied to test) and the cache returns the frame without calling ScriptClip #1.
7. Calculation of a2 requests a frame from Trim (50, 99, 50, last is 50 because current_frame has not changed)
8. Trim requests a frame from ScriptClip #1 (100, 149, 100)
9. ScriptClip #1 delivers the frame and prints its number on it (100, 149, 100) but because its output at the 2nd AverageLuma call is not used as the script's output it does not show at the result
10. f3 is now the value of last assignment to current frame (100, 149, 50). Last value is 50 because frame #100 has already been requested (at the linear, forward scan that I have applied to test) and the cache returns the frame without calling ScriptClip #1.

This BTW confirms my speculation in the other thread you mention. Whether local, or global a scheme that depends on setting a single variable will show-up this behavior in such situations. I don't know if a remedy to this can be applied without incurring serious performance issues; your proposed solution intervenes with the normal way of operation of runtime filters, which is to set the current_frame. But for the time being, as we wait for the Avisynth devs to decide on this issue, one can use the following strategy to get the intended results in such situations:

1. Store the value of current_frame in a local variable of the runtime script before calling the runtime function.
2. Call the runtime function
3. Restore the value of current_frame by explicitly assigning to it the value stored at the local variable.

[Gavino]

Originally Posted by gzarkadas

I see that you had time to test it seriously, congratulations .

It was rather that I had time to think about it clearly enough to realise the problem existed. The test case was then the easy bit.

This BTW confirms my speculation in the other thread you mention. Whether local, or global a scheme that depends on setting a single variable will show-up this behavior in such situations.

I think the scheme itself is sound, but currently it has a flaw in its implementation.

I don't know if a remedy to this can be applied without incurring serious performance issues; your proposed solution intervenes with the normal way of operation of runtime filters, which is to set the current_frame.

I would expect the performance overhead to be minimal. My fix simply ensures that the effect of assignment to current_frame is localised to the processing of the frame in question. The code already does the same thing with the variable last.

[gzarkadas]

I made some modifications to my script to simulate your proposed solution of setting current_frame at the end of runtime filters, by backing up its value in a chained FrameEvaluate and restoring at the end of each script block:

# 100 frames black + 100 white, to give predictable luma
BlankClip(100, pixel_type="YV12")+BlankClip(100, pixel_type="YV12", color=color_white)
ScriptClip("""
  SubTitle(String(current_frame), y=10)
  current_frame = backup2
  SubTitle(String(current_frame), y=10, x=320)
""")
FrameEvaluate("backup2 = current_frame")
Trim(50, 149)
ScriptClip("""
  f1 = current_frame
  a1 = AverageLuma() # OK
  f2 = current_frame
  a2 = AverageLuma() # gives wrong value
  f3 = current_frame
  SubTitle("a1="+string(a1)+" a2="+string(a2), y=30) # shows a1 != a2
  SubTitle(String(f1)+","+String(f2)+","+String(f3), y=50)
  current_frame = backup1
  SubTitle(String(f1), y=70)
  SubTitle(String(current_frame), y=70, x=320)
""")
FrameEvaluate("backup1 = current_frame")

However, this does not work; the output is exactly (disregarding the added text) the same as the non-corrected script's.

In order to correct the output, the backup/restore of current_frame has to be done inline the second script, as I suggested at my previous post:

# 100 frames black + 100 white, to give predictable luma
BlankClip(100, pixel_type="YV12")+BlankClip(100, pixel_type="YV12", color=color_white)
ScriptClip("""
  SubTitle(String(current_frame), y=10)
  current_frame = backup2
  SubTitle(String(current_frame), y=10, x=320)
""")
FrameEvaluate("backup2 = current_frame")
Trim(50, 149)
ScriptClip("""
  f1 = current_frame
  a1 = AverageLuma() # OK
  current_frame = f1
  a2 = AverageLuma() # OK
  current_frame = f1
  SubTitle("a1="+string(a1)+" a2="+string(a2), y=30) # OK
  current_frame = backup1
  SubTitle(String(f1), y=50)
  SubTitle(String(current_frame), y=50, x=320)
""")
FrameEvaluate("backup1 = current_frame")

The reason is that the entire script's evaluation inside runtime filters is just a line of code (here from ScriptClip's GetFrame):

    ScriptParser parser(env, script.AsString(), "[ScriptClip]");
    PExpression exp = parser.Parse();
    result = exp->Evaluate(env);

thus the runtime filter does not have the chance to properly restore the value of current_frame (which happens inside the Evaluate call), unless a hook is placed either at the parser or at the Evaluate method to analyse the chain and recognise that a situation such the one that we talk about occures and apply logic to overcome it. This solution is costly in terms of performance; the price would have to be paid at every frame.

[Gavino]

Originally Posted by gzarkadas

I made some modifications to my script to simulate your proposed solution of setting current_frame at the end of runtime filters, by backing up its value in a chained FrameEvaluate and restoring at the end of each script block ... However, this does not work; the output is exactly (disregarding the added text) the same as the non-corrected script's.

You cannot fix the problem with a chained FrameEvaluate because FrameEvaluate itself has already set current_frame to the wrong value before you can get your hands on it to save it in backup2. Catch 22!

The inline re-assignment after calling AverageLuma works of course, but it is just a work-around. It could be implemented 'for real' by putting my save/restore code around the GetFrame call in every run-time function, but this is putting the fix in the wrong place.

... the runtime filter does not have the chance to properly restore the value of current_frame (which happens inside the Evaluate call), unless a hook is placed either at the parser or at the Evaluate method to analyse the chain and recognise that a situation such the one that we talk about occures and apply logic to overcome it. This solution is costly in terms of performance; the price would have to be paid at every frame.

What you are missing here is that the overwriting of current_frame during the evaluation only occurs inside the other run-time filter's GetFrame call, invoked as part of that evaluation. It is there that the fix restores the original value (still inside the evaluation), which is then seen by the remainder of the evaluation.

Effectively, each run-time filter's GetFrame should leave current_frame as they found it, by far the cleanest solution.

This is still a 'per-frame' overhead, but only on run-time filters, and very minor in comparison to the work they already have to do for every frame (parsing and evaluating the run-time script and fetching a frame from it).

[gzarkadas]

Well, since we both tend to defend our opinions with long explanations , I though it would be nice to save some time with a real experiment; I thus present NewScriptClip, a concept-check filter to see who is finally right on this issue (attached).

I have also made a test with a modified (again) version of our reccuring test pattern, see below:

LoadPlugin("newscriptclip.dll")

# 100 frames black + 100 white, to give predictable luma
BlankClip(100, pixel_type="YV12")+BlankClip(100, pixel_type="YV12", color=color_white)
NewScriptClip("""
  SubTitle(String(current_frame), y=10)
""")
Trim(50, 149)
NewScriptClip("""
  f1 = current_frame
  a1 = AverageLuma() # OK
  f2 = current_frame
  a2 = AverageLuma() # gives wrong value
  f3 = current_frame
  SubTitle("a1="+string(a1)+" a2="+string(a2), y=30) # shows a1 != a2
  SubTitle(String(f1)+","+String(f2)+","+String(f3), y=50)
""")

to verify that you are right and I was wrong (but I don;t care for this; its nice to be wrong when stating that something is costly to do ), at least about the feasibility of correcting this issue.

It remains to verify - because this is a very fresh alpha and in this part of the world is late at night and I won't do any testing till half a day at least - that the modded ScriptClip behaves correctly in normal situations (don't set show=true though; the implementation of this is a stub). A llitle help from the rest of the Avisynth community would be highly appreciated .

PS: If the modded implementation is proved to behave correctly in all respects, then IMO making current_frame a global would not be a problem to do.

[Gavino]

Thanks, gzarkadas, I'm glad to see you are at least provisionally persuaded.

I can't try out your version yet (attachment still pending approval), but while you were doing that I also wrote a small plugin to verify my solution. Basically it is just a wrapper that calls the real ScriptClip's GetFrame (so it implements the latter's full functionality).

The GetFrame code is just:

AVSValue prevFrame;

try {
  prevFrame = env->GetVar("current_frame");
}
catch (IScriptEnvironment::NotFound) {}

PVideoFrame result = inner->GetFrame(n, env);

if (prevFrame.Defined())
  env->SetVar("current_frame", prevFrame);

return result;

where inner is a PClip representing the real ScriptClip and saved in my filter's constructor.

Seems to work OK.
Next step is to try making current_frame global...

[Gavino]

@gzarkadas, I've now been able to try out your concept-check filter and it seems to work fine.

At the same time, I've also extended my own experimental plug-in to cover all the relevant run-time filters, while fixing both this bug and the issue of making current_frame a global (thus allowing run-time functions to be called from a user function).

For want of a better name, I've called it GRunT, Gavino's Run_Time (no, it's not because this run-time stuff is a pig to use ).

It provides the following filters which are straight replacements for the original versions (indeed, they are currently implemented as wrappers around the originals):

• GScriptClip - alternative version of ScriptClip, while similarly we have
• GFrameEvaluate
• GConditionalFilter
• GWriteFile
• GWriteFileIf

I have some ideas for adding other features to make the run-time environment easier to use, but I'll leave that for another day.

All feedback, both positive and negative, will be welcomed.

[IanB]

Try this version AviSynth_080620.exe

[Gavino]

IanB, thanks for getting this out so quickly. I haven't yet used it extensively, but initial tests show it fixes the bug.

Any chance you could also make current_frame global, or do we need more debate on that?

[IanB]

@Gavino,

Why do you think you want "current_frame" as a global var. It is only supposed to have scope in the code and routines subordinate to a conditional invocation.

[Gavino]

Originally Posted by IanB

Why do you think you want "current_frame" as a global var. It is only supposed to have scope in the code and routines subordinate to a conditional invocation.

The short answer is, to allow run-time functions to be used inside user functions called from a run-time script (see here).

Currently, this doesn't work - I don't know whether this was an intentional restriction, or something accepted as an unfortunate side-effect of the implementation, or simply an oversight.

While useful in its own right (eg you could write a function to compute a weighted second order luma interpolation value), the ability to call run-time functions in a user function gives you more than just that. For the first time, it would allow the entire run-time script to be put inside a function body, eg called as ScriptClip("f()"). The script's statements would then be evaluated in a separate scope, eliminating the problems that can arise from unintended sharing of variable names in different run-time scripts.

Making current_frame global makes all this possible. And since it would not be defined until the first run-time script is evaluated, it would still be available only inside run-time scripts.

[gzarkadas]

@Gavino,

The problem I see in making current_frame global is a performance one. This would mean that env->GetVar would have to make in each frame request a full scan of the local vartable table (since by design it would fail) and then a scan of the typically larger global vartable to get the value.

This is a small overhead but is added up at each frame. And the fact is that most Avisynth users do not make such advanced use of runtime scripts; thus any performance penalty, albeit small should be optional and not the norm.

In addition, for all the possible uses of a global current_frame there are alternative solutions. For example:

• Instead of calling runtime functions inside a function, you can pass their result as argument.
• Instead of calling ScriptClip("f()") you can pass in a multiline string. In fact, I find this to be most of the time better style because the documentation of what f() does is at the point of use.
• If the problem is to make functions utilising runtime scripts that can be callable as normal functions, you can use the simple technique of putting a suffix at your key-script-variables (with text substitution). See my FrameFilter and FrameFilterReader filters for a working example (code quick-link here).

Thus, to sum up, the change is possible, but offers optional functionality. In exchange it requires an additional possibly small (but I don't know exactly how much; IanB is in better position to answer this ) runtime cost to be beared by all Avisynth runtime filters' users. It has to be weighted whether it should be implemented.

After all, for all those that do need this functionality, there is now your plugin available .

[Gavino]

@gzarkadas, thanks for your input.

Your points are all valid and need to be taken into consideration. However, I feel that the gain in usability outweighs any small performance penalty (and in my view, the overhead here is likely to be pretty small).

Sure, you can work around the problem in the ways we have identified (including my plugin ). But, if current_frame had been global from the start, giving us the features I described, do you really think we'd be sitting here planning to take it out because it was slowing things down too much?

That said, I can understand people are reluctant to change something that has worked 'well enough', especially at the request of someone relatively new to Avisynth like myself. Maybe the idea needs more time before it catches on.

What do others think? (Or are gzarkadas and me the only ones interested in this issue???)

[IanB]

@Gavino,

The programmatically better way to express your example :-

Function F(Clip, Current_Frame) {
  return Clip.Subtitle(String(Current_Frame))
}

ScriptClip("f(current_frame)")

Global variables generally are horrid and if you find yourself using them, then there probably is a better way to express the idea your are implementing. And I did say generally, very occasionally using a global variable can be the easier solution.

If you really have you heart set on a global var solution, do this:-

Function F(Clip) {
  return Clip.Subtitle(String(Current_Frame))
}

ScriptClip("Global Current_Frame=current_frame
f()")

@gzarkadas,

Performance is not that much of an issue here, ScriptClip, et el, all recompile the script string every frame so a few extra GetVar calls won't make a difference. If you are using ScriptClip then performance is already shot to hell, using it will generally be the result of a wrong thought process i.e. Linear (C) instead of Functional (AVS) thinking.

[Gavino]

Originally Posted by IanB

Global variables generally are horrid and if you find yourself using them, then there probably is a better way to express the idea your are implementing.

Oh yes, absolutely.

But from the way it is already used by AverageLuma, etc, current_frame is effectively a global state variable of the run-time environment. My idea is (was?) just to extend its visibility to user functions too.

However, I really do like your elegant Function F(Clip, Current_Frame) alternative, which is an accurate encapsulation of what a run-time script really is (or should be). So I can live with this.

(Even here though, it seems a bit of a hack that AverageLuma and friends will now use the local Current_Frame instead of the 'real' one, but oh well...)

[Wilbert]

@gzarkadas,

Performance is not that much of an issue here, ScriptClip, et el, all recompile the script string every frame so a few extra GetVar calls won't make a difference. If you are using ScriptClip then performance is already shot to hell, using it will generally be the result of a wrong thought process i.e. Linear (C) instead of Functional (AVS) thinking.

Why not make current_frame global if there is no downside to it? It would be could if one could use user functions inside runtime functions.

[gzarkadas]

If performance is not an issue, as IanB correctly states (just finished testing it) then moving current_frame to global var space is something that can be afforded. The only thing that will break is scripts that explicitly assign current_frame, but these are very rare and easy to fix.

The way that Avisynth currently implements its vartables does not provide any advantage on having current_frame in a local vartable, because it always lives at the script-level local vartable, which never dies, thus effectively having all disadvantages that a global variable has, plus the disadvantage that it is not a trully global variable.

In fact, now that I am thinking of it I wonder whether it would worth the effort to make the global vartable being just an alias (for not breaking existing code) of the script-level local vartable and get rid of the global keyword in exchange for a new can of worms .

[Gavino]

Originally Posted by gzarkadas

The only thing that will break is scripts that explicitly assign current_frame, but these are very rare and easy to fix.

I have to confess I thought about this, but was afraid to mention it.

In fact though, I think even scripts that assign to current_frame will still work because the new local value will be used by run-time functions in preference to the global one. What won't work is if you then call a user function that needs current_frame, but no existing script can do that yet, so I think we're safe.

[Gavino]

Originally Posted by IanB

Try this version AviSynth_080620.exe

Returning for a moment to the bug that started off this thread, I should also have said that the variable current_sample (rarely used, I believe) suffers from the same problem and needs to be reset in the same way as current_frame on exit from GetFrame in ScriptClip/FrameEvaluate. (gzarkadas got this right in his plugin.)

However, trying out a simple test of the 080620 build, it seems that current_sample is no longer visible at all (even ScriptClip("Subtitle(string(current_sample))") fails), although it was visible and showed the buggy behaviour in the 080527 build.