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