[patch] correctfps

[tgoyne]

From 191919 on July 16, 2009 01:39:09

I have made a patch which adds correctfps to FFVideoSource. The idea is
same as the one of DirectShowSource's.

Attachment: correctfps.patch

Original issue: http://code.google.com/p/ffmpegsource/issues/detail?id=1

[tgoyne]

From 191919 on July 17, 2009 01:53:02

Attachment: ffmpeg_source_correctfps.patch

[tgoyne]

From kalle.blomster on July 18, 2009 16:20:32

Extremely rejected, for the following reasons:

• you probably meant "convertfps" not "correctfps", and if you did, you're
  reimplementing functionality that is already there (hint: set fpsnum to something > 0)
• you unconditionally added of a bunch of random encoding-only libraries to libs.cpp,
  forcing everyone to compile their ffmpeg's with that if they want to compile at all
• randomly changing the default fps for no particular reason
• use of random windows-isms (i.e. Int32x32To64, __int64) in files that are going to
  be compiled on other platforms
• attempt to add a feature to only one of the three possible source readers

If you want this feature in the API too and not just in the AVS plugin, you are free
to try to submit a patch that does that, but be aware that it's rather trivial to do
the conversion yourself if you're using the API.

Status: WontFix

[tgoyne]

From kalle.blomster on July 18, 2009 17:02:53

(hint: FFMS_GetFrameByTime() exists for a reason)

[tgoyne]

From 191919 on July 19, 2009 18:23:05

(1) No, it's to "correct", not to "convert". If the file is VBR or it fps number
doesn't make sense, "fpsnum>0" doesn't work. Try a RMVB file, and you know how it
works.

(2) Those libs enabled me to pass the compilation, so if the idea is accepted, the
lib list should be rearranged.

(3) int64_t is ok?

(4) It's my attempt for RMVB files, Haali and other source doesn't need it (it is a
MP4 splitter).

(5) FFMS_GetFrameByTime is O(n), that is why I use CorrectIndex as a cache, it is a
std::vector (O(1)).

[tgoyne]

From kalle.blomster on July 19, 2009 21:57:45

1. You most likely meant VFR, not VBR. I looked closer at your patch and now I think
   I understand what you were actually trying to do. Unfortunately it makes even less
   sense than reimplementing VFR to CFR conversion, which was what I thought you were
   doing at first (your confusing comment regarding directshowsource didn't really help).

In any case, the reported framerate is always completely bogus; the manual states
this quite clearly. For some CFR files under certain conditions it may turn out to be
usably close to the actual framerate, but you can't really trust it, especially not
with esoteric fileformats, or ones that are inherently VFR (like MKV for example).
The fact that the framerate for RMVB's is reported as 1000 isn't really a bug; if you
want some other equally bogus value instead you can just assumefps() it; you don't
need to add some random assumption to the API. If you want the real timestamps, use
the timecodes file or (in the API case) calculate them yourself.

2. Regarding the libs: what you need to link against obviously depends what you
   compiled your ffmpeg with; I would strongly discourage you from linking ffms2 to a
   ffmpeg compiled with a pile of encoding libs. Most of us do not compile ffmpeg with
   that, so regardless of what you do, you should not add ANYTHING beyond what is
   absolutely required to libs.cpp, unless you hide it within an #ifdef. If you want
   anything more than what is absolutely required (and what there is autodetection for
   already), add detection for it to build-msvc/msvc-autoconf.bat and add appropriate
   #ifdefs in libs.cpp. Patches to do this are likely to be accepted, assuming they are
   correctly done. Alternatively you can just use your own project file and add the
   extra libs to Visual Studio's list of additional dependencies.

3. int64_t is what is used everywhere else, so yes, that is OK. Remember that ffms2
   is a crossplatform library; windows-only code is only acceptable in the Avisynth part.

4. I have no idea what you just said and what does mp4 splitters have to do with
   anything?

5. The fact that FFMS_GetFrameByTime() (or rather, ClosestFrameFromDTS(), which it
   uses) is slow may be regarded as a bug and patches to fix that bug are welcome. It's
   trivial to work around it though by building your own list of timestamps and
   implementing your own version of FFMS_GetFrameByTime, if VFR->CFR conversion is time
   critical for you.

This patch is still rejected.