FrameTimeCode should be identic to itself when applying the resulting values to create a new FrameTimeCode

[Wirg]

Hi !

Thank you for PySceneDetect.

I ran into issues where after some serialization the resulting time frame was not the expected one.
For example:

frame_time_code = FrameTimecode(61, fps=14)
seconds = frame_time_code.get_seconds()
time_code = frame_time_code.get_timecode()

...

FrameTimecode(seconds, fps=14).get_frames() == 60 # not 61
FrameTimecode(time_code, fps=14).get_frames() == 60 # not 61

I found the corresponding code with flooring (with int instead of round) :

PySceneDetect/scenedetect/frame_timecode.py

Lines 227 to 236 in f6c61dd

	def _seconds_to_frames(self, seconds):
	# type: (float) -> int
	""" Converts the passed value seconds to the nearest number of frames using
	the current FrameTimecode object's FPS (self.framerate).
	Returns:
	Integer number of frames the passed number of seconds represents using
	the current FrameTimecode's framerate property.
	"""
	return int(seconds * self.framerate)

PySceneDetect/scenedetect/frame_timecode.py

Line 307 in f6c61dd

return int(secs * self.framerate)

I ran into the following code which seems to indicate this is on purpose. What is the reason?

PySceneDetect/tests/test_frame_timecode.py

Lines 114 to 116 in f6c61dd

	assert FrameTimecode(timecode='00:00:01', fps=1).frame_num == 1
	assert FrameTimecode(timecode='00:00:01.9999', fps=1).frame_num == 1
	assert FrameTimecode(timecode='00:00:02.0000', fps=1).frame_num == 2

Have a nice day !

[Breakthrough]

Hey @Wirg;

Thanks for looking into this. It looks like some of the existing test cases are failing after your change (probably just a few off-by-one errors now), so will need to see why that is before merging #269 (thanks for the PR as well). I'm not sure if this should be taken for v0.5 or not, as this could possibly be a breaking change, but I'd be happy to consider it for v0.6.

I ended up doing a similar workaround in v0.6 for the PyAV backend, so I think your change is correct. Truthfully it's been quite a few years since I've touched FrameTimecode, so I can't really remember the intent behind these test cases you pointed out:

PySceneDetect/tests/test_frame_timecode.py

Lines 114 to 116 in f6c61dd

	assert FrameTimecode(timecode='00:00:01', fps=1).frame_num == 1
	assert FrameTimecode(timecode='00:00:01.9999', fps=1).frame_num == 1
	assert FrameTimecode(timecode='00:00:02.0000', fps=1).frame_num == 2

I may mark this as a known issue for v0.5, but bring your change in for v0.6. Thanks for the report!

[Breakthrough]

I've accepted your suggested solution for v0.6 (c5ac42b) and it will be included in the next beta release, as well as v0.6. I'm reluctant to merge this fix into v0.5 as it may constitute a breaking change for some existing applications.

TODO before closing this issue:

• Add identity unit tests as @Wirg suggested in fix: FrameTimeCode approximation flooring instead of rounding #269
• Update v0.5 changelog marking this as a known issue

[Wirg]

Thanks for the fast answer!

I think it's ok for me, I agree with your reasoning. I may try the v0.6 branch then.

[Breakthrough]

This is now available in the latest release of v0.6 (v0.6-rc0), feel free to give it a try!