feat: support ncnn ocr#142
Open
Aliothmoon wants to merge 2 commits into
Open
Conversation
setup_maa_core.py 解压官方 android tarball 后，调用新增的 convert_ocr_ncnn.py 就地把 PaddleOCR/PaddleCharOCR/global 的 inference.onnx 转成 ncnn（rec: onnxsim 固定 [1,3,48,320] + pnnx fp16=0；det: pnnx fp16=0 保 fp32），再删掉 OCR onnx（保留 resource/onnx 的 OnnxSessions）。
审阅者指南在构建阶段新增一条流水线，将已部署的 PP-OCR Android 构建中 OCR onnx 到 ncnn 转换的时序图sequenceDiagram
participant GitHubActions
participant setup_maa_core
participant convert_ocr_ncnn
participant pnnx
participant onnxsim
GitHubActions->>setup_maa_core: main(--abi, --skip-ncnn, --keep-onnx, --rec-fp16)
alt not args.skip_ncnn
setup_maa_core->>convert_ocr_ncnn: convert_tree(resource_dir, cache_dir, keep_onnx, rec_fp16)
loop each inference.onnx
convert_ocr_ncnn->>convert_ocr_ncnn: convert_one(onnx_path, kind, cache_dir, rec_fp16)
alt cache_miss
convert_ocr_ncnn->>convert_ocr_ncnn: _convert_into_cache(onnx_path, kind, rec_fp16, cache_param, cache_bin)
opt kind == rec
convert_ocr_ncnn->>onnxsim: _run([python, -m, onnxsim, onnx_path, rec_sim.onnx, --overwrite-input-shape])
end
convert_ocr_ncnn->>pnnx: _run([pnnx, work_onnx.name, inputshape, fp16])
else cache_hit
convert_ocr_ncnn-->>convert_ocr_ncnn: reuse cached .param/.bin
end
end
convert_ocr_ncnn-->>setup_maa_core: stats
else args.skip_ncnn
setup_maa_core-->>GitHubActions: [SKIP] ncnn conversion
end
文件级变更
技巧与命令与 Sourcery 交互
自定义使用体验打开你的 控制面板 来：
获取帮助Original review guide in EnglishReviewer's GuideAdds a build-time pipeline to convert deployed PP-OCR inference.onnx models into ncnn format for Android OCR, with caching and CLI flags, and wires it into setup_maa_core and GitHub Android build workflows. Sequence diagram for OCR onnx to ncnn conversion in Android buildsequenceDiagram
participant GitHubActions
participant setup_maa_core
participant convert_ocr_ncnn
participant pnnx
participant onnxsim
GitHubActions->>setup_maa_core: main(--abi, --skip-ncnn, --keep-onnx, --rec-fp16)
alt not args.skip_ncnn
setup_maa_core->>convert_ocr_ncnn: convert_tree(resource_dir, cache_dir, keep_onnx, rec_fp16)
loop each inference.onnx
convert_ocr_ncnn->>convert_ocr_ncnn: convert_one(onnx_path, kind, cache_dir, rec_fp16)
alt cache_miss
convert_ocr_ncnn->>convert_ocr_ncnn: _convert_into_cache(onnx_path, kind, rec_fp16, cache_param, cache_bin)
opt kind == rec
convert_ocr_ncnn->>onnxsim: _run([python, -m, onnxsim, onnx_path, rec_sim.onnx, --overwrite-input-shape])
end
convert_ocr_ncnn->>pnnx: _run([pnnx, work_onnx.name, inputshape, fp16])
else cache_hit
convert_ocr_ncnn-->>convert_ocr_ncnn: reuse cached .param/.bin
end
end
convert_ocr_ncnn-->>setup_maa_core: stats
else args.skip_ncnn
setup_maa_core-->>GitHubActions: [SKIP] ncnn conversion
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了 1 个安全问题、1 个其他问题，并留下了一些整体性的反馈：
安全问题：
- 检测到未使用静态字符串的
subprocess.run调用。如果这一数据可能被恶意行为者控制，就可能存在命令注入风险。请审查该调用的使用场景，确保其不会被外部资源控制。你可以考虑使用shlex.escape()。(link)
总体评论：
- 在
_pnnx_outputs中，binp路径用两种不同方式计算了两次，第一次的值被覆盖了；建议简化为单一且明确正确的构造方式，以避免困惑和潜在的路径相关 bug。 _find_pnnx()会在每次转换时通过_convert_into_cache间接调用；你可以只解析一次pnnx并将路径向下传递，以避免在大型目录树中多次执行shutil.which查找。
给 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- In `_pnnx_outputs`, the `binp` path is computed twice with two different approaches and the first value is overwritten; consider simplifying this to a single, clearly correct construction to avoid confusion and potential path bugs.
- `_find_pnnx()` is invoked indirectly for every conversion via `_convert_into_cache`; you could resolve `pnnx` once and pass the path down to avoid repeated `shutil.which` lookups in large trees.
## Individual Comments
### Comment 1
<location path="scripts/convert_ocr_ncnn.py" line_range="86-94" />
<code_context>
+
+
+def _pnnx_outputs(workdir: Path) -> tuple[Path, Path]:
+ params = list(workdir.glob("*.ncnn.param"))
+ if len(params) != 1:
+ raise SystemExit(f"expected exactly one *.ncnn.param in {workdir}, found {len(params)}")
+ param = params[0]
+ binp = param.with_suffix("").with_suffix(".bin") # foo.ncnn.param -> foo.ncnn.bin
+ binp = param.parent / (param.name[: -len(".param")] + ".bin")
+ if not binp.exists():
+ raise SystemExit(f"pnnx .bin not found next to {param}")
+ return param, binp
+
+
</code_context>
<issue_to_address>
**nitpick:** Avoid redundant and slightly confusing `.bin` path construction in `_pnnx_outputs`.
Inside `_pnnx_outputs`, `binp` is assigned twice and only the second value is used. Please remove the unused first assignment and stick to a single, clear construction for the `.bin` filename (e.g. derived from `param.stem`), so there’s no dead code or ambiguity about the naming logic.
</issue_to_address>
### Comment 2
<location path="scripts/convert_ocr_ncnn.py" line_range="58-65" />
<code_context>
res = subprocess.run(
[str(c) for c in cmd],
cwd=str(cwd) if cwd else None,
capture_output=True,
text=True,
encoding="utf-8",
errors="replace",
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续评审。
Original comment in English
Hey - I've found 1 security issue, 1 other issue, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- In
_pnnx_outputs, thebinppath is computed twice with two different approaches and the first value is overwritten; consider simplifying this to a single, clearly correct construction to avoid confusion and potential path bugs. _find_pnnx()is invoked indirectly for every conversion via_convert_into_cache; you could resolvepnnxonce and pass the path down to avoid repeatedshutil.whichlookups in large trees.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_pnnx_outputs`, the `binp` path is computed twice with two different approaches and the first value is overwritten; consider simplifying this to a single, clearly correct construction to avoid confusion and potential path bugs.
- `_find_pnnx()` is invoked indirectly for every conversion via `_convert_into_cache`; you could resolve `pnnx` once and pass the path down to avoid repeated `shutil.which` lookups in large trees.
## Individual Comments
### Comment 1
<location path="scripts/convert_ocr_ncnn.py" line_range="86-94" />
<code_context>
+
+
+def _pnnx_outputs(workdir: Path) -> tuple[Path, Path]:
+ params = list(workdir.glob("*.ncnn.param"))
+ if len(params) != 1:
+ raise SystemExit(f"expected exactly one *.ncnn.param in {workdir}, found {len(params)}")
+ param = params[0]
+ binp = param.with_suffix("").with_suffix(".bin") # foo.ncnn.param -> foo.ncnn.bin
+ binp = param.parent / (param.name[: -len(".param")] + ".bin")
+ if not binp.exists():
+ raise SystemExit(f"pnnx .bin not found next to {param}")
+ return param, binp
+
+
</code_context>
<issue_to_address>
**nitpick:** Avoid redundant and slightly confusing `.bin` path construction in `_pnnx_outputs`.
Inside `_pnnx_outputs`, `binp` is assigned twice and only the second value is used. Please remove the unused first assignment and stick to a single, clear construction for the `.bin` filename (e.g. derived from `param.stem`), so there’s no dead code or ambiguity about the naming logic.
</issue_to_address>
### Comment 2
<location path="scripts/convert_ocr_ncnn.py" line_range="58-65" />
<code_context>
res = subprocess.run(
[str(c) for c in cmd],
cwd=str(cwd) if cwd else None,
capture_output=True,
text=True,
encoding="utf-8",
errors="replace",
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+86
to
+94
| params = list(workdir.glob("*.ncnn.param")) | ||
| if len(params) != 1: | ||
| raise SystemExit(f"expected exactly one *.ncnn.param in {workdir}, found {len(params)}") | ||
| param = params[0] | ||
| binp = param.with_suffix("").with_suffix(".bin") # foo.ncnn.param -> foo.ncnn.bin | ||
| binp = param.parent / (param.name[: -len(".param")] + ".bin") | ||
| if not binp.exists(): | ||
| raise SystemExit(f"pnnx .bin not found next to {param}") | ||
| return param, binp |
There was a problem hiding this comment.
nitpick： 请避免在 _pnnx_outputs 中对 .bin 路径进行冗余且略显困惑的构造。
在 _pnnx_outputs 内部，binp 被赋值了两次，但只使用了第二次的值。请删除未使用的第一次赋值，并仅保留一种清晰的 .bin 文件名构造方式（例如基于 param.stem），以消除无效代码并避免命名逻辑上的歧义。
Original comment in English
nitpick: Avoid redundant and slightly confusing .bin path construction in _pnnx_outputs.
Inside _pnnx_outputs, binp is assigned twice and only the second value is used. Please remove the unused first assignment and stick to a single, clear construction for the .bin filename (e.g. derived from param.stem), so there’s no dead code or ambiguity about the naming logic.
Comment on lines
+58
to
+65
| res = subprocess.run( | ||
| [str(c) for c in cmd], | ||
| cwd=str(cwd) if cwd else None, | ||
| capture_output=True, | ||
| text=True, | ||
| encoding="utf-8", | ||
| errors="replace", | ||
| ) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit)： 检测到未使用静态字符串的 subprocess.run 调用。如果这一数据可能被恶意行为者控制，就可能存在命令注入风险。请审查该调用的使用场景，确保其不会被外部资源控制。你可以考虑使用 shlex.escape()。
来源：opengrep
Original comment in English
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
onnxsim 在 GitHub macOS runner 的 Python 3.14 上 SIGSEGV(exit -11)，导致 rec 转换失败。实测 pnnx 仅凭固定 inputshape=[1,3,48,320] 即可正确转 SVTR rec(对 CN/PaddleCharOCR/全部 global 实测 cos=1.0、argmax=100%)，无需 onnxsim 预简化。故移除 onnxsim 步骤，rec 与 det 统一为纯 pnnx；requirements 仅保留 pnnx(不再装 onnxsim/onnxruntime)。
Aliothmoon
force-pushed
the
feat/ncnn-build-conversion
branch
from
58de704 to
cd344f5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
setup_maa_core.py 解压官方 android tarball 后，把 PaddleOCR/PaddleCharOCR/global 的 inference.onnx 转成 ncnn 模型
rec: onnxsim 固定 [1,3,48,320] + pnnx fp16=0；det: pnnx fp16=0 保 fp32），再删掉 OCR onnx（保留 resource/onnx 的 OnnxSessions
Sourcery 总结
在构建阶段将已部署的 OCR ONNX 模型转换为用于 Android 的 NCNN 格式，并将其集成到 MAA 核心的安装流程和 CI 构建流程中。
新特性：
增强：
构建：
CI：
Original summary in English
Summary by Sourcery
Add build-time conversion of deployed OCR ONNX models to NCNN for Android and wire it into the MAA core setup and CI build workflows.
New Features:
Enhancements:
Build:
CI: