fix: correct libaec constants, HDF5→libaec option mapping, and VDS serialization

Critical fixes from whole-branch code review:
- libaec-sys: fix flag constants to match <libaec.h> exactly
  (PREPROCESS=8, MSB=4, RESTRICTED=16; drop non-existent AEC_ALLOW_K13)
  and add aec_buffer_encode FFI declaration
- filters_szip: fix cd index for bits_per_sample (cd[2] per H5Z_SZIP_PARM_BPP,
  not cd[4]); fix option-mask mapping (NN=0x20, MSB unconditional); add two
  real encode→decode roundtrip tests (no-NN and NN) that exercise libaec end-to-end
- file_writer: fix serialize_vds_mappings to delegate to data_layout_write
  (eliminates the buggy duplicate that always emitted version=1 even for
  external-file mappings); retains trailing Jenkins checksum

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

crates/clawhdf5-format/src/file_writer.rs

@@ -805,30 +805,14 @@ fn serialize_attribute_info(fh_addr: u64, btree_name_addr: u64) -> Vec<u8> {
 
 // ---- VDS helpers ----
 
-/// Serialize VDS mappings into the on-disk format stored in the global heap.
+/// Serialize VDS mappings for storage in a global heap object.
 ///
-/// Layout (version 1, same-file references):
+/// Delegates to `data_layout_write::serialize_vds_mappings` (the canonical
-/// ```text
+/// implementation with full version/external-file handling), then appends a
-/// version(1) · nused(8, LE) · entry[nused]
+/// trailing 4-byte Jenkins lookup3 checksum that parsers skip after consuming
-/// ```
+/// all `nused` entries.
-/// Each entry: same-file marker(0x04) · source_dataset\0 · source_sel · virtual_sel
 pub(crate) fn serialize_vds_mappings(mappings: &[VdsMapping]) -> Vec<u8> {
-    let mut buf = Vec::new();
+    let mut buf = crate::data_layout_write::serialize_vds_mappings(mappings, 8);
-    buf.push(1u8); // block version 1 (same-file marker for "." source_file)
-    buf.extend_from_slice(&(mappings.len() as u64).to_le_bytes()); // nused
-    for m in mappings {
-        if m.source_file == "." {
-            buf.push(0x04); // same-file marker
-        } else {
-            buf.extend_from_slice(m.source_file.as_bytes());
-            buf.push(0); // null terminator
-        }
-        buf.extend_from_slice(m.source_dataset.as_bytes());
-        buf.push(0); // null terminator
-        buf.extend_from_slice(&m.source_selection);
-        buf.extend_from_slice(&m.virtual_selection);
-    }
-    // Checksum (4 bytes at end; parsers skip it after reaching nused entries)
     let cksum = crate::checksum::jenkins_lookup3(&buf);
     buf.extend_from_slice(&cksum.to_le_bytes());
     buf

crates/clawhdf5-format/src/filters_szip.rs

@@ -6,11 +6,11 @@ use crate::error::FormatError;
 
 /// Decompress SZIP-compressed data using libaec.
 ///
-/// `cd` is the HDF5 filter client data:
+/// `cd` is the HDF5 SZIP filter client data (matches `H5Z_SZIP_PARM_*` indices):
-///   cd[0] = options mask (NN flag = 0x04, LSB = 0x40, allow_k13 = 0x100)
+///   cd[0] = options mask  (`H5_SZIP_NN_OPTION_MASK = 0x20` enables NN preprocessing)
-///   cd[1] = pixels per block (8, 10, 16, or 32)
+///   cd[1] = pixels per block (H5Z_SZIP_PARM_PPB; 8, 10, 16, or 32)
-///   cd[2] = pixels per scan line
+///   cd[2] = bits per sample  (H5Z_SZIP_PARM_BPP; element bit width)
-///   cd[4] = bits per sample (element bit width)
+///   cd[3] = pixels per scan line (H5Z_SZIP_PARM_PPS; informational only)
 pub(crate) fn szip_decompress(
     _data: &[u8],
     _cd: &[u32],
@@ -30,14 +30,14 @@ pub(crate) fn szip_decompress(
 
 #[cfg(feature = "szip")]
 fn szip_decode_impl(data: &[u8], cd: &[u32], chunk_size: usize) -> Result<Vec<u8>, FormatError> {
-    if cd.len() < 5 {
+    if cd.len() < 3 {
         return Err(FormatError::ChunkedReadError(
             "szip: missing client data".into(),
         ));
     }
     let options = cd[0];
     let pixels_per_block = cd[1];
-    let bits_per_sample = cd[4];
+    let bits_per_sample = cd[2]; // H5Z_SZIP_PARM_BPP
     if bits_per_sample == 0 || bits_per_sample > 32 {
         return Err(FormatError::ChunkedReadError(
             "szip: invalid bits per sample".into(),
@@ -54,20 +54,13 @@ fn szip_decode_impl(data: &[u8], cd: &[u32], chunk_size: usize) -> Result<Vec<u8
         ));
     }
 
-    // Map HDF5 options mask to libaec flags.
+    // Map HDF5 option mask to libaec flags.
-    // bit 2 (0x04): NN (nearest-neighbor) preprocessing
+    // HDF5 always stores SZIP data in MSB order, so AEC_DATA_MSB is unconditional.
-    // bit 6 (0x40): LSB order; absence means MSB
+    // H5_SZIP_NN_OPTION_MASK (0x20): NN differential preprocessing.
-    // bit 8 (0x100): allow k=13
+    let mut flags: u32 = libaec_sys::AEC_DATA_MSB;
-    let mut flags: u32 = 0;
+    if options & 0x20 != 0 {
-    if options & 0x04 != 0 {
         flags |= libaec_sys::AEC_DATA_PREPROCESS;
     }
-    if options & 0x40 == 0 {
-        flags |= libaec_sys::AEC_DATA_MSB;
-    }
-    if options & 0x100 != 0 {
-        flags |= libaec_sys::AEC_ALLOW_K13;
-    }
 
     let mut out = vec![0u8; chunk_size];
     let mut strm = libaec_sys::AecStream::zeroed();
@@ -99,7 +92,7 @@ mod tests {
     fn szip_disabled_returns_unsupported() {
         #[cfg(not(feature = "szip"))]
         {
-            let result = szip_decompress(&[], &[4, 8, 10, 0, 8], 64);
+            let result = szip_decompress(&[], &[0, 8, 8, 1024], 64);
             assert!(
                 matches!(result, Err(FormatError::UnsupportedFilter(4))),
                 "expected UnsupportedFilter(4), got {result:?}"
@@ -108,8 +101,72 @@ mod tests {
         #[cfg(feature = "szip")]
         {
             // When szip IS enabled, an empty buffer should error but not panic.
-            let result = szip_decompress(&[], &[4, 8, 10, 0, 8], 64);
+            let result = szip_decompress(&[], &[0, 8, 8, 1024], 64);
             assert!(result.is_err(), "empty buffer must not succeed");
         }
     }
+
+    /// Round-trip test: encode with libaec then decode through szip_decompress.
+    ///
+    /// Uses 1024 samples (rsi=128 × block_size=8) so the block count is exact.
+    #[cfg(feature = "szip")]
+    #[test]
+    fn roundtrip_u8_msb_no_nn() {
+        use libaec_sys::{AecStream, AEC_DATA_MSB};
+
+        let original: Vec<u8> = (0..1024u32).map(|i| (i % 256) as u8).collect();
+
+        // Encode with libaec directly (no NN, MSB — mirrors what HDF5 always writes).
+        let mut encoded = vec![0u8; original.len() * 2];
+        let mut enc = AecStream::zeroed();
+        enc.next_in = original.as_ptr();
+        enc.avail_in = original.len();
+        enc.next_out = encoded.as_mut_ptr();
+        enc.avail_out = encoded.len();
+        enc.bits_per_sample = 8;
+        enc.block_size = 8;
+        enc.rsi = 128;
+        enc.flags = AEC_DATA_MSB;
+        let rc = unsafe { libaec_sys::aec_buffer_encode(&mut enc) };
+        assert_eq!(rc, 0, "aec_buffer_encode failed: {rc}");
+        let enc_len = encoded.len() - enc.avail_out;
+        encoded.truncate(enc_len);
+
+        // Decode through our public interface.
+        // cd[0]=0 (no NN bit 0x20), cd[1]=8 (ppb), cd[2]=8 (bpp), cd[3]=1024 (pps).
+        let cd = [0u32, 8, 8, 1024];
+        let decoded = szip_decompress(&encoded, &cd, original.len())
+            .expect("szip_decompress must succeed on valid libaec output");
+        assert_eq!(decoded, original, "round-trip must reproduce original data");
+    }
+
+    /// Same round-trip but with NN preprocessing enabled (H5_SZIP_NN_OPTION_MASK = 0x20).
+    #[cfg(feature = "szip")]
+    #[test]
+    fn roundtrip_u8_msb_with_nn() {
+        use libaec_sys::{AecStream, AEC_DATA_MSB, AEC_DATA_PREPROCESS};
+
+        let original: Vec<u8> = (0..1024u32).map(|i| (i % 256) as u8).collect();
+
+        let mut encoded = vec![0u8; original.len() * 2];
+        let mut enc = AecStream::zeroed();
+        enc.next_in = original.as_ptr();
+        enc.avail_in = original.len();
+        enc.next_out = encoded.as_mut_ptr();
+        enc.avail_out = encoded.len();
+        enc.bits_per_sample = 8;
+        enc.block_size = 8;
+        enc.rsi = 128;
+        enc.flags = AEC_DATA_MSB | AEC_DATA_PREPROCESS;
+        let rc = unsafe { libaec_sys::aec_buffer_encode(&mut enc) };
+        assert_eq!(rc, 0, "aec_buffer_encode with NN failed: {rc}");
+        let enc_len = encoded.len() - enc.avail_out;
+        encoded.truncate(enc_len);
+
+        // cd[0] = 0x20 (H5_SZIP_NN_OPTION_MASK) → decoder must set AEC_DATA_PREPROCESS.
+        let cd = [0x20u32, 8, 8, 1024];
+        let decoded = szip_decompress(&encoded, &cd, original.len())
+            .expect("szip_decompress with NN must succeed");
+        assert_eq!(decoded, original, "NN round-trip must reproduce original data");
+    }
 }

crates/libaec-sys/src/lib.rs

@@ -1,15 +1,16 @@
 //! Raw FFI bindings to libaec (Adaptive Entropy Coding library).
 //!
-//! Exposes the `aec_buffer_decode` one-shot convenience function via
+//! Exposes `aec_buffer_encode` and `aec_buffer_decode` via the `AecStream`
-//! the `AecStream` control structure, matching the libaec C API.
+//! control structure, matching the libaec C API defined in `<libaec.h>`.
 
 use std::os::raw::c_void;
 
-// AEC flag constants matching aec.h
+// AEC flag constants — values match <libaec.h> exactly.
-pub const AEC_DATA_PREPROCESS: u32 = 1; // NN preprocessing
+pub const AEC_DATA_SIGNED: u32 = 1;
-pub const AEC_DATA_MSB: u32 = 2; // big-endian sample order
+pub const AEC_DATA_3BYTE: u32 = 2;
-pub const AEC_RESTRICTED: u32 = 4; // restricted coding set
+pub const AEC_DATA_MSB: u32 = 4;
-pub const AEC_ALLOW_K13: u32 = 8; // allow k=13 option
+pub const AEC_DATA_PREPROCESS: u32 = 8;
+pub const AEC_RESTRICTED: u32 = 16;
 
 /// Mirror of `struct aec_stream` from `<libaec.h>`.
 ///
@@ -50,6 +51,13 @@ impl AecStream {
 }
 
 unsafe extern "C" {
+    /// One-shot compression.  Returns `AEC_OK` (0) on success.
+    ///
+    /// # Safety
+    /// `strm.next_in` must be valid for `strm.avail_in` bytes;
+    /// `strm.next_out` must be valid for `strm.avail_out` bytes.
+    pub fn aec_buffer_encode(strm: *mut AecStream) -> i32;
+
     /// One-shot decompression.  Returns `AEC_OK` (0) on success.
     ///
     /// # Safety
@@ -63,9 +71,12 @@ mod tests {
     use super::*;
 
     #[test]
-    fn constants_are_correct() {
+    fn constants_match_libaec_header() {
-        assert_eq!(AEC_DATA_PREPROCESS, 1);
+        assert_eq!(AEC_DATA_SIGNED, 1);
-        assert_eq!(AEC_DATA_MSB, 2);
+        assert_eq!(AEC_DATA_3BYTE, 2);
+        assert_eq!(AEC_DATA_MSB, 4);
+        assert_eq!(AEC_DATA_PREPROCESS, 8);
+        assert_eq!(AEC_RESTRICTED, 16);
     }
 
     #[test]