Skip to content

Remove LZ4HDF5 from blosc subcompressor list#580

Open
jwlodek wants to merge 5 commits intoareaDetector:masterfrom
jwlodek:fix-ndcodec-blosc-array
Open

Remove LZ4HDF5 from blosc subcompressor list#580
jwlodek wants to merge 5 commits intoareaDetector:masterfrom
jwlodek:fix-ndcodec-blosc-array

Conversation

@jwlodek
Copy link
Copy Markdown
Member

@jwlodek jwlodek commented Apr 24, 2026

I missed this on review of the LZ4HDF5 PR. The compressBlosc function, uses the enum value for NDCodecBloscComp_t to get the compressor name from the bloscCompName array. Following this addition, the codecs after lz4 no longer matched up. Specifying a zstd codec would actually cause a segfault since it was last. - I noticed that when I added the NDPluginCodec unit tests, but I thought my version of blosc just didn't support zstd.

My understanding is that the lz4hdf5 codec is standalone and not meant as a blosc subcompressor, is that correct? If it should exist as a subcompressor, then it'll need to be added to the string array and the record as well.

@jwlodek jwlodek requested a review from MarkRivers April 24, 2026 14:03
@jwlodek
Copy link
Copy Markdown
Member Author

jwlodek commented Apr 24, 2026

Also, I wonder if this enumeration (and the array of matching string names) should move to Codec.h alongside the main compressor enumeration - I have a driver that is producing blosc compressed arrays, but to include this enumeration I need to include the NDPluginCodec header, which doesn't seem like it should be the case.

@MarkRivers
Copy link
Copy Markdown
Member

Also, I wonder if this enumeration (and the array of matching string names) should move to Codec.h alongside the main compressor enumeration - I have a driver that is producing blosc compressed arrays, but to include this enumeration I need to include the NDPluginCodec header, which doesn't seem like it should be the case.

Sorry about that mistake in added lz4hdf5 to blosc enum.

I agree that moving the blosc enum to Codec.h is a good idea.

Should we also rename Codec.h to NDCodec.h, so it is less likely to conflict with other packages?

@jwlodek
Copy link
Copy Markdown
Member Author

jwlodek commented Apr 27, 2026

Should we also rename Codec.h to NDCodec.h, so it is less likely to conflict with other packages?

I think that is also a good idea. I'll make both changes and add them to this PR. I'll also check which drivers import Codec.h and update those as well.

@jwlodek
Copy link
Copy Markdown
Member Author

jwlodek commented Apr 27, 2026

I've renamed the file as well as the name of the Codec_t struct and the codecName array to be prefixed with ND. I also moved the static name arrays into a C++ file w/ extern, since using static would lead to compiler warnings about the array of blosc compressor names being unused in most of the files that imported this header. I also changed that to an array of C++ std::string to follow the pattern set by the base NDCodecName array.

@jwlodek
Copy link
Copy Markdown
Member Author

jwlodek commented Apr 27, 2026

It doesn't seem like any existing drivers actually import this header - in ADEiger the codec name is set directly as a string without the array lookup. So I think this change would actually be non-breaking for existing drivers.

@MarkRivers
Copy link
Copy Markdown
Member

It would probably be good to change ADEiger to use the array lookup to future proof it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants