fix(client): handle unaligned ImageBytes response buffers#19
Open
ivonnyssen wants to merge 3 commits into
Open
fix(client): handle unaligned ImageBytes response buffers#19ivonnyssen wants to merge 3 commits into
ivonnyssen wants to merge 3 commits into
Conversation
`ASCOMResult<ImageArray>::from_reqwest` reads the 44-byte `ImageBytesMetadata` header via `bytemuck::try_from_bytes` and the pixel payload via `bytemuck::try_cast_slice::<u8, T>`. Both APIs require the source `&[u8]` to be aligned to the target type's alignment (4 for `i32`, 2 for `i16`/`u16`). The `&[u8]` here is a slice of the HTTP response body — typically a `bytes::Bytes` chunk that reqwest assembled from one or more chunked reads — and its start pointer is not guaranteed to satisfy that alignment. When it doesn't, both calls return `PodCastError::TargetAlignmentGreaterAndInputNotAligned` and the client panics with `ASCOM error UNSPECIFIED: TargetAlignmentGreaterAndInputNotAligned` — intermittently, depending on the host allocator's runtime behaviour. Fix: use the unaligned-friendly equivalents. - Metadata: `bytemuck::pod_read_unaligned` reads the full struct via `ptr::read_unaligned`, no source-alignment requirement. - Pixel data: `bytemuck::pod_collect_to_vec` allocates a fresh `Vec<T>` (which IS aligned) and `memcpy`s the payload in. Adds an upfront length-modulo check to keep the existing slop-error semantics. Adds a regression test that constructs an `imagebytes` payload at each of the four leading-pad offsets (0..4) within a `Vec<u8>`, guaranteeing at least three of them place the body slice on a non-4-aligned start. The test fails on the previous code path (`try_from_bytes` rejects the unaligned metadata immediately) and passes after the fix. Closes RReverser#18. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes client-side parsing of application/imagebytes responses when the underlying HTTP body buffer is not properly aligned for i16/i32/u16 reads (issue #18), avoiding intermittent bytemuck alignment failures on some allocators/runners.
Changes:
- Switch metadata parsing from
try_from_bytestobytemuck::pod_read_unaligned. - Replace raw pixel casting with an unaligned-safe path using
bytemuck::pod_collect_to_vec(plus a length check). - Add a regression test that exercises multiple leading offsets to ensure unaligned slice starts are handled.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- cast_raw_data: keep the zero-copy fast path. Try `bytemuck::try_cast_slice::<u8, T>` first; only fall back to the copying `pod_collect_to_vec` when it returns `TargetAlignmentGreaterAndInputNotAligned`. Slop / length-mismatch errors propagate verbatim. Common (aligned) case keeps the pre-PR memory and copy profile; unaligned case still works. - Drop redundant `if !data.len().is_multiple_of(elem_size)` length check now that the slop error path is reached via try_cast_slice directly. - Metadata fix doc comment: refer to `align_of::<ImageBytesMetadata>()` instead of the hard-coded `align_of::<i32>() == 4` so it stays correct if the struct's layout grows a wider field. - Test helper doc comment: clarify the alignment guarantee — the `Vec<u8>` base allocation is high-aligned by the system allocator, so looping leading_pad over 0..4 covers all four mod-4 rotations and guarantees at least three unaligned cases. - Test fn signature: split into a `Result<()>`-returning helper (`check_one_offset`) using `eyre::ensure!` for validation and a panicking `#[test]` shell that surfaces the failing leading_pad in the panic message. Avoids clippy::panic_in_result_fn while keeping diagnostic context. - Replace `data.len() % elem_size != 0` with the `is_multiple_of` form (clippy::manual_is_multiple_of), superseded by the fast-path refactor above. - Clean up `as i32` casts in the test helper to `i32::from(...)` for the enum reprs and `i32::try_from(...)?` for the size_of/len conversions (clippy::cast_lossless / cast_possible_truncation). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the slow path's two-memcpy bytes→Vec<T>→Vec<i32> staging with a single chunked pass that decodes each `size_of::<T>()`-byte chunk via a new `widen_from_le_chunk` trait method on `AsTransmissionElementType` and writes straight into the output `Vec<i32>`. Savings: - One full pixel-buffer memcpy on unaligned input. - Memory ceiling on the slow path drops from `data.len() + sizeof_output` to just `sizeof_output`. For a 32 MP `i32` image that's ~128 MB peak instead of ~256 MB. - Drops the `bytemuck::NoUninit` bound on the local function (was only required by `pod_collect_to_vec`). The fast (aligned) path is unchanged from the previous commit — `try_cast_slice` reinterprets in place, and we iterate-and-widen into the output. So the common case keeps the pre-PR memory and copy profile; the slow path is now strictly an improvement. `from_le_bytes` on `i32` lowers to the same instruction as a normal aligned `i32` load on little-endian targets, so the per-element loop in the slow path is essentially identical in cost to a bulk memcpy + widening loop. For the smaller types (`i16` / `u16` / `u8`) the upconversion was element-wise anyway, so the staging buffer was pure overhead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ivonnyssen
added a commit
to ivonnyssen/ascom-alpaca-rs
that referenced
this pull request
May 10, 2026
…y ceiling Squashes upstream RReverser#19: - `cast_raw_data` and the `ImageBytesMetadata` parse no longer assume the HTTP response body buffer is `align_of::<T>()`-aligned. The fast (aligned) path is unchanged — `try_cast_slice` reinterprets in place — and the unaligned path decodes each `size_of::<T>()`-byte chunk via a new `AsTransmissionElementType::widen_from_le_chunk` trait method, going straight from response bytes into `Vec<i32>` in one pass. - Metadata parse switches to `bytemuck::pod_read_unaligned`, which uses `ptr::read_unaligned` and works for any pointer. - Slow-path peak memory drops from `data.len() + sizeof_output` to just `sizeof_output`. For a 32 MP `i32` capture that's ~128 MB instead of ~256 MB. - Adds a regression test that loops `leading_pad` over `0..4` so at least three of the four iterations exercise an unaligned source slice. Fixes the intermittent `ASCOM error UNSPECIFIED: TargetAlignmentGreaterAndInputNotAligned` in ascom-alpaca clients that depended on the host allocator's runtime alignment behaviour (observed on macOS under bazel). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ivonnyssen
added a commit
to ivonnyssen/ascom-alpaca-rs
that referenced
this pull request
May 12, 2026
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.
Summary
bytemuck::try_from_bytes::<ImageBytesMetadata>(line 114) withbytemuck::pod_read_unaligned.bytemuck::try_cast_slice::<u8, T>incast_raw_datawith a length-checkedbytemuck::pod_collect_to_vecthat allocates a fresh alignedVec<T>andmemcpys the payload in.Vec<u8>so at least three iterations place the body slice on a non-4-byte-aligned start.Both call sites assumed the HTTP response body buffer (
bytes::Bytesslice from reqwest's chunked read) is 4-byte aligned. The OS allocator usually delivers that, but not always — particularly on macOS under bazel's test runner — and clients see intermittentASCOM error UNSPECIFIED: TargetAlignmentGreaterAndInputNotAlignedpanics.Fixes #18.
Test plan
cargo test --all-features -p ascom-alpaca --lib api::camera::image_array::client::tests— new regression test passes.cargo check --all-features— clean.cast_raw_datagains abytemuck::NoUninitbound, satisfied by all four impl types).🤖 Generated with Claude Code