Overhaul for streaming data directly from zip (optimization)

[frheault]

(Similar description to trx-rs PR)

Proposed modifications to allow fair comparison for benchmarking across languages, the biggest modification is to up/down-cast to uint32 offsets since it is not completely illegal. Some of the benchmark files add offsets as uint64. I believe this is mostly to smooth operations between languages, I personally think that virtually no one will create a tractogram with 40M streamlines with 100 points each, but it is possible.

I apologize for the formatting, I believe my VScode did some automatic formatting, @mattcieslak if you could tell me what standard/linter/tool to use (or maybe I should manually revert the identical lines?).

I believe some unit testing is needed to verify if everything is alright, but I tested single language round-trip (load/save) and between language compatibility. Then I benchmarked on big files: https://github.com/tee-ar-ex/trx-manuscript-2026-benchmark

[Copilot]

Pull request overview

This PR updates AnyTrxFile::_create_from_pointer()’s handling of groups/ entries to allow loading group membership arrays stored in additional integer dtypes by converting them into the internal uint32 representation, enabling more consistent cross-language benchmarking/interoperability.

Changes:

• Allow groups/ arrays with integer dtypes beyond uint32 (e.g., uint64, int64, int32, uint16, etc.) by casting to uint32.
• Add a guard intended to prevent unsafe downcasting when NB_STREAMLINES exceeds 32-bit capacity.
• Materialize group arrays to owned memory before storing in trx.groups.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

❌ Patch coverage is 62.11251% with 660 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.62%. Comparing base (5cf07d1) to head (72d9a39).

Files with missing lines	Patch %	Lines
src/legacy_io.cpp	39.02%	461 Missing ⚠️
src/trx.cpp	73.16%	139 Missing ⚠️
include/trx/trx.tpp	79.87%	33 Missing ⚠️
tests/test_trx_legacy_io.cpp	87.95%	20 Missing ⚠️
tests/test_trx_gs_consistency.cpp	90.90%	5 Missing and 1 partial ⚠️
tests/test_trx_anytrxfile.cpp	98.27%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #49      +/-   ##
==========================================
- Coverage   88.66%   83.62%   -5.05%     
==========================================
  Files          15       18       +3     
  Lines        7765     9195    +1430     
  Branches     1044     1328     +284     
==========================================
+ Hits         6885     7689     +804     
- Misses        880     1505     +625     
- Partials        0        1       +1     

