From 738b9491b2f8c00bffa002604e088bd6b367832f Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 01:23:58 -0500 Subject: [PATCH] fix(format): a codec can be registered for filter 32023 (Granular BitRound) With the pcodec feature, 32023 was a built-in entry (the legacy reader for the pcodec chunks clawhdf5 <= 2.7.0 wrote under that ID), so register_filter(32023, ...) was refused as "built in", although UnsupportedFilter(32023) names Granular BitRound as not implemented and the registry docs point to register_filter for such IDs. That entry is now shared: it claims only chunks whose filter is named "pcodec"; any other chunk with ID 32023 goes to the registered codec (or, with none registered, gets UnsupportedFilter as before), and writing 32023 uses the registered codec. Every other built-in ID still refuses registration. Test: a_codec_can_be_registered_for_granular_bitround (registers, round- trips chunks with no name and other names, still reads a legacy "pcodec" chunk with the built-in reader, and after unregistering reads nothing). It fails without the change ("filter 32023 ... is built in and cannot be re-registered"). It and the existing legacy-pcodec test share a lock, since the registry is process-wide. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/clawhdf5-format/src/filter_registry.rs | 97 +++++++++++++++++-- crates/clawhdf5-format/src/filters.rs | 6 +- 2 files changed, 94 insertions(+), 9 deletions(-) diff --git a/crates/clawhdf5-format/src/filter_registry.rs b/crates/clawhdf5-format/src/filter_registry.rs index 62e22b8..6d39613 100644 --- a/crates/clawhdf5-format/src/filter_registry.rs +++ b/crates/clawhdf5-format/src/filter_registry.rs @@ -9,7 +9,11 @@ //! [`builtin_filters`] lists them. //! * **Registered filters** (`std` only) — codecs the application supplies //! for any other ID with [`register_filter`] (a [`FilterCodec`], or just a -//! decoding closure). A registered codec cannot shadow a built-in one. +//! decoding closure). A registered codec cannot shadow a built-in one, +//! except under 32023: that ID belongs to Granular BitRound, and the +//! built-in entry there only reads the pcodec chunks clawhdf5 <= 2.7.0 +//! wrote (filter name `"pcodec"`), so a codec registered for 32023 handles +//! every other chunk with that ID, and writes. //! //! An ID in neither tier fails with [`FormatError::UnsupportedFilter`], as it //! always has. @@ -118,6 +122,19 @@ impl BuiltinFilter { pub fn can_encode(&self) -> bool { self.encode.is_some() } + + /// Whether the built-in entry only borrows its ID for some chunks, so a + /// registered codec may take the rest: the legacy pcodec entry under + /// Granular BitRound's 32023, which claims only chunks named `"pcodec"`. + fn is_shared(&self) -> bool { + self.id == crate::filter_pipeline::FILTER_PCODEC_LEGACY + } + + /// Whether this entry decodes chunks written with `filter`. + fn claims(&self, filter: &crate::filter_pipeline::FilterDescription) -> bool { + !self.is_shared() + || filter.name.as_deref() == Some(crate::filter_pipeline::FILTER_PCODEC_LEGACY_NAME) + } } /// The filters compiled into this build, in ID order. @@ -198,7 +215,11 @@ mod custom { /// A plain closure `Fn(&[u8], &FilterContext) -> Result, FormatError>` /// registers a decoder. Replaces (and returns) an earlier registration for /// the same ID. Fails with [`FormatError::FilterError`] if `id` is a built-in -/// filter of this build: those cannot be overridden. +/// filter of this build: those cannot be overridden. The exception is 32023 +/// (Granular BitRound): with the `pcodec` feature the built-in entry there +/// reads only chunks whose filter is named `"pcodec"` (clawhdf5 <= 2.7.0's +/// files); a codec registered for 32023 decodes every other chunk with that +/// ID and does all the writing. #[cfg(feature = "std")] pub fn register_filter( id: u16, @@ -207,7 +228,7 @@ pub fn register_filter( where C: FilterCodec + 'static, { - if let Some(builtin) = builtin_filter(id) { + if let Some(builtin) = builtin_filter(id).filter(|b| !b.is_shared()) { return Err(FormatError::FilterError(format!( "filter {id} ({}) is built in and cannot be re-registered", builtin.name @@ -229,11 +250,13 @@ pub fn registered(id: u16) -> Option> { custom::with_read(|r| r.get(&id).cloned()) } -/// Undo filter `ctx.filter` on `input`: the built-in decoder if there is one, -/// else a registered one, else [`FormatError::UnsupportedFilter`]. +/// Undo filter `ctx.filter` on `input`: the built-in decoder if there is one +/// that claims the chunk, else a registered one, else the built-in decoder's +/// own refusal or [`FormatError::UnsupportedFilter`]. pub(crate) fn decode(input: &[u8], ctx: &FilterContext<'_>) -> Result, FormatError> { let id = ctx.filter.filter_id; - if let Some(builtin) = builtin_filter(id) { + let builtin = builtin_filter(id); + if let Some(builtin) = builtin.filter(|b| b.claims(ctx.filter)) { return (builtin.decode)(input, ctx); } #[cfg(feature = "std")] @@ -250,12 +273,21 @@ pub(crate) fn decode(input: &[u8], ctx: &FilterContext<'_>) -> Result, F } return Ok(out); } - Err(FormatError::UnsupportedFilter(id)) + match builtin { + Some(builtin) => (builtin.decode)(input, ctx), + None => Err(FormatError::UnsupportedFilter(id)), + } } /// Apply filter `ctx.filter` to `input`. pub(crate) fn encode(input: &[u8], ctx: &FilterContext<'_>) -> Result, FormatError> { let id = ctx.filter.filter_id; + #[cfg(feature = "std")] + if builtin_filter(id).is_some_and(|b| b.is_shared()) + && let Some(codec) = registered(id) + { + return codec.encode(input, ctx); + } if let Some(builtin) = builtin_filter(id) { return match builtin.encode { Some(encode) => encode(input, ctx), @@ -270,7 +302,7 @@ pub(crate) fn encode(input: &[u8], ctx: &FilterContext<'_>) -> Result, F } #[cfg(all(test, feature = "std"))] -mod tests { +pub(crate) mod tests { use super::*; use crate::filter_pipeline::{FILTER_FLETCHER32, FILTER_SHUFFLE, FilterPipeline}; use crate::filters::{compress_chunk, decompress_chunk}; @@ -368,6 +400,55 @@ mod tests { assert!(builtin_filter(FILTER_SHUFFLE).is_some()); } + /// Serialises the tests that register or read filter 32023 (the + /// registry is process-wide). + pub(crate) static ID_32023: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + /// 32023 is Granular BitRound's ID; the `pcodec` build's built-in entry + /// there reads only clawhdf5 <= 2.7.0's pcodec chunks (named "pcodec"), + /// so a codec can be registered for the rest, and writes with it. + #[test] + fn a_codec_can_be_registered_for_granular_bitround() { + let _guard = ID_32023 + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let named = |name: Option<&str>| FilterPipeline { + version: 2, + filters: vec![FilterDescription { + filter_id: 32023, + name: name.map(Into::into), + flags: 0, + client_data: vec![7], + }], + }; + let prev = register_filter(32023, Xor).expect("32023 must be registrable"); + assert!(prev.is_none()); + let data = b"granular bitround".to_vec(); + for name in [None, Some("granular_bitround"), Some("test")] { + let pl = named(name); + let enc = compress_chunk(&data, &pl, 1).unwrap(); + assert_ne!(enc, data); + assert_eq!(decompress_chunk(&enc, &pl, data.len(), 1).unwrap(), data); + } + // clawhdf5 <= 2.7.0's pcodec chunks still go to the built-in reader. + #[cfg(feature = "pcodec")] + { + let raw: Vec = (0..64) + .flat_map(|i| (f64::from(i) * 0.5).to_le_bytes()) + .collect(); + let comp = crate::filters::pcodec_compress(&raw, 8).unwrap(); + let mut pl = named(Some("pcodec")); + pl.filters[0].client_data = vec![8]; + assert_eq!(decompress_chunk(&comp, &pl, raw.len(), 8).unwrap(), raw); + } + assert!(unregister_filter(32023)); + let pl = named(None); + assert!(matches!( + decompress_chunk(&data, &pl, data.len(), 1), + Err(FormatError::UnsupportedFilter(32023)) + )); + } + #[test] fn unsupported_filter_error_names_the_filter() { let msg = FormatError::UnsupportedFilter(32026).to_string(); diff --git a/crates/clawhdf5-format/src/filters.rs b/crates/clawhdf5-format/src/filters.rs index 99a5d8e..36f2e96 100644 --- a/crates/clawhdf5-format/src/filters.rs +++ b/crates/clawhdf5-format/src/filters.rs @@ -1446,7 +1446,7 @@ fn fletcher32_append(data: &[u8]) -> Result, FormatError> { // --------------------------------------------------------------------------- #[cfg(feature = "pcodec")] -fn pcodec_compress(data: &[u8], element_size: usize) -> Result, FormatError> { +pub(crate) fn pcodec_compress(data: &[u8], element_size: usize) -> Result, FormatError> { use pco::ChunkConfig; use pco::standalone::simple_compress; let config = ChunkConfig::default(); @@ -2758,6 +2758,10 @@ mod tests { #[cfg(feature = "pcodec")] fn pcodec_uses_private_id_and_reads_legacy_32023() { use crate::chunked_write::ChunkOptions; + #[cfg(feature = "std")] + let _guard = crate::filter_registry::tests::ID_32023 + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); let opts = ChunkOptions { pcodec: true, ..Default::default()