[ml service] Refactor: Decompose ml_single_open_custom function for better maintainability - #651

Open
songgot wants to merge 1 commit into
nnstreamer:mainfrom
songgot:Repectoring_open_custom
Open

[ml service] Refactor: Decompose ml_single_open_custom function for better maintainability#651
songgot wants to merge 1 commit into
nnstreamer:mainfrom
songgot:Repectoring_open_custom

Conversation

@songgot

Copy link
Copy Markdown
Contributor

This commit refactors the monolithic ml_single_open_custom function (200+ lines) into smaller, focused helper functions to improve code readability and maintainability. The main function is now structured as a clear sequence of steps, each handled by a dedicated helper function.

@hj210hj210 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This patch refactors a 200+ line function into five smaller functions,
improving readability and maintainability. 👍

Additional recommendations:
a. Avoid the '__' prefix—it’s reserved for system/compiler use.
b. Split _configure_nnfw_tensors by NNFW types to adhere to SRP.

Comment threadc/src/ml-api-inference-single.c Outdated
Comment threadc/src/ml-api-inference-single.c Outdated
…etter maintainability
This commit refactors the monolithic ml_single_open_custom function (200+ lines)
into smaller, focused helper functions to improve code readability and
maintainability. The main function is now structured as a clear sequence
of steps, each handled by a dedicated helper function.
Signed-off-by: hyunil park <hyunil46.park@samsung.com>
@songgot
songgotforce-pushed the Repectoring_open_custom branch from 94e9e4e to 9ed6579CompareNovember 5, 2025 04:29
@myungjoo

Copy link
Copy Markdown
Member

From cursor. (both look legit)

  1. c/src/ml-api-inference-single.c:L1154

_configure_nnfw_tensors()가 info->nnfw만 참조합니다. 그런데 직전에 호출한 _validate_and_determine_nnfw()는 검증 결과를 info->nnfw에 다시 써주지 않아서, 사용자가 ML_NNFW_TYPE_ANY(또는 잘못된 값)을 넘기면 여기 switch가 default로 빠집니다. TensorFlow/NNFW 등에서는 in/out 텐서 메타데이터 설정이 필수인데 이 단계가 생략돼 런타임 오류가 발생합니다. 검증된 nnfw를 info->nnfw에 저장하거나 이 함수가 nnfw 인자를 직접 받아 사용하도록 조정이 필요해 보입니다.

  1. c/src/ml-api-inference-single.c:L1181

framework_name/framework 옵션을 통해 info->fw_name에 값을 넣을 수 있지만, _configure_filter_properties()에서는 항상 fw_name 인자(자동 감지 결과)만 사용합니다. 그래서 사용자가 명시적으로 프레임워크 문자열을 지정해도 더 이상 반영되지 않아 기존 기능이 회귀된 상태입니다. info->fw_name이 존재하면 이를 우선 사용하도록 복원해 주세요.

@songgot

Copy link
Copy Markdown
ContributorAuthor

From cursor. (both look legit)

  1. c/src/ml-api-inference-single.c:L1154

_configure_nnfw_tensors()가 info->nnfw만 참조합니다. 그런데 직전에 호출한 _validate_and_determine_nnfw()는 검증 결과를 info->nnfw에 다시 써주지 않아서, 사용자가 ML_NNFW_TYPE_ANY(또는 잘못된 값)을 넘기면 여기 switch가 default로 빠집니다. TensorFlow/NNFW 등에서는 in/out 텐서 메타데이터 설정이 필수인데 이 단계가 생략돼 런타임 오류가 발생합니다. 검증된 nnfw를 info->nnfw에 저장하거나 이 함수가 nnfw 인자를 직접 받아 사용하도록 조정이 필요해 보입니다.

  1. c/src/ml-api-inference-single.c:L1181

framework_name/framework 옵션을 통해 info->fw_name에 값을 넣을 수 있지만, _configure_filter_properties()에서는 항상 fw_name 인자(자동 감지 결과)만 사용합니다. 그래서 사용자가 명시적으로 프레임워크 문자열을 지정해도 더 이상 반영되지 않아 기존 기능이 회귀된 상태입니다. info->fw_name이 존재하면 이를 우선 사용하도록 복원해 주세요.

Thank you for your review.
I will properly revise this PR after completing some minor refactoring.

