Preserve RTMP chunk stream state on abort - #3515
Open
wasphin wants to merge 1 commit into
Open
Conversation
Reset the incomplete message when processing an RTMP Abort command instead of deleting the chunk stream. This keeps the previous message header needed by subsequent compressed chunk headers.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts bRPC’s RTMP protocol handling so that an RTMP Abort control message resets a chunk stream’s in-progress (incomplete) message without deleting the chunk stream object, preserving the previous message header needed to decode subsequent compressed chunk headers. It also expands the RTMP regression test to cover aborting both the currently parsed chunk stream and a different chunk stream.
Changes:
- Replace deletion-based chunk stream clearing with an abort/reset path that preserves the chunk stream and its last header.
- Add
RtmpChunkStream::OnAbort()to reset read-assembly state for a chunk stream. - Extend RTMP unit tests to cover abort behavior for both “self” and “other” chunk streams, including split control messages via small inbound chunk size.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/brpc_rtmp_unittest.cpp | Expands regression coverage to ensure Abort preserves chunk stream/header state for both same-stream and cross-stream abort targets. |
| src/brpc/policy/rtmp_protocol.h | Renames/clarifies the context API from deletion semantics to abort/reset semantics; adds OnAbort() hook on chunk streams. |
| src/brpc/policy/rtmp_protocol.cpp | Implements abort-as-reset (AbortChunkStream + RtmpChunkStream::OnAbort) and applies it from Abort message handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
869
to
880
| SubChunkArray* sub_array = | ||
| _cstream_ctx[index1].load(butil::memory_order_consume); | ||
| if (sub_array == nullptr) { | ||
| LOG(ERROR) << "chunk_stream_id=" << cs_id << " does not exist"; | ||
| return; | ||
| } | ||
| const uint32_t index2 = cs_id - index1 * RTMP_CHUNK_ARRAY_2ND_SIZE; | ||
| RtmpChunkStream* cstream = | ||
| sub_array->ptrs[index2].load(butil::memory_order_consume); | ||
| if (cstream == nullptr) { | ||
| LOG(ERROR) << "chunk_stream_id=" << cs_id << " does not exist"; | ||
| return; |
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.
What problem does this PR solve?
Issue Number: N/A
Problem Summary:
Processing an RTMP Abort command currently deletes the target chunk stream.
This also discards its previous message header, so later compressed chunk
headers cannot be decoded. Ignoring an Abort that targets the currently
parsed chunk stream also leaves the intended state transition implicit.
What is changed and the side effects?
Changed:
Reset the target chunk stream's incomplete message while preserving the chunk
stream and its previous message header. Extend the RTMP regression test to
cover aborting both the current and a different chunk stream.
Side effects:
Performance effects: None expected. Abort now resets existing state instead
of deleting and potentially recreating the chunk stream.
Breaking backward compatibility: No.
Check List: