membrane_rtmp_plugin icon indicating copy to clipboard operation
membrane_rtmp_plugin copied to clipboard

Draft: Improve socket connection dropped and delete stream events

Open dmorn opened this issue 1 year ago • 4 comments

This is here to close https://github.com/membraneframework/membrane_core/issues/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 avatar Oct 03 '24 16:10 dmorn

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.

dmorn avatar Oct 04 '24 11:10 dmorn

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 ;)

varsill avatar Oct 04 '24 15:10 varsill

@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

dmorn avatar Oct 08 '24 09:10 dmorn

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 avatar Oct 09 '24 15:10 varsill