Skip to content

fix(encoding): single-segment frames whenever the window covers the content - #591

Merged
polaz merged 1 commit into
mainfrom
fix/#570-single-segment
Oct 17, 2026
Merged

polaz merged 1 commit into
mainfrom
fix/#570-single-segment

Conversation

@polaz

@polaz polaz commented Oct 17, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Both encoders write a single-segment frame whenever the content size is in the header and the window covers it, with or without a dictionary, as upstream decides it (ZSTD_writeFrameHeader).
  • A decoder then reserves the content rather than the power of two above it: a 140 MiB file at --long=28 declares 140 MiB instead of 256 MiB.

Changes

  • frame_header::single_segment holds the rule; the streaming encoder and the frame compressor both call it.
  • The streaming encoder drops its 512 B to 16 KiB range and its dictionary exclusion; the frame compressor no longer needs a size hinted before the frame, since its header is written after the content.
  • Frames change in the header only: every byte after it is identical, and the header is up to four bytes shorter.

Testing

Unit, CLI, ffi (header layout against libzstd from 100 bytes to 1 MiB with and without a dictionary, C round-trip) and C ABI tests, clippy over the CI feature sets, formatting, rustdoc and doc tests pass on macOS.

Closes #570

Summary by CodeRabbit

  • Bug Fixes
    • Frames that record their content size now use a single-segment layout when the content fits within the frame window, even if the input size wasn’t provided in advance.
    • Streaming and regular compression now apply consistent frame-header rules across a wider range of input sizes and dictionary use, while preserving successful decoding.

…ontent

The streaming encoder wrote a single-segment frame only for pledged
sizes of 512 bytes to 16 KiB and never with a dictionary, and the frame
compressor only when a size was hinted before the frame. Upstream's
rule (`ZSTD_writeFrameHeader`) is one condition: the content size is
written and the window covers it. Elsewhere our frames declared the
power of two above the content as their window, so a decoder reserved
more memory than the data needs: a 140 MiB file at `--long=28` asked for
256 MiB where upstream's frame asks for 140 MiB.

- One rule in `frame_header::single_segment`, read by both encoders,
  dictionary frames included (RFC 8878 3.1.1.1.2: the window is the
  content size, and the dictionary stays referenceable before it).
- The CLI's frames change in the header alone: everything after it is
  byte-identical at levels 1-19 on four fixture sizes, with and without
  a dictionary, and the header is up to four bytes shorter.

Tests check both encoders against libzstd's header layout from 100
bytes to 1 MiB with and without a dictionary, with a C round-trip; two
tests that read the window from the header now use content past it.

Closes #570
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 17, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-17T07:33:26.307553Z 89bdaa8 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: structured-world/structured-zstd/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 6d318952-b0ca-4807-a941-c3d18b0f6d92
📥 Commits

Reviewing files that changed from the base of the PR and between e9067ad and 89bdaa8.

📒 Files selected for processing: 8
  • ffi-bench/tests/frame_compressor_ffi.rs
  • zstd/src/decoding/frame_decoder/tests.rs
  • zstd/src/encoding/frame_compressor.rs
  • zstd/src/encoding/frame_compressor/tests.rs
  • zstd/src/encoding/frame_header.rs
  • zstd/src/encoding/streaming_encoder.rs
  • zstd/src/encoding/streaming_encoder/tests.rs
  • zstd/src/tests/roundtrip_integrity.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

The frame compressor and streaming encoder now use a shared rule for single-segment framing. Tests cover content sizes across multiple ranges, dictionary cases, frame decoding, and comparisons with C zstd.

Changes: Single-segment frame headers

Layer / File(s) Summary
Shared header rule and encoder integration
zstd/src/encoding/frame_header.rs, zstd/src/encoding/frame_compressor.rs, zstd/src/encoding/streaming_encoder.rs
A shared helper selects single-segment framing when the content size is present and no greater than the window. Both encoders use this rule; the frame compressor no longer requires a known source-size hint.
Header behavior validation
zstd/src/encoding/*/tests.rs, ffi-bench/tests/frame_compressor_ffi.rs, zstd/src/tests/roundtrip_integrity.rs, zstd/src/decoding/frame_decoder/tests.rs
Tests cover unhinted and pledged frames, dictionary cases, C zstd header comparisons, round-trip decoding, advertised windows, and a payload larger than the default window.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 89bda

No actionable issue remains in the supplied review evidence; the PR is mergeable after normal checks.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: using single-segment frames when the window covers the content.
Linked Issues check Passed Issue #570 requires one rule for both encoders. frame_header::single_segment returns true when a content size is present and content_size <= window_size. StreamingEncoder and FrameCompressor b…
Out of Scope Changes check Passed The changes stay within issue #570. The production changes centralize the frame-header rule and apply it to both encoders. The added and updated tests verify header layout, dictionary behavior, unhint…
Docstring Coverage Passed Docstring coverage is 93.75% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 8 files.
Full details: Linked Issues check

Explanation

Issue #570 requires one rule for both encoders. frame_header::single_segment returns true when a content size is present and content_size &lt;= window_size. StreamingEncoder and FrameCompressor both use this helper. The rule includes dictionary frames and has no obsolete size range limit. The tests compare both encoders with C zstd for sizes from 100 bytes through 1 MiB, with and without a dictionary. They also verify C decoding and local decoding. The implementation applies the same rule to larger sizes, including sizes above 128 MiB.

Full details: Out of Scope Changes check

Explanation

The changes stay within issue #570. The production changes centralize the frame-header rule and apply it to both encoders. The added and updated tests verify header layout, dictionary behavior, unhinted content, decoding, and unchanged frame content. These tests support the linked objective and do not add unrelated behavior.

✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@polaz
polaz enabled auto-merge (squash) October 17, 2026 07:41
@codecov

codecov Bot commented Oct 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@polaz
polaz merged commit 1faeb2f into main Oct 17, 2026
29 checks passed
@polaz
polaz deleted the fix/#570-single-segment branch October 17, 2026 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(encoding): single-segment frames whenever the window covers the content

1 participant