Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
adding a method to read the model file from an Android Archive #333
base: develop
Are you sure you want to change the base?
adding a method to read the model file from an Android Archive #333
Changes from all commits
96cc6f3
6fe6d38
51250d7
93dafcd
7c8ef9b
0760eea
766dcba
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Are we guaranteed to not have a null-terminator anywhere in the model file? Maybe for the current model only? Should we
write
instead (if we keep this loop)?...or even store to a
std::vector<char>
instead?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.
We read the forest params as string and the yaml file with the model is a text file. Using the '\0' is safe due it is the text termination and will not occur in the text file.
I checked the reading routine in Android and
Moving this to a vector or messing with the string termination will easily invalidate the hash value.
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.
You can have
\0
in a text file and in the middle of astring
. We might not in the current model, but usingoperator<<
(vswrite
with a size) will not read allBUFSIZ
bytes if a future model did have a\0
inside. My overall question for this is and the two comments for line 154 are why are we reading in chunks and manipulating C-style strings if we don't have to with the API Android gives us?Changing the storage container won't change the contents and we need to get to a C-style string for the digest function anyway (
vector<char>.data()
). I knowcalculateHashString
takes astring
, but it could just as easily and maybe more appropriately take something else in a refactor.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.
@RLessmann: please ignore my second comment (we have to get to a
std::string
to pass to OpenCVcv::FileStorage
eventually, so thevector
solution would duplicate RAM. However, I still find thess.write
to be more appropriate thanoperator<<
. This should not change anything, avoids string conversion on every iteration, and guards us in the future if there is ever a\0
in a file. Would you please make and test this change on device? It should be identical. I believe you can commit the suggestion below and run the branch.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.