Add packed 24-bit sample types (S24_3LE and friends) - #203
Open
mdwn wants to merge 6 commits into
Open
Conversation
The existing I24 and U24 store a 24-bit value in an i32, so they are four bytes wide with an alignment of four. That is the S24_LE layout. A great many interfaces instead carry 24-bit audio as exactly three bytes -- ALSA calls it S24_3LE -- and some, such as the Behringer WING, support nothing else. Those buffers cannot be reinterpreted as a slice of I24: the stride is wrong and the alignment is not satisfied. These four types have that layout instead. Each is repr(transparent) over [u8; 3], so size_of is exactly 3 and align_of is exactly 1, and every one of the 2^24 byte patterns is a valid value. A buffer of packed 24-bit PCM can therefore be reinterpreted as a slice of them without a copy. They are storage types and deliberately have no arithmetic of their own: operating on three bytes in place would mean a decode and an encode per operation hidden behind an operator. Ord is written by hand rather than derived, because deriving it would compare [u8; 3] lexicographically -- which orders the least significant byte first for the little-endian types and reads the sign byte as unsigned for the signed ones. Having no spare byte to put it in, an out-of-range value wraps modulo 2^24, which is what impl From<i32> for I24 already does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every ordered pair of the eighteen sample types now converts, so the four packed types are as usable as any other. The conversions are defined in terms of the padded equivalents, which is not a detour: packing is a pure re-arrangement of the same 24 bits, so there is no shorter path to any other width. For any in-range value a packed conversion and its padded counterpart agree exactly. They differ only for input that is already out of range, and only from a floating point source, because every integer conversion into 24 bits is a shift that fits by construction: i8 << 16 and i16 << 8 widen into range, and i32 >> 8, i64 >> 40 and I48 >> 24 land exactly on -8_388_608..=8_388_607. A float outside the documented -1.0 <= v < 1.0 range can overflow. I24 and U24 absorb the overshoot in the spare byte of their container and hold a numerically out-of-range value; a packed type has no spare byte, so it wraps. That tolerance is a property of their storage rather than a deliberate policy -- see RustAudio#73, which reports the same overflow against the primitive types and notes in passing that I24 and I48 happen not to be affected. No attempt is made here to settle the wider question of overflow behaviour raised by RustAudio#39 and RustAudio#73; the packed types wrap, matching impl From<i32> for I24, and the conv module documents it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed and Float are taken from the padded equivalents -- I24 and f32 for the signed types, i32 and f32 for the unsigned ones -- so add_amp and mul_amp unpack, operate and repack rather than working three bytes at a time. The packed types are deliberately not SignedSample: arithmetic goes through Signed, which keeps the unpacking explicit and lets a caller unpack once and operate many times. dasp_frame gains the corresponding one-channel Frame impls. Both crates go to 0.11.1, and dasp_frame's requirement on dasp_sample is raised from "0.11" to "0.11.1". dasp_frame now names types::I24LE3, and a caret requirement of "0.11" permits the published 0.11.0, which does not have it. Inside the workspace the path dependency masks that, so no test run or CI job can catch it; a downstream crate resolving 0.11.0 would fail to compile dasp_frame. Happy to drop the version changes if they are better made at release time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tests/packed.rs covers what is specific to these types being packed: the memory layout, byte order and sign extension, the ordering that a derived Ord would get wrong, the wrapping on out-of-range input, and the zero-copy reinterpretation the types exist for. Two of its tests are exhaustive, over all 2^24 values and all 2^24 byte patterns. Three weaknesses in the surrounding tests are fixed, each confirmed by mutating the source and watching the suite fail: - The conversion matrix test wrote its target types out a second time and compared the pair count against types * types, which only proved the two lists were the same length. Swapping one type for another in just one list passed, leaving that type untested and its replacement tested twice. The list is now given once and expanded along both dimensions. - Sample::Signed was unasserted for I24BE3 and U24LE3: changing I24BE3's from I24 to i32 compiled and passed the whole workspace suite, despite making add_amp read its argument on a scale 256x off. All four are now pinned, and that mutation is a compile error. - conv_cmp! built expected values with new_unchecked, which wraps for the packed types, so an out-of-range expectation quietly became the value the assertion then agreed with. It now goes through new, and a wrong expectation fails loudly. All existing expectations were already in range. The ordering test asserts that each case would fail under a derived Ord, so a case that proves nothing cannot be added by accident. U24BE3 has no such case and can have none: for a big-endian unsigned value, lexicographic byte order and numeric order are the same relation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mdwn
marked this pull request as ready for review
August 15, 2026 20:45
A signed sample type that is not `SignedSample` is an anomaly: every other signed sample type in the crate is one, and the trait's own description -- "types whose equilibrium is at 0" -- describes `I24LE3` accurately. The original reasoning for keeping them out was that arithmetic on three unaligned bytes would be slow, and measurement did not support it. The release-mode wrap is in fact free: truncating a value to three bytes *is* reduction modulo 2^24, landing on the same value the padded types reach through an explicit `wrap_overflow_once`, so the packed types skip a compare and a branch the padded ones pay. So `Sample::Signed` for `I24LE3` and `I24BE3` is now `Self`, both are added to `impl_signed_sample!`, and all four types gain the arithmetic operators `new_sample_type!` gives the padded types: `Add`, `Sub`, `Mul`, `Div` and `Rem`, with the same contract of panicking on overflow in debug and wrapping in release, `Div` and `Rem` unchecked as they are there too. `U24LE3` and `U24BE3` remain outside `SignedSample`, as no unsigned type is in it. The bitwise and shift operators are deliberately not mirrored. They are bit manipulation rather than arithmetic and do not carry over: `!x` on a padded type inverts the 32 bits of its container, including the byte that holds no sample data. Two of them are also marked `// TODO: Needs review` where they are defined. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`dasp_graph`'s dependencies on `dasp_frame`, `dasp_ring_buffer`,
`dasp_signal` and `dasp_slice` carry no `path`, unlike every other member
of this workspace, so it builds against the published crates instead of
the ones next to it. That pulls a second copy of most of the workspace
into the graph, including `dasp_window 0.11.1`, which is newer than the
0.11.0 in this repository.
It went unnoticed because the duplicated crates had the same version
numbers as the local ones, so `cargo doc` wrote both copies of each into
the same `target/doc/<crate>` directory with identical content. Bumping
`dasp_sample` and `dasp_frame` to 0.11.1 in this branch made the contents
differ, and the two rustdoc invocations then raced over the same output
paths -- `cargo doc --all --all-features` failed with
error: ".../target/doc/dasp_frame/struct.N23.html": No such file or
directory (os error 2)
on one CI run and passed on another, which is what a race looks like.
With the paths added, each crate is documented exactly once and no
published dasp crate is downloaded at all. This is independent of the
packed sample types and can be split into its own PR if preferred; it is
here because the version bump is what exposed it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Author
|
Sorry to tag you directly here @roderickvd -- are you who I would ask to review this? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Full disclosure: Claude Opus 5 was used in the implementation of this.
Adds
I24LE3,I24BE3,U24LE3andU24BE3— 24-bit samples packed into exactly three bytes, in a fixed byte order. These are the formats ALSA callsS24_3LE,S24_3BE,U24_3LEandU24_3BE.Why
I24andU24store their value in ani32, so they are four bytes wide with an alignment of four — theS24_LElayout. A lot of hardware instead carries 24-bit audio as exactly three bytes, and some supports nothing else. A buffer in that format cannot be reinterpreted as a slice ofI24: the stride is wrong and the alignment is not satisfied, so the only options today are a conversion pass or letting ALSA'spluglayer convert in software.The concrete case that prompted this: a Behringer WING offers
S24_3LEat 48 channels and nothing else. cpal, which builds ondasp_sample, reports zero usable configurations for it.What the types are
Each is
#[repr(transparent)]over[u8; 3], sosize_ofis exactly 3,align_ofis exactly 1, and all 2^24 byte patterns are valid values. That is what makes the zero-copy reinterpretation sound.They carry the same arithmetic as their padded equivalents —
Add,Sub,Mul,Div,Rem, andNegon the signed pair — with the same contract: panic on overflow in debug, wrap in release,DivandRemunchecked. The release wrap costs nothing extra, because truncating a value to three bytes is reduction modulo 2^24, where the padded types need an explicit compare.I24LE3andI24BE3are thereforeSignedSample, like every other signed sample type;U24LE3andU24BE3are not, like every other unsigned one.The bitwise and shift operators are not mirrored. They are bit manipulation rather than arithmetic and do not carry over:
!xon a padded type inverts the 32 bits of its container, including the byte holding no sample data.Ordis hand-written rather than derived. A derivedOrdcompares[u8; 3]lexicographically, which orders the least significant byte first for the little-endian types and reads the sign byte as unsigned for the signed ones.PartialEqis still derived, since the encoding is bijective.They otherwise mirror their padded equivalents: the same
Fromimpls,Negon the signed pair only, andI48/U48accept them as sources. Between that and the arithmetic above, a call site can be retargeted fromI24toI24LE3by changing the type name, for everything but the bitwise and shift operators.Overflow: not litigated here
Every ordered pair of the eighteen sample types converts. For any in-range value a packed conversion and its padded counterpart agree exactly.
They differ only for input that is already out of range, and only from a floating point source — every integer conversion into 24 bits is a shift that fits by construction, so no integer source can overflow. A float outside the documented
-1.0 <= v < 1.0range can:I24andU24absorb the overshoot in the spare byte of their container, and a packed type has no spare byte, so it wraps modulo 2^24.That tolerance looks accidental rather than designed. #73 reports the same overflow against the primitive types and notes in passing that
I24andI48happen not to be affected — which is exactly this spare-byte behaviour, not a decision anyone made.I considered saturating instead, and measured it: ~+0.45 ns/sample on aarch64, about +32% of that conversion, though only 0.1% of a core for 48 channels at 48 kHz. I have not done it. There is no saturating behaviour anywhere in the crate today, #39 has been open since 2016, and a feature PR is the wrong place to decide policy that would apply to every type. So these types wrap, which is what
impl From<i32> for I24already does, and theconvmodule documents the behaviour and points at #39 and #73. Happy to revisit if you would rather they saturate.Version bump
dasp_sampleanddasp_framego to 0.11.1, anddasp_frame's requirement ondasp_sampleis raised from"0.11"to"0.11.1".dasp_framenow namestypes::I24LE3, and a caret requirement of"0.11"permits the published 0.11.0, which does not have it. Inside the workspace the path dependency masks this, so no test run or CI job catches it, but a downstream crate resolving 0.11.0 would fail to compiledasp_frame. Please drop these changes if versioning is better done at release time — the requirement bump is the part that matters.A
dasp_graphbuild fix, dragged in by the abovedasp_graph's dependencies ondasp_frame,dasp_ring_buffer,dasp_signalanddasp_slicecarry nopath, unlike every other member of this workspace, so it builds against the published crates rather than the ones sitting next to it. That pulls a second copy of most of the workspace into the dependency graph, includingdasp_window 0.11.1, which is newer than the 0.11.0 in this repository.It was invisible while the duplicates carried the same version numbers as the local crates:
cargo docwrote both copies of each into the sametarget/doc/<crate>directory with identical content. The bump to 0.11.1 above makes the contents differ, and the two rustdoc invocations then race over the same output paths —cargo doc --all --all-featuresfailed withon one CI run of this branch and passed on another. Adding the four
pathentries means each crate is documented exactly once and no published dasp crate is downloaded at all.This is independent of the sample types and is happy to become its own PR; it is here because the version bump is what exposed it.
Testing
tests/packed.rscovers layout, byte order, sign extension, ordering, wrapping, and the zero-copy reinterpretation. Two tests are exhaustive, over all 2^24 values and all 2^24 byte patterns.types * types, which only proved the lists were the same length.Sample::Signedis now pinned for all four types. It was unasserted forI24BE3andU24LE3: changingI24BE3's fromI24toi32compiled and passed the entire suite.conv_cmp!builds expected values withnewrather thannew_unchecked, so an out-of-range expectation fails instead of quietly becoming the value the assertion agrees with. All existing expectations were already in range.no_stdverified by building forriscv32imafc-unknown-none-elf, not just--no-default-features.cargo fmt --check,cargo test --alland the per-crate--no-default-featuresjobs all pass, andcargo clippyreports no new warnings.Hardware
Verified end-to-end against a Behringer WING over ALSA, using a patched cpal:
hw:device reports no supported configurations. After, it reportsi24packedat 48 channels / 48 kHz in both directions, matchingaplay --dump-hw-params.S24_3LEbytes assembled from the format spec in Python, played throughsnd-aloop, and decoded correctly by these types — includingFF 00 00→ 255, which a big-endian reading would render as −65536. A loopback round-trip alone would not prove this, since a consistent byte-order error cancels out.Each commit builds on its own.