Optimize bitmap operations#78
Conversation
meesoft
force-pushed
the
features/OptimizeBitmapOps
branch
from
97d643c to
33997d4
Compare
meesoft
force-pushed
the
features/OptimizeBitmapOps
branch
from
c69f683 to
3ad7470
Compare
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 34 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughRefactors bitmap processing and tests: color-tone HSI math adjusted, float-bitmap conversion and statistics inner loops optimized, Lanczos resampler weight storage and fixed-point apply rewritten, MaxFramesOperation adds single-frame fast path, CropControl contrast threshold tweaked, and tests expanded/parameterized. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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: 1
🧹 Nitpick comments (2)
PhotoLocator/BitmapOperations/ColorToneAdjustOperation.cs (1)
128-128: Optional hot-path cleanup: precompute channel deltas.On Line 128, you can reduce repeated arithmetic and make the expression easier to scan by precomputing deltas.
♻️ Optional refactor
- var a = 0.5 * (r - g + (r - b)) / Math.Sqrt((r - g) * (r - g) + (r - b) * (g - b)); + var rg = r - g; + var rb = r - b; + var gb = g - b; + var a = 0.5 * (rg + rb) / Math.Sqrt(rg * rg + rb * gb);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PhotoLocator/BitmapOperations/ColorToneAdjustOperation.cs` at line 128, In ColorToneAdjustOperation (the method that computes 'a'), precompute channel deltas like dr = r - g, db = r - b, dg = g - b and use those variables in the expression for var a to avoid repeating (r - g), (r - b), and (g - b); update the numerator and denominator to use dr, db, dg and ensure the Math.Sqrt uses dr*dr + db*dg so behavior is unchanged while the expression becomes clearer and faster.PhotoLocator/BitmapOperations/LanczosResizeOperation.cs (1)
232-237: Always write the clamped pixel value.Line 232 currently makes correctness depend on
destbeing zero-initialized by the caller. Writing0explicitly keepsLineResamplerFixedPoint.Applyself-contained if the destination buffer strategy changes later.Suggested tweak
- if (sum > 0) - { - sum >>= 16; - if (sum > 255) - sum = 255; - dest[dstOffset + i * dstSampleDistance] = (byte)sum; - } + if (sum > 0) + { + sum >>= 16; + if (sum > 255) + sum = 255; + dest[dstOffset + i * dstSampleDistance] = (byte)sum; + } + else + { + dest[dstOffset + i * dstSampleDistance] = 0; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PhotoLocator/BitmapOperations/LanczosResizeOperation.cs` around lines 232 - 237, The loop in LineResamplerFixedPoint.Apply (LanczosResizeOperation.cs) only writes dest[dstOffset + i * dstSampleDistance] when sum>0, relying on dest being pre-zeroed; change it to always compute sum >>= 16, clamp sum to the 0..255 range (if sum < 0 set to 0, if sum > 255 set to 255) and then unconditionally assign dest[dstOffset + i * dstSampleDistance] = (byte)sum so the method is self-contained regardless of caller buffer initialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@PhotoLocator/BitmapOperations/LanczosResizeOperation.cs`:
- Around line 312-318: The code currently rents srcPixels and may return it to
the ArrayPool before creating the BitmapSource, which causes a use-after-return
if Apply returns the same buffer (and also leaks on exceptions); fix by wrapping
the rent/CopyPixels/Apply/Return sequence in a try/finally so Return always
runs, detect aliasing between srcPixels and dstPixels after Apply (compare
references) and if they alias allocate a new buffer, copy the pixel data into
it, and pass that new buffer to BitmapSource.Create; ensure you only Return
buffers you rented (srcPixels) and do not Return the buffer passed into
BitmapSource.Create if you copied it for lifetime reasons (or otherwise document
ownership), referencing srcPixels, dstPixels, Apply, BitmapSource.Create,
ArrayPool<byte>.Shared.Rent/Return and CopyPixels so the changes are applied in
the correct spots.
---
Nitpick comments:
In `@PhotoLocator/BitmapOperations/ColorToneAdjustOperation.cs`:
- Line 128: In ColorToneAdjustOperation (the method that computes 'a'),
precompute channel deltas like dr = r - g, db = r - b, dg = g - b and use those
variables in the expression for var a to avoid repeating (r - g), (r - b), and
(g - b); update the numerator and denominator to use dr, db, dg and ensure the
Math.Sqrt uses dr*dr + db*dg so behavior is unchanged while the expression
becomes clearer and faster.
In `@PhotoLocator/BitmapOperations/LanczosResizeOperation.cs`:
- Around line 232-237: The loop in LineResamplerFixedPoint.Apply
(LanczosResizeOperation.cs) only writes dest[dstOffset + i * dstSampleDistance]
when sum>0, relying on dest being pre-zeroed; change it to always compute sum
>>= 16, clamp sum to the 0..255 range (if sum < 0 set to 0, if sum > 255 set to
255) and then unconditionally assign dest[dstOffset + i * dstSampleDistance] =
(byte)sum so the method is self-contained regardless of caller buffer
initialization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f80389db-2416-48ed-b874-b21a525f0a50
📒 Files selected for processing (7)
PhotoLocator/BitmapOperations/ColorToneAdjustOperation.csPhotoLocator/BitmapOperations/FloatBitmap.csPhotoLocator/BitmapOperations/LanczosResizeOperation.csPhotoLocator/BitmapOperations/MaxFramesOperation.csPhotoLocator/Controls/CropControl.xaml.csPhotoLocatorTest/BitmapOperations/FloatBitmapTest.csPhotoLocatorTest/BitmapOperations/MaxFramesOperationTest.cs
1 participant
Summary by CodeRabbit
UI Improvements
Performance Enhancements
Bug Fixes
Tests