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] Add time units module in lib_ccxr
#1556
base: master
Are you sure you want to change the base?
[FEAT] Add time units module in lib_ccxr
#1556
Conversation
lib_ccxr
|
||
impl MpegClockTick { | ||
/// The ratio to convert a clock tick to time duration. | ||
pub const MPEG_CLOCK_FREQ: i64 = 90000; |
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.
This is a global value in C. AFAIK, If you redefine it here it will contain an incorrect value, if the C constant is changed
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 converted it into a AtomicI64
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.
Why is there a need to use atomics here? We are running a single thread
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.
The only way to create a global variable in rust is to declare a static variable. Because a static variable can be changed by multiple threads, it requires atomics. There is a way to bypass it using a mutable static variable and using unsafe eveytime we try to access or change it. Do you want me go with this approach? I was hesitant at first since it used unsafe rust.
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.
Yes, as we don't use any multithreading, I would prefer if you just re-use the C global variable and not redeclare it in rust.
This is what we have decided in #1547 too
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 now re-used MPEG_CLOCK_FREQ
from C. And converted unwrap
to expect
.
ce4d9fc
to
43c5b83
Compare
43c5b83
to
b479f49
Compare
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results:
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). Your PR breaks these cases:
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:
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). Your PR breaks these cases:
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):
This is an attempt to split #1495 into multiple independant PRs. Dependant on #1551.
This PR adds types that enables to store time in different representations like simple milliseconds, number of frames, etc. And also allow to interconvert between them. It also provides functions for formatting time in different kinds of formats. Certain functions are integrated into C codebase.