chore(monorepo): refactor entire project to monorepo and setup initial JS package - #250
Conversation
- kmeans won't work for some reason - RGBA always = 0,0,0,255
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
📝 WalkthroughWalkthroughAdds a monorepo layout and build infrastructure: root CMake orchestration, a core C++ library with public header, JS (WASM) and Python bindings, a worker-based JS client/package, React example updates, and assorted .gitignore and cleanup changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Web as Web Component
participant Client as wasmClient.js
participant Worker as wasmWorker.js
participant WASM as WASM Module
participant CppLib as C++ Core
Web->>Client: callWasm({funcName, args, bufferKeys})
activate Client
Client->>Client: gen id, store promise handlers
Client->>Worker: postMessage({id, funcName, args, bufferKeys, returnType})
deactivate Client
activate Worker
Worker->>Worker: validate payload (funcName,args,bufferKeys,returnType)
Worker->>WASM: invoke module[funcName](...)
deactivate Worker
activate WASM
WASM->>CppLib: call img2num::<function>(...)
CppLib->>CppLib: process image data
WASM->>WASM: prepare outputs/buffers
deactivate WASM
activate Worker
Worker->>Worker: extract buffers / free strings if needed
Worker->>Client: postMessage({id, output, returnValue})
deactivate Worker
activate Client
Client->>Client: resolve promise, return {output, returnValue}
Client->>Web: promise resolves
deactivate Client
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @.gitignore:
- Line 22: Replace the overly broad gitignore pattern 'build*' with a
directory-only pattern so you don't accidentally ignore files named like
build.sh or build.config; update the .gitignore entry that currently contains
'build*' to use 'build/' (or '/build/' for root-only) and optionally 'build/**'
to ignore contents while preserving any build.* files.
In `@core/include/cielab_impl.h`:
- Around line 214-219: The computed RGB are stored in local variables r,g,b by
lab_to_rgb<Tin, Tout>(laba.l, laba.a, laba.b, r, g, b) but never written back
into the output struct, so rgba.red/green/blue remain unchanged; after the call
assign rgba.red = static_cast<Tout>(r), rgba.green = static_cast<Tout>(g),
rgba.blue = static_cast<Tout>(b) (and keep the existing rgba.alpha =
static_cast<Tout>(laba.alpha)); update these assignments near the lab_to_rgb
call referencing the local r/g/b, the rgba variable, and laba to ensure the
converted values are returned.
In `@core/src/image_utils.cpp`:
- Around line 9-10: Remove the unconditional std::cout debug print inside
gaussian_blur_fft: locate the debug output call within the gaussian_blur_fft
function and either delete it or guard it behind a verbose/debug flag (e.g., a
global or passed-in bool like debug/verbose) so that no user-visible stdout
occurs in hot paths (and especially in WASM builds); update any related
includes/usings if std::cout is no longer needed.
In `@example-apps/react-js/vite.config.js`:
- Around line 43-46: The sitemap configuration using VitePluginSitemap omits the
public /about route; update the VitePluginSitemap call to include "/about" in
the dynamicRoutes array (alongside "/" and "/credits") so the generated sitemap
includes the about page; modify the dynamicRoutes parameter inside the
VitePluginSitemap invocation to add "/about" to ensure proper SEO coverage.
In `@packages/js/package.json`:
- Line 22: The package.json currently uses an invalid ESM type value ("type":
"esm"); update the package.json "type" field to the valid Node.js ESM value
"module" so the package is treated as ESM (replace the "type" property value
from "esm" to "module"); ensure no other tooling or scripts expect the old value
and run a quick local install/test to confirm ESM imports resolve correctly.
In `@packages/js/wasmClient.js`:
- Around line 51-56: terminateWasmWorker currently calls callbacks.clear() which
drops all pending promise handlers without rejecting them; instead iterate over
the callbacks Map (the stored resolve/reject pairs), call each reject with a
clear Error (e.g., "WASM worker terminated") before clearing the map, then
terminate the worker and set initialized = false; update terminateWasmWorker to
reject each pending callback from the callbacks collection (the same collection
used by create/execute functions) to ensure awaiting callers receive a
rejection.
🧹 Nitpick comments (10)
example-apps/react-js/vite.config.js (2)
3-6: Remove unused imports.
exec,fg(fast-glob), andfsare imported but never used in this configuration file.♻️ Proposed fix
import { defineConfig } from "vite"; import react from "@vitejs/plugin-react-swc"; -import { exec } from "child_process"; import path from "path"; -import fg from "fast-glob"; -import fs from "fs"; import { imagetools } from "vite-imagetools"; import generateContributorCreditsPlugin from "./scripts/generate-contributor-credits-json.js"; import VitePluginSitemap from "vite-plugin-sitemap";
22-25: Duplicate COOP/COEP header configuration.The same
Cross-Origin-Opener-PolicyandCross-Origin-Embedder-Policyheaders are set both inserver.headers(lines 22-25) and via theforce-security-headersmiddleware plugin (lines 53-54). The middleware approach is more comprehensive as it intercepts all responses, making theserver.headersblock redundant.Consider removing the
server.headersblock or keeping only one approach for clarity.♻️ Proposed fix — keep only the middleware plugin
server: { host: "0.0.0.0", // Allow connections from outside Docker port: 5173, // Match docker-compose port fs: { allow: [ path.resolve(__dirname), path.resolve(__dirname, '../../packages/js') ], }, - headers: { - "Cross-Origin-Opener-Policy": "same-origin", - "Cross-Origin-Embedder-Policy": "require-corp", - }, },Also applies to: 49-58
packages/js/package.json (1)
5-9: Deduplicate keywords."computer-vision" is listed twice.
♻️ Proposed fix
"keywords": [ "img2num", - "computer-vision", "computer-vision", "vectorization", "image-to-svg" ],core/CMakeLists.txt (1)
11-12: Add CONFIGURE_DEPENDS to keep globbed sources in sync.Without it, newly added files won’t be picked up until a manual reconfigure.
♻️ Proposed fix
-file(GLOB_RECURSE CORE_SRC "${CMAKE_CURRENT_SOURCE_DIR}/src/*.cpp") -file(GLOB_RECURSE CORE_HEADERS "${CMAKE_CURRENT_SOURCE_DIR}/include/*.h") +file(GLOB_RECURSE CORE_SRC CONFIGURE_DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/src/*.cpp") +file(GLOB_RECURSE CORE_HEADERS CONFIGURE_DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/include/*.h")bindings/js/src/wasm_wrapper.cpp (1)
1-3: Drop or guard unused embind include to avoid toolchain errors.The file doesn’t use embind, and static analysis reports
emscripten/bind.hmissing. Removing (or guarding) the include reduces dependency on embind headers.♻️ Proposed fix (remove unused include)
-#include <emscripten/bind.h> `#include` "image_utils.h" `#include` "exported.h"bindings/js/CMakeLists.txt (1)
31-44: Validate 1–2 GB WASM memory defaults for browser targets.These values can cause instantiation failures on many devices/browsers. Consider lowering defaults or making them configurable via cache variables.
🔧 Example config-based approach
+set(WASM_INITIAL_MEMORY "256MB" CACHE STRING "Initial WASM memory") +set(WASM_MAXIMUM_MEMORY "1024MB" CACHE STRING "Max WASM memory") set(COMMON_FLAGS @@ - "SHELL:-s INITIAL_MEMORY=1024MB" - "SHELL:-s MAXIMUM_MEMORY=2048MB" + "SHELL:-s INITIAL_MEMORY=${WASM_INITIAL_MEMORY}" + "SHELL:-s MAXIMUM_MEMORY=${WASM_MAXIMUM_MEMORY}"packages/js/wasmClient.js (1)
16-28: Consider adding anonerrorhandler for uncaught worker errors.The worker currently only has an
onmessagehandler. If the worker throws an uncaught error (e.g., during module initialization or an unhandled rejection), it won't be surfaced to the caller.Proposed addition
worker.onmessage = ({ data }) => { const { id, error, output, returnValue } = data; const cb = callbacks.get(id); if (!cb) return; error ? cb.reject(error) : cb.resolve({ output, returnValue }); callbacks.delete(id); }; + worker.onerror = (e) => { + console.error("WASM worker error:", e.message); + }; + initialized = true;example-apps/react-js/src/components/WasmImageProcessor.jsx (1)
20-26: Minor:URL.revokeObjectURLon a data URL is a no-op.
filteredSrcis set viacanvas.toDataURL()(line 89), which produces a data URL string, not an object URL. CallingURL.revokeObjectURL()on a data URL has no effect. This is harmless but could be clarified.Proposed clarification
useEffect(() => { return () => { if (originalSrc) URL.revokeObjectURL(originalSrc); - if (filteredSrc) URL.revokeObjectURL(filteredSrc); + // filteredSrc is a data URL, no revocation needed }; }, [originalSrc, filteredSrc]);packages/js/safeWasmWrappers.js (2)
1-4: Module import triggers side effect (worker initialization).Calling
initWasmWorker()at module scope means importing this module (or anything that re-exports it, likeindex.js) will immediately spawn a Web Worker. This is convenient but:
- Prevents tree-shaking if only some exports are used
- Cannot defer worker creation until actually needed
- Initialization errors cannot be caught by the importer
If this is intentional for the "just works" developer experience, consider documenting this behavior. Otherwise, consider requiring explicit
init()calls.Alternative: Lazy initialization
-import { initWasmWorker, callWasm } from "./wasmClient.js"; - -// Ensure worker is ready as soon as this module is imported -initWasmWorker(); +import { initWasmWorker, callWasm } from "./wasmClient.js"; + +// Lazily initialize on first call +async function ensureInit() { + initWasmWorker(); // idempotent +} export async function gaussianBlur({ pixels, width, height, sigma_pixels = width * 0.005, }) { + ensureInit(); return ( await callWasm(Apply
ensureInit()at the start of each wrapper function.
97-119: VerifyfindContoursfunction name mapping.The wrapper calls
"kmeans_clustering_graph"which suggests this function does more than just "find contours" — it appears to perform clustering and graph-based contour extraction. The function namefindContoursmay be misleading if it's actually doing k-means clustering as a prerequisite.Consider renaming to better reflect the underlying operation, or document that this expects pre-clustered
labelsinput.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
367da31 to
47746e8
Compare
Ryan-Millard
changed the title
Ryan-Millard
changed the title
Ryan-Millard
changed the title
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@example-apps/react-js/vite.config.js`:
- Around line 11-20: The config uses __dirname in ESM which is undefined; derive
a root directory from import.meta.url (e.g., use fileURLToPath(import.meta.url)
and path.dirname) at the top of the file, then replace every __dirname
occurrence in defineConfig — specifically in server.fs.allow (the array entries
currently using path.resolve(__dirname)) and in resolve.alias entries — to use
that derived root (e.g., path.resolve(rootDir, '...')) and ensure you import
fileURLToPath from 'url' and path from 'path' so the new root variable is
available to all replacements.
🧹 Nitpick comments (1)
example-apps/react-js/vite.config.js (1)
40-58: Avoid duplicate COOP/COEP header sources.Line 22-25 already set these headers via
server.headers, while Line 50-55 sets them again in middleware. Consider keeping a single source of truth to prevent future drift.♻️ Proposed simplification
generateContributorCreditsPlugin(), - // For multithreading (in img2num js package) - { - 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(); - }); - }, - },
|
make sure |
That is definitely where things are going to be different and there won't be a way to fix it. Previously, the app and the library were a single unit. Now, they are separate entities and the app imports the library like it would with any other library in JavaScript. That means that the library will need separate compilation from the app itself and the app will never trigger recompilation of the library since it is just a consumer of the library.
|
Ryan-Millard
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@example-apps/react-js/src/components/WasmImageProcessor.jsx`:
- Around line 20-26: The cleanup in the useEffect erroneously calls
URL.revokeObjectURL(filteredSrc) even though filteredSrc is created via
canvas.toDataURL() (a data: URI) in the image processing flow, so either stop
revoking filteredSrc or change how filteredSrc is produced; update the code in
WasmImageProcessor.jsx by removing the revoke call for filteredSrc in the return
cleanup of the useEffect (leave revoke for originalSrc), or alternatively change
the canvas result path (where canvas.toDataURL() is used) to create a Blob and
set filteredSrc via URL.createObjectURL(blob) so the existing
URL.revokeObjectURL(filteredSrc) is valid; ensure references to filteredSrc,
originalSrc, useEffect, and the canvas.toDataURL() call are updated
consistently.
In `@packages/js/wasmClient.js`:
- Line 26: The rejection currently calls cb.reject(error) where the worker sends
back { error: error.message } (a string), which loses stack and fails instanceof
checks; update the rejection logic around cb.reject/ cb.resolve to ensure you
always reject with an Error object: if error is already an Error pass it
through, otherwise construct a new Error using error.message if present or
String(error) and call cb.reject(thatError). Reference the existing symbols
cb.reject, cb.resolve and the error variable so the change is localized to the
same callback/resolution branch.
- Around line 36-45: callWasm is dropping additional properties like returnType
because it only destructures { funcName, args, bufferKeys } and forwards a
limited payload to worker.postMessage; update callWasm to accept and forward
returnType (and any other extra fields) to the worker by including returnType in
the parameters passed to worker.postMessage so wrappers such as
safeWasmWrappers.findContours which pass returnType: "string" arrive intact at
the worker.
- Around line 21-28: The worker has no error handler so load/init failures leave
the callbacks Map hanging; add a worker.onerror handler (and consider
worker.onmessage fallback) that iterates through callbacks (the Map referenced
as callbacks) and calls each callback.reject with the received Error/event, then
clears callbacks.delete for each id (or callbacks.clear()), and optionally log
the error via your logger; implement this near the existing worker.onmessage
code so that any worker load or runtime error rejects all pending promises
instead of leaving them unresolved.
🧹 Nitpick comments (3)
packages/js/wasmClient.js (1)
13-14:initWasmWorkeris not async — callers have no way to know when the worker/WASM module is actually ready.The worker internally loads and initializes the WASM module asynchronously (
readyPromiseinwasmWorker.js).initWasmWorkersetsinitialized = truesynchronously after creating the worker, but the WASM module may not yet be loaded. IfcallWasmis invoked immediately afterinitWasmWorker, the message is queued and processed only afterreadyPromiseresolves in the worker, which works — but any initialization failure in the worker is completely invisible to the caller. Consider returning a promise that resolves once the worker signals readiness.packages/js/safeWasmWrappers.js (1)
3-4: Module-level side effect: importing this file eagerly creates a Web Worker.
initWasmWorker()runs at import time, meaning any consumer that imports from this module (or transitively viaindex.js) will immediately spin up a Web Worker and begin loading WASM — even if they never call any wrapper. This can be wasteful in SSR/test environments and surprising for library consumers. Consider lazy initialization (e.g., init on firstcallWasm) or leaving init to the consumer.example-apps/react-js/src/components/WasmImageProcessor.jsx (1)
104-154:useMemoforLoadedStateis ineffective —progresschanges on every step.
LoadedStatedepends onprogress, which changes multiple times during processing. Each change triggers a re-computation of the memo, negating any benefit. Either extract the progress-dependent part into a child component or simply inline this as regular render logic.EmptyState(line 104) has the same pattern but with[]deps, which is fine.
…erly * `npm link ../../packages/js` still required for `example-apps/react-js`
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/bilateral_filter.cpp (1)
91-93:⚠️ Potential issue | 🔴 CriticalPre-existing out-of-bounds read:
cie_imageaccessed unconditionally in RGB mode.Lines 91–93 read from
cie_image[neighbor_idx]on every iteration regardless ofcolor_space. Whencolor_space == COLOR_SPACE_OPTION_RGB,cie_imageis never resized (remains empty — see Line 200–201), so these reads are undefined behavior.The values are only used inside the
COLOR_SPACE_OPTION_CIELABswitch case, but the reads themselves still occur and constitute UB.🐛 Proposed fix — guard the CIELAB reads
uint8_t r{image[neighbor_idx]}; uint8_t g{image[neighbor_idx + 1]}; uint8_t b{image[neighbor_idx + 2]}; - 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]; + }
🤖 Fix all issues with AI agents
In `@bindings/js/CMakeLists.txt`:
- Around line 34-38: The CMake Emscripten flags should not force runtime exit or
an oversized initial heap: change "SHELL:-s EXIT_RUNTIME=1" to use
EXIT_RUNTIME=0 so the Web Worker (see wasmWorker.js) keeps the runtime/atexit
handlers intact across calls, and reduce "SHELL:-s INITIAL_MEMORY=1024MB" to a
much smaller value (e.g., 64–256MB) while keeping ALLOW_MEMORY_GROWTH=1 so
memory can grow on demand; update the flags where they are set in
bindings/js/CMakeLists.txt (look for the lines exporting EXIT_RUNTIME and
INITIAL_MEMORY) and run a quick test to verify the worker remains stable across
multiple invocations.
In `@core/include/image_utils.h`:
- Around line 6-8: The header pulls heavy dependencies unnecessarily: remove the
includes "Image.h", "PixelConverters.h", and "RGBAPixel.h" from image_utils.h
and replace them with the minimal required include <cstdint> so the
quantize(uint8_t*, size_t, int) prototype (or whatever quantize signature is
present) compiles; then move the removed includes into image_utils.cpp where the
quantize implementation (function named quantize) actually needs Image,
PixelConverters, and RGBAPixel definitions (or add forward declarations there if
appropriate).
In `@core/src/image_utils.cpp`:
- Around line 13-33: quantize currently divides by region_size and will UB if
region_size == 0; add a defensive early check at the top of the quantize
function to handle region_size == 0 (e.g., return the original value or a
sensible default) before any division, so bucket/bucket_boundary/bucket_midpoint
computation never runs; update any callers/tests if they rely on a different
behavior.
- Around line 115-117: The threshold_image function computes REGION_SIZE as 255
/ num_thresholds which can divide by zero or produce zero for out-of-range
inputs; add an explicit guard at the start of threshold_image to validate
num_thresholds is within [1,255] (or clamp it to that range) and handle invalid
values by returning early or setting a safe default before computing
REGION_SIZE, so downstream calls (including quantize which does a division by
REGION_SIZE) never see a zero divisor.
In `@core/src/kmeans.cpp`:
- Around line 228-234: The loop writing pixel RGBA values can cast centroid
float components out of range directly to uint8_t (undefined/wrapping); modify
the loop in kmeans.cpp where centroids, labels, out_data and num_pixels are used
to clamp centroids[cluster].red/green/blue to the [0.0f, 255.0f] range before
casting to uint8_t (e.g., use std::clamp or explicit min/max), then static_cast
the clamped value for out_data[i*4 + 0..2]; keep alpha assignment as 255.
- Around line 46-61: The loop currently uses PixelT::colorDistance (which
returns the Euclidean distance) and stores it in min_dist_sq, so selection
probabilities are proportional to D(x) not D(x)^2; change the update to use
squared distances everywhere: compute the squared distance (either by calling a
PixelT::colorDistanceSq or by squaring the returned distance) when
assigning/updating min_dist_sq[j] and when accumulating sum_dist_sq, and ensure
the sampling step that uses min_dist_sq also expects squared distances; update
references to PixelT::colorDistance, min_dist_sq, sum_dist_sq, and
centroids.back() accordingly so the weighting matches k-means++ (proportional to
D(x)^2).
- Around line 93-96: The kmeans function lacks validation for the cluster count
parameter k, which can cause invalid Image construction and out-of-bounds
behavior in kMeansPlusPlusInit; update the start of img2num::kmeans to compute
num_pixels = width * height and verify k is between 1 and num_pixels (e.g., if k
< 1 or k > num_pixels handle by clamping to valid range or returning/logging an
error), and return early or adjust k before any use of Image{..., k, 1} or
calling kMeansPlusPlusInit so subsequent code only runs with a valid k.
In `@packages/js/wasmWorker.js`:
- Around line 79-86: Move the validation that currently throws (checks for
funcName, args, bufferKeys, returnType) inside the onmessage handler's try block
(the same try that wraps callWasm) or replace the throws with self.postMessage({
id, error: <string or serialized error> }) followed by return so the caller's
promise (callWasm in wasmClient.js) is settled; also remove or adjust the
now-dead bufferKeys?.length && !args branch (since args==null is already
handled) and ensure error payload includes the original id and a clear message
referencing funcName so the caller can correlate responses.
🧹 Nitpick comments (8)
bindings/js/CMakeLists.txt (2)
17-17:BINDING_HEADERScollected but never used.
GLOB_RECURSE BINDING_HEADERSis populated but never added to the target (e.g., viatarget_sourcesor as part ofadd_executable). This makes it a dead variable. Either add it to the target for IDE visibility or remove the line.
56-61:CMAKE_SOURCE_DIRis appropriate for this project structure.This CMakeLists.txt is always included via
add_subdirectory(bindings/js)from the root project, soCMAKE_SOURCE_DIRreliably resolves to the repository root. The conditional concern in the original comment is not applicable here. If you wish to improve robustness as a general best practice,PROJECT_SOURCE_DIRwould be equivalent and slightly more defensive, but the current implementation is correct for this project.core/include/image_utils.h (1)
10-10: Movequantizeintonamespace img2numfor consistency.The function is declared and implemented at global scope in both
core/include/image_utils.handcore/src/image_utils.cpp, but all other image-processing functions (e.g.,gaussian_blur_fft,invert_image,threshold_image) are scoped withinnamespace img2num. Sincequantizeis used exclusively withinthreshold_imageand is not exposed elsewhere, wrapping it in the namespace would align with the broader codebase organization.core/img2num.h (2)
22-36:bilateral_filterdoc comment is incomplete — missingcolor_spaceparameter.The comment documents
image,width,height,sigma_spatial, andsigma_range, but omits thecolor_spaceparameter, which selects between RGB and CIELAB filtering modes. Also, line 24 is missing a space after the asterisk (*Apply→* Apply).📝 Proposed fix
// bilateral_filter.cpp /* - *Apply bilateral filter to an image. - *The filter modifies the image buffer in-place. - *Parameters: + * Apply bilateral filter to an image. + * The filter modifies the image buffer in-place. + * Parameters: * - image: Pointer to RGBA pixel buffer * - width, height: Image dimensions (px) * - sigma_spatial: Gaussian standard deviation for spatial proximity (spatial * decay) * - sigma_range: Gaussian standard deviation for intensity difference * (radiometric decay) + * - color_space: Color space selector (e.g., RGB or CIELAB) */
38-41: Document ownership semantics forlabels_to_svgin the public header.The function returns a raw
char*pointer allocated withmalloc(). While the C implementation explicitly documents this pattern ("Dynamic C-style allocation (since returned over C ABI)") in the source file, the public header has no ownership documentation. This creates ambiguity for C/C++ callers about whether they own the memory and must callfree().Add a brief doc comment to the header to clarify the contract:
📝 Suggested documentation
// labels_to_svg.cpp + // Returns a heap-allocated SVG string. The caller owns the memory + // and must free it with free(). char *labels_to_svg(uint8_t *data, int32_t *labels, const int width, const int height, const int min_area, const bool draw_contour_borders);core/src/image_utils.cpp (2)
122-122: Unused variablesimgWidthandimgHeight.These are declared but never referenced in either
threshold_image(line 122) orblack_threshold_image(line 139).🧹 Proposed fix
- const auto imgWidth{img.getWidth()}, imgHeight{img.getHeight()}; for (ImageLib::RGBAPixel<uint8_t> &p : img) {Also applies to: 139-139
99-112: Repeated load → process → memcpy pattern across three functions.
invert_image,threshold_image, andblack_threshold_imageeach repeat the same boilerplate:loadFromBuffer→ iterate pixels →memcpyback. Consider extracting a helper that accepts a pixel-transform callable to reduce duplication. Not blocking, but worth noting for future cleanup.Also applies to: 115-132, 134-152
bindings/js/src/wasm_wrapper.cpp (1)
1-3: Consider using quotes for the project header include.
<img2num.h>uses angle brackets, which conventionally denote system/external headers. Since this is a project-internal header,"img2num.h"would be more idiomatic and signals the dependency relationship more clearly. Both work if the include path is configured in CMake, but the convention helps readers.🧹 Proposed fix
`#include` <emscripten/emscripten.h> -#include <img2num.h> +#include "img2num.h"
@Krasner that's true, but we still need to handle the concerns CodeQL raised (just in case we ever change things), so I'm going to do what it recommends and cast to an int. |
…for core and js bindings - add base Doxyfile and per-package configs - add generation script - prepare output directories for internal API docs - update ignore rules - minor header adjustments
…reen support - Added api-reference.md as proxies for generated HTML - Added FullscreenWrapper component for full-page view - Hooked Doxygen-generated HTML into static docs site
…ption - Update read package.json script, too - Fix undeclared script
39b13c3 to
49d5bc8
Compare
|
good job! |
Thanks. I finally merged it because I realized that I was just messing around with things that should come later on down the line. Doxygen, for example, was a pain and still is because I can't find a good way to convert the XMLA output to markdown. I embedded the HTML in the docs website, so we at least have that. |
3 participants
What was changed & why
Complete repository refactor & conversion of C++ into an object library with JS bindings.
Img2Num's C++ module has always been reusable and this refactor has been needed for a long time. This change enables that.
Fixes: #207
Fixes: #213
Changes
Builds & Dev
Split logically to modularize the project. This prevents the tight coupling we previously faced.
C++
Currently,
coreis an object library and onlybindings/jsimports it.React
Not much has changed - just the package manager.
Docusaurus
Also only the package manager.
C++ & JS Module
Split into 3 pieces:
useWasmWorkerhook, but is now a full JS package imported intoWasmImageProcessorin the React example.img2num.Filesystem
Completely changed:
src/wasm->core&bindings/jsimg2num.his imported into the WASM bindings.packages/js/build-wasmsrc/workers/wasmWorker.js&src/hooks/useWasmWorker.js->packages/jspackages/jsimports builtbindings/jsfrompackages/js/build-wasm./->example-apps/react-jsimport ... from 'img2num' becausepackages/js` is now a proper JS packagescripts&scripts/img2num-dev-scriptsimg2num-dev-scriptsis a workspace package imported by all the otherpackage.jsonfiles.Package Manager
Completely changed. It is now
pnpminstead ofnpmbecausepnpmis better at managing monorepos (although Docusaurus is a pain - not the fault ofpnpm).Docker
Only the
Dockerfile.devwas updated. This isn't a major change - the main addition ispnpm.Testing & Verification
example-apps/react-js) serves as a good way to test the usability of the packaged library.Additional Resources
Coming soon
Summary by CodeRabbit
New Features
Documentation
Chores