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

Draft: Improve socket connection dropped and delete stream events #100

Merged
merged 4 commits into from
Oct 25, 2024

Conversation

dmorn
Copy link
Contributor

@dmorn dmorn commented Oct 3, 2024

This is here to close membraneframework/membrane_core#792.

  • behaviour remains the same, on socket connection closed end_of_stream is forwarded
  • a new notification was added from the source component: :stream_deleted. This is delivered when the DeleteStream is received from RTMP (which signals the actual termination of a stream). Clients can decide how to react.

I'll keep it a draft waiting for your feedback (and some extensive tests on our side).

\cc @mat-hek

@dmorn dmorn requested a review from varsill as a code owner October 3, 2024 16:01
Copy link
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.

Hello!
Thanks for the contribution. It definitely makes sense to distinguish between "gracefull" shutdown with DeleteStream and some unexpected one.

I am just slightly concerned about the fact that :stream_deleted is not synchronized with an actual stream (perhaps an event would be better here?)
I also wonder how do you mean to use this :stream_deleted notification. Do you plan to set some flag upon receiving :stream_deleted notification in your parent element and than, depending on value of that flag, react differently when handle_element_end_of_stream is called?
If so, I think it is OK to leave the notification, but in a final run I would prefer to add some generic event to membrane core, that we could send along :end_of_stream to mark a "graceful" end of stream.

lib/membrane_rtmp_plugin/rtmp_server/client_handler.ex Outdated Show resolved Hide resolved
lib/membrane_rtmp_plugin/rtmp_server/client_handler.ex Outdated Show resolved Hide resolved
@dmorn
Copy link
Contributor Author

dmorn commented Oct 4, 2024

Hi @varsill!

I am just slightly concerned about the fact that :stream_deleted is not synchronized with an actual stream (perhaps an event would be better here?)
I also wonder how do you mean to use this :stream_deleted notification. Do you plan to set some flag upon receiving :stream_deleted notification in your parent element and than, depending on value of that flag, react differently when handle_element_end_of_stream is called?
If so, I think it is OK to leave the notification, but in a final run I would prefer to add some generic event to membrane core, that we could send along :end_of_stream to mark a "graceful" end of stream.

Our pipeline is pretty long and in case of an event we would have to pass it over the whole pipeline. A notification allows us to "jump" over to the concerned element. In our case, we turn the output HLS playlist into VOD only if we receive both the notification and end of stream.

@varsill
Copy link
Contributor

varsill commented Oct 4, 2024

Our pipeline is pretty long and in case of an event we would have to pass it over the whole pipeline. A notification allows us to "jump" over to the concerned element. In our case, we turn the output HLS playlist into VOD only if we receive both the notification and end of stream.

I see, in this case it shouldn't be a problem at all. You could encounter a problem if you relied just on the notification (there wouldn't be any guarantee that the stream in the rest of the pipeline is already processed when you turn the playlist into VOD).
Concerning event passing, I don't think it would be a problem to pass it over the whole pipeline, as default implementation of the handle_event callback in elements is forwarding incoming events further. But for now I think it's OK just to send the notification and use it along :end_of_stream ;)

@dmorn
Copy link
Contributor Author

dmorn commented Oct 8, 2024

@varsill I did not know that! If that's the case, an event might be even better, let me know what you're more keen on merging. About the linter, I'm running mix format in the proj but it's still complaining, am I missing something? Side note: can we move a bit faster in the merge process? I have other 2 PR's coming and it would be great if we could do all of this today :D

@varsill
Copy link
Contributor

varsill commented Oct 9, 2024

Hi @dmorn , sure, sorry for late reply, I am OK with merging what we currently have (the notification sent from the source). Concerning the formatter, I think there might be a mismatch in the version of elixir/mix you are using (could you tell me what do you use?)

@varsill varsill self-assigned this Oct 23, 2024
@mat-hek mat-hek merged commit 9fd16d3 into membraneframework:master Oct 25, 2024
2 of 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
Archived in project
Development

Successfully merging this pull request may close these issues.

RTMP plugin delivers end_of_stream on TCP disconnection
3 participants