[MM] Change to the common resource manager by again4you · Pull Request #567 · nnstreamer/api

again4you · 2024-10-11T01:23:35Z

This patch updates the mm resource manager.

Origin: https://review.tizen.org/gerrit/#/c/platform/core/api/machine-learning/+/318887/

Change-Id: I108b500c796a8899e6d582346a184d9e6a8136c4

Signed-off-by: YoungHun Kim yh8004.kim@samsung.com
Signed-off-by: Sangjung Woo sangjung.woo@samsung.com

This patch updates the mm resource manager. - Origin: https://review.tizen.org/gerrit/#/c/platform/core/api/machine-learning/+/318887/ Change-Id: I108b500c796a8899e6d582346a184d9e6a8136c4 Signed-off-by: YoungHun Kim <yh8004.kim@samsung.com> Signed-off-by: Sangjung Woo <sangjung.woo@samsung.com>

taos-ci · 2024-10-11T01:23:38Z

📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #567. Please a submit 1commit/1PR (one commit per one PR) policy to get comments quickly from reviewers. Your PR must pass all verificiation processes of cibot before starting a review process from reviewers. If you are new member to join this project, please read manuals in documentation folder and wiki page. In order to monitor a progress status of your PR in more detail, visit http://ci.nnstreamer.ai/.

again4you · 2024-10-11T01:36:28Z

 # 3. Meson   : ./meson.build
 # 4. Android : ./java/android/nnstreamer/src/main/jni/Android.mk
-Version:	1.8.6
+Version:	1.1.0


The rpm version is changed! why?

again4you · 2024-10-11T01:37:11Z

