-
Notifications
You must be signed in to change notification settings - Fork 472
[FEAT][Rust] Added Encoder Module #1710
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FEAT][Rust] Added Encoder Module #1710
Conversation
0xcd => (0xe28c9d, 3), // Top right corner | ||
0xce => (0xe28c9e, 3), // Bottom left corner | ||
0xcf => (0xe28c9f, 3), // Bottom right corner | ||
_ => (b'?' as u32, 1), // I'll do it eventually, I promise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this comment be resolved by adding the remaining ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not actually my comment, it is there in the C code, and it's also there in the Encoding module from last year.
I could add more chars if I had a table, a paper or a reference.
This source seems to have some, but a lot of them are already made. If you can confirm that this source is the one, I will implement it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have very less context around this.
@cfsmp3 @canihavesomecoffee any suggestions for resolving this comment or to keep it like this for now?
@steel-bucket some tests are not passing. |
@prateekmedia I think that's a bug, those tests are not passing in main either I assume. It's the exact same output in the latest PR 1721(not mine). I could make an empty PR to confirm it. |
Yeah its a bug, its caused by some very minute timing differences in the SRT files. I'm currently looking into it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Will merge after regressions run.
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit b63a29c...:
NOTE: The following tests have been failing on the master branch as well as the PR:
Congratulations: Merging this PR would fix the following tests:
All tests passing on the master branch were passed completely. Check the result page for more info. |
CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results, when compared to test for commit b63a29c...:
Your PR breaks these cases:
NOTE: The following tests have been failing on the master branch as well as the PR:
Congratulations: Merging this PR would fix the following tests:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Check the result page for more info. |
In raising this pull request, I confirm the following (please check boxes):
My familiarity with the project is as follows (check one):
In this PR, I have migrated a large part of
ccx_encoders_common.c
and the libraryccx_encoders_g608.c
alongside it's helper functions. I have also fixed some issues(wrong functions) in theencoding
module which were caught during regression testing. After this PR all the writing that is done on txt files is done by Rust.