PyonFX icon indicating copy to clipboard operation
PyonFX copied to clipboard

Correction of FrameUtility V3

Open moi15moi opened this issue 2 years ago • 2 comments

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_frames and Convert.frames_to_ms
  • Add many tests to be sure that we have the right behaviour
  • Update FrameUtility to use the Timestamps class.
  • 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.add to use Timestamps. 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

moi15moi avatar Nov 07 '23 21:11 moi15moi

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".

CoffeeStraw avatar Jan 16 '24 20:01 CoffeeStraw

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.

moi15moi avatar Jan 16 '24 21:01 moi15moi

I push some changes.

moi15moi avatar Dec 23 '24 01:12 moi15moi