-    }
+  status = ml_tizen_mm_res_allocate(p->resources, res_type);
+  if (status != ML_ERROR_NONE) {
+    _ml_loge("Faied to allocate resource.");


Faied -> Failed

again4you · 2024-10-11T01:37:43Z

+  for (type = RES_TYPE_VIDEO_DECODER; type < RES_TYPE_MAX; type++) {
+    ret = ml_tizen_mm_res_deallocate(mm_handle, type);
+    if (ret != ML_ERROR_NONE) {
+      _ml_loge("Fail to deallocate resoure [type %d].", type);


resoure -> resource

gichan-jang · 2024-10-11T02:10:53Z

@@ -23,7 +23,7 @@ if (get_option('enable-tizen'))

    nns_capi_deps += dependency('mm-camcorder')
    if (tizenVmajor >= 5)


Suggested change

if (tizenVmajor >= 5)

if (tizenVmajor >= 9)

anyj0527 · 2024-10-11T02:15:39Z

@@ -110,7 +110,8 @@ BuildRequires:	pkgconfig(capi-privacy-privilege-manager)
 %endif
 BuildRequires:	pkgconfig(mm-camcorder)
 %if 0%{tizen_version_major} >= 5


5 -> 9. Or make an another if phrase ?

gichan-jang · 2024-10-11T02:25:46Z

+	base = g_path_get_basename(contents);
+
+	if (g_strlcpy(appid, base, size) >= size)
+		LOGE("string truncated");


Return error or change log level to warning?

songgot · 2024-10-15T01:15:21Z

+	_ml_logi("app id : %s type %d", resources->rci.app_id, type);
+	memset(&request_resources, 0x0, sizeof(rm_category_request_s));
+
+	device = &resources->devices[type];


Isn't validation of device necessary?

myungjoo-bot

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

Summary: This PR migrates from mm-resource-manager to the Tizen 9 common resource manager and removes the pipe_h->resources hash table / cleanup_resource machinery. Current main already contains this migration (0ed5144 [Tizen/C-Api] update res-mgr changes, version-gated tizenVmajor >= 9 / >= 5), and git merge-tree reports content conflicts in all 5 touched files (gh shows CONFLICTING). I recommend closing this PR or rebasing it down to whatever delta is still missing from main.

Independent of the conflict, the PR-side code has the following issues:

[High] Resource/handle leak on every pipeline destroy — c/src/ml-api-inference-pipeline.c: cleanup_resource() (the only caller of release_tizen_resource) is deleted and ml_pipeline_destroy no longer touches p->resources (PR line ~1131). _ml_tizen_release_resource() becomes dead code, so the rm_register registration, the exclusively allocated camera, mm_handle, and the DPM callback leak per pipeline until process exit. Fix: call release_tizen_resource (p->resources, TIZEN_RES_MM); p->resources = NULL; in ml_pipeline_destroy (or keep the hash-table design as main does).
[High] Out-of-bounds write — c/src/ml-api-inference-tizen-privilege-check.c:290-291: device = &resources->devices[type]; memset (device, 0, ...) executes before the switch (type) validation. _ml_tizen_get_resource() (line ~820) passes RES_TYPE_MAX, which indexes one past the array and clobbers rci / need_to_acquire. Fix: g_return_val_if_fail (type < RES_TYPE_MAX, ...) before indexing, and handle RES_TYPE_MAX ("re-acquire all") explicitly.
[High] Regression: pipeline no longer paused on resource conflict — privilege-check.c:396-415: the pause in the release callback is commented out (//gst_element_set_state (...)) and the re-acquire loop in ml_pipeline_start is deleted; need_to_acquire[] is declared but never used. When the resource manager revokes the camera the pipeline keeps running and fails inside the camera element. Fix: pass the pipeline as cb_data, pause under p->lock, set a re-acquire flag, and restore re-acquisition in ml_pipeline_start. Please also remove the commented-out code.
[High] Broken build dependency — c/meson.build:26: dependency('resource-manager resource-center-api') asks pkg-config for a single module with that literal name, which does not exist. It also drops mm-resource-manager for tizenVmajor >= 5, breaking Tizen 5-8 builds (as gichan-jang / anyj0527 already noted). Same in packaging/machine-learning-api.spec:113-114. Fix: two dependency() calls under if tizenVmajor >= 9, elif >= 5 keeping mm-resource-manager.
[High] Version regression — packaging/machine-learning-api.spec:79: Version: 1.8.6 -> 1.1.0 downgrades the RPM (main is 1.8.8). Please drop this hunk.
[Medium] Leak on rm_register failure — privilege-check.c:499-502 returns directly, skipping rm_error: cleanup; mm_handle and its GMutex leak. Fix: status = ML_ERROR_UNKNOWN; goto rm_error;.
[Medium] NULL deref before NULL check — privilege-check.c:399-402: g_mutex_locker_new (&mm_handle->lock) runs before g_return_val_if_fail (cb_data, ...).
[Medium] Mutex cleared regardless of destroy — ml_tizen_mm_res_release() (~line 445) calls g_mutex_clear even when destroy == FALSE, leaving a reusable handle with a destroyed mutex. Move it inside if (destroy).
[Medium] Missing validation — line 261-262: g_strlcpy truncation of the app id only logs and proceeds; line ~290: the rm_allocate_resources result (allocated_num) is not validated before treating allocation as successful (both already raised by reviewers).
[Medium] No tests / CI cannot catch any of the above — no test references TIZEN_RES_MM / get_tizen_resource, GBS CI builds only Tizen-Unified, and this PR shows no CI checks at all.
[Low] Style — new functions use tab indentation and func(arg) instead of the repo's gst-indent GNU style; LOGE/LOGW/LOGI instead of _ml_loge/_ml_logw/_ml_logi; typos Faied (line 569), resoure (line 435); tizen_mm_handle_s fields lack /**< */ comments; the commit message carries a Gerrit Change-Id: line.

No back-door or suspicious behavior found.

again4you added DO NOT MERGE Tizen 9.0 M2 labels Oct 11, 2024

again4you requested review from anyj0527, gichan-jang, jaeyun-jung, jijoongmoon, myungjoo and songgot as code owners October 11, 2024 01:23

again4you commented Oct 11, 2024

View reviewed changes

gichan-jang reviewed Oct 11, 2024

View reviewed changes

anyj0527 reviewed Oct 11, 2024

View reviewed changes

gichan-jang reviewed Oct 11, 2024

View reviewed changes

again4you removed the Tizen 9.0 M2 label Oct 14, 2024

songgot reviewed Oct 15, 2024

View reviewed changes

myungjoo-bot suggested changes Sep 3, 2026

View reviewed changes

Navigation Menu

Uh oh!

[MM] Change to the common resource manager - #567

[MM] Change to the common resource manager#567
again4you wants to merge 1 commit into
nnstreamer:mainfrom
again4you:dev/mm_res_mgr_bugfix

again4you commented Oct 11, 2024

Uh oh!

taos-ci commented Oct 11, 2024

Uh oh!

again4you Oct 11, 2024

Uh oh!

again4you Oct 11, 2024

Uh oh!

again4you Oct 11, 2024

Uh oh!

gichan-jang Oct 11, 2024

Uh oh!

anyj0527 Oct 11, 2024 •

edited

Loading

Uh oh!

gichan-jang Oct 11, 2024 •

edited

Loading

Uh oh!

songgot Oct 15, 2024

Uh oh!

myungjoo-bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

		@@ -23,7 +23,7 @@ if (get_option('enable-tizen'))

		nns_capi_deps += dependency('mm-camcorder')
		if (tizenVmajor >= 5)

Uh oh!

Conversation

again4you commented Oct 11, 2024

Uh oh!

taos-ci commented Oct 11, 2024

Uh oh!

again4you Oct 11, 2024

Choose a reason for hiding this comment

Uh oh!

again4you Oct 11, 2024

Choose a reason for hiding this comment

Uh oh!

again4you Oct 11, 2024

Choose a reason for hiding this comment

Uh oh!

gichan-jang Oct 11, 2024

Choose a reason for hiding this comment

Uh oh!

anyj0527 Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gichan-jang Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

songgot Oct 15, 2024

Choose a reason for hiding this comment

Uh oh!

myungjoo-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

anyj0527 Oct 11, 2024 •

edited

Loading

gichan-jang Oct 11, 2024 •

edited

Loading