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. |
| @@ -50,7 +50,7 @@ def test_empty_file(self, sample_scc_empty): | |||
| SCCReader().read(sample_scc_empty) | |||
|
|
|||
| 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.
| @@ -983,5 +983,7 @@ def _restructure_bytes_to_position_map(byte_to_pos_map): | |||
| # 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
| @@ -246,6 +250,28 @@ def read(self, content, lang='en-US', simulate_roll_up=False, offset=0): | |||
|
|
|||
| 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.
| @@ -195,7 +195,7 @@ def detect(self, content): | |||
| else: | |||
| 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.
| @@ -212,8 +212,12 @@ def read(self, content, lang='en-US', simulate_roll_up=False, offset=0): | |||
| :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
| @@ -474,7 +503,7 @@ class SCCWriter(BaseWriter): | |||
| def __init__(self, *args, **kw): | |||
| 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?
Default fps is 30.0 modified method such that it will support existing functions