Fix I/O bugs by danrbailey · Pull Request #2230 · AcademySoftwareFoundation/openvdb

danrbailey · 2026-06-10T16:57:12Z

This addresses some issues in the new feature/io branch, one of which was discovered in production, a couple more through AI code review, one that I believe pre-dated the I/O changes.

An underflow bug - this is the only serious one which affects VDB Points grids with few attributes that would cause the unsigned number of attributes to underflow.
The background value was incorrectly using ValueT instead of StorageValueT, which caused an issue reading inactive values.
Fix an issue when reading old VDB files from prior to 2014 (<221)
Instance grids are being clipped with the wrong transform, the world-space bbox transform needs to be transformed to the index-space of the instanced grid.

Signed-off-by: Dan Bailey <danbailey@ilm.com>

…he value mask was used instead of the child mask Signed-off-by: Dan Bailey <danbailey@ilm.com>

…ect transform Signed-off-by: Dan Bailey <danbailey@ilm.com>

…chive assignment operator Signed-off-by: Dan Bailey <danbailey@ilm.com>

Signed-off-by: Dan Bailey <danbailey@ilm.com>

… stream is not seekable Signed-off-by: Dan Bailey <danbailey@ilm.com>

…nd align with all the other initialize methods Signed-off-by: Dan Bailey <danbailey@ilm.com>

Signed-off-by: Dan Bailey <danbailey@ilm.com>

…nstead of StorageValueT causing inactive values to be read incorrectly Signed-off-by: Dan Bailey <danbailey@ilm.com>

Signed-off-by: Dan Bailey <danbailey@ilm.com>

…mance Signed-off-by: Dan Bailey <danbailey@ilm.com>

Copilot

Pull request overview

This PR improves OpenVDB I/O robustness and correctness in the feature/io work by addressing several read/write edge cases (points grids underflow/hang, inactive background reconstruction, old-file compatibility, and instanced-grid clipping), and by adding regression tests to prevent recurrence.

Changes:

Fix multiple codec/streaming issues: point-data pass-count underflow, non-seekable stream skipping behavior, and inactive-value reconstruction by removing reliance on a stream background pointer.
Correct instanced-grid clipping by deferring world-space clip application until after the instance transform is applied.
Add/expand regression tests and clarify/adjust topology-only behavior (including new MetadataOnly read mode and leaf-buffer allocation behavior).

Reviewed changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
openvdb/openvdb/unittest/TestStream.cc	Adds regression coverage ensuring `io::Stream` assignment preserves archive flags.
openvdb/openvdb/unittest/TestPointCodec.cc	Expands topology-only expectations; adds tests for empty point grids and non-seekable attribute skipping.
openvdb/openvdb/unittest/TestFile.cc	Adds regression test for correct clipping behavior on instanced grids with differing transforms.
openvdb/openvdb/unittest/TestCodec.cc	Adds initialization idempotency test and regression test for inactive-value reconstruction.
openvdb/openvdb/tree/LeafNode.h	Documents PartialCreate/leaf allocation expectations and threading considerations.
openvdb/openvdb/tree/LeafBuffer.h	Tightens `data()` semantics (debug assertion) and improves PartialCreate/alloc documentation.
openvdb/openvdb/tree/InternalNode.h	Documents PartialCreate usage and threading expectations.
openvdb/openvdb/points/StreamCompression.cc	Updates page skipping to handle non-seekable streams (but currently uses an allocating discard buffer).
openvdb/openvdb/points/PointDataIO.h	Updates point-data compression helpers to accept an explicit background pointer.
openvdb/openvdb/points/AttributeArray.h	Updates paged-buffer skipping to handle non-seekable streams (but currently allocates and uses `std::vector` without including it).
openvdb/openvdb/io/Stream.h	Adds `io::Stream` ctor overload that accepts `ReadOptions`.
openvdb/openvdb/io/Stream.cc	Wires `ReadOptions` through stream reading and ensures assignment copies `Archive` state.
openvdb/openvdb/io/io.h	Extends `StreamMetadata` with an `allocateLeafBuffers()` flag used during topology-only reads.
openvdb/openvdb/io/File.cc	Uses `MetadataOnly` for metadata reads; fixes instanced-grid clipping by clipping after instance assembly.
openvdb/openvdb/io/Compression.h	Passes explicit background to (de)compression to avoid stale background pointer usage.
openvdb/openvdb/io/Codec.h	Introduces `MetadataOnly` and adjusts `TopologyOnly`; changes codec `readBuffers` signature to include a size.
openvdb/openvdb/io/Codec.cc	Makes `io::internal::initialize()` clear the codec registry to be idempotent.
openvdb/openvdb/io/Archive.cc	Updates codec selection fallback and read-mode handling; adds leaf-buffer allocation flagging for topology-only legacy path.
openvdb/openvdb/Grid.h	Allocates/fills leaf buffers for topology-only legacy reads when requested via stream metadata.
openvdb/openvdb/codecs/ValueMaskCodec.h	Updates `readBuffers` override signature.
openvdb/openvdb/codecs/TopologyCodec.h	Fixes background handling and old-version ordering; stores storage-typed background in codec data and allocates leaf buffers for topology-only.
openvdb/openvdb/codecs/ScalarCodec.h	Removes reliance on stream background pointer by threading background explicitly.
openvdb/openvdb/codecs/PointIndexCodec.h	Threads storage background into scalar-leaf reads for point index grids.
openvdb/openvdb/codecs/PointDataCodec.h	Guards against pass-count underflow; adds attribute skipping and descriptor-homogeneity check; threads storage background.
openvdb/openvdb/codecs/impl/ScalarLeafCodec.h	Threads storage background into `readCompressedValues` calls; updates write path to pass explicit background.
openvdb/openvdb/codecs/BoolCodec.h	Updates `readBuffers` override signature.

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

