Welcome to Doom9's Forum, THE in-place to be for everyone interested in DVD conversion. Before you start posting please read the forum rules. By posting to this forum you agree to abide by the rules. |
16th June 2008, 18:36 | #1 | Link |
Avisynth language lover
Join Date: Dec 2007
Location: Spain
Posts: 3,433
|
(Bug) Run-time functions can produce wrong results
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:
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: Code:
# 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 """) Code:
ScriptClip(""" f1 = current_frame a1 = AverageLuma() f2 = current_frame # now wrong, = f1+50 Subtitle("f1="+string(f1)+" f2="+string(f2)) """)
(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.) |
17th June 2008, 00:12 | #2 | Link |
Registered User
Join Date: Sep 2005
Location: 100011110010001000001 10000011111111000001
Posts: 221
|
@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): Code:
# 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 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.
__________________
AVSLib, a free extension library for Avisynth. Current version: 1.1.0 (beta), 14/05/2007. [ Home page | Download page ] |
17th June 2008, 08:20 | #3 | Link | |||
Avisynth language lover
Join Date: Dec 2007
Location: Spain
Posts: 3,433
|
Quote:
Quote:
Quote:
|
|||
17th June 2008, 16:57 | #4 | Link |
Registered User
Join Date: Sep 2005
Location: 100011110010001000001 10000011111111000001
Posts: 221
|
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:
Code:
# 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") 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: Code:
# 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") Code:
ScriptParser parser(env, script.AsString(), "[ScriptClip]"); PExpression exp = parser.Parse(); result = exp->Evaluate(env);
__________________
AVSLib, a free extension library for Avisynth. Current version: 1.1.0 (beta), 14/05/2007. [ Home page | Download page ] |
17th June 2008, 18:08 | #5 | Link | ||
Avisynth language lover
Join Date: Dec 2007
Location: Spain
Posts: 3,433
|
Quote:
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. Quote:
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). |
||
18th June 2008, 00:36 | #6 | Link |
Registered User
Join Date: Sep 2005
Location: 100011110010001000001 10000011111111000001
Posts: 221
|
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: Code:
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) """) 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.
__________________
AVSLib, a free extension library for Avisynth. Current version: 1.1.0 (beta), 14/05/2007. [ Home page | Download page ] |
18th June 2008, 01:16 | #7 | Link |
Avisynth language lover
Join Date: Dec 2007
Location: Spain
Posts: 3,433
|
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: Code:
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; Seems to work OK. Next step is to try making current_frame global... |
18th June 2008, 16:38 | #8 | Link |
Avisynth language lover
Join Date: Dec 2007
Location: Spain
Posts: 3,433
|
@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):
All feedback, both positive and negative, will be welcomed. |
20th June 2008, 07:19 | #9 | Link |
Avisynth Developer
Join Date: Jan 2003
Location: Melbourne, Australia
Posts: 3,167
|
Try this version AviSynth_080620.exe
|
20th June 2008, 10:57 | #10 | Link |
Avisynth language lover
Join Date: Dec 2007
Location: Spain
Posts: 3,433
|
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? |
20th June 2008, 16:29 | #12 | Link | |
Avisynth language lover
Join Date: Dec 2007
Location: Spain
Posts: 3,433
|
Quote:
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. |
|
20th June 2008, 23:43 | #13 | Link |
Registered User
Join Date: Sep 2005
Location: 100011110010001000001 10000011111111000001
Posts: 221
|
@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:
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 .
__________________
AVSLib, a free extension library for Avisynth. Current version: 1.1.0 (beta), 14/05/2007. [ Home page | Download page ] |
21st June 2008, 02:16 | #14 | Link |
Avisynth language lover
Join Date: Dec 2007
Location: Spain
Posts: 3,433
|
@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???) |
21st June 2008, 03:27 | #15 | Link |
Avisynth Developer
Join Date: Jan 2003
Location: Melbourne, Australia
Posts: 3,167
|
@Gavino,
The programmatically better way to express your example :- Code:
Function F(Clip, Current_Frame) { return Clip.Subtitle(String(Current_Frame)) } ScriptClip("f(current_frame)") If you really have you heart set on a global var solution, do this:- Code:
Function F(Clip) { return Clip.Subtitle(String(Current_Frame)) } ScriptClip("Global Current_Frame=current_frame f()") 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. |
21st June 2008, 10:15 | #16 | Link | |
Avisynth language lover
Join Date: Dec 2007
Location: Spain
Posts: 3,433
|
Quote:
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...) |
|
21st June 2008, 11:41 | #17 | Link | |
Moderator
Join Date: Nov 2001
Location: Netherlands
Posts: 6,370
|
Quote:
|
|
21st June 2008, 20:45 | #18 | Link |
Registered User
Join Date: Sep 2005
Location: 100011110010001000001 10000011111111000001
Posts: 221
|
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 .
__________________
AVSLib, a free extension library for Avisynth. Current version: 1.1.0 (beta), 14/05/2007. [ Home page | Download page ] |
21st June 2008, 22:16 | #19 | Link | |
Avisynth language lover
Join Date: Dec 2007
Location: Spain
Posts: 3,433
|
Quote:
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. |
|
22nd June 2008, 00:35 | #20 | Link | |
Avisynth language lover
Join Date: Dec 2007
Location: Spain
Posts: 3,433
|
Quote:
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. |
|
Thread Tools | Search this Thread |
Display Modes | |
|
|