From 259b43154e41648ae75570275ea3f737b89de855 Mon Sep 17 00:00:00 2001 From: leonarski_f Date: Sat, 22 Aug 2026 21:44:04 +0200 Subject: [PATCH] Writer: refuse a mistyped stream, and mark unreadable VDS frames Two ways the written files could misdescribe themselves without anyone noticing. The pixel format is stated twice and the two were never compared: each image carries its own type as a CBOR tag, which is what the data files are written with, while the master is typed from the start message. A stream whose header contradicts its images produced data files of one type under a master declaring another, and with NXmxVDS, HDF5 then converts silently on every read. HDF5DataFile::CreateFile now checks the two agree and refuses the run otherwise - the point where the values first meet, so it covers every path into the writer. Two test fixtures were relying on exactly that inconsistency. The HDF5 writer tests wrote uint16 buffers under a JUNGFRAU experiment, which converts to photon counts by default and so declares int16; they never read the pixels back, so it went unnoticed. The receiver-lite tests feed frames from compression_benchmark.h5, which really are signed int16, through a DECTRIS experiment, which declares unsigned by default - the same class of bug the pixel_signed propagation fixed on the live path. Both now declare what they send. Second: a virtual dataset whose source file is absent reads as the fill value, and HDF5 defaults that to zero, so a data file that was not copied alongside the master is indistinguishable from frames of genuine zero counts. Measured with DIALS on a four-file set with one file removed: 25 frames of pure zeros, no error and no warning. The image VDS is now filled with the error marker instead, which sits outside underload_value..saturation_value, so a reader masks those frames. Same measurement after the change: -32768 throughout, which DIALS excludes. Only the images ask for a fill value; the per-image metadata datasets keep the default. Co-Authored-By: Claude Opus 5 (1M context) --- docs/CHANGELOG.md | 2 ++ tests/HDF5WritingTest.cpp | 8 +++--- tests/JFJochReceiverLiteTest.cpp | 8 ++++-- writer/HDF5DataFile.cpp | 19 ++++++++++++++- writer/HDF5DataFile.h | 5 ++++ writer/HDF5NXmx.cpp | 42 +++++++++++++++++++++++++++++--- writer/HDF5NXmx.h | 8 ++++-- writer/HDF5Objects.cpp | 4 +++ writer/HDF5Objects.h | 1 + 9 files changed, 84 insertions(+), 13 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 3c3f1ad8..addfd3d0 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -13,6 +13,8 @@ This is an UNSTABLE release. It includes many experimental features, as well as * rugnux: the `.poni` file written by `--mode calibration` no longer negates `Rot3`, which exported a detector rotation about the beam with the wrong sign. * Tests: a tilted detector is now cross-checked against pyFAI and DIALS, in the unit tests and against a written file in CI. * rugnux: the detector geometry is also logged in XDS's convention (`ORGX`/`ORGY`, detector axis vectors, rotation axis), so it can be compared directly with an XDS refinement. +* HDF5: a data file missing next to a VDS master now reads as the error-pixel marker instead of zero counts, so those frames are masked rather than silently integrated as blank. +* The writer refuses a stream whose start message declares a different pixel format than its images carry, instead of writing a master that does not describe its own data. ### 1.0.0-rc.161 This is an UNSTABLE release. It includes many experimental features, as well as many AI generated fixes. We recommend using rc.152 for production use. diff --git a/tests/HDF5WritingTest.cpp b/tests/HDF5WritingTest.cpp index 657c4bca..2f31ea4f 100644 --- a/tests/HDF5WritingTest.cpp +++ b/tests/HDF5WritingTest.cpp @@ -478,7 +478,7 @@ TEST_CASE("HDF5Writer", "[HDF5][Full]") { x.FillMessage(start_message); FileWriter file_set(start_message); - std::vector image(x.GetPixelsNum()); + std::vector image(x.GetPixelsNum()); for (int i = 0; i < x.GetImageNum(); i++) { DataMessage message{}; @@ -527,7 +527,7 @@ TEST_CASE("HDF5Writer_Socket", "[HDF5][Full]") { FileWriter file_set(start_message); file_set.SetupFinalizedFileSocket("ipc://#1"); - std::vector image(x.GetPixelsNum()); + std::vector image(x.GetPixelsNum()); ZMQSocket s(ZMQSocketType::Sub); s.Connect("ipc://#1"); @@ -601,7 +601,7 @@ TEST_CASE("HDF5Writer_Spots", "[HDF5][Full]") { x.FillMessage(start_message); FileWriter file_set(start_message); - std::vector image(x.GetPixelsNum()); + std::vector image(x.GetPixelsNum()); for (int i = 0; i < x.GetImageNum(); i++) { DataMessage message{}; @@ -640,7 +640,7 @@ TEST_CASE("HDF5Writer_Rad_Int_Profile", "[HDF5][Full]") { start_message.az_int_q_bin_count = mapping.GetQBinCount(); FileWriter file_set(start_message); - std::vector image(x.GetPixelsNum()); + std::vector image(x.GetPixelsNum()); for (int i = 0; i < x.GetImageNum(); i++) { DataMessage message{}; diff --git a/tests/JFJochReceiverLiteTest.cpp b/tests/JFJochReceiverLiteTest.cpp index dabaa5e5..3d19fb50 100644 --- a/tests/JFJochReceiverLiteTest.cpp +++ b/tests/JFJochReceiverLiteTest.cpp @@ -11,6 +11,10 @@ #include "../image_pusher/HDF5FilePusher.h" #include "../image_puller/TestImagePuller.h" +// PixelSigned(true) below is not incidental: the frames fed in come from compression_benchmark.h5, +// which stores signed int16, while a DECTRIS experiment declares unsigned by default. The receiver +// forwards images byte-for-byte, so the start message it re-emits has to describe them - otherwise +// the master file declares a type the data files do not have, which HDF5DataFile now refuses. TEST_CASE("JFJochReceiverLite", "[JFJochReceiver]") { Logger logger("JFJochReceiverLite"); @@ -21,7 +25,7 @@ TEST_CASE("JFJochReceiverLite", "[JFJochReceiver]") { DiffractionExperiment experiment(DetDECTRIS(2068, 2164, "Test", {})); experiment.ImagesPerTrigger(5).NumTriggers(1).UseInternalPacketGenerator(true).ImagesPerFile(2) - .FilePrefix("crystal_test_lite").JungfrauConvPhotonCnt(false).SetFileWriterFormat( + .FilePrefix("crystal_test_lite").JungfrauConvPhotonCnt(false).PixelSigned(true).SetFileWriterFormat( FileWriterFormat::NXmxVDS).OverwriteExistingFiles(true) .DetectorDistance_mm(75).BeamY_pxl(1136).BeamX_pxl(1090).IncidentEnergy_keV(12.4) .SetUnitCell(UnitCell{.a = 36.9, .b = 78.95, .c = 78.95, .alpha =90, .beta = 90, .gamma = 90}); @@ -108,7 +112,7 @@ TEST_CASE("JFJochReceiverLite_Cancel", "[JFJochReceiver]") { DiffractionExperiment experiment(DetDECTRIS(2068, 2164, "Test", {})); experiment.ImagesPerTrigger(5).NumTriggers(1).UseInternalPacketGenerator(true).ImagesPerFile(2) - .FilePrefix("crystal_test_lite").JungfrauConvPhotonCnt(false).SetFileWriterFormat( + .FilePrefix("crystal_test_lite").JungfrauConvPhotonCnt(false).PixelSigned(true).SetFileWriterFormat( FileWriterFormat::NXmxVDS).OverwriteExistingFiles(true) .DetectorDistance_mm(75).BeamY_pxl(1136).BeamX_pxl(1090).IncidentEnergy_keV(12.4) .SetUnitCell(UnitCell{.a = 36.9, .b = 78.95, .c = 78.95, .alpha =90, .beta = 90, .gamma = 90}); diff --git a/writer/HDF5DataFile.cpp b/writer/HDF5DataFile.cpp index 88fc38ed..79e03f70 100644 --- a/writer/HDF5DataFile.cpp +++ b/writer/HDF5DataFile.cpp @@ -22,7 +22,9 @@ HDF5DataFile::HDF5DataFile(const StartMessage &msg, uint64_t file_number, const std::string &filename) : filename(filename), file_number(file_number), -write_images(msg.write_images.value_or(true)) { +write_images(msg.write_images.value_or(true)), +declared_bit_depth(msg.bit_depth_image), +declared_pixel_signed(msg.pixel_signed) { if (msg.overwrite.has_value()) overwrite = msg.overwrite.value(); @@ -135,6 +137,21 @@ void HDF5DataFile::CreateFile(const DataMessage& msg, std::shared_ptr if (write_images) { HDF5Dcpl dcpl; + // The only point where the two independent statements of the pixel format meet: the images + // carry their own type (a CBOR tag per image, which is what the data files are written with) + // while the master is typed from the START message. Nothing else compares them, so a stream + // whose header contradicts its images produced data files of one type under a master + // declaring another - and with NXmxVDS, HDF5 then converts silently on every read. + if (msg.image.GetByteDepth() * 8 != declared_bit_depth + || msg.image.IsSigned() != declared_pixel_signed) + throw JFJochException(JFJochExceptionCategory::InputParameterInvalid, + "Image is " + std::to_string(msg.image.GetByteDepth() * 8) + + "-bit " + (msg.image.IsSigned() ? "signed" : "unsigned") + + ", but the start message declares " + + std::to_string(declared_bit_depth) + "-bit " + + (declared_pixel_signed ? "signed" : "unsigned") + + " - the master file would not describe the data it links to"); + HDF5DataType data_type(msg.image.GetMode()); xpixel = msg.image.GetWidth(); diff --git a/writer/HDF5DataFile.h b/writer/HDF5DataFile.h index 0979f99f..db905f7a 100644 --- a/writer/HDF5DataFile.h +++ b/writer/HDF5DataFile.h @@ -25,6 +25,11 @@ class HDF5DataFile { const uint64_t file_number; const bool write_images; + // Pixel format the START message declared. The data files are typed from the images themselves + // and the master from the start message, so these two have to agree - see CreateFile. + const uint64_t declared_bit_depth; + const bool declared_pixel_signed; + std::string tmp_filename; std::shared_ptr data_file = nullptr; diff --git a/writer/HDF5NXmx.cpp b/writer/HDF5NXmx.cpp index 8f0261d8..3c877a71 100644 --- a/writer/HDF5NXmx.cpp +++ b/writer/HDF5NXmx.cpp @@ -119,7 +119,8 @@ void NXmx::LinkToData_VDS(const StartMessage &start, const EndMessage &end) { auto data_dataset = VDS(start, "/entry/data/data", {total_images, height, width}, - HDF5DataType(start.bit_depth_image / 8, start.pixel_signed)); + HDF5DataType(start.bit_depth_image / 8, start.pixel_signed), + start.error_value); data_dataset->Attr("image_nr_low", (int32_t) 1) .Attr("image_nr_high",(int32_t) total_images); @@ -281,18 +282,44 @@ void NXmx::LinkToReflections_VDS(const StartMessage &start, const EndMessage &en } } +namespace { + void SetFillValue(HDF5Dcpl &dcpl, const HDF5DataType &data_type, + const std::optional &fill_value) { + if (!fill_value.has_value()) + return; + const int64_t value = fill_value.value(); + if (data_type.IsSigned()) { + switch (data_type.GetElemSize()) { + case 1: dcpl.SetFillValue8(static_cast(value)); break; + case 2: dcpl.SetFillValue16(static_cast(value)); break; + case 4: dcpl.SetFillValue32(static_cast(value)); break; + default: break; + } + } else { + switch (data_type.GetElemSize()) { + case 1: dcpl.SetFillValueU8(static_cast(value)); break; + case 2: dcpl.SetFillValueU16(static_cast(value)); break; + case 4: dcpl.SetFillValueU32(static_cast(value)); break; + default: break; + } + } + } +} + std::unique_ptr NXmx::VDS(const StartMessage &start, const std::string &name, const std::vector &dim, - const HDF5DataType &data_type) { - return VDS(start, name, name, dim, data_type); + const HDF5DataType &data_type, + const std::optional &fill_value) { + return VDS(start, name, name, dim, data_type, fill_value); } std::unique_ptr NXmx::VDS(const StartMessage &start, const std::string &name_src, const std::string &name_dest, const std::vector &dim, - const HDF5DataType &data_type) { + const HDF5DataType &data_type, + const std::optional &fill_value) { if (dim.empty() || dim.size() > 3) throw JFJochException(JFJochExceptionCategory::InputParameterInvalid, "Dimension must be in range 1-3"); @@ -312,6 +339,13 @@ std::unique_ptr NXmx::VDS(const StartMessage &start, if (dim.size() == 3) dcpl.SetChunking({1, dim[1], dim[2]}); + // Where a virtual dataset has no source file to read from, HDF5 hands back the fill value, and + // that defaults to zero - so a data file that was not copied alongside the master reads as + // frames of zero counts, with no error and no warning. Fill with the error marker instead: it is + // outside underload_value..saturation_value, so a reader masks those frames rather than + // integrating them. + SetFillValue(dcpl, data_type, fill_value); + for (hsize_t file_id = 0; file_id < file_count; file_id++) { hsize_t images_in_file = images_per_file; if (file_id == file_count - 1) diff --git a/writer/HDF5NXmx.h b/writer/HDF5NXmx.h index f79d09d6..a669d973 100644 --- a/writer/HDF5NXmx.h +++ b/writer/HDF5NXmx.h @@ -25,16 +25,20 @@ class NXmx { void LinkToData_ProcessingVDS(const StartMessage &start, const EndMessage &end); void LinkToReflections_VDS(const StartMessage &start, const EndMessage &end); + // fill_value, when given, is what the dataset reads as where no source file is available - see + // the note in HDF5NXmx.cpp. Left unset for everything but the images. std::unique_ptr VDS(const StartMessage &start, const std::string& name, const std::vector &dim, - const HDF5DataType &data_type); + const HDF5DataType &data_type, + const std::optional &fill_value = {}); std::unique_ptr VDS(const StartMessage &start, const std::string& name_src, const std::string& name_dest, const std::vector &dim, - const HDF5DataType &data_type); + const HDF5DataType &data_type, + const std::optional &fill_value = {}); void Detector(const StartMessage &start); void Detector(const StartMessage &start, const EndMessage &end); diff --git a/writer/HDF5Objects.cpp b/writer/HDF5Objects.cpp index 66aa2fe2..ae5f5258 100644 --- a/writer/HDF5Objects.cpp +++ b/writer/HDF5Objects.cpp @@ -331,6 +331,10 @@ void HDF5Dcpl::SetFillValueU16(uint16_t val) { H5Pset_fill_value(id, H5T_NATIVE_UINT16, &val); } +void HDF5Dcpl::SetFillValueU8(uint8_t val) { + H5Pset_fill_value(id, H5T_NATIVE_UINT8, &val); +} + void HDF5Dcpl::SetVirtual(const std::string &path, const std::string &dataset, const HDF5DataSpace &src_dataspace, const HDF5DataSpace &dest_dataspace) { layout = HDF5DataSetLayout::VIRTUAL; std::string filename = ExtractFilename(path); diff --git a/writer/HDF5Objects.h b/writer/HDF5Objects.h index ba085b45..02c32d1b 100644 --- a/writer/HDF5Objects.h +++ b/writer/HDF5Objects.h @@ -106,6 +106,7 @@ public: void SetFillValue8(int8_t val); void SetFillValueU32(uint32_t val); void SetFillValueU16(uint16_t val); + void SetFillValueU8(uint8_t val); void SetChunking(const std::vector & dims); std::vector GetChunking(); CompressionAlgorithm GetCompression();