danrbailey · 2026-07-18T01:10:38Z

+    auto meta = io::getStreamMetadataPtr(is);
+    if (meta && meta->seekable()) {
+        is.seekg(bytes, std::ios_base::cur);
+    } else {
+        std::vector<char> tempData(bytes);
+        is.read(tempData.data(), bytes);
+    }


This is valid feedback, but this was an intentional choice. We don't use istream::ignore() anywhere in the codebase at present and use this pattern of reading to a temporary buffer.

I would like to convert all of these over so that we can use non-seekable streams. However for consistency, I'd prefer this fits in with the rest of the codebase for now and then do a bulk upgrade and add non-seekable stream unit tests in a separate pass.

danrbailey · 2026-07-18T01:10:52Z

+            auto meta = io::getStreamMetadataPtr(inputStream);
+            if (meta && meta->seekable()) {
+                inputStream.seekg(mCompressedBytes, std::ios_base::cur);
+            } else {
+                std::vector<char> tempData(mCompressedBytes);
+                inputStream.read(tempData.data(), mCompressedBytes);
+            }


danrbailey · 2026-07-18T01:13:53Z

+            // Attribute filtering requires homogeneous descriptors across all
+            // leaves because skip decisions are made per-index across all leaves.
+            const auto* firstDesc = &leaves[0]->attributeSet().descriptor();
+            for (size_t i = 1; i < leaves.size(); ++i) {
+                if (&leaves[i]->attributeSet().descriptor() != firstDesc) {
+                    OPENVDB_THROW(IoError,
+                        "Attribute filtering is not supported for PointDataGrids "
+                        "with heterogeneous descriptors");
+                }
+            }
+            const auto& nameMap = firstDesc->map();


Actually, the stricter test is what we are after here. The goal is to only filter when using shared descriptors.

danrbailey · 2026-07-18T01:14:47Z

    /// Deserialize topology only; value buffers are skipped. The resulting
    /// grid has a valid tree structure (active/inactive state, node
-    /// hierarchy) but leaf buffer data is left at its default (background)
-    /// value. Useful when only the active-voxel mask is needed and
-    /// avoiding the cost of reading large value buffers is desirable.
-    TopologyOnly
+    /// hierarchy) and all leaf buffers are allocated and zero-filled.
+    /// Useful when only the active-voxel mask is needed and avoiding the
+    /// cost of reading large value buffers is desirable.
+    TopologyOnly,


That's a fair comment.

danrbailey · 2026-07-18T01:15:30Z

+    // When called from the legacy (non-codec) TopologyOnly path, the stream
+    // metadata carries a flag requesting allocation+zero-fill of leaf buffers
+    // (PartialCreate leaves them unallocated after readTopology).


Yup, will change this, this implementation changed and the comment was not updated.

danrbailey · 2026-07-18T01:18:04Z

+            if (readOptions.readMode == io::ReadMode::TopologyOnly) {
+                // signal Grid<TreeT>::readTopology to allocate+zero-fill leaf buffers
+                if (auto meta = io::getStreamMetadataPtr(is)) {
+                    meta->setAllocateLeafBuffers(true);
+                }
+            }
            grid->readTopology(is);
        }


Probably wouldn't hurt to change this, though I'm not sure we'd ever end up in this state where it would be valid to continue.

apradhana · 2026-07-15T21:29:19Z

        gd.readStreamPos(is);
        descriptors.push_back(gd);
-        GridBase::Ptr grid = Archive::readGrid(gd, is, io::ReadOptions{});
+        GridBase::Ptr grid = Archive::readGrid(gd, is, readOptions);


This can break when readOptions asks for MetadataOnly or TopologyOnly. Archive::readGrid() stops before reading all bytes for the grid, but Stream then tries to read the next grid from the current position. That position is still inside the previous grid’s data, so the next grid header is read from the wrong place. Can we either disallow these read modes for Stream, or skip the unread bytes before reading the next grid?

This is the failure case:

