Fix calculation of frame rate in JRSR parser

[SandTAS]

@fsvgm777 discovered that Frames and FrameRateOverride get set such that Frames / FrameRateOverride is not quite equal to the total movie duration. This is because the computation uses integer math that loses precision in intermediate results. The bug exists before and after #1062, which overhauled the parser.

You can see this in the fact that JRSR movies since the new site was inaugurated have an exact integer frame rate, whereas earlier movies have a frame rate that is close to 60.0 but not exactly equal, such that multiplying the frame rate by the number of frames gives the correct total duration. The exception is 6944, which I can't explain: besides the integer frame rate, the frame count should be 23458, not 23459, if computed by the formula in the code as I understand it.

Submission	Frame Count	Frame Rate
8079	38324	60
8033	25135	60
5893	93467	60
7105	951	60.018933417481854
7003	2454	59.99559934479134
6944	23459	60

I do not have a dev environment set up, so I won't make a pull request, but something like the patch below should fix it. It's possible some tests will have to be adjusted. The change also avoids a divide-by-zero when the move is less than 1 second long. (if (lastNonSpecialTimestamp > 0) looks like it was trying to avoid division by zero, but it was inadequate, because lastNonSpecialTimestamp / 1000000000L could still be zero.)

From 1cec646e5d156c035a80ec635768da83341065f9 Mon Sep 17 00:00:00 2001
From: David Fifield <david@bamsoftware.com>
Date: Fri, 11 Aug 2023 02:46:58 +0000
Subject: [PATCH] Fix calculation of FrameRateOverride in JRSR parser.

The calculation formerly used only integer arithmetic, which means that
Frames / FrameRateOverride was usually not quite equal to the actual
total duration.

https://tasvideos.org/Forum/Posts/524532

Also avoids a divide-by-zero when the movie is less than 1 second long.
---
 TASVideos.Parsers/Parsers/Jrsr.cs | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/TASVideos.Parsers/Parsers/Jrsr.cs b/TASVideos.Parsers/Parsers/Jrsr.cs
index f61f75c9..ed6bc02b 100644
--- a/TASVideos.Parsers/Parsers/Jrsr.cs
+++ b/TASVideos.Parsers/Parsers/Jrsr.cs
@@ -205,14 +205,21 @@ public class Jrsr : IParser
 				// initialization, diskinfo-*, others are ignored.
 			}
 
-			// "When computing movie length, it is customary to ignore all
-			// special events."
-			if (lastNonSpecialTimestamp > 0)
+			checked
 			{
-				checked
+				// Get total duration in seconds.
+				// "When computing movie length, it is
+				// customary to ignore all special events."
+				double duration = (double)lastNonSpecialTimestamp / 1e9;
+				// Compute an integer number of frames,
+				// assuming a frame rate close to 60 fps.
+				result.Frames = (int)Math.floor(duration * 60.0);
+				// Fine-tune the frame rate to yield the
+				// correct total duration when multiplied by
+				// the number of frames.
+				if (duration > 0.0)
 				{
-					result.Frames = (int)(lastNonSpecialTimestamp / 16666667);
-					result.FrameRateOverride = result.Frames / (lastNonSpecialTimestamp / 1000000000L);
+					result.FrameRateOverride = (double)result.Frames / duration;
 				}
 			}
 		}
-- 
2.39.2

[fsvgm777]

I've tested your code in my local environment just now, and while it does work as intended, it seems result.FrameRateOverride = result.Frames / ((double)lastNonSpecialTimestamp / 1000000000L); (i.e. converting the result of the lastNonSpecialTimestamp variable to a double in this line in the current code) fixes the issue as well and also won't trip up any division by zero problems (from my testing, at least). A PR for either fix can be opened, though I personally prefer the simpler one. What do you think?

The exception is 6944, which I can't explain: besides the integer frame rate, the frame count should be 23458, not 23459, if computed by the formula in the code as I understand it.

This was an "issue" on the old site, which had a different parser. Although if I recall correctly, it just ignored the frame count altogether, so the total run time is still correct.

[adelikat]

I've tested your code in my local environment just now, and while it does work as intended, it seems result.FrameRateOverride = result.Frames / ((double)lastNonSpecialTimestamp / 1000000000L); (i.e. converting the result of the lastNonSpecialTimestamp variable to a double in this line in the current code) fixes the issue as well and also won't trip up any division by zero problems (from my testing, at least). A PR for either fix can be opened, though I personally prefer the simpler one. What do you think?

Yes, I would prefer a PR that casts as double like you show here

[SandTAS]

Computing Frames as in the suggested patch avoids a similar but more minor loss of precision. Consider a movie that's 166666668888 ns in duration (about 3m47s). At 60 fps, that should be just over 10000 frames, 10000.00013328, which rounds down to 10000. The current code, though, would (integer) divide by 16666667 to get 9999. In cases like this, the suggested patch gives a more accurate Frames as well as a FrameRateOverride that is closer to the target of 60. Such a discrepancy is possible whenever the total duration is close to a multiple of 1/60 s.

Since you have to compute (double)lastNonSpecialTimestamp / 1000000000L for the denominator of FrameRateOverride anyway, you might as well do it at the top and use it for a more accurate computation of Frames as well.

Ignoring the loss of precision, the way the current code divides by 16666667 ns is a roundabout way to express what it's really doing: multiplying by 60 Hz. I think it's more clear if the code actually says * 60.0.

I noted an error in a comment in the patch I originally posted. It should have said "divided by", not "multiplied by". Here is a revision.
jrsr-framerate.patch.gz

[fsvgm777]

In the current code, would dividing lastNonSpecialTimestamp by (double)1000000000 / 60 instead result in the correct frame count if a movie is 166666668888 ns long as opposed to dividing it by 16666667?

Although I personally don't think it matters either way, considering most DOS games run at closer to 70 FPS anyway (according to JPC-RR).

[SandTAS]

In the current code, would dividing lastNonSpecialTimestamp by (double)1000000000 / 60 instead result in the correct frame count if a movie is 166666668888 ns long as opposed to dividing it by 16666667?

Yes—which is what the suggested patch does.

lastNonSpecialTimestamp / ((double)1000000000 / 60)

is the same as

(double)lastNonSpecialTimestamp / 1000000000 * 60.0

which is the same as

duration * 60.0

Although I personally don't think it matters either way, considering most DOS games run at closer to 70 FPS anyway (according to JPC-RR).

Right. This invention of a frame count and framerate is a complete fiction on the part of the parser. JRSR has no notion of frames, only nanoseconds. I can only guess that it works this way because other parts of the site code have a requirement that movie duration be expressed in the form num_frames / frame_rate.

In terms of getting the correct total duration, it's true that the frame count doesn't matter, as the frame rate can always be adjusted appropriately. If all that matters is the total duration, it would be enough to always set Frames = 1 and FrameRateOverride = 1.0 / duration. But if the goal is to have Frames be the largest integer number of frames at 60 fps such that it does not exceed the actual total duration (which seems to be the intent of the old code), then the way I suggest will be slightly more accurate in a few corner cases. Whether what the code is trying to do is sensible is worth further consideration, but my only goal in this issue was to make it do what it's trying to do, more correctly.

[fsvgm777]

Feel free to open a PR with your fixes. Though I did notice one small error which I forgot to tell you, my bad on that (it's supposed to be Math.Floor, otherwise, it yields an error). I've uploaded the fixed patch file right here. Note that I already tested your code (with the small fix) in my local environment, and it works as it should.

jrsr-framerate-fix.patch

[adelikat]

This was fixed with the merged PR