Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send RTMP messages to client handler #103

Conversation

lastcanal
Copy link
Contributor

Adds optional callback handle_rtmp_message/2 to ClientHandlerBehaviour

I've put together a demo (membraneframework/membrane_demo#287) that uses a custom client handler with the proposed handle_rtmp_message callback to forward RTMP messages to a pipeline that creates multi-variant HLS playlists.

Add optional callback `handle_rtmp_message/2` to ClientHandlerBehaviour
@lastcanal lastcanal force-pushed the send_messages_to_client_handler_impl branch from c69a797 to 32c2aaa Compare October 29, 2024 20:28
@varsill
Copy link
Contributor

varsill commented Oct 30, 2024

Hello @lastcanal !
Thank you for your contribution.
After some consideration I've come to the conclusion that calling handle_rtmp_message only for the messages after the handshake is finished might be slightly inconsistent (at the same time, we cannot do it before the handshake finishes as we don't have client handler then).
I guess it would be better to add handle_metadata callback and have an explicit list of messages for which this callback would be called.
I've opened a PR: lastcanal#1 with the changes I suggested.

handle_rtmp_message -> handle_metadata
@lastcanal
Copy link
Contributor Author

Thank you @varsill! I've merged your changes into my branch. Your way makes the most sense, especially considering there is no need for the audio and video messages bogging down the client handler. I appreciate your help!

@varsill
Copy link
Contributor

varsill commented Oct 30, 2024

No problem, thank you very much @lastcanal !

@varsill varsill merged commit d6600a7 into membraneframework:master Oct 30, 2024
3 checks passed
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.

2 participants