From b5a24bad5ab8b0c2cb34ecdc74f871a7fb6aaecc Mon Sep 17 00:00:00 2001 From: GF Date: Sat, 31 Oct 2026 17:04:45 -0400 Subject: [PATCH] fix(j2k): write the last row of output buffers without trailing padding The packed 8- and 16-bit writers zipped the source rows with `out.chunks_exact_mut(stride)`. Buffer validation accepts a buffer of `strided_output_len` bytes, `stride * (height - 1) + row_bytes`, which has no padding after the last row; with a stride wider than the row, that last row is a partial chunk, `chunks_exact_mut` drops it, and the decode returned Ok with the row unwritten. This hit full-image and scaled 8-bit decodes at such a stride (row copies, RGB to RGBA and RGBA to RGB) and every 16-bit decode at one, regions included. The 8-bit region row sink and the component-plane writers already index rows and were not affected. The five loops now take their rows from one helper that chunks by stride and keeps the last, shorter chunk, sliced to the row's bytes as before. The loops are otherwise unchanged. A new decode test writes every 8- and 16-bit format of gray, RGB and RGBA images at 8 and 12 bits, full, region, scaled and region-scaled, serial and parallel, into a buffer of exactly `strided_output_len` bytes and compares it with a decode into a padded buffer: 96 of its 160 cases lost the last row before this change. --- CHANGELOG.md | 7 ++ crates/j2k/src/decode/output/u16.rs | 18 +--- crates/j2k/src/decode/output/u8.rs | 38 +++++--- crates/j2k/tests/decode.rs | 136 +++++++++++++++++++++++++++- 4 files changed, 166 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9153adcd7..cbf717479 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,13 @@ and stale roadmap entries have been removed from the public documentation set. 18000 x 12000 RGB image decodes 3 to 11% faster at 12 threads and 9 to 18% faster at 4, with about 3 MiB less peak heap. Output is unchanged, and a failure reruns whole-window Tier-1 serially for the same error. +- Decodes into a caller buffer whose stride is wider than a row and which + ends right after the last row's pixels, the length `strided_output_len` + reports and buffer validation accepts, now write that last row. Full-image + and scaled 8-bit decodes at such a stride and every 16-bit decode at one, + regions included, left it unwritten and still returned `Ok`. 8-bit region + decodes, and buffers with any bytes after the last row, were not affected. + The other rows are unchanged. ## [0.13.1] - 2026-10-29 diff --git a/crates/j2k/src/decode/output/u16.rs b/crates/j2k/src/decode/output/u16.rs index 9fd5ff7da..f76306fe5 100644 --- a/crates/j2k/src/decode/output/u16.rs +++ b/crates/j2k/src/decode/output/u16.rs @@ -6,7 +6,7 @@ use crate::backend::{ColorSpace, DecodedComponents as NativeDecodedComponents, R use crate::J2kError; use j2k_core::{PixelFormat, Unsupported}; -use super::u8::{component_sample_count, validate_component_planes}; +use super::u8::{component_sample_count, output_rows, validate_component_planes}; pub(in crate::decode) fn write_components_u16_output( components: &NativeDecodedComponents<'_>, @@ -281,12 +281,8 @@ fn write_u16_channel_rows(job: U16ChannelRows<'_, '_>) { let dst_row_bytes = width * dst_channels * 2; let alpha = opaque_alpha_u16(bytes_per_sample, bit_depth); - for (src_row, dst_row) in src - .chunks_exact(src_row_bytes) - .zip(out.chunks_exact_mut(stride)) - .take(height) - { - let dst_row = &mut dst_row[..dst_row_bytes]; + let dst_rows = output_rows(out, stride, dst_row_bytes, height); + for (src_row, dst_row) in src.chunks_exact(src_row_bytes).zip(dst_rows) { for x in 0..width { let src_pixel = &src_row[x * source_channels * bytes_per_sample..]; let dst_pixel = &mut dst_row[x * dst_channels * 2..(x + 1) * dst_channels * 2]; @@ -347,12 +343,8 @@ fn convert_or_copy_u16( let (width, height) = dims; let dst_row_bytes = width * channels * 2; let src_row_bytes = width * channels * usize::from(bytes_per_sample); - for (src_row, dst_row) in src - .chunks_exact(src_row_bytes) - .zip(out.chunks_exact_mut(stride)) - .take(height) - { - let dst_row = &mut dst_row[..dst_row_bytes]; + let dst_rows = output_rows(out, stride, dst_row_bytes, height); + for (src_row, dst_row) in src.chunks_exact(src_row_bytes).zip(dst_rows) { if bytes_per_sample == 2 { dst_row.copy_from_slice(src_row); continue; diff --git a/crates/j2k/src/decode/output/u8.rs b/crates/j2k/src/decode/output/u8.rs index 060291b10..0797d3aa0 100644 --- a/crates/j2k/src/decode/output/u8.rs +++ b/crates/j2k/src/decode/output/u8.rs @@ -173,6 +173,21 @@ pub(super) fn validate_component_planes( Ok(()) } +/// The first `row_bytes` of each of the first `height` rows of `out` at +/// `stride`. A validated buffer may end right after the last row's pixels +/// (see `j2k_core::strided_output_len`), so the last row may be shorter +/// than `stride`. +pub(super) fn output_rows( + out: &mut [u8], + stride: usize, + row_bytes: usize, + height: usize, +) -> impl Iterator { + out.chunks_mut(stride) + .take(height) + .map(move |row| &mut row[..row_bytes]) +} + fn write_component_rows_u8( plane: &j2k_native::ComponentPlane<'_>, out: &mut [u8], @@ -303,26 +318,22 @@ pub(in crate::decode) fn write_u8_output( fn copy_rows_exact(src: &[u8], out: &mut [u8], stride: usize, row_bytes: usize, height: usize) { for (src_row, dst_row) in src .chunks_exact(row_bytes) - .zip(out.chunks_exact_mut(stride)) - .take(height) + .zip(output_rows(out, stride, row_bytes, height)) { - dst_row[..row_bytes].copy_from_slice(src_row); + dst_row.copy_from_slice(src_row); } } fn add_opaque_alpha_u8(src: &[u8], out: &mut [u8], stride: usize, width: usize, height: usize) { let src_row_bytes = width * 3; let dst_row_bytes = width * 4; - for (src_row, dst_row) in src - .chunks_exact(src_row_bytes) - .zip(out.chunks_exact_mut(stride)) - .take(height) - { + let dst_rows = output_rows(out, stride, dst_row_bytes, height); + for (src_row, dst_row) in src.chunks_exact(src_row_bytes).zip(dst_rows) { for (rgb, rgba) in src_row .as_chunks::<3>() .0 .iter() - .zip(dst_row[..dst_row_bytes].as_chunks_mut::<4>().0) + .zip(dst_row.as_chunks_mut::<4>().0) { rgba[..3].copy_from_slice(rgb); rgba[3] = u8::MAX; @@ -355,16 +366,13 @@ pub(in crate::decode) fn expand_rgb8_to_rgba8(out: &mut [u8]) { fn drop_alpha_u8(src: &[u8], out: &mut [u8], stride: usize, width: usize, height: usize) { let src_row_bytes = width * 4; let dst_row_bytes = width * 3; - for (src_row, dst_row) in src - .chunks_exact(src_row_bytes) - .zip(out.chunks_exact_mut(stride)) - .take(height) - { + let dst_rows = output_rows(out, stride, dst_row_bytes, height); + for (src_row, dst_row) in src.chunks_exact(src_row_bytes).zip(dst_rows) { for (rgba, rgb) in src_row .as_chunks::<4>() .0 .iter() - .zip(dst_row[..dst_row_bytes].as_chunks_mut::<3>().0) + .zip(dst_row.as_chunks_mut::<3>().0) { rgb.copy_from_slice(&rgba[..3]); } diff --git a/crates/j2k/tests/decode.rs b/crates/j2k/tests/decode.rs index c8d315a7c..eeaac395b 100644 --- a/crates/j2k/tests/decode.rs +++ b/crates/j2k/tests/decode.rs @@ -1,11 +1,11 @@ // SPDX-License-Identifier: MIT OR Apache-2.0 use j2k::{ - encode_j2k_lossless, encode_j2k_lossless_components, DecodeSettings as FacadeDecodeSettings, - EncodeBackendPreference, J2kBlockCodingMode, J2kCodec, J2kComponentPlane, J2kContext, - J2kDecodeWarning, J2kDecoder, J2kError, J2kLosslessComponentPlane, J2kLosslessComponentSamples, - J2kLosslessEncodeOptions, J2kLosslessSamples, J2kRowDecodeOptions, J2kView, - ReversibleTransform, + encode_j2k_lossless, encode_j2k_lossless_components, CpuDecodeParallelism, + DecodeSettings as FacadeDecodeSettings, EncodeBackendPreference, J2kBlockCodingMode, J2kCodec, + J2kComponentPlane, J2kContext, J2kDecodeWarning, J2kDecoder, J2kError, + J2kLosslessComponentPlane, J2kLosslessComponentSamples, J2kLosslessEncodeOptions, + J2kLosslessSamples, J2kRowDecodeOptions, J2kView, ReversibleTransform, }; use j2k_core::{ BufferError, CodecContext, Downscale, ImageDecodeRows, PixelFormat, Rect, RowSink, @@ -1003,6 +1003,132 @@ fn decode_rejects_too_small_stride() { )); } +/// The decode entry points [`decode_to_strided_buffer`] covers. +#[derive(Debug, Clone, Copy)] +enum StridedRoute { + Full, + Region, + Scaled, + RegionScaled, +} + +/// Decode `bytes` on `route` to `fmt` at a stride three bytes past the +/// output rows, into a buffer of exactly [`j2k_core::strided_output_len`] +/// bytes when `tight`, which has no padding after its last row, and of one +/// padded row per output row otherwise. Every byte starts as `0xa5`. +fn decode_to_strided_buffer( + bytes: &[u8], + parallelism: CpuDecodeParallelism, + route: StridedRoute, + fmt: PixelFormat, + tight: bool, +) -> Vec { + let mut decoder = J2kDecoder::new(bytes).expect("decoder"); + decoder.set_cpu_decode_parallelism(parallelism); + let dims = decoder.info().dimensions; + let roi = Rect { + x: 3, + y: 5, + w: dims.0 - 7, + h: dims.1 - 6, + }; + let scale = Downscale::Half; + let output = match route { + StridedRoute::Full => Rect::full(dims), + StridedRoute::Region => roi, + StridedRoute::Scaled => Rect::full((dims.0.div_ceil(2), dims.1.div_ceil(2))), + StridedRoute::RegionScaled => roi.scaled_covering(scale), + }; + let stride = output.w as usize * fmt.bytes_per_pixel() + 3; + let len = if tight { + j2k_core::strided_output_len((output.w, output.h), stride, fmt).expect("output length") + } else { + stride * output.h as usize + }; + let mut out = vec![0xa5; len]; + let pool = &mut j2k::J2kScratchPool::new(); + let outcome = match route { + StridedRoute::Full => decoder.decode_into(&mut out, stride, fmt), + StridedRoute::Region => decoder.decode_region_into(pool, &mut out, stride, fmt, roi), + StridedRoute::Scaled => decoder.decode_scaled_into(pool, &mut out, stride, fmt, scale), + StridedRoute::RegionScaled => { + decoder.decode_region_scaled_into(pool, &mut out, stride, fmt, roi, scale) + } + } + .unwrap_or_else(|error| panic!("{route:?} {fmt:?} decode: {error}")); + assert_eq!(outcome.decoded, output); + out +} + +#[test] +fn decode_writes_the_last_row_of_a_buffer_without_trailing_padding() { + let (width, height) = (45_u32, 31_u32); + let encode_image = |components: u16, bit_depth: u8, reversible: bool| { + let bytes_per_sample = u32::from(bit_depth).div_ceil(8); + let mut pixels: Vec = (0..width * height * u32::from(components) * bytes_per_sample) + .map(|i| (i * 37 + i / 11).to_le_bytes()[0]) + .collect(); + if bit_depth > 8 { + // Keep each little-endian sample within its bit depth. + for sample in pixels.as_chunks_mut::<2>().0 { + sample[1] &= (1 << (bit_depth - 8)) - 1; + } + } + let options = EncodeOptions { + reversible, + num_decomposition_levels: 3, + ..EncodeOptions::default() + }; + encode( + &pixels, width, height, components, bit_depth, false, &options, + ) + .expect("encode") + }; + let gray8 = encode_image(1, 8, true); + let gray12 = encode_image(1, 12, false); + let rgb8 = encode_image(3, 8, false); + let rgb12 = encode_image(3, 12, true); + let rgba8_jp2 = wrap_jp2_rgba_codestream(&encode_image(4, 8, true), width, height, 8); + let gray_formats = [PixelFormat::Gray8, PixelFormat::Gray16]; + let color_formats = [ + PixelFormat::Rgb8, + PixelFormat::Rgba8, + PixelFormat::Rgb16, + PixelFormat::Rgba16, + ]; + let cases: [(&str, &[u8], &[PixelFormat]); 5] = [ + ("gray8", &gray8, &gray_formats), + ("gray12", &gray12, &gray_formats), + ("rgb8", &rgb8, &color_formats), + ("rgb12", &rgb12, &color_formats), + ("rgba8", &rgba8_jp2, &color_formats), + ]; + let mut failures = Vec::new(); + for (name, bytes, formats) in cases { + for &fmt in formats { + for route in [ + StridedRoute::Full, + StridedRoute::Region, + StridedRoute::Scaled, + StridedRoute::RegionScaled, + ] { + for parallelism in [CpuDecodeParallelism::Auto, CpuDecodeParallelism::Serial] { + let padded = decode_to_strided_buffer(bytes, parallelism, route, fmt, false); + let tight = decode_to_strided_buffer(bytes, parallelism, route, fmt, true); + if tight != padded[..tight.len()] { + failures.push(format!("{name} {fmt:?} {route:?} {parallelism:?}")); + } + } + } + } + } + assert!( + failures.is_empty(), + "a buffer that ends after the last row's pixels must hold the padded decode's \ + bytes: {failures:#?}" + ); +} + #[test] fn decode_scaled_into_matches_backend_target_resolution_decode() { let pixels: Vec = (0_u8..48).collect();