Don't accept streams until server is ready to read them #94

bartkrak · 2024-06-13T23:14:15Z

solves: membraneframework/membrane_core#812

Qizot · 2024-06-18T07:40:36Z

Please make sure tests and linter passes.

varsill

I think we could make use of the the lambda to serve as a "subscription mechanism" and remove the built-in subscription mechanism. It would mean that lambda should receive client_ref as one of it's arguments as well.
The, the general flow would be following:

Client connects and the {app, stream_key} pair is resolved
The lambda is called and a user custom action is invoked - in most cases the user would simply send client_ref to some process that waits for it
Then the user makes use of the client_ref for instance by passing it to the RTMP.Source (which makes a :demand) or making a custom :demand on it
Once the client_ref receives the :demand message for the first time, it finishes the handshake
I believe such an approach would simplify the usage of the server

varsill · 2024-07-08T08:05:18Z

.gitignore

@@ -177,3 +177,4 @@ $RECYCLE.BIN/
 *.lnk

 # End of https://www.gitignore.io/api/c,vim,linux,macos,elixir,windows,visualstudiocode
+received.flv


do we really want this one in .gitignore?

varsill · 2024-07-08T08:06:59Z

lib/membrane_rtmp_plugin/rtmp_server.ex

@@ -1,6 +1,12 @@
 defmodule Membrane.RTMP.Server do
  @moduledoc """
  A simple RTMP server, which handles each new incoming connection.
+
+  When new client connects to the server, it goes into :client_waiting_queue and its RTMP handshake will remanin unfinished.
+  Only when pipeline tries to pull data from client, its handshake will be finished, and client will be registered.


I wouldn't mention the "pipeline" - the RTMPServer is meant to be used outside of Membrane pipeline as well and using it with the Membrane.RTMP.Source is just one of the options.

lib/membrane_rtmp_plugin/rtmp_server.ex

lib/membrane_rtmp_plugin/rtmp_server/client_handler.ex

varsill

Great job! Now it's much more simple.
Concerning the "lambda" (now called new_client_callback) - what do you think about defining that callback in the ClientHandlerBehaviour?

examples/source.exs

lib/membrane_rtmp_plugin/rtmp/source/source.ex

varsill · 2024-07-09T12:08:36Z

lib/membrane_rtmp_plugin/rtmp_server.ex

@@ -1,6 +1,12 @@
 defmodule Membrane.RTMP.Server do
  @moduledoc """
  A simple RTMP server, which handles each new incoming connection.
+
+  When new client connects to the server, it goes into :client_waiting_queue and its RTMP handshake will remanin unfinished.


I don't think this part is valid anymore

lib/membrane_rtmp_plugin/rtmp_server/client_handler.ex

varsill

We are almost there!
I've left some minor comments mostly concerning docs and one suggestion (about default new_client_callback implementation) to think about

lib/membrane_rtmp_plugin/rtmp/source/source.ex

lib/membrane_rtmp_plugin/rtmp_server.ex

varsill · 2024-07-12T10:03:26Z

test/membrane_rtmp_plugin/rtmp_source_bin_test.exs

+    parent_process_pid = self()
+
+    new_client_callback = fn client_ref, app, stream_key ->
+      send(parent_process_pid, {:client_ref, client_ref, app, stream_key})
+    end


This one seems to look as a kind of a pattern we repeat in most use cases - what do you think about making it a default new_client_callback implementation?

varsill · 2024-07-12T10:04:29Z

lib/membrane_rtmp_plugin/rtmp_server.ex

-  A simple RTMP server, which handles each new incoming connection.
+  A simple RTMP server, which handles each new incoming connection. When a new client connects, the new_client_callback is invoked.
+  New connections remain in an incomplete RTMP handshake state until another process makes demand for dara data.
+  If no data is demanded within the client_timeout period, the connection is closed.


Perhaps it's good to emphasise that it's a client TCP socket that is closed under such circumstances?

lib/membrane_rtmp_plugin/rtmp_server/client_handler.ex

varsill

I've left two minor comments. I also believe you need to update credo to v1.7 to make the CI pass ;) (mix deps.update credo)

varsill · 2024-07-23T17:05:09Z

lib/membrane_rtmp_plugin/rtmp/source/source.ex

-  connect on this URL, the source won't complete its setup
-  * by spawning `Membrane.RTMP.Server`, subscribing for a given app and stream key on which the client
-  will connect, waiting for a client reference and passing the client reference to the `#{inspect(__MODULE__)}`.
+  connect on this URL, the source won't complete its setup. Note that all attepted connections to


Suggested change

connect on this URL, the source won't complete its setup. Note that all attepted connections to

connect on this URL, the source won't complete its setup. Note that all attempted connections to

varsill · 2024-07-23T17:07:36Z

lib/membrane_rtmp_plugin/rtmp_server.ex

