ChangeFPS doesn't modify number of frames

[Xesdeeni]

Using the following script to convert from NTSC to PAL (from xxxFPS dox), the duration(!) of the clip is changed, because the total number of frames is NOT changed:
AVISource("NTSCTest.avi")
Bob()
ChangeFPS(50)
BicubicResize(720, 576)
SeparateFields()
SelectEvery(4, 0, 3)
Weave()
But replacing the ChangeFPS() with ConvertFPS() adjusts the duration correctly.

I don't know anything about SourceForge and CVS, but I have made the change to the source itself (by borrowing a line from the constructor of ConvertFPS()). If someone can help me, I can check in the changes...

But the big question is:
Should I make the modification of the number of frames as a blanket change, or should I make the modification optional (with an optional "modlen" parameter) to maintain backwards compatibility?

Xesdeeni

[WarpEnterprises]

To me the script looks wrong:

Bob should come after SeparateFields, and what is the SelectEvery pulldown supposed to do if you want to convert TRUE NTSC?

[Xesdeeni]

I was just giving a full example to show why one might use ChangeFPS. Try this:
AVISource("NTSCTest.avi")
ChangeFPS(15)
The resulting video should be the same length as the original, but it will be twice as long.

Xesdeeni

[WarpEnterprises]

Now I see what you mean.
The number of frames of the whole clip is held constant, but individual frames are deleted or duplicated.
So if you make a conversion from 25 to 30 the end of the clip is lost, when you do 30->25 the end consists of freezed frames.

==> don't use this function would be my advice.

[sh0dan]

Originally posted by Xesdeeni

The resulting video should be the same length as the original, but it will be twice as long.

Is it just me, or isn't this a selfcontradiction?

[Xesdeeni]

WarpEnterprises:==> don't use this function would be my advice.Gee, thanks....

sh0danquote:Originally posted by Xesdeeni
"The resulting video should be the same length as the original, but it will be twice as long."
Is it just me, or isn't this a selfcontradiction?This is a tough crowd!
Note "should be" vs. "will be."

It only takes one line to fix this:
FPS.CPP (starting on line 88):
ChangeFPS::ChangeFPS(PClip _child, int new_numerator, int new_denominator)
: GenericVideoFilter(_child)
{
a = __int64(vi.fps_numerator) * new_denominator;
b = __int64(vi.fps_denominator) * new_numerator;
vi.SetFPS(new_numerator, new_denominator);
vi.num_frames = int((vi.num_frames * b + (a >> 1)) / a); // <-- New line
}All I wanted to know whether you guys thought we should make the change above, or maintain backwards compatibility by adding an optional parameter to ChangeFPS ("modlen = true/false").

And then I was hoping to get a little help on how to check out and check in the code at SourceForge.

Geesh..."I didn't expect the Spanish Inquisition!"

Xesdeeni

[dividee]

I agree, Xesdeeni, I'd call this behaviour buggy, or at least incoherent. I don't think backward compatibility is a big deal here. Moreover, most of the time you need to work around it, and any workaround I can think of would still give the same output if the bug is fixed.

Thanks for the proposed fix, I'll test and check it in, if appropriate.

[WarpEnterprises]

In which case would you like to use ChangeFPS anyway?
To me it seems the only possible use of it would be to double the frames and the framerate, but what would the sense of that be?
For fractional changes the behaviour is too stupid.

Or do you have other uses in your mind?

And for changes in the CVS you either post a feature request at sourceforce or get a developer (richard berg, dividee, sh0dan, poptones to mention the most active) make the change for you.

[edit]: when speaking of dividee...

[Xesdeeni]

In which case would you like to use ChangeFPS anyway?

To me it seems the only possible use of it would be to double the frames and the framerate, but what would the sense of that be?

For fractional changes the behaviour is too stupid.

Or do you have other uses in your mind?I'm using it to do PAL to NTSC conversion. There are several reasons I'm using ChangeFPS() instead of ConvertFPS() at this point:

1. ConvertFPS() didn't exist when I first started doing my conversion work (or at least I didn't know about AVISynth 2.x, which amounts to the same thing).
2. The conversion takes long enough as it is, so right now I don't want to add what is probably more overhead (admittedly added overhead is supposition, but with the conversions + encoding originally taking 13-14 hours per 30 minutes of video, I haven't wanted to experiment with it yet to determine if there is indeed any additional overhead).
3. Much PAL to NTSC converted video suffers from something called "jutter" caused by the 4-field conversion used in the original (and now less expensive) conversion hardware. ConvertFPS() will exhibit the same type of artifact due to the interpolated frames...and I really hate this artifact.
4. Even though I have one of the most critical eyes around (I can provide references :-) ) I cannot see the stutter artifact that should by rights be there, even on pans.
The quality of these converted videos is comparable to professional conversions. And for changes in the CVS you either post a feature request at sourceforce or get a developer (richard berg, dividee, sh0dan, poptones to mention the most active) make the change for you.Really? Hmm. I guess I thought that an open source project on SourceForge was available for just about anyone to modify. But I guess I understand the reasoning.

I guess if I see any other issues that I can just post the changes here and let them take a look.

Xesdeeni

P.S. So far I've cut the conversion + encoding down to 6-7 hours per 30 minutes of video. I'm hoping that removing the workaround for the video length, plus some other issues I'm working on separately together can further reduce this time.