feat: unified image_to_svg function as complete pipeline#335
Merged
Conversation
github-actions
Bot
added
c/cpp
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a unified image_to_svg pipeline: new ImageToSvgConfig types and core image_to_svg implementation; exposes C config lifecycle and C API; updates WASM/JS and Python bindings and example apps to call the new config-driven image_to_svg entrypoint. ChangesImage-to-SVG consolidated pipeline
Sequence DiagramsequenceDiagram
participant Client
participant Config as ImageToSvgConfig
participant Core as C++ Core<br/>image_to_svg
participant BF as BilateralFilter
participant KM as KMeans
participant SVG as labels_to_svg
Client->>Config: create/configure (sigma, k, color_space, min_area)
Client->>Core: image_to_svg(data, width, height, config)
activate Core
Core->>Core: allocate buffers / copy input
Core->>BF: apply bilateral_filter(config.bilateral_filter, config.color_space)
BF-->>Core: filtered image
Core->>KM: run kmeans(config.kmeans, config.color_space)
KM-->>Core: labels
Core->>SVG: labels_to_svg(labels, config.min_cluster_area)
SVG-->>Core: svg string
Core-->>Client: svg string
deactivate Core
sequenceDiagram
participant Browser
participant Wrapper as safeWasmWrappers
participant WASM as WASM Module
participant CBinding as C Binding
participant CppCore as C++ Core
Browser->>Wrapper: imgToSVG(pixels, params)
Wrapper->>WASM: image_to_svg(ptr, w, h, sigma_spatial, sigma_range, k, max_iter, min_area, color_space)
activate WASM
WASM->>WASM: create ImageToSvgConfig
WASM->>CBinding: img2num_image_to_svg(data, config)
CBinding->>CppCore: image_to_svg(data, w, h, *config)
CppCore-->>CBinding: std::string(svg)
CBinding-->>WASM: char* (malloc'd) svg
WASM->>WASM: free config
WASM-->>Wrapper: svg string
Wrapper-->>Browser: { svg, visualization }
deactivate WASM
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
|
@Ryan-Millard overly paranoid lint check |
Ryan-Millard
requested changes
Apr 29, 2026
Owner
Ryan-Millard
left a comment
Ryan-Millard
left a comment
There was a problem hiding this comment.
This is very nice and clean. Thank you.
There are just a few minor questions I have and one major architectural discussion I think we need to have before this can be merged. The architectural concern is about the core image_to_svg function.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/c/include/cimg2num.h`:
- Around line 15-23: The C header exposes an opaque ImageToSvgConfig but
provides no way for C callers to construct or initialize it, making
img2num_image_to_svg unusable from pure C; add C-compatible construction and
initialization APIs (e.g., img2num_image_to_svg_config_new /
img2num_image_to_svg_config_free and an init/reset function or a flat C struct
equivalent) and expose setter/getter functions for any fields used by
img2num_image_to_svg so C users can populate the config; update cimg2num.h (and
the corresponding symbols referenced around ImageToSvgConfig and
img2num_image_to_svg, including the other occurrences at lines 52-56) to declare
these factory/init/setter prototypes so the public C API mirrors the C++
surface.
In `@bindings/c/src/cimg2num.cpp`:
- Around line 76-87: The lambda in img2num_image_to_svg dereferences cfg
unconditionally causing a crash when config is NULL; update the lambda in
img2num_image_to_svg to first check if cfg is nullptr and if so either use a
default ImageToSvgConfig (create a local default_config) or immediately set
result to nullptr and return (so the C binding returns the sentinel nullptr on
failure), and only call img2num::image_to_svg(d, w, h, *cfg) after ensuring cfg
is non-null; also ensure memory allocation and memcpy happen only after svg is
produced and that failures leave result as nullptr.
In `@bindings/js/src/wasm_wrapper.cpp`:
- Around line 41-47: The WASM wrapper function image_to_svg must construct an
ImageToSvgConfig instance from the wrapper arguments and pass its pointer to
img2num_image_to_svg (which now expects const ImageToSvgConfig*); update
image_to_svg to populate a local ImageToSvgConfig with fields mapped from data,
width, height, sigma_spatial, sigma_range, k, max_iter, min_area, and
color_space, then call img2num_image_to_svg(&config) and return the result,
ensuring types match the new C API.
In `@bindings/py/src/img2num_pybind.cpp`:
- Around line 124-140: The ImageToSvgConfig binding exposes the nested types but
not the struct members, so Python cannot set bilateral_filter, kmeans,
min_cluster_area or color_space; update the pybind definition for
img2num::ImageToSvgConfig (the variable config) to expose those members by
adding .def_readwrite("bilateral_filter",
&img2num::ImageToSvgConfig::bilateral_filter), .def_readwrite("kmeans",
&img2num::ImageToSvgConfig::kmeans), .def_readwrite("min_cluster_area",
&img2num::ImageToSvgConfig::min_cluster_area) and .def_readwrite("color_space",
&img2num::ImageToSvgConfig::color_space) (remove the commented-out block and
keep the existing custom init that returns IMAGE_TO_SVG_DEFAULT_CONFIG).
In `@core/include/img2num.h`:
- Around line 18-38: Add Doxygen documentation for the new public types and
members: document the ImageToSvgConfig struct, its nested BilateralFilterConfig
and KMeansConfig types, and each public field (sigma_spatial, sigma_range,
min_cluster_area, color_space, k, max_iter) so they appear in generated API
docs; also add a brief doc comment for the IMAGE_TO_SVG_DEFAULT_CONFIG constant.
Ensure comments use Doxygen style (/** ... */) immediately above the
declarations of ImageToSvgConfig, BilateralFilterConfig, KMeansConfig, and
IMAGE_TO_SVG_DEFAULT_CONFIG and briefly describe purpose, units/range where
applicable, and defaults.
In `@core/src/internal/image_to_svg.cpp`:
- Around line 10-17: The function image_to_svg currently allocates
img_data/out_data/out_labels and calls std::memcpy using width * height * 4
computed as signed ints, which can overflow or be negative; validate inputs at
the top of image_to_svg by checking that width and height are positive and that
the product (cast to size_t) does not overflow when multiplied by 4 (e.g.,
ensure (size_t)width * (size_t)height <= SIZE_MAX / 4), return an error or empty
string on invalid dimensions, then compute sizes into size_t variables and use
those for vector sizing and for the memcpy length to avoid huge or negative
allocations and unsafe copies (refer to image_to_svg, img_data, out_data,
out_labels, and the memcpy call).
In `@example-apps/console-cpp/main.cpp`:
- Line 69: The call to img2num::image_to_svg is using img_data which has been
modified by the manual bilateral/kmeans path; change it to use the original
unmodified image buffer (e.g., save the raw input into a separate variable
before applying bilateral/kmeans and pass that raw buffer to
img2num::image_to_svg) so the example demonstrates the unified API on raw input;
ensure references to image_to_svg and the bilateral/kmeans modification code use
the preserved original buffer variable rather than the mutated img_data.
In `@packages/js/safeWasmWrappers.js`:
- Around line 234-240: The wrapper for the image_to_svg entrypoint incorrectly
returns a visualization from result.output.pixels even though the WASM function
only produces an SVG string; update the code in safeWasmWrappers.js around the
callWasm invocation for funcName "image_to_svg" (and any helper that consumes
its return) to stop accessing result.output.pixels and only return the SVG
(e.g., return { svg: result.returnValue } or equivalent), removing the
nonexistent visualization field so callers no longer expect an output pixel
buffer.
- Around line 233-236: The public API destructures and forwards a param named
sigma_pixels but the project uses sigma_range elsewhere; update the imgToSVG
function signature to destructure sigma_range (default 50) instead of
sigma_pixels and ensure the args passed to callWasm use the sigma_range property
(i.e., replace occurrences of sigma_pixels with sigma_range in the imgToSVG
parameter list and the args object so callers using { sigma_range: ... } are
honored).
In `@packages/py/img2num/api.py`:
- Around line 69-71: The wrapper image_to_svg currently requires a config even
though the core _image_to_svg accepts a default; make config optional by
defaulting it to ImageToSvgConfig() (e.g., accept config=None and if None set
config = ImageToSvgConfig()) before calling _image_to_svg(image, width, height,
config), so callers can use the simple form or pass a custom config; reference
image_to_svg, _image_to_svg, and ImageToSvgConfig when locating and updating the
code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3c3b9f2e-8ad0-48ee-bbb4-025f78b337e4
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
bindings/c/include/cimg2num.hbindings/c/src/cimg2num.cppbindings/js/src/wasm_wrapper.cppbindings/py/src/img2num_pybind.cppcore/include/img2num.hcore/include/internal/gpu.hcore/src/internal/bilateral_filter.cppcore/src/internal/bilateral_filter_gpu.cppcore/src/internal/image_to_svg.cppcore/src/internal/kmeans_gpu.cppcore/src/internal/labels_to_svg.cppexample-apps/console-cpp/main.cppexample-apps/console-py/main.pyexample-apps/react-js/src/components/WasmImageProcessor.jsxpackages/js/safeWasmWrappers.jspackages/py/img2num/api.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
example-apps/**
⚙️ CodeRabbit configuration file
example-apps/**: - Example applications. These are for demonstration; keep them minimal and ensure
they correctly reflect the public API. Flag any use of internal/private APIs.
- IMPORTANT: These example apps must be a good reflection of how to use Img2Num's
library, which means that they need good comments and must have clean code. This
is meant to be external and is designed for others to use to get started.
Files:
example-apps/console-py/main.pyexample-apps/console-cpp/main.cppexample-apps/react-js/src/components/WasmImageProcessor.jsx
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Python source files. Review for:
- PEP 8 compliance (enforced by ruff; focus on logic, not style).
- Type annotation coverage — prefer typed signatures for public functions.
- Exception handling: avoid bare
except:clauses; always catch specific exceptions.- Resource management: use
withstatements for file/socket/connection handling.- No mutable default arguments (e.g.
def foo(x=[]):).- When suggesting how to run Python tooling, prefer the
./img2numDocker wrapper
rather than directpython/ruff/pylintinvocations.
Files:
example-apps/console-py/main.pypackages/py/img2num/api.py
**/*.{c,cc,cpp,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (.clang-format)
**/*.{c,cc,cpp,cxx,h,hpp,hxx}: Follow Google style guide for C/C++ code formatting
Use 4 spaces for indentation
Maintain a column limit of 100 characters per line
Do not allow short functions on a single line
Files:
core/src/internal/kmeans_gpu.cppcore/src/internal/bilateral_filter.cppcore/include/internal/gpu.hbindings/js/src/wasm_wrapper.cppcore/src/internal/labels_to_svg.cppcore/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppcore/src/internal/bilateral_filter_gpu.cppbindings/c/include/cimg2num.hbindings/c/src/cimg2num.cppcore/include/img2num.hbindings/py/src/img2num_pybind.cpp
**/*.{hpp,cpp,c,h}
📄 CodeRabbit inference engine (.editorconfig)
**/*.{hpp,cpp,c,h}: Use 4-space indentation for C/C++ files
Maintain 120 character maximum line length for C/C++ files
Files:
core/src/internal/kmeans_gpu.cppcore/src/internal/bilateral_filter.cppcore/include/internal/gpu.hbindings/js/src/wasm_wrapper.cppcore/src/internal/labels_to_svg.cppcore/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppcore/src/internal/bilateral_filter_gpu.cppbindings/c/include/cimg2num.hbindings/c/src/cimg2num.cppcore/include/img2num.hbindings/py/src/img2num_pybind.cpp
core/**/*.{cpp,c,h,hpp}
⚙️ CodeRabbit configuration file
core/**/*.{cpp,c,h,hpp}: This is the Img2Num core C/C++ library. Review for:
- Memory safety: null pointer dereferences, use-after-free, buffer overflows.
- Correct RAII usage and smart pointer idioms.
- Adherence to the .clang-format style; formatting must be applied via
./img2num format-cpp(or./img2num format-wasmfor WASM modules),
NOT by calling clang-format directly.- When suggesting build/test steps, always use the Docker-first wrapper scripts
(./img2num,img2num.ps1, orimg2num.bat) rather than direct tool invocations,
since contributors may not have dependencies installed locally.- Must have Doxygen docstrings in the API (
img2num.h) files. This is important!
Files:
core/src/internal/kmeans_gpu.cppcore/src/internal/bilateral_filter.cppcore/include/internal/gpu.hcore/src/internal/labels_to_svg.cppcore/src/internal/image_to_svg.cppcore/src/internal/bilateral_filter_gpu.cppcore/include/img2num.h
**/*.{cpp,c,h,hpp}
⚙️ CodeRabbit configuration file
**/*.{cpp,c,h,hpp}: - For any C/C++ code outside core/ (e.g. bindings, example-apps), apply the same
memory-safety and style standards as the core. Formatting via./img2num format-cpp.
- Doxygen docstrings are required.
Files:
core/src/internal/kmeans_gpu.cppcore/src/internal/bilateral_filter.cppcore/include/internal/gpu.hbindings/js/src/wasm_wrapper.cppcore/src/internal/labels_to_svg.cppcore/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppcore/src/internal/bilateral_filter_gpu.cppbindings/c/include/cimg2num.hbindings/c/src/cimg2num.cppcore/include/img2num.hbindings/py/src/img2num_pybind.cpp
bindings/**
⚙️ CodeRabbit configuration file
bindings/**: Language bindings for the Img2Num library. Ensure the public API surface matches
the core C/C++ headers and that error propagation is handled correctly for each
binding language. Also ensure the relevant docstrings are present to enable
auto-generation of documentation.
Files:
bindings/js/src/wasm_wrapper.cppbindings/c/include/cimg2num.hbindings/c/src/cimg2num.cppbindings/py/src/img2num_pybind.cpp
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.editorconfig)
**/*.{js,ts,jsx,tsx}: Use 2-space indentation for JavaScript and TypeScript files
Maintain 200 character maximum line length for JavaScript/TypeScript files
Files:
packages/js/safeWasmWrappers.jsexample-apps/react-js/src/components/WasmImageProcessor.jsx
🧠 Learnings (18)
📓 Common learnings
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-25T16:20:34.359Z
Learning: In the Ryan-Millard/Img2Num repository, the following follow-up issues were created after merging PR `#307` (Python bindings): `#328` (Python docstrings/docs), `#329` (numpy img.shape inference for width/height), `#330` (.coderabbit.yaml Python tooling config), `#331` (CI workflow for Python bindings build test).
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-25T16:31:13.204Z
Learning: In the Ryan-Millard/Img2Num repository, issue `#331` (CI workflow for Python bindings build test) was addressed in PR `#307` via the `build-py` job in `.github/workflows/cmake-build.yml`. That job runs `uv venv`, `uv sync --no-build-isolation`, a smoke test (`uv run python3 -c "import img2num;"`), and uploads artifacts.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-23T18:48:02.692Z
Learning: In the Ryan-Millard/Img2Num repository, PR `#311` adds `.coderabbit.yaml` to main. PR `#307` (adding Python .py files) must rebase onto main after `#311` merges so that `.coderabbit.yaml` exists and can be updated with: `ruff: enabled: true`, `pylint: enabled: true`, `flake8: enabled: false` under `tools:`, and a `path_instructions` entry for `**/*.py` covering PEP 8, type annotations, exception handling, resource management, mutable default arguments, and Docker-first command suggestions via the `./img2num` wrapper. The "CodeRabbit Config Needs Update" custom check will automatically flag `#307` if the Python config is missing.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-23T18:48:02.712Z
Learning: In the Ryan-Millard/Img2Num repository, PR `#311` adds `.coderabbit.yaml` to main. PR `#307` (adding Python .py files) must rebase onto main after `#311` merges so that `.coderabbit.yaml` exists and can be updated with: `ruff: enabled: true`, `pylint: enabled: true`, `flake8: enabled: false` under `tools:`, and a `path_instructions` entry for `**/*.py` covering PEP 8, type annotations, exception handling, resource management, mutable default arguments, and Docker-first command suggestions via the `./img2num` wrapper. The "CodeRabbit Config Needs Update" custom check will automatically flag `#307` if the Python config is missing.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-01T20:45:20.732Z
Learning: In the Ryan-Millard/Img2Num repository, PRs that introduce documentation or code examples referencing functions or symbols not present in the codebase (e.g., a `vectorize` function from `img2num`) should be flagged as potential AI-generated slop. Always cross-reference newly introduced API names/symbols against actual codebase exports.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-23T18:35:01.229Z
Learning: In the Ryan-Millard/Img2Num repository, Python (.py files) is being added to the codebase via PR `#307`. The `.coderabbit.yaml` should include `ruff: enabled: true` and `pylint: enabled: true` under `tools:`, and a `path_instructions` entry for `**/*.py` covering PEP 8, type annotations, exception handling, resource management, mutable default arguments, and Docker-first command suggestions via the `./img2num` wrapper.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-23T18:35:01.229Z
Learning: In the Ryan-Millard/Img2Num repository, Python (.py files) is being added to the codebase via PR `#307`. The `.coderabbit.yaml` should include `ruff: enabled: true` and `pylint: enabled: true` under `tools:`, and a `path_instructions` entry for `**/*.py` covering PEP 8, type annotations, exception handling, resource management, mutable default arguments, and Docker-first command suggestions.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-01T20:45:20.732Z
Learning: In the Ryan-Millard/Img2Num repository, a PR body containing generic boilerplate such as "This contribution was made as part of an open-source documentation improvement initiative. All changes are meant to be helpful additions to the project." is a strong signal of an AI-generated slop PR and should be explicitly flagged during review.
📚 Learning: 2025-12-31T17:46:54.476Z
Learnt from: fransafu
Repo: Ryan-Millard/Img2Num PR: 176
File: src/wasm/modules/image/src/bilateral_filter.cpp:98-101
Timestamp: 2025-12-31T17:46:54.476Z
Learning: In bilateral_filter.cpp (src/wasm/modules/image/src/bilateral_filter.cpp), rely on the fact that the center pixel contributes a weight of exactly 1.0 to both spatial and range components (exp(0) = 1) so the normalization sum cannot be zero. This implies you do not need explicit guards against division by zero for normalization in this implementation.
Applied to files:
core/src/internal/kmeans_gpu.cppcore/src/internal/bilateral_filter.cppbindings/js/src/wasm_wrapper.cppcore/src/internal/labels_to_svg.cppcore/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppcore/src/internal/bilateral_filter_gpu.cppbindings/c/src/cimg2num.cppbindings/py/src/img2num_pybind.cpp
📚 Learning: 2026-01-06T04:56:57.269Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 191
File: src/wasm/modules/image/src/kmeans.cpp:128-134
Timestamp: 2026-01-06T04:56:57.269Z
Learning: In C++ sources compiled for WASM with Emscripten/Clang, designated initializers (e.g., RGBXY{.r = ..., .g = ...}) are allowed as a C++17 extension. When reviewing code that relies on designated initializers, verify that the target toolchain enables CXX_STANDARD 17 or higher and that the build system (CMake/emsdk) uses Emscripten with a compatible clang. If not, avoid such initializers or provide portable alternatives.
Applied to files:
core/src/internal/kmeans_gpu.cppcore/src/internal/bilateral_filter.cppbindings/js/src/wasm_wrapper.cppcore/src/internal/labels_to_svg.cppcore/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppcore/src/internal/bilateral_filter_gpu.cppbindings/c/src/cimg2num.cppbindings/py/src/img2num_pybind.cpp
📚 Learning: 2026-01-06T21:06:24.476Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 191
File: src/wasm/modules/image/src/cielab.cpp:43-57
Timestamp: 2026-01-06T21:06:24.476Z
Learning: In the Img2Num project, prefer recommending and using the provided docker/script wrappers (e.g., ./img2num format-wasm, ./img2num clean-wasm) instead of invoking local tools directly (e.g., clang-format -i). This reduces dependency requirements for users and ensures consistent tooling across environments. Apply this guidance to C++ source files under the project when reviewing changes.
Applied to files:
core/src/internal/kmeans_gpu.cppcore/src/internal/bilateral_filter.cppbindings/js/src/wasm_wrapper.cppcore/src/internal/labels_to_svg.cppcore/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppcore/src/internal/bilateral_filter_gpu.cppbindings/c/src/cimg2num.cppbindings/py/src/img2num_pybind.cpp
📚 Learning: 2026-01-19T00:02:34.957Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 234
File: src/wasm/modules/image/src/kmeans.cpp:79-81
Timestamp: 2026-01-19T00:02:34.957Z
Learning: In C++ multithreading contexts, prefer unsigned types for thread-count-like parameters (e.g., n_threads) to prevent negative values. Validate that the value is at least 1 before any division to avoid divide-by-zero at runtime. At the start of functions handling such values, create a safe count like: const unsigned int thread_count{std::max(1u, n_threads)}; This ensures non-negative, non-zero usage for divisions and related arithmetic. Apply this pattern to similar parameters across C++ modules, not just the specific file.
Applied to files:
core/src/internal/kmeans_gpu.cppcore/src/internal/bilateral_filter.cppbindings/js/src/wasm_wrapper.cppcore/src/internal/labels_to_svg.cppcore/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppcore/src/internal/bilateral_filter_gpu.cppbindings/c/src/cimg2num.cppbindings/py/src/img2num_pybind.cpp
📚 Learning: 2026-02-25T21:24:19.036Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 272
File: core/src/internal/bilateral_filter_gpu.cpp:171-185
Timestamp: 2026-02-25T21:24:19.036Z
Learning: Adopt brace initialization (e.g., Type var{}) over copy initialization (e.g., Type var = {}) for zero-initialization in C++ across the repository. This should apply to variables, arrays, and structs in most C++ files (e.g., core/src/internal/bilateral_filter_gpu.cpp). It improves safety by avoiding narrowing conversions and makes initialization intent explicit.
Applied to files:
core/src/internal/kmeans_gpu.cppcore/src/internal/bilateral_filter.cppbindings/js/src/wasm_wrapper.cppcore/src/internal/labels_to_svg.cppcore/src/internal/image_to_svg.cppexample-apps/console-cpp/main.cppcore/src/internal/bilateral_filter_gpu.cppbindings/c/src/cimg2num.cppbindings/py/src/img2num_pybind.cpp
📚 Learning: 2026-04-09T20:09:36.128Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-09T20:09:36.128Z
Learning: Img2Num is a cross-platform library supporting multiple runtimes. When the GPU is inaccessible for any reason (e.g., adapter/device acquisition fails, the runtime blocks GPU access, or no GPU is available), the code must not assume usability based solely on GPU API availability. Instead, it must detect the actual failure, automatically fall back to CPU execution, provide a clear message explaining the fallback, and include timeout guards to prevent hangs. This applies across all supported runtimes, not just browsers.
Applied to files:
core/src/internal/bilateral_filter.cpp
📚 Learning: 2026-04-27T15:40:33.329Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 333
File: bindings/c/src/cimg2num.cpp:0-0
Timestamp: 2026-04-27T15:40:33.329Z
Learning: In the Ryan-Millard/Img2Num project, `bindings/c/src/cimg2num.cpp` contains `extern "C"` C bindings. Do NOT suggest throwing C++ exceptions (e.g., `std::bad_alloc`) from within these C binding functions, even from lambdas passed internally to `clear_last_error_and_catch`. The correct C-API idiom is to return `nullptr` (or another sentinel) on failure and let the caller check the return value.
Applied to files:
core/src/internal/bilateral_filter.cppbindings/c/include/cimg2num.hbindings/py/src/img2num_pybind.cpp
📚 Learning: 2026-02-25T21:24:34.055Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 272
File: core/include/internal/bilateral_filter_gpu.h:108-109
Timestamp: 2026-02-25T21:24:34.055Z
Learning: In WGSL shaders, .rgb swizzle on a vec4 returns the first three components regardless of color space. Do not assume data is RGB color space when the texture contains non-RGB data (e.g., CIELAB L, A, B). If LAB data is stored, treat and compute distances in LAB space, not RGB. Clearly document shader code paths that rely on specific color spaces and prefer explicit conversions or comments when using swizzled components with non-RGB textures. Apply this guidance to WGSL shader files across the codebase (not just this header) when handling color data or color-like channels.
Applied to files:
core/include/internal/gpu.hbindings/c/include/cimg2num.hcore/include/img2num.h
📚 Learning: 2026-04-09T19:05:40.514Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 302
File: core/include/internal/gpu.h:203-216
Timestamp: 2026-04-09T19:05:40.514Z
Learning: In Img2Num’s internal GPU initialization code, if querying adapter limits fails (e.g., `adapter.GetLimits(...)` returns `false`), treat this as a GPU initialization failure and route execution to the CPU fallback path. Do not continue with default limits in this case; the failure should cause the same fallback behavior as other GPU init errors.
Applied to files:
core/include/internal/gpu.h
📚 Learning: 2026-04-02T22:17:22.903Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 290
File: bindings/js/CMakeLists.txt:52-53
Timestamp: 2026-04-02T22:17:22.903Z
Learning: In the Img2Num project, `bindings/js/CMakeLists.txt` uses `ASYNCIFY=1` (required for WebGPU via emdawnwebgpu) which is incompatible with `-fwasm-exceptions`. To enable C++ exception handling alongside ASYNCIFY, use `-fexceptions` (Emscripten's JS-based EH) applied consistently to both compile and link options for all build types, and add `-sNO_DISABLE_EXCEPTION_CATCHING` to COMMON_FLAGS so that catch blocks in `clear_last_error_and_catch()` are not stripped by Emscripten's runtime.
Applied to files:
bindings/js/src/wasm_wrapper.cpp
📚 Learning: 2026-03-14T17:55:27.180Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 278
File: example-apps/react-js/src/components/GlassModal.module.css:35-43
Timestamp: 2026-03-14T17:55:27.180Z
Learning: In the Img2Num React example app (example-apps/react-js), the project convention is: CSS Module files (*.module.css) use camelCase for identifiers (class names, keyframe names, etc.), while vanilla CSS files use kebab-case. For example, keyframe names `fadeIn` and `scaleIn` are correct in a `.module.css` file.
Applied to files:
example-apps/react-js/src/components/WasmImageProcessor.jsx
📚 Learning: 2026-04-01T20:45:20.732Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-01T20:45:20.732Z
Learning: In the Ryan-Millard/Img2Num repository, PRs that introduce documentation or code examples referencing functions or symbols not present in the codebase (e.g., a `vectorize` function from `img2num`) should be flagged as potential AI-generated slop. Always cross-reference newly introduced API names/symbols against actual codebase exports.
Applied to files:
bindings/c/include/cimg2num.hbindings/py/src/img2num_pybind.cpp
📚 Learning: 2026-04-27T15:40:33.329Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 333
File: bindings/c/src/cimg2num.cpp:0-0
Timestamp: 2026-04-27T15:40:33.329Z
Learning: In the Ryan-Millard/Img2Num C bindings (e.g., `bindings/c/src/cimg2num.cpp`) that expose `extern "C"` functions, do not allow C++ exceptions (including `std::bad_alloc` and exceptions thrown from lambdas used internally) to propagate out of these C binding functions. Use the C-API failure idiom instead: catch exceptions internally as needed, clear/record error state via the existing error mechanism, and return `nullptr` (or the documented sentinel return value) on failure so the C caller can detect errors by checking the return value.
Applied to files:
bindings/c/src/cimg2num.cpp
📚 Learning: 2026-04-29T20:06:23.790Z
Learnt from: CR
Repo: Ryan-Millard/Img2Num PR: 0
File: docs/docs/contributing/setup-and-dependencies/index.md:0-0
Timestamp: 2026-04-29T20:06:23.790Z
Learning: Compile C++ library to WebAssembly (WASM) using Emscripten before using JavaScript library or example apps
Applied to files:
core/include/img2num.h
📚 Learning: 2026-04-25T16:20:34.359Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-25T16:20:34.359Z
Learning: In the Ryan-Millard/Img2Num repository, the following follow-up issues were created after merging PR `#307` (Python bindings): `#328` (Python docstrings/docs), `#329` (numpy img.shape inference for width/height), `#330` (.coderabbit.yaml Python tooling config), `#331` (CI workflow for Python bindings build test).
Applied to files:
bindings/py/src/img2num_pybind.cpp
📚 Learning: 2026-04-25T16:43:10.485Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-25T16:43:10.485Z
Learning: In Ryan-Millard/Img2Num, the Python documentation tool chosen is pdoc, mirroring the Doxygen pattern for C/C++. The plan: install python3-pip and pdoc in a new Dockerfile.dev layer (between layer 4 Doxygen and layer 5 pnpm), create docs/scripts/pdoc.js (mirrors doxygen.js), output to docs/static/docs/python/api/, update prebuild in docs/package.json to "pnpm doxygen && pnpm pdoc", scaffold docs/docs/python/index.md and docs/docs/python/api-reference.md (uses FullscreenIframe component pointing to /Img2Num/info/docs/python/api/).
Applied to files:
bindings/py/src/img2num_pybind.cpp
📚 Learning: 2026-04-25T16:31:13.204Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-25T16:31:13.204Z
Learning: In the Ryan-Millard/Img2Num repository, issue `#331` (CI workflow for Python bindings build test) was addressed in PR `#307` via the `build-py` job in `.github/workflows/cmake-build.yml`. That job runs `uv venv`, `uv sync --no-build-isolation`, a smoke test (`uv run python3 -c "import img2num;"`), and uploads artifacts.
Applied to files:
bindings/py/src/img2num_pybind.cpp
🪛 Cppcheck (2.20.0)
core/src/internal/labels_to_svg.cpp
[style] 189-189: The function 'labels_to_svg' is never used.
(unusedFunction)
core/src/internal/image_to_svg.cpp
[style] 10-10: The function 'image_to_svg' is never used.
(unusedFunction)
🔇 Additional comments (6)
core/src/internal/bilateral_filter_gpu.cpp (1)
289-289: Reconsider the fixed 100ms Emscripten delay.This introduces unconditional per-call latency in a hot path; the same concern was already raised earlier in review history.
core/src/internal/kmeans_gpu.cpp (1)
668-668: No additional comment; same concern already captured at the root pattern.core/src/internal/bilateral_filter.cpp (1)
228-231: Please remove or gate these runtime stdout logs in the core API path.This duplicates a previously raised cleanup request and should still be addressed before release.
core/include/internal/gpu.h (1)
260-260: Good lifecycle-state reset in destructor.Resetting
gpu_initializedavoids stale initialization state after teardown.core/src/internal/labels_to_svg.cpp (1)
189-190: Formatting-only change looks good.No functional impact from this signature line-wrap.
example-apps/react-js/src/components/WasmImageProcessor.jsx (1)
84-106: ⚡ Quick winRemove the large commented legacy pipeline block.
Now that
imgToSVGis the active path, this dead block adds noise and makes the example harder to follow.[ suggest_recommended_refactor ]
As per coding guidelines,example-apps/**should remain minimal, clean, and a clear reflection of public API usage.
Comment on lines
+10
to
+17
| std::string image_to_svg(const uint8_t *data, const int width, const int height, const ImageToSvgConfig& config) { | ||
| // self deallocate | ||
| std::vector<uint8_t> img_data(width * height * 4); | ||
| std::vector<uint8_t> out_data(width * height * 4); | ||
| std::vector<int32_t> out_labels(width * height); | ||
|
|
||
| std::memcpy(img_data.data(), data, | ||
| static_cast<size_t>(width) * static_cast<size_t>(height) * 4); |
Contributor
There was a problem hiding this comment.
Validate dimensions before sizing buffers and copying.
width * height * 4 is computed in signed int here and then used for both vector sizes and memcpy. Negative or overflowing dimensions can turn into huge allocations and bogus copy lengths before the function ever reaches a recoverable error path.
Suggested change
+#include <limits>
+
std::string image_to_svg(const uint8_t *data, const int width, const int height, const ImageToSvgConfig& config) {
- // self deallocate
- std::vector<uint8_t> img_data(width * height * 4);
- std::vector<uint8_t> out_data(width * height * 4);
- std::vector<int32_t> out_labels(width * height);
+ if (data == nullptr || width <= 0 || height <= 0) {
+ throw std::invalid_argument("image_to_svg requires non-null data and positive dimensions");
+ }
+
+ const size_t pixel_count{static_cast<size_t>(width) * static_cast<size_t>(height)};
+ if (pixel_count > std::numeric_limits<size_t>::max() / 4) {
+ throw std::overflow_error("image_to_svg image dimensions overflow");
+ }
+
+ std::vector<uint8_t> img_data(pixel_count * 4);
+ std::vector<uint8_t> out_data(pixel_count * 4);
+ std::vector<int32_t> out_labels(pixel_count);
- std::memcpy(img_data.data(), data,
- static_cast<size_t>(width) * static_cast<size_t>(height) * 4);
+ std::memcpy(img_data.data(), data, pixel_count * 4);🧰 Tools
🪛 Cppcheck (2.20.0)
[style] 10-10: The function 'image_to_svg' is never used.
(unusedFunction)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/internal/image_to_svg.cpp` around lines 10 - 17, The function
image_to_svg currently allocates img_data/out_data/out_labels and calls
std::memcpy using width * height * 4 computed as signed ints, which can overflow
or be negative; validate inputs at the top of image_to_svg by checking that
width and height are positive and that the product (cast to size_t) does not
overflow when multiplied by 4 (e.g., ensure (size_t)width * (size_t)height <=
SIZE_MAX / 4), return an error or empty string on invalid dimensions, then
compute sizes into size_t variables and use those for vector sizing and for the
memcpy length to avoid huge or negative allocations and unsafe copies (refer to
image_to_svg, img_data, out_data, out_labels, and the memcpy call).
Owner
There was a problem hiding this comment.
If this is outdated, please resolve it or give an updated comment about this.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cat core/src/internal/image_to_svg.cpp
Length of output: 1117
@Ryan-Millard, the comment is still valid — the PR has made a partial improvement but the two most important safety checks are still missing.
What was fixed: The static_cast<size_t> casts were added to the multiplications, which prevents signed-integer overflow at the arithmetic site.
What is still missing:
data == nullptrcheck —std::memcpywith a null source is undefined behaviour.- Positive-dimension guard (
width <= 0 || height <= 0) — this is the critical gap. A negativeintsilently wraps to nearSIZE_MAXwhen cast tosize_t, sostatic_cast<size_t>(-1)is18446744073709551615. The vector constructors will almost certainly throwstd::bad_alloc, but the behaviour is still undefined and the failure mode is unpredictable for callers. size_toverflow guard — multiplying twosize_tvalues can itself overflow before the* 4step, though this only matters for pathologically large dimensions.
The suggested fix from the original comment still applies:
`#include` <limits>
`#include` <stdexcept>
std::string image_to_svg(const uint8_t* data, const int width, const int height,
const ImageToSvgConfig& config) {
if (data == nullptr || width <= 0 || height <= 0) {
throw std::invalid_argument("image_to_svg requires non-null data and positive dimensions");
}
const size_t pixel_count{static_cast<size_t>(width) * static_cast<size_t>(height)};
if (pixel_count > std::numeric_limits<size_t>::max() / 4) {
throw std::overflow_error("image_to_svg: image dimensions overflow");
}
std::vector<uint8_t> img_data(pixel_count * 4);
std::vector<uint8_t> out_data(pixel_count * 4);
std::vector<int32_t> out_labels(pixel_count);
std::memcpy(img_data.data(), data, pixel_count * 4);
// ... rest unchanged🧠 Learnings used
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 333
File: bindings/c/src/cimg2num.cpp:0-0
Timestamp: 2026-04-27T15:40:33.329Z
Learning: In the Ryan-Millard/Img2Num project, `bindings/c/src/cimg2num.cpp` contains `extern "C"` C bindings. Do NOT suggest throwing C++ exceptions (e.g., `std::bad_alloc`) from within these C binding functions, even from lambdas passed internally to `clear_last_error_and_catch`. The correct C-API idiom is to return `nullptr` (or another sentinel) on failure and let the caller check the return value.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-25T16:20:34.359Z
Learning: In the Ryan-Millard/Img2Num repository, the following follow-up issues were created after merging PR `#307` (Python bindings): `#328` (Python docstrings/docs), `#329` (numpy img.shape inference for width/height), `#330` (.coderabbit.yaml Python tooling config), `#331` (CI workflow for Python bindings build test).
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-25T16:31:13.204Z
Learning: In the Ryan-Millard/Img2Num repository, issue `#331` (CI workflow for Python bindings build test) was addressed in PR `#307` via the `build-py` job in `.github/workflows/cmake-build.yml`. That job runs `uv venv`, `uv sync --no-build-isolation`, a smoke test (`uv run python3 -c "import img2num;"`), and uploads artifacts.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-23T18:48:02.692Z
Learning: In the Ryan-Millard/Img2Num repository, PR `#311` adds `.coderabbit.yaml` to main. PR `#307` (adding Python .py files) must rebase onto main after `#311` merges so that `.coderabbit.yaml` exists and can be updated with: `ruff: enabled: true`, `pylint: enabled: true`, `flake8: enabled: false` under `tools:`, and a `path_instructions` entry for `**/*.py` covering PEP 8, type annotations, exception handling, resource management, mutable default arguments, and Docker-first command suggestions via the `./img2num` wrapper. The "CodeRabbit Config Needs Update" custom check will automatically flag `#307` if the Python config is missing.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-23T18:48:02.712Z
Learning: In the Ryan-Millard/Img2Num repository, PR `#311` adds `.coderabbit.yaml` to main. PR `#307` (adding Python .py files) must rebase onto main after `#311` merges so that `.coderabbit.yaml` exists and can be updated with: `ruff: enabled: true`, `pylint: enabled: true`, `flake8: enabled: false` under `tools:`, and a `path_instructions` entry for `**/*.py` covering PEP 8, type annotations, exception handling, resource management, mutable default arguments, and Docker-first command suggestions via the `./img2num` wrapper. The "CodeRabbit Config Needs Update" custom check will automatically flag `#307` if the Python config is missing.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-01T20:45:20.732Z
Learning: In the Ryan-Millard/Img2Num repository, PRs that introduce documentation or code examples referencing functions or symbols not present in the codebase (e.g., a `vectorize` function from `img2num`) should be flagged as potential AI-generated slop. Always cross-reference newly introduced API names/symbols against actual codebase exports.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-05-01T00:15:18.172Z
Learning: In Ryan-Millard/Img2Num, the correct release propagation strategy uses TWO GitHub Actions jobs added to release.yml:
1. `propagate-core`: fires when `core_created == 'true'` → creates separate PRs (not direct commits) for bindings/c, packages/js, and packages/py using `git commit --allow-empty` with conventional commit messages and `gh pr create`. Requires RELEASE_PROPAGATION_TOKEN.
2. `propagate-c`: fires when `c_created == 'true' AND core_created != 'true'` → creates a PR for packages/js only (the guard prevents double-bumping JS when core already propagated to it in the same run).
Dependency graph: core → {bindings/c, packages/js, packages/py}; bindings/c → {packages/js}. packages/py depends directly on core (not via bindings/c). PRs must be created (not direct commits) so Ryan-Millard can review each propagation before merging.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-23T18:35:01.229Z
Learning: In the Ryan-Millard/Img2Num repository, Python (.py files) is being added to the codebase via PR `#307`. The `.coderabbit.yaml` should include `ruff: enabled: true` and `pylint: enabled: true` under `tools:`, and a `path_instructions` entry for `**/*.py` covering PEP 8, type annotations, exception handling, resource management, mutable default arguments, and Docker-first command suggestions via the `./img2num` wrapper.
Learnt from: fransafu
Repo: Ryan-Millard/Img2Num PR: 176
File: src/wasm/modules/image/src/bilateral_filter.cpp:98-101
Timestamp: 2025-12-31T17:46:54.476Z
Learning: In bilateral_filter.cpp (src/wasm/modules/image/src/bilateral_filter.cpp), rely on the fact that the center pixel contributes a weight of exactly 1.0 to both spatial and range components (exp(0) = 1) so the normalization sum cannot be zero. This implies you do not need explicit guards against division by zero for normalization in this implementation.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 191
File: src/wasm/modules/image/src/kmeans.cpp:128-134
Timestamp: 2026-01-06T04:56:57.269Z
Learning: In C++ sources compiled for WASM with Emscripten/Clang, designated initializers (e.g., RGBXY{.r = ..., .g = ...}) are allowed as a C++17 extension. When reviewing code that relies on designated initializers, verify that the target toolchain enables CXX_STANDARD 17 or higher and that the build system (CMake/emsdk) uses Emscripten with a compatible clang. If not, avoid such initializers or provide portable alternatives.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 191
File: src/wasm/modules/image/src/cielab.cpp:43-57
Timestamp: 2026-01-06T21:06:24.476Z
Learning: In the Img2Num project, prefer recommending and using the provided docker/script wrappers (e.g., ./img2num format-wasm, ./img2num clean-wasm) instead of invoking local tools directly (e.g., clang-format -i). This reduces dependency requirements for users and ensures consistent tooling across environments. Apply this guidance to C++ source files under the project when reviewing changes.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 234
File: src/wasm/modules/image/src/kmeans.cpp:79-81
Timestamp: 2026-01-19T00:02:34.957Z
Learning: In C++ multithreading contexts, prefer unsigned types for thread-count-like parameters (e.g., n_threads) to prevent negative values. Validate that the value is at least 1 before any division to avoid divide-by-zero at runtime. At the start of functions handling such values, create a safe count like: const unsigned int thread_count{std::max(1u, n_threads)}; This ensures non-negative, non-zero usage for divisions and related arithmetic. Apply this pattern to similar parameters across C++ modules, not just the specific file.
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 272
File: core/src/internal/bilateral_filter_gpu.cpp:171-185
Timestamp: 2026-02-25T21:24:19.036Z
Learning: Adopt brace initialization (e.g., Type var{}) over copy initialization (e.g., Type var = {}) for zero-initialization in C++ across the repository. This should apply to variables, arrays, and structs in most C++ files (e.g., core/src/internal/bilateral_filter_gpu.cpp). It improves safety by avoiding narrowing conversions and makes initialization intent explicit.
|
|
||
| img2num::ImageToSvgConfig config = img2num::IMAGE_TO_SVG_DEFAULT_CONFIG; | ||
| config.kmeans.k = 32; | ||
| std::string res_svg2{img2num::image_to_svg(img_data, width, height, config)}; |
Contributor
There was a problem hiding this comment.
Use the original image buffer for image_to_svg in this example.
img_data is already modified by the manual bilateral/kmeans path, so this no longer demonstrates the unified API on raw input as intended.
Proposed fix
- std::string res_svg2{img2num::image_to_svg(img_data, width, height, config)};
+ std::string res_svg2{img2num::image_to_svg(image_data_original, width, height, config)};As per coding guidelines, example-apps/** should correctly reflect public API usage with clean, minimal examples.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@example-apps/console-cpp/main.cpp` at line 69, The call to
img2num::image_to_svg is using img_data which has been modified by the manual
bilateral/kmeans path; change it to use the original unmodified image buffer
(e.g., save the raw input into a separate variable before applying
bilateral/kmeans and pass that raw buffer to img2num::image_to_svg) so the
example demonstrates the unified API on raw input; ensure references to
image_to_svg and the bilateral/kmeans modification code use the preserved
original buffer variable rather than the mutated img_data.
5 tasks
Owner
|
@Krasner, please see the review I previously left - some of those things are still relevant. Additionally, please clean up the old code that was commented out and update the C example app and ensure that the C bindings work (they currently cause the error below). root@8c5abede0223:/usr/src/app# cmake --build build
No changes in /usr/src/app/build/embedded_shaders.h, skipping write
[ 0%] Built target Img2Num_shaders
[ 68%] Built target Img2Num
[ 81%] Built target CImg2Num
[ 90%] Built target console_cpp_app
[ 95%] Building C object example-apps/console-c/CMakeFiles/console_c_app.dir/main.c.o
/usr/src/app/example-apps/console-c/main.c: In function 'main':
/usr/src/app/example-apps/console-c/main.c:55:5: error: variable 'config' has initializer but incomplete type
55 | ImageToSvgConfig config = {0};
| ^~~~~~~~~~~~~~~~
/usr/src/app/example-apps/console-c/main.c:55:32: warning: excess elements in struct initializer
55 | ImageToSvgConfig config = {0};
| ^
/usr/src/app/example-apps/console-c/main.c:55:32: note: (near initialization for 'config')
/usr/src/app/example-apps/console-c/main.c:55:22: error: storage size of 'config' isn't known
55 | ImageToSvgConfig config = {0};
| ^~~~~~
gmake[2]: *** [example-apps/console-c/CMakeFiles/console_c_app.dir/build.make:79: example-apps/console-c/CMakeFiles/console_c_app.dir/main.c.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:307: example-apps/console-c/CMakeFiles/console_c_app.dir/all] Error 2
gmake: *** [Makefile:136: all] Error 2 |
Collaborator
Author
good catch. let me see what's going on there |
Owner
|
Hi @Krasner. Please rather prefer separate PRs for each task you do. For example, the transition from C++ to C-only for This will become very important when we have our first release (which I plan to have once this PR is merged) because a commit like this will break the Conventional Commit and SemVer standards that the people who use our library will expect us to follow. Also, if you introduced a dangerous bug in the Keeping it smaller makes it easier to review and spot bugs, too. Moving foward, please try to keep your pull requests simple - especially when it comes to things that will affect users. |
Owner
Nobody likes documentation - that's why we basically have none. The stuff we have is pretty much all outdated. |
Collaborator
Author
|
@Ryan-Millard what's the status here? |
Ryan-Millard
previously approved these changes
May 11, 2026
Owner
Sorry for the late reply. I was away over the weekend. Everything looks good. I'm just going to clean up a few things, then I'll merge it. |
Owner
|
@Krasner, my latest commit clearly separates our logic:
This separation of concerns protects us from overly scary-looking bindings (C++ and Pybind11 are very verbose and unreadable for the untrained eye, whereas Python is borderline pseudocode). |
Collaborator
Author
Oh yeah that's a much better solution :) |
Ryan-Millard
changed the title
Owner
|
@coderabbitai please make a few issues here:
DO NOT BE LAZY @coderabbitai!!!! |
This was referenced May 11, 2026
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 413 All 6 sub-issues created. Now creating the parent documentation tracking issue and the Python API cleanup issue. 🏁 Script executed: Length of output: 138
📄 Documentation — Parent tracking issue
📄 Documentation — Sub-issues
🐍 Python API cleanup
All issues are assigned to you. |
Owner
Owner
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
2 participants
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.
Combines
bilateral_filter,kmeansandlabel_to_svginto one unifiedimage_to_svgfunction.Updated js and py bindings
What was changed & why
Fixes: #318
Fixes: #360
Fixes: #361
Changes
Testing & Verification
Additional Resources