Closed RLessmann closed 2 years ago
:x: Conformance regression test for ubuntu-18.04 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for ubuntu-20.04 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for windows-2019 (x86) failed. Please review the artifacts for this run.
:x: Conformance regression test for windows-2022 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for windows-2022 (x86) failed. Please review the artifacts for this run.
:x: Conformance regression test for windows-2019 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for windows-2016 (x86) failed. Please review the artifacts for this run.
:x: Conformance regression test for windows-2016 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for macOS-11 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for macOS-10.15 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for ubuntu-18.04 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for ubuntu-20.04 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for macOS-11 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for ubuntu-20.04 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for windows-2019 (x86) failed. Please review the artifacts for this run.
:x: Conformance regression test for ubuntu-18.04 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for windows-2019 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for windows-2016 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for windows-2022 (x86) failed. Please review the artifacts for this run.
:x: Conformance regression test for windows-2022 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for macOS-11 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for windows-2016 (x86) failed. Please review the artifacts for this run.
:x: Conformance regression test for macOS-10.15 (x64) failed. Please review the artifacts for this run.
Note: Conformance test issue should be fixed in b392812bd1ee494f9b6325ad8c2f58bc8db14ad0, this isn't you. Though, CI is not testing build of the #ifdef
code here, so we should add that before merging, since this PR is destined for master
.
:x: Conformance regression test for ubuntu-20.04 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for windows-2022 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for ubuntu-18.04 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for windows-2016 (x86) failed. Please review the artifacts for this run.
:x: Conformance regression test for windows-2019 (x86) failed. Please review the artifacts for this run.
:x: Conformance regression test for windows-2022 (x86) failed. Please review the artifacts for this run.
:x: Conformance regression test for windows-2019 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for windows-2016 (x64) failed. Please review the artifacts for this run.
:x: Conformance regression test for macOS-11 (x64) failed. Please review the artifacts for this run.
:white_check_mark: Conformance regression test passed on all tested platforms.
:white_check_mark: Conformance regression test passed on all tested platforms.
:white_check_mark: Conformance regression test passed on all tested platforms.
:white_check_mark: Conformance regression test passed on all tested platforms.
:white_check_mark: Conformance regression test passed on all tested platforms.
Hello Greg,
Reading your comments, it makes completely sense. I will modify my pull request in this regard. However, I pressed on GitHub commit to your changes, which was probably not a good idea. It looks this makes it more difficult to handle the PR. I’m thinking of rejecting the PR and I will implement and test the changes locally on my side, afterwards I will open a new pull request for this. I’m kind of hesitant to commit the suggested changes without testing them on an actual device.
Regards
Ralph
From: Greg Fiumara @.> Sent: Mittwoch, 2. Februar 2022 13:56 To: usnistgov/NFIQ2 @.> Cc: RLessmann @.>; Author @.> Subject: [EXT] Re: [usnistgov/NFIQ2] Pull req using android asset mgr (PR #332)
Please use caution this is an externally originating email. This message was sent to a "crossmatch.com" email address. Please instruct the sender to update their contact information to use the replacement "hidglobal.com" email address.
@gfiumara requested changes on this pull request.
In NFIQ2/NFIQ2Algorithm/src/prediction/RandomForestML.cpphttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fusnistgov%2FNFIQ2%2Fpull%2F332%23discussion_r797542578&data=04%7C01%7Cralph.lessmann%40hidglobal.com%7Cc423f7759b2d4163254e08d9e64b590b%7Cf0bdc1c951484f86ac40edd976e1814c%7C0%7C0%7C637794034760382693%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=1jEjD%2B%2BTctQ%2B3BgVV1Cum%2BGioC7VVbn7VszRuwYH2Xw%3D&reserved=0:
@@ -123,6 +123,58 @@ NFIQ2::Prediction::RandomForestML::initModule(const std::string &fileName,
return hash;
}
+#ifdef ANDROID
+std::string
+NFIQ2::Prediction::RandomForestML::initModule(AAssetManager* assets,
const std::string &fileName,
const std::string &fileHash)
+{
if( assets == nullptr )
{
throw NFIQ2::Exception(NFIQ2::ErrorCode::InvalidConfiguration,
"The trained network could not be initialized!");
}
__android_log_write( ANDROID_LOG_INFO, "NFIQ2::Prediction::RandomForestML", "Using android asset manager to read model file" );
Let's not write to the Android log. This library doesn't produce status anywhere else, and only communicates errors through exceptions.
In NFIQ2/NFIQ2Algorithm/src/prediction/RandomForestML.cpphttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fusnistgov%2FNFIQ2%2Fpull%2F332%23discussion_r797545277&data=04%7C01%7Cralph.lessmann%40hidglobal.com%7Cc423f7759b2d4163254e08d9e64b590b%7Cf0bdc1c951484f86ac40edd976e1814c%7C0%7C0%7C637794034760382693%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=b5caCZ34Kfrd3cMoaGKs%2BSB8L2q1f%2Bdm6OZ6faa4XJ0%3D&reserved=0:
- throw NFIQ2::Exception(NFIQ2::ErrorCode::InvalidConfiguration,
Communicate a more descriptive error. ⬇️ Suggested change
throw NFIQ2::Exception(NFIQ2::ErrorCode::InvalidConfiguration,
"The trained network could not be initialized!");
throw NFIQ2::Exception(NFIQ2::ErrorCode::InvalidConfiguration,
"Prediction::RandomForestML::initModule: AAssetManager is "
"null");
In NFIQ2/NFIQ2Algorithm/src/prediction/RandomForestML.cpphttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fusnistgov%2FNFIQ2%2Fpull%2F332%23discussion_r797551978&data=04%7C01%7Cralph.lessmann%40hidglobal.com%7Cc423f7759b2d4163254e08d9e64b590b%7Cf0bdc1c951484f86ac40edd976e1814c%7C0%7C0%7C637794034760538908%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=datVTkqwuIHWDy12acVoYT%2F8zc80MYJ9cNQ5DNl7UJg%3D&reserved=0:
- while( ( file = AAssetDir_getNextFileName( assetDir ) ) != NULL )
{
if( fileName == std::string( file ) )
{
AAsset* asset = AAssetManager_open( assets, file, AASSET_MODE_STREAMING );
char buffer[BUFSIZ + 1];
memset( buffer, 0, BUFSIZ + 1 );
std::stringstream ss;
while( AAsset_read( asset, buffer, BUFSIZ ) > 0 )
{
ss << buffer;
}
params = ss.str();
AAsset_close( asset );
break;
}
}
AAssetDir_close( assetDir );
Do we need to inspect every file? Let's open the one we want directly. Haven't checked compilation but: ⬇️ Suggested change
while( ( file = AAssetDir_getNextFileName( assetDir ) ) != NULL )
{
if( fileName == std::string( file ) )
{
AAsset* asset = AAssetManager_open( assets, file, AASSET_MODE_STREAMING );
char buffer[BUFSIZ + 1];
memset( buffer, 0, BUFSIZ + 1 );
std::stringstream ss;
while( AAsset_read( asset, buffer, BUFSIZ ) > 0 )
{
ss << buffer;
}
params = ss.str();
AAsset_close( asset );
break;
}
}
AAssetDir_close( assetDir );
AAsset* asset = AAssetManager_open( assets, fileName.c_str(),
AASSET_MODE_STREAMING );
if (asset == nullptr) {
throw Exception(NFIQ2::ErrorCode::CannotReadFromFile,
"Prediction::RandomForestML::initModule: Cannot open model "
"asset (" + fileName + ")");
}
char buffer[BUFSIZ + 1];
memset( buffer, 0, BUFSIZ + 1 );
std::stringstream ss;
while( AAsset_read( asset, buffer, BUFSIZ ) > 0 )
{
ss << buffer;
}
params = ss.str();
AAsset_close( asset );
In NFIQ2/NFIQ2Algorithm/include/prediction/RandomForestML.hhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fusnistgov%2FNFIQ2%2Fpull%2F332%23discussion_r797561842&data=04%7C01%7Cralph.lessmann%40hidglobal.com%7Cc423f7759b2d4163254e08d9e64b590b%7Cf0bdc1c951484f86ac40edd976e1814c%7C0%7C0%7C637794034760538908%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=T3eGum51C8htwkxSrr5tjr%2F6MDGbGsP5S6JR9obnu08%3D&reserved=0:
@@ -8,6 +8,11 @@
+#ifdef ANDROID
+#include <android/asset_manager.h>
+#include <android/log.h>
Suggest we maintain no logging from this library ⬇️ Suggested change
-#include <android/log.h>
In NFIQ2/NFIQ2Algorithm/src/prediction/RandomForestML.cpphttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fusnistgov%2FNFIQ2%2Fpull%2F332%23discussion_r797565276&data=04%7C01%7Cralph.lessmann%40hidglobal.com%7Cc423f7759b2d4163254e08d9e64b590b%7Cf0bdc1c951484f86ac40edd976e1814c%7C0%7C0%7C637794034760538908%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=XyCJRePEQfQT3FKedAAEFHpXPurK5ufcPTvfUwPqIMs%3D&reserved=0:
- __android_log_write(ANDROID_LOG_INFO,
"NFIQ2::Prediction::RandomForestML",
"Using android asset manager to read model file");
Let's not log status from the library. ⬇️ Suggested change
__android_log_write(ANDROID_LOG_INFO,
"NFIQ2::Prediction::RandomForestML",
"Using android asset manager to read model file");
In NFIQ2/NFIQ2Algorithm/src/prediction/RandomForestML.cpphttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fusnistgov%2FNFIQ2%2Fpull%2F332%23discussion_r797567368&data=04%7C01%7Cralph.lessmann%40hidglobal.com%7Cc423f7759b2d4163254e08d9e64b590b%7Cf0bdc1c951484f86ac40edd976e1814c%7C0%7C0%7C637794034760538908%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=aCzEWq5CGmolrVBmJc9Y8C0hvX2u2xB0NZoZ%2FO3rg3Q%3D&reserved=0:
- AAssetDir *assetDir = AAssetManager_openDir(assets, "");
const char *file = nullptr;
std::string params;
while ((file = AAssetDir_getNextFileName(assetDir)) != NULL) {
if (fileName == std::string(file)) {
AAsset *asset = AAssetManager_open(assets, file,
AASSET_MODE_STREAMING);
char buffer[BUFSIZ + 1];
memset(buffer, 0, BUFSIZ + 1);
std::stringstream ss;
while (AAsset_read(asset, buffer, BUFSIZ) > 0) {
ss << buffer;
}
params = ss.str();
AAsset_close(asset);
break;
}
}
AAssetDir_close(assetDir);
Can we jump right to the file without iterating the asset directory contents? Also check that asset was correctly opened. ⬇️ Suggested change
AAssetDir *assetDir = AAssetManager_openDir(assets, "");
const char *file = nullptr;
std::string params;
while ((file = AAssetDir_getNextFileName(assetDir)) != NULL) {
if (fileName == std::string(file)) {
AAsset *asset = AAssetManager_open(assets, file,
AASSET_MODE_STREAMING);
char buffer[BUFSIZ + 1];
memset(buffer, 0, BUFSIZ + 1);
std::stringstream ss;
while (AAsset_read(asset, buffer, BUFSIZ) > 0) {
ss << buffer;
}
params = ss.str();
AAsset_close(asset);
break;
}
}
AAssetDir_close(assetDir);
AAsset *asset = AAssetManager_open(assets, fileName.c_str(),
AASSET_MODE_STREAMING);
if (asset == nullptr) {
throw Exception{CannotReadFromFile, "Could not open "
"asset (" + fileName + ")"};
}
char buffer[BUFSIZ + 1];
memset(buffer, 0, BUFSIZ + 1);
std::stringstream ss;
while (AAsset_read(asset, buffer, BUFSIZ) > 0) {
ss << buffer;
}
params = ss.str();
AAsset_close(asset);
In NFIQ2/NFIQ2Algorithm/src/prediction/RandomForestML.cpphttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fusnistgov%2FNFIQ2%2Fpull%2F332%23discussion_r797569284&data=04%7C01%7Cralph.lessmann%40hidglobal.com%7Cc423f7759b2d4163254e08d9e64b590b%7Cf0bdc1c951484f86ac40edd976e1814c%7C0%7C0%7C637794034760538908%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=vg5NhFLHHZRHi4%2BNTpNmrauGTl9%2FSz%2FWNVQvk%2F%2FoKuE%3D&reserved=0:
- AASSET_MODE_STREAMING);
char buffer[BUFSIZ + 1];
memset(buffer, 0, BUFSIZ + 1);
std::stringstream ss;
while (AAsset_read(asset, buffer, BUFSIZ) > 0) {
ss << buffer;
}
params = ss.str();
Can we eliminate all this C-style code with AAsset_getBuffer() instead? Might be better to have the OS efficiently read the file for us anyway?
In NFIQ2/NFIQ2Algorithm/src/prediction/RandomForestML.cpphttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fusnistgov%2FNFIQ2%2Fpull%2F332%23discussion_r797574402&data=04%7C01%7Cralph.lessmann%40hidglobal.com%7Cc423f7759b2d4163254e08d9e64b590b%7Cf0bdc1c951484f86ac40edd976e1814c%7C0%7C0%7C637794034760538908%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=a3r9IhNPc5EX%2FOKxnbV8oK4b%2FqV%2FQfq%2FXyrVdIHxSO4%3D&reserved=0:
- throw NFIQ2::Exception(NFIQ2::ErrorCode::InvalidConfiguration,
Communicate a better error message ⬇️ Suggested change
throw NFIQ2::Exception(NFIQ2::ErrorCode::InvalidConfiguration,
"The trained network could not be initialized!");
throw Exception{ErrorCode::CouldNotReadFile,
"The trained network could not be initialized (no data "
"read from asset (" + fileName + ")"};
— Reply to this email directly, view it on GitHubhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fusnistgov%2FNFIQ2%2Fpull%2F332%23pullrequestreview-870449996&data=04%7C01%7Cralph.lessmann%40hidglobal.com%7Cc423f7759b2d4163254e08d9e64b590b%7Cf0bdc1c951484f86ac40edd976e1814c%7C0%7C0%7C637794034760538908%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Y8i0eMQjJmsQu3lN1M5jUhHNSUSvniW4dcla9AyEEgk%3D&reserved=0, or unsubscribehttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAIYBGYZMG3IUQMGKMZXD723UZESULANCNFSM5NLWKLGA&data=04%7C01%7Cralph.lessmann%40hidglobal.com%7Cc423f7759b2d4163254e08d9e64b590b%7Cf0bdc1c951484f86ac40edd976e1814c%7C0%7C0%7C637794034760538908%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=DjkOP0EXoTKcIC%2BVqA%2FcgHYPXpPINDCBDVFzX%2BMN4BI%3D&reserved=0. You are receiving this because you authored the thread.Message ID: @.**@.>>
@RLessmann whatever you feel comfortable with, I'd also be happier with a real device test as well. I sent another email outside of GitHub suggesting that the develop
branch might be a better target for the PR as well.
It would be beneficial if we had a very small sample app we could put in the examples directory.
Hello Greg,
Yes but an sample app needs to use an jni interface to access the native code. I'm currently unable to contribute this sample, but would be willing too, if time permits.
Regards
Ralph
From: Greg Fiumara @.> Sent: Donnerstag, 3. Februar 2022 13:30 To: usnistgov/NFIQ2 @.> Cc: RLessmann @.>; Mention @.> Subject: [EXT] Re: [usnistgov/NFIQ2] Pull req using android asset mgr (PR #332)
Please use caution this is an externally originating email. This message was sent to a "crossmatch.com" email address. Please instruct the sender to update their contact information to use the replacement "hidglobal.com" email address.
@RLessmannhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FRLessmann&data=04%7C01%7Cralph.lessmann%40hidglobal.com%7Ca8a92e15c4784215cc9d08d9e710dc56%7Cf0bdc1c951484f86ac40edd976e1814c%7C0%7C0%7C637794881868601381%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=nd%2FAfbIjiGDrGVFMwqTrJA2k0xnJkkJ57lQCouZKsoY%3D&reserved=0 whatever you feel comfortable with, I'd also be happier with a real device test as well. I sent another email outside of GitHub suggesting that the develop branch might be a better target for the PR as well.
It would be beneficial if we had a very small sample app we could put in the examples directory.
- Reply to this email directly, view it on GitHubhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fusnistgov%2FNFIQ2%2Fpull%2F332%23issuecomment-1028941583&data=04%7C01%7Cralph.lessmann%40hidglobal.com%7Ca8a92e15c4784215cc9d08d9e710dc56%7Cf0bdc1c951484f86ac40edd976e1814c%7C0%7C0%7C637794881868601381%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=gV0i6u6udOB2lPWpERBbwqbKgcuCx2F7OnopMr2h7ak%3D&reserved=0, or unsubscribehttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAIYBGYZCIE3V43IWPCFTVBLUZJYKVANCNFSM5NLWKLGA&data=04%7C01%7Cralph.lessmann%40hidglobal.com%7Ca8a92e15c4784215cc9d08d9e710dc56%7Cf0bdc1c951484f86ac40edd976e1814c%7C0%7C0%7C637794881868601381%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=9TsR0bORHoeon2lLZj3W50W5%2B0yMVo22kbKMycA8VTI%3D&reserved=0. You are receiving this because you were mentioned.Message ID: @.**@.>>
On Android native libraries are packaged in Android Archive Files (AAR). These AAR files contains also an assets folder. The assets folder for the android NFIQ2 will contain the model file. However the native code cannot access the asset folder directly. It is required to use the asset manager. A reference to the asset manager can only be instantiated by the app using Java/Kotlin. This reference needs to be passed in into the JNI library and converted into a C/C++ opaque pointer by the NDK function AAssetManager_fromJava(...). The model file will be read by using the NDK Asset Manager functions.
This code part only applies to android and is excluded by pragmas for other operating systems.