From badd7360ca50c837bf07fdf46dd14e128a268e13 Mon Sep 17 00:00:00 2001 From: Filip Leonarski Date: Fri, 9 Oct 2026 21:15:01 +0200 Subject: [PATCH] HDF5 reader: read variable-length string attributes and datasets ReadAttrStr and HDF5DataSet::ReadString read every string as fixed-length, sizing the buffer by H5Tget_size. For a variable-length string (what h5py writes for a str, UTF-8) that size is a pointer, so the read put a char* into the buffer and returned garbage. transformation_type then never equalled "rotation", and an EIGER master whose goniometer attributes were written that way (depends_on, transformation_type, units on /entry/sample/transformations) was processed as stills although it holds a valid rotation sweep. All string reads now go through one helper, ReadHDF5String, which handles both fixed- and variable-length strings in either character set; ReadElement already handled the variable-length case and now uses the same helper. Test: JFJochReader_Goniometer_VariableLengthStrings reads the rotation axis from a master with fixed-length ASCII and with variable-length UTF-8 string attributes and depends_on dataset. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01SVmAWnzCmRKAXVUCdc4iNi --- tests/JFJochReaderTest.cpp | 91 ++++++++++++++++++++++++++++++++++++++ writer/HDF5Objects.cpp | 59 +++++++++++++++--------- writer/HDF5Objects.h | 33 ++++---------- 3 files changed, 136 insertions(+), 47 deletions(-) diff --git a/tests/JFJochReaderTest.cpp b/tests/JFJochReaderTest.cpp index 782dff797..32d044c49 100644 --- a/tests/JFJochReaderTest.cpp +++ b/tests/JFJochReaderTest.cpp @@ -475,6 +475,97 @@ TEST_CASE("JFJochReader_Goniometer", "[HDF5][Full]") { REQUIRE(H5Fget_obj_count(H5F_OBJ_ALL, H5F_OBJ_ALL) == 0); } +// h5py writes str attributes and datasets as variable-length UTF-8 strings; our writer writes +// fixed-length ASCII. A reader that only understood the second read transformation_type of the +// first as garbage and processed a third-party rotation sweep as stills. +TEST_CASE("JFJochReader_Goniometer_VariableLengthStrings", "[HDF5][Full]") { + DiffractionExperiment x(DetJF(1)); + + x.FilePrefix("test_reader_vlen_str").ImagesPerTrigger(5).OverwriteExistingFiles(true); + x.BeamX_pxl(100).BeamY_pxl(200).DetectorDistance_mm(150) + .IncidentEnergy_keV(WVL_1A_IN_KEV).PixelSigned(false).BitDepthImage(16) + .FrameTime(std::chrono::microseconds(500), std::chrono::microseconds(10)); + x.Goniometer(GoniometerAxis("omega", 95, 0.1f, Coord(1,0,0),{})); + + RegisterHDF5Filter(); + + ScanResultGenerator generator(x); + std::vector image(x.GetPixelsNum(), 0); + { + StartMessage start_message; + x.FillMessage(start_message); + + FileWriter file_set(start_message); + + DataMessage message{}; + for (int i = 0; i < 5; i++) { + message.image = CompressedImage(image, x.GetXPixelsNum(), x.GetYPixelsNum()); + message.number = i; + REQUIRE_NOTHROW(file_set.WriteHDF5(message)); + REQUIRE_NOTHROW(generator.Add(message)); + } + + EndMessage end_message; + end_message.max_image_number = 5; + generator.FillEndMessage(end_message); + file_set.WriteHDF5(end_message); + + file_set.Finalize(); + } + + const bool variable_length = GENERATE(false, true); + if (variable_length) { + hid_t file = H5Fopen("test_reader_vlen_str_master.h5", H5F_ACC_RDWR, H5P_DEFAULT); + REQUIRE(file >= 0); + hid_t str_type = H5Tcopy(H5T_C_S1); + H5Tset_size(str_type, H5T_VARIABLE); + H5Tset_cset(str_type, H5T_CSET_UTF8); + hid_t scalar = H5Screate(H5S_SCALAR); + + hid_t omega = H5Dopen2(file, "/entry/sample/transformations/omega", H5P_DEFAULT); + REQUIRE(omega >= 0); + for (const auto &[name, value] : std::vector>{ + {"transformation_type", "rotation"}, {"depends_on", "."}, {"units", "deg"}}) { + H5Adelete(omega, name.c_str()); + const char *ptr = value.c_str(); + hid_t attr = H5Acreate2(omega, name.c_str(), str_type, scalar, H5P_DEFAULT, H5P_DEFAULT); + REQUIRE(attr >= 0); + REQUIRE(H5Awrite(attr, str_type, &ptr) >= 0); + H5Aclose(attr); + } + H5Dclose(omega); + + H5Ldelete(file, "/entry/sample/depends_on", H5P_DEFAULT); + const char *dep = "/entry/sample/transformations/omega"; + hid_t ds = H5Dcreate2(file, "/entry/sample/depends_on", str_type, scalar, + H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT); + REQUIRE(ds >= 0); + REQUIRE(H5Dwrite(ds, str_type, H5S_ALL, H5S_ALL, H5P_DEFAULT, &dep) >= 0); + H5Dclose(ds); + + H5Sclose(scalar); + H5Tclose(str_type); + H5Fclose(file); + } + { + JFJochHDF5Reader reader; + reader.ReadFile("test_reader_vlen_str_master.h5"); + auto dataset = reader.GetDataset(); + + REQUIRE(dataset->experiment.GetGoniometer().has_value()); + CHECK(dataset->experiment.GetGoniometer()->IsScanning()); + CHECK(dataset->experiment.GetGoniometer()->GetName() == "omega"); + CHECK(dataset->experiment.GetGoniometer()->GetStart_deg() == Catch::Approx(95.0)); + CHECK(dataset->experiment.GetGoniometer()->GetIncrement_deg() == Catch::Approx(0.1f).margin(0.00001f)); + CHECK(dataset->experiment.GetGoniometer()->GetAxis().x == 1); + CHECK(dataset->experiment.GetGoniometer()->GetAxis().y == 0); + CHECK(dataset->experiment.GetGoniometer()->GetAxis().z == 0); + } + remove("test_reader_vlen_str_master.h5"); + + REQUIRE(H5Fget_obj_count(H5F_OBJ_ALL, H5F_OBJ_ALL) == 0); +} + // The axis name is free-form in the API, on the wire and in the writer - tests/CBORTest.cpp round // trips one literally called "z". The reader used to look only for "omega", so a sweep recorded // under any other name came back as stills, with nothing to indicate it. This is that case. diff --git a/writer/HDF5Objects.cpp b/writer/HDF5Objects.cpp index 3b0f8beae..dcf5156f0 100644 --- a/writer/HDF5Objects.cpp +++ b/writer/HDF5Objects.cpp @@ -506,6 +506,31 @@ HDF5Object & HDF5Object::Attr(const std::string &name, const std::vector return *this; } +std::string ReadHDF5String(hid_t type_id, const std::function &read) { + if (H5Tis_variable_str(type_id) > 0) { + char *tmp = nullptr; + if (read(type_id, &tmp) < 0) + throw JFJochException(JFJochExceptionCategory::HDF5, "Cannot read variable-length string"); + std::string out = (tmp != nullptr) ? std::string(tmp) : std::string{}; + if (tmp != nullptr) + H5free_memory(tmp); + return out; + } + + const size_t size = H5Tget_size(type_id); + if (size == 0) + return ""; + + std::string buffer(size, '\0'); + if (read(type_id, buffer.data()) < 0) + throw JFJochException(JFJochExceptionCategory::HDF5, "Cannot read string"); + + const size_t end = buffer.find('\0'); + if (end != std::string::npos) + buffer.resize(end); + return buffer; +} + std::vector HDF5Object::ReadAttrVec(const std::string &name) { hid_t attr_id = H5Aopen(id, name.c_str(), H5P_DEFAULT); if (attr_id < 0) @@ -579,11 +604,10 @@ std::string HDF5Object::ReadAttrStr(const std::string &name) { throw JFJochException(JFJochExceptionCategory::HDF5, "Attribute " + name + " is not a string"); } - // Get string size and read it - size_t size = H5Tget_size(type_id); - std::vector buffer(size + 1, '\0'); // +1 for null terminator to be safe - - if (H5Aread(attr_id, type_id, buffer.data()) < 0) { + std::string ret; + try { + ret = ReadHDF5String(type_id, [&](hid_t type, void *buffer) { return H5Aread(attr_id, type, buffer); }); + } catch (...) { H5Tclose(type_id); H5Aclose(attr_id); throw JFJochException(JFJochExceptionCategory::HDF5, "Cannot read attribute " + name); @@ -591,7 +615,7 @@ std::string HDF5Object::ReadAttrStr(const std::string &name) { H5Tclose(type_id); H5Aclose(attr_id); - return {buffer.data()}; + return ret; } @@ -923,22 +947,13 @@ std::string HDF5DataSet::ReadString() const { } HDF5DataType file_data_type(*this); - const size_t size = file_data_type.GetElemSize(); - - if (size == 0) - return ""; - - std::string buffer(size, '\0'); - - if (H5Dread(id, file_data_type.GetID(), H5S_ALL, H5S_ALL, H5P_DEFAULT, buffer.data()) < 0) - throw JFJochException(JFJochExceptionCategory::HDF5, - dataset_name + ": read unsuccessful"); - - const size_t end = buffer.find('\0'); - if (end != std::string::npos) - buffer.resize(end); - - return buffer; + try { + return ReadHDF5String(file_data_type.GetID(), [&](hid_t type, void *buffer) { + return H5Dread(id, type, H5S_ALL, H5S_ALL, H5P_DEFAULT, buffer); + }); + } catch (const JFJochException &) { + throw JFJochException(JFJochExceptionCategory::HDF5, dataset_name + ": read unsuccessful"); + } } void HDF5DataSet::Close() { diff --git a/writer/HDF5Objects.h b/writer/HDF5Objects.h index 9de50bdb0..eaed2d108 100644 --- a/writer/HDF5Objects.h +++ b/writer/HDF5Objects.h @@ -4,6 +4,7 @@ #pragma once #include +#include #include #include #include @@ -19,6 +20,10 @@ extern std::mutex hdf5_mutex; class HDF5DataSet; +// Reads one string of HDF5 type `type_id`, fixed- or variable-length (h5py writes str attributes +// as variable-length UTF-8). `read(type, buffer)` is the H5Aread/H5Dread call for one element. +std::string ReadHDF5String(hid_t type_id, const std::function &read); + struct HDF5VirtualDatasetMapping { std::string filename; std::string dataset; @@ -495,32 +500,10 @@ inline std::optional HDF5DataSet::ReadElement(size_t n if (H5Tget_class(file_type.GetID()) != H5T_STRING) throw JFJochException(JFJochExceptionCategory::HDF5, "ReadElement: dataset is not a string type"); - // Variable-length string - if (H5Tis_variable_str(file_type.GetID()) > 0) { - HDF5DataSpace mem_space({1}); - char *tmp = nullptr; - - if (H5Dread(id, file_type.GetID(), mem_space.GetID(), file_space.GetID(), H5P_DEFAULT, &tmp) < 0) - throw JFJochException(JFJochExceptionCategory::HDF5, "ReadElement unsuccessful"); - - std::string out = (tmp != nullptr) ? std::string(tmp) : std::string{}; - if (tmp != nullptr) - H5free_memory(tmp); - return out; - } - - // Fixed-length string - const size_t elem_size = file_type.GetElemSize(); - if (elem_size == 0) - return std::string{}; - - std::vector buffer(elem_size + 1, '\0'); HDF5DataSpace mem_space({1}); - - if (H5Dread(id, file_type.GetID(), mem_space.GetID(), file_space.GetID(), H5P_DEFAULT, buffer.data()) < 0) - throw JFJochException(JFJochExceptionCategory::HDF5, "ReadElement unsuccessful"); - - return std::string(buffer.data()); + return ReadHDF5String(file_type.GetID(), [&](hid_t type, void *buffer) { + return H5Dread(id, type, mem_space.GetID(), file_space.GetID(), H5P_DEFAULT, buffer); + }); } template