Dev/patch 5 #144
Dev/patch 5 #144
Conversation
|
Automatic code review is enabled for this repository. Reviewing this PR now. |
![devloai[bot]](https://avatars.githubusercontent.com/in/886113?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've reviewed the changes adding RKNN2 support and build system modifications. The implementation is technically sound, with good platform-specific handling. Suggested some improvements around documentation and build system structure to enhance maintainability and user experience. Please address the comments about documenting RKNN2-specific behavior and build system changes.
| @@ -228,7 +228,11 @@ int32_t FacePipelineModule::InitMaskPredict(InspireModel &model) { | |||
|
|
|||
| int32_t FacePipelineModule::InitRBGAntiSpoofing(InspireModel &model) { | |||
| auto input_size = model.Config().get<std::vector<int>>("input_size"); | |||
| #ifdef INFERENCE_HELPER_ENABLE_RKNN2 | |||
| m_rgb_anti_spoofing_ = std::make_shared<RBGAntiSpoofingAdapt>(input_size[0], true); | |||
There was a problem hiding this comment.
Consider adding a comment explaining why the second parameter true is needed for RKNN2. This will help future maintainers understand the purpose of this conditional compilation.
| @@ -98,9 +98,9 @@ cmake -DCMAKE_SYSTEM_NAME=Linux \ | |||
| -DISF_ENABLE_BENCHMARK=OFF \ | |||
| -DISF_ENABLE_USE_LFW_DATA=OFF \ | |||
| -DISF_ENABLE_TEST_EVALUATION=OFF \ | |||
| -DISF_BUILD_SHARED_LIBS=ON ${SCRIPT_DIR} | |||
| -DISF_BUILD_SHARED_LIBS=OFF ${SCRIPT_DIR} | |||
There was a problem hiding this comment.
Changing from shared to static libraries is a significant change. Please document the rationale for this change in the commit message or PR description, as it affects how the library will be linked by clients.
|
|
||
| make -j4 | ||
| make install | ||
| # make install |
There was a problem hiding this comment.
Instead of commenting out installation steps, consider making this configurable through a build flag. This would make it clearer when installation is intentionally skipped vs accidentally disabled.
| @@ -118,6 +118,8 @@ TEST_CASE("test_FacePipeline", "[face_pipeline]") { | |||
| TEST_PRINT_OUTPUT(true); | |||
|
|
|||
| SECTION("rgb liveness detect") { | |||
| #ifndef INFERENCE_HELPER_ENABLE_RKNN2 | |||
There was a problem hiding this comment.
The comment about quantization issues with RKNPU2 should be moved to a more visible location (like a known issues section in documentation) since this is an important limitation that affects functionality.
| message("Enable RKNN Inference") | ||
| link_directories(${ISF_RKNN_API_LIB}) | ||
| set(DEPEND rknn_api dl) | ||
| elseif(ISF_RKNPU_MAJOR STREQUAL "rknpu2" AND ISF_RK_COMPILER_TYPE STREQUAL "aarch64") |
There was a problem hiding this comment.
The RKNN dependency logic is getting complex. Consider extracting the RKNN configuration into a separate CMake module file to improve maintainability and reusability across different parts of the build system.
No description provided.