Bug in Interleave frame count

[Gavino]

Interleave gets its frame count wrong when given clips of unequal lengths, unless the longest clip is given last.

The code in its constructor (frame.cpp) is

vi.num_frames = (vi.num_frames - 1) * num_children + 1;
for (int i=1; i<num_children; ++i)
{
const VideoInfo& vi2 = child_array[i]->GetVideoInfo();
...
vi.num_frames = max(vi.num_frames, (vi2.num_frames - 1) * num_children + i + 1);
}

This is quite clearly brain-damaged. The number of frames cannot possibly depend on the loop index i. :(
That it actually works at all in most cases is almost a coincidence.

Clearly, it should be

vi.num_frames = vi.num_frames * num_children;
for (int i=1; i<num_children; ++i)
{
const VideoInfo& vi2 = child_array[i]->GetVideoInfo();
...
vi.num_frames = max(vi.num_frames, vi2.num_frames * num_children);
}

An example of what happens:

a=BlankClip(8)
b=BlankClip(9)
c=BlankClip(10)
Interleave(a,b,c) # framecount=30, OK
Interleave(b,c,a) # framecount=29, WRONG
Interleave(c,a,b) # framecount=28, WRONG

Incidentally, there is a broken link in the documentation: corefilters/interleave.htm (and probably
others) refers to ../syntax.htm#multiclip which no longer exists. (Why? Where has this very useful table gone?)

[Manao]

Actually, it would be better to use min(vi.num_frames, vi2.num_frames * num_children). GetFrame request to the child filter n / num_children, and with the max, you may end up requesting a frame that doesn't exist. If the child plugin isn't properly coded, it may crash.

[Gavino]

Actually, it would be better to use min(vi.num_frames, vi2.num_frames * num_children). GetFrame request to the child filter n / num_children, and with the max, you may end up requesting a frame that doesn't exist. If the child plugin isn't properly coded, it may crash.
Good point. However, that would imply changing the specification so that it takes its length from the shortest clip rather than the longest. Do we want that change in behaviour?

OTOH, to keep the existing specification, Interleave::GetFrame needs to check for running off the end of a child clip. (This is nothing to do with the first bug, it's actually another one. :() Currently, it just does

inline PVideoFrame __stdcall GetFrame(int n, IScriptEnvironment* env)
{ return child_array[n % num_children]->GetFrame(n / num_children, env); }

BTW, Is it fair to describe a plugin as 'not properly coded' if it doesn't bound check its GetFrame parameter? I would have thought it is the caller's responsibility to get it right.

[Manao]

OTOH, to keep the existing specification, Interleave::GetFrame needs to check for running off the end of a child clipI know, and I've already had crashes because of that.

Is it fair to describe a plugin as 'not properly coded' if it doesn't bound check its GetFrame parameterThat's what I meant. Now, whether it's the caller's or callee's responsibility, I'd say both. At least, if the callee is overzealous in its checkings, the caller can't do anything wrong. And what about a callee that just pass through the framenum to its child (thus becoming a caller?)

In the end, only the source filter isn't a caller. But were I to design such a filter, I wouldn't trust the upstream filters. Better safe than sorry.

[Gavino]

Now, whether it's the caller's or callee's responsibility, I'd say both. At least, if the callee is overzealous in its checkings, the caller can't do anything wrong. And what about a callee that just pass through the framenum to its child (thus becoming a caller?)

The caller definitely has the primary responsibility, I would say. It knows (or can find out) the valid frame range for any PClip it has access to, before requesting a frame from it. The PClip's contract with the outside world is just to honour valid frame requests.

But yes, better safe than sorry, so it's good practice for a filter to protect itself against unruly callers. The standard approach seems to be to clamp the frame number within the valid range - perhaps PClip should provide a standard function that does just that.

[IanB]

The ../syntax.htm was temporarily lacking contents between 20th April 2008, rev 1.42 and 24th June 2008, rev 1.44.

The Interleave filter works exactly the way Ben Rudiak-Gould originally intended. There has been only 1 code change, to deal with extra colour space comparison. Your example is exactly the case that is specially handled, the last stripe stops early when all of the clips have been drained. The documentation could be somewhat improved.

The Cache filter enforces frame range validityn = min(vi.num_frames-1, max(0,n));It is acceptable for filters to make shadow GetFrame or GetAudio calls before the start and after the end of a PClip to simplify boundary processing. The expected behaviour is copies of frame 0 if n<0 and copies of vi.num_frames-1 if n>=vi.num_frames.

[Wilbert]

Incidentally, there is a broken link in the documentation: corefilters/interleave.htm (and probably
others) refers to ../syntax.htm#multiclip which no longer exists. (Why? Where has this very useful table gone?)

Thanks for mentioning this. I wanted to move this to corefilters.htm, just like i did with the online documentation:

http://avisynth.org/mediawiki/Internal_filters

but i forgot to do that.

The ../syntax.htm was temporarily lacking contents between 20th April 2008, rev 1.42 and 24th June 2008, rev 1.44.
Fizick restored that for reference and history. But all contents of syntax.htm is (or should be) also described elsewhere. In my opinion it should be removed.

The Interleave filter works exactly the way Ben Rudiak-Gould originally intended. There has been only 1 code change, to deal with extra colour space comparison. Your example is exactly the case that is specially handled, the last stripe stops early when all of the clips have been drained. The documentation could be somewhat improved.
Will it break anything if we change it into the more logical behaviour that Gavino suggests (the way it is currently described in the docs :)). I mean i understand it when reading the code snippet that Gavino posted, but if fail to see any logic in the current behaviour:

max ( nrfr_1, (nrfr_2 - 1) * nrclips + 2, ..., (nrfr_nrclips - 1) * nrclips + nrclips )

with nrfr_i the number of frames of clip i, and nrclips the number of clips.

[Gavino]

The Interleave filter works exactly the way Ben Rudiak-Gould originally intended. ... Your example is exactly the case that is specially handled, the last stripe stops early when all of the clips have been drained.
Yikes. I certainly didn't mean to imply that Ben R-G (or anyone else) was in any way brain-damaged. :o

After thinking about it, I can see some sort of rationale behind this scheme. Basically, it means you stop on the last frame of the longest clip. Though I still think the scheme implied by the docs is easier to understand, where you always get a whole number of complete stripes.

The Cache filter enforces frame range validity
That's a relief. But how come Manao has had crashes then?