Correction of FrameUtility V3
Finally, this should be the last version of FrameUtility. For reference, here are the previous version: #37, #46
Why it is needed?
The previous FrameUtility would not work with VFR video, now yes. Finally, the previous FrameUtility was more a hack than anything else.
What has been done
- Add TimeType Enum
- Add ffmpeg dependency to get the video file timestamps
- Add mkvtoolnix dependency to get the video file timestamps for mkv file since it is more performant then ffmpeg
- Be able to create Timestamps from 3 methods (
from_fps,from_video_file,from_timestamps_file) - Correction how to convert ms to ass_timestamps (
Convert.time) - Update of the algorithm of
Convert.ms_to_framesandConvert.frames_to_ms - Add many tests to be sure that we have the right behaviour
- Update
FrameUtilityto use theTimestampsclass. - Add proof for the algorith of ms_to_frames since it isn't intuitive.
What still need to be been done
- Correct the github action workflow. I don't know why, but when I try to run it with act, the workflow blocks and stop doing anything
- Update the logic of
FrameUtility.addto useTimestamps. I don't understand what is the expected behaviour of this method. - Update pyonfx version
- Pack ffmpeg with pyonfx. We could try to replicate this method
Had another quick glance at the code: do you think we could avoid uploading a new font (thus making use of the font we already fetch in the workflow)?
Also, how much does it take to generate the fake videos? Could we generate them on the fly while testing to keep the codebase minimal?
Finally, folder name "timestamps" doesn't seem to me to represent well what's inside. If you manage to remove the file I told you, we could drop it and move the python generator file outside of it, naming it as something like "utils.py".
Had another quick glance at the code: do you think we could avoid uploading a new font (thus making use of the font we already fetch in the workflow)?
If i do that, how could we generate the video locally (sewhen we don't run the workflow but simply running the file tests/timestamps/generate_test_video.py)? We need to have the font to generate the video.
Also, how much does it take to generate the fake videos? Could we generate them on the fly while testing to keep the codebase minimal?
Yes we could. It take less then 10 seconds. Important to note, we need to have installed mkvmerge AND ffmpeg to be able to run that script.
Finally, folder name "timestamps" doesn't seem to me to represent well what's inside. If you manage to remove the file I told you, we could drop it and move the python generator file outside of it, naming it as something like "utils.py".
If you are talking about the folder tests/timestamps, there is no need to rename the file generate_test_video.py to utils.py. I provided the script since I guess it can be useful for the maintenance to know how those videos where generated. A user will never call this script.
All the test in tests/test_timestamps.py use the file in the timestamps folder as source which if why I named it like that.
I push some changes.
- I removed the ms_to_frames, frames_to_ms and move_ms_to_frame method. Now, the user should use VideoTimestamps because the conversion weren't always perfect before.
- Now, the constructor of FrameUtility use
ABCTimestampsinstead of afps. The user will be able to pass a FPSTimestamps, VideoTimestamps or a TextFileTimestamps which should cover any use case. - I updated all the packages used in the CI.