-
Notifications
You must be signed in to change notification settings - Fork 0
Fix/manifest implementation #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Added benchmarks for: - Zstd compression/decompression with and without context reuse - Compression level comparisons - Buffer allocation strategies - Manifest marshal/unmarshal operations - Archive header operations - Lookup table key strategies
Major changes: - Reorganized code into pkg/archive and pkg/manifest packages - Created clean CLI tool in cmd/evrtools - Used idiomatic Go naming conventions (CamelCase exports) - Added comprehensive documentation and comments - Consolidated duplicate types (removed redundancy between tool/ and evrManifests/) - Added unit tests and benchmarks for new packages - Updated README with library usage examples - Updated Makefile with proper targets Benchmark results show: - Context reuse for ZSTD decompression: ~3.7x faster (6290ns vs 1688ns) - Zero allocations with context reuse - CombinedInt64Key lookup: ~2.7x faster than StructKey Legacy code in tool/ and evrManifests/ retained for backwards compatibility.
Header operations (archive): - Marshal: 136.3 ns → 1.05 ns (130x faster), 3 allocs → 0 allocs - Unmarshal: 134.5 ns → 3.8 ns (35x faster), 2 allocs → 0 allocs Manifest operations: - Marshal: 1,354,843 ns → 122,781 ns (11x faster) - Memory: 3,228,085 B → 729,093 B (4.4x reduction) - Allocs: 9 → 1 (9x reduction) - Unmarshal: 1,345,174 ns → 154,367 ns (8.7x faster) - Memory: 1,474,805 B → 737,286 B (2x reduction) - Allocs: 8 → 3 (2.7x reduction) Changes: - Replaced bytes.Buffer + binary.Write with direct LittleEndian encoding - Pre-calculate and allocate exact buffer sizes - Use inline field encoding instead of reflection-based binary package - Added size constants for all binary structures
Frame content lookup: - LinearScan: 2619 ns → PrebuiltIndex: 7 ns (374x faster) - Build frame index map before extraction loop - Eliminates O(n²) complexity in package extraction String formatting: - fmt.Sprintf: 68.5 ns/op, 1 alloc → strconv.FormatInt: 26.5 ns/op, 0 allocs - Use strconv.FormatInt/FormatUint for hex/decimal conversion - 2.6x faster with no allocations Other optimizations: - Builder.incrementSection: removed loop, use direct arithmetic - Package.Extract: cache created directories to avoid repeated MkdirAll - Added benchmarks for frame index and hex formatting strategies
Performance improvements: - Added EncodeTo/DecodeFrom methods to Header for zero-allocation encoding - Reader now uses embedded headerBuf array instead of allocating - Writer now uses embedded headerBuf array instead of allocating - Added BinarySize and EncodeTo methods to Manifest for pre-allocated encoding Benchmark results: - Header DecodeFrom: 3.8x faster than UnmarshalBinary (1.0ns vs 3.8ns) - Archive Encode: 13→11 allocations (15% reduction) - Archive Decode: 10→9 allocations (10% reduction) Remaining allocations are at practical minimum: - zstd compression/decompression buffers - Manifest slice allocations for data storage
Changes: - Remove legacy tool/ package (duplicated pkg/ functionality) - Remove legacy evrManifests/ package (unused manifest versions) - Remove legacy main.go CLI (replaced by cmd/evrtools) - Update module path from goopsie to EchoTools organization - Clean up benchmark log files - Update Makefile (remove legacy targets) - Update README with current structure and usage Final structure: cmd/evrtools/ - CLI application pkg/archive/ - ZSTD archive format handling pkg/manifest/ - EVR manifest/package operations All tests pass, build verified.
Changes: - Add -decimal-names flag to CLI (default: false, uses hex) - When -decimal-names is set, extract uses decimal format for filenames - Add WithDecimalNames() option to manifest.Extract() - Type symbols remain hex in directory names - File symbols can now be decimal (old behavior) or hex (new default) Usage: evrtools -mode extract ... -decimal-names # Use decimal filenames
…; delete obsolete 'evrtools' binary
Changes: - Hex filenames are now formatted as uint64 (e.g. 0xc8c33e483b601ab6) - Decimal filenames remain int64 (e.g. -3980269165710665034) - Type symbols are now formatted as uint64 hex
- Corrected the handling of section lengths and element sizes in the manifest. - Fixed decoding of FrameContents, Metadata, and Frames to use actual data sizes. - Updated compatibility tests to reflect the fixed implementation.
| } | ||
|
|
||
| // Grow slice if needed | ||
| for int(chunkNum) >= len(files) { |
Check failure
Code scanning / CodeQL
Incorrect conversion between integer types High
strconv.ParseInt
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
In general, to fix this class of problem you must ensure that any integer parsed as a wider type (here int64) is either parsed at the correct bit-size for the target type, or is explicitly checked to be within the valid range of the narrower type before conversion. For values that will be used as slice indices, you must also reject negative values.
For this specific case, the most direct fix without changing functionality is:
- Parse
chunkNumas a 32‑bit value, since the target type isintused as a slice index. On all supported architectures,intis at least 32 bits, soint32is a safe intermediary. - Or, if we keep parsing as
int64, add explicit bounds checks to ensure0 <= chunkNum <= math.MaxInt(and alsochunkNumfits inint), before casting toint.
The cleanest minimal change is to:
- Import
math(already used elsewhere? Not in this file, so we add it). - After successfully parsing
chunkNum, add a check that:chunkNum >= 0chunkNum <= int64(math.MaxInt)(or equivalently compare against a constant derived fromint(^uint(0)>>1)).
- Only then cast to
intby first storingint(chunkNum)in a local variable (e.g.chunkIndex) and using that variable in the loop condition and index. This also makes the code clearer. - Replace
files[chunkNum]withfiles[chunkIndex]to avoid indexing a slice with anint64.
These changes are all confined to pkg/manifest/scanner.go and require adding the math import and a small bounds-check block after parsing chunkNum. No new methods or external dependencies are needed.
-
Copy modified line R5 -
Copy modified lines R44-R48 -
Copy modified line R74 -
Copy modified line R78
| @@ -2,6 +2,7 @@ | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "math" | ||
| "os" | ||
| "path/filepath" | ||
| "strconv" | ||
| @@ -40,6 +41,11 @@ | ||
| if err != nil { | ||
| return fmt.Errorf("parse chunk number: %w", err) | ||
| } | ||
| // Ensure chunkNum is within the valid range for int and non-negative | ||
| if chunkNum < 0 || chunkNum > int64(math.MaxInt) { | ||
| return fmt.Errorf("chunk number out of range: %d", chunkNum) | ||
| } | ||
| chunkIndex := int(chunkNum) | ||
|
|
||
| typeSymbol, err := strconv.ParseInt(parts[len(parts)-2], 10, 64) | ||
| if err != nil { | ||
| @@ -65,11 +71,11 @@ | ||
| } | ||
|
|
||
| // Grow slice if needed | ||
| for int(chunkNum) >= len(files) { | ||
| for chunkIndex >= len(files) { | ||
| files = append(files, nil) | ||
| } | ||
|
|
||
| files[chunkNum] = append(files[chunkNum], file) | ||
| files[chunkIndex] = append(files[chunkIndex], file) | ||
| return nil | ||
| }) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the EVR file tools implementation to fix critical bugs in manifest parsing discovered through analysis comparing it with the NRadEngine C++ implementation. The main goal is to ensure accurate parsing of EVR package manifest files by correcting struct sizes and offset calculations.
Key changes:
- Fixed
FileMetadatastructure size from 40 to 32 bytes to match the actual format - Corrected section offset calculations to use
Lengthfields instead of hardcoded sizes - Complete rewrite from monolithic script to modular library with CLI tool
Reviewed changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/manifest/manifest.go | Core manifest parsing with corrected 32-byte FileMetadata size and Length-based offsets |
| pkg/manifest/compatibility_test.go | Comprehensive tests validating fixes against NRadEngine format |
| pkg/manifest/package.go | Package extraction with frame indexing optimization |
| pkg/manifest/builder.go | Package building functionality with compression |
| pkg/archive/header.go | ZSTD archive header handling (24 bytes) |
| cmd/evrtools/main.go | CLI tool for extract/build operations |
| README.md | Documentation for library and CLI usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ElementSize: 32, | ||
| }, | ||
| Metadata: Section{ | ||
| ElementSize: 40, |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ElementSize for Metadata section is incorrectly set to 40. According to the PR description and the fix in manifest.go, FileMetadata is 32 bytes (matching NRadEngine's ManifestSomeStructure). This should be 32, not 40.
| ElementSize: 40, | |
| ElementSize: 32, |
| ElementSize: 32, | ||
| }, | ||
| Metadata: Section{ | ||
| ElementSize: 40, |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ElementSize for Metadata section is set to 40, but FileMetadata is 32 bytes. This should be 32 to match the corrected FileMetadataSize constant.
|
|
||
| manifest.Header.Metadata.Count = uint64(len(manifest.Metadata)) | ||
| manifest.Header.Metadata.ElementCount = uint64(len(manifest.Metadata)) | ||
| manifest.Header.Metadata.Length = uint64(len(manifest.Metadata)) * 40 |
Copilot
AI
Dec 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Length calculation uses 40 bytes per element, but FileMetadata is 32 bytes. This should multiply by 32 (or use FileMetadataSize constant) to correctly calculate the section length.
5497de8 to
2b4e809
Compare
EVR Archive Format Analysis: evrFileTools vs NRadEngine
Executive Summary
This document analyzes the differences between the evrFileTools Go implementation and the NRadEngine C++ implementation for reading EVR (Echo VR) package/manifest files.
Status: FIXED - All identified bugs have been corrected. The evrFileTools implementation now correctly parses manifests matching the NRadEngine format.
File Format Structure
Archive Wrapper (ZSTD Compression)
Both implementations agree on the archive header format:
Total: 24 bytes
Note: Package files do NOT have this wrapper - they contain raw ZSTD frames directly.
Manifest Header
Total: 192 bytes
Section Descriptor
Total: 48 bytes
Critical Differences Found (NOW FIXED)
1. FileMetadata/SomeStructure Size Mismatch ✅ FIXED
Previously: evrFileTools defined
FileMetadataSize = 40with an extraAssetTypefield.Now Fixed:
FileMetadataSize = 32matching NRadEngine:2. Section Offset Calculation Bug ✅ FIXED
Previously: evrFileTools calculated section positions using hardcoded element sizes.
Now Fixed: Uses the
Lengthfield from section descriptors:3. Frame Field Order Discrepancy
evrFileTools/NRadEngine (CORRECT):
Carnation (DIFFERENT - may be incorrect):
Actual data validation: Reading with evrFileTools/NRadEngine order produces sensible values:
This confirms evrFileTools has the correct Frame field order.
4. Element Size vs Count vs Length
The manifest allows for padding between elements. The relationship is:
Key insight: Always use Section.Length for position calculation, not ElementSize * Count.
Test Validation Results
All Tests Passing:
Remaining Observations:
CompressedSize > 0butLength = 0Applied Fixes Summary
Fix 1: Updated FileMetadata Structure ✅
Fix 2: Length-Based Section Offsets ✅
Fix 3: Correct Element Stride ✅
Test Data Analysis
Small Manifest (2b47aab238f60515)
Large Manifest (48037dc70b0ecab2)
Compatibility Notes
vs Carnation (JavaScript)
Carnation uses different Frame field order. If carnation works with actual files, either:
vs NRadEngine (C++)
NRadEngine structures match the actual file format:
Appendix: Raw Data Samples
Frame Data Sample (from offset 3288):
Manifest Section Headers:
Analysis performed: December 19, 2025
Fixes applied: December 19, 2025
Tools analyzed: evrFileTools (Go), NRadEngine (C++), carnation (JavaScript)