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

Add general use RTMP server #85

Merged
merged 47 commits into from
Jun 18, 2024
Merged

Add general use RTMP server #85

merged 47 commits into from
Jun 18, 2024

Conversation

varsill
Copy link
Contributor

@varsill varsill commented Feb 13, 2024

This PR:

  • adds a separate RTMP server capable of serving multiple clients

TODO:

  • Rewritte Membrane.RTMP.Source in such a manner that it uses the RTMP.Server
  • Add support for SSL socket
  • Update docs and examples

To run the server, use the following command:

Membrane.RTMP.Server.run(port: 1935)

and start some RTMP client, e.g. with the use FFmpeg:

ffmpeg -r 30 -f lavfi -i testsrc -vf scale=1280:960 -vcodec h264 -profile:v baseline -pix_fmt yuv420p -f flv rtmp://localhost:1935/live/test

@varsill varsill requested a review from Qizot as a code owner February 13, 2024 15:23
@varsill varsill marked this pull request as draft February 13, 2024 15:23
@varsill varsill requested a review from mat-hek February 22, 2024 15:49
@type ssl_socket_control_needed_t() :: {:ssl_socket_control_needed, :ssl.sslsocket(), pid()}

@type validation_stage_t :: :publish | :release_stream | :set_data_frame
flow_control: :push
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we should change it to push 🤔

@behaviour Membrane.RTMP.Server.ClientHandlerBehaviour

@impl true
def handle_init() do
Copy link
Member

Choose a reason for hiding this comment

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

ability to pass some options to init would be probably useful


@impl true
def handle_end_of_stream(state) do
if state.source_pid != nil, do: send(state.source_pid, :end_of_stream)
Copy link
Member

Choose a reason for hiding this comment

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

this probably should be buffered too

This notification is only sent when the `:output` pad never reaches `:start_of_stream`.
"""
@type unexpected_socket_closed_t() :: :unexpected_socket_closed
def_options app: [], stream_key: [], server: []
Copy link
Member

Choose a reason for hiding this comment

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

a possibility to pass server options and have the server started by the source would be nice for use cases with a single client

@type t :: %__MODULE__{
port: :inet.port_number(),
behaviour: Membrane.RTMP.Server.ClientHandlerBehaviour.t(),
listener: pid()
Copy link
Member

Choose a reason for hiding this comment

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

it seems that the listener is always spawned and this pid is ignored in init

lib/membrane_rtmp_plugin/rtmp_server/client_handler.ex Outdated Show resolved Hide resolved
lib/membrane_rtmp_plugin/rtmp/source/message_parser.ex Outdated Show resolved Hide resolved
test/support/rtmp_source_test_pipeline.ex Outdated Show resolved Hide resolved
test/membrane_rtmp_plugin/rtmp_source_bin_test.exs Outdated Show resolved Hide resolved
_message ->
:noop
end
send(state.server, {:subscribe, state.app, state.stream_key, self()})
Copy link
Member

Choose a reason for hiding this comment

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

  • let's wrap all these sends with functions
  • since the source subscribes for the given stream key, does it mean you need to spawn the source upfront for each key you want to accept? Wouldn't it be better to receive the key from the server and decide what to do with it (spawn a pipeline possibly)?

@varsill varsill requested a review from mat-hek May 22, 2024 13:40
@varsill varsill marked this pull request as ready for review May 28, 2024 14:01
Copy link
Member

@mat-hek mat-hek left a comment

Choose a reason for hiding this comment

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

Please also make sure that the CPU usage is still reasonable

@@ -0,0 +1,82 @@
defmodule Membrane.RTMP.Source.DefaultBehaviourImplementation do
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
defmodule Membrane.RTMP.Source.DefaultBehaviourImplementation do
defmodule Membrane.RTMP.Source.ClientHandler do

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


alias Membrane.RTMP.Server.ClientHandlerBehaviour

@enforce_keys [:behaviour, :port, :use_ssl?, :listen_options]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@enforce_keys [:behaviour, :port, :use_ssl?, :listen_options]
@enforce_keys [:client_handler, :port, :use_ssl?, :listen_options]

and perhaps listen_options shouldn't be configurable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Starts the RTMP server.
"""
@spec start_link(server_options :: t(), name :: atom()) :: GenServer.on_start()
def start_link(server_options, name \\ nil) do
Copy link
Member

Choose a reason for hiding this comment

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

Let's move name to server_options, then we'll be able to pass {Membrane.RTMP.Server, options} to a supervisor. Also, the options are usually a keyword list in such cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@varsill varsill requested a review from mat-hek June 11, 2024 10:08
README.md Outdated Show resolved Hide resolved
examples/source.exs Outdated Show resolved Hide resolved
examples/source_with_standalone_server.exs Outdated Show resolved Hide resolved

# Wait for the client handler
client_handler =
receive do
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could have a very simple GenServer that would start a pipeline each time it gets the client handler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this one would require adding an option to subscribe to "any" app and any stream_key. It could be quite useful to have such an option, but perhaps let' add it in a separate PR, WDYT?

Copy link
Member

@mat-hek mat-hek Jun 14, 2024

Choose a reason for hiding this comment

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

@varsill Ok, please create an issue, put it in TODO and assign @bartkrak ;) We should also have documentation for the message with client_ref, and the way to get the app and the stream key from there. Maybe we should allow awaiting for any app and stream key too.

examples/source_with_standalone_server.exs Outdated Show resolved Hide resolved
lib/membrane_rtmp_plugin/rtmp/source/source.ex Outdated Show resolved Hide resolved
varsill added 2 commits June 14, 2024 09:58
…handler. Rename client handler pid into client ref. Make use of await_subscription function. Add app and stream key to the message with client ref. Improve README with description where output is expected. Update unifex to v1.2
@varsill varsill requested a review from mat-hek June 14, 2024 08:06
@varsill varsill enabled auto-merge (squash) June 17, 2024 14:42
@varsill varsill disabled auto-merge June 17, 2024 14:43
@mat-hek mat-hek merged commit 00c9671 into master Jun 18, 2024
3 checks passed
@mat-hek mat-hek deleted the rtmp_server branch June 18, 2024 09:48
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