RRD footers 1: framing #12044
Open
RRD footers 1: framing #12044
+753
−119
Conversation
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
teh-cmc
added
📉 performance
|
Web viewer built successfully.
View image diff on kitdiff. Note: This comment is updated whenever you push a commit. |
teh-cmc
force-pushed
the
cmc/rrd_footers_1_framing
branch
from
7929d56 to
440537c
Compare
teh-cmc
force-pushed
the
cmc/rrd_footers_1_framing
branch
from
440537c to
80ff0a6
Compare
This was referenced Dec 2, 2025
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces the foundational framing infrastructure for RRD stream footers, which will enable richer metadata and improved stream navigation. It establishes the protocol-level types and state machine changes needed to support footers without yet populating them with content.
Key changes include:
- Addition of
RrdFootermessage types at both transport (protobuf) and application levels - Introduction of
StreamFooterframe for locating and validating RRD footers - Enhanced decoder state machine to handle the new footer frames
- Updated encoder to emit footers with basic state tracking infrastructure
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| rerun_py/rerun_sdk/rerun/recording_stream.py | Added TODO comment about flushing behavior on garbage collection |
| crates/store/re_server/src/store/layer.rs | Moved use sha2::Digest import to local scope |
| crates/store/re_protos/src/v1alpha1/rerun.log_msg.v1alpha1.rs | Generated protobuf code for new RrdFooter message type |
| crates/store/re_protos/proto/rerun/v1alpha1/log_msg.proto | Defined RrdFooter protobuf message with documentation |
| crates/store/re_log_encoding/tests/footer_roundtrip.rs | Added test validating footer encoding/decoding roundtrip |
| crates/store/re_log_encoding/src/transport_to_app.rs | Renamed conversion functions and added RrdFooter transport/application conversions |
| crates/store/re_log_encoding/src/rrd/mod.rs | Reorganized module structure and exports for footer support |
| crates/store/re_log_encoding/src/rrd/log_msg.rs | Implemented Encodable/Decodable for ArrowMsg and RrdFooter |
| crates/store/re_log_encoding/src/rrd/frames.rs | Added StreamFooter frame type with encoding/decoding logic |
| crates/store/re_log_encoding/src/rrd/footer/mod.rs | Created footer module structure |
| crates/store/re_log_encoding/src/rrd/footer/instances.rs | Defined application-level RrdFooter type |
| crates/store/re_log_encoding/src/rrd/errors.rs | Added frame decoding and CRC error variants |
| crates/store/re_log_encoding/src/rrd/encoder.rs | Updated encoder to track state and emit footers |
| crates/store/re_log_encoding/src/rrd/decoder/stream.rs | Updated async decoder state check for new naming |
| crates/store/re_log_encoding/src/rrd/decoder/state_machine.rs | Refactored decoder state machine to handle footer frames |
| crates/store/re_log_encoding/src/rrd/decoder/iterator.rs | Updated iterator state check for new naming |
| crates/store/re_log_encoding/Cargo.toml | Added dependencies for itertools and xxhash-rust |
| Cargo.toml | Added xxhash-rust workspace dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
teh-cmc
commented
Dec 2, 2025
teh-cmc
commented
Dec 2, 2025
Co-authored-by: Copilot <[email protected]>
teh-cmc
commented
Dec 2, 2025
teh-cmc
commented
Dec 2, 2025
Comment on lines
+312
to
+313
| // * Backward compatibility: the payload is empty (i.e. `header.len == 0`) because the | ||
| // `End` message was written by a legacy encoder that predates the introduction of footers. |
Member
Author
There was a problem hiding this comment.
That's bit of a spoiler: obviously as of this PR, the footer is still always empty.
Member
|
@rerun-bot full-check |
|
Started a full build: https://github.com/rerun-io/rerun/actions/runs/19868769655 |
emilk
approved these changes
Dec 2, 2025
Comment on lines
+229
to
+230
| byte_offset_excluding_header, | ||
| byte_size_excluding_header, |
Member
There was a problem hiding this comment.
Nit: could use re_span::Span for this if you like
| "fragile-send-sync-non-atomic-wasm", | ||
| ] } | ||
| xshell = "0.2.7" | ||
| xxhash-rust = { version = "0.8", features = ["xxh32"] } |
Member
There was a problem hiding this comment.
curious: why this particular hash?
Comment on lines
+245
to
+254
| pub rrd_footer_byte_offset_from_start_excluding_header: u64, | ||
|
|
||
| /// The size in bytes of the serialized [`RrdFooter`] payload, excluding the message header. | ||
| /// | ||
| /// This is guaranteed to be the same value as the `len` found in the associated message | ||
| /// header, but duplicating it here makes it possible for decoders to get everything they | ||
| /// need using a single IO. | ||
| /// | ||
| /// [`RrdFooter`]: [crate::RrdFooter] | ||
| pub rrd_footer_byte_size_excluding_header: u64, |
Member
There was a problem hiding this comment.
Again, could be using re_span::Span just to reduce the number of variables the reader has to have in their head
| // TODO(cmc): It shouldn't be the job of the StreamFooter to carry checksums for a specific | ||
| // message's payload. All frames should have identifiers and CRCs for both themselves and their | ||
| // payloads, in which case this CRC would belong in the MessageHeader. | ||
| pub crc_excluding_header: u32, |
Member
There was a problem hiding this comment.
Also should document what checksum algo is being used here
emilk
added a commit
that referenced
this pull request
Dec 2, 2025
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.
LLM summary:
To which I would add the following:
This introduces all the framing infrastructure so that RRD streams are now capable of carrying footers in all circumstances (including pipes).
Specifically, this introduces a couple new types:
RrdFooter, a(n empty) protobuf message that will from now on act as the payload for messages of typeMessageKind::End.MessageKind::Endisn't new, it's something that was always there, but until now was always empty.StreamFooter, a simple binary frame that mirrorsStreamHeader, and whose main job is to keep track of where theRrdFooter.RrdFooter, e.g. when registering data on a Redap-compliant server.For now, these footers are always an empty protobuf messages. We will be filling them up in the following PRs.
Part of RRD footers series of PRs:
TODO
@rerun-bot full-check