Skip to content
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

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

RLessmann
Copy link
Contributor

Android Archives AAR contians an asset folder, which is
accessible by using the Android Asset Manager.
A reference to the Android Asset manager can be obtained from
the App using Java/Kotlin. Within a JNI call the reference
must be converted into an opaque C pointer.
The NDK Asset Manager functions can then be used to load the
model file from the AAR assets.

Android Archives AAR contians an asset folder, which is
accessible by using the Android Asset Manager.
A reference to the Android Asset manager can be obtained from
the App using Java/Kotlin. Within a JNI call the reference
must be converted into an opaque C pointer.
The NDK Asset Manager functions can then be used to load the
model file from the AAR assets.
@github-actions
Copy link

github-actions bot commented Feb 5, 2022

✅ Conformance regression test passed on all tested platforms.

@github-actions
Copy link

github-actions bot commented Feb 7, 2022

✅ Conformance regression test passed on all tested platforms.

Comment on lines +157 to +158
buffer[cnt] = 0; // add terminating '\0'
ss << buffer;
Copy link
Member

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)?

Suggested change
buffer[cnt] = 0; // add terminating '\0'
ss << buffer;
ss.write(buffer, cnt);

...or even store to a std::vector<char> instead?

Copy link
Contributor Author

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

  • it does not need much time
  • the MD5 of the file content can successfully compared with the given hash string.
    Moving this to a vector or messing with the string termination will easily invalidate the hash value.

Copy link
Member

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 a string. We might not in the current model, but using operator<< (vs write with a size) will not read all BUFSIZ 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 know calculateHashString takes a string, but it could just as easily and maybe more appropriately take something else in a refactor.

Copy link
Member

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 OpenCV cv::FileStorage eventually, so the vector solution would duplicate RAM. However, I still find the ss.write to be more appropriate than operator<<. 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.

NFIQ2/NFIQ2Algorithm/include/prediction/RandomForestML.h Outdated Show resolved Hide resolved
@gfiumara
Copy link
Member

gfiumara commented Feb 7, 2022

Also: can we push this to branch develop instead? I think that might be more appropriate, since the forwarding of different CMake args is needed.

RLessmann and others added 2 commits February 7, 2022 14:12
Co-authored-by: Greg Fiumara <greg@gregfiumara.com>
Co-authored-by: Greg Fiumara <greg@gregfiumara.com>
@github-actions
Copy link

github-actions bot commented Feb 7, 2022

✅ Conformance regression test passed on all tested platforms.

1 similar comment
@github-actions
Copy link

github-actions bot commented Feb 7, 2022

✅ Conformance regression test passed on all tested platforms.

@gfiumara gfiumara changed the base branch from master to develop February 8, 2022 13:15
@github-actions
Copy link

github-actions bot commented Feb 8, 2022

✅ Conformance regression test passed on all tested platforms.

Comment on lines +154 to +158
char buffer[BUFSIZ + 1]; // BUFSIZ defined in stdio.h
int cnt = AAsset_read(asset, buffer, BUFSIZ);
while (cnt > 0 && cnt <= BUFSIZ) {
buffer[cnt] = 0; // add terminating '\0'
ss << buffer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
char buffer[BUFSIZ + 1]; // BUFSIZ defined in stdio.h
int cnt = AAsset_read(asset, buffer, BUFSIZ);
while (cnt > 0 && cnt <= BUFSIZ) {
buffer[cnt] = 0; // add terminating '\0'
ss << buffer;
char buffer[BUFSIZ];
int cnt = AAsset_read(asset, buffer, BUFSIZ);
while (cnt > 0 && cnt <= BUFSIZ) {
ss.write(buffer, cnt);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants