Skip to content

Implement spec-compliant chunk-stream-id serialization/deserialization#118

Merged
varsill merged 20 commits intomembraneframework:masterfrom
kim-company:chunk-stream-id
Apr 3, 2026
Merged

Implement spec-compliant chunk-stream-id serialization/deserialization#118
varsill merged 20 commits intomembraneframework:masterfrom
kim-company:chunk-stream-id

Conversation

@dmorn
Copy link
Copy Markdown
Contributor

@dmorn dmorn commented Oct 21, 2025

The RTMP server was crashing when receiving streams from certain hardware encoders with the following error:

[warning] Unexpected header type when combining body chunks: 1
[error] GenServer #PID<0.1014.0> terminating
** (KeyError) key :timestamp not found in: nil
    (membrane_rtmp_plugin 0.29.1) lib/membrane_rtmp_plugin/rtmp/header.ex:114: Membrane.RTMP.Header.deserialize/2
    (membrane_rtmp_plugin 0.29.1) lib/membrane_rtmp_plugin/rtmp/message_parser.ex:180: Membrane.RTMP.MessageParser.read_frame/3

The crash was caused by incomplete support for RTMP chunk interleaving. Further, only chunk-stream-id singe byte serialization/deserialization was implemented.

This PR adds support for 1/2/3 byte type stream chunk identifiers and handles the situation where chunks from different streams are interleaved together. Fully backward compatible.

@dmorn dmorn requested a review from varsill as a code owner October 21, 2025 17:01
@dmorn dmorn changed the title Implement spec-compliant chunk-stream-id serialization/deserialization WIP: Implement spec-compliant chunk-stream-id serialization/deserialization Oct 24, 2025
@dmorn dmorn marked this pull request as draft October 24, 2025 15:22
@dmorn dmorn changed the title WIP: Implement spec-compliant chunk-stream-id serialization/deserialization Implement spec-compliant chunk-stream-id serialization/deserialization Oct 24, 2025
@dmorn dmorn mentioned this pull request Mar 19, 2026
@dmorn dmorn marked this pull request as ready for review March 19, 2026 16:05
Copy link
Copy Markdown
Contributor

@varsill varsill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!
I left one minor comment.
I also wonder if it would be possible for you to add a single integration test checking the interleaved chunks behaviour (especially if you already have some stream dump we could use as a fixture).

@dmorn
Copy link
Copy Markdown
Contributor Author

dmorn commented Mar 27, 2026

Thanks! I addressed the logging comment by including both the discarded client_ref and the already-handled one in the warning.

I also added an integration-style parser regression test based on a real Blackmagic Web Presenter 4K capture. The fixture stores post-handshake client→server RTMP TCP reads with original read boundaries preserved, which turned out to be important for reproducing the issue. I kept it constrained (~800 KB) instead of committing a full 4K media recording.

The interesting bit is that replaying this fixture leaves leftover buffered data on master, while this branch fully consumes it, so it gives us a real-world regression check for the parser behavior we’re fixing.

@varsill varsill merged commit 96b4ac6 into membraneframework:master Apr 3, 2026
2 of 3 checks passed
@varsill
Copy link
Copy Markdown
Contributor

varsill commented Apr 3, 2026

Thanks for contribution @dmorn ! I will release it together with #122, is that OK?

@dmorn
Copy link
Copy Markdown
Contributor Author

dmorn commented Apr 3, 2026

Of course! I'll prepare in next week (tuesday) @varsill!

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.

3 participants