Flag	Coverage Δ
linux	82.63% <61.76%> (-5.24%)	⬇️
macos	83.44% <62.03%> (-5.06%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Suppressed comments (6)

src/trx.cpp:934

• StreamingZipWriter::add_file_stream_begin truncates out.tellp() into a uint32 offset. If the archive grows past 4GiB, offsets wrap and the central directory becomes invalid. Add a size/offset guard (or implement Zip64).

    void add_file_stream_begin(const std::string& name) {
        Entry e;
        e.name = name;
        e.offset = static_cast<uint32_t>(out.tellp());
        e.size = 0; // Will be updated later
        e.crc = 0;  // Will be updated later
        e.flags = 8;

src/trx.cpp:960

• StreamingZipWriter::stream_data accumulates a streamed entry size in a uint32. Once the streamed file exceeds 4GiB, the addition wraps and produces a corrupted ZIP. Add an overflow guard and fail fast with a clear message (or implement Zip64).

    void stream_data(const char* data, size_t size) {
        if (size > 0) {
            out.write(data, size);
            entries.back().size += static_cast<uint32_t>(size);
            entries.back().crc = calculate_crc32(entries.back().crc, reinterpret_cast<const unsigned char*>(data), size);
        }

src/trx.cpp:973

• StreamingZipWriter::finalize truncates the central-directory offset (out.tellp()) into uint32 and the entry count into uint16 (via later casts). If the archive grows too large, this silently produces an invalid ZIP. Add a guard before writing the central directory/EOCD (or implement Zip64).

    void finalize() {
        uint32_t cd_offset = static_cast<uint32_t>(out.tellp());
        for (const auto& e : entries) {

src/trx.cpp:188

• read_le32 shifts promoted int values (from uint8_t) by 16/24 bits. If the high byte is >= 0x80, the expression (ptr[3] << 24) can overflow a signed int, which is undefined behavior. Cast each byte to uint32_t before shifting to keep the operations unsigned.

static uint32_t read_le32(const uint8_t* ptr) {
    return ptr[0] | (ptr[1] << 8) | (ptr[2] << 16) | (ptr[3] << 24);
}

src/trx.cpp:473

• header.json is read with zip_fread() but the return value is ignored (and zip_stat_index() isn’t checked). On a short read or error, this can parse uninitialized/partial data. Also, if sb.size==0, writing to &header_str[0] is undefined. Check zip_stat_index() and zip_fread() results before parsing.

          zip_stat_t sb;
          zip_stat_index(zf.get(), header_idx, 0, &sb);
          std::string header_str(sb.size, ' ');
          zip_fread(hz.get(), &header_str[0], sb.size);
          std::string err;

src/trx.cpp:906

• StreamingZipWriter stores entry offsets/sizes in uint32 (classic ZIP). For large TRX archives, out.tellp() and/or entry size can exceed UINT32_MAX; the current code truncates via static_cast<uint32_t>, producing a corrupted archive with no error. At minimum, detect this and throw a clear error (or implement Zip64 / fall back to libzip for large outputs).

This issue also appears in the following locations of the same file:

• line 928
• line 955
• line 971

    void add_file(const std::string& name, const char* data, size_t size) {
        Entry e;
        e.name = name;
        e.offset = static_cast<uint32_t>(out.tellp());
        e.size = static_cast<uint32_t>(size);
        e.crc = calculate_crc32(0, reinterpret_cast<const unsigned char*>(data), size);
        e.flags = 0;

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 17 changed files in this pull request and generated 7 comments.

Suppressed comments (3)

src/trx.cpp:2

• std::vector<uint8_t> only guarantees 1-byte alignment. Reinterpreting its storage as float*, double*, or Eigen::half* and writing through those pointers is undefined behavior on platforms that require alignment (can crash on ARM). Prefer writing via std::memcpy per-element into the byte buffer, or allocate a properly-aligned typed buffer (e.g., std::vector<float>/std::vector<double>/std::vector<Eigen::half>) and then serialize/copy its bytes.
  include/trx/trx.h:46
• This introduces a json type alias in the global namespace and inside namespace trx. Exporting json globally can easily collide with other translation units/libraries that define their own json alias/type, and it also makes using ::json; compile even when the library intended trx::json. Recommend removing the global using json = ...; and keeping the alias only within namespace trx (or behind a dedicated trx::json name).

using json = json11::Json;

namespace trx {
namespace fs = std::filesystem;
using json = json11::Json;
}

include/trx/legacy_io.h:16

• This header uses std::shared_ptr but does not include <memory>. Relying on indirect includes is fragile and can break builds depending on include order. Add #include <memory> to this header.

    std::shared_ptr<trx::AnyTrxFile> original_trx;

[arokem]

@mattcieslak : any chance you could review this PR??

[mattcieslak]

If helpful I had an automated claude review on this:

Blocking

1. [HIGH] Every TRK/TCK/VTK → TRX conversion writes an unreadable .trx — src/legacy_io.cpp:620
   save_trx builds a TrxFile (header seeded with NB_VERTICES/NB_STREAMLINES), then trx.header = header_to_use; overwrites it with the legacy header, which only carries DIMENSIONS+VOXEL_TO_RASMM. TrxFile::save() doesn't re-inject the counts, so the written header.json lacks them and any reload throws "Missing NB_VERTICES or NB_STREAMLINES." Reproduced: gs.tck → save_trx(--ref gs.nii) → load_any fails. This is my July fix that got dropped in a rebase — the fix is known-good (merge the counts into header_to_use before assigning). No test catches it because nothing exercises a legacy save.

2. [MED-HIGH] load_vtk crashes the process on crafted/truncated .vtk — src/legacy_io.cpp:361, :396
   tr.offsets.resize(num_offsets) and skip_buf.resize(n_pts) use file-controlled counts with no size check (a negative n_pts casts to a huge size_t). load_vtk and main() have no try/catch, so std::bad_alloc reaches std::terminate instead of returning false. The POINTS path right above it is guarded — the newer OFFSETS/LINES code just doesn't copy that guard.

3. [MED] Truncated VTK OFFSETS → uninitialized offsets → OOB read — src/legacy_io.cpp:363–377
   The OFFSETS read loop never checks stream state (val left uninitialized on a short read), unlike the fallback loop immediately below which does if (!f) break;. Those garbage offsets feed save_trk's unchecked tr.pts[start..end] indexing → out-of-bounds read.

4. [MED] In-place directory save deletes the mmap-backed source before writing — src/trx.cpp:1174–1187
   The base branch detected a same-directory save (weakly_canonical compare, old lines 742–747) and skipped the destructive delete; that guard was removed. Now save("/x.trx", overwrite) on a directory loaded from /x.trx calls rm_dir first. Positions/offsets are mmap-backed by files inside that directory, so on Windows remove_all fails (open handles) → save throws; on Linux it destroys the on-disk data before the new bytes are committed (data-loss window if the write fails).

Lower priority

5. [LOW-MED] uint32 groups aren't range-validated — src/trx.cpp:584, :811 — the native uint32 path stores indices with no < NB_STREAMLINES check, while every other integer dtype is checked; the comment at :820 claims all values are validated. A uint32 group with an out-of-range index loads silently.

6. [LOW/latent] Zip-loaded arrays mapped writable over the source archive — src/trx.cpp:522 — a mutate-after-zip-load would corrupt the user's .trx. No current caller triggers it.

Test gaps (highest-value first)
The entire legacy save half (save_trx/trk/tck/vtk) has zero test calls — exactly where bug #1 lives. Add a load .tck → save_trx → load_any round-trip.
gs.nii is committed but unused — the new cross-endian NIfTI parsing is untested.
The gs-consistency test compares VTK points by magnitude, masking any sign/orientation error in the VTK reader.

[arokem]

I believe that 72d9a39 addresses most of the (major) issues in the review. Is that correct, @frheault

[frheault]

Yes @arokem and I brought back an older test in trx-cpp and made sure that the benchmark testing routine was strict about that (at least the bugs were potentially risky, but they wrapper functions were doing the right things)