Adding various fps support to SCC read and SCC write#264
Adding various fps support to SCC read and SCC write#264
Conversation
Default fps is 30.0 modified method such that it will support existing functions
|
@ana-nichifor please review I have resolved merge conflicts. |
|
|
||
| def test_positioning(self, sample_scc_multiple_positioning): | ||
| captions = SCCReader().read(sample_scc_multiple_positioning) | ||
| captions = SCCReader().read(sample_scc_multiple_positioning, src_fps=25.0) |
There was a problem hiding this comment.
This test doesn't check the timestamps and it's only focused on the SCCReader, please write dedicated tests for both SCCReader and SCCWriter and check if the resulted timestamps are correct.
| # Time to transmit a single codeword = 1 second / 29.97 | ||
| MICROSECONDS_PER_CODEWORD = 1000.0 * 1000.0 / (30.0 * 1000.0 / 1001.0) | ||
|
|
||
| def MICROSECONDS_PER_CODEWORD_fps(src_fps=30.0): |
There was a problem hiding this comment.
This is a function, not a constant, so it shouldn't be declared with capital letters. Please use only lowercase for function names as per PEP 8 style guide
|
|
||
| return captions | ||
|
|
||
| def _fix_last_timing(self, timing, src_fps=30.0): |
There was a problem hiding this comment.
This function is no longer used and it needs to be removed.
| return False | ||
|
|
||
| def read(self, content, lang='en-US', simulate_roll_up=False, offset=0): | ||
| def read(self, content, lang='en-US', simulate_roll_up=False, offset=0, src_fps=30.0): |
There was a problem hiding this comment.
It seems that the src_fps parameter gets passed a lot between methods without being used, so maybe it would be a good idea to declare it as a class attribute and assign its value in init. That way you can use it directly anywhere you need as self.fps. Please do the same for the writer class to have consistency.
| :type offset: int | ||
| :param offset: | ||
|
|
||
| :type src_fps: float |
There was a problem hiding this comment.
Please update the user documentation as well with a short explanation and a usage example for both reader and writer class. You can find the documentation in docs/supported_formats.rst at line 120
| super().__init__(*args, **kw) | ||
|
|
||
| def write(self, caption_set): | ||
| def write(self, caption_set, src_fps=30.0): |
There was a problem hiding this comment.
I don't think the name src_fps is a good fit, as this is no longer the fps dedicated to the source file, but to the output file. Maybe calling it simply fps would be easier?
2 participants
Default fps is 30.0 modified method such that it will support existing functions