Repository navigation
ios: deflate the text entries in the shared scan zip - #87
AidenJohnston wants to merge 4 commits into
Conversation
ZipWriter stored every entry (method 0). The scan bundle's motion CSVs, manifest and scene are text, about a sixth of a 4.1 bundle, and deflate well. Each entry except JPEGs now goes through Apple's Compression framework (COMPRESSION_ZLIB writes raw RFC 1951 DEFLATE, which is ZIP method 8) and is written deflated only when that is strictly smaller; anything else stays stored. The CRC and uncompressed size still describe the raw data. Without the Compression framework every entry is stored. Tests parse the central directory and local headers: a CSV entry is method 8, under half its size and inflates back to the original; JPEGs, random bytes, one-byte and empty entries stay stored. The unzip -t test now covers a deflated entry. Part of #68. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughZipWriter now attempts raw DEFLATE for entries that are not already compressed by filename suffix. It uses compressed output only when smaller than the source and records the selected method and sizes in ZIP headers. Tests cover compressed entries, stored entries, and ZIP metadata. ChangesZIP entry compression
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change reduces compressible ZIP entries while preserving stored entries when compression is unsuitable. No concrete merge-blocking issue remains; full-scan size, save time, and Files compatibility still warrant device validation. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change is confined to scan archive encoding. Completed-file publication and the separate JSON upload path limit exposure. Compatibility with external archive readers remains unverified; no introduced security vulnerability was established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
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 |
AidenJohnston
left a comment
There was a problem hiding this comment.
Adversarial review of 5150321 (U18, Part of #68), against base t3/ios-mvf.
Must fix
None found.
Should fix
None found. The items under Notes are optional tightening.
Notes
-
No compiler has seen this Swift yet (
gh pr checks 87: iOS buildskipping, because the PR is a draft; PR Size passed; CodeRabbit skipped the draft). My mental compile passed:compression_encode_buffer(UnsafeMutablePointer<UInt8>, Int, UnsafePointer<UInt8>, Int, UnsafeMutableRawPointer?, compression_algorithm) -> Intmatches the call atZipWriter.swift:213.- The typed
withUnsafeMutableBytes/withUnsafeBytesclosures andbindMemory(to:)return types line up. There is no exclusivity conflict (outis mutated,datais only read), and neither#ifbranch has unreachable code. - The new members are
private, and the public signatures didn't change.NSData.decompressed(using: .zlib)is available because the package floor is macOS 26.
Before merging, get a green
swift test --package-path ios/HouseScanKit -Xswiftc -warnings-as-errorsrun. Marking the PR ready triggers it, orworkflow_dispatchon the branch does. -
The fallback and the test disagree off Apple platforms. Scenario: HouseScanKit is ever built where
Compressionis missing (for example the Linux corelibs). The writer falls back to stored as designed (ZipWriter.swift:202). ButtextEntryIsDeflatedassertsmethod == 8unconditionally (ZipWriterTests.swift:108), andNSData.decompressed(using:)doesn't exist in swift-corelibs-foundation, so that test file wouldn't compile there. TodayPackage.swiftdeclares iOS and macOS only and CI runs onmacos-26, so this can't happen yet. If Linux ever matters, wrap the test in the same#if canImport(Compression). -
"Byte-identical" now holds per OS build only (
ZipWriter.swift:67-68). The deflate output comes from Apple's encoder, and its bytes can change between iOS or macOS releases.deterministicWithoutDateandstreamedArchiveMatchesInMemorystill hold, because both sides run on the same machine. Nothing hashesscan.zipitself (the manifest hashes the files inside it), so nothing breaks. The doc comment could say "on the same OS". -
Correctness against #68 and the 4.1 evidence. The fix covers the lossless part of #68, deflating the text entries:
- Run 1 had 19.7 MB of motion CSVs, and #68 measured gzip taking them to 8.0 MB. Photos stay stored, so the roughly 12 MB estimate is plausible.
- "Part of #68" is correct. JPEG quality and pixel size are untouched, as the brief requires, and wait on U16's measurement.
.heicand.pngare also skipped. The packet writes neither, so nothing changes.
-
Header fields checked against APPNOTE.
- Method 8 goes in both the local header and the central directory.
- The compressed size is the deflated body, and the uncompressed size and CRC-32 describe the raw data.
- Version needed stays 20, and bit 3 (data descriptor) is not set, which is right because the sizes are known before the header is written.
writtenadvances bybody.count, so the offsets and the end record stay consistent.- The capacity is one byte under the input, and the 0-on-overflow return gives a strictly smaller result or stored. The
count > 1guard avoidsData(count: -1)on empty entries.
-
Flows around it.
ZipWriter.writehas one caller:KeyframeStore.zipPacket(KeyframeStore.swift:217), called fromScanEngine.writePacket(ScanEngine+Packet.swift:287). It runs insideTask.detachedinsaveBundle(ScanEngine.swift:1497-1519), and the upload never waits on it.- The only visible effect is that "Share scan" appears a little later, while about 20 MB is compressed. At zlib speeds on an A15 that should be well under a second. Only a device can confirm it.
- Every reader of the bundle I found handles method 8:
/usr/bin/unzipinios/Tools/check-app-scene.sh, Pythonzipfilein thepacket/validator and inexperiments/evalsonbeta, andserver/api.py_open_bundleont3/server, which sizes withfile_size, the uncompressed size. - Nothing on any branch parses the zip by hand. No JS reader exists and no stored-only parser exists.
-
Test parser. The offsets in
parsematch APPNOTE: CDH method@10, crc@16, sizes@20/24, name length@28, offset@42; LFH method@8, crc@14, sizes@18/22; EOCD count@10, offset@16.- The 4096-byte LCG noise can't deflate below 4095 bytes (a stored block alone costs 5 extra bytes), so the "stays stored" check is sound.
- The 2000-row CSV (about 110 KB) spans several deflate blocks, and the fixture now puts it through
unzip -t.
-
Scope. Only
ZipWriter.swiftandZipWriterTests.swiftchanged. There are noproject.ymlorproject.pbxprojedits, no signing changes and no new dependency.Compressionis a system framework.- The branch is behind
t3/ios-mvfby 5 commits, but none of them touch these files or their callers, and a test merge is clean. - No other open PR touches
ZipWriterorKeyframeStore.
- The branch is behind
-
Public-repo check. The diff, the PR body and the commit message contain no names, meter numbers, addresses, emails,
private/content or scan data. The fixtures are synthetic. -
PR body. It has Part of #68, the base, which checks I made and which need CI or a device, the files touched, the dependency on no other PR, and the attribution line. No sample packet was on hand, so there are no measured before/after sizes. Running one scan on a phone after merging would confirm the saving.
Verdict: ready-with-notes
🤖 Generated with Claude Code
|
Verdict: ready-with-notes
🤖 Generated with Claude Code |
AidenJohnston
left a comment
There was a problem hiding this comment.
Adversarial review of #87 (U18, t3/41-zip-deflate at 5150321, base t3/ios-mvf, part of #68)
I read the full new ZipWriter.swift and ZipWriterTests.swift, the only caller (KeyframeStore.zipPacket, called from ScanEngine+Packet.writePacket inside the detached saveBundle task), the server's bundle reader (server/api.py _open_bundle on beta/t3/server), #68 and the public run 1/2/3 analyses.
Must fix
None.
Should fix
- The PR body's Verification section is out of date. It says "the package has not been compiled or tested yet". The manually started iOS workflow run 36296274160 on head
5150321passed: theTest HouseScanKitstep compiledZipWriterTests.swiftunder-warnings-as-errors, andtextEntryIsDeflated,incompressibleEntriesAreStored,unzipAcceptsArchive(/usr/bin/unzip -t, which inflates and checks each CRC),streamedArchiveMatchesInMemoryanddeterministicWithoutDateall passed, with 383 tests in total. The build, the committed-project drift check and the Simulator UI flow also passed. Move those items from "only CI or a device can verify" to "verified", and cite the run. Otherwise a merger can't tell the Swift has been built at all.
Notes
ios/HouseScanKit/Sources/HouseScanKit/Scene/ZipWriter.swift:76-77: thewritedoc comment still says memory holds "one file and the central directory".deflated(:207) now allocates an output buffer ofdata.count - 1bytes next to the loaded file, so a deflatable entry briefly needs about twice its size. Run 1's largest CSV was about 10 MB, and a LiDARlidar/mesh.plycould be larger. The PR body's Risk section says this, but the doc comment doesn't. A one-line doc fix is enough.- Correctness checks that held up.
- With
COMPRESSION_ZLIB, the encoder writes raw RFC 1951 data with no header (method 8). Theunzip -ttest proves this with an independent inflater, not only with Apple's round trip. - The CRC and the uncompressed size are computed from the raw data (
:123-124). The compressed size and the method appear in both headers (:134/:138and:151/:155). Version needed 2.0 is correct for deflate. - With a capacity of
count - 1, the encoder returns 0 on overflow, so the noise, one-byte and empty entries fall back to stored. The noise test would have caught a truncated result. - The typed
withUnsafe*Bytesclosures avoid the deprecated overloads. The new helpers are private, and the public signatures are unchanged. KeyframeStore.zipPacketatios/HouseScan/Runtime/KeyframeStore.swift:230is the only call site, and nothing else inios/,web/orexperiments/parses the zip at the byte level. The packet manifest hashes the files on disk, not zip bodies, so it is unaffected.
- With
- Server compatibility.
_open_bundleinserver/api.py(onbeta) useszipfile, which reads method 8. It already catcheszlib.error, and its size cap sumsinfo.file_size, which is still the uncompressed size, so the limit means the same thing. No server change is needed. - Scope. The PR only reaches the lossless part of #68 (about 12 MB of the 119.5 MB on run 1). Photos are still about 83% of the bundle, so this does not make run 1's 119.5 MB share or its slow Drive upload noticeably better. The PR says so and correctly leaves the JPEG quality or size for U16. The added
.jpeg/.heic/.pngskips go slightly beyond the brief (.jpgonly), but they're harmless and aren't a product decision. Noproject.yml/project.pbxprojedits and no unrelated changes. - Branch state. The branch is 41 commits behind
t3/ios-mvf. None of those commits touchZipWriter.swift, its tests,KeyframeStore.swiftorScanEngine+Packet.swift, andgit merge-treeshows a clean merge. - CI. The iOS check on the PR shows "skipping", because draft PRs don't trigger the iOS job. See Should fix 1 for the manually started run on the head. PR size check passed. CodeRabbit skipped the draft.
- Device-only. The real zip size and the extra save time before "Share scan" becomes available are both unmeasured.
shareableScanis set only afterwritePacketreturns, so any deflate time delays the button. With about 20 MB of text at the default zlib level, that is probably well under a second, but it should be timed on the next device test. - Public-repo rules. The diff, the PR body and the commit message contain no names, meter numbers, addresses, private/ content or scan data. The test fixtures are synthetic. The PR body has Part of #68, the base, dependencies, the files touched and the attribution line.
Verdict: ready-with-notes
🤖 Generated with Claude Code
|
Note Review skipped Codemetrics is installed on this repository, but it hasn't been added to your team yet. To enable AI code reviews for this repo, go to your dashboard and add house-scanning-master to your team's repositories. |
|
| let capacity = data.count - 1 | ||
| var out = Data(count: capacity) |
There was a problem hiding this comment.
The writer loads each entry into memory, then allocates another buffer almost as large before checking whether compression helps. A long LiDAR scan can include binary depth frames and a mesh, so saving its ZIP can use substantially more memory and may fail under memory pressure. A bounded or streaming compression path would preserve the writer’s one-file-at-a-time memory goal.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ios/HouseScanKit/Sources/HouseScanKit/Scene/ZipWriter.swift
Line: 206-207
Comment:
**Large compression buffer**
The writer loads each entry into memory, then allocates another buffer almost as large before checking whether compression helps. A long LiDAR scan can include binary depth frames and a mesh, so saving its ZIP can use substantially more memory and may fail under memory pressure. A bounded or streaming compression path would preserve the writer’s one-file-at-a-time memory goal.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ized one Greptile noted the deflate attempt allocated a second buffer almost as large as each entry. The encoder now streams through 64 KiB, so the only added memory is the compressed output, and stops once that output stops being a saving. The write() and byte-identical doc comments say what they now mean, and the deflate tests are behind the same canImport(Compression) as the code. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Updated for review. Head is now
CI: the iOS build passed on The real zip size and the save time still need a device scan. 🤖 Generated with Claude Code |
|
@codex review Please review current head |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
ZipWriter.write's doc comment conflicted: the base now writes to a temporary file and moves it over the old archive, and this branch says memory also holds the compressed copy. The comment keeps both. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@greptileai review Please review the current head |
Part of #68
Base branch:
t3/ios-mvf. This PR depends on no other PR.What changed
The zip that "Share scan" offers stored every file uncompressed. In the 4.1 field test run 1, that zip held about 20 MB of text: the 100 Hz motion CSVs under
streams/, plusmanifest.jsonandscene.json. #68 measured gzip taking the CSVs from 19.7 MB to 8.0 MB.ZipWriternow tries DEFLATE on every entry except JPEGs. It uses Apple's Compression framework: acompression_streamwithCOMPRESSION_ZLIBwrites raw RFC 1951 DEFLATE, which is exactly what ZIP method 8 stores, so the change adds no dependency..jpg,.jpeg,.heicor.pngskip the attempt, because those formats are already compressed. The packet only writes.jpgphotos today.#if canImport(Compression). Without the framework, every entry is stored as before.KeyframeStore.zipPacketis the only caller, and its call is unchanged. It builds the zip in the detached save task, and the upload sendsscene.jsonalone, so the upload screen doesn't wait on the extra compression.Why
The motion streams are text and deflate to well under half their size. The photos make up about 83% of the bundle, but this PR leaves them alone. A lower JPEG quality or a smaller image for the shared copy has to wait until someone measures what reconstruction needs, which is why this PR only covers part of #68. The image size must also keep matching the intrinsics. A HEIC switch would break the
photos/<id>.jpgnames the packet uses.The saving that #68's numbers suggest is roughly 12 MB on run 1, from 119.5 MB to about 108 MB. No sample packet is on hand to measure the new writer's output. The phone's encoder runs at a fixed zlib level, close to gzip's default.
Verification
t3/ios-mvfe62be65) passed: HouseScanKit tests with-warnings-as-errors, the app build, the project drift check and the Simulator UI tests. Run 36313813163. The first head, 5150321, also passed (run 36296274160).ZipWriterTests(9 tests) passed on the Mac mini with-warnings-as-errorsbefore the push:textEntryIsDeflated: a synthetic CSV is written with method 8, comes out under half its size, has the raw data's CRC and size, and inflates back to the original withNSData.decompressed(using: .zlib).largeEntriesCrossOutputChunks: an entry whose deflated form spans several 64 KiB output chunks round-trips, and 400 KB of noise is stored.incompressibleEntriesAreStored: JPEG names (including an upper-case.JPG), random bytes, a one-byte entry and an empty entry stay method 0 with their bytes unchanged.unzipAcceptsArchive(/usr/bin/unzip -t, which checks each CRC after inflating) and the streamed-vs-in-memory and determinism tests include a deflated CSV entry.KeyframeStore.zipPacket(ZipWriter.write) is the only call site outside the tests. No server file changed. Python'szipfilereads method 8, and the server's bundle check only looks at entry names.Only a device can verify these: the real zip size and the time added to the save task on a full scan, and opening a new bundle with the Files app or the share targets.
Files touched
ios/HouseScanKit/Sources/HouseScanKit/Scene/ZipWriter.swiftios/HouseScanKit/Tests/HouseScanKitTests/ZipWriterTests.swiftRisk
Compressing costs CPU time in the background save. Besides the loaded entry, memory holds only the compressed output, which is never allowed to reach the entry's size, and a 64 KiB buffer. Photos skip the encoder, so they add neither cost. Deflated bytes come from Apple's encoder, so an archive is byte-identical for the same inputs only on the same OS build.
Checklist
🤖 Generated with Claude Code
Summary by CodeRabbit