Feat/multithread - #232
Conversation
📝 WalkthroughWalkthroughAdded documentation and WASM/Vite tooling pages; replaced mergeSmallRegionsInPlace with a graph+contours-based pipeline (new Node/Graph/kmeans_graph/contours C++ APIs), made bilateral filter and kmeans multi-threaded, updated JS worker and hook exports to use findContours and label outputs, removed legacy mergeSmallRegionsInPlace implementation and tests, and added security headers to Vite dev server. Changes
Sequence Diagram(s)sequenceDiagram
participant React as React (WasmImageProcessor)
participant Hook as useWasmWorker Hook
participant Worker as WASM Worker (WebWorker)
participant WASM as Image WASM module (C++)
participant Graph as Graph/Contours subsystems (C++)
React->>Hook: requestProcess(image)
Hook->>Worker: postMessage({ cmd: 'bilateralFilter', ... })
Worker->>WASM: call exported bilateral_filter(..., n_threads)
WASM-->>Worker: returns filtered pixels
Hook->>Worker: postMessage({ cmd: 'kmeans', pixels, out_buffers })
Worker->>WASM: call exported kmeans(..., out_data, out_labels, n_threads)
WASM-->>Worker: returns { out_data, out_labels }
Hook->>Worker: postMessage({ cmd: 'kmeans_clustering_graph', data, labels, min_area, draw_contours })
Worker->>WASM: call exported kmeans_clustering_graph(...)
WASM->>Graph: build graph, discover_edges, merge_small_area_nodes
Graph->>WASM: returns merged regions + contours
WASM-->>Worker: returns final RGBA and contours
Worker-->>Hook: postMessage(result)
Hook-->>React: deliver processed pixels + contours/labels
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
continues from #219 - check last commit |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/wasm/modules/image/src/bilateral_filter.cpp (1)
107-109: Unconditional CIELAB lookup even in RGB mode.Lines 107-109 read from
cie_imageregardless ofcolor_space. In RGB mode,cie_imageis empty, causing undefined behavior (reading from empty vector).🐛 Proposed fix - conditionally access cie_image
- double L{cie_image[neighbor_idx]}; - double A{cie_image[neighbor_idx + 1]}; - double B{cie_image[neighbor_idx + 2]}; + double L{0}, A{0}, B{0}; + if (color_space == COLOR_SPACE_OPTION_CIELAB) { + L = cie_image[neighbor_idx]; + A = cie_image[neighbor_idx + 1]; + B = cie_image[neighbor_idx + 2]; + }src/wasm/modules/image/src/kmeans.cpp (1)
207-212: Alpha channel not set in output.Unlike
kmeans(line 100) which explicitly sets alpha to 255, this function doesn't write the alpha channel. If the input data has non-opaque alpha values or if the buffer is newly allocated, the output may have incorrect transparency. This inconsistency could be the "poorly indexed channel" bug referenced in the comment at line 108.Proposed fix
for (int32_t i = 0; i < num_pixels; ++i) { int32_t cluster = labels[i]; data[i * 4 + 0] = static_cast<uint8_t>(centroids[cluster].r * 255); data[i * 4 + 1] = static_cast<uint8_t>(centroids[cluster].g * 255); data[i * 4 + 2] = static_cast<uint8_t>(centroids[cluster].b * 255); + data[i * 4 + 3] = 255; }
🤖 Fix all issues with AI agents
In `@docs/docs/reference/react/hooks/useWasmWorker/index.md`:
- Around line 73-95: Fix the malformed Markdown code fence inside the TabItem
that contains the C++ snippet: remove the extra indentation before the
declaration and replace the orphaned closing backticks so the block starts with
a proper fenced code block label (cpp) and ends with four backticks that close
the outer fenced region; specifically adjust the block containing EXPORTED void
add_arrays(int* a, int* b, int* out, int length); and ensure the TabItem/ Tabs
wrappers remain balanced so the following TabItem value="emscripten-method"
renders correctly.
In `@docs/docs/reference/wasm/modules/cmake-root-orchestrator.md`:
- Around line 147-148: The diagram/text claiming "Hot-reload WASM modules on
source changes" is misleading; update the copy near nodes K/L (the "Hot-reload
WASM with dev server" label) to clarify that Vite HMR only hot-reloads the
JavaScript integration layer and that native C/C++ -> WASM artifacts must be
rebuilt via CMake manually (e.g., rephrase to "Hot-reload JS integration with
dev server — rebuild WASM via CMake on source changes" or similar).
- Line 162: Update the explanatory text for the "**Development:** Hot-reloads
WASM modules on source changes." bullet to include the same clarification as
elsewhere: define what "hot-reloads" means (e.g., it rebuilds the module and
replaces the running instance without restarting the whole orchestrator), state
whether module state is preserved or reset, and note how to enable/disable this
behavior (matching the phrasing used in the other explanation). Target the
sentence containing "**Development:** Hot-reloads WASM modules on source
changes." to ensure consistency and clarity.
In `@src/wasm/modules/image/include/bilateral_filter.h`:
- Around line 18-20: In bilateral_filter, add validation to clamp the n_threads
parameter into a safe range to avoid division by zero and unsigned loop issues:
compute a max_threads = std::thread::hardware_concurrency() (fallback to 1 if it
returns 0), then set n_threads = std::max(1, std::min(n_threads,
static_cast<int>(max_threads))); use this validated n_threads when computing
rows_per_thread and any loop bounds (ensure casts to signed/size_t are
deliberate), and replace any unsigned loop comparisons that rely on possibly
negative n_threads with iterations based on the validated positive n_threads to
prevent undefined behavior.
In `@src/wasm/modules/image/include/graph.h`:
- Line 54: The size() method in graph.h currently declares the return type as
const (inline const size_t size()) which is meaningless for a value type; change
the signature of the Graph::size() method to put the const qualifier on the
method instead (e.g., inline size_t size() const) so it is a const-qualified
accessor; update the declaration/definition of size() in graph.h to remove the
leading const on the return type and append const after the parameter list to
ensure const-correctness.
In `@src/wasm/modules/image/src/bilateral_filter.cpp`:
- Line 29: The global std::mutex write_mutex and any locking in _process should
be removed because threads write disjoint ranges (use start_row/end_row) into
the result buffer; locate the write_mutex declaration and delete it, then remove
the lock_guard/lock/unlock usage inside the _process function (and any includes
only used for the mutex) so that each worker thread writes directly to result
for its assigned rows without synchronization.
- Line 191: The current calculation int rows_per_thread =
static_cast<int>(height) / n_threads can yield zero when n_threads > height;
clamp the thread count first (e.g., used_threads =
std::min(static_cast<int>(height), n_threads)) and then compute rows_per_thread
= static_cast<int>(height) / used_threads so each thread gets at least one row;
update all uses of n_threads in the scheduling logic (e.g., where
rows_per_thread and loop bounds are used) to use used_threads to avoid
division-by-zero and incorrect distribution.
In `@src/wasm/modules/image/src/graph.cpp`:
- Around line 120-147: In merge_small_area_nodes, the neighbors vector is
incorrectly initialized with size n->num_edges() and then appended to (creating
default elements) and the code can index past the end if all neighbor areas are
zero; fix by creating neighbors as an empty vector and calling
neighbors.reserve(n->num_edges()) before copying edges, then replace the manual
idx loop with a safe search (e.g., std::find_if on neighbors for area()>0) and
if no such neighbor exists, skip this node (continue) or otherwise handle the
case without accessing neighbors[idx]; update references in this function
(neighbors, n, merge_nodes, get_nodes) accordingly.
In `@src/wasm/modules/image/src/kmeans_graph.cpp`:
- Around line 143-146: The multiplication width * height in
kmeans_clustering_graph is done as int and assigned to int32_t num_pixels,
risking integer overflow for large images; change the calculation to perform the
multiplication in an unsigned wider type (e.g., size_t) by casting operands
before multiplying and assign to a size_t (or uint64_t) variable (e.g., size_t
num_pixels = static_cast<size_t>(width) * static_cast<size_t>(height)); also
validate width/height are non-negative and optionally check that num_pixels fits
expected bounds before later casts/uses to prevent silent overflow.
In `@src/wasm/modules/image/src/kmeans.cpp`:
- Around line 1-9: Add the missing <cstring> include so std::memcpy is declared:
update the include block at the top of kmeans.cpp (near the existing includes in
this file) to add `#include` <cstring>; this fixes the use of std::memcpy
(referenced in the code around the memcpy call) and avoids relying on transitive
includes.
In `@src/wasm/modules/image/src/node.cpp`:
- Around line 22-39: The Node::color method divides by m_pixels_size without
guarding against an empty m_pixels, risking division by zero; update Node::color
to check that m_pixels is non-null and not empty (e.g., if m_pixels->empty() or
m_pixels_size == 0) before performing the average, and return a sensible default
color (such as {0,0,0}) or handle the empty case similarly to centroid() to
avoid the division; reference m_pixels, m_pixels_size and Node::color when
making the change.
- Around line 8-20: Node::centroid computes an average without guarding against
an empty m_pixels, causing division by zero; update Node::centroid to check
m_pixels->empty() (or m_pixels_size == 0) before dividing and return a safe
default XY (e.g., {0,0} or another agreed sentinel) or handle the empty case
appropriately, ensuring all references to m_pixels and the centroid calculation
(centroid.x/centroid.y and m_pixels_size) are skipped when empty.
- Around line 41-65: In Node::bounding_box_xywh(), x_max and y_max are
initialized to 0 which yields incorrect boxes when all pixel coordinates are
negative; change their initialization to INT_MIN (matching x_min/y_min using
INT_MAX) so the loop correctly computes extremes for negative coordinates and
the returned x,y,w,h are valid.
In `@vite.config.js`:
- Around line 116-126: Prettier formatting is off in vite.config.js; run the
formatter and ensure the code around the plugin named "force-security-headers"
(the configureServer function that calls server.middlewares.use and sets
Cross-Origin-Opener-Policy and Cross-Origin-Embedder-Policy headers) is properly
formatted—run `prettier --write vite.config.js` and commit the reformatted file
so the configureServer middleware block and surrounding server.headers remain
consistently styled.
🧹 Nitpick comments (18)
src/wasm/modules/image/include/bilateral_filter.h (1)
9-20: Add documentation for the newn_threadsparameter.The comment block documents all parameters except the newly added
n_threads. Consider adding:// - sigma_range: Gaussian standard deviation for intensity difference // (radiometric decay) +// - n_threads: Number of threads for parallel processing (default: 8) void bilateral_filter(uint8_t *image, size_t width, size_t height,src/wasm/modules/image/CMakeLists.txt (1)
45-47: Consider makingPTHREAD_POOL_SIZEconfigurable via CMake variable.The hardcoded value of
8matches the default inbilateral_filter.hbut is inflexible across deployment environments. Best practices recommend matching pool size to actual concurrent thread usage or device capabilities (vianavigator.hardwareConcurrency). Each precreated thread consumes memory and stack; oversizing can increase page memory overhead on memory-constrained devices (mobile) while undersizing may under-utilize multi-core machines.Consider:
- Exposing
PTHREAD_POOL_SIZEas a CMake variable with a sensible default (e.g., 4–8 depending on target).- Using
-sPTHREAD_POOL_DELAY_LOAD=1to defer worker creation and avoid startup blocking.Also verify your deployment environment supports
SharedArrayBuffer(requiresCross-Origin-Opener-PolicyandCross-Origin-Embedder-Policyheaders in secure contexts).docs/docs/reference/wasm/modules/image/cmake.md (1)
76-104: Documentation is missing the new pthread/multithreading flags.According to the AI summary, the actual
CMakeLists.txtnow includes pthread-related flags (USE_PTHREADS=1,PTHREAD_POOL_SIZE=8,-pthread) to enable multithreading for the bilateral filter. This documentation should be updated to reflect those additions.Consider adding a section documenting:
-pthreadcompile/link flagsUSE_PTHREADS=1PTHREAD_POOL_SIZE=8docs/docs/reference/react/vite-config.md (2)
122-136: Documentation is missing the new security headers for multithreading.The actual
vite.config.jsnow includesserver.headerswithCross-Origin-Opener-PolicyandCross-Origin-Embedder-Policyheaders required for SharedArrayBuffer/pthread support. This section should be updated to document these headers.Suggested addition
### Security Headers (Multithreading) ```javascript server: { headers: { "Cross-Origin-Opener-Policy": "same-origin", "Cross-Origin-Embedder-Policy": "require-corp", }, },
- Required for
SharedArrayBuffersupport in browsers.- Enables pthread-based multithreading in WASM modules.
</details> --- `197-241`: **Missing documentation for the `force-security-headers` plugin.** The actual `vite.config.js` includes a new plugin named `force-security-headers` that applies COOP/COEP headers via middleware. This plugin should be documented alongside the other custom plugins for completeness. </blockquote></details> <details> <summary>src/wasm/modules/image/src/contours.cpp (1)</summary><blockquote> `112-119`: **Consider using `get` instead of `set` for reading pixel values.** At line 116, `set(y3, x3) == 1` works because `set` returns `int&`, but semantically this is a read operation. Using `get(y3, x3) == 1` would be clearer and more consistent with the rest of the code. <details> <summary>Suggested change</summary> ```diff if (rightZeroExamined) { set(y3, x3) = -nbd; } else { - if (set(y3, x3) == 1) { + if (get(y3, x3) == 1) { set(y3, x3) = nbd; } }src/wasm/modules/image/include/contours.h (1)
4-9: Consider removing unused includes from the header.The includes
<cmath>and<cstdlib>are not used by any declarations in this header. They appear to be needed only by the implementation incontours.cpp. Moving them to the.cppfile would reduce header dependencies and improve compile times.Suggested change
`#include` <array> -#include <cmath> `#include` <cstdint> -#include <cstdlib> `#include` <stdexcept> `#include` <vector>The static analysis error about
'array' file not foundis a false positive—standard library headers are available in the Emscripten toolchain.src/wasm/modules/image/include/kmeans.h (1)
8-10: Type mismatch with implementation.The header declares
width,height,k,max_iterasint, but the implementation inkmeans.cppusesint32_t. While these are typically identical, using consistent types avoids subtle ABI or toolchain issues.🔧 Suggested fix for consistency
-EXPORTED void kmeans(const uint8_t *data, uint8_t *out_data, int *out_labels, - const int width, const int height, const int k, - const int max_iter); +EXPORTED void kmeans(const uint8_t *data, uint8_t *out_data, int *out_labels, + const int32_t width, const int32_t height, const int32_t k, + const int32_t max_iter);src/wasm/modules/image/src/kmeans_graph.cpp (3)
1-3: Acknowledge the TODO; consider tracking.The TODO about alpha channel handling is noted. This could lead to unexpected behavior for images with transparency. Consider creating an issue to track this.
Would you like me to open an issue to track the alpha channel handling improvement?
92-96: Unused variablecounts.The variable
countsreturned fromflood_fillis computed but never used. Thenum_pixelsvariable is also computed separately but unused.🧹 Remove unused variables
- int counts = flood_fill(labels, regions, data, i, j, label, r_lbl, + flood_fill(labels, regions, data, i, j, label, r_lbl, width, height, p_ptr); - int num_pixels = p_ptr->size(); - // num_pixels == counts always Node_ptr n_ptr = std::make_shared<Node>(r_lbl, p_ptr);
102-109: Static RNG may cause non-reproducible results across calls.Using
staticforrnganddistmeans the state persists across calls tovisualize_contours. This could be intentional for variety, but may cause non-deterministic debugging. If reproducibility is needed, consider seeding with a fixed value or making it non-static.src/hooks/useWasmWorker.js (2)
45-49: Consider dynamically determining thread count.The default
n_threads = 8may not be optimal for all systems. Consider usingnavigator.hardwareConcurrencyto determine available cores, with a fallback.♻️ Suggested improvement
const bilateralFilter = async ({ pixels, width, height, sigma_spatial = 3.0, sigma_range = 50.0, color_space = 0, - n_threads = 8, + n_threads = navigator.hardwareConcurrency || 4, }) => {
75-82: Function namefindContoursmay be misleading.The function calls
kmeans_clustering_graphwhich performs region merging and optionally draws contours. The namefindContourssuggests only contour detection. Consider renaming to better reflect its purpose (e.g.,processRegionsWithContoursorkmeansClusteringGraph).src/wasm/modules/image/src/bilateral_filter.cpp (1)
49-63: Parametern_threadsis unused in_process.The
n_threadsparameter is passed to_processbut never used within the function.🧹 Remove unused parameter
void _process( const uint8_t* image, const std::vector<double>& cie_image, std::vector<uint8_t>& result, const std::vector<double>& spatial_weights, const std::vector<double>& range_lut, int radius, double sigma_range, int start_row, int end_row, size_t height, size_t width, - uint8_t color_space, - int n_threads + uint8_t color_space ) {And update call sites accordingly.
src/wasm/modules/image/include/node.h (1)
4-4: Use quotes for project-local headers.Static analysis indicates
RGBPixel.hshould use quotes ("RGBPixel.h") instead of angle brackets (<RGBPixel.h>) since it's a project header, not a system header. Angle brackets search system include paths first.♻️ Proposed fix
-#include <RGBPixel.h> +#include "RGBPixel.h"src/wasm/modules/image/src/graph.cpp (2)
9-14: Fix incorrect complexity comment.The comment states
std::unordered_maplookup isO(log(N)), but it's actuallyO(1)average case (hash table).O(log(N))would be forstd::map(balanced tree).📝 Fix comment
/* *To quickly search m_nodes (std::vector) for the index of a node id *create an std::unordered_map of node id - index pairs *indexing time of std::vector by value is O(N) - *lookup time of std::unordered_map by key is O(log(N)) + *lookup time of std::unordered_map by key is O(1) average */
15-20: Potential signed/unsigned comparison warning.Comparing
int32_t iwithm_nodes->size()(which returnssize_t) may produce a compiler warning on some platforms.♻️ Use consistent types
void Graph::hash_node_ids() { - for (int32_t i{0}; i < m_nodes->size(); i++) { + for (size_t i{0}; i < m_nodes->size(); ++i) { const int32_t key{m_nodes->at(i)->id()}; - m_node_ids[key] = i; + m_node_ids[key] = static_cast<int32_t>(i); } }src/wasm/modules/image/src/kmeans.cpp (1)
29-29: Consider using consistent integer types.
labelsandcountsvectors useint, while loop indices and other variables useint32_t. While these are typically equivalent, usingint32_tconsistently would improve clarity and ensure portability.Proposed fix
- std::vector<int> labels(num_pixels, 0); + std::vector<int32_t> labels(num_pixels, 0);- std::vector<int> counts(k, 0); + std::vector<int32_t> counts(k, 0);Also applies to: 71-71
| ````cpp | ||
| #include "exported.h" | ||
|
|
||
| EXPORTED void add_arrays(int* a, int* b, int* out, int length); | ||
| ``` | ||
|
|
||
| </TabItem> | ||
|
|
||
| <TabItem value="emscripten-method" label="Emscripten Method"> | ||
| :::caution Don't use this method in Img2Num! | ||
| This method leads to bloat in the CMake file, is difficult to maintain and tough to debug. | ||
|
|
||
| Use the Img2Num method instead because it provides convenience, reduces boilerplate, and saves you from numerous possible bugs. | ||
| ::: | ||
| Add the function to the CMake file's `EXPORTED_FUNCTIONS` flag: | ||
| ```cmake | ||
| "SHELL:-s EXPORTED_FUNCTIONS=['_malloc','_free','_add_arrays']" | ||
| ``` | ||
|
|
||
| </TabItem> | ||
| </Tabs> | ||
|
|
||
| ```` |
There was a problem hiding this comment.
Markdown formatting issue in code block.
The code block within the TabItem has indentation issues and orphaned backticks that may cause rendering problems:
- Lines 73-77: The code block content is incorrectly indented inside the
TabItem - Line 95: Orphaned closing backticks
🔧 Suggested fix
````cpp
`#include` "exported.h"
- EXPORTED void add_arrays(int* a, int* b, int* out, int length);
- ```
+ EXPORTED void add_arrays(int* a, int* b, int* out, int length);
+ ````
</TabItem>
<TabItem value="emscripten-method" label="Emscripten Method">
:::caution Don't use this method in Img2Num!
This method leads to bloat in the CMake file, is difficult to maintain and tough to debug.
Use the Img2Num method instead because it provides convenience, reduces boilerplate, and saves you from numerous possible bugs.
:::
Add the function to the CMake file's `EXPORTED_FUNCTIONS` flag:
```cmake
"SHELL:-s EXPORTED_FUNCTIONS=['_malloc','_free','_add_arrays']"
```
</TabItem>
</Tabs>
-
- ````📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ````cpp | |
| #include "exported.h" | |
| EXPORTED void add_arrays(int* a, int* b, int* out, int length); | |
| ``` | |
| </TabItem> | |
| <TabItem value="emscripten-method" label="Emscripten Method"> | |
| :::caution Don't use this method in Img2Num! | |
| This method leads to bloat in the CMake file, is difficult to maintain and tough to debug. | |
| Use the Img2Num method instead because it provides convenience, reduces boilerplate, and saves you from numerous possible bugs. | |
| ::: | |
| Add the function to the CMake file's `EXPORTED_FUNCTIONS` flag: | |
| ```cmake | |
| "SHELL:-s EXPORTED_FUNCTIONS=['_malloc','_free','_add_arrays']" | |
| ``` | |
| </TabItem> | |
| </Tabs> | |
| ```` |
🤖 Prompt for AI Agents
In `@docs/docs/reference/react/hooks/useWasmWorker/index.md` around lines 73 - 95,
Fix the malformed Markdown code fence inside the TabItem that contains the C++
snippet: remove the extra indentation before the declaration and replace the
orphaned closing backticks so the block starts with a proper fenced code block
label (cpp) and ends with four backticks that close the outer fenced region;
specifically adjust the block containing EXPORTED void add_arrays(int* a, int*
b, int* out, int length); and ensure the TabItem/ Tabs wrappers remain balanced
so the following TabItem value="emscripten-method" renders correctly.
| K -->|Development| L["Hot-reload WASM with dev server"] | ||
| K -->|Production| M["Use prebuilt WASM modules"] |
There was a problem hiding this comment.
Clarify the hot-reload claim for WASM modules.
The statement "Hot-reload WASM modules on source changes" may be misleading. Vite's HMR doesn't automatically rebuild C++ source to WASM on changes—developers must manually rerun the CMake build. Consider rephrasing to clarify that hot-reload applies to the JavaScript integration layer, while WASM modules require manual rebuilds.
- K -->|Development| L["Hot-reload WASM with dev server"]
+ K -->|Development| L["Serve rebuilt WASM via dev server"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| K -->|Development| L["Hot-reload WASM with dev server"] | |
| K -->|Production| M["Use prebuilt WASM modules"] | |
| K -->|Development| L["Serve rebuilt WASM via dev server"] | |
| K -->|Production| M["Use prebuilt WASM modules"] |
🤖 Prompt for AI Agents
In `@docs/docs/reference/wasm/modules/cmake-root-orchestrator.md` around lines 147
- 148, The diagram/text claiming "Hot-reload WASM modules on source changes" is
misleading; update the copy near nodes K/L (the "Hot-reload WASM with dev
server" label) to clarify that Vite HMR only hot-reloads the JavaScript
integration layer and that native C/C++ -> WASM artifacts must be rebuilt via
CMake manually (e.g., rephrase to "Hot-reload JS integration with dev server —
rebuild WASM via CMake on source changes" or similar).
| 5. **Compilation:** Each module is added via `add_subdirectory` and compiled to WASM. | ||
| 6. **WASM Output:** Compiled files go to `module/build`. | ||
| 7. **Vite Integration:** | ||
| - **Development:** Hot-reloads WASM modules on source changes. |
There was a problem hiding this comment.
Same clarification needed in the explanation.
- - **Development:** Hot-reloads WASM modules on source changes.
+ - **Development:** Serves WASM modules via dev server after rebuild.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **Development:** Hot-reloads WASM modules on source changes. | |
| - **Development:** Serves WASM modules via dev server after rebuild. |
🤖 Prompt for AI Agents
In `@docs/docs/reference/wasm/modules/cmake-root-orchestrator.md` at line 162,
Update the explanatory text for the "**Development:** Hot-reloads WASM modules
on source changes." bullet to include the same clarification as elsewhere:
define what "hot-reloads" means (e.g., it rebuilds the module and replaces the
running instance without restarting the whole orchestrator), state whether
module state is preserved or reset, and note how to enable/disable this behavior
(matching the phrasing used in the other explanation). Target the sentence
containing "**Development:** Hot-reloads WASM modules on source changes." to
ensure consistency and clarity.
| void bilateral_filter(uint8_t *image, size_t width, size_t height, | ||
| double sigma_spatial, double sigma_range, | ||
| uint8_t color_space); | ||
| uint8_t color_space, int n_threads=8); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for n_threads validation in the codebase
rg -n -A5 'n_threads' --type=cppRepository: Ryan-Millard/Img2Num
Length of output: 4313
Add validation for n_threads parameter to prevent undefined behavior.
The function does not validate n_threads, which allows negative values that cause issues: division by zero at line 191 (rows_per_thread = height / n_threads), and undefined behavior in the unsigned loop comparison at line 248. Clamp n_threads to a valid range (e.g., [1, std::thread::hardware_concurrency()]) in the validation block at the start of the function.
🤖 Prompt for AI Agents
In `@src/wasm/modules/image/include/bilateral_filter.h` around lines 18 - 20, In
bilateral_filter, add validation to clamp the n_threads parameter into a safe
range to avoid division by zero and unsigned loop issues: compute a max_threads
= std::thread::hardware_concurrency() (fallback to 1 if it returns 0), then set
n_threads = std::max(1, std::min(n_threads, static_cast<int>(max_threads))); use
this validated n_threads when computing rows_per_thread and any loop bounds
(ensure casts to signed/size_t are deliberate), and replace any unsigned loop
comparisons that rely on possibly negative n_threads with iterations based on
the validated positive n_threads to prevent undefined behavior.
| inline const std::vector<Node_ptr> &get_nodes() const { return *m_nodes; } | ||
|
|
||
| bool all_areas_bigger_than(int32_t min_area); | ||
| inline const size_t size() { return m_nodes->size(); } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix const-correctness on size() method.
The const qualifier should be on the method (after the parameter list), not on the return type. const size_t as a return type is meaningless for value types.
♻️ Proposed fix
- inline const size_t size() { return m_nodes->size(); }
+ inline size_t size() const { return m_nodes->size(); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| inline const size_t size() { return m_nodes->size(); } | |
| inline size_t size() const { return m_nodes->size(); } |
🤖 Prompt for AI Agents
In `@src/wasm/modules/image/include/graph.h` at line 54, The size() method in
graph.h currently declares the return type as const (inline const size_t size())
which is meaningless for a value type; change the signature of the Graph::size()
method to put the const qualifier on the method instead (e.g., inline size_t
size() const) so it is a const-qualified accessor; update the
declaration/definition of size() in graph.h to remove the leading const on the
return type and append const after the parameter list to ensure
const-correctness.
| #include "kmeans.h" | ||
|
|
||
| float colorDistance(const RGB &a, const RGB &b) { | ||
| return std::sqrt((a.r - b.r) * (a.r - b.r) + (a.g - b.g) * (a.g - b.g) + | ||
| (a.b - b.b) * (a.b - b.b)); | ||
| } | ||
|
|
||
| float colorDistance(const ImageLib::RGBAPixel<float> &a, const RGB &b) { | ||
| return std::sqrt((a.red - b.r) * (a.red - b.r) + | ||
| (a.green - b.g) * (a.green - b.g) + | ||
| (a.blue - b.b) * (a.blue - b.b)); | ||
| } | ||
|
|
||
| float colorDistance(const ImageLib::RGBAPixel<float> &a, | ||
| const ImageLib::RGBAPixel<float> &b) { | ||
| #include "Image.h" | ||
| #include "PixelConverters.h" | ||
| #include "RGBAPixel.h" | ||
| #include <cmath> | ||
| #include <cstdlib> | ||
| #include <ctime> | ||
| #include <limits> | ||
| #include <vector> |
There was a problem hiding this comment.
Missing <cstring> header for std::memcpy.
Line 104 uses std::memcpy, but <cstring> is not included. This may compile on some toolchains due to transitive includes, but it's not guaranteed and violates the standard.
Proposed fix
`#include` <cstdlib>
+#include <cstring>
`#include` <ctime>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #include "kmeans.h" | |
| float colorDistance(const RGB &a, const RGB &b) { | |
| return std::sqrt((a.r - b.r) * (a.r - b.r) + (a.g - b.g) * (a.g - b.g) + | |
| (a.b - b.b) * (a.b - b.b)); | |
| } | |
| float colorDistance(const ImageLib::RGBAPixel<float> &a, const RGB &b) { | |
| return std::sqrt((a.red - b.r) * (a.red - b.r) + | |
| (a.green - b.g) * (a.green - b.g) + | |
| (a.blue - b.b) * (a.blue - b.b)); | |
| } | |
| float colorDistance(const ImageLib::RGBAPixel<float> &a, | |
| const ImageLib::RGBAPixel<float> &b) { | |
| #include "Image.h" | |
| #include "PixelConverters.h" | |
| #include "RGBAPixel.h" | |
| #include <cmath> | |
| #include <cstdlib> | |
| #include <ctime> | |
| #include <limits> | |
| #include <vector> | |
| `#include` "kmeans.h" | |
| `#include` "Image.h" | |
| `#include` "PixelConverters.h" | |
| `#include` "RGBAPixel.h" | |
| `#include` <cmath> | |
| `#include` <cstdlib> | |
| `#include` <cstring> | |
| `#include` <ctime> | |
| `#include` <limits> | |
| `#include` <vector> |
🧰 Tools
🪛 Clang (14.0.6)
[error] 1-1: 'kmeans.h' file not found
(clang-diagnostic-error)
🤖 Prompt for AI Agents
In `@src/wasm/modules/image/src/kmeans.cpp` around lines 1 - 9, Add the missing
<cstring> include so std::memcpy is declared: update the include block at the
top of kmeans.cpp (near the existing includes in this file) to add `#include`
<cstring>; this fixes the use of std::memcpy (referenced in the code around the
memcpy call) and avoids relying on transitive includes.
| XY Node::centroid() const { | ||
| XY centroid{0, 0}; | ||
| for (auto &[_, pos] : *m_pixels) { | ||
| centroid.x += pos.x; | ||
| centroid.y += pos.y; | ||
| } | ||
|
|
||
| const int32_t m_pixels_size{static_cast<int32_t>(m_pixels->size())}; | ||
| centroid.x /= m_pixels_size; | ||
| centroid.y /= m_pixels_size; | ||
|
|
||
| return centroid; | ||
| } |
There was a problem hiding this comment.
Division by zero if m_pixels is empty.
If m_pixels is empty, m_pixels_size will be 0, causing undefined behavior on lines 16-17.
🔧 Suggested fix
XY Node::centroid() const {
+ if (m_pixels->empty()) {
+ return XY{0, 0};
+ }
+
XY centroid{0, 0};
for (auto &[_, pos] : *m_pixels) {
centroid.x += pos.x;
centroid.y += pos.y;
}
const int32_t m_pixels_size{static_cast<int32_t>(m_pixels->size())};
centroid.x /= m_pixels_size;
centroid.y /= m_pixels_size;
return centroid;
}🤖 Prompt for AI Agents
In `@src/wasm/modules/image/src/node.cpp` around lines 8 - 20, Node::centroid
computes an average without guarding against an empty m_pixels, causing division
by zero; update Node::centroid to check m_pixels->empty() (or m_pixels_size ==
0) before dividing and return a safe default XY (e.g., {0,0} or another agreed
sentinel) or handle the empty case appropriately, ensuring all references to
m_pixels and the centroid calculation (centroid.x/centroid.y and m_pixels_size)
are skipped when empty.
| ImageLib::RGBPixel<uint8_t> Node::color() const { | ||
| float r{0}; | ||
| float g{0}; | ||
| float b{0}; | ||
| for (auto &[color, _] : *m_pixels) { | ||
| r += color.red; | ||
| g += color.green; | ||
| b += color.blue; | ||
| } | ||
|
|
||
| const int32_t m_pixels_size{static_cast<int32_t>(m_pixels->size())}; | ||
| r /= m_pixels_size; | ||
| g /= m_pixels_size; | ||
| b /= m_pixels_size; | ||
|
|
||
| return {static_cast<uint8_t>(r), static_cast<uint8_t>(g), | ||
| static_cast<uint8_t>(b)}; | ||
| } |
There was a problem hiding this comment.
Same division by zero risk in color().
This function has the same vulnerability as centroid() when m_pixels is empty.
🔧 Suggested fix
ImageLib::RGBPixel<uint8_t> Node::color() const {
+ if (m_pixels->empty()) {
+ return {0, 0, 0};
+ }
+
float r{0};
float g{0};
float b{0};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ImageLib::RGBPixel<uint8_t> Node::color() const { | |
| float r{0}; | |
| float g{0}; | |
| float b{0}; | |
| for (auto &[color, _] : *m_pixels) { | |
| r += color.red; | |
| g += color.green; | |
| b += color.blue; | |
| } | |
| const int32_t m_pixels_size{static_cast<int32_t>(m_pixels->size())}; | |
| r /= m_pixels_size; | |
| g /= m_pixels_size; | |
| b /= m_pixels_size; | |
| return {static_cast<uint8_t>(r), static_cast<uint8_t>(g), | |
| static_cast<uint8_t>(b)}; | |
| } | |
| ImageLib::RGBPixel<uint8_t> Node::color() const { | |
| if (m_pixels->empty()) { | |
| return {0, 0, 0}; | |
| } | |
| float r{0}; | |
| float g{0}; | |
| float b{0}; | |
| for (auto &[color, _] : *m_pixels) { | |
| r += color.red; | |
| g += color.green; | |
| b += color.blue; | |
| } | |
| const int32_t m_pixels_size{static_cast<int32_t>(m_pixels->size())}; | |
| r /= m_pixels_size; | |
| g /= m_pixels_size; | |
| b /= m_pixels_size; | |
| return {static_cast<uint8_t>(r), static_cast<uint8_t>(g), | |
| static_cast<uint8_t>(b)}; | |
| } |
🤖 Prompt for AI Agents
In `@src/wasm/modules/image/src/node.cpp` around lines 22 - 39, The Node::color
method divides by m_pixels_size without guarding against an empty m_pixels,
risking division by zero; update Node::color to check that m_pixels is non-null
and not empty (e.g., if m_pixels->empty() or m_pixels_size == 0) before
performing the average, and return a sensible default color (such as {0,0,0}) or
handle the empty case similarly to centroid() to avoid the division; reference
m_pixels, m_pixels_size and Node::color when making the change.
| std::array<int32_t, 4> Node::bounding_box_xywh() const { | ||
| int32_t x_min{INT_MAX}; | ||
| int32_t y_min{INT_MAX}; | ||
| int32_t x_max{0}; | ||
| int32_t y_max{0}; | ||
| for (auto &[_, p] : *m_pixels) { | ||
| if (p.x < x_min) { | ||
| x_min = p.x; | ||
| } | ||
| if (p.x > x_max) { | ||
| x_max = p.x; | ||
| } | ||
| if (p.y < y_min) { | ||
| y_min = p.y; | ||
| } | ||
| if (p.y > y_max) { | ||
| y_max = p.y; | ||
| } | ||
| } | ||
|
|
||
| const int32_t w{x_max - x_min + 1}; | ||
| const int32_t h{y_max - y_min + 1}; | ||
|
|
||
| return std::array<int32_t, 4>{x_min, y_min, w, h}; | ||
| } |
There was a problem hiding this comment.
Bounding box computation may fail for negative coordinates.
x_max and y_max are initialized to 0, but if all pixel coordinates are negative, the computed bounding box will be incorrect. Initialize them to INT_MIN for correctness.
🔧 Suggested fix
std::array<int32_t, 4> Node::bounding_box_xywh() const {
+ if (m_pixels->empty()) {
+ return {0, 0, 0, 0};
+ }
+
int32_t x_min{INT_MAX};
int32_t y_min{INT_MAX};
- int32_t x_max{0};
- int32_t y_max{0};
+ int32_t x_max{INT_MIN};
+ int32_t y_max{INT_MIN};
for (auto &[_, p] : *m_pixels) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::array<int32_t, 4> Node::bounding_box_xywh() const { | |
| int32_t x_min{INT_MAX}; | |
| int32_t y_min{INT_MAX}; | |
| int32_t x_max{0}; | |
| int32_t y_max{0}; | |
| for (auto &[_, p] : *m_pixels) { | |
| if (p.x < x_min) { | |
| x_min = p.x; | |
| } | |
| if (p.x > x_max) { | |
| x_max = p.x; | |
| } | |
| if (p.y < y_min) { | |
| y_min = p.y; | |
| } | |
| if (p.y > y_max) { | |
| y_max = p.y; | |
| } | |
| } | |
| const int32_t w{x_max - x_min + 1}; | |
| const int32_t h{y_max - y_min + 1}; | |
| return std::array<int32_t, 4>{x_min, y_min, w, h}; | |
| } | |
| std::array<int32_t, 4> Node::bounding_box_xywh() const { | |
| if (m_pixels->empty()) { | |
| return {0, 0, 0, 0}; | |
| } | |
| int32_t x_min{INT_MAX}; | |
| int32_t y_min{INT_MAX}; | |
| int32_t x_max{INT_MIN}; | |
| int32_t y_max{INT_MIN}; | |
| for (auto &[_, p] : *m_pixels) { | |
| if (p.x < x_min) { | |
| x_min = p.x; | |
| } | |
| if (p.x > x_max) { | |
| x_max = p.x; | |
| } | |
| if (p.y < y_min) { | |
| y_min = p.y; | |
| } | |
| if (p.y > y_max) { | |
| y_max = p.y; | |
| } | |
| } | |
| const int32_t w{x_max - x_min + 1}; | |
| const int32_t h{y_max - y_min + 1}; | |
| return std::array<int32_t, 4>{x_min, y_min, w, h}; | |
| } |
🤖 Prompt for AI Agents
In `@src/wasm/modules/image/src/node.cpp` around lines 41 - 65, In
Node::bounding_box_xywh(), x_max and y_max are initialized to 0 which yields
incorrect boxes when all pixel coordinates are negative; change their
initialization to INT_MIN (matching x_min/y_min using INT_MAX) so the loop
correctly computes extremes for negative coordinates and the returned x,y,w,h
are valid.
| // for multithreading | ||
| { | ||
| name: 'force-security-headers', | ||
| configureServer(server) { | ||
| server.middlewares.use((_req, res, next) => { | ||
| res.setHeader("Cross-Origin-Opener-Policy", "same-origin"); | ||
| res.setHeader("Cross-Origin-Embedder-Policy", "require-corp"); | ||
| next(); | ||
| }); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Redundant header setting, but intentional for robustness.
The same COOP/COEP headers are set both in server.headers (lines 49-52) and via this middleware plugin. While this appears redundant, the middleware approach ensures headers are applied to all responses including HMR websocket upgrades, which server.headers may not cover consistently in all Vite versions. This is acceptable.
However, the pipeline indicates a Prettier formatting issue in this file. Please run prettier --write vite.config.js to fix formatting before merging.
🤖 Prompt for AI Agents
In `@vite.config.js` around lines 116 - 126, Prettier formatting is off in
vite.config.js; run the formatter and ensure the code around the plugin named
"force-security-headers" (the configureServer function that calls
server.middlewares.use and sets Cross-Origin-Opener-Policy and
Cross-Origin-Embedder-Policy headers) is properly formatted—run `prettier
--write vite.config.js` and commit the reformatted file so the configureServer
middleware block and surrounding server.headers remain consistently styled.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/wasm/modules/image/src/kmeans.cpp`:
- Around line 61-65: The mutex around writes in _apply_labels is unnecessary
because each thread writes exclusively to its own range
labels[start_pixel..end_pixel) so remove the std::unique_lock<std::mutex>
lock(write_mutex); and the explicit lock.unlock(); and simply assign labels[i] =
best_cluster when labels[i] != best_cluster; keep the conditional and variable
references (labels, best_cluster, start_pixel, end_pixel, _apply_labels) intact
to preserve logic and avoid synchronization overhead.
- Around line 149-153: The code unconditionally sets changed = true after the
threaded label assignment, preventing early termination; modify the threaded
label update so _apply_labels reports whether it changed any labels (e.g.,
return a bool or increment a shared atomic<int> changed_count), run each thread
to produce a per-thread changed flag or increment the atomic, join/aggregate
those results in kmeans() and then set changed = (changed_count > 0) (or any
thread flag OR), and only then clear threads(); ensure to use an atomic or
mutex-safe per-thread storage when aggregating to avoid races.
♻️ Duplicate comments (1)
src/wasm/modules/image/src/kmeans.cpp (1)
1-13: Missing<mutex>header forstd::mutex.Line 22 uses
std::mutex, but the<mutex>header is not included. This will cause a compilation error on strict toolchains.Proposed fix
`#include` <thread> +#include <mutex> `#include` <functional>Also, as noted in a prior review,
<cstring>is needed forstd::memcpyon line 222.
🧹 Nitpick comments (5)
src/wasm/modules/image/src/kmeans.cpp (5)
35-37: Redundantlock.unlock()withstd::unique_lock.
std::unique_lockautomatically releases the mutex when it goes out of scope at loop iteration end. The explicitunlock()is unnecessary.Proposed simplification
- std::unique_lock<std::mutex> lock(write_mutex); - std::copy(_res.begin(), _res.end(), output[j].begin()); - lock.unlock(); + { + std::lock_guard<std::mutex> lock(write_mutex); + std::copy(_res.begin(), _res.end(), output[j].begin()); + }
22-22: Global mutex may cause contention with concurrentkmeanscalls.If
kmeansis ever called concurrently (e.g., from multiple workers or contexts), this global mutex would serialize unrelated operations. Consider making the mutex local to eachkmeansinvocation and passing it to helper functions.
83-84: Edge case: whenk < n_threads, most threads process zero centroids.If
k=3andn_threads=8, thencentroids_per_thread=0. Most threads will havestart_c == end_cand do no work. Consider capping effective threads:int effective_threads = std::min(n_threads, k);
87-91: Considerrand_ror<random>for reproducibility and quality.While
srand/randare safe here (called before threading), consider using<random>facilities for better distribution quality. This is optional since k-means is generally tolerant of initialization quality.
225-231: Noted: Spatial variant is marked as buggy.The comment indicates known issues with channel indexing. This appears to be pre-existing code. Consider opening a tracking issue if not already done.
Would you like me to help identify the channel indexing bug mentioned in the comment, or open an issue to track this?
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/wasm/modules/image/src/kmeans.cpp`:
- Around line 112-116: Loop variable type and thread count validation are wrong:
change the loop in the centroid-splitting logic to use a signed int for i
(matching n_threads), validate n_threads before use (ensure n_threads > 0 and
clamp/limit it to at most k), and recompute centroids_per_thread accordingly so
you don't get zero when k < n_threads (e.g., distribute at least one centroid to
the first k threads or compute start/end by integer division with remainders).
Update references in this block (n_threads, centroids_per_thread, k, and the for
loop that assigns start_c and end_c) to use the validated n_threads and the
corrected partitioning strategy.
- Around line 230-236: The exported function kmeans_clustering_spatial is buggy
and unused; either remove it or fix alpha handling: update the RGBXY struct (or
create RGBA-XY) to include an alpha field, change kmeans_clustering_spatial to
read input alpha from the source buffer when building pixel records, preserve
and carry alpha through any copies/assignments during clustering, and finally
write the original/preserved alpha values back into the output buffer; also
ensure the function's exported signature/documentation reflects RGBA
input/output if you keep it.
♻️ Duplicate comments (1)
src/wasm/modules/image/src/kmeans.cpp (1)
1-14: Missing<cstring>header forstd::memcpy.Line 227 uses
std::memcpy, but<cstring>is not included. This was flagged in a previous review and remains unaddressed.Proposed fix
`#include` <cstdlib> +#include <cstring> `#include` <ctime>
🧹 Nitpick comments (4)
src/wasm/modules/image/src/kmeans.cpp (3)
33-39: Unnecessary mutex in_process_dist_per_centroid- each thread writes to disjoint rows.Each thread writes to
output[j]forj ∈ [start_centroid, end_centroid), which are non-overlapping ranges. The mutex is pure overhead since there's no data race.Proposed fix
for (int j{start_centroid}; j < end_centroid; ++j) { std::transform(pixels.begin(), pixels.end(), _res.begin(), [¢roids, j](const ImageLib::RGBAPixel<float>& p) { return colorDistance(p, centroids[j]); }); - std::unique_lock<std::mutex> lock(write_mutex); std::copy(_res.begin(), _res.end(), output[j].begin()); - lock.unlock(); }
63-68: Remove commented-out dead code.The mutex lock was correctly removed per previous review feedback. Clean up the leftover commented code.
Proposed fix
if (labels[i] != best_cluster) { - //std::unique_lock<std::mutex> lock(write_mutex); labels[i] = best_cluster; changed.store(true, std::memory_order_relaxed); - //lock.unlock(); }
131-132: Remove debug artifacts.Commented-out debug statements should be cleaned up before merging.
Also applies to: 183-185
src/components/WasmImageProcessor.jsx (1)
82-88: Remove debug timing statements before merge.
console.time/console.timeEndcalls are useful for development but should be removed or conditionally disabled for production builds. Additionally, the pipeline indicates Prettier formatting issues—these lines are missing semicolons.Proposed fix
step(20); // NOTE: Gaussian blur destroys the sharp outlines first, preventing the Bilateral filter from detecting and preserving them - console.time("bilateralFilter") const imgBilateralFiltered = await bilateralFilter({ pixels: fileData.pixels, width, height, }); - console.timeEnd("bilateralFilter") step(45);step(70); - console.time("kmeans") const { pixels: kmeansed, labels } = await kmeans({ ...fileData, pixels: thresholded, num_colors: 16, }); - console.timeEnd("kmeans")Also applies to: 98-104
| for (unsigned int i = 0; i < n_threads; ++i) { | ||
|
|
||
| int start_c = i * centroids_per_thread; | ||
| int end_c = (i == n_threads - 1 ) ? k : (i + 1) * centroids_per_thread; | ||
|
|
There was a problem hiding this comment.
Type mismatch between loop variable and n_threads.
n_threads is int but the loop uses unsigned int i. If n_threads is negative (invalid input), the comparison behaves unexpectedly. Also, if k < n_threads, centroids_per_thread is 0, so all threads except the last process nothing.
Proposed fix - add validation and fix type
+ const int effective_threads = std::min(n_threads, std::max(1, k));
- for (unsigned int i = 0; i < n_threads; ++i) {
+ for (int i = 0; i < effective_threads; ++i) {🤖 Prompt for AI Agents
In `@src/wasm/modules/image/src/kmeans.cpp` around lines 112 - 116, Loop variable
type and thread count validation are wrong: change the loop in the
centroid-splitting logic to use a signed int for i (matching n_threads),
validate n_threads before use (ensure n_threads > 0 and clamp/limit it to at
most k), and recompute centroids_per_thread accordingly so you don't get zero
when k < n_threads (e.g., distribute at least one centroid to the first k
threads or compute start/end by integer division with remainders). Update
references in this block (n_threads, centroids_per_thread, k, and the for loop
that assigns start_c and end_c) to use the validated n_threads and the corrected
partitioning strategy.
| /* | ||
| *BUGGY CODE BELOW - one of the channels is poorly indexed | ||
| * | ||
| * TODO: The kmeans algorithm actually ignores the values of alpha where it | ||
| *should actually be taken into account. | ||
| */ | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if kmeans_clustering_spatial is used or exported anywhere
echo "=== Checking for usages of kmeans_clustering_spatial ==="
rg -n "kmeans_clustering_spatial" --type cpp --type h
echo ""
echo "=== Checking if it's exported via Emscripten bindings ==="
rg -n "kmeans_clustering_spatial" -g "*.js" -g "*.ts" -g "CMakeLists.txt"Repository: Ryan-Millard/Img2Num
Length of output: 419
🏁 Script executed:
#!/bin/bash
# Read the kmeans_clustering_spatial function implementation
echo "=== Reading kmeans_clustering_spatial function in kmeans.cpp ==="
sed -n '230,290p' src/wasm/modules/image/src/kmeans.cpp
echo ""
echo "=== Check what EXPORTED means in this codebase ==="
rg -n "EXPORTED" src/wasm/modules/image/include/kmeans.h -A 2 -B 2
echo ""
echo "=== Check if EXPORTED is used elsewhere ==="
rg -n "define EXPORTED" --type cpp --type hRepository: Ryan-Millard/Img2Num
Length of output: 3188
🏁 Script executed:
#!/bin/bash
# Search for kmeans_clustering_spatial in all JS/TS files and bindings
echo "=== Searching for kmeans_clustering_spatial in JS/TS files ==="
find . -type f \( -name "*.js" -o -name "*.ts" -o -name "*.jsx" -o -name "*.tsx" \) -exec grep -l "kmeans_clustering_spatial" {} \;
echo ""
echo "=== Search entire repo for kmeans_clustering_spatial (excluding node_modules) ==="
rg "kmeans_clustering_spatial" --type-list | head -20
rg "kmeans_clustering_spatial" -g "!node_modules" 2>/dev/null | head -30Repository: Ryan-Millard/Img2Num
Length of output: 849
🏁 Script executed:
#!/bin/bash
# Get more of the function to see the actual buggy indexing
sed -n '252,320p' src/wasm/modules/image/src/kmeans.cppRepository: Ryan-Millard/Img2Num
Length of output: 2330
🏁 Script executed:
#!/bin/bash
# Get the end of the function to see the buggy indexing and alpha handling
sed -n '320,360p' src/wasm/modules/image/src/kmeans.cppRepository: Ryan-Millard/Img2Num
Length of output: 766
Remove or fix the kmeans_clustering_spatial function before merge.
This function is marked as EXPORTED (public WASM API) but has confirmed bugs: it ignores the alpha channel entirely when reading input data, and never writes it back, causing data corruption. Additionally, no C++ or JavaScript code in the repository calls this function, making it dead/unused code. Either fix the alpha channel handling (include it in the RGBXY struct and write it back to output) or remove the function entirely.
🤖 Prompt for AI Agents
In `@src/wasm/modules/image/src/kmeans.cpp` around lines 230 - 236, The exported
function kmeans_clustering_spatial is buggy and unused; either remove it or fix
alpha handling: update the RGBXY struct (or create RGBA-XY) to include an alpha
field, change kmeans_clustering_spatial to read input alpha from the source
buffer when building pixel records, preserve and carry alpha through any
copies/assignments during clustering, and finally write the original/preserved
alpha values back into the output buffer; also ensure the function's exported
signature/documentation reflects RGBA input/output if you keep it.
|
Hi. Please see my comment on the related issue. Please also start using the PR templates or at least filling in the body of the PR so I can understand everything that has changed - it makes it a bit difficult for me to understand the changes when all I have to look at is code and minimal documentation. |
|
@Ryan-Millard updated to target feat/contour-tracing you can see the changes much better now |
2 participants
Please choose one of the following:
If none of these fit, you may use this default to describe your change manually.
If this is the right template, go ahead and complete it below 👇
📌 Description
Please describe the changes made in this PR and why they are necessary.
Fixes #issue-number (if applicable)
✅ Type of Change
Place an "x" in the brackets below:
🧪 How Has This Been Tested?
Please describe how you tested your changes (e.g., unit tests, manual testing, screenshots, etc.)
🧩 Checklist
Place an "x" in the brackets below:
📸 Screenshots / Demo (if applicable)
Paste images, GIFs, or demo links here.
💬 Additional Context
Anything else relevant to the PR.
Summary by CodeRabbit
New Features
Documentation
Removals
Security
✏️ Tip: You can customize this high-level summary in your review settings.