Multithreading + Color Rework - #234
Conversation
📝 WalkthroughWalkthroughAdded multithreading and CIELAB support to WASM image algorithms (bilateral_filter, kmeans); JS worker API extended with Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client (React)
participant Hook as useWasmWorker (Main Thread)
participant Worker as WASM Worker
participant WASM as WASM Module
participant PThreads as PThread Pool
UI->>Hook: requestProcess(pixels, params, color_space, n_threads)
Hook->>Worker: postMessage({ pixels, params, color_space, n_threads })
Worker->>WASM: bilateral_filter(pixels..., color_space, n_threads)
WASM->>PThreads: dispatch row segments (n_threads)
PThreads-->>WASM: return segment results
WASM-->>Worker: bilateral result
Worker->>WASM: kmeans(bilateral_pixels..., num_colors, max_iter, color_space, n_threads)
WASM->>PThreads: partition centroids/pixels (n_threads)
PThreads-->>WASM: distances & labels
WASM-->>Worker: kmeans result (pixels + labels)
Worker-->>Hook: postMessage(results)
Hook-->>UI: deliver processed pixels/labels
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
To fix the CI errors on this PR, you should run one of these two: Generic: ./img2num formatSpecific to JS: ./img2num format-js |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
vite.config.js (1)
1-1: Pipeline warning: Prettier formatting issue.The CI pipeline flagged a Prettier formatting issue. Run
prettier --write vite.config.jsto fix.src/hooks/useWasmWorker.js (1)
1-1: Pipeline warning: Prettier formatting issue.The CI pipeline flagged a Prettier formatting issue. Run
prettier --write src/hooks/useWasmWorker.jsto fix.src/wasm/modules/image/include/kmeans.h (1)
13-16: Addn_threadsparameter tokmeans_clustering_spatialfor consistency and performance.The
kmeansfunction uses multi-threading (default 8 threads) for parallel distance calculations and pixel assignments.kmeans_clustering_spatial, which performs similar k-means clustering work with spatial weighting, does not support threading and runs purely sequentially. Since both functions are exposed as public APIs and perform computationally intensive work, adding threading support tokmeans_clustering_spatialwould improve consistency and performance parity.src/wasm/modules/image/src/kmeans.cpp (1)
85-157: Guardn_threadsbefore division and loop bounds.
num_pixels / n_threadsandk / n_threadswill divide by zero ifn_threads <= 0. Clamp to at least 1 and use the clamped value consistently.🛠️ Suggested diff
- std::vector<std::thread> threads; - int pixels_per_thread = num_pixels / n_threads; - int centroids_per_thread = k / n_threads; + const int thread_count = std::max(1, n_threads); + std::vector<std::thread> threads; + const int pixels_per_thread = num_pixels / thread_count; + const int centroids_per_thread = k / thread_count; ... - if (n_threads > 1) { - for (unsigned int i = 0; i < n_threads; ++i) { + if (thread_count > 1) { + for (int i = 0; i < thread_count; ++i) { int start_c = i * centroids_per_thread; - int end_c = (i == n_threads - 1 ) ? k : (i + 1) * centroids_per_thread; + int end_c = (i == thread_count - 1 ) ? k : (i + 1) * centroids_per_thread; ... - for (unsigned int i = 0; i < n_threads; ++i) { + for (int i = 0; i < thread_count; ++i) { int start_pixel = i * pixels_per_thread; - int end_pixel = (i == n_threads - 1 ) ? num_pixels : (i + 1) * pixels_per_thread; + int end_pixel = (i == thread_count - 1 ) ? num_pixels : (i + 1) * pixels_per_thread;src/wasm/modules/image/src/bilateral_filter.cpp (1)
103-139: RGB path readscie_imageout of bounds.
cie_imageis empty in RGB mode, but it’s still indexed for every neighbor. This is UB and can crash. Move the CIELAB reads into the CIELAB case.🐛 Suggested diff
- double L{cie_image[neighbor_idx]}; - double A{cie_image[neighbor_idx + 1]}; - double B{cie_image[neighbor_idx + 2]}; ... case COLOR_SPACE_OPTION_CIELAB: { - dL = L - L0; - dA = A - A0; - dB = B - B0; + const double L{cie_image[neighbor_idx]}; + const double A{cie_image[neighbor_idx + 1]}; + const double B{cie_image[neighbor_idx + 2]}; + dL = L - L0; + dA = A - A0; + dB = B - B0;
🤖 Fix all issues with AI agents
In `@src/wasm/modules/image/src/bilateral_filter.cpp`:
- Around line 179-270: The function bilateral_filter computes rows_per_thread =
height / n_threads which will divide by zero when n_threads <= 0; clamp or
sanitize n_threads at start of bilateral_filter (e.g., set int threads =
std::max(1, n_threads)) and use that sanitized value when computing
rows_per_thread, launching threads, and when passing the thread-count into calls
to _process; update all uses of n_threads in the thread loop, bounds
(start_row/end_row), and the single-thread _process call to use the sanitized
threads variable so no division by zero or negative-thread behavior occurs.
In `@src/wasm/modules/image/src/kmeans.cpp`:
- Around line 42-67: The function _apply_labels has a type mismatch: it
currently takes std::vector<int>& labels but callers pass std::vector<int32_t>,
causing a compile error; change the parameter type in _apply_labels to
std::vector<int32_t>& labels (keep best_cluster as int32_t and other logic
unchanged), and update any call sites to match this signature so the label
vector types are consistent across _apply_labels and its callers.
🧹 Nitpick comments (4)
vite.config.js (1)
116-126: Consider removing redundant header configuration.The middleware plugin sets the same COOP/COEP headers already defined in
server.headers(lines 49-52). While having both is harmless, the middleware approach is more robust as it catches all responses including proxied requests.You could remove the
server.headersblock and rely solely on this middleware, or vice versa, to reduce duplication.src/wasm/modules/image/include/bilateral_filter.h (1)
18-20: LGTM! Consider updating documentation.The
n_threadsparameter is appropriately added with a sensible default. The default of 8 aligns withPTHREAD_POOL_SIZEin the CMake configuration.The function documentation (lines 9-17) should be updated to describe the new
n_threadsparameter for completeness.src/wasm/modules/image/src/kmeans.cpp (1)
23-38: Avoid serializing centroid writes with a global mutex.Each thread writes distinct
output[j]slices, so the lock only adds contention. Removing the lock (andwrite_mutexif unused elsewhere) should improve scalability.♻️ Suggested diff
-std::mutex write_mutex; ... - std::unique_lock<std::mutex> lock(write_mutex); - std::copy(_res.begin(), _res.end(), output[j].begin()); - lock.unlock(); + std::copy(_res.begin(), _res.end(), output[j].begin());src/wasm/modules/image/src/bilateral_filter.cpp (1)
145-173: Avoid per‑pixel mutex; row partitioning already isolates writes.Each thread writes disjoint rows, so locking for every pixel adds contention and can erase threading gains. Removing the lock (and
write_mutexif unused) should speed up.♻️ Suggested diff
- // writing - must grab mutex - std::unique_lock<std::mutex> lock(write_mutex); - switch (color_space) { case COLOR_SPACE_OPTION_RGB: { result[center_idx] = static_cast<uint8_t>( std::clamp(weight_acc_channel_0 / weight_acc, 0.0, 255.0)); ... break; } } - // done writing - lock.unlock();
| void _apply_labels( | ||
| const ImageLib::Image<ImageLib::RGBAPixel<float>>& pixels, | ||
| const std::vector<std::vector<float>>& distances, | ||
| std::vector<int>& labels, | ||
| int start_pixel, | ||
| int end_pixel, | ||
| int k, | ||
| std::atomic<bool>& changed | ||
| ) | ||
| { | ||
| float min_color_dist{std::numeric_limits<float>::max()}; | ||
| int32_t best_cluster{0}; | ||
| for (int i{start_pixel}; i < end_pixel; ++i) { | ||
| min_color_dist = std::numeric_limits<float>::max(); | ||
| best_cluster = 0; | ||
| for (int j{0}; j < k; ++j) { | ||
| if (distances[j][i] < min_color_dist) { | ||
| min_color_dist = distances[j][i]; | ||
| best_cluster = j; | ||
| } | ||
| } | ||
| 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(); |
There was a problem hiding this comment.
Fix labels vector type mismatch (won’t compile).
_apply_labels expects std::vector<int>&, but the caller passes std::vector<int32_t>. This is a hard compile error.
🐛 Suggested diff
- std::vector<int>& labels,
+ std::vector<int32_t>& labels,🤖 Prompt for AI Agents
In `@src/wasm/modules/image/src/kmeans.cpp` around lines 42 - 67, The function
_apply_labels has a type mismatch: it currently takes std::vector<int>& labels
but callers pass std::vector<int32_t>, causing a compile error; change the
parameter type in _apply_labels to std::vector<int32_t>& labels (keep
best_cluster as int32_t and other logic unchanged), and update any call sites to
match this signature so the label vector types are consistent across
_apply_labels and its callers.
|
i'm getting this error running I'm not using docker. I'm just using |
|
@Ryan-Millard before you jump on to the next stuff, can you review this PR. it really does make things a lot faster with not many changes |
That's not nice. You basically have 2 options:
I recommend just running: yarn run formatIt's a JS script. |
I was already looking at it. It seems much better. |
this works! |
Try this one: yarn run helpCI enforces that all new scripts get descriptions added to the package.json file so this script can give nice outputs. |
There was a problem hiding this comment.
Actionable comments posted: 2
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 (2)
136-164: Per-pixel mutex locking severely degrades multithreading performance.Each pixel write acquires/releases the global mutex, serializing nearly all work and negating the benefit of multiple threads. Since each thread writes to a disjoint row range, no synchronization is needed.
🛠️ Suggested fix: Remove mutex from _process
- // writing - must grab mutex - std::unique_lock<std::mutex> lock(write_mutex); - switch (color_space) { // ... (keep switch body unchanged) } - // done writing - lock.unlock();
98-100: Undefined behavior: accessing emptycie_imagein RGB mode.Lines 98-100 unconditionally read from
cie_image, but whencolor_space == COLOR_SPACE_OPTION_RGB, the vector is empty. This causes out-of-bounds access.🐛 Suggested fix: Move CIELAB reads inside the conditional
- double L{cie_image[neighbor_idx]}; - double A{cie_image[neighbor_idx + 1]}; - double B{cie_image[neighbor_idx + 2]}; - w_space = spatial_weights[(ky + radius) * kernel_diameter + (kx + radius)]; switch (color_space) { case COLOR_SPACE_OPTION_RGB: { // ... RGB logic unchanged break; } case COLOR_SPACE_OPTION_CIELAB: { + double L{cie_image[neighbor_idx]}; + double A{cie_image[neighbor_idx + 1]}; + double B{cie_image[neighbor_idx + 2]}; dL = L - L0; // ... rest unchanged }
🤖 Fix all issues with AI agents
In `@src/wasm/modules/image/src/kmeans.cpp`:
- Around line 92-100: The block initializing distances in kmeans.cpp is
misformatted (the std::vector<std::vector<float>> distances declaration and
subsequent resize loop); run the repository formatter and commit the changes so
the clang-format CI check passes (e.g., run `yarn run format` or `./img2num
format`), ensuring the declarations around distances, n_threads, k, and
num_pixels follow the project's clang-format rules before pushing.
- Around line 79-81: Validate and clamp n_threads to be at least 1 before
computing pixels_per_thread and centroids_per_thread: create a local
thread_count = std::max(1, n_threads) (or equivalent), then compute int
pixels_per_thread = num_pixels / thread_count and int centroids_per_thread = k /
thread_count; also replace subsequent uses of n_threads in the kmeans threading
logic (references to threads, pixels_per_thread, centroids_per_thread and any
loops using n_threads) with thread_count to prevent division-by-zero and
negative-thread counts.
♻️ Duplicate comments (2)
src/wasm/modules/image/src/bilateral_filter.cpp (1)
179-181: Division by zero whenn_threads <= 0remains unaddressed.This issue was flagged in a previous review. Clamp
n_threadsto at least 1 before computingrows_per_thread.src/wasm/modules/image/src/kmeans.cpp (1)
42-44: Type mismatch betweenlabelsvector and_apply_labelsparameter.This was flagged in a previous review. Change parameter type to
std::vector<int32_t>&to match the caller.
🧹 Nitpick comments (3)
src/wasm/modules/image/src/bilateral_filter.cpp (1)
237-246: Type mismatch in thread loop.Loop variable
iisunsigned intwhilen_threadsisint. After fixing then_threadsvalidation, use a consistent type (e.g.,int i) or cast appropriately to avoid signed/unsigned comparison warnings.src/wasm/modules/image/src/kmeans.cpp (2)
36-38: Mutex is unnecessary for disjoint centroid writes.Each thread writes to its own range of
output[j]indices (start_centroidtoend_centroid), so no concurrent writes occur. Remove the mutex to improve performance.♻️ Suggested 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(); }
107-114: Edge case: Whenk < n_threads, most threads do no work.If the number of centroids
kis less thanthread_count,centroids_per_threadbecomes 0, and only the last thread processes all centroids. Consider limiting thread count tomin(thread_count, k)for the distance computation phase.
Ryan-Millard
left a comment
Ryan-Millard
left a comment
There was a problem hiding this comment.
I'm sorry for taking so long to review this.
This is an excellent implementation and I like it a lot. It's clean and efficient. I just have a few requests from you in conjunction to the comments I left on the files:
- Please will you use the Image class - it makes code much cleaner.
- I'd like to discuss the possibility of tiling and SIMD in this PR since the point is to speed things up rather than to add threads and I think adding those as well would really help (especially SIMD).
| num_colors: 16, | ||
| }); | ||
|
|
||
| step(70); | ||
| const { pixels: kmeansed, labels } = await kmeans({ | ||
| ...fileData, | ||
| pixels: thresholded, | ||
| num_colors: 8, | ||
| num_colors: 16, |
There was a problem hiding this comment.
I don't recommend using 16 for this right now because it really slows down larger images and will make it difficult for mobile users to use the app.
My proposed reversion
| num_colors: 16, | |
| }); | |
| step(70); | |
| const { pixels: kmeansed, labels } = await kmeans({ | |
| ...fileData, | |
| pixels: thresholded, | |
| num_colors: 8, | |
| num_colors: 16, | |
| num_colors: 8, | |
| }); | |
| step(70); | |
| const { pixels: kmeansed, labels } = await kmeans({ | |
| ...fileData, | |
| pixels: thresholded, | |
| num_colors: 8, |
Try running this image through it:
Unless you have a strong PC, it'll take a while to process and then lag when you try to color it in or interact with the viewport on the editor page.
There was a problem hiding this comment.
this is in my set of test images. with multithreading it processes fast on my machine which is using an i7-13700H cpu
There was a problem hiding this comment.
Try processing the image on a cellphone.
In the future, we will add a config panel for the web app, but we don't have that right now, so this could be a problem for some users.
There is no problem doing multithreading and SIMD together. A smart compiler should figure out SIMD on it's own but it also is very hardware specific. So for now let's trust the compiler |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wasm/modules/image/src/bilateral_filter.cpp (1)
94-124: Guard CIELAB buffer reads in RGB mode (out-of-bounds).
In RGB modecie_imageis empty, yet reads are unconditional. This is UB; move these reads into the CIELAB case.🐛 Suggested fix
- double L{cie_image[neighbor_idx]}; - double A{cie_image[neighbor_idx + 1]}; - double B{cie_image[neighbor_idx + 2]}; - w_space = spatial_weights[(ky + radius) * kernel_diameter + (kx + radius)]; @@ case COLOR_SPACE_OPTION_CIELAB: { + const double L{cie_image[neighbor_idx]}; + const double A{cie_image[neighbor_idx + 1]}; + const double B{cie_image[neighbor_idx + 2]}; dL = L - L0; dA = A - A0; dB = B - B0;
🤖 Fix all issues with AI agents
In `@src/wasm/modules/image/src/bilateral_filter.cpp`:
- Around line 237-241: The block that spawns threads (referencing n_threads,
rows_per_thread, start_row, end_row) is failing clang-format; reformat the file
using the repository formatting wrapper (e.g., run ./img2num format-wasm or
./img2num format) instead of invoking clang-format directly, then commit the
updated bilateral_filter.cpp so the loop and related declarations conform to the
project's style rules.
♻️ Duplicate comments (2)
src/wasm/modules/image/src/kmeans.cpp (1)
41-44: Fix labels vector type mismatch (won’t compile).
_apply_labelstakesstd::vector<int>&but callers passstd::vector<int32_t>.🐛 Suggested fix
- std::vector<int> &labels, int start_pixel, int end_pixel, + std::vector<int32_t> &labels, int start_pixel, int end_pixel,src/wasm/modules/image/src/bilateral_filter.cpp (1)
179-182: Clampn_threadsbefore computingrows_per_thread.
rows_per_threaddivides byn_threads; passing 0 will crash. Use a clamped thread_count for division, loop bounds, and thread dispatch.Based on learnings, keep the thread-count clamp before any division.🛠️ Suggested fix
- std::vector<std::thread> threads; - - int rows_per_thread = static_cast<int>(height) / static_cast<int>(n_threads); + const int thread_count{std::max(1, static_cast<int>(n_threads))}; + std::vector<std::thread> threads; + int rows_per_thread{static_cast<int>(height) / thread_count}; @@ - if (n_threads > 1) { - for (int i = 0; i < n_threads; ++i) { + if (thread_count > 1) { + for (int i = 0; i < thread_count; ++i) { int start_row{i * rows_per_thread}; - int end_row{(i == n_threads - 1) ? static_cast<int>(height) : (i + 1) * rows_per_thread}; + int end_row{(i == thread_count - 1) ? static_cast<int>(height) + : (i + 1) * rows_per_thread}; @@ - threads.emplace_back(_process, std::cref(image), std::cref(cie_image), - std::ref(result), std::cref(spatial_weights), - std::cref(range_lut), radius, sigma_range, start_row, - end_row, height, width, color_space, n_threads); + threads.emplace_back(_process, std::cref(image), std::cref(cie_image), + std::ref(result), std::cref(spatial_weights), + std::cref(range_lut), radius, sigma_range, start_row, + end_row, height, width, color_space, + static_cast<uint8_t>(thread_count)); } @@ - } else { - _process(image, cie_image, result, spatial_weights, range_lut, radius, - sigma_range, 0, static_cast<int>(height), height, width, - color_space, n_threads); + } else { + _process(image, cie_image, result, spatial_weights, range_lut, radius, + sigma_range, 0, static_cast<int>(height), height, width, + color_space, static_cast<uint8_t>(thread_count)); }
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wasm/modules/image/src/bilateral_filter.cpp (1)
98-100: Movecie_imagereads into the CIELAB case to prevent out-of-bounds access in RGB mode.Lines 98–100 unconditionally read from
cie_image, but this vector is only populated in CIELAB mode (line 215–216). In RGB mode,cie_imageremains empty, causing out-of-bounds access. SinceL,A, andBare used only in the CIELAB case (lines 45–47), move these reads inside the case block.Proposed fix
- double L{cie_image[neighbor_idx]}; - double A{cie_image[neighbor_idx + 1]}; - double B{cie_image[neighbor_idx + 2]}; - w_space = spatial_weights[(ky + radius) * kernel_diameter + (kx + radius)]; switch (color_space) { case COLOR_SPACE_OPTION_RGB: { // ... } case COLOR_SPACE_OPTION_CIELAB: { + const double L{cie_image[neighbor_idx]}; + const double A{cie_image[neighbor_idx + 1]}; + const double B{cie_image[neighbor_idx + 2]}; dL = L - L0; dA = A - A0; dB = B - B0;
🤖 Fix all issues with AI agents
In `@src/wasm/modules/image/src/kmeans.cpp`:
- Around line 79-82: Validate the centroid count k before calling std::clamp to
avoid UB: at the start of the function that contains n_threads, k, and
num_pixels (where nthreads, pixels_per_thread, centroids_per_thread are
computed) add a guard that checks k > 0 and handle invalid input (e.g., throw
std::invalid_argument or return an error status), then use the validated k when
computing nthreads = std::clamp(static_cast<int>(n_threads), 1, k) and the
subsequent pixels_per_thread and centroids_per_thread calculations; ensure the
new check occurs before any use of k in std::clamp or division operations.
♻️ Duplicate comments (2)
src/wasm/modules/image/src/kmeans.cpp (1)
41-44: Fix labels vector type mismatch (compile error).
_apply_labelstakesstd::vector<int>&, but the caller passesstd::vector<int32_t>. This won’t compile; align the parameter type with the callers.🐛 Proposed fix
-void _apply_labels(const ImageLib::Image<ImageLib::RGBAPixel<float>> &pixels, - const std::vector<std::vector<float>> &distances, - std::vector<int> &labels, int start_pixel, int end_pixel, - int k, std::atomic<bool> &changed) { +void _apply_labels(const ImageLib::Image<ImageLib::RGBAPixel<float>> &pixels, + const std::vector<std::vector<float>> &distances, + std::vector<int32_t> &labels, int start_pixel, int end_pixel, + int k, std::atomic<bool> &changed) {src/wasm/modules/image/src/bilateral_filter.cpp (1)
179-182: Clampn_threadsto at least 1 before division.
rows_per_threaddivides byn_threads, son_threads == 0will crash. Clamp once and use the sanitized value everywhere in this block. Based on learnings, ensure the clamped count is used for all thread math.🛠️ Suggested fix
- std::vector<std::thread> threads; - - int rows_per_thread = static_cast<int>(height) / static_cast<int>(n_threads); + const int thread_count{std::max(1, static_cast<int>(n_threads))}; + std::vector<std::thread> threads; + const int rows_per_thread{static_cast<int>(height) / thread_count}; @@ - if (n_threads > 1) { - for (int i = 0; i < n_threads; ++i) { + if (thread_count > 1) { + for (int i{0}; i < thread_count; ++i) { int start_row{i * rows_per_thread}; - int end_row{(i == n_threads - 1) ? static_cast<int>(height) + int end_row{(i == thread_count - 1) ? static_cast<int>(height) : (i + 1) * rows_per_thread}; @@ - end_row, height, width, color_space, n_threads); + end_row, height, width, color_space, + static_cast<uint8_t>(thread_count)); @@ - _process(image, cie_image, result, spatial_weights, range_lut, radius, - sigma_range, 0, static_cast<int>(height), height, width, - color_space, n_threads); + _process(image, cie_image, result, spatial_weights, range_lut, radius, + sigma_range, 0, static_cast<int>(height), height, width, + color_space, static_cast<uint8_t>(thread_count));
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wasm/modules/image/src/bilateral_filter.cpp (1)
94-100: Guardcie_imageaccess in RGB mode (out-of-bounds read).Lines 98–100 read from
cie_imageunconditionally; in RGB mode, the vector is empty and these accesses are undefined behavior. The center pixel reads are correctly guarded at lines 70–74, but the neighbor pixel reads are not. Move the reads into the CIELAB case where the vector is guaranteed populated and the values are actually used.Proposed fix
- double L{cie_image[neighbor_idx]}; - double A{cie_image[neighbor_idx + 1]}; - double B{cie_image[neighbor_idx + 2]}; - w_space = spatial_weights[(ky + radius) * kernel_diameter + (kx + radius)]; switch (color_space) { case COLOR_SPACE_OPTION_RGB: { ... break; } case COLOR_SPACE_OPTION_CIELAB: { + const double L{cie_image[neighbor_idx]}; + const double A{cie_image[neighbor_idx + 1]}; + const double B{cie_image[neighbor_idx + 2]}; dL = L - L0; dA = A - A0; dB = B - B0;Also applies to: 118–130
🤖 Fix all issues with AI agents
In `@src/components/WasmImageProcessor.jsx`:
- Around line 97-101: The destructured variable kmeansed from the kmeans(...)
call is unused and causing an ESLint error; update the await call in
WasmImageProcessor.jsx to only destructure the needed value(s) (e.g., const {
labels } = await kmeans({ ...fileData, pixels: imgBilateralFiltered, num_colors:
16 });) or otherwise use kmeansed where intended—modify the kmeans invocation or
subsequent code so you no longer declare an unused variable named kmeansed.
In `@src/wasm/modules/image/include/LABPixel.h`:
- Around line 16-28: The setGray implementation in class LABPixel currently
zeroes a and b but fails to assign the luminance channel; update
LABPixel::setGray(NumberT gray) to set l = gray (with appropriate static_cast if
needed) and then set a = b = static_cast<NumberT>(0) so the L channel reflects
the grayscale value; ensure you modify the inline void setGray(NumberT gray)
method accordingly.
In `@src/wasm/modules/image/src/graph.cpp`:
- Around line 10-22: The function colorDistance uses std::sqrt but the
translation unit lacks <cmath>; add the missing include directive so std::sqrt
is defined. Open graph.cpp and add `#include` <cmath> near the top of the file
(alongside other includes) to resolve the compile error referenced in the
colorDistance function.
In `@src/wasm/modules/image/src/kmeans.cpp`:
- Around line 82-145: In kMeansPlusPlusInit, add input validation at the top to
guard against num_pixels == 0 and nonpositive k: compute int num_pixels =
pixels.getSize(); if (num_pixels <= 0 || k <= 0) return or throw (consistent
with project error handling); also validate k <= num_pixels and that
out_centroids has capacity for k (e.g., out_centroids.getSize() >= k) and handle
the error similarly; these checks must appear before the
uniform_int_distribution call and any indexing (references: function
kMeansPlusPlusInit, variables num_pixels, k, out_centroids, and the
uniform_int_distribution usage).
♻️ Duplicate comments (4)
src/wasm/modules/image/src/bilateral_filter.cpp (2)
29-29: Remove the per-pixel mutex to avoid serializing output.Threads operate on disjoint row ranges, so the lock at Line 136 just serializes writes and negates parallelism. Safe to drop it.
♻️ Proposed fix
-std::mutex write_mutex; ... - // writing - must grab mutex - std::unique_lock<std::mutex> lock{write_mutex}; - switch (color_space) { ... } - // done writing - lock.unlock();Also applies to: 136-165
179-183: Clampn_threadsbefore computingrows_per_thread.Line 181 divides by
n_threadsbefore then_threads > 1check;n_threads == 0will crash. Use a sanitizedthread_count >= 1for division, loop bounds, and_processcalls.🛠️ Proposed fix
- std::vector<std::thread> threads; - - int rows_per_thread = static_cast<int>(height) / static_cast<int>(n_threads); + const int thread_count{std::max(1, static_cast<int>(n_threads))}; + std::vector<std::thread> threads; + int rows_per_thread{static_cast<int>(height) / thread_count}; ... - if (n_threads > 1) { - for (int i = 0; i < n_threads; ++i) { + if (thread_count > 1) { + for (int i = 0; i < thread_count; ++i) { int start_row{i * rows_per_thread}; - int end_row{(i == n_threads - 1) ? static_cast<int>(height) + int end_row{(i == thread_count - 1) ? static_cast<int>(height) : (i + 1) * rows_per_thread}; threads.emplace_back(_process, std::cref(image), std::cref(cie_image), std::ref(result), std::cref(spatial_weights), std::cref(range_lut), radius, sigma_range, start_row, - end_row, height, width, color_space, n_threads); + end_row, height, width, color_space, thread_count); } ... - } else { + } else { _process(image, cie_image, result, spatial_weights, range_lut, radius, sigma_range, 0, static_cast<int>(height), height, width, - color_space, n_threads); + color_space, thread_count); }Based on learnings, apply a safe thread-count before any division.
Also applies to: 237-259
src/wasm/modules/image/src/kmeans.cpp (2)
147-175: Guardkbeforestd::clamp.Line 171 calls
std::clamp(..., 1, k)which requiresk >= 1. Ifk <= 0, this is UB and can also break per-thread divisions. Validatek(and image dimensions) before computing thread counts.🛠️ Proposed fix
void kmeans(const uint8_t *data, uint8_t *out_data, int32_t *out_labels, const int32_t width, const int32_t height, const int32_t k, const int32_t max_iter, const uint8_t color_space, const uint8_t n_threads) { + if (k <= 0 || width <= 0 || height <= 0) { + return; + } ImageLib::Image<ImageLib::RGBAPixel<float>> pixels; pixels.loadFromBuffer(data, width, height, ImageLib::RGBA_CONVERTER<float>); const int32_t num_pixels{pixels.getSize()}; ... - int nthreads = std::clamp(static_cast<int>(n_threads), 1, k); + const int nthreads{std::clamp(static_cast<int>(n_threads), 1, k)};Based on learnings, derive a safe thread count before any divisions.
58-77: Fix labels vector type mismatch (compile error).
labelsisstd::vector<int32_t>inkmeans, but_apply_labelsexpectsstd::vector<int>&. This won’t compile.🐛 Proposed fix
template <typename PixelT> void _apply_labels(const ImageLib::Image<PixelT> &pixels, const std::vector<std::vector<float>> &distances, - std::vector<int> &labels, int start_pixel, int end_pixel, + std::vector<int32_t> &labels, int start_pixel, int end_pixel, int k, std::atomic<bool> &changed) {
🧹 Nitpick comments (4)
src/wasm/modules/image/CMakeLists.txt (1)
34-50: Make pthread pool size configurable to match runtimen_threads.
PTHREAD_POOL_SIZE=8is hard-coded, but JS allows passingn_threads. If a caller supplies a higher value, the worker pool may cap or error depending on Emscripten behavior. Consider exposing the pool size as a CMake cache option and keeping defaults aligned with the JS-side defaults.♻️ Proposed refactor
+# Allow overriding pthread pool size at configure time +set(PTHREAD_POOL_SIZE 8 CACHE STRING "Emscripten pthread pool size") + set(COMMON_FLAGS "SHELL:-s MODULARIZE=1" "SHELL:-s EXPORT_ES6=1" "SHELL:-s EXIT_RUNTIME=1" "SHELL:-s ENVIRONMENT=web,worker" "SHELL:-s EXPORTED_FUNCTIONS=['_malloc','_free']" "SHELL:-s EXPORTED_RUNTIME_METHODS=['ccall','cwrap','getValue','setValue','HEAPU8','HEAP32']" "SHELL:-s INITIAL_MEMORY=1024MB" "SHELL:-s MAXIMUM_MEMORY=2048MB" "SHELL:-s ALLOW_MEMORY_GROWTH=1" "SHELL:-s EXPORT_NAME=create${CAP_MODULE_NAME}Module" "SHELL:-s USE_PTHREADS=1" - "SHELL:-s PTHREAD_POOL_SIZE=8" + "SHELL:-s PTHREAD_POOL_SIZE=${PTHREAD_POOL_SIZE}" )src/wasm/modules/image/src/graph.cpp (1)
10-22: Avoidsqrtin the sort key for better throughput.You only need relative ordering, so a squared distance avoids
sqrt. This also matches the approach insrc/wasm/modules/image/src/kmeans.cpp(lines 19-25), which explicitly avoidssqrt.♻️ Proposed refactor
- return std::sqrt((af.red - bf.red) * (af.red - bf.red) + - (af.green - bf.green) * (af.green - bf.green) + - (af.blue - bf.blue) * (af.blue - bf.blue)); + return (af.red - bf.red) * (af.red - bf.red) + + (af.green - bf.green) * (af.green - bf.green) + + (af.blue - bf.blue) * (af.blue - bf.blue);src/hooks/useWasmWorker.js (1)
38-69: Clampn_threadsto a safe minimum before sending to WASM.If a caller passes
0or a negative value, the wasm side may hit divide-by-zero or invalid thread scheduling. Consider sanitizingn_threadsin the hook.♻️ Proposed refactor
const bilateralFilter = async ({ pixels, width, height, sigma_spatial = 3.0, sigma_range = 50.0, color_space = 0, n_threads = 8, }) => { + const threadCount = Math.max(1, Math.floor(n_threads)); return ( - await call('bilateral_filter', { pixels, width, height, sigma_spatial, sigma_range, color_space, n_threads }, [ + await call('bilateral_filter', { pixels, width, height, sigma_spatial, sigma_range, color_space, n_threads: threadCount }, [ 'pixels', ]) ).output.pixels; }; const kmeans = async ({ pixels, out_pixels = new Uint8ClampedArray(pixels.length), width, height, out_labels = new Int32Array(width * height), num_colors, max_iter = 250, color_space = 0, n_threads = 8, }) => { + const threadCount = Math.max(1, Math.floor(n_threads)); const result = ( - await call('kmeans', { pixels, out_pixels, out_labels, width, height, num_colors, max_iter, color_space, n_threads }, [ + await call('kmeans', { pixels, out_pixels, out_labels, width, height, num_colors, max_iter, color_space, n_threads: threadCount }, [ 'pixels', 'out_pixels', 'out_labels', ]) ).output;Based on learnings, ensure thread counts are non-zero before any division or scheduling logic.
src/wasm/modules/image/include/cielab_impl.h (1)
98-133: Avoid integral wraparound in LAB outputs.
rgb_to_labcasts toToutbefore clamping; ifToutis integral/unsigned, negative a/b (and any pre-clamp L) can wrap. Consider clamping indoubleand/or constrainingToutto floating-point.♻️ Proposed fix
template <typename Tin, typename Tout> void rgb_to_lab(const Tin r_u8, const Tin g_u8, const Tin b_u8, Tout &out_l, Tout &out_a, Tout &out_b) { + static_assert(std::is_floating_point_v<Tout>, + "LAB outputs should be floating-point to preserve sign/precision."); // 1. Convert 8-bit RGB [0, 255] to linear RGB [0.0, 1.0] ... - out_l = static_cast<Tout>(LAB_L_FACTOR * fy - LAB_L_OFFSET); - out_a = static_cast<Tout>(LAB_A_FACTOR * (fx - fy)); - out_b = static_cast<Tout>(LAB_B_FACTOR * (fy - fz)); - - out_l = std::clamp(out_l, static_cast<Tout>(0.0), static_cast<Tout>(100.0)); + const double l = LAB_L_FACTOR * fy - LAB_L_OFFSET; + const double a = LAB_A_FACTOR * (fx - fy); + const double b = LAB_B_FACTOR * (fy - fz); + out_l = static_cast<Tout>(std::clamp(l, 0.0, 100.0)); + out_a = static_cast<Tout>(a); + out_b = static_cast<Tout>(b); }
| // The K-Means++ Initialization Function | ||
| template <typename PixelT> | ||
| void kMeansPlusPlusInit(const ImageLib::Image<PixelT>& pixels, ImageLib::Image<PixelT>& out_centroids, int k) { | ||
| std::vector<PixelT> centroids; | ||
|
|
||
| int num_pixels = pixels.getSize(); | ||
| int c_idx{0}; // centroid index | ||
| // Random number generator setup | ||
| std::random_device rd; | ||
| std::mt19937 gen(rd()); | ||
|
|
||
| // --- Step 1: Choose the first centroid uniformly at random --- | ||
| std::uniform_int_distribution<> dis(0, num_pixels - 1); | ||
| int first_index = dis(gen); | ||
| centroids.push_back(pixels[first_index]); | ||
|
|
||
| // Vector to store the squared distance of each pixel to its NEAREST existing centroid. | ||
| // Initialize with max double so the first distance calculation always updates it. | ||
| std::vector<double> min_dist_sq(num_pixels, std::numeric_limits<double>::max()); | ||
|
|
||
| // --- Step 2 & 3: Repeat until we have k centroids --- | ||
| for (int i = 1; i < k; ++i) { | ||
|
|
||
| double sum_dist_sq = 0.0; | ||
|
|
||
| // Update distances relative to the LAST added centroid (centroids.back()) | ||
| // We don't need to recheck previous centroids; min_dist_sq already holds the best distance to them. | ||
| for (int j = 0; j < num_pixels; ++j) { | ||
| double d = colorDistance(pixels[j], centroids.back()); | ||
|
|
||
| // If this new centroid is closer than the previous best, update the min distance | ||
| if (d < min_dist_sq[j]) { | ||
| min_dist_sq[j] = d; | ||
| } | ||
| sum_dist_sq += min_dist_sq[j]; | ||
| } | ||
|
|
||
| // --- Step 3: Choose new center with probability proportional to D(x)^2 --- | ||
| // We use a weighted random selection (Roulette Wheel Selection) | ||
| std::uniform_real_distribution<> dist_selector(0.0, sum_dist_sq); | ||
| double random_value = dist_selector(gen); | ||
|
|
||
| double current_sum = 0.0; | ||
| int selected_index = -1; | ||
|
|
||
| // Iterate to find the pixel corresponding to the random_value | ||
| for (int j = 0; j < num_pixels; ++j) { | ||
| current_sum += min_dist_sq[j]; | ||
| if (current_sum >= random_value) { | ||
| selected_index = j; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Fallback for floating point rounding errors (pick last one if loop finishes) | ||
| if (selected_index == -1) { | ||
| selected_index = num_pixels - 1; | ||
| } | ||
|
|
||
| centroids.push_back(pixels[selected_index]); | ||
| } | ||
|
|
||
| std::copy(centroids.begin(), centroids.end(), out_centroids.begin()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file and confirm the code matches the review comment
cat -n src/wasm/modules/image/src/kmeans.cpp | sed -n '82,145p'Repository: Ryan-Millard/Img2Num
Length of output: 2755
🏁 Script executed:
# Also check the full function context to see if there are any surrounding checks
cat -n src/wasm/modules/image/src/kmeans.cpp | sed -n '75,155p'Repository: Ryan-Millard/Img2Num
Length of output: 3390
🏁 Script executed:
# Let's search for how this function is called in the codebase
rg "kMeansPlusPlusInit" -A 3 -B 3Repository: Ryan-Millard/Img2Num
Length of output: 1267
🏁 Script executed:
# Check if there's an Image class and its getSize() method
fd -e "\.h" -e "\.hpp" | xargs rg "class Image" -A 10 | head -50Repository: Ryan-Millard/Img2Num
Length of output: 1552
🏁 Script executed:
# Find the Image class definition and getSize() method
cat -n src/wasm/modules/image/include/Image.h | head -100Repository: Ryan-Millard/Img2Num
Length of output: 3360
🏁 Script executed:
# Look for the getSize() method specifically
rg "getSize" src/wasm/modules/image/include/Image.h -A 2Repository: Ryan-Millard/Img2Num
Length of output: 176
🏁 Script executed:
# Check the kmeans function to see if k is validated before calling kMeansPlusPlusInit
cat -n src/wasm/modules/image/src/kmeans.cpp | sed -n '151,200p'Repository: Ryan-Millard/Img2Num
Length of output: 2215
🏁 Script executed:
# Check if there's any validation of k parameter in the kmeans function
cat -n src/wasm/modules/image/src/kmeans.cpp | sed -n '151,250p'Repository: Ryan-Millard/Img2Num
Length of output: 4021
Add input validation to prevent undefined behavior with empty or invalid inputs.
uniform_int_distribution(0, num_pixels - 1) results in undefined behavior when num_pixels == 0 (invalid range). Additionally, k <= 0 creates invalid state. Add an early guard:
Proposed fix
template <typename PixelT>
void kMeansPlusPlusInit(const ImageLib::Image<PixelT> &pixels,
ImageLib::Image<PixelT> &out_centroids, int k) {
std::vector<PixelT> centroids;
- int num_pixels = pixels.getSize();
+ const int num_pixels{pixels.getSize()};
+ if (k <= 0 || num_pixels <= 0) {
+ return;
+ }🤖 Prompt for AI Agents
In `@src/wasm/modules/image/src/kmeans.cpp` around lines 82 - 145, In
kMeansPlusPlusInit, add input validation at the top to guard against num_pixels
== 0 and nonpositive k: compute int num_pixels = pixels.getSize(); if
(num_pixels <= 0 || k <= 0) return or throw (consistent with project error
handling); also validate k <= num_pixels and that out_centroids has capacity for
k (e.g., out_centroids.getSize() >= k) and handle the error similarly; these
checks must appear before the uniform_int_distribution call and any indexing
(references: function kMeansPlusPlusInit, variables num_pixels, k,
out_centroids, and the uniform_int_distribution usage).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/wasm/modules/image/src/kmeans.cpp`:
- Around line 166-208: Validate the color_space input early and add robust
handling for invalid values: check the color_space parameter (used in switches
around COLOR_SPACE_OPTION_RGB and COLOR_SPACE_OPTION_CIELAB) before any
processing and return/throw an error (or set a safe default) if it's not one of
the expected enums; also add default branches to every switch that currently
lacks one (including the initialization switch that calls kMeansPlusPlusInit,
the distance computation switch that sets dist, and any switches that touch lab
or centroids_lab) to either initialize variables to safe values or abort with a
clear error so dist is never left uninitialized and lab is not left
unconstructed when COLOR_SPACE_OPTION_CIELAB is required. Ensure any early
return or exception prevents further use of uninitialized variables (dist, lab)
and keep behavior consistent across functions that reference color_space.
♻️ Duplicate comments (5)
src/wasm/modules/image/src/graph.cpp (1)
10-21: Add the missing<cmath>include forstd::sqrt.Line 19 calls
std::sqrt, but<cmath>is not included in this translation unit. This will fail to compile unless it’s indirectly included, which is fragile. Please add<cmath>explicitly.🔧 Suggested fix
`#include` "Pixel.h" `#include` <algorithm> +#include <cmath> `#include` <iterator>src/wasm/modules/image/src/kmeans.cpp (3)
151-180: Validatekbeforestd::clampand thread partitioning.
std::clamp(..., 1, k)is undefined ifk <= 0, and subsequent divisions (/ nthreads) can blow up. Add an early guard before creating centroids or clamping.🐛 Suggested guard
pixels.loadFromBuffer(data, width, height, ImageLib::RGBA_CONVERTER<float>); const int32_t num_pixels{pixels.getSize()}; + if (k <= 0 || num_pixels <= 0) { + return; + } // width = k, height = 1 ImageLib::Image<ImageLib::RGBAPixel<float>> centroids{k, 1};
56-60: Fix labels vector type mismatch (won’t compile).
_apply_labelstakesstd::vector<int>&but callers passstd::vector<int32_t>. This is a hard compile error.🐛 Suggested fix
void _apply_labels(const ImageLib::Image<PixelT> &pixels, const std::vector<std::vector<float>> &distances, - std::vector<int> &labels, int start_pixel, int end_pixel, + std::vector<int32_t> &labels, int start_pixel, int end_pixel, int k, std::atomic<bool> &changed) {
82-95: Guard kMeansPlusPlusInit against empty inputs.
uniform_int_distribution(0, num_pixels - 1)is invalid whennum_pixels == 0, andk <= 0creates invalid state.🐛 Proposed guard
- int num_pixels = pixels.getSize(); + const int num_pixels = pixels.getSize(); + if (k <= 0 || num_pixels <= 0) { + return; + }src/wasm/modules/image/include/LABPixel.h (1)
27-28: Fix grayscale conversion to set the L channel.
setGraycurrently leaveslunchanged, so grayscale writes keep stale luminance values.🐛 Proposed fix
- inline void setGray(NumberT gray) { a = b = static_cast<NumberT>(0); } + inline void setGray(NumberT gray) { + l = gray; + a = b = static_cast<NumberT>(0); + }
🧹 Nitpick comments (3)
src/wasm/modules/image/src/graph.cpp (1)
152-159: Precompute sort keys to avoid repeatedcolorDistancework inside the comparator.The comparator recomputes
colorDistance(andstd::sqrt) many times during sort. Consider precomputing a weighted key per neighbor and sorting by that to reduce CPU cost without changing behavior.♻️ Suggested refactor
- std::sort(neighbors.begin(), neighbors.end(), - [col](Node_ptr a, Node_ptr b) { - float cdista = colorDistance(a->color(), col); - float cdistb = colorDistance(b->color(), col); - return (static_cast<float>(a->area()) + 10.f * cdista) < - (static_cast<float>(b->area()) + 10.f * cdistb); - }); + struct NeighborScore { + Node_ptr node; + float score; + }; + std::vector<NeighborScore> scored; + scored.reserve(neighbors.size()); + for (const auto &ne : neighbors) { + const float cdist = colorDistance(ne->color(), col); + const float score = static_cast<float>(ne->area()) + 10.f * cdist; + scored.push_back({ne, score}); + } + std::sort(scored.begin(), scored.end(), + [](const NeighborScore &a, const NeighborScore &b) { + return a.score < b.score; + }); + neighbors.clear(); + neighbors.reserve(scored.size()); + for (const auto &s : scored) { + neighbors.push_back(s.node); + }src/wasm/modules/image/include/cielab_impl.h (1)
98-179: Add round‑trip accuracy tests for the RGB↔LAB templates.Given the forward/inverse matrix constants, add tests that round‑trip RGB→LAB→RGB (and LAB→RGB→LAB) within a defined tolerance to catch coefficient drift and precision regressions.
Based on learnings, add a tolerance-based symmetry test to validate conversion accuracy.
Also applies to: 181-210
src/hooks/useWasmWorker.js (1)
38-72: Normalizen_threadsbefore posting to WASM.Optional: coerce to a positive integer to avoid accidental floats/zeros being sent across the ABI.
♻️ Suggested tweak
const bilateralFilter = async ({ pixels, width, height, sigma_spatial = 3.0, sigma_range = 50.0, color_space = 0, n_threads = 8, }) => { + const threadCount = Math.max(1, Math.trunc(n_threads)); return ( - await call('bilateral_filter', { pixels, width, height, sigma_spatial, sigma_range, color_space, n_threads }, [ + await call('bilateral_filter', { pixels, width, height, sigma_spatial, sigma_range, color_space, n_threads: threadCount }, [ 'pixels', ]) ).output.pixels; }; @@ const kmeans = async ({ pixels, out_pixels = new Uint8ClampedArray(pixels.length), width, height, out_labels = new Int32Array(width * height), num_colors, max_iter = 250, color_space = 0, n_threads = 8, }) => { + const threadCount = Math.max(1, Math.trunc(n_threads)); const result = ( await call( 'kmeans', - { pixels, out_pixels, out_labels, width, height, num_colors, max_iter, color_space, n_threads }, + { pixels, out_pixels, out_labels, width, height, num_colors, max_iter, color_space, n_threads: threadCount }, ['pixels', 'out_pixels', 'out_labels'] ) ).output;
| ImageLib::Image<ImageLib::LABAPixel<float>> lab(pixels.getWidth(), | ||
| pixels.getHeight()); | ||
| if (color_space == COLOR_SPACE_OPTION_CIELAB) { | ||
| std::cout << "KMeans using CIELAB" << std::endl; | ||
| for (int i{0}; i < pixels.getSize(); ++i) { | ||
| rgb_to_lab<float, float>(pixels[i], lab[i]); | ||
| } | ||
| } | ||
|
|
||
| std::vector<std::thread> threads; | ||
|
|
||
| int nthreads = std::clamp(static_cast<int>(n_threads), 1, k); | ||
|
|
||
| int pixels_per_thread{num_pixels / nthreads}; | ||
| int centroids_per_thread{k / nthreads}; | ||
|
|
||
| // Step 2: Initialize centroids randomly | ||
| srand(static_cast<uint32_t>(time(nullptr))); | ||
| // This does not give satisfactory initializations | ||
| /*srand(static_cast<uint32_t>(time(nullptr))); | ||
| for (int32_t i{0}; i < k; ++i) { | ||
| int32_t idx = rand() % num_pixels; | ||
| centroids[i] = pixels[idx]; | ||
| switch (color_space) { | ||
| case COLOR_SPACE_OPTION_RGB : { | ||
| centroids[i] = pixels[idx]; | ||
| break; | ||
| } | ||
| case COLOR_SPACE_OPTION_CIELAB : { | ||
| centroids_lab[i] = lab[idx]; | ||
| break; | ||
| } | ||
| } | ||
| }*/ | ||
|
|
||
| switch (color_space) { | ||
| case COLOR_SPACE_OPTION_RGB: { | ||
| kMeansPlusPlusInit<ImageLib::RGBAPixel<float>>(pixels, centroids, k); | ||
| break; | ||
| } | ||
| case COLOR_SPACE_OPTION_CIELAB: { | ||
| kMeansPlusPlusInit<ImageLib::LABAPixel<float>>(lab, centroids_lab, k); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the kmeans.cpp file
find . -name "kmeans.cpp" -type fRepository: Ryan-Millard/Img2Num
Length of output: 103
🏁 Script executed:
# Read the kmeans.cpp file to see the full context
cat -n src/wasm/modules/image/src/kmeans.cpp | head -250Repository: Ryan-Millard/Img2Num
Length of output: 10110
🏁 Script executed:
# Search for the dist variable and switch statements handling color_space
rg -n "dist" src/wasm/modules/image/src/kmeans.cppRepository: Ryan-Millard/Img2Num
Length of output: 2618
🏁 Script executed:
# Look for switch statements in the file
rg -n "switch" src/wasm/modules/image/src/kmeans.cpp -A 15Repository: Ryan-Millard/Img2Num
Length of output: 4013
🏁 Script executed:
# Check if there's any validation of color_space at the function caller level
rg -n "kmeans\(" src/wasm/modules/image/src/ -A 3Repository: Ryan-Millard/Img2Num
Length of output: 478
🏁 Script executed:
# Check the header file to see if there are any comments about valid values
cat src/wasm/modules/image/src/kmeans.hRepository: Ryan-Millard/Img2Num
Length of output: 131
🏁 Script executed:
# Find the kmeans header file
find . -name "kmeans.h" -type fRepository: Ryan-Millard/Img2Num
Length of output: 105
🏁 Script executed:
# Check where kmeans function is called from
rg -n "kmeans\(" --type cpp -B 2 -A 2 | head -100Repository: Ryan-Millard/Img2Num
Length of output: 902
🏁 Script executed:
# Read the kmeans header file
cat src/wasm/modules/image/include/kmeans.hRepository: Ryan-Millard/Img2Num
Length of output: 739
🏁 Script executed:
# Search for where kmeans is called in bindings or main entry points
rg -n "kmeans\(" src/ --type cpp -B 5 | grep -A 5 -B 5 "color_space"Repository: Ryan-Millard/Img2Num
Length of output: 46
🏁 Script executed:
# Check if COLOR_SPACE constants are exported or used elsewhere
rg -n "COLOR_SPACE_OPTION" src/Repository: Ryan-Millard/Img2Num
Length of output: 2661
Add validation for color_space parameter before processing.
Multiple switch statements (lines 199–208, 230–245, 264–279, 301–310, 335–348, 358–371) lack default cases. If an invalid color_space value is passed, the dist variable at line 298 remains uninitialized, causing undefined behavior when used at line 311. The lab image is also left uninitialized if the value doesn't match COLOR_SPACE_OPTION_CIELAB.
Suggested guard
ImageLib::Image<ImageLib::LABAPixel<float>> lab(pixels.getWidth(),
pixels.getHeight());
+ if (color_space != COLOR_SPACE_OPTION_RGB &&
+ color_space != COLOR_SPACE_OPTION_CIELAB) {
+ return;
+ }🤖 Prompt for AI Agents
In `@src/wasm/modules/image/src/kmeans.cpp` around lines 166 - 208, Validate the
color_space input early and add robust handling for invalid values: check the
color_space parameter (used in switches around COLOR_SPACE_OPTION_RGB and
COLOR_SPACE_OPTION_CIELAB) before any processing and return/throw an error (or
set a safe default) if it's not one of the expected enums; also add default
branches to every switch that currently lacks one (including the initialization
switch that calls kMeansPlusPlusInit, the distance computation switch that sets
dist, and any switches that touch lab or centroids_lab) to either initialize
variables to safe values or abort with a clear error so dist is never left
uninitialized and lab is not left unconstructed when COLOR_SPACE_OPTION_CIELAB
is required. Ensure any early return or exception prevents further use of
uninitialized variables (dist, lab) and keep behavior consistent across
functions that reference color_space.
|
Hi, @Krasner. Please will you update the body of the PR to state which issues this fixes. That will allow the issues to be closed when this PR is merged. For example, you would write: Fixes: #234
Fixes: #xyz |
|
updated |
|
I will return |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/WasmImageProcessor.jsx (1)
16-16: CI blocker:blackThresholdis imported but never used.The pipeline failure confirms ESLint fails because
blackThresholdis destructured but the call is commented out on lines 88-94.🐛 Proposed fix
- const { bilateralFilter, blackThreshold, kmeans, findContours } = useWasmWorker(); + const { bilateralFilter, kmeans, findContours } = useWasmWorker();
🤖 Fix all issues with AI agents
In `@src/utils/image-utils.js`:
- Line 2: Remove the unused named import "number" from 'prop-types' in the
import statement; locate the import line that reads import { number } from
'prop-types' in image-utils and delete the "number" specifier (or remove the
entire import if nothing else from 'prop-types' is used) so ESLint no longer
flags an unused import.
♻️ Duplicate comments (5)
src/components/WasmImageProcessor.jsx (1)
97-101: CI blocker:kmeansedis assigned but never used.Static analysis confirms ESLint fails on line 97. Additionally, per reviewer discussion, using
imgBilateralFilteredinfindContours(line 104) instead ofkmeansedmay cause border mismatches since K-Means shifts coordinates during clustering.If the quantized pixels are intentionally discarded, destructure only
labels. Otherwise, passkmeansedtofindContoursas originally intended.🐛 Option A: Remove unused variable (if intentional)
- const { pixels: kmeansed, labels } = await kmeans({ + const { labels } = await kmeans({🐛 Option B: Use kmeansed in findContours (preserves K-Means benefits)
const contours = await findContours({ - pixels: imgBilateralFiltered, + pixels: kmeansed, labels,src/wasm/modules/image/src/kmeans.cpp (4)
54-58: Fix labels vector type mismatch (won’t compile).
_apply_labelsexpectsstd::vector<int>, but the caller passesstd::vector<int32_t>. This is a hard compile error.🐛 Proposed fix
-void _apply_labels(const ImageLib::Image<PixelT> &pixels, - const std::vector<std::vector<float>> &distances, - std::vector<int> &labels, int start_pixel, int end_pixel, - int k, std::atomic<bool> &changed) { +void _apply_labels(const ImageLib::Image<PixelT> &pixels, + const std::vector<std::vector<float>> &distances, + std::vector<int32_t> &labels, int start_pixel, int end_pixel, + int k, std::atomic<bool> &changed) {
84-91: Guard against empty input in KMeans++ init.
num_pixels == 0ork <= 0makes the uniform distribution invalid.🐛 Proposed fix
- int num_pixels = pixels.getSize(); + const int num_pixels{pixels.getSize()}; + if (k <= 0 || num_pixels <= 0) { + return; + }
163-170: Validatecolor_spaceearly and remove unguarded logging.Invalid
color_spacecan leave LAB buffers and distances undefined; also avoidstd::coutin production.🐛 Proposed fix
- ImageLib::Image<ImageLib::LABAPixel<float>> lab(pixels.getWidth(), - pixels.getHeight()); - if (color_space == COLOR_SPACE_OPTION_CIELAB) { - std::cout << "KMeans using CIELAB" << std::endl; + if (color_space != COLOR_SPACE_OPTION_RGB && + color_space != COLOR_SPACE_OPTION_CIELAB) { + return; + } + ImageLib::Image<ImageLib::LABAPixel<float>> lab(pixels.getWidth(), + pixels.getHeight()); + if (color_space == COLOR_SPACE_OPTION_CIELAB) { for (int i{0}; i < pixels.getSize(); ++i) { rgb_to_lab<float, float>(pixels[i], lab[i]); } }
174-178: Validatekbeforestd::clampand division.
std::clamprequireslow <= high;k <= 0is UB and can lead to divide-by-zero.🐛 Proposed fix
- std::vector<std::thread> threads; - - int nthreads = std::clamp(static_cast<int>(n_threads), 1, k); + std::vector<std::thread> threads; + if (k <= 0 || num_pixels <= 0) { + return; + } + const int nthreads{std::clamp<int>(static_cast<int>(n_threads), 1, k)};
🧹 Nitpick comments (3)
src/wasm/modules/image/CMakeLists.txt (1)
50-51: Consider consolidating-pthreadand-msimd128intoCOMMON_FLAGS.Per the prior review suggestion, adding these flags to
COMMON_FLAGSwould reduce duplication and simplify maintenance.♻️ Proposed consolidation
set(COMMON_FLAGS "SHELL:-s MODULARIZE=1" "SHELL:-s EXPORT_ES6=1" "SHELL:-s EXIT_RUNTIME=1" "SHELL:-s ENVIRONMENT=web,worker" "SHELL:-s EXPORTED_FUNCTIONS=['_malloc','_free']" "SHELL:-s EXPORTED_RUNTIME_METHODS=['ccall','cwrap','getValue','setValue','HEAPU8','HEAP32']" "SHELL:-s INITIAL_MEMORY=1024MB" "SHELL:-s MAXIMUM_MEMORY=2048MB" "SHELL:-s ALLOW_MEMORY_GROWTH=1" "SHELL:-s EXPORT_NAME=create${CAP_MODULE_NAME}Module" "SHELL:-s USE_PTHREADS=1" "SHELL:-s PTHREAD_POOL_SIZE=8" + -pthread + -msimd128 ) # Apply common flags -target_compile_options(${MODULE_NAME}_wasm PRIVATE -pthread -msimd128) -target_link_options(${MODULE_NAME}_wasm PRIVATE ${COMMON_FLAGS} -pthread -msimd128) +target_compile_options(${MODULE_NAME}_wasm PRIVATE ${COMMON_FLAGS}) +target_link_options(${MODULE_NAME}_wasm PRIVATE ${COMMON_FLAGS})src/wasm/modules/image/include/LABPixel.h (1)
20-22: Consider epsilon-based comparison for floating-point types.Direct equality comparison (
==) on floating-point types (float/double, as noted in the comment on line 8) can fail due to precision errors. For pixel equality checks in color processing, this may cause unexpected mismatches.♻️ Optional: Add tolerance-based comparison for floating-point
+#include <cmath> +#include <type_traits> + +// ... + [[nodiscard]] inline bool operator==(const LABPixel &other) const { - return l == other.l && a == other.a && b == other.b; + if constexpr (std::is_floating_point_v<NumberT>) { + constexpr NumberT eps = static_cast<NumberT>(1e-6); + return std::abs(l - other.l) < eps && + std::abs(a - other.a) < eps && + std::abs(b - other.b) < eps; + } else { + return l == other.l && a == other.a && b == other.b; + } }src/wasm/modules/image/include/cielab_impl.h (1)
134-139: Minor: Clamp before assignment for clarity.
out_lis assigned on line 134, then clamped on line 138. For clarity and to avoid any intermediate out-of-range state, consider clamping in-place during assignment.♻️ Proposed simplification
// 4. Output values - out_l = static_cast<Tout>(LAB_L_FACTOR * fy - LAB_L_OFFSET); + out_l = static_cast<Tout>( + std::clamp(LAB_L_FACTOR * fy - LAB_L_OFFSET, 0.0, 100.0)); out_a = static_cast<Tout>(LAB_A_FACTOR * (fx - fy)); out_b = static_cast<Tout>(LAB_B_FACTOR * (fy - fz)); - - out_l = std::clamp(out_l, static_cast<Tout>(0.0), static_cast<Tout>(100.0)); }
|
@Ryan-Millard do you think this is ready to merge. I can address the Pixel data structure issue in the next PR. |
Yes. It is. Please just fix the linting errors and remove these: [19:31] ~/projects/Img2Num/src/wasm/modules/image $ rg "cout" include/ src/
src/kmeans.cpp
166: std::cout << "KMeans using CIELAB" << std::endl;
251: // std::cout << "Computed distances" << std::endl;
[19:32] ~/projects/Img2Num/src/wasm/modules/image $ rg "iostream" include/ src/
include/image_utils.h
6:#include <iostream>
src/kmeans.cpp
13:#include <iostream>
src/kmeans_graph.cpp
16:#include <iostream>
[19:32] ~/projects/Img2Num/src/wasm/modules/image $I'll make an issue now to make a debug suite that evaporates in production. |
|
done |
The issue is #240. |
@Krasner, do you need help with understanding how to use Docusaurus? We don't need much - just API documentation, an explanation of the implementation and a few short extras. |
|
@Ryan-Millard I can update the existing cielab documentation to match the new templated implementation. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/docs/reference/wasm/modules/image/cielab/api.md`:
- Around line 121-128: The parameter type annotations in the table are reversed:
update the entries so inputs L, A, B use the input type (const Tin) and the
outputs r_u8, g_u8, b_u8 use the output reference type (Tout&); specifically
change `L`, `A`, `B` from `Tout` to `const Tin` and change `r_u8`, `g_u8`,
`b_u8` from `Tin&` to `Tout&`, keeping the same ranges and descriptions to match
the function signature for L/A/B and r_u8/g_u8/b_u8.
🧹 Nitpick comments (1)
docs/docs/reference/wasm/modules/image/cielab/api.md (1)
29-37: Consider clarifying parameter naming for templated functions.The parameter names use a
_u8suffix (e.g.,r_u8,g_u8,b_u8) which impliesuint8_ttypes, but the functions are templated to accept arbitrary input/output types. While this may be legacy naming preserved for API compatibility, it could confuse developers about the acceptable types.Consider either:
- Adding a note explaining that
_u8is a legacy naming convention and the parameters accept any type compatible withTin/Tout- Updating parameter names to be more generic (e.g.,
r,g,borr_in,g_in,b_in)This would align better with the templated nature emphasized at lines 14-18 and the note at line 20 suggesting
floatordoublefor CIELAB.Also applies to: 54-61, 96-104, 121-128
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/docs/reference/wasm/modules/image/cielab/api.md (1)
1-251: Fix Prettier formatting to unblock CI.CI reports a Prettier failure for this file; please run the formatter and update the doc accordingly.
|
@Ryan-Millard ready to merge |
Ryan-Millard
left a comment
Ryan-Millard
left a comment
There was a problem hiding this comment.
Thank you very much, @Krasner. You always have such wonderful PRs!
|
This broke the production code last because GitHub Pages is a terrible hosting service.😢 I need to migrate elsewhere soon. Unfortunately, I'll only have time on Monday or Tuesday to do a migration. The problem is that we can't set our own headers, which causes the below: wasmWorker-9o6JCJ9n.js:1 Uncaught (in promise) DataCloneError: Failed to execute 'postMessage' on 'Worker': SharedArrayBuffer transfer requires self.crossOriginIsolated.
at wasmWorker-9o6JCJ9n.js:1:6126
at new Promise (<anonymous>)
at loadWasmModuleToWorker (wasmWorker-9o6JCJ9n.js:1:5380)
at Array.map (<anonymous>)
at Object.loadWasmModuleToAllWorkers (wasmWorker-9o6JCJ9n.js:1:6270)
at wasmWorker-9o6JCJ9n.js:1:4936
at We (wasmWorker-9o6JCJ9n.js:1:3849)
at fr (wasmWorker-9o6JCJ9n.js:1:2093)
at G (wasmWorker-9o6JCJ9n.js:1:13217)
at _e (wasmWorker-9o6JCJ9n.js:1:13448)
crossOriginIsolated
false
crossOriginIsolated=true
true
crossOriginIsolated
falseIt is related to threads. |
|
Oh that sucks! I think cloudflare workers might be a good hosting option but I dont't have experience with that. |
I'm busy looking into the solution. I don't really want to disable the threads, but I think I may need to do it since this project has such a complex build process and most free hosts out there don't support these types of fancy builds. The biggest issue with threads is that we will need to inform users that they need specific headers if they use the library in JS. |
2 participants
Sorry template is overwhelming to use.
Fixes #231
Fixes #236
Multithreading enable for bilateral filter and kmeans. Must enable USE_PTHREADS for emscripten to allow
std::thread. Also vite config must allow browser to enable SharedArrayBuffer.This is done with:
in plugins:
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.