- Notifications
You must be signed in to change notification settings - Fork50
[fix]: correctly retrieve the fileSize on 32 bits OS#306
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
"size_t" size depends on the OS. On 32-bit system it can only hold a 32bits variable = 4 294 967 296, which is not enough to store a big filesize (e.g: 14Go). We should use a 64-bits variable
| @@ -210,7 +210,7 @@ size_t FileProperties::getBitRate() const | |||
| return _avFormatContext->bit_rate; | |||
| } | |||
| size_t FileProperties::getFileSize() const | |||
| unsigned __int64 FileProperties::getFileSize() const | |||
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.
is it possible to define more something like in TuttleOFX:
https://github.com/tuttleofx/TuttleOFX/blob/develop/libraries/tuttle/src/tuttle/common/system/compatibility.hpp
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.
Do you want me to define a new type in std namespace ?
e.g (this is pseudo-code)
// compatibility problems...namespacestd{#ifdef 32bit-OS// use https://stackoverflow.com/questions/1505582/determining-32-vs-64-bit-in-c to define the system typetypedef Custom_size_tc_size_t// can hold 64-bits#elsetypedef ::size_tc_size_t#endif}
I'm not really use to c++ but doesn't it seem a bit weird ?
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
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 not just useuint64_t? It is multi-platform (#include <cstdint>) and dedicated to provide fixed size integer.
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.
Indeeduint64_t seems to be what you are looking for (it is provided by the C library).
Did you test it@adoussot ?
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, i thoughtunsigned __int64 was cross platform (which is not). souint64_t is correct. Thanks!
Is this Ok (RTM) ? Or should we still change something ? |
Would be good to add a comment to the function , like |
Since the previous PR#305 was incrementing the avtranscoder version directly in develop (and didn't merge to master) I stick to the same release policy and incremented micro version in086d074. If it's ok for you, could someone merge it ? Without any response, I will merge this PR by the end of the weekend. thanks |
"size_t" size depends on the OS. On 32-bit system it can only hold a 32
bits variable = 4 294 967 296, which is not enough to store a big file
size (e.g: 14Go). We should use a 64-bits variable