+                                app :: String.t(),
+                                stream_key :: String.t() ->
+                                  any()),
+          client_timeout: non_neg_integer()


In most plugins we use Membrane.Time for time handling, so it might be good to be consistent with that here as well:

Suggested change

client_timeout: non_neg_integer()

client_timeout: Membrane.Time.t()

varsill

🥇

varsill · 2024-07-25T09:00:20Z

lib/membrane_rtmp_plugin/rtmp_server.ex

    server_options = Enum.into(server_options, %{})
+
+    server_options =
+      if Map.get(server_options, :new_client_callback, nil) == nil do


[NIT] I believe you could do as well:

Suggested change

if Map.get(server_options, :new_client_callback, nil) == nil do

if server_options[:new_client_callback] == nil do

Qizot

I will approve this but @varsill can we move the code owner to someone else? I won't be working on the repository anytime soon.

bartkrak requested a review from Qizot as a code owner June 13, 2024 23:14

bartkrak removed the request for review from Qizot June 14, 2024 13:47

Base automatically changed from rtmp_server to master June 18, 2024 09:48

bartkrak added 3 commits June 19, 2024 16:44

wip, unifex version bumped

e358e26

works but goes into infinite loop

a9166de

infinite loop fixed

85e49c2

bartkrak force-pushed the handle_new_client branch from 4cc62b6 to 85e49c2 Compare June 19, 2024 14:51

bartkrak added 5 commits July 3, 2024 03:44

removed subscription mechanism, lambda on new client connection

7018f7a

test tests on CI

44f1690

quick test fix

6089407

example

52ff895

another test fix

db779f3

varsill requested changes Jul 8, 2024

View reviewed changes

bartkrak added 9 commits July 8, 2024 16:40

lambda trigger moved to client handler

2ddc8f3

standalone server works with new api

6b79c2a

CI tests test

54b6e50

typo

b61aad8

other app stream key test fix

fcffb0d

test timeout fix

8283bdc

maybe CI will work this time

614ec22

formatting

d068af2

review suggestions

0579cfb

bartkrak mentioned this pull request Jul 8, 2024

[RTMP] Support streaming sole audio / video in the source bin membraneframework/membrane_core#813

Closed

varsill reviewed Jul 9, 2024

View reviewed changes

bartkrak added 5 commits July 9, 2024 18:25

some warnings added

ff54122

reject unused connections, tests fix

b3a0720

gitignore fix

9bd3632

format

859550b

another format fix

29709ae

varsill reviewed Jul 12, 2024

View reviewed changes

docs minor fix

c3357ce

bartkrak mentioned this pull request Jul 18, 2024

Use new rtmp api membraneframework/boombox#12

Merged

varsill self-requested a review July 23, 2024 17:04

varsill approved these changes Jul 23, 2024

View reviewed changes

bartkrak and others added 11 commits July 23, 2024 20:09

added default new_client_callback implementation

9aa29b7

credo update, minor fixes

032b5e2

url parsing moved to utils.ex and made public

76d45d1

parse_url doc

ca181ea

Update config.yml

b5f8263

bundlex bump

938fb06

dialyzer fix

ed16832

mime fix

0fa80b1

CI cache

b31bbd3

ci fix

822aa2e

mix lock fix

81d30d9

varsill approved these changes Jul 25, 2024

View reviewed changes

cosmetic change

b2a21c6

varsill self-requested a review July 25, 2024 16:26

varsill approved these changes Jul 25, 2024

View reviewed changes

bartkrak requested a review from Qizot July 25, 2024 16:28

Qizot approved these changes Jul 25, 2024

View reviewed changes

bartkrak merged commit dc6a295 into master Jul 25, 2024
3 checks passed

bartkrak deleted the handle_new_client branch July 25, 2024 20:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't accept streams until server is ready to read them #94

Don't accept streams until server is ready to read them #94

bartkrak commented Jun 13, 2024

Qizot commented Jun 18, 2024

varsill left a comment

varsill Jul 8, 2024

varsill Jul 8, 2024

varsill left a comment

varsill Jul 9, 2024

varsill left a comment

varsill Jul 12, 2024

varsill Jul 12, 2024

varsill left a comment

varsill Jul 23, 2024

varsill Jul 23, 2024

varsill left a comment

varsill Jul 25, 2024

Qizot left a comment

	connect on this URL, the source won't complete its setup. Note that all attepted connections to
	connect on this URL, the source won't complete its setup. Note that all attempted connections to

	client_timeout: non_neg_integer()
	client_timeout: Membrane.Time.t()

	if Map.get(server_options, :new_client_callback, nil) == nil do
	if server_options[:new_client_callback] == nil do

Don't accept streams until server is ready to read them #94

Don't accept streams until server is ready to read them #94

Conversation

bartkrak commented Jun 13, 2024

Qizot commented Jun 18, 2024

varsill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varsill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varsill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varsill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varsill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Qizot left a comment

Choose a reason for hiding this comment