TEST_F(TestStream, testMetadataOnlyReadMultipleGrids) { using namespace openvdb; Int32Grid::Ptr grid1 = Int32Grid::create(0); grid1->setName("first"); grid1->tree().setValue(Coord(0, 0, 0), 1); FloatGrid::Ptr grid2 = FloatGrid::create(0.0f); grid2->setName("second"); grid2->tree().setValue(Coord(1, 2, 3), 2.0f); std::ostringstream ostr(std::ios_base::binary); io::Stream(ostr).write(GridPtrVec{grid1, grid2}); io::ReadOptions readOptions; readOptions.readMode = io::ReadMode::MetadataOnly; std::istringstream is(ostr.str(), std::ios_base::binary); io::Stream strm(is, readOptions); GridPtrVecPtr grids = strm.getGrids(); ASSERT_TRUE(grids); ASSERT_EQ(grids->size(), size_t(2)); EXPECT_EQ((*grids)[0]->getName(), std::string("first")); EXPECT_EQ((*grids)[1]->getName(), std::string("second")); }

with an error of

[ RUN ] TestStream.testMetadataOnlyReadMultipleGrids unknown file: Failure C++ exception with description "KeyError: Cannot read grid " thrown in the test body. [ FAILED ] TestStream.testMetadataOnlyReadMultipleGrids

Thanks, I'll disable these modes for Stream as you suggest for now. Ultimately, I'd like to skip the bytes but perhaps that could be implemented together with the extension to make this work for non-seekable streams.

Thanks, Dan. A suggestion: maybe you can add a //TODO for byte skipping as part of non-seekable stream support.

Fix underflow bug when reading or writing empty grids

c1118e2

Signed-off-by: Dan Bailey <danbailey@ilm.com>

danrbailey requested a review from kmuseth as a code owner June 10, 2026 16:57

danrbailey added 9 commits June 10, 2026 22:44

Fix an issue in the old version codepath for reading topology where t…

74db979

…he value mask was used instead of the child mask Signed-off-by: Dan Bailey <danbailey@ilm.com>

Fix issue where instanced grids were not being clipped using the corr…

b8f7f6c

…ect transform Signed-off-by: Dan Bailey <danbailey@ilm.com>

Fix a bug where the Stream assignment operator was not calling the Ar…

98d9d0e

…chive assignment operator Signed-off-by: Dan Bailey <danbailey@ilm.com>

Fix topology-only and data race condition

8cb8ceb

Signed-off-by: Dan Bailey <danbailey@ilm.com>

Add conversion codec fallback and read diagnostics

863900f

Signed-off-by: Dan Bailey <danbailey@ilm.com>

Fix a bug in PointDataGrid where read-and-discard not being used when…

8dceecf

… stream is not seekable Signed-off-by: Dan Bailey <danbailey@ilm.com>

Calling initialize will now clear the registry making it idempotent a…

132ee15

…nd align with all the other initialize methods Signed-off-by: Dan Bailey <danbailey@ilm.com>

Fix a potential memory leak when reading topology

6624c4d

Signed-off-by: Dan Bailey <danbailey@ilm.com>

Throw an error if the attribute descriptor is not homogeneous

f173582

Signed-off-by: Dan Bailey <danbailey@ilm.com>

danrbailey force-pushed the fix_io_bugs branch from a6bc14b to 2b9a5e1 Compare June 12, 2026 18:06

danrbailey requested a review from Idclip as a code owner June 12, 2026 18:06

danrbailey force-pushed the fix_io_bugs branch from 2b9a5e1 to a239572 Compare June 12, 2026 18:09

Fix an issue where the background value was previously using ValueT i…

4208d6b

…nstead of StorageValueT causing inactive values to be read incorrectly Signed-off-by: Dan Bailey <danbailey@ilm.com>

danrbailey force-pushed the fix_io_bugs branch 2 times, most recently from 415fabf to 2a116b4 Compare June 12, 2026 22:56

Add ReadOptions argument to Stream

ca04e21

Signed-off-by: Dan Bailey <danbailey@ilm.com>

danrbailey force-pushed the fix_io_bugs branch from 2a116b4 to ca04e21 Compare June 17, 2026 20:38

Merge remote-tracking branch 'upstream/feature/io' into fix_io_bugs

009cf88

danrbailey added the io label Jun 18, 2026

Add an Index64 size to readBuffers() to aid seeking for faster perfor…

392162b

…mance Signed-off-by: Dan Bailey <danbailey@ilm.com>

swahtz requested a review from Copilot July 15, 2026 20:42

Copilot started reviewing on behalf of swahtz July 15, 2026 20:43 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

apradhana reviewed Jul 15, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix I/O bugs - #2230

Fix I/O bugs#2230
danrbailey wants to merge 14 commits into
AcademySoftwareFoundation:feature/iofrom
danrbailey:fix_io_bugs

danrbailey commented Jun 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

danrbailey Jul 18, 2026

Uh oh!

danrbailey Jul 18, 2026

Uh oh!

danrbailey Jul 18, 2026

Uh oh!

danrbailey Jul 18, 2026

Uh oh!

danrbailey Jul 18, 2026

Uh oh!

danrbailey Jul 18, 2026

Uh oh!

apradhana Jul 15, 2026

Uh oh!

apradhana Jul 16, 2026

Uh oh!

danrbailey Jul 18, 2026

Uh oh!

apradhana Jul 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

danrbailey commented Jun 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants