pdf-object: implement CCITTFaxDecode filter

[Velli20]

Add pub mod ccitt with a pure-Rust CCITTFaxDecode decoder supporting all three encoding modes: Group 3 1D (K=0), Group 3 2D (K>0), and Group 4 / MMR (K<0). Wire it into StreamObject::data() with a ccitt_params_for_filter() helper that reads /DecodeParms for both single-filter and multi-filter streams.

The implementation embeds Huffman tables from ITU-T T.4 Appendix A (WHITE/BLACK terminating and make-up codes, common extended make-up codes) and a T.6 mode-code decoder (Pass, Horizontal, Vertical ±0–3). CCITTFaxParams is parsed directly from PDF dictionary entries without requiring an ObjectResolver.

Includes 14 unit tests covering params defaults, bit reader, Huffman run-length decoding, EOL handling, 1D rows (all-white, all-black, mixed), 2D Group 4 rows (V0 / Horizontal modes, all-white, row limit), and row packing with both polarities.

[Copilot]

Pull request overview

Adds CCITTFaxDecode support to pdf-object by introducing a pure-Rust CCITT (Group 3/4) decoder and wiring it into stream filter decoding so CCITT-compressed streams can be decompressed via StreamObject::data().

Changes:

• Added ccitt module implementing CCITTFaxDecode decoding (Group 3 1D, Group 3 2D, Group 4/MMR) with unit tests.
• Wired Filter::CCITTFaxDecode into StreamObject::data() and added a /DecodeParms extraction helper.
• Implemented Filter::decode_ccitt_fax(.., params) as a thin wrapper over the new decoder.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File	Description
crates/pdf-object/src/stream.rs	Adds CCITTFaxDecode handling in StreamObject::data() and parses /DecodeParms per filter index.
crates/pdf-object/src/lib.rs	Exposes the new ccitt module publicly.
crates/pdf-object/src/filter.rs	Implements CCITTFaxDecode decode entrypoint and documents its behavior.
crates/pdf-object/src/ccitt.rs	New CCITT decoder implementation + parameter parsing + unit tests.

Comments suppressed due to low confidence (1)

crates/pdf-object/src/stream.rs:99

• StreamObject::data() documents that it errors on unsupported filter chains, but the default match arm prints to stdout and returns partially decoded data. This can mask failures and produce incorrect output for callers. Consider returning an ObjectError for Filter::Unsupported(_) / unknown filters instead of println! + break.

                _ => {
                    println!(
                        "Unsupported filter in data_with_remaining_filter: {:?}",
                        filter
                    );
                    break;
                }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

CCITTFaxParams::end_of_block is never consulted during decoding. For Group 3/4 streams with rows = 0 (the default), this will treat the RTC/EOFB terminator as image data and can emit a spurious final row (often all-white) or stop mid-row depending on bit patterns. Please add explicit RTC (Group 3) / EOFB (Group 4) detection when end_of_block is true and stop decoding before appending another row; also add unit tests that cover termination behavior.

[Copilot]

skip_eol(&mut reader) is called unconditionally before every row, even when params.end_of_line is false (and for Group 4, which normally has no EOLs). This can mis-synchronize decoding if a data bit sequence happens to match the EOL pattern. Consider only consuming a leading EOL when the encoding mode/params require it (e.g., end_of_line for Group 3 1D, mandatory row EOL for Group 3 2D) and avoid scanning for EOLs in Group 4 unless you are explicitly checking for EOFB.

[Copilot]

The helper assumes /DecodeParms values are always inline objects (and ignores ObjectVariant::Reference). In PDF, /DecodeParms can legally be an indirect reference, so this will silently fall back to defaults and potentially decode CCITT data with the wrong parameters (K/Columns/BlackIs1/etc.). Either resolve references here (which likely requires threading an ObjectResolver into StreamObject::data) or detect references and return a clear DecompressionError/update the doc comment to state indirect /DecodeParms is unsupported.

[Copilot]

The decode_ccitt_fax docstring says it returns DecompressionError on truncated/invalid bit patterns, but crate::ccitt::decode() currently only errors for columns == 0 and otherwise decodes best-effort (including on truncation). Please align the documentation with the actual behavior (or change the decoder to return errors as documented).

[Copilot]

damaged_rows_before_error is parsed from /DecodeParms but never used by the decoder. This is misleading (callers may assume it has an effect). Either implement damaged-row tracking/erroring per the PDF spec, or remove the field / mark it as currently ignored in the docs.

[Copilot]

PR description says the unit tests cover EOL handling, mixed 1D rows, and Group 4 2D modes (V0/Horizontal, row limit), but the tests in this file currently only cover fill_bits/find_bit basics, a few simple 1D cases, and a Group 4 empty-stream case (no 2D mode coverage). Either expand the tests to match the stated coverage (especially around 2D decoding and end-of-block handling) or update the PR description to reflect the current test scope.

[Copilot]

Pull request overview

Copilot reviewed 22 out of 23 changed files in this pull request and generated 9 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The type-level docs claim stream data is always decoded at insertion time by ObjectCollection, but StreamObject::new is public and can be constructed with raw (still-filtered) bytes outside of an ObjectCollection context. To avoid misleading API consumers, consider rewording this to describe the behavior when streams are stored in an ObjectCollection (or otherwise clarify that StreamObject itself does not enforce decoding).

[Copilot]

decode() currently hard-codes PassthroughResolver, so any /Filter value that is an indirect reference (or contains indirect references) will not be resolved and filter parsing will fail or silently degrade. Consider taking an &dyn ObjectResolver (or providing a decode_with_resolver) so callers with an ObjectCollection can correctly resolve referenced filter metadata.

Suggested change

	///
	/// # Errors
	///
	/// Returns [`FilterError`] if any filter in the chain fails or is unsupported.
	pub fn decode(stream: &StreamObject) -> Result<Cow<'_, [u8]>, FilterError> {
	let mut data: Cow<'_, [u8]> = Cow::Borrowed(&stream.data);
	let objects = PassthroughResolver;
	let filters = Filter::from_dictionary(&stream.dictionary, &objects)?;
	let Some(filters) = &filters else {
	return Ok(data);
	};
	let decode_params = parse_decode_params(&stream.dictionary, filters, &objects);
	/// For streams whose filter metadata may contain indirect references, use
	/// [`decode_with_resolver`] to provide an [`ObjectResolver`].
	///
	/// # Errors
	///
	/// Returns [`FilterError`] if any filter in the chain fails or is unsupported.
	pub fn decode(stream: &StreamObject) -> Result<Cow<'_, [u8]>, FilterError> {
	let objects = PassthroughResolver;
	decode_with_resolver(stream, &objects)
	}
	/// Decodes a [`StreamObject`] by applying its full filter chain using the
	/// provided [`ObjectResolver`] for `/Filter` and `/DecodeParms` metadata.
	///
	/// This should be used when the stream dictionary may contain indirect
	/// references that need to be resolved before parsing the filter chain.
	///
	/// # Errors
	///
	/// Returns [`FilterError`] if any filter in the chain fails or is unsupported.
	pub fn decode_with_resolver(
	stream: &StreamObject,
	objects: &dyn ObjectResolver,
	) -> Result<Cow<'_, [u8]>, FilterError> {
	let mut data: Cow<'_, [u8]> = Cow::Borrowed(&stream.data);
	let filters = Filter::from_dictionary(&stream.dictionary, objects)?;
	let Some(filters) = &filters else {
	return Ok(data);
	};
	let decode_params = parse_decode_params(&stream.dictionary, filters, objects);

[Copilot]

parse_decode_params only handles /DecodeParms when it is embedded directly as a Dictionary/Array in the stream dictionary; it does not resolve references (e.g. /DecodeParms 12 0 R or array elements that are references/null). This can cause CCITT decoding to use default parameters even when the PDF provides explicit decode parms. Consider resolving the /DecodeParms object (and array elements) via objects.resolve_object(...) before pattern matching.

Suggested change

	let params_entry = dict.get("DecodeParms");
	// Pre-extract per-index dictionaries from the `/DecodeParms` value.
	let param_dicts: Vec<Option<&Dictionary>> = match params_entry {
	Some(ObjectVariant::Dictionary(d)) => {
	// Single dict applies to all filters (common single-filter case).
	vec![Some(d); filters.len()]
	}
	Some(ObjectVariant::Array(arr)) => filters
	.iter()
	.enumerate()
	.map(|(i, _)| {
	arr.get(i).and_then(|v| {
	if let ObjectVariant::Dictionary(d) = v {
	Some(d.as_ref())
	} else {
	None
	}
	})
	})
	.collect(),
	_ => vec![None; filters.len()],
	};
	filters
	.iter()
	.zip(param_dicts.iter())
	.map(|(filter, param_dict)| match (filter, param_dict) {
	(Filter::CCITTFaxDecode, Some(d)) => {
	let p = CCITTFaxParams::from_dictionary(d, objects).unwrap_or_default();
	DecodeParms::CcittFax(p)
	}
	(Filter::CCITTFaxDecode, None) => DecodeParms::CcittFax(CCITTFaxParams::default()),
	_ => DecodeParms::None,
	let params_entry = dict
	.get("DecodeParms")
	.and_then(|value| objects.resolve_object(value).ok());
	filters
	.iter()
	.enumerate()
	.map(|(i, filter)| {
	let param_dict = match params_entry.as_deref() {
	Some(ObjectVariant::Dictionary(d)) => Some(d.as_ref()),
	Some(ObjectVariant::Array(arr)) => arr
	.get(i)
	.and_then(|value| objects.resolve_object(value).ok())
	.and_then(|resolved| match resolved.as_ref() {
	ObjectVariant::Dictionary(d) => Some(d.as_ref()),
	ObjectVariant::Null => None,
	_ => None,
	}),
	_ => None,
	};
	match (filter, param_dict) {
	(Filter::CCITTFaxDecode, Some(d)) => {
	let p = CCITTFaxParams::from_dictionary(d, objects).unwrap_or_default();
	DecodeParms::CcittFax(p)
	}
	(Filter::CCITTFaxDecode, None) => {
	DecodeParms::CcittFax(CCITTFaxParams::default())
	}
	_ => DecodeParms::None,
	}

[Copilot]

Decoding streams during ObjectCollection::insert means the persisted stream data depends on pdf_filter::filter::decode, which currently cannot resolve indirect /Filter or /DecodeParms entries (it uses a passthrough resolver). This will store incorrectly decoded bytes for PDFs that use referenced filter metadata. Consider deferring decoding until the collection is populated (second pass) or otherwise providing decode with a resolver that can follow references in the collection.

[Copilot]

CCITTFaxParams includes end_of_block, but the decoder never stores or consults it, so the implementation cannot stop cleanly on RTC/EOFB markers when Rows = 0 (the default). This can lead to extra garbage rows or spurious row-level errors after the actual image data ends. Consider wiring end_of_block into CcittDecoder and terminating decoding when the appropriate end-of-block marker is encountered.

[Copilot]

In 2D Horizontal mode, the a0 == None case sets a1 = run_len1 + 1, which is inconsistent with the Some(pos) branch (a1 = pos + run_len1) and is very likely an off-by-one that shifts the first pair of runs by one pixel when Horizontal is the first opcode in a row. Consider making the None case follow the same coordinate convention as the Some case so a1 advances by exactly run_len1 pixels from the start of the line.

Suggested change

	None => (0, run_len1.saturating_add(1)),
	None => (0, run_len1),

[Copilot]

TwoDMode::EndOfBlock is currently a no-op, so the 2D row decoder loop continues without advancing a0/a0color, which can produce incorrect output instead of terminating at the EOFB marker. If end-of-block markers are supported, consider treating this mode as an explicit termination condition for the current row/stream (e.g. return Ok(()) and let the outer loop stop).

Suggested change

	// First EOL of EOFB; bits already consumed in read_2d_mode.
	// EOFB marker consumed by read_2d_mode; terminate decoding
	// instead of continuing with stale a0/a0color state.
	return Ok(());

[Copilot]

The ASCII85 tests use expect(...) without a #[allow(clippy::expect_used)] / #[allow(clippy::unwrap_used)] guard. Since the workspace Clippy config denies expect_used/unwrap_used, cargo clippy --all-targets will likely fail. Consider adding the appropriate #[allow(...)] on the tests module (as done elsewhere in the repo) or rewriting tests to return Result and use ? instead of expect/unwrap.

[Copilot]

The CCITT test module uses expect(...) / unwrap_err() without a #[allow(clippy::expect_used)] / #[allow(clippy::unwrap_used)] guard. Given the workspace Clippy config denies these lints, this will likely fail cargo clippy --all-targets. Consider adding the allow attributes on the tests module or rewriting tests to avoid expect/unwrap.