Optimize compression/decompression and add tests #611
Conversation
📝 WalkthroughWalkthroughReworks QRCodeData to stream-based compression/decompression and bit-packing/unpacking instead of List assembly; constructor adds header/length validation and revised version extraction. Adds parameterized round-trip tests for Uncompressed, Deflate, and GZip. Bumps C# LangVersion to 12. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test/Caller
participant QRCtor as QRCodeData(.ctor)
participant MS as MemoryStream
participant Decomp as Deflate/GZip Stream
Test->>QRCtor: new QRCodeData(rawBytes, compression)
QRCtor->>QRCtor: validate rawBytes length/header
alt compression != Uncompressed
QRCtor->>MS: wrap rawBytes
MS->>Decomp: wrap with decompression stream
Decomp->>QRCtor: read decompressed bytes
else Uncompressed
QRCtor->>QRCtor: use rawBytes directly
end
QRCtor->>QRCtor: parse header, derive Version/side length (Micro vs Standard)
QRCtor->>QRCtor: unpack data bytes (from index 5) into ModuleMatrix
QRCtor-->>Test: constructed QRCodeData
sequenceDiagram
autonumber
participant Test as Test/Caller
participant QRGet as QRCodeData.GetRawData
participant Out as MemoryStream
participant Comp as Deflate/GZip Stream
Test->>QRGet: GetRawData(compression)
QRGet->>Out: create MemoryStream
alt compression != Uncompressed
Out->>Comp: wrap with compression stream
QRGet->>Comp: write header + row count, then stream packed bytes (pad to byte)
Comp->>Comp: flush/close
else Uncompressed
Out->>Out: write header + row count, then write packed bytes (pad to byte)
end
QRGet->>Out: finalize
Out-->>Test: return Out.ToArray()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🧬 Code graph analysis (1)QRCoder/QRCodeData.cs (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (7)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
QRCoder/QRCodeData.cs (1)
95-105: Missing payload-length guard can throw during matrix fillIf compressed/raw data is truncated,
modules.Dequeue()will throw. Fail fast with a clear error by checking availableBits >= requiredBits.Apply this diff just before building the module matrix:
//Unpack var modules = new Queue<bool>(8 * (count - 5)); for (int j = 5; j < count; j++) { var b = bytes[j]; for (int i = 7; i >= 0; i--) { modules.Enqueue((b & (1 << i)) != 0); } } //Build module matrix + var requiredBits = sideLen * sideLen; + var availableBits = (count - 5) * 8; + if (availableBits < requiredBits) + throw new Exception("Invalid raw data file. Payload too short."); ModuleMatrix = new List<BitArray>(sideLen);
🧹 Nitpick comments (2)
QRCoder/QRCodeData.cs (2)
150-173: Stream-pack bits directly and fix padding edge caseAvoid per-bit Queue (allocations, boxing) and correct the padding when already byte-aligned.
Apply this diff to replace the queue build + process loop:
- //Build data queue - var dataQueue = new Queue<int>(); - foreach (var row in ModuleMatrix) - { - foreach (var module in row) - { - dataQueue.Enqueue((bool)module ? 1 : 0); - } - } - for (int i = 0; i < 8 - (ModuleMatrix.Count * ModuleMatrix.Count) % 8; i++) - { - dataQueue.Enqueue(0); - } - - //Process queue - while (dataQueue.Count > 0) - { - byte b = 0; - for (int i = 7; i >= 0; i--) - { - b += (byte)(dataQueue.Dequeue() << i); - } - targetStream.WriteByte(b); - } + // Stream-pack bits MSB-first, row-major + int bitCount = 0; + byte acc = 0; + int side = ModuleMatrix.Count; + for (int y = 0; y < side; y++) + { + var row = ModuleMatrix[y]; + for (int x = 0; x < side; x++) + { + acc <<= 1; + if (row[x]) acc |= 0x01; + bitCount++; + if ((bitCount & 7) == 0) + { + targetStream.WriteByte(acc); + acc = 0; + } + } + } + // Pad with zeros to next full byte if needed + int padBits = (8 - (bitCount & 7)) & 7; + if (padBits != 0) + { + acc = (byte)(acc << padBits); + targetStream.WriteByte(acc); + }
86-90: Consider validating the reserved header byte for format versioningYou write 0x00 at header byte [3] when saving; reading ignores it. Optionally enforce
bytes[3] == 0x00to catch unknown future revisions early.Apply this small check:
if (count < 5) throw new Exception("Invalid raw data file. File too short."); if (bytes[0] != 0x51 || bytes[1] != 0x52 || bytes[2] != 0x52) throw new Exception("Invalid raw data file. Filetype doesn't match \"QRR\"."); + if (bytes[3] != 0x00) + throw new Exception("Invalid raw data file. Unsupported header version.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
QRCoder/QRCodeData.cs(2 hunks)QRCoderTests/QRGeneratorTests.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
QRCoder/QRCodeData.cs (2)
QRCoder/PngByteQRCode.cs (1)
Deflate(307-311)QRCoder/Extensions/StreamExtensions.cs (1)
CopyTo(9-17)
QRCoderTests/QRGeneratorTests.cs (2)
QRCoder/QRCodeData.cs (6)
QRCodeData(12-236)QRCodeData(23-30)QRCodeData(37-44)QRCodeData(51-53)QRCodeData(60-116)GetRawData(123-183)QRCoder/QRCodeGenerator.cs (12)
QRCodeData(33-34)QRCodeData(43-44)QRCodeData(57-58)QRCodeData(67-68)QRCodeData(77-78)QRCodeData(87-94)QRCodeData(107-158)QRCodeData(168-237)QRCodeData(246-259)QRCodeData(287-460)QRCodeGenerator(23-25)ECCLevel(266-274)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
QRCoderTests/QRGeneratorTests.cs (1)
626-648: Solid round‑trip test across all compression modesCovers Uncompressed/Deflate/GZip, checks Version, size, and full matrix equality. Good placement under !NETFRAMEWORK guard.
If you want broader coverage, consider adding a Micro QR variant (GenerateMicroQrCode) to ensure raw save/load also preserves negative versions. I can draft it if useful.
QRCoder/QRCodeData.cs
Outdated
| if (bytes[0] != 0x51 || bytes[1] != 0x52 || bytes[2] != 0x52) | ||
| var count = rawData.Length; | ||
|
|
||
| if (count < 5) |
There was a problem hiding this comment.
| if (count < 5) | |
| if (rawData.Length < 5) |
To give the JIT some hints, so the bound checks in the array access below can be removed (at least on newer JIT versions), and there's no harm for older ones.
There was a problem hiding this comment.
I removed count and replaced with rawData.Length all 3 times - please verify.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
QRCoder/QRCodeData.cs (1)
105-126: Consider validating queue size after unpacking.The unpacking logic correctly processes the raw data, but doesn't verify that the queue is empty after building the ModuleMatrix. If the raw data contains extra bytes beyond the expected
sideLen * sideLenbits (plus padding), those extra bits will silently remain in the queue. While the current implementation always pads to 8-bit boundaries inGetRawData, an explicit validation would catch corrupted or malicious input.Add validation after line 125:
ModuleMatrix[y][x] = modules.Dequeue(); } } + + if (modules.Count > 0) + throw new Exception("Invalid raw data file. Extra data after module matrix."); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
QRCoder/QRCodeData.cs(2 hunks)QRCoderTests/QRGeneratorTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- QRCoderTests/QRGeneratorTests.cs
🧰 Additional context used
🧬 Code graph analysis (1)
QRCoder/QRCodeData.cs (1)
QRCoder/Extensions/StreamExtensions.cs (1)
CopyTo(9-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
QRCoder/QRCodeData.cs (6)
63-82: LGTM! Clean streaming-based decompression.The refactor properly replaces in-memory List usage with stream-based processing. Resource management is correct with proper disposal before accessing the output array.
84-87: LGTM! Validation is correct and JIT-friendly.The length check helps the JIT eliminate bounds checks on subsequent array accesses. Magic byte validation properly enforces the "QRR" signature.
89-103: LGTM! Version calculation now correctly handles Micro QR.The fix properly distinguishes Micro QR (side length < 29) from Standard QR and applies the correct inverse formula for each case. Validation ensures only valid side lengths are accepted, preventing silent corruption of version numbers during save/load round-trips.
Based on past review comments, this resolves the reported issue where Micro QR versions -2, -3, and -4 were incorrectly computed.
135-151: LGTM! Compression stream setup is correct.The use of nullable compression streams with proper initialization and the
leaveOpen: trueparameter ensures the output stream remains accessible after flushing compressed data.
152-184: LGTM! Efficient encoding with optimized modulus calculation.The encoding logic correctly writes headers, packs module bits into bytes, and handles padding. The uint-based modulus calculation at line 169 addresses the past review comment about avoiding expensive re-evaluation and enables efficient bit-shift operations.
186-194: LGTM! Proper resource cleanup ensures data integrity.The finally block correctly disposes compression streams to flush buffered data before reading the output array. This pattern ensures all compressed data is written even if exceptions occur during encoding.
gfoidl
left a comment
gfoidl
left a comment
There was a problem hiding this comment.
Forgot to send that comment before.
| byte b = 0; | ||
| for (int i = 7; i >= 0; i--) | ||
| //Add header - signature ("QRR") | ||
| targetStream.Write(new byte[] { 0x51, 0x52, 0x52, 0x00 }, 0, 4); |
There was a problem hiding this comment.
Let's use collection expression to avoid the byte[] allocation for .NET 8+ (where they were introduced).
| targetStream.Write(new byte[] { 0x51, 0x52, 0x52, 0x00 }, 0, 4); | |
| #if NET8_OR_GREATER | |
| targetStream.Write([0x51, 0x52, 0x52, 0x00]); | |
| #else | |
| targetStream.Write(new byte[] { 0x51, 0x52, 0x52, 0x00 }, 0, 4); | |
| #endif |
I thinks .NET 8+ is finde, as older targets aren't supported anymore by .NET.
Else, we could keep the signature in a static readonly and load it from there. But not so nice code...
Also .NET 10's JIT would stack allocate that array, so no heap allocation.
But the collection expression reads much nicer.
There was a problem hiding this comment.
I bumped the C# version to 12, so the code you supplied will work as far back as .NET Core 2.1 -- anything supported by Span. I'll update the conditional again to HAS_SPAN in a future PR, and probably add netstandard2_1 as a target at the same time.
| { | ||
| //Add header - signature ("QRR") | ||
| #if NETCOREAPP2_1_OR_GREATER || NETSTANDARD2_1 | ||
| targetStream.Write([0x51, 0x52, 0x52, 0x00]); |
There was a problem hiding this comment.
Just as note: for .NET less than 8 this will still allocate the array, as the collection builder, etc. isn't available in .NET, so Roslyn can't use these types and produces "fallback-code".
The change is still good, as it clearer to read. Thanks.
Summary by CodeRabbit
New Features
Refactor
Tests
Chores