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

RTMP plugin delivers end_of_stream on TCP disconnection #792

Open
dmorn opened this issue Apr 15, 2024 · 5 comments
Open

RTMP plugin delivers end_of_stream on TCP disconnection #792

dmorn opened this issue Apr 15, 2024 · 5 comments

Comments

@dmorn
Copy link

dmorn commented Apr 15, 2024

This issue is also related to

The RTMP stream is delivering end of stream when the TCP connection is dropped, see here. For us this is plain wrong logic, as one thing is an error such as a connection drop, one thing is an actual end of stream, in amf terms a deleteStream message.

We're turning output HLS streams into VOD when the transmission is ended, and this is an event you cannot withdraw, once player see that tag they stop reloading the playlist.

Can we add an option tunes this behaviour? Or even better (this is how we deal it in our work)

--- a/lib/membrane_rtmp_plugin/rtmp/source/source.ex
+++ b/lib/membrane_rtmp_plugin/rtmp/source/source.ex
@@ -224,8 +224,9 @@ defmodule Membrane.RTMP.Source do
   def handle_info({:socket_closed, _socket}, ctx, state) do
     cond do
       ctx.pads.output.end_of_stream? -> {[], state}
-      ctx.pads.output.start_of_stream? -> {[end_of_stream: :output], state}
-      true -> {[notify_parent: :unexpected_socket_closed, end_of_stream: :output], state}
+      # This might be a connection error. Only deleteStream message signals that
+      # the transmission is finished.
+      true -> {[notify_parent: :unexpected_socket_closed], state}
     end
   end

Basically we just notify the parent of the event, then the pipeline can decide how the rest of the children should react.

@mat-hek
Copy link
Member

mat-hek commented Apr 26, 2024

Since dropping the connection means we won't send anything anymore from the source, the end of stream is quite expected. I assume that the logic is to remove the element on the connection drop and spawn a new one - in that case, some further element should take care of intercepting the end_of_stream and be linked to the newly spawned source.

@dmorn
Copy link
Author

dmorn commented Apr 29, 2024

Since dropping the connection means we won't send anything anymore from the source

A connection drop is something related to a problem in the transport layer. It signals a network fault, which in my opinion is unrelated to the end of stream event, which means that everything is done (whereas is not in this case).

I assume that the logic is to remove the element on the connection drop and spawn a new one

Thanks to crash groups & dynamic pads this is very easy to accomplish and it is actually how we're handling it.

element should take care of intercepting the end_of_stream

This element the has to know wether that was a false positive or not. I really think this is very confusing. This is an RTMP plugin, RTMP has a message that signals the real end_of_stream, let's just stick with it @mat-hek!

@mat-hek
Copy link
Member

mat-hek commented May 6, 2024

I assume that the logic is to remove the element on the connection drop and spawn a new one

Thanks to crash groups & dynamic pads this is very easy to accomplish and it is actually how we're handling it.

Hmm, let's assume we don't send the end_of_stream. You get the unexpected_socket_closed notification and remove the RTMP source and possibly other elements. When you do that, the first thing that happens is that end_of_stream is generated on the input pad just after the removed elements, so it seems the behaviour is the same anyways 🤔

@dmorn
Copy link
Author

dmorn commented May 6, 2024

Not in case the removed elements and the leftovers are connected with a dynamic pad right?

@mat-hek
Copy link
Member

mat-hek commented May 6, 2024

I’d have to check, but I think in this case as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

No branches or pull requests

2 participants