@myungjoo-botmyungjoo-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated review (transcribed from an AI review agent's report; please verify before acting).

Summary: The PR splits ml_single_open_custom() in c/src/ml-api-inference-single.c into eight static helpers, renames __setup_in_out_tensors -> _setup_in_out_tensors, and reformats type * var -> type *var across ~25 unrelated hunks. I traced every path of the original (validation order, goto error -> ml_single_close, list_models / hw_name / converted_models release, *single = NULL init, return codes, NNTR_INF block) against the decomposed version. Cleanup order and resource handling are preserved, but two data-flow changes alter behavior, and the PR conflicts with current main.

  1. [High] Resolved nnfw type not used for tensors setup_configure_nnfw_tensors (PR ~line 1154, called at ~1283) switches on info->nnfw (user value) instead of the local nnfw rewritten by _ml_validate_model_file() for ML_NNFW_TYPE_ANY. With ML_NNFW_TYPE_ANY and a .pb / .pt model the switch hits default, the mandatory in/out info push is skipped, and open fails with ML_ERROR_STREAMS_PIPE even when valid infos were supplied (previously: success, or a clear ML_ERROR_INVALID_PARAMETER). ml_single_create_handle (nnfw) and fw_name do use the resolved value, so handle and tensors config disagree. Fix: pass nnfw as a parameter and switch (nnfw) (do not write back into caller-owned info).
  2. [High] framework_name option silently ignored_configure_filter_properties (~1181): the original used info->fw_name when set, else _ml_get_nnfw_subplugin_name (nnfw). The PR always passes the auto-derived name, so the explicit framework_name from ml_single_open_with_option() (still populated at ~1412-1414) is dead. Public-API regression. Fix: const char *framework = info->fw_name ? info->fw_name : fw_name; in g_object_set.
  3. [High] Merge conflict with maingit merge-tree reports a content conflict in this file. Since the PR base, main added "latency", info->latency_mode to the g_object_set call (507be1d; would be dropped by a naive rebase into _configure_filter_properties), rewrote __setup_in_out_tensors (d53de78), and changed ml_single_close (472be75). Also, your own open #661 replaces exactly the _configure_general/armnn/nnfw_tensors region; the two PRs conflict with each other and should be rebased into one series.
  4. [Medium] Existing tests cannot catch #1 or #2 — the ML_NNFW_TYPE_ANY tests use tflite (falls into default in both versions), and the only framework_name test sets the name equal to the auto-detected one. Please add (a) under ENABLE_TENSORFLOW, ml_single_open of the .pb model with ML_NNFW_TYPE_ANY + in/out infos expecting ML_ERROR_NONE, and (b) an ml_option test with a framework_name different from the default subplugin name, asserting via ml_single_get_property (single, "framework", ...).
  5. [Medium] Android CI failed on all four ABIs (logs expired). _ml_convert_predefined_entity has an __ANDROID__-only branch and is now called twice (see #7). Please re-run after rebase and confirm green.
  6. [Low]_configure_handle (~1045) is a single assignment that can only return ML_ERROR_NONE, yet the caller (~1277-1280) has a dead goto error. Fold it into _configure_async_settings (as void) or inline both.
  7. [Low]_ml_convert_predefined_entity (info->models) is now called twice (~1010 and ~1178). Convert once in ml_single_open_custom and pass the result to both helpers.
  8. [Low] Unrelated churn — 25 of 30 hunks are type * var -> type *var reformatting in untouched functions, contradicting the repo-wide gst-indent convention (.github/workflows/static.check.scripts/indent.sh) and being the main cause of the conflict. The __ -> _ rename is also partial (__invoke, __process_output, __ml_validate_model_file untouched). Please drop the reformatting / rename from this PR.
  9. [Low] Doxygen — remove the "Single Responsibility Principle" editorializing in _configure_nnfw_tensors; document @param[out] determined_nnfw / @return in _validate_and_determine_nnfw; char *hw_name in _configure_filter_properties should be const char *.

No back-door or suspicious behavior found.

Sign up for freeto 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.

4 participants

@songgot@myungjoo@hj210@myungjoo-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

[ml service] Refactor: Decompose ml_single_open_custom function for better maintainability - #651

Open
songgot wants to merge 1 commit into
nnstreamer:mainfrom
songgot:Repectoring_open_custom
Open

[ml service] Refactor: Decompose ml_single_open_custom function for better maintainability#651
songgot wants to merge 1 commit into
nnstreamer:mainfrom
songgot:Repectoring_open_custom

Conversation

@songgot

Copy link
Copy Markdown
Contributor

This commit refactors the monolithic ml_single_open_custom function (200+ lines) into smaller, focused helper functions to improve code readability and maintainability. The main function is now structured as a clear sequence of steps, each handled by a dedicated helper function.

@hj210hj210 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This patch refactors a 200+ line function into five smaller functions,
improving readability and maintainability. 👍

Additional recommendations:
a. Avoid the '__' prefix—it’s reserved for system/compiler use.
b. Split _configure_nnfw_tensors by NNFW types to adhere to SRP.

Comment threadc/src/ml-api-inference-single.c Outdated
Comment threadc/src/ml-api-inference-single.c Outdated
…etter maintainability
This commit refactors the monolithic ml_single_open_custom function (200+ lines)
into smaller, focused helper functions to improve code readability and
maintainability. The main function is now structured as a clear sequence
of steps, each handled by a dedicated helper function.
Signed-off-by: hyunil park <hyunil46.park@samsung.com>
@songgot
songgotforce-pushed the Repectoring_open_custom branch from 94e9e4e to 9ed6579CompareNovember 5, 2025 04:29
@myungjoo

Copy link
Copy Markdown
Member

From cursor. (both look legit)

  1. c/src/ml-api-inference-single.c:L1154

_configure_nnfw_tensors()가 info->nnfw만 참조합니다. 그런데 직전에 호출한 _validate_and_determine_nnfw()는 검증 결과를 info->nnfw에 다시 써주지 않아서, 사용자가 ML_NNFW_TYPE_ANY(또는 잘못된 값)을 넘기면 여기 switch가 default로 빠집니다. TensorFlow/NNFW 등에서는 in/out 텐서 메타데이터 설정이 필수인데 이 단계가 생략돼 런타임 오류가 발생합니다. 검증된 nnfw를 info->nnfw에 저장하거나 이 함수가 nnfw 인자를 직접 받아 사용하도록 조정이 필요해 보입니다.

  1. c/src/ml-api-inference-single.c:L1181

framework_name/framework 옵션을 통해 info->fw_name에 값을 넣을 수 있지만, _configure_filter_properties()에서는 항상 fw_name 인자(자동 감지 결과)만 사용합니다. 그래서 사용자가 명시적으로 프레임워크 문자열을 지정해도 더 이상 반영되지 않아 기존 기능이 회귀된 상태입니다. info->fw_name이 존재하면 이를 우선 사용하도록 복원해 주세요.

@songgot

Copy link
Copy Markdown
ContributorAuthor

From cursor. (both look legit)

  1. c/src/ml-api-inference-single.c:L1154

_configure_nnfw_tensors()가 info->nnfw만 참조합니다. 그런데 직전에 호출한 _validate_and_determine_nnfw()는 검증 결과를 info->nnfw에 다시 써주지 않아서, 사용자가 ML_NNFW_TYPE_ANY(또는 잘못된 값)을 넘기면 여기 switch가 default로 빠집니다. TensorFlow/NNFW 등에서는 in/out 텐서 메타데이터 설정이 필수인데 이 단계가 생략돼 런타임 오류가 발생합니다. 검증된 nnfw를 info->nnfw에 저장하거나 이 함수가 nnfw 인자를 직접 받아 사용하도록 조정이 필요해 보입니다.

  1. c/src/ml-api-inference-single.c:L1181

framework_name/framework 옵션을 통해 info->fw_name에 값을 넣을 수 있지만, _configure_filter_properties()에서는 항상 fw_name 인자(자동 감지 결과)만 사용합니다. 그래서 사용자가 명시적으로 프레임워크 문자열을 지정해도 더 이상 반영되지 않아 기존 기능이 회귀된 상태입니다. info->fw_name이 존재하면 이를 우선 사용하도록 복원해 주세요.

Thank you for your review.
I will properly revise this PR after completing some minor refactoring.

@myungjoo-botmyungjoo-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated review (transcribed from an AI review agent's report; please verify before acting).

Summary: The PR splits ml_single_open_custom() in c/src/ml-api-inference-single.c into eight static helpers, renames __setup_in_out_tensors -> _setup_in_out_tensors, and reformats type * var -> type *var across ~25 unrelated hunks. I traced every path of the original (validation order, goto error -> ml_single_close, list_models / hw_name / converted_models release, *single = NULL init, return codes, NNTR_INF block) against the decomposed version. Cleanup order and resource handling are preserved, but two data-flow changes alter behavior, and the PR conflicts with current main.

  1. [High] Resolved nnfw type not used for tensors setup_configure_nnfw_tensors (PR ~line 1154, called at ~1283) switches on info->nnfw (user value) instead of the local nnfw rewritten by _ml_validate_model_file() for ML_NNFW_TYPE_ANY. With ML_NNFW_TYPE_ANY and a .pb / .pt model the switch hits default, the mandatory in/out info push is skipped, and open fails with ML_ERROR_STREAMS_PIPE even when valid infos were supplied (previously: success, or a clear ML_ERROR_INVALID_PARAMETER). ml_single_create_handle (nnfw) and fw_name do use the resolved value, so handle and tensors config disagree. Fix: pass nnfw as a parameter and switch (nnfw) (do not write back into caller-owned info).
  2. [High] framework_name option silently ignored_configure_filter_properties (~1181): the original used info->fw_name when set, else _ml_get_nnfw_subplugin_name (nnfw). The PR always passes the auto-derived name, so the explicit framework_name from ml_single_open_with_option() (still populated at ~1412-1414) is dead. Public-API regression. Fix: const char *framework = info->fw_name ? info->fw_name : fw_name; in g_object_set.
  3. [High] Merge conflict with maingit merge-tree reports a content conflict in this file. Since the PR base, main added "latency", info->latency_mode to the g_object_set call (507be1d; would be dropped by a naive rebase into _configure_filter_properties), rewrote __setup_in_out_tensors (d53de78), and changed ml_single_close (472be75). Also, your own open #661 replaces exactly the _configure_general/armnn/nnfw_tensors region; the two PRs conflict with each other and should be rebased into one series.
  4. [Medium] Existing tests cannot catch #1 or #2 — the ML_NNFW_TYPE_ANY tests use tflite (falls into default in both versions), and the only framework_name test sets the name equal to the auto-detected one. Please add (a) under ENABLE_TENSORFLOW, ml_single_open of the .pb model with ML_NNFW_TYPE_ANY + in/out infos expecting ML_ERROR_NONE, and (b) an ml_option test with a framework_name different from the default subplugin name, asserting via ml_single_get_property (single, "framework", ...).
  5. [Medium] Android CI failed on all four ABIs (logs expired). _ml_convert_predefined_entity has an __ANDROID__-only branch and is now called twice (see #7). Please re-run after rebase and confirm green.
  6. [Low]_configure_handle (~1045) is a single assignment that can only return ML_ERROR_NONE, yet the caller (~1277-1280) has a dead goto error. Fold it into _configure_async_settings (as void) or inline both.
  7. [Low]_ml_convert_predefined_entity (info->models) is now called twice (~1010 and ~1178). Convert once in ml_single_open_custom and pass the result to both helpers.
  8. [Low] Unrelated churn — 25 of 30 hunks are type * var -> type *var reformatting in untouched functions, contradicting the repo-wide gst-indent convention (.github/workflows/static.check.scripts/indent.sh) and being the main cause of the conflict. The __ -> _ rename is also partial (__invoke, __process_output, __ml_validate_model_file untouched). Please drop the reformatting / rename from this PR.
  9. [Low] Doxygen — remove the "Single Responsibility Principle" editorializing in _configure_nnfw_tensors; document @param[out] determined_nnfw / @return in _validate_and_determine_nnfw; char *hw_name in _configure_filter_properties should be const char *.

No back-door or suspicious behavior found.

Sign up for freeto 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.

4 participants

@songgot@myungjoo@hj210@myungjoo-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[ml service] Refactor: Decompose ml_single_open_custom function for better maintainability - #651

Open
songgot wants to merge 1 commit into
nnstreamer:mainfrom
songgot:Repectoring_open_custom
Open

[ml service] Refactor: Decompose ml_single_open_custom function for better maintainability#651
songgot wants to merge 1 commit into
nnstreamer:mainfrom
songgot:Repectoring_open_custom

Conversation

@songgot

Copy link
Copy Markdown
Contributor

This commit refactors the monolithic ml_single_open_custom function (200+ lines) into smaller, focused helper functions to improve code readability and maintainability. The main function is now structured as a clear sequence of steps, each handled by a dedicated helper function.

@hj210hj210 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This patch refactors a 200+ line function into five smaller functions,
improving readability and maintainability. 👍

Additional recommendations:
a. Avoid the '__' prefix—it’s reserved for system/compiler use.
b. Split _configure_nnfw_tensors by NNFW types to adhere to SRP.

Comment threadc/src/ml-api-inference-single.c Outdated
Comment threadc/src/ml-api-inference-single.c Outdated
…etter maintainability
This commit refactors the monolithic ml_single_open_custom function (200+ lines)
into smaller, focused helper functions to improve code readability and
maintainability. The main function is now structured as a clear sequence
of steps, each handled by a dedicated helper function.
Signed-off-by: hyunil park <hyunil46.park@samsung.com>
@songgot
songgotforce-pushed the Repectoring_open_custom branch from 94e9e4e to 9ed6579CompareNovember 5, 2025 04:29
@myungjoo

Copy link
Copy Markdown
Member

From cursor. (both look legit)

  1. c/src/ml-api-inference-single.c:L1154

_configure_nnfw_tensors()가 info->nnfw만 참조합니다. 그런데 직전에 호출한 _validate_and_determine_nnfw()는 검증 결과를 info->nnfw에 다시 써주지 않아서, 사용자가 ML_NNFW_TYPE_ANY(또는 잘못된 값)을 넘기면 여기 switch가 default로 빠집니다. TensorFlow/NNFW 등에서는 in/out 텐서 메타데이터 설정이 필수인데 이 단계가 생략돼 런타임 오류가 발생합니다. 검증된 nnfw를 info->nnfw에 저장하거나 이 함수가 nnfw 인자를 직접 받아 사용하도록 조정이 필요해 보입니다.

  1. c/src/ml-api-inference-single.c:L1181

framework_name/framework 옵션을 통해 info->fw_name에 값을 넣을 수 있지만, _configure_filter_properties()에서는 항상 fw_name 인자(자동 감지 결과)만 사용합니다. 그래서 사용자가 명시적으로 프레임워크 문자열을 지정해도 더 이상 반영되지 않아 기존 기능이 회귀된 상태입니다. info->fw_name이 존재하면 이를 우선 사용하도록 복원해 주세요.

@songgot

Copy link
Copy Markdown
ContributorAuthor

From cursor. (both look legit)

  1. c/src/ml-api-inference-single.c:L1154

_configure_nnfw_tensors()가 info->nnfw만 참조합니다. 그런데 직전에 호출한 _validate_and_determine_nnfw()는 검증 결과를 info->nnfw에 다시 써주지 않아서, 사용자가 ML_NNFW_TYPE_ANY(또는 잘못된 값)을 넘기면 여기 switch가 default로 빠집니다. TensorFlow/NNFW 등에서는 in/out 텐서 메타데이터 설정이 필수인데 이 단계가 생략돼 런타임 오류가 발생합니다. 검증된 nnfw를 info->nnfw에 저장하거나 이 함수가 nnfw 인자를 직접 받아 사용하도록 조정이 필요해 보입니다.

  1. c/src/ml-api-inference-single.c:L1181

framework_name/framework 옵션을 통해 info->fw_name에 값을 넣을 수 있지만, _configure_filter_properties()에서는 항상 fw_name 인자(자동 감지 결과)만 사용합니다. 그래서 사용자가 명시적으로 프레임워크 문자열을 지정해도 더 이상 반영되지 않아 기존 기능이 회귀된 상태입니다. info->fw_name이 존재하면 이를 우선 사용하도록 복원해 주세요.

Thank you for your review.
I will properly revise this PR after completing some minor refactoring.

@myungjoo-botmyungjoo-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated review (transcribed from an AI review agent's report; please verify before acting).

Summary: The PR splits ml_single_open_custom() in c/src/ml-api-inference-single.c into eight static helpers, renames __setup_in_out_tensors -> _setup_in_out_tensors, and reformats type * var -> type *var across ~25 unrelated hunks. I traced every path of the original (validation order, goto error -> ml_single_close, list_models / hw_name / converted_models release, *single = NULL init, return codes, NNTR_INF block) against the decomposed version. Cleanup order and resource handling are preserved, but two data-flow changes alter behavior, and the PR conflicts with current main.

  1. [High] Resolved nnfw type not used for tensors setup_configure_nnfw_tensors (PR ~line 1154, called at ~1283) switches on info->nnfw (user value) instead of the local nnfw rewritten by _ml_validate_model_file() for ML_NNFW_TYPE_ANY. With ML_NNFW_TYPE_ANY and a .pb / .pt model the switch hits default, the mandatory in/out info push is skipped, and open fails with ML_ERROR_STREAMS_PIPE even when valid infos were supplied (previously: success, or a clear ML_ERROR_INVALID_PARAMETER). ml_single_create_handle (nnfw) and fw_name do use the resolved value, so handle and tensors config disagree. Fix: pass nnfw as a parameter and switch (nnfw) (do not write back into caller-owned info).
  2. [High] framework_name option silently ignored_configure_filter_properties (~1181): the original used info->fw_name when set, else _ml_get_nnfw_subplugin_name (nnfw). The PR always passes the auto-derived name, so the explicit framework_name from ml_single_open_with_option() (still populated at ~1412-1414) is dead. Public-API regression. Fix: const char *framework = info->fw_name ? info->fw_name : fw_name; in g_object_set.
  3. [High] Merge conflict with maingit merge-tree reports a content conflict in this file. Since the PR base, main added "latency", info->latency_mode to the g_object_set call (507be1d; would be dropped by a naive rebase into _configure_filter_properties), rewrote __setup_in_out_tensors (d53de78), and changed ml_single_close (472be75). Also, your own open #661 replaces exactly the _configure_general/armnn/nnfw_tensors region; the two PRs conflict with each other and should be rebased into one series.
  4. [Medium] Existing tests cannot catch #1 or #2 — the ML_NNFW_TYPE_ANY tests use tflite (falls into default in both versions), and the only framework_name test sets the name equal to the auto-detected one. Please add (a) under ENABLE_TENSORFLOW, ml_single_open of the .pb model with ML_NNFW_TYPE_ANY + in/out infos expecting ML_ERROR_NONE, and (b) an ml_option test with a framework_name different from the default subplugin name, asserting via ml_single_get_property (single, "framework", ...).
  5. [Medium] Android CI failed on all four ABIs (logs expired). _ml_convert_predefined_entity has an __ANDROID__-only branch and is now called twice (see #7). Please re-run after rebase and confirm green.
  6. [Low]_configure_handle (~1045) is a single assignment that can only return ML_ERROR_NONE, yet the caller (~1277-1280) has a dead goto error. Fold it into _configure_async_settings (as void) or inline both.
  7. [Low]_ml_convert_predefined_entity (info->models) is now called twice (~1010 and ~1178). Convert once in ml_single_open_custom and pass the result to both helpers.
  8. [Low] Unrelated churn — 25 of 30 hunks are type * var -> type *var reformatting in untouched functions, contradicting the repo-wide gst-indent convention (.github/workflows/static.check.scripts/indent.sh) and being the main cause of the conflict. The __ -> _ rename is also partial (__invoke, __process_output, __ml_validate_model_file untouched). Please drop the reformatting / rename from this PR.
  9. [Low] Doxygen — remove the "Single Responsibility Principle" editorializing in _configure_nnfw_tensors; document @param[out] determined_nnfw / @return in _validate_and_determine_nnfw; char *hw_name in _configure_filter_properties should be const char *.

No back-door or suspicious behavior found.

Sign up for freeto 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.

4 participants

@songgot@myungjoo@hj210@myungjoo-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[ml service] Refactor: Decompose ml_single_open_custom function for better maintainability - #651

Open
songgot wants to merge 1 commit into
nnstreamer:mainfrom
songgot:Repectoring_open_custom
Open

[ml service] Refactor: Decompose ml_single_open_custom function for better maintainability#651
songgot wants to merge 1 commit into
nnstreamer:mainfrom
songgot:Repectoring_open_custom

Conversation

@songgot

Copy link
Copy Markdown
Contributor

This commit refactors the monolithic ml_single_open_custom function (200+ lines) into smaller, focused helper functions to improve code readability and maintainability. The main function is now structured as a clear sequence of steps, each handled by a dedicated helper function.

@hj210hj210 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This patch refactors a 200+ line function into five smaller functions,
improving readability and maintainability. 👍

Additional recommendations:
a. Avoid the '__' prefix—it’s reserved for system/compiler use.
b. Split _configure_nnfw_tensors by NNFW types to adhere to SRP.

Comment threadc/src/ml-api-inference-single.c Outdated
Comment threadc/src/ml-api-inference-single.c Outdated
…etter maintainability
This commit refactors the monolithic ml_single_open_custom function (200+ lines)
into smaller, focused helper functions to improve code readability and
maintainability. The main function is now structured as a clear sequence
of steps, each handled by a dedicated helper function.
Signed-off-by: hyunil park <hyunil46.park@samsung.com>
@songgot
songgotforce-pushed the Repectoring_open_custom branch from 94e9e4e to 9ed6579CompareNovember 5, 2025 04:29
@myungjoo

Copy link
Copy Markdown
Member

From cursor. (both look legit)

  1. c/src/ml-api-inference-single.c:L1154

_configure_nnfw_tensors()가 info->nnfw만 참조합니다. 그런데 직전에 호출한 _validate_and_determine_nnfw()는 검증 결과를 info->nnfw에 다시 써주지 않아서, 사용자가 ML_NNFW_TYPE_ANY(또는 잘못된 값)을 넘기면 여기 switch가 default로 빠집니다. TensorFlow/NNFW 등에서는 in/out 텐서 메타데이터 설정이 필수인데 이 단계가 생략돼 런타임 오류가 발생합니다. 검증된 nnfw를 info->nnfw에 저장하거나 이 함수가 nnfw 인자를 직접 받아 사용하도록 조정이 필요해 보입니다.

  1. c/src/ml-api-inference-single.c:L1181

framework_name/framework 옵션을 통해 info->fw_name에 값을 넣을 수 있지만, _configure_filter_properties()에서는 항상 fw_name 인자(자동 감지 결과)만 사용합니다. 그래서 사용자가 명시적으로 프레임워크 문자열을 지정해도 더 이상 반영되지 않아 기존 기능이 회귀된 상태입니다. info->fw_name이 존재하면 이를 우선 사용하도록 복원해 주세요.

@songgot

Copy link
Copy Markdown
ContributorAuthor

From cursor. (both look legit)

  1. c/src/ml-api-inference-single.c:L1154

_configure_nnfw_tensors()가 info->nnfw만 참조합니다. 그런데 직전에 호출한 _validate_and_determine_nnfw()는 검증 결과를 info->nnfw에 다시 써주지 않아서, 사용자가 ML_NNFW_TYPE_ANY(또는 잘못된 값)을 넘기면 여기 switch가 default로 빠집니다. TensorFlow/NNFW 등에서는 in/out 텐서 메타데이터 설정이 필수인데 이 단계가 생략돼 런타임 오류가 발생합니다. 검증된 nnfw를 info->nnfw에 저장하거나 이 함수가 nnfw 인자를 직접 받아 사용하도록 조정이 필요해 보입니다.

  1. c/src/ml-api-inference-single.c:L1181

framework_name/framework 옵션을 통해 info->fw_name에 값을 넣을 수 있지만, _configure_filter_properties()에서는 항상 fw_name 인자(자동 감지 결과)만 사용합니다. 그래서 사용자가 명시적으로 프레임워크 문자열을 지정해도 더 이상 반영되지 않아 기존 기능이 회귀된 상태입니다. info->fw_name이 존재하면 이를 우선 사용하도록 복원해 주세요.

Thank you for your review.
I will properly revise this PR after completing some minor refactoring.

@myungjoo-botmyungjoo-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated review (transcribed from an AI review agent's report; please verify before acting).

Summary: The PR splits ml_single_open_custom() in c/src/ml-api-inference-single.c into eight static helpers, renames __setup_in_out_tensors -> _setup_in_out_tensors, and reformats type * var -> type *var across ~25 unrelated hunks. I traced every path of the original (validation order, goto error -> ml_single_close, list_models / hw_name / converted_models release, *single = NULL init, return codes, NNTR_INF block) against the decomposed version. Cleanup order and resource handling are preserved, but two data-flow changes alter behavior, and the PR conflicts with current main.

  1. [High] Resolved nnfw type not used for tensors setup_configure_nnfw_tensors (PR ~line 1154, called at ~1283) switches on info->nnfw (user value) instead of the local nnfw rewritten by _ml_validate_model_file() for ML_NNFW_TYPE_ANY. With ML_NNFW_TYPE_ANY and a .pb / .pt model the switch hits default, the mandatory in/out info push is skipped, and open fails with ML_ERROR_STREAMS_PIPE even when valid infos were supplied (previously: success, or a clear ML_ERROR_INVALID_PARAMETER). ml_single_create_handle (nnfw) and fw_name do use the resolved value, so handle and tensors config disagree. Fix: pass nnfw as a parameter and switch (nnfw) (do not write back into caller-owned info).
  2. [High] framework_name option silently ignored_configure_filter_properties (~1181): the original used info->fw_name when set, else _ml_get_nnfw_subplugin_name (nnfw). The PR always passes the auto-derived name, so the explicit framework_name from ml_single_open_with_option() (still populated at ~1412-1414) is dead. Public-API regression. Fix: const char *framework = info->fw_name ? info->fw_name : fw_name; in g_object_set.
  3. [High] Merge conflict with maingit merge-tree reports a content conflict in this file. Since the PR base, main added "latency", info->latency_mode to the g_object_set call (507be1d; would be dropped by a naive rebase into _configure_filter_properties), rewrote __setup_in_out_tensors (d53de78), and changed ml_single_close (472be75). Also, your own open #661 replaces exactly the _configure_general/armnn/nnfw_tensors region; the two PRs conflict with each other and should be rebased into one series.
  4. [Medium] Existing tests cannot catch #1 or #2 — the ML_NNFW_TYPE_ANY tests use tflite (falls into default in both versions), and the only framework_name test sets the name equal to the auto-detected one. Please add (a) under ENABLE_TENSORFLOW, ml_single_open of the .pb model with ML_NNFW_TYPE_ANY + in/out infos expecting ML_ERROR_NONE, and (b) an ml_option test with a framework_name different from the default subplugin name, asserting via ml_single_get_property (single, "framework", ...).
  5. [Medium] Android CI failed on all four ABIs (logs expired). _ml_convert_predefined_entity has an __ANDROID__-only branch and is now called twice (see #7). Please re-run after rebase and confirm green.
  6. [Low]_configure_handle (~1045) is a single assignment that can only return ML_ERROR_NONE, yet the caller (~1277-1280) has a dead goto error. Fold it into _configure_async_settings (as void) or inline both.
  7. [Low]_ml_convert_predefined_entity (info->models) is now called twice (~1010 and ~1178). Convert once in ml_single_open_custom and pass the result to both helpers.
  8. [Low] Unrelated churn — 25 of 30 hunks are type * var -> type *var reformatting in untouched functions, contradicting the repo-wide gst-indent convention (.github/workflows/static.check.scripts/indent.sh) and being the main cause of the conflict. The __ -> _ rename is also partial (__invoke, __process_output, __ml_validate_model_file untouched). Please drop the reformatting / rename from this PR.
  9. [Low] Doxygen — remove the "Single Responsibility Principle" editorializing in _configure_nnfw_tensors; document @param[out] determined_nnfw / @return in _validate_and_determine_nnfw; char *hw_name in _configure_filter_properties should be const char *.

No back-door or suspicious behavior found.

Sign up for freeto 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.

4 participants

@songgot@myungjoo@hj210@myungjoo-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

[ml service] Refactor: Decompose ml_single_open_custom function for better maintainability - #651

Open
songgot wants to merge 1 commit into
nnstreamer:mainfrom
songgot:Repectoring_open_custom
Open

[ml service] Refactor: Decompose ml_single_open_custom function for better maintainability#651
songgot wants to merge 1 commit into
nnstreamer:mainfrom
songgot:Repectoring_open_custom

Conversation

@songgot

Copy link
Copy Markdown
Contributor

This commit refactors the monolithic ml_single_open_custom function (200+ lines) into smaller, focused helper functions to improve code readability and maintainability. The main function is now structured as a clear sequence of steps, each handled by a dedicated helper function.

@hj210hj210 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This patch refactors a 200+ line function into five smaller functions,
improving readability and maintainability. 👍

Additional recommendations:
a. Avoid the '__' prefix—it’s reserved for system/compiler use.
b. Split _configure_nnfw_tensors by NNFW types to adhere to SRP.

Comment threadc/src/ml-api-inference-single.c Outdated
Comment threadc/src/ml-api-inference-single.c Outdated
…etter maintainability
This commit refactors the monolithic ml_single_open_custom function (200+ lines)
into smaller, focused helper functions to improve code readability and
maintainability. The main function is now structured as a clear sequence
of steps, each handled by a dedicated helper function.
Signed-off-by: hyunil park <hyunil46.park@samsung.com>
@songgot
songgotforce-pushed the Repectoring_open_custom branch from 94e9e4e to 9ed6579CompareNovember 5, 2025 04:29
@myungjoo

Copy link
Copy Markdown
Member

From cursor. (both look legit)

  1. c/src/ml-api-inference-single.c:L1154

_configure_nnfw_tensors()가 info->nnfw만 참조합니다. 그런데 직전에 호출한 _validate_and_determine_nnfw()는 검증 결과를 info->nnfw에 다시 써주지 않아서, 사용자가 ML_NNFW_TYPE_ANY(또는 잘못된 값)을 넘기면 여기 switch가 default로 빠집니다. TensorFlow/NNFW 등에서는 in/out 텐서 메타데이터 설정이 필수인데 이 단계가 생략돼 런타임 오류가 발생합니다. 검증된 nnfw를 info->nnfw에 저장하거나 이 함수가 nnfw 인자를 직접 받아 사용하도록 조정이 필요해 보입니다.

  1. c/src/ml-api-inference-single.c:L1181

framework_name/framework 옵션을 통해 info->fw_name에 값을 넣을 수 있지만, _configure_filter_properties()에서는 항상 fw_name 인자(자동 감지 결과)만 사용합니다. 그래서 사용자가 명시적으로 프레임워크 문자열을 지정해도 더 이상 반영되지 않아 기존 기능이 회귀된 상태입니다. info->fw_name이 존재하면 이를 우선 사용하도록 복원해 주세요.

@songgot

Copy link
Copy Markdown
ContributorAuthor

From cursor. (both look legit)

  1. c/src/ml-api-inference-single.c:L1154

_configure_nnfw_tensors()가 info->nnfw만 참조합니다. 그런데 직전에 호출한 _validate_and_determine_nnfw()는 검증 결과를 info->nnfw에 다시 써주지 않아서, 사용자가 ML_NNFW_TYPE_ANY(또는 잘못된 값)을 넘기면 여기 switch가 default로 빠집니다. TensorFlow/NNFW 등에서는 in/out 텐서 메타데이터 설정이 필수인데 이 단계가 생략돼 런타임 오류가 발생합니다. 검증된 nnfw를 info->nnfw에 저장하거나 이 함수가 nnfw 인자를 직접 받아 사용하도록 조정이 필요해 보입니다.

  1. c/src/ml-api-inference-single.c:L1181

framework_name/framework 옵션을 통해 info->fw_name에 값을 넣을 수 있지만, _configure_filter_properties()에서는 항상 fw_name 인자(자동 감지 결과)만 사용합니다. 그래서 사용자가 명시적으로 프레임워크 문자열을 지정해도 더 이상 반영되지 않아 기존 기능이 회귀된 상태입니다. info->fw_name이 존재하면 이를 우선 사용하도록 복원해 주세요.

Thank you for your review.
I will properly revise this PR after completing some minor refactoring.

@myungjoo-botmyungjoo-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated review (transcribed from an AI review agent's report; please verify before acting).

Summary: The PR splits ml_single_open_custom() in c/src/ml-api-inference-single.c into eight static helpers, renames __setup_in_out_tensors -> _setup_in_out_tensors, and reformats type * var -> type *var across ~25 unrelated hunks. I traced every path of the original (validation order, goto error -> ml_single_close, list_models / hw_name / converted_models release, *single = NULL init, return codes, NNTR_INF block) against the decomposed version. Cleanup order and resource handling are preserved, but two data-flow changes alter behavior, and the PR conflicts with current main.

  1. [High] Resolved nnfw type not used for tensors setup_configure_nnfw_tensors (PR ~line 1154, called at ~1283) switches on info->nnfw (user value) instead of the local nnfw rewritten by _ml_validate_model_file() for ML_NNFW_TYPE_ANY. With ML_NNFW_TYPE_ANY and a .pb / .pt model the switch hits default, the mandatory in/out info push is skipped, and open fails with ML_ERROR_STREAMS_PIPE even when valid infos were supplied (previously: success, or a clear ML_ERROR_INVALID_PARAMETER). ml_single_create_handle (nnfw) and fw_name do use the resolved value, so handle and tensors config disagree. Fix: pass nnfw as a parameter and switch (nnfw) (do not write back into caller-owned info).
  2. [High] framework_name option silently ignored_configure_filter_properties (~1181): the original used info->fw_name when set, else _ml_get_nnfw_subplugin_name (nnfw). The PR always passes the auto-derived name, so the explicit framework_name from ml_single_open_with_option() (still populated at ~1412-1414) is dead. Public-API regression. Fix: const char *framework = info->fw_name ? info->fw_name : fw_name; in g_object_set.
  3. [High] Merge conflict with maingit merge-tree reports a content conflict in this file. Since the PR base, main added "latency", info->latency_mode to the g_object_set call (507be1d; would be dropped by a naive rebase into _configure_filter_properties), rewrote __setup_in_out_tensors (d53de78), and changed ml_single_close (472be75). Also, your own open #661 replaces exactly the _configure_general/armnn/nnfw_tensors region; the two PRs conflict with each other and should be rebased into one series.
  4. [Medium] Existing tests cannot catch #1 or #2 — the ML_NNFW_TYPE_ANY tests use tflite (falls into default in both versions), and the only framework_name test sets the name equal to the auto-detected one. Please add (a) under ENABLE_TENSORFLOW, ml_single_open of the .pb model with ML_NNFW_TYPE_ANY + in/out infos expecting ML_ERROR_NONE, and (b) an ml_option test with a framework_name different from the default subplugin name, asserting via ml_single_get_property (single, "framework", ...).
  5. [Medium] Android CI failed on all four ABIs (logs expired). _ml_convert_predefined_entity has an __ANDROID__-only branch and is now called twice (see #7). Please re-run after rebase and confirm green.
  6. [Low]_configure_handle (~1045) is a single assignment that can only return ML_ERROR_NONE, yet the caller (~1277-1280) has a dead goto error. Fold it into _configure_async_settings (as void) or inline both.
  7. [Low]_ml_convert_predefined_entity (info->models) is now called twice (~1010 and ~1178). Convert once in ml_single_open_custom and pass the result to both helpers.
  8. [Low] Unrelated churn — 25 of 30 hunks are type * var -> type *var reformatting in untouched functions, contradicting the repo-wide gst-indent convention (.github/workflows/static.check.scripts/indent.sh) and being the main cause of the conflict. The __ -> _ rename is also partial (__invoke, __process_output, __ml_validate_model_file untouched). Please drop the reformatting / rename from this PR.
  9. [Low] Doxygen — remove the "Single Responsibility Principle" editorializing in _configure_nnfw_tensors; document @param[out] determined_nnfw / @return in _validate_and_determine_nnfw; char *hw_name in _configure_filter_properties should be const char *.

No back-door or suspicious behavior found.

Sign up for freeto 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.

4 participants

@songgot@myungjoo@hj210@myungjoo-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[ml service] Refactor: Decompose ml_single_open_custom function for better maintainability - #651

Open
songgot wants to merge 1 commit into
nnstreamer:mainfrom
songgot:Repectoring_open_custom
Open

[ml service] Refactor: Decompose ml_single_open_custom function for better maintainability#651
songgot wants to merge 1 commit into
nnstreamer:mainfrom
songgot:Repectoring_open_custom

Conversation

@songgot

Copy link
Copy Markdown
Contributor

This commit refactors the monolithic ml_single_open_custom function (200+ lines) into smaller, focused helper functions to improve code readability and maintainability. The main function is now structured as a clear sequence of steps, each handled by a dedicated helper function.

@hj210hj210 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This patch refactors a 200+ line function into five smaller functions,
improving readability and maintainability. 👍

Additional recommendations:
a. Avoid the '__' prefix—it’s reserved for system/compiler use.
b. Split _configure_nnfw_tensors by NNFW types to adhere to SRP.

Comment threadc/src/ml-api-inference-single.c Outdated
Comment threadc/src/ml-api-inference-single.c Outdated
…etter maintainability
This commit refactors the monolithic ml_single_open_custom function (200+ lines)
into smaller, focused helper functions to improve code readability and
maintainability. The main function is now structured as a clear sequence
of steps, each handled by a dedicated helper function.
Signed-off-by: hyunil park <hyunil46.park@samsung.com>
@songgot
songgotforce-pushed the Repectoring_open_custom branch from 94e9e4e to 9ed6579CompareNovember 5, 2025 04:29
@myungjoo

Copy link
Copy Markdown
Member

From cursor. (both look legit)

  1. c/src/ml-api-inference-single.c:L1154

_configure_nnfw_tensors()가 info->nnfw만 참조합니다. 그런데 직전에 호출한 _validate_and_determine_nnfw()는 검증 결과를 info->nnfw에 다시 써주지 않아서, 사용자가 ML_NNFW_TYPE_ANY(또는 잘못된 값)을 넘기면 여기 switch가 default로 빠집니다. TensorFlow/NNFW 등에서는 in/out 텐서 메타데이터 설정이 필수인데 이 단계가 생략돼 런타임 오류가 발생합니다. 검증된 nnfw를 info->nnfw에 저장하거나 이 함수가 nnfw 인자를 직접 받아 사용하도록 조정이 필요해 보입니다.

  1. c/src/ml-api-inference-single.c:L1181

framework_name/framework 옵션을 통해 info->fw_name에 값을 넣을 수 있지만, _configure_filter_properties()에서는 항상 fw_name 인자(자동 감지 결과)만 사용합니다. 그래서 사용자가 명시적으로 프레임워크 문자열을 지정해도 더 이상 반영되지 않아 기존 기능이 회귀된 상태입니다. info->fw_name이 존재하면 이를 우선 사용하도록 복원해 주세요.

@songgot

Copy link
Copy Markdown
ContributorAuthor

From cursor. (both look legit)

  1. c/src/ml-api-inference-single.c:L1154

_configure_nnfw_tensors()가 info->nnfw만 참조합니다. 그런데 직전에 호출한 _validate_and_determine_nnfw()는 검증 결과를 info->nnfw에 다시 써주지 않아서, 사용자가 ML_NNFW_TYPE_ANY(또는 잘못된 값)을 넘기면 여기 switch가 default로 빠집니다. TensorFlow/NNFW 등에서는 in/out 텐서 메타데이터 설정이 필수인데 이 단계가 생략돼 런타임 오류가 발생합니다. 검증된 nnfw를 info->nnfw에 저장하거나 이 함수가 nnfw 인자를 직접 받아 사용하도록 조정이 필요해 보입니다.

  1. c/src/ml-api-inference-single.c:L1181

framework_name/framework 옵션을 통해 info->fw_name에 값을 넣을 수 있지만, _configure_filter_properties()에서는 항상 fw_name 인자(자동 감지 결과)만 사용합니다. 그래서 사용자가 명시적으로 프레임워크 문자열을 지정해도 더 이상 반영되지 않아 기존 기능이 회귀된 상태입니다. info->fw_name이 존재하면 이를 우선 사용하도록 복원해 주세요.

Thank you for your review.
I will properly revise this PR after completing some minor refactoring.

@myungjoo-botmyungjoo-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated review (transcribed from an AI review agent's report; please verify before acting).

Summary: The PR splits ml_single_open_custom() in c/src/ml-api-inference-single.c into eight static helpers, renames __setup_in_out_tensors -> _setup_in_out_tensors, and reformats type * var -> type *var across ~25 unrelated hunks. I traced every path of the original (validation order, goto error -> ml_single_close, list_models / hw_name / converted_models release, *single = NULL init, return codes, NNTR_INF block) against the decomposed version. Cleanup order and resource handling are preserved, but two data-flow changes alter behavior, and the PR conflicts with current main.

  1. [High] Resolved nnfw type not used for tensors setup_configure_nnfw_tensors (PR ~line 1154, called at ~1283) switches on info->nnfw (user value) instead of the local nnfw rewritten by _ml_validate_model_file() for ML_NNFW_TYPE_ANY. With ML_NNFW_TYPE_ANY and a .pb / .pt model the switch hits default, the mandatory in/out info push is skipped, and open fails with ML_ERROR_STREAMS_PIPE even when valid infos were supplied (previously: success, or a clear ML_ERROR_INVALID_PARAMETER). ml_single_create_handle (nnfw) and fw_name do use the resolved value, so handle and tensors config disagree. Fix: pass nnfw as a parameter and switch (nnfw) (do not write back into caller-owned info).
  2. [High] framework_name option silently ignored_configure_filter_properties (~1181): the original used info->fw_name when set, else _ml_get_nnfw_subplugin_name (nnfw). The PR always passes the auto-derived name, so the explicit framework_name from ml_single_open_with_option() (still populated at ~1412-1414) is dead. Public-API regression. Fix: const char *framework = info->fw_name ? info->fw_name : fw_name; in g_object_set.
  3. [High] Merge conflict with maingit merge-tree reports a content conflict in this file. Since the PR base, main added "latency", info->latency_mode to the g_object_set call (507be1d; would be dropped by a naive rebase into _configure_filter_properties), rewrote __setup_in_out_tensors (d53de78), and changed ml_single_close (472be75). Also, your own open #661 replaces exactly the _configure_general/armnn/nnfw_tensors region; the two PRs conflict with each other and should be rebased into one series.
  4. [Medium] Existing tests cannot catch #1 or #2 — the ML_NNFW_TYPE_ANY tests use tflite (falls into default in both versions), and the only framework_name test sets the name equal to the auto-detected one. Please add (a) under ENABLE_TENSORFLOW, ml_single_open of the .pb model with ML_NNFW_TYPE_ANY + in/out infos expecting ML_ERROR_NONE, and (b) an ml_option test with a framework_name different from the default subplugin name, asserting via ml_single_get_property (single, "framework", ...).
  5. [Medium] Android CI failed on all four ABIs (logs expired). _ml_convert_predefined_entity has an __ANDROID__-only branch and is now called twice (see #7). Please re-run after rebase and confirm green.
  6. [Low]_configure_handle (~1045) is a single assignment that can only return ML_ERROR_NONE, yet the caller (~1277-1280) has a dead goto error. Fold it into _configure_async_settings (as void) or inline both.
  7. [Low]_ml_convert_predefined_entity (info->models) is now called twice (~1010 and ~1178). Convert once in ml_single_open_custom and pass the result to both helpers.
  8. [Low] Unrelated churn — 25 of 30 hunks are type * var -> type *var reformatting in untouched functions, contradicting the repo-wide gst-indent convention (.github/workflows/static.check.scripts/indent.sh) and being the main cause of the conflict. The __ -> _ rename is also partial (__invoke, __process_output, __ml_validate_model_file untouched). Please drop the reformatting / rename from this PR.
  9. [Low] Doxygen — remove the "Single Responsibility Principle" editorializing in _configure_nnfw_tensors; document @param[out] determined_nnfw / @return in _validate_and_determine_nnfw; char *hw_name in _configure_filter_properties should be const char *.

No back-door or suspicious behavior found.

Sign up for freeto 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.

4 participants

@songgot@myungjoo@hj210@myungjoo-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[ml service] Refactor: Decompose ml_single_open_custom function for better maintainability - #651

Open
songgot wants to merge 1 commit into
nnstreamer:mainfrom
songgot:Repectoring_open_custom
Open

[ml service] Refactor: Decompose ml_single_open_custom function for better maintainability#651
songgot wants to merge 1 commit into
nnstreamer:mainfrom
songgot:Repectoring_open_custom

Conversation

@songgot

Copy link
Copy Markdown
Contributor

This commit refactors the monolithic ml_single_open_custom function (200+ lines) into smaller, focused helper functions to improve code readability and maintainability. The main function is now structured as a clear sequence of steps, each handled by a dedicated helper function.

@hj210hj210 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This patch refactors a 200+ line function into five smaller functions,
improving readability and maintainability. 👍

Additional recommendations:
a. Avoid the '__' prefix—it’s reserved for system/compiler use.
b. Split _configure_nnfw_tensors by NNFW types to adhere to SRP.

Comment threadc/src/ml-api-inference-single.c Outdated
Comment threadc/src/ml-api-inference-single.c Outdated
…etter maintainability
This commit refactors the monolithic ml_single_open_custom function (200+ lines)
into smaller, focused helper functions to improve code readability and
maintainability. The main function is now structured as a clear sequence
of steps, each handled by a dedicated helper function.
Signed-off-by: hyunil park <hyunil46.park@samsung.com>
@songgot
songgotforce-pushed the Repectoring_open_custom branch from 94e9e4e to 9ed6579CompareNovember 5, 2025 04:29
@myungjoo

Copy link
Copy Markdown
Member

From cursor. (both look legit)

  1. c/src/ml-api-inference-single.c:L1154

_configure_nnfw_tensors()가 info->nnfw만 참조합니다. 그런데 직전에 호출한 _validate_and_determine_nnfw()는 검증 결과를 info->nnfw에 다시 써주지 않아서, 사용자가 ML_NNFW_TYPE_ANY(또는 잘못된 값)을 넘기면 여기 switch가 default로 빠집니다. TensorFlow/NNFW 등에서는 in/out 텐서 메타데이터 설정이 필수인데 이 단계가 생략돼 런타임 오류가 발생합니다. 검증된 nnfw를 info->nnfw에 저장하거나 이 함수가 nnfw 인자를 직접 받아 사용하도록 조정이 필요해 보입니다.

  1. c/src/ml-api-inference-single.c:L1181

framework_name/framework 옵션을 통해 info->fw_name에 값을 넣을 수 있지만, _configure_filter_properties()에서는 항상 fw_name 인자(자동 감지 결과)만 사용합니다. 그래서 사용자가 명시적으로 프레임워크 문자열을 지정해도 더 이상 반영되지 않아 기존 기능이 회귀된 상태입니다. info->fw_name이 존재하면 이를 우선 사용하도록 복원해 주세요.

@songgot

Copy link
Copy Markdown
ContributorAuthor

From cursor. (both look legit)

  1. c/src/ml-api-inference-single.c:L1154

_configure_nnfw_tensors()가 info->nnfw만 참조합니다. 그런데 직전에 호출한 _validate_and_determine_nnfw()는 검증 결과를 info->nnfw에 다시 써주지 않아서, 사용자가 ML_NNFW_TYPE_ANY(또는 잘못된 값)을 넘기면 여기 switch가 default로 빠집니다. TensorFlow/NNFW 등에서는 in/out 텐서 메타데이터 설정이 필수인데 이 단계가 생략돼 런타임 오류가 발생합니다. 검증된 nnfw를 info->nnfw에 저장하거나 이 함수가 nnfw 인자를 직접 받아 사용하도록 조정이 필요해 보입니다.

  1. c/src/ml-api-inference-single.c:L1181

framework_name/framework 옵션을 통해 info->fw_name에 값을 넣을 수 있지만, _configure_filter_properties()에서는 항상 fw_name 인자(자동 감지 결과)만 사용합니다. 그래서 사용자가 명시적으로 프레임워크 문자열을 지정해도 더 이상 반영되지 않아 기존 기능이 회귀된 상태입니다. info->fw_name이 존재하면 이를 우선 사용하도록 복원해 주세요.

Thank you for your review.
I will properly revise this PR after completing some minor refactoring.

@myungjoo-botmyungjoo-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated review (transcribed from an AI review agent's report; please verify before acting).

Summary: The PR splits ml_single_open_custom() in c/src/ml-api-inference-single.c into eight static helpers, renames __setup_in_out_tensors -> _setup_in_out_tensors, and reformats type * var -> type *var across ~25 unrelated hunks. I traced every path of the original (validation order, goto error -> ml_single_close, list_models / hw_name / converted_models release, *single = NULL init, return codes, NNTR_INF block) against the decomposed version. Cleanup order and resource handling are preserved, but two data-flow changes alter behavior, and the PR conflicts with current main.

  1. [High] Resolved nnfw type not used for tensors setup_configure_nnfw_tensors (PR ~line 1154, called at ~1283) switches on info->nnfw (user value) instead of the local nnfw rewritten by _ml_validate_model_file() for ML_NNFW_TYPE_ANY. With ML_NNFW_TYPE_ANY and a .pb / .pt model the switch hits default, the mandatory in/out info push is skipped, and open fails with ML_ERROR_STREAMS_PIPE even when valid infos were supplied (previously: success, or a clear ML_ERROR_INVALID_PARAMETER). ml_single_create_handle (nnfw) and fw_name do use the resolved value, so handle and tensors config disagree. Fix: pass nnfw as a parameter and switch (nnfw) (do not write back into caller-owned info).
  2. [High] framework_name option silently ignored_configure_filter_properties (~1181): the original used info->fw_name when set, else _ml_get_nnfw_subplugin_name (nnfw). The PR always passes the auto-derived name, so the explicit framework_name from ml_single_open_with_option() (still populated at ~1412-1414) is dead. Public-API regression. Fix: const char *framework = info->fw_name ? info->fw_name : fw_name; in g_object_set.
  3. [High] Merge conflict with maingit merge-tree reports a content conflict in this file. Since the PR base, main added "latency", info->latency_mode to the g_object_set call (507be1d; would be dropped by a naive rebase into _configure_filter_properties), rewrote __setup_in_out_tensors (d53de78), and changed ml_single_close (472be75). Also, your own open #661 replaces exactly the _configure_general/armnn/nnfw_tensors region; the two PRs conflict with each other and should be rebased into one series.
  4. [Medium] Existing tests cannot catch #1 or #2 — the ML_NNFW_TYPE_ANY tests use tflite (falls into default in both versions), and the only framework_name test sets the name equal to the auto-detected one. Please add (a) under ENABLE_TENSORFLOW, ml_single_open of the .pb model with ML_NNFW_TYPE_ANY + in/out infos expecting ML_ERROR_NONE, and (b) an ml_option test with a framework_name different from the default subplugin name, asserting via ml_single_get_property (single, "framework", ...).
  5. [Medium] Android CI failed on all four ABIs (logs expired). _ml_convert_predefined_entity has an __ANDROID__-only branch and is now called twice (see #7). Please re-run after rebase and confirm green.
  6. [Low]_configure_handle (~1045) is a single assignment that can only return ML_ERROR_NONE, yet the caller (~1277-1280) has a dead goto error. Fold it into _configure_async_settings (as void) or inline both.
  7. [Low]_ml_convert_predefined_entity (info->models) is now called twice (~1010 and ~1178). Convert once in ml_single_open_custom and pass the result to both helpers.
  8. [Low] Unrelated churn — 25 of 30 hunks are type * var -> type *var reformatting in untouched functions, contradicting the repo-wide gst-indent convention (.github/workflows/static.check.scripts/indent.sh) and being the main cause of the conflict. The __ -> _ rename is also partial (__invoke, __process_output, __ml_validate_model_file untouched). Please drop the reformatting / rename from this PR.
  9. [Low] Doxygen — remove the "Single Responsibility Principle" editorializing in _configure_nnfw_tensors; document @param[out] determined_nnfw / @return in _validate_and_determine_nnfw; char *hw_name in _configure_filter_properties should be const char *.

No back-door or suspicious behavior found.

Sign up for freeto 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.

4 participants

@songgot@myungjoo@hj210@myungjoo-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

[ml service] Refactor: Decompose ml_single_open_custom function for better maintainability - #651

Open
songgot wants to merge 1 commit into
nnstreamer:mainfrom
songgot:Repectoring_open_custom
Open

[ml service] Refactor: Decompose ml_single_open_custom function for better maintainability#651
songgot wants to merge 1 commit into
nnstreamer:mainfrom
songgot:Repectoring_open_custom

Conversation

@songgot

Copy link
Copy Markdown
Contributor

This commit refactors the monolithic ml_single_open_custom function (200+ lines) into smaller, focused helper functions to improve code readability and maintainability. The main function is now structured as a clear sequence of steps, each handled by a dedicated helper function.

@hj210hj210 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This patch refactors a 200+ line function into five smaller functions,
improving readability and maintainability. 👍

Additional recommendations:
a. Avoid the '__' prefix—it’s reserved for system/compiler use.
b. Split _configure_nnfw_tensors by NNFW types to adhere to SRP.

Comment threadc/src/ml-api-inference-single.c Outdated
Comment threadc/src/ml-api-inference-single.c Outdated
…etter maintainability
This commit refactors the monolithic ml_single_open_custom function (200+ lines)
into smaller, focused helper functions to improve code readability and
maintainability. The main function is now structured as a clear sequence
of steps, each handled by a dedicated helper function.
Signed-off-by: hyunil park <hyunil46.park@samsung.com>
@songgot
songgotforce-pushed the Repectoring_open_custom branch from 94e9e4e to 9ed6579CompareNovember 5, 2025 04:29
@myungjoo

Copy link
Copy Markdown
Member

From cursor. (both look legit)

  1. c/src/ml-api-inference-single.c:L1154

_configure_nnfw_tensors()가 info->nnfw만 참조합니다. 그런데 직전에 호출한 _validate_and_determine_nnfw()는 검증 결과를 info->nnfw에 다시 써주지 않아서, 사용자가 ML_NNFW_TYPE_ANY(또는 잘못된 값)을 넘기면 여기 switch가 default로 빠집니다. TensorFlow/NNFW 등에서는 in/out 텐서 메타데이터 설정이 필수인데 이 단계가 생략돼 런타임 오류가 발생합니다. 검증된 nnfw를 info->nnfw에 저장하거나 이 함수가 nnfw 인자를 직접 받아 사용하도록 조정이 필요해 보입니다.

  1. c/src/ml-api-inference-single.c:L1181

framework_name/framework 옵션을 통해 info->fw_name에 값을 넣을 수 있지만, _configure_filter_properties()에서는 항상 fw_name 인자(자동 감지 결과)만 사용합니다. 그래서 사용자가 명시적으로 프레임워크 문자열을 지정해도 더 이상 반영되지 않아 기존 기능이 회귀된 상태입니다. info->fw_name이 존재하면 이를 우선 사용하도록 복원해 주세요.

@songgot

Copy link
Copy Markdown
ContributorAuthor

From cursor. (both look legit)

  1. c/src/ml-api-inference-single.c:L1154

_configure_nnfw_tensors()가 info->nnfw만 참조합니다. 그런데 직전에 호출한 _validate_and_determine_nnfw()는 검증 결과를 info->nnfw에 다시 써주지 않아서, 사용자가 ML_NNFW_TYPE_ANY(또는 잘못된 값)을 넘기면 여기 switch가 default로 빠집니다. TensorFlow/NNFW 등에서는 in/out 텐서 메타데이터 설정이 필수인데 이 단계가 생략돼 런타임 오류가 발생합니다. 검증된 nnfw를 info->nnfw에 저장하거나 이 함수가 nnfw 인자를 직접 받아 사용하도록 조정이 필요해 보입니다.

  1. c/src/ml-api-inference-single.c:L1181

framework_name/framework 옵션을 통해 info->fw_name에 값을 넣을 수 있지만, _configure_filter_properties()에서는 항상 fw_name 인자(자동 감지 결과)만 사용합니다. 그래서 사용자가 명시적으로 프레임워크 문자열을 지정해도 더 이상 반영되지 않아 기존 기능이 회귀된 상태입니다. info->fw_name이 존재하면 이를 우선 사용하도록 복원해 주세요.

Thank you for your review.
I will properly revise this PR after completing some minor refactoring.

@myungjoo-botmyungjoo-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated review (transcribed from an AI review agent's report; please verify before acting).

Summary: The PR splits ml_single_open_custom() in c/src/ml-api-inference-single.c into eight static helpers, renames __setup_in_out_tensors -> _setup_in_out_tensors, and reformats type * var -> type *var across ~25 unrelated hunks. I traced every path of the original (validation order, goto error -> ml_single_close, list_models / hw_name / converted_models release, *single = NULL init, return codes, NNTR_INF block) against the decomposed version. Cleanup order and resource handling are preserved, but two data-flow changes alter behavior, and the PR conflicts with current main.

  1. [High] Resolved nnfw type not used for tensors setup_configure_nnfw_tensors (PR ~line 1154, called at ~1283) switches on info->nnfw (user value) instead of the local nnfw rewritten by _ml_validate_model_file() for ML_NNFW_TYPE_ANY. With ML_NNFW_TYPE_ANY and a .pb / .pt model the switch hits default, the mandatory in/out info push is skipped, and open fails with ML_ERROR_STREAMS_PIPE even when valid infos were supplied (previously: success, or a clear ML_ERROR_INVALID_PARAMETER). ml_single_create_handle (nnfw) and fw_name do use the resolved value, so handle and tensors config disagree. Fix: pass nnfw as a parameter and switch (nnfw) (do not write back into caller-owned info).
  2. [High] framework_name option silently ignored_configure_filter_properties (~1181): the original used info->fw_name when set, else _ml_get_nnfw_subplugin_name (nnfw). The PR always passes the auto-derived name, so the explicit framework_name from ml_single_open_with_option() (still populated at ~1412-1414) is dead. Public-API regression. Fix: const char *framework = info->fw_name ? info->fw_name : fw_name; in g_object_set.
  3. [High] Merge conflict with maingit merge-tree reports a content conflict in this file. Since the PR base, main added "latency", info->latency_mode to the g_object_set call (507be1d; would be dropped by a naive rebase into _configure_filter_properties), rewrote __setup_in_out_tensors (d53de78), and changed ml_single_close (472be75). Also, your own open #661 replaces exactly the _configure_general/armnn/nnfw_tensors region; the two PRs conflict with each other and should be rebased into one series.
  4. [Medium] Existing tests cannot catch #1 or #2 — the ML_NNFW_TYPE_ANY tests use tflite (falls into default in both versions), and the only framework_name test sets the name equal to the auto-detected one. Please add (a) under ENABLE_TENSORFLOW, ml_single_open of the .pb model with ML_NNFW_TYPE_ANY + in/out infos expecting ML_ERROR_NONE, and (b) an ml_option test with a framework_name different from the default subplugin name, asserting via ml_single_get_property (single, "framework", ...).
  5. [Medium] Android CI failed on all four ABIs (logs expired). _ml_convert_predefined_entity has an __ANDROID__-only branch and is now called twice (see #7). Please re-run after rebase and confirm green.
  6. [Low]_configure_handle (~1045) is a single assignment that can only return ML_ERROR_NONE, yet the caller (~1277-1280) has a dead goto error. Fold it into _configure_async_settings (as void) or inline both.
  7. [Low]_ml_convert_predefined_entity (info->models) is now called twice (~1010 and ~1178). Convert once in ml_single_open_custom and pass the result to both helpers.
  8. [Low] Unrelated churn — 25 of 30 hunks are type * var -> type *var reformatting in untouched functions, contradicting the repo-wide gst-indent convention (.github/workflows/static.check.scripts/indent.sh) and being the main cause of the conflict. The __ -> _ rename is also partial (__invoke, __process_output, __ml_validate_model_file untouched). Please drop the reformatting / rename from this PR.
  9. [Low] Doxygen — remove the "Single Responsibility Principle" editorializing in _configure_nnfw_tensors; document @param[out] determined_nnfw / @return in _validate_and_determine_nnfw; char *hw_name in _configure_filter_properties should be const char *.

No back-door or suspicious behavior found.

Sign up for freeto 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.

4 participants

@songgot@myungjoo@hj210@myungjoo-bot