feat(WASM): add bilateral filter - #176
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds an in-place bilateral filter to the WASM image module, exposes Changes
Sequence Diagram(s)sequenceDiagram
participant UI as React Component
participant Hook as useWasmWorker Hook
participant WASM as WASM Module
participant Buf as Image Buffer
UI->>Hook: bilateralFilter(pixels, width, height, sigma_spatial?, sigma_range?)
Hook->>WASM: call({ funcName: "bilateral_filter", args..., bufferKeys: ["pixels"] })
WASM->>Buf: Read RGBA input buffer
Note over WASM: Precompute spatial kernel and range LUT
WASM->>WASM: For each pixel: iterate neighborhood → combine spatial & range weights → accumulate RGB
WASM->>Buf: Write filtered RGB results (preserve alpha)
WASM-->>Hook: Return { pixels: filteredBuffer }
Hook-->>UI: Return filteredBuffer
UI->>Hook: blackThreshold(filteredBuffer) → continue pipeline (kmeans, mergeSmallRegionsInPlace)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 0
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)
149-149: Fix the dependency array: addbilateralFilter, removegaussianBlur.The
processImagecallback usesbilateralFilter(line 87) but it's missing from the dependency array. Meanwhile,gaussianBluris listed but no longer used in the function body. This violates React's rules of hooks and could cause stale closure bugs.🔎 Suggested fix
- }, [fileData, gaussianBlur, blackThreshold, kmeans, mergeSmallRegionsInPlace, navigate, step]); + }, [fileData, bilateralFilter, blackThreshold, kmeans, mergeSmallRegionsInPlace, navigate, step]);
🧹 Nitpick comments (4)
src/hooks/useWasmWorker.js (1)
38-40: Consider consistent parameter naming across filter functions.The
bilateralFilterfunction usesimageas the parameter name for the pixel buffer, whilegaussianBlurandblackThresholdusepixels. This inconsistency could cause confusion when switching between filters in the pipeline (as seen inWasmImageProcessor.jsxwhere the caller must use different property names).🔎 Suggested consistency fix
- const bilateralFilter = async ({ image, width, height, sigma_spatial = 3.0, sigma_range = 50.0 }) => { - return (await call('bilateral_filter', { image, width, height, sigma_spatial, sigma_range }, ['image'])).output.image; + const bilateralFilter = async ({ pixels, width, height, sigma_spatial = 3.0, sigma_range = 50.0 }) => { + return (await call('bilateral_filter', { image: pixels, width, height, sigma_spatial, sigma_range }, ['image'])).output.image; };src/components/WasmImageProcessor.jsx (1)
16-16: Remove unusedgaussianBlurfrom destructuring.
gaussianBluris imported but only referenced in commented-out code. Consider removing it from the destructuring to avoid confusion and reduce the bundle impact (if tree-shaking doesn't remove it).🔎 Suggested fix
- const { gaussianBlur, bilateralFilter, blackThreshold, kmeans, mergeSmallRegionsInPlace } = useWasmWorker(); + const { bilateralFilter, blackThreshold, kmeans, mergeSmallRegionsInPlace } = useWasmWorker();src/wasm/modules/image/src/bilateral_filter.cpp (2)
8-8: Remove unused constant.
MAX_PIXEL_VALis defined but never used in the implementation.🔎 Suggested fix
static constexpr double SIGMA_RADIUS_FACTOR = 3.0; -static constexpr int MAX_PIXEL_VAL = 255; // Max possible squared Euclidean distance in a 3-channel 8-bit image: 255^2 * 3 = 195075
29-29: Remove unused variablestride.The
stridevariable is computed but never referenced in the code.🔎 Suggested fix
const int kernel_width = 2 * radius + 1; - const size_t stride = width * 4; std::vector<uint8_t> result(width * height * 4);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/WasmImageProcessor.jsxsrc/hooks/useWasmWorker.jssrc/wasm/modules/image/include/image_utils.hsrc/wasm/modules/image/src/bilateral_filter.cpp
🧰 Additional context used
🧬 Code graph analysis (2)
src/wasm/modules/image/include/image_utils.h (1)
src/wasm/modules/image/src/bilateral_filter.cpp (2)
bilateral_filter(23-103)bilateral_filter(23-24)
src/components/WasmImageProcessor.jsx (1)
src/hooks/useWasmWorker.js (3)
useWasmWorker(4-53)bilateralFilter(38-40)blackThreshold(41-43)
🔇 Additional comments (7)
src/wasm/modules/image/include/image_utils.h (1)
24-26: LGTM!The function declaration is consistent with the existing API style and correctly uses the
EXPORTEDmacro. The signature matches the implementation inbilateral_filter.cpp.src/hooks/useWasmWorker.js (1)
52-52: LGTM!The
bilateralFilterfunction is correctly exposed in the return object alongside the other image processing functions.src/components/WasmImageProcessor.jsx (1)
84-91: Good documentation of the design decision.The comment clearly explains why the bilateral filter is preferred over Gaussian blur for this use case. The implementation correctly passes the pixel data and dimensions to the filter.
src/wasm/modules/image/src/bilateral_filter.cpp (4)
7-11: LGTM on constants definition.The constants are well-documented and correctly calculated.
MAX_RGB_DIST_SQ = 195075is the theoretical maximum squared Euclidean distance for RGB channels.
23-25: LGTM!The early return for non-positive sigma values prevents undefined behavior and is a sensible guard.
55-100: Solid bilateral filter implementation.The algorithm correctly:
- Clamps neighbor coordinates to handle border pixels
- Combines spatial and range weights using precomputed LUTs
- Normalizes by accumulated weight (safe from division by zero since center pixel always contributes weight ≥ 1.0)
- Preserves the alpha channel from the center pixel
102-102: LGTM!The final copy back to the input buffer correctly implements the in-place modification semantics.
822e331 to
073e6f8
Compare
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)
src/hooks/useWasmWorker.test.js (1)
57-71: Update test to include bilateralFilter in the API surface check.The test verifies that helper methods are returned by the hook, but bilateralFilter is not included in the assertions (lines 61-64 and 67-70). Since bilateralFilter is now part of the public API (added in line 52 of useWasmWorker.js), it should be verified here.
🔎 Suggested fix
it('should return call function and helper methods', () => { const { result } = renderHook(() => useWasmWorker()); expect(result.current).toHaveProperty('call'); expect(result.current).toHaveProperty('gaussianBlur'); + expect(result.current).toHaveProperty('bilateralFilter'); expect(result.current).toHaveProperty('blackThreshold'); expect(result.current).toHaveProperty('kmeans'); expect(result.current).toHaveProperty('mergeSmallRegionsInPlace'); expect(typeof result.current.call).toBe('function'); expect(typeof result.current.gaussianBlur).toBe('function'); + expect(typeof result.current.bilateralFilter).toBe('function'); expect(typeof result.current.blackThreshold).toBe('function'); expect(typeof result.current.kmeans).toBe('function'); expect(typeof result.current.mergeSmallRegionsInPlace).toBe('function'); });
🧹 Nitpick comments (3)
src/hooks/useWasmWorker.js (1)
38-40: Consider renaming parameter for API consistency.The bilateralFilter function uses
imageas the buffer parameter name, while all other helper methods (gaussianBlur, blackThreshold, kmeans, mergeSmallRegionsInPlace) usepixels. For API consistency and a more intuitive developer experience, consider renamingimagetopixels.🔎 Suggested refactor for naming consistency
- const bilateralFilter = async ({ image, width, height, sigma_spatial = 3.0, sigma_range = 50.0 }) => { - return (await call('bilateral_filter', { image, width, height, sigma_spatial, sigma_range }, ['image'])).output.image; + const bilateralFilter = async ({ pixels, width, height, sigma_spatial = 3.0, sigma_range = 50.0 }) => { + return (await call('bilateral_filter', { image: pixels, width, height, sigma_spatial, sigma_range }, ['image'])).output.image; };This would align with the naming convention used throughout the hook while still passing the correct parameter name to the underlying WASM function.
src/wasm/modules/image/src/bilateral_filter.cpp (2)
7-11: Remove unused constant MAX_PIXEL_VAL.The constant MAX_PIXEL_VAL is defined but never referenced in the implementation. Consider removing it to reduce code clutter.
🔎 Suggested cleanup
static constexpr double SIGMA_RADIUS_FACTOR = 3.0; -static constexpr int MAX_PIXEL_VAL = 255; // Max possible squared Euclidean distance in a 3-channel 8-bit image: 255^2 * 3 = 195075 // Means max delta between images (imageA - imageB) in RGB channels (255^2 * 3) static constexpr int MAX_RGB_DIST_SQ = 255 * 255 * 3;
27-30: Remove unused variable stride.The variable stride is calculated on Line 29 but is never used in the implementation. Consider removing it.
🔎 Suggested cleanup
const int radius = static_cast<int>(std::ceil(SIGMA_RADIUS_FACTOR * sigma_spatial)); const int kernel_width = 2 * radius + 1; - const size_t stride = width * 4; std::vector<uint8_t> result(width * height * 4);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/WasmImageProcessor.jsxsrc/hooks/useWasmWorker.jssrc/hooks/useWasmWorker.test.jssrc/wasm/modules/image/include/image_utils.hsrc/wasm/modules/image/src/bilateral_filter.cpp
🧰 Additional context used
🧬 Code graph analysis (3)
src/wasm/modules/image/include/image_utils.h (1)
src/wasm/modules/image/src/bilateral_filter.cpp (2)
bilateral_filter(23-103)bilateral_filter(23-24)
src/components/WasmImageProcessor.jsx (2)
src/hooks/useWasmWorker.js (5)
useWasmWorker(4-53)bilateralFilter(38-40)blackThreshold(41-43)kmeans(44-46)mergeSmallRegionsInPlace(47-50)src/wasm/modules/image/src/mergeSmallRegionsInPlace.cpp (2)
mergeSmallRegionsInPlace(51-117)mergeSmallRegionsInPlace(51-52)
src/hooks/useWasmWorker.test.js (1)
src/workers/wasmWorker.js (1)
result(42-42)
🔇 Additional comments (7)
src/hooks/useWasmWorker.test.js (1)
205-256: LGTM! Comprehensive test coverage for bilateralFilter.The test suite correctly validates both default parameter behavior (sigma_spatial: 3.0, sigma_range: 50.0) and custom parameter overrides. The test structure follows the established pattern from gaussianBlur tests, ensuring consistency.
src/wasm/modules/image/include/image_utils.h (1)
24-25: LGTM! Function prototype correctly declared.The bilateral_filter prototype matches the implementation and uses the EXPORTED macro for proper WebAssembly visibility.
src/hooks/useWasmWorker.js (1)
52-52: LGTM! Export correctly updated.The return statement properly includes bilateralFilter in the public API.
src/components/WasmImageProcessor.jsx (2)
84-91: LGTM! Clear rationale for the filter choice.The switch from Gaussian blur to bilateral filter is well-documented with the inline comment explaining that bilateral filtering preserves sharp edges. The implementation correctly passes the image buffer and dimensions with sensible default sigma values.
149-149: LGTM! Dependencies correctly updated.The useCallback dependencies array properly reflects the change from gaussianBlur to bilateralFilter.
src/wasm/modules/image/src/bilateral_filter.cpp (2)
23-25: LGTM! Input validation with early return.The function correctly validates sigma parameters and returns early for invalid values, leaving the image buffer unchanged. This is a safe default behavior.
32-102: LGTM! Solid bilateral filter implementation.The implementation is algorithmically correct with several good design choices:
- Precomputation optimization: Both spatial weights and range weights are precomputed, avoiding expensive exp() calls in the inner loop.
- Boundary handling: Uses std::clamp for edge replication, a standard approach.
- Numerical stability: The center pixel always contributes with maximum weight (w = 1.0), ensuring weight_acc is never zero and avoiding division-by-zero issues.
- Alpha channel preservation: Correctly preserves the alpha value from the center pixel without blurring it.
The O(width × height × kernel_size²) complexity is typical for bilateral filtering and expected given the algorithm's nature.
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)
src/hooks/useWasmWorker.test.js (1)
57-71: Add bilateralFilter to property existence checks.The test validates that helper methods are exported, but
bilateralFilteris missing from the checks even though it's now part of the public API (added in line 52 of useWasmWorker.js).🔎 Proposed fix
expect(result.current).toHaveProperty('call'); expect(result.current).toHaveProperty('gaussianBlur'); + expect(result.current).toHaveProperty('bilateralFilter'); expect(result.current).toHaveProperty('blackThreshold'); expect(result.current).toHaveProperty('kmeans'); expect(result.current).toHaveProperty('mergeSmallRegionsInPlace'); expect(typeof result.current.call).toBe('function'); expect(typeof result.current.gaussianBlur).toBe('function'); + expect(typeof result.current.bilateralFilter).toBe('function'); expect(typeof result.current.blackThreshold).toBe('function'); expect(typeof result.current.kmeans).toBe('function'); expect(typeof result.current.mergeSmallRegionsInPlace).toBe('function');
🧹 Nitpick comments (1)
src/components/WasmImageProcessor.jsx (1)
84-91: Consider clarifying the comment.The implementation correctly applies the bilateral filter. However, the comment could be clearer about the rationale. Currently, it reads as if both filters might be used together.
🔎 Suggested comment improvement
- // NOTE: Gaussian blur destroys the sharp outlines first, preventing the Bilateral filter from detecting and preserving them - // const blurred = await gaussianBlur(fileData); + // NOTE: Using bilateral filter instead of Gaussian blur to preserve sharp edges. + // Gaussian blur would destroy outlines that we want to preserve during edge detection. const imgBilateralFiltered = await bilateralFilter({
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/WasmImageProcessor.jsxsrc/hooks/useWasmWorker.jssrc/hooks/useWasmWorker.test.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/hooks/useWasmWorker.test.js (1)
src/hooks/useWasmWorker.js (1)
useWasmWorker(4-53)
🔇 Additional comments (6)
src/hooks/useWasmWorker.test.js (1)
205-256: LGTM!The test suite comprehensively validates both default and custom parameter handling for the bilateral filter, following the established testing patterns.
src/hooks/useWasmWorker.js (2)
38-40: Verify default sigma parameters are appropriate for your use case.The implementation correctly follows the established pattern. However, confirm that the default values (
sigma_spatial = 3.0,sigma_range = 50.0) produce satisfactory results across typical input images in your workflow.For reference, bilateral filter parameters typically work as follows:
sigma_spatial: Controls spatial smoothing radius (3-5 for edge preservation)sigma_range: Controls intensity/color smoothing (20-80 typical range)Consider testing with various image types (photos, line art, different resolutions) to ensure the defaults work well across your expected inputs.
52-52: LGTM!The export correctly includes the new
bilateralFiltermethod alongside existing methods.src/components/WasmImageProcessor.jsx (3)
16-16: LGTM!Correctly updated to destructure
bilateralFilterinstead ofgaussianBlurfrom the hook.
94-98: LGTM!Correctly wired to consume the bilateral-filtered pixels for the threshold step.
149-149: LGTM!The dependency array correctly includes
bilateralFilterand reflects the actual dependencies used in the callback.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/hooks/useWasmWorker.test.js (1)
207-258: LGTM! Comprehensive test coverage for bilateralFilter.The test suite for
bilateralFilteris well-structured and follows the established patterns:
- Verifies the correct function name ('bilateral_filter') is passed to the worker
- Validates default parameters (sigma_spatial: 3.0, sigma_range: 50.0) match the production implementation
- Confirms custom parameters are respected
- Consistent structure with the
gaussianBlurtestsOptional: Consider testing promise resolution and return values
While the current tests verify that
postMessageis called with the correct payload, they don't verify the promise resolution or return value extraction. This is consistent with existing tests but could be enhanced:it('should return processed pixels from worker response', async () => { const { result } = renderHook(() => useWasmWorker()); const pixels = new Uint8ClampedArray([255, 0, 0, 255]); const processedPixels = new Uint8ClampedArray([128, 128, 128, 255]); let filterPromise; act(() => { filterPromise = result.current.bilateralFilter({ pixels, width: 1, height: 1 }); }); // Simulate worker response act(() => { mockWorkerInstance.instance.onmessage({ data: { id: 0, output: { pixels: processedPixels } }, }); }); const resultPixels = await filterPromise; expect(resultPixels).toBe(processedPixels); });This would provide more complete coverage, though it would ideally be applied consistently across all helper method tests.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/hooks/useWasmWorker.test.jssrc/wasm/modules/image/src/bilateral_filter.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/wasm/modules/image/src/bilateral_filter.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
src/hooks/useWasmWorker.test.js (3)
src/workers/wasmWorker.js (1)
result(42-42)src/hooks/useWasmWorker.js (1)
useWasmWorker(4-53)src/components/WasmImageProcessor.jsx (1)
useWasmWorker(16-16)
🔇 Additional comments (1)
src/hooks/useWasmWorker.test.js (1)
62-62: LGTM! bilateralFilter property checks added.The additions correctly verify that
bilateralFilteris included in the hook's return value and is a function, consistent with the pattern used for other helper methods.Also applies to: 69-69
|
Original: 
Gaussian Blur filter Bilateral Filter |
Ryan-Millard
left a comment
Ryan-Millard
left a comment
There was a problem hiding this comment.
Thank you so much for this! I can't describe how happy I am about this with words. This fixes so many of the problems the Gaussian blur brought to the table - it initially seemed like the better choice since bilateral filters are slower, but this is evidence that there is no point in using a Gaussian blur for the most part.
Would it be possible for you to write some documentation for this? We use Docusaurus (which uses a blend between Markdown and React). Currently, a lot of the code is undocumented and I'm trying to fix that. If you can't or don't feel like it, that's okay.
- Our C++ image module's documentation
- Our guide on writing documentation
- The official documentation guide from Docusaurus
Thanks, once again! There is a huge different between the old code and the new stuff, and it's truly wonderful.
Click here to see more outputs of images I used
Important
I'm only requesting changes because this needs documentation. If you don't want to write it, please just let me know and I'll handle it.
|
Sure, give me today to do this. I started my day recently, I have to do some stuff first, and then I'll work on this and add the documentation. I'm happy to collaborate on this feature |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/wasm/modules/image/include/bilateral_filter.h (1)
1-20: Document that the filter modifies the image in-place.The function signature and parameter documentation are clear, but the header should explicitly state that the filter modifies the
imagebuffer in-place. This is important information for callers to understand memory ownership and mutation behavior.🔎 Suggested documentation enhancement
// 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) void bilateral_filter(uint8_t *image, size_t width, size_t height, double sigma_spatial, double sigma_range);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/docs/reference/wasm/modules/image/bilateral_filter/_category_.jsondocs/docs/reference/wasm/modules/image/bilateral_filter/api.mddocs/docs/reference/wasm/modules/image/bilateral_filter/explained.mddocs/docs/reference/wasm/modules/image/bilateral_filter/implementation.mddocs/docs/reference/wasm/modules/image/bilateral_filter/overview.mddocs/docs/reference/wasm/modules/image/overview.mdsrc/wasm/modules/image/include/bilateral_filter.hsrc/wasm/modules/image/src/bilateral_filter.cpp
✅ Files skipped from review due to trivial changes (3)
- docs/docs/reference/wasm/modules/image/bilateral_filter/category.json
- docs/docs/reference/wasm/modules/image/bilateral_filter/api.md
- docs/docs/reference/wasm/modules/image/bilateral_filter/explained.md
🧰 Additional context used
🧬 Code graph analysis (1)
src/wasm/modules/image/include/bilateral_filter.h (1)
src/wasm/modules/image/src/bilateral_filter.cpp (3)
bilateral_filter(27-106)bilateral_filter(27-28)bilateral_filter(111-112)
🪛 LanguageTool
docs/docs/reference/wasm/modules/image/bilateral_filter/implementation.md
[style] ~13-~13: Consider placing the discourse marker ‘first’ at the beginning of the sentence for more clarity.
Context: ...ation. ## 1. Parameters & Window Size The filter first calculates the kernel size based on the...
(SENT_START_FIRST_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
docs/docs/reference/wasm/modules/image/bilateral_filter/overview.md
12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (8)
docs/docs/reference/wasm/modules/image/overview.md (1)
28-28: LGTM! Documentation accurately reflects the new bilateral filter subsystem.The additions to the file structure and subsystem descriptions are clear and consistent with the existing documentation style.
Also applies to: 42-42
docs/docs/reference/wasm/modules/image/bilateral_filter/overview.md (1)
1-31: LGTM! Well-structured overview documentation.The bilateral filter overview is clear, well-organized, and provides helpful navigation to related documentation. The algorithm description and key steps are accurate.
docs/docs/reference/wasm/modules/image/bilateral_filter/implementation.md (1)
1-73: LGTM! Comprehensive implementation documentation.The documentation thoroughly explains the bilateral filter implementation with clear code snippets and rationale for optimization strategies. The mapping from theory to implementation is well-articulated.
src/wasm/modules/image/src/bilateral_filter.cpp (5)
1-16: LGTM! Includes and constants are appropriate.The include structure is clean, and the constants are well-documented. The
SIGMA_RADIUS_FACTORof 3.0 is a standard choice (covering ~99.7% of the Gaussian distribution), andMAX_RGB_DIST_SQis correctly calculated.
35-53: LGTM! Weight precomputation is an effective optimization.The precomputed spatial Gaussian kernel and range LUT are well-implemented optimizations that avoid expensive
std::expcalls in the inner loop. The approach is correct and efficient.
58-96: LGTM! Main processing loop correctly implements the bilateral filter algorithm.The nested loop structure properly:
- Handles image edges using
std::clamp(border replication)- Combines spatial and range weights according to the bilateral filter formula
- Accumulates weighted RGB values
- Preserves the alpha channel without filtering (reasonable choice for RGBA images)
The implementation is algorithmically correct and well-optimized with precomputed weights.
105-105: LGTM! In-place update via memcpy is correct.The temporary buffer approach followed by
memcpyensures the filter reads from the original image throughout processing and updates it atomically at the end. This is the correct way to implement an in-place filter.
110-114: LGTM! WASM export wrapper is correctly implemented.The separation between the namespaced implementation and the global EXPORTED wrapper is good design. This keeps the bilateral namespace clean while providing the required WASM API surface.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/wasm/modules/image/include/bilateral_filter.h (1)
9-17: Consider documenting parameter constraints.The documentation is clear and informative. As an optional enhancement, you could mention valid parameter ranges (e.g., "sigma_spatial and sigma_range should be positive values") to help API consumers understand expected inputs without diving into the implementation.
📝 Optional documentation enhancement
// 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) +// - sigma_spatial: Gaussian standard deviation for spatial proximity (spatial decay, must be > 0) +// - sigma_range: Gaussian standard deviation for intensity difference (radiometric decay, must be > 0) void bilateral_filter(uint8_t *image, size_t width, size_t height, double sigma_spatial, double sigma_range);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/wasm/modules/image/include/bilateral_filter.hsrc/wasm/modules/image/src/bilateral_filter.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/wasm/modules/image/src/bilateral_filter.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-31T17:46:54.476Z
Learnt from: fransafu
Repo: Ryan-Millard/Img2Num PR: 176
File: src/wasm/modules/image/src/bilateral_filter.cpp:98-101
Timestamp: 2025-12-31T17:46:54.476Z
Learning: In bilateral filter implementations, the center pixel always contributes a weight of exactly 1.0 to the weight accumulator (spatial weight = exp(0) = 1.0 and range weight = exp(0) = 1.0), which inherently prevents division by zero during normalization without requiring explicit guards.
Applied to files:
src/wasm/modules/image/include/bilateral_filter.h
🧬 Code graph analysis (1)
src/wasm/modules/image/include/bilateral_filter.h (1)
src/wasm/modules/image/src/bilateral_filter.cpp (3)
bilateral_filter(28-109)bilateral_filter(28-29)bilateral_filter(114-115)
🔇 Additional comments (1)
src/wasm/modules/image/include/bilateral_filter.h (1)
1-21: LGTM! Clean and well-structured header.The header follows best practices with proper include guards, minimal includes, clear namespace organization, and an appropriate function signature for WASM integration. The raw pointer interface is correct for this use case.
|
@fransafu take a quick looks at my PR #177
Feel free to grab anything from there that might be of interest. Also see issues with the merging of small regions. |
Ryan-Millard
left a comment
Ryan-Millard
left a comment
There was a problem hiding this comment.
This is great. Thank you for the documentation!
Please will you add the below in conjunction with the individual comments I left on these files:
- Add a section for beginners that explains the prerequisite theory they need to get started with the bilateral filter (e.g., understanding Gaussian blurs).
- Add direct references to parts of the formula the bilateral filter uses to the code explanations in the docs to make it easier understand how they relate.
Both of those will make onboarding easier for future contributors.
Thank you!
- Simple info admonition that explains the link to Gaussian functions
6dad78d
into
Ryan-Millard:feat/kmean-preprocessing/bilateral-filter
…re K-Means (#191) * bilateral filter with CIELAB distance * fix nomenclature * fix bug * bug fix * variable name fix * incorporate git actions advice * cleanup * Fix indexing problem in kmeans_clustering_spatial. Verify that it works but don't call explicitly in useWasmWorker.json - commented out for future * kernel range fix * feat(WASM): add bilateral filter * test: add call method test, and custom parameters test * refactor: rename image parameter to pixels to match lib conventions * test: add bilateralFilter as part of helper methods return test * refactor: remove unused variable MAX_PIXEL_VAL * feat: add headers to be used by WASM (best practice) * docs(WASM): add bilateral_filter documentation (overview, explained, implementation, and api) * feat(bilateral_filter): add upper bound validation for sigma_spatial * docs: improve bilateral headers documentation * docs(bilateral filter): explain use of Gaussian kernels inside formula - Simple info admonition that explains the link to Gaussian functions * docs(bilateral filter): better styling * docs(bilateral filter): better styling * docs(bilateral filter): correct sidebar_position * docs(bilateral filter): mobile accessibility * docs(bilateral filter): explicit description in module overview * === end of commits from #176 === * fix(duplicate symbol: bilateral_filter): temp rename cielab -> bilateral_filter_cielab * docs(bilateral filter): fix api.md styling * feat(bilateral filter): combine CIELAB & RGB implementations into single function * refactor(cielab.h): split into .h & .cpp, include guard * fix(bilateral filter): guard against unknown color_space param Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(cielab.{h,cpp} includes): properly structured * debugging memory and run time... * Convert full RGB image to CIELAB then look up during convolution step * put in missing if * updates * update bilateral filter docs to include cielab color space * for cielab apply bilateral filter weights on LAB components then convert to RGB * fix bug * docs(bilateral filter): update for better clarity * refactor(cielab): use constexpr functions and improve naming * refactor(bilateral_filter): improve readability and use brace initialization - Replace acc0/acc1/acc2 with descriptive weight_acc_channel_0/1/2 - Use brace-initialization for ints and doubles - Minor spacing and formatting cleanup * condense 2 switch cases * refactor(bilateral filter): inline gaussian function * fix(cpp: cielab): gaussian function inlined now Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * RGB-CIELAB conversion write up * docs(bilateral filter): add color space docs * style(formatting): fix formatting issues on all files * style(docs files): fix display styles * feat(cielab): improve RGB to Lab documentation and matrix precision - Add Mermaid diagrams for RGB → Lab and Lab → RGB - Update RGB↔XYZ and XYZ↔RGB matrices to high-precision Khronos/W3C values - Add explanatory comments for matrices in cielab.cpp * style(docs: cielab): fix comment formatting - line 47 * docs(cpp: bilateral filter): document the difference between sigma_range in the color spaces and recommend a value * style(docs: bilateral filter): fix formatting * fix(cpp: cielab.h): add clamp values to protect against bad data --------- Co-authored-by: Krasner <aakrasner@gmail.com> Co-authored-by: Francisco Sanchez <fransafu@gmail.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Hi @fransafu! Your implementation here was very clean, and the documentation you added made the feature easy to review and use. I really appreciate the care you put into both the code and the docs. I’d like to offer you write access for feature work so you can push branches and continue contributing code and documentation more easily. The If that works for you, let me know and I’ll add you. |
3 participants
Please choose one of the following:
If none of these fit, you may use this default to describe your change manually.
If this is the right template, go ahead and complete it below 👇
📌 Description
Please describe the changes made in this PR and why they are necessary.
Helped add the bilateral filter feature based on this post:
✅ Type of Change
Place an "x" in the brackets below:
🧪 How Has This Been Tested?
I have to install emsdk locally, then run
build-wasmfrom the package scripts. Finally, run the dev environment (React frontend) and upload an image to check if it works🧩 Checklist
Place an "x" in the brackets below:
📸 Screenshots / Demo (if applicable)
Paste images, GIFs, or demo links here.
💬 Additional Context
Anything else relevant to the PR.
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.