fix(ArtQRCode): scale and dispose background image#88
Conversation
Resize was creating an empty scaledImage and returning it without drawing the source image, so the background image was invisible. Use image.Resize() to scale and dispose the temporary bitmap, and dispose the resized result in RenderGraphicCore. Co-Authored-By: Afonso Dutra Nogueira Filho <afonsoft@gmail.com>
Co-Authored-By: Afonso Dutra Nogueira Filho <afonsoft@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| return null; | ||
|
|
||
| float scale = Math.Min((float)newSize / image.Width, (float)newSize / image.Height); | ||
| var scaledWidth = (int)(image.Width * scale); |
There was a problem hiding this comment.
🟡 Resized image dimensions can round down to zero for extreme aspect ratios, risking an exception and bitmap leak
The computed scaled dimensions can truncate to zero (scaledWidth or scaledHeight at QRCoder.Core/Renderers/ArtQRCode.cs:340-341) when the source image has an extreme aspect ratio, so the resize call may throw or silently fail and leak the already-allocated output bitmap.
Impact: A background image with a very extreme aspect ratio could cause an unhandled exception or a native bitmap resource leak.
Truncation mechanism and leak path
When image.Width is much larger than image.Height (or vice versa) relative to newSize, the scale factor becomes very small. For example, with image.Width=1000, image.Height=1, newSize=1: scale = 0.001, scaledHeight = (int)(1 * 0.001) = 0. Then image.Resize(new SKSizeI(1, 0), ...) at QRCoder.Core/Renderers/ArtQRCode.cs:349 will either return null (handled) or throw an ArgumentException for invalid dimensions. If it throws, the bm bitmap allocated at line 345 is not protected by try/finally, so it leaks.
The guard at QRCoder.Core/Renderers/ArtQRCode.cs:336 checks newSize <= 0 but does not check the computed scaledWidth and scaledHeight after truncation.
(Refers to lines 340-341)
Was this helpful? React with 👍 or 👎 to provide feedback.
| { | ||
| using (var brush = new SKPaint { Color = SKColors.Transparent, }) | ||
| graphics.Clear(SKColors.Transparent); | ||
| using (var scaledImage = image.Resize(new SKSizeI(scaledWidth, scaledHeight), new SKSamplingOptions(SKFilterMode.Linear))) |
There was a problem hiding this comment.
📝 Info: Old Resize method never actually resized the image — this fix is a significant behavioral change
The old Resize method at QRCoder.Core/Renderers/ArtQRCode.cs:334 created var scaledImage = new SKBitmap(scaledWidth, scaledHeight) — an empty, uninitialized bitmap — and drew that empty bitmap onto the output canvas. The source image was never actually scaled or copied. This meant background images were silently ignored in all prior versions.
The new code correctly calls image.Resize(new SKSizeI(scaledWidth, scaledHeight), ...) which produces a properly scaled copy. This is confirmed by the test: the old hash for can_create_standard_qrcode_graphic_with_background was b9ecef2ee7e769d17f5e00914c7452bb — identical to the no-background test can_create_standard_qrcode_graphic — proving the background was never rendered before.
This is a real behavioral change for any consumer relying on ArtQRCode with a background image. Previously they got no background; now they'll get one.
Was this helpful? React with 👍 or 👎 to provide feedback.
| { | ||
| if (resizedImage != null) | ||
| graphics.DrawBitmap(resizedImage, 0, 0); | ||
| graphics.Flush(); |
There was a problem hiding this comment.
📝 Info: Redundant graphics.Flush() calls inside background image rendering blocks
The graphics.Flush() calls at lines 205 and 215 are called immediately after drawing the background image, but before the module dots are rendered. There is already a graphics.Flush() at line 249 after all rendering (modules + finder patterns) is complete. The intermediate flushes are unnecessary — SKCanvas.Flush() forces pending draw operations to be submitted, but SkiaSharp doesn't require intermediate flushes between draw calls on the same canvas. These add minor overhead without benefit.
Was this helpful? React with 👍 or 👎 to provide feedback.
| using (var resizedImage = Resize(backgroundImage, size)) | ||
| { | ||
| if (resizedImage != null) | ||
| graphics.DrawBitmap(resizedImage, 0, 0); | ||
| graphics.Flush(); | ||
| } | ||
| } | ||
| else if (backgroundImageStyle == BackgroundImageStyle.DataAreaOnly) | ||
| { | ||
| var bgOffset = 4 - offset; | ||
| graphics.DrawBitmap(Resize(backgroundImage, size - (2 * bgOffset * pixelsPerModule)), 0 + (bgOffset * pixelsPerModule), (bgOffset * pixelsPerModule)); | ||
| using (var resizedImage = Resize(backgroundImage, size - (2 * bgOffset * pixelsPerModule))) | ||
| { | ||
| if (resizedImage != null) | ||
| graphics.DrawBitmap(resizedImage, 0 + (bgOffset * pixelsPerModule), (bgOffset * pixelsPerModule)); | ||
| graphics.Flush(); | ||
| } |
There was a problem hiding this comment.
📝 Info: Old code also leaked the scaledImage bitmap — fixed by this PR
In the old Resize method, var scaledImage = new SKBitmap(scaledWidth, scaledHeight) was allocated but never disposed. The new code replaces this with image.Resize(...) wrapped in a using block at line 349, properly disposing the intermediate bitmap. Similarly, the callers in RenderGraphicCore (lines 201, 211) now wrap the Resize() return value in using blocks, fixing a resource leak where the resized bitmap was previously passed directly to DrawBitmap and never disposed.
Was this helpful? React with 👍 or 👎 to provide feedback.
…t ratios Ensure scaled dimensions are at least 1 pixel before calling image.Resize(). Create the output bitmap only after a successful resize to prevent leaking it if the resize operation fails. Co-Authored-By: Afonso Dutra Nogueira Filho <afonsoft@gmail.com>
|
04829e5
into
feature/devin-20260712-sonar-quality
1 participant
All Submissions:
Changes to Core Features:
Summary
Fixes the pre-existing
ArtQRCodebackground image bug reported by Devin Review:Resizenow actually scales the source image usingSKBitmap.Resizeinstead of returning an emptyscaledImage.Resizedisposes the temporaryscaledImageand theSKCanvas/SKPaintresources.RenderGraphicCorenow disposes theSKBitmapreturned byResizeafter drawing it.Resizeis nowstaticand guards against invalid dimensions.ArtQRCodeRendererTests.can_create_standard_qrcode_graphic_with_backgroundexpected hash updated to reflect the rendered background image.Build:
dotnet build QRCoder.Core.sln --configuration Releasepasses.dotnet test QRCoder.Core.Tests/ -f net10.0 --configuration Releasepasses (502 tests).dotnet test QRCoder.Core.Tests/ -f net8.0 --configuration Releasepasses (502 tests).net48tests are skipped on Linux due to missingmono.Link to Devin session: https://app.devin.ai/sessions/20c92a1bad724f63b8be89a9e195e664
Requested by: @afonsoft