Codec Framework and Delayed Loading Removal - #2281
Conversation
…by it Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Remove Delayed Loading
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Remove unused class members and methods in future ABI=14
Signed-off-by: Dan Bailey <danbailey@ilm.com>
…f I/O methods Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
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>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Fix I/O bugs
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
Signed-off-by: Dan Bailey <danbailey@ilm.com>
| // Cleanup | ||
| std::remove(floatPath.c_str()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Codex found an edge case of failing to read float as half when a file doesn't have a grid offsets
TEST_F(TestCodec, testReadModeHalfAppliesToFileWithoutGridOffsets)
{
using namespace openvdb;
using namespace openvdb::io;
CodecRegistry::clear();
io::internal::initialize();
FloatGrid::Ptr srcGrid = FloatGrid::create(3.25f);
srcGrid->setName("float_to_half");
srcGrid->tree().setValue(Coord(0, 0, 0), 1.0f / 3.0f);
const std::string path = "test_float_to_half_no_offsets.vdb";
{
std::ofstream os(path, std::ios_base::out | std::ios_base::binary);
io::Stream(os).write(GridPtrVec{srcGrid});
}
ReadOptions readOptions;
readOptions.readMode = ReadMode::Half;
{
io::File f(path);
f.open();
GridPtrVecPtr grids = f.getGrids(readOptions);
ASSERT_TRUE(grids);
ASSERT_EQ(size_t(1), grids->size());
EXPECT_TRUE((*grids)[0]->isType<HalfGrid>());
f.close();
}
{
io::File f(path);
f.open();
GridBase::Ptr grid = f.readGrid(srcGrid->getName(), readOptions);
ASSERT_TRUE(grid);
EXPECT_TRUE(grid->isType<HalfGrid>());
f.close();
}
std::remove(path.c_str());
}
The problem is io::Stream(os).write(...) creates a VDB file without grid offsets. For that kind of file, File::open() reads the grids using default options before the caller passes ReadOptions. So later calls to getGrids(readOptions) or readGrid(name, readOptions) return the cached original FloatGrid, not a converted HalfGrid.
There was a problem hiding this comment.
Hi Andre, thanks for testing. This is true and I think it's possibly also an issue for instance VDBs too. We currently only offer on-the-fly conversion, as in the data is converted between types while it is being read. The caching approach used for non grid offset VDBs doesn't align with that.
I don't think it's a good idea to add the ReadOptions to the File::open() method, but there are a few of other options to discuss:
- We just add a ReadDiagnostics warning that documents that on-the-fly grid conversion is not supported for non grid offset VDBs. I don't think it deserves to be an exception, but maybe we should consider that too.
- We do in-memory grid-to-grid conversion - have you explored that for float->half grids and is it a feature that is currently supported?
- We remove on-the-fly grid conversion altogether and support it properly for the new file format only. I don't intend to support writing non grid offset VDBs in the new file layouts (except for writing legacy files).
I do agree that quietly ignoring the hint and just returning the float grid is unexpected and should be resolved, so I'll have a think about which of these options might be most viable.
There was a problem hiding this comment.
I opted to do the in-memory grid conversion to resolve this issue. See #2298
There was a problem hiding this comment.
Thanks for addressing this issue this way.
| bool mDelayedLoadMeta = false; | ||
| uint64_t mLeaf = 0; | ||
| uint32_t mTest = 0; // for testing only | ||
| bool mAllocateLeafBuffers = false; |
There was a problem hiding this comment.
This may be blocking because of ABI incompatibilities between 13.0 and 13. 1.
StreamMetadata is shared between coexisting OpenVDB versions and is part of the protected Grid ABI. A 13.1 reader can access or copy this field from an Impl created by 13.0, where this bool does not exist or was never initialized. Tail padding does not make that access safe. Could this flag use existing compatible storage such as AuxDataMap, or be restricted to ABI 14?
For example, setAllocateLeafBuffers(true) could store true under a private key in auxData(). allocateLeafBuffers() could look up that key with std::any_cast<bool>, and clearing the flag could erase it. This uses storage that already exists and is initialized in ABI 13, so no new Impl member is needed.
There was a problem hiding this comment.
From TSC meeting 8/26/2026 this is okay, because ABI changes is applied only for Tree and Grid contracts say that we want to be able to reinterpret_cast the pointer. In the discussion, it was mentioned that it's for the old grid reading workflow. It is used to be read during a topology-only read. So it seems okay.
For completeness, I'll also include a discussion all the way back from 2019: https://github.com/AcademySoftwareFoundation/openvdb/blob/master/tsc/meetings/2019-09-26.md?plain=1#L27-L45. So there may be a compatibility issue with Houdini.
|
This is mainly a comment regarding the removal of Without PImpl, those private members become part of the public object’s binary layout. Future internal changes may therefore require applications and plugins to be rebuilt. Mixing versions also becomes unsafe: old headers might allocate 72 bytes for File, while the new library expects 296 bytes, potentially causing crashes or memory corruption. Again, this is non-blocking. |
This is sort of a general comment about why pimpl might be useful. If we remove delayed loading, then the StreamMetadata is no longer part of the Grid ABI because you cannot pass around a partially created grid. Using the codec framework improves flexibility and does away with all the complexity of managing and maintaining StreamMetadata objects. That is the overall intention here. |
Thanks for clarifying this and for the discussion during TSC. |
This introduces the new codec framework and completely removes delayed-loading / out-of-core into the master branch.
All of the components included in this PR have been reviewed independently:
#2155
#2164
#2166
#2179
#2180
#2184
#2185
#2197
#2196
#2230
These changes have been tested extensively at ILM.
For completeness, here are the new additions to the changelog on this branch from aggregating all the pending changes: