From 77c6d0f4ec997e13c08d14f6170dd16223cc02fd Mon Sep 17 00:00:00 2001 From: leonarski_f Date: Sat, 22 Aug 2026 18:15:44 +0200 Subject: [PATCH] Report the stored image depth in bit_depth_readout, and write underload_value NXmx has no field for the depth of the stored image - only bit_depth_readout, "how many bits the electronics record per pixel". The two diverge exactly when summation is used: the readout keeps the detector's native width while the summed image must be wider to hold the sum. Every NXmx reader nonetheless takes bit_depth_readout as the width of the stored pixel. dxtbx ignores the non-standard bit_depth_image entirely for a generic NXmx file, derives its masking markers from bit_depth_readout, and raises "Unsupported integer dtype uint32" for a 32-bit image when the field is absent. Reporting the electronic value there would mislead precisely where it differs. So report the image depth in both fields, and drop the machinery that existed to carry the electronic one for a DECTRIS detector: the SIMPLON read, the DetectorSetup setter, and the receiver-side propagation of a key that the DECTRIS stream2 protocol does not even define. JUNGFRAU and PSI EIGER keep their readout depth, which the FPGA acquisition genuinely needs. Also write NXmx underload_value, the lowest valid value. Without it a reader takes the trusted minimum to be -0x7FFFFFFF, so the error-pixel marker sits inside the trusted range and is consumed as an intensity. Measured with DIALS 3.27 on a written file: trusted_range goes from (-2147483647, 32766) to (-32767, 32766), so the INT16_MIN gap pixels are now masked. Third fix in the same area: JFJochReceiverLite::Configure took the image width from the incoming stream but not the sign, while the image itself is forwarded byte-for-byte. A detector sending int32 was re-declared uint32, and the VDS master was typed unsigned over signed data files. Take pixel_signed from the stream too - it and the width are both carried by the one image_dtype key. Co-Authored-By: Claude Opus 5 (1M context) --- common/DetectorSetup.cpp | 23 ++++-------------- common/DetectorSetup.h | 1 - common/DiffractionExperiment.cpp | 12 +++++++++- common/JFJochMessages.h | 1 + detector_control/DectrisSimplonClient.cpp | 4 +++- docs/CBOR.md | 5 ++-- docs/CHANGELOG.md | 3 +++ docs/HDF5.md | 26 +++++++++++++++++---- frame_serialize/CBORStream2Deserializer.cpp | 2 ++ frame_serialize/CBORStream2Serializer.cpp | 2 ++ receiver/JFJochReceiverLite.cpp | 7 +++--- tests/DetectorSetupTest.cpp | 17 ++++---------- tools/jfjoch_simplon_test.cpp | 2 -- writer/HDF5NXmx.cpp | 2 ++ 14 files changed, 61 insertions(+), 46 deletions(-) diff --git a/common/DetectorSetup.cpp b/common/DetectorSetup.cpp index f6b2b3cc..c00ef508 100644 --- a/common/DetectorSetup.cpp +++ b/common/DetectorSetup.cpp @@ -74,8 +74,10 @@ DetectorSetup::DetectorSetup(std::shared_ptr in_geometry, break; case DetectorType::DECTRIS: high_voltage = 0; - bit_depth_readout = 16; - bit_depth_image = 16; + // bit_depth_readout stays unset: a DECTRIS detector's electronic readout depth is a + // constant we never need, and what is reported downstream is the image depth (see + // DiffractionExperiment::FillMessage). + bit_depth_image = 16; // placeholder, replaced from the stream2 image_dtype when armed read_out_time = std::chrono::microseconds(0); if (!det_modules_hostname.empty() && ( det_modules_hostname.size() != 1)) throw JFJochException(JFJochExceptionCategory::InputParameterInvalid, @@ -324,23 +326,6 @@ DetectorSetup &DetectorSetup::BitDepthImage(int64_t input) { } } -DetectorSetup &DetectorSetup::BitDepthReadout(int64_t input) { - if (GetDetectorType() != DetectorType::DECTRIS) - throw JFJochException(JFJochExceptionCategory::InputParameterInvalid, - "Bit depth readout can be only changed for DECTRIS detector"); - switch (input) { - case 8: - case 12: - case 16: - case 32: - bit_depth_readout = input; - return *this; - default: - throw JFJochException(JFJochExceptionCategory::InputParameterInvalid, - "Bit depth readout can be only 8, 12, 16 or 32"); - } -} - std::optional DetectorSetup::GetBitDepthReadout() const { return bit_depth_readout; } diff --git a/common/DetectorSetup.h b/common/DetectorSetup.h index 4a2472b2..e0c84e54 100644 --- a/common/DetectorSetup.h +++ b/common/DetectorSetup.h @@ -76,7 +76,6 @@ public: DetectorSetup& ModuleSync(bool input); DetectorSetup& ReadOutTime(std::chrono::nanoseconds input); DetectorSetup& Geometry(const DetectorGeometryFixed& input); - DetectorSetup& BitDepthReadout(int64_t input); DetectorSetup& BitDepthImage(int64_t input); DetectorSetup& MinFrameTime(std::chrono::nanoseconds input); DetectorSetup& MinCountTime(std::chrono::nanoseconds input); diff --git a/common/DiffractionExperiment.cpp b/common/DiffractionExperiment.cpp index 1015bd0b..30e37503 100644 --- a/common/DiffractionExperiment.cpp +++ b/common/DiffractionExperiment.cpp @@ -669,7 +669,17 @@ void DiffractionExperiment::FillMessage(StartMessage &message) const { message.sensor_material = detector.GetSensorMaterial(); message.sensor_thickness = detector.GetSensorThickness_um() * 1e-6f; message.bit_depth_image = GetByteDepthImage() * 8; - message.bit_depth_readout = GetBitDepthReadout(); + // Both fields carry the depth of the STORED image, deliberately. NXmx has no field for it - + // only bit_depth_readout, defined as the depth of the detector electronics - so every NXmx + // reader takes bit_depth_readout as the width of the stored pixel (DIALS uses it to decide + // which values are masking markers, and cannot read a 32-bit image without it). The electronic + // readout depth is a constant of the detector and of no use to a data consumer, so reporting it + // here would only mislead: it differs from the image depth exactly when summation widens the + // image, which is when getting it wrong does damage. + message.bit_depth_readout = message.bit_depth_image; + // Lowest valid value, so a reader can tell the error marker from data. Unsigned images use 0; + // signed ones reserve INTx_MIN as the marker (GetUnderflow), so the lowest valid is one above. + message.underload_value = IsPixelSigned() ? GetUnderflow() + 1 : 0; message.indexing_algorithm = GetIndexingAlgorithm(); message.images_per_trigger = dataset.GetImageNumPerTrigger(); diff --git a/common/JFJochMessages.h b/common/JFJochMessages.h index 9a8989e1..ffbeecc6 100644 --- a/common/JFJochMessages.h +++ b/common/JFJochMessages.h @@ -230,6 +230,7 @@ struct StartMessage { int64_t saturation_value; std::optional error_value; + std::optional underload_value; // NXmx: lowest valid value float pixel_size_x; float pixel_size_y; diff --git a/detector_control/DectrisSimplonClient.cpp b/detector_control/DectrisSimplonClient.cpp index 1170fc51..0da7b7c9 100644 --- a/detector_control/DectrisSimplonClient.cpp +++ b/detector_control/DectrisSimplonClient.cpp @@ -198,7 +198,9 @@ void DectrisSimplonClient::ReadDetectorConfig(DetectorSetup &setup) { setup.SensorMaterial(GetDetCfg( "sensor_material").val); setup.SensorThickness_um(std::round(GetDetCfg( "sensor_thickness").val.get() * 1e6f)); - setup.BitDepthReadout(GetDetCfg("bit_depth_readout").val.get()); + // Only the image depth is read: it sizes the image buffer before the first frame arrives, and + // is replaced from the stream2 image_dtype when the detector arms. The electronic readout depth + // is deliberately not read - nothing consumes it. setup.BitDepthImage(GetDetCfg("bit_depth_image").val.get()); setup.MinFrameTime(float2time(GetDetCfg("frame_time").min.get())); setup.MinCountTime(float2time(GetDetCfg("count_time").min.get())); diff --git a/docs/CBOR.md b/docs/CBOR.md index 93f6ae24..fa411393 100644 --- a/docs/CBOR.md +++ b/docs/CBOR.md @@ -69,7 +69,7 @@ There are minor differences at the moment: | storage_cell_number | uint64 (optional) | Number of storage cells used by JUNGFRAU | | | storage_cell_delay | Rational | Delay of storage cells in JUNGFRAU | | | threshold_energy | Map(string -> float) | Per-channel threshold energy \[eV\] (map of channel name to value) | | -| image_dtype | string | Pixel bit type (e.g. uint16) | X | +| image_dtype | string | Pixel type of the image data: `uint8`, `uint16`, `uint32` (DECTRIS), plus `int8`, `int16`, `int32` as a Jungfraujoch extension. Sole wire encoding of both the bit depth and the sign, and must agree with the per-image typed-array tag | X | | unit_cell | object (optional) | Unit cell of the system: a, b, c \[angstrom\] and alpha, beta, gamma \[degree\] | | | az_int_q_bin_count | uint64 | Number of azimuthal integration bins in the radial direction | | | az_int_phi_bin_count | uint64 | Number of azimuthal integration bins in the phi angle direction | | @@ -104,7 +104,8 @@ There are minor differences at the moment: | - experiment_group | string | ID of instrument user, e.g., p-group (SLS/SwissFEL) or proposal number | | | - jfjoch_release | string | Jungfraujoch release number | | | - socket_number | uint64 | Number of ZeroMQ socket (on `jfjoch_broker` side) used for transmission | | -| - bit_depth_readout | uint64 | Bit depth of the detector readout | | +| - bit_depth_readout | uint64 | Bit depth of the **stored image** (see note below), copied to NXmx `bit_depth_readout` | | +| - underload_value | int64 | Lowest valid value; copied to NXmx `underload_value`. `0` for an unsigned image, `INTx_MIN + 1` for a signed one | | | - writer_notification_zmq_addr | string | ZeroMQ address to inform `jfjoch_broker` about writers that finished operation | | | - xfel_pulse_id | uint64 | Pulse IDs are recorded for images | | | - ring_current_mA | float | Ring current at the start of the measurement | | diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 56260509..68e417c1 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -5,6 +5,9 @@ This is an UNSTABLE release. It includes many experimental features, as well as * CUDA builds made with a CUDA 12 toolkit now also contain device code for Volta, so the RHEL 8 packages and the portable Linux `.tgz` run on a V100; CUDA 13 dropped Volta, so the RHEL 9, Ubuntu and Windows artefacts remain Turing and newer. * Documentation: the supported GPU generations and the minimum NVIDIA driver version of every released artefact. +* `bit_depth_readout` now reports the bit depth of the stored image rather than the detector's electronic readout depth, which is what NXmx readers expect - DIALS derives its masking markers from that field and cannot read a 32-bit image without it. +* NXmx `underload_value` (lowest valid value) is now written, so a reader masks the error-pixel marker instead of treating it as an intensity. +* A DECTRIS detector sending signed images is no longer declared unsigned in the outgoing stream and in HDF5. ### 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/docs/HDF5.md b/docs/HDF5.md index fad40276..20941532 100644 --- a/docs/HDF5.md +++ b/docs/HDF5.md @@ -160,13 +160,31 @@ File-level HDF5 attributes `file_name`, `file_time`, `HDF5_Version` are also set | `threshold_energy` | NXmx | eV (EIGER; written only for a single channel) | | `x_pixel_size`, `y_pixel_size` | base | m | | `serial_number` | base | | -| `bit_depth_readout` | NXmx | | -| `saturation_value` | NXmx | | +| `bit_depth_readout` | NXmx | bit depth of the **stored image**, not of the detector electronics - see below | +| `saturation_value` | NXmx | highest valid value | +| `underload_value` | NXmx | lowest valid value: `0` for an unsigned image, `INTx_MIN + 1` for a signed one | | `flatfield_applied` | NXmx | | | `pixel_mask`, `pixel_mask_applied` | NXmx | `pixel_mask` is `[y, x]`, hard-linked from `detectorSpecific/pixel_mask` | | `countrate_correction_applied` | NXmx | | | `number_of_cycles` | base | frame-summation factor | +#### Why `bit_depth_readout` is the image depth + +NXmx defines only `bit_depth_readout`, "how many bits the electronics record per pixel", and has no +field for the depth of the image actually stored. The two differ whenever summation is used: the +readout stays at the detector's native width while the summed image must be wider to hold the sum. + +Jungfraujoch writes the **stored image depth** into `bit_depth_readout` (and the identical value +into the non-standard `bit_depth_image`). The electronic value is a constant of the detector and +tells a data consumer nothing, whereas readers do use `bit_depth_readout` as the width of the +stored pixel — DIALS, for instance, derives its masking markers from it and cannot read a 32-bit +image without it. Writing the electronic value there would therefore mislead exactly in the case +where the two differ. + +Note that `bit_depth_readout` gives the width only. The **sign** is carried solely by the HDF5 +element type of `/entry/data/data` (and, on the wire, by `image_dtype`); there is no NXmx field for +it. + ### `/entry/instrument/detector/transformations` (NXtransformations) The NXtransformations *mechanism* (the `depends_on` chain, `transformation_type`, `vector`, @@ -425,8 +443,8 @@ group for compatibility with existing tooling: |-------|-------|---------| | `detector_distance` | m | duplicate of `distance` (Dectris/Neggia compatibility) | | `detector_number` | | detector identifier (Dectris convention) | -| `error_value` | | masked/error pixel sentinel (NXmx standard would be `underload_value`) | -| `bit_depth_image` | | stored image bit depth (NXmx standard is `bit_depth_readout`) | +| `error_value` | | masked/error pixel sentinel: `UINTx_MAX` unsigned, `INTx_MIN` signed (NXmx has no equivalent; `underload_value` is written too, and is one above it) | +| `bit_depth_image` | | stored image bit depth; equal to `bit_depth_readout` (DECTRIS convention, not NXmx) | | `acquisition_type` | | always `triggered` (Dectris convention) | | `jungfrau_conversion_applied` | | JUNGFRAU photon/keV conversion applied | | `jungfrau_conversion_factor` | eV | conversion factor | diff --git a/frame_serialize/CBORStream2Deserializer.cpp b/frame_serialize/CBORStream2Deserializer.cpp index 6ba60651..d62460a7 100644 --- a/frame_serialize/CBORStream2Deserializer.cpp +++ b/frame_serialize/CBORStream2Deserializer.cpp @@ -1128,6 +1128,8 @@ namespace { message.writer_notification_zmq_addr = j["writer_notification_zmq_addr"]; if (j.contains("bit_depth_readout")) message.bit_depth_readout = j["bit_depth_readout"]; + if (j.contains("underload_value")) + message.underload_value = j["underload_value"]; if (j.contains("summation_mode")) message.summation_mode = j["summation_mode"]; if (j.contains("overwrite")) diff --git a/frame_serialize/CBORStream2Serializer.cpp b/frame_serialize/CBORStream2Serializer.cpp index 21056641..e2b8d549 100644 --- a/frame_serialize/CBORStream2Serializer.cpp +++ b/frame_serialize/CBORStream2Serializer.cpp @@ -569,6 +569,8 @@ inline void CBOR_ENC_START_USER_DATA(CborEncoder& encoder, const char* key, j["socket_number"] = message.socket_number.value(); if (message.bit_depth_readout) j["bit_depth_readout"] = message.bit_depth_readout.value(); + if (message.underload_value) + j["underload_value"] = message.underload_value.value(); if (!message.writer_notification_zmq_addr.empty()) j["writer_notification_zmq_addr"] = message.writer_notification_zmq_addr; if (message.summation_mode.has_value()) diff --git a/receiver/JFJochReceiverLite.cpp b/receiver/JFJochReceiverLite.cpp index 9b5dde20..06b8ab7a 100644 --- a/receiver/JFJochReceiverLite.cpp +++ b/receiver/JFJochReceiverLite.cpp @@ -205,10 +205,11 @@ void JFJochReceiverLite::Configure(const StartMessage &msg) { experiment.Detector().SensorMaterial(msg.sensor_material); experiment.Detector().SensorThickness_um(msg.sensor_thickness * 1e6); experiment.Detector().SaturationLimit(msg.saturation_value); + // Images are forwarded byte-for-byte, so the stream's own image_dtype - not anything configured + // locally - decides both the width and the sign the outgoing metadata must declare. Taking only + // the width used to leave a signed stream declared unsigned in NXmx. experiment.Detector().BitDepthImage(msg.bit_depth_image); - - if (msg.bit_depth_readout) - experiment.Detector().BitDepthReadout(msg.bit_depth_readout.value()); + experiment.PixelSigned(msg.pixel_signed); } void JFJochReceiverLite::MaskThread(uint32_t id) { diff --git a/tests/DetectorSetupTest.cpp b/tests/DetectorSetupTest.cpp index 5e81b8c5..9f1d316e 100644 --- a/tests/DetectorSetupTest.cpp +++ b/tests/DetectorSetupTest.cpp @@ -32,26 +32,17 @@ TEST_CASE("DetectorSetup_MismatchInGeometry") { } TEST_CASE("DetectorSetup_ReadoutDepth") { + // The electronic readout depth is only carried where the acquisition needs it: JUNGFRAU is + // fixed at 16, PSI EIGER takes it from the detector settings, and a DECTRIS detector does not + // carry one at all - what it reports downstream is the image depth. auto setup = DetDECTRIS(123,123, "zzz", "a"); - REQUIRE_NOTHROW(setup.BitDepthReadout(16)); - REQUIRE(setup.GetBitDepthReadout() == 16); - REQUIRE_NOTHROW(setup.BitDepthReadout(12)); - REQUIRE(setup.GetBitDepthReadout() == 12); - REQUIRE_NOTHROW(setup.BitDepthReadout(32)); - REQUIRE(setup.GetBitDepthReadout() == 32); - REQUIRE_NOTHROW(setup.BitDepthReadout(8)); - REQUIRE(setup.GetBitDepthReadout() == 8); - REQUIRE_THROWS(setup.BitDepthReadout(0)); - REQUIRE_THROWS(setup.BitDepthReadout(15)); - REQUIRE_THROWS(setup.BitDepthReadout(-1)); + REQUIRE(!setup.GetBitDepthReadout()); auto setup2 = DetJF(1); REQUIRE(setup2.GetBitDepthReadout() == 16); - REQUIRE_THROWS(setup2.BitDepthReadout(32)); auto setup3 = DetEIGER(1); REQUIRE(!setup3.GetBitDepthReadout()); - REQUIRE_THROWS(setup3.BitDepthReadout(32)); } TEST_CASE("DetectorSetup_ImageDepth") { diff --git a/tools/jfjoch_simplon_test.cpp b/tools/jfjoch_simplon_test.cpp index 1da1a3a4..cb917e0c 100644 --- a/tools/jfjoch_simplon_test.cpp +++ b/tools/jfjoch_simplon_test.cpp @@ -50,8 +50,6 @@ int main(int argc, char **argv) { setup.GetGeometry().GetHeight(true)); WriteTIFFToFile("det_mask.tiff", image); - if (setup.GetBitDepthReadout()) - logger.Info("Bit depth readout {:8d}", setup.GetBitDepthReadout().value()); if (setup.GetBitDepthImage()) logger.Info("Bit depth image {:8d}", setup.GetBitDepthImage().value()); diff --git a/writer/HDF5NXmx.cpp b/writer/HDF5NXmx.cpp index 5ab2a398..0bcb9e5f 100644 --- a/writer/HDF5NXmx.cpp +++ b/writer/HDF5NXmx.cpp @@ -365,6 +365,8 @@ void NXmx::Detector(const StartMessage &start) { if (start.bit_depth_readout) SaveScalar(group, "bit_depth_readout", start.bit_depth_readout.value()); SaveScalar(group, "saturation_value", start.saturation_value); + if (start.underload_value) + SaveScalar(group, "underload_value", start.underload_value.value()); if (start.error_value) SaveScalar(group, "error_value", start.error_value.value()); // this is not NXmx SaveScalar(group, "flatfield_applied", start.flatfield_enabled);