Standalone server #88

Noarkhh · 2025-05-07T16:18:04Z

This PR:

Adds a Realtimer before HLS sink, since when the packets came in faster than in real time the sink deleted the segments too quickly.
Fixes a bug where an elixir stream sink could demand on a pad with eos and never get a buffer and never responding to the demanding process, effectively bricking the generator.
Makes that if the user passed audio options to the :stream input they are passed to the ElixirStream.Source.
Improved handling of audio format in ElixirStream.Source - remembering last format, either passed via options or received in a Boombox.Packet, and sending stream format if the format changed.
Add an Application specification that runs Boombox.Server when running a server release
Adds a standalone server, Boombox.Server, that allows for running Boombox in a separate process and communicate with it through synchronous functions, GenServer calls and functions.

To do:

Tests
Consider whether consuming and producing is the best nomenclature, maybe regular read, write and close could be clearer

FelonEkonom

As we spoke, let's remove Boombox.Server from the public API

FelonEkonom · 2025-05-08T12:00:58Z

lib/boombox/application.ex

+      case System.get_env("RELEASE_NAME") do
+        "server" ->
+          node_to_ping = System.get_env("NODE_TO_PING")


Can we prefix these environment variables with "BOOMBOX_", like "BOOMBOX_NODE_TO_PING"?

I'll prefix NODE_TO_PING, but RELEASE_NAME is an internal one and I don't have control over it.

lib/boombox/application.ex

lib/boombox/internal_bin/elixir_stream/source.ex

lib/boombox/server.ex

FelonEkonom · 2025-05-08T12:24:07Z

lib/boombox/server.ex

+  Mode in which Boombox is operating:
+    * `:consuming` - Boombox consumes packets provided with `consume_packet/2` calls,
+                     `{:consume_packet, packet}` GenServer calls or receiving
+                     `{:call, sender, {:consume_packet, packet}.
+    * `:producing` - Boombox produces packets in response to `produce_packet/1` calls,
+                     being called directly with `:produce_packet` or receiving
+                     `{:call, sender, :produce_packet}` messages.
+    * `:standalone` - Boombox neither consumes nor produces packets.


I would specify that we are talking about %Boombox.Packet{}. Boombox.run{input: rtp, output: {:webrtc, signaling}) consumes and produces packets in some way, but they are not %Boombox.Packet{}

I added a line in the moduledoc comment clarifying that

lib/boombox/server.ex

mat-hek · 2025-05-26T07:47:20Z

lib/boombox/internal_bin/elixir_stream.ex

@@ -31,8 +31,14 @@ defmodule Boombox.InternalBin.ElixirStream do
           |> via_out(Pad.ref(:output, :audio))}
      end)

+    audio_options =
+      case Map.take(options, @options_audio_keys) do
+        audio_options when map_size(audio_options) == 3 -> audio_options


Let's not use magic numbers like 3 here

mat-hek · 2025-05-26T07:49:13Z

lib/boombox/internal_bin/elixir_stream/sink.ex

+  def handle_info(:boombox_demand, ctx, state) do
+    available_pads =
+      state.last_pts
+      |> Enum.reject(fn {kind, _pts} -> ctx.pads[Pad.ref(:input, kind)].end_of_stream? end)


Can we remove the entry in last_pts on handle_end_of_stream instead?

varsill · 2025-05-26T08:32:10Z

lib/boombox/application.ex

+          node_to_ping = System.get_env("NODE_TO_PING")
+
+          if node_to_ping != nil do
+            Node.ping(String.to_atom(node_to_ping))


don't you want to match the return value of that function?

varsill · 2025-05-26T08:33:04Z

lib/boombox/application.ex

+  Boombox application. When released as a standalone server will start Boombox.Server under the
+  supervision tree.


How about describing how the user should set RELEASE_NAME and NODE_TO_PING?

varsill · 2025-05-26T08:34:17Z

lib/boombox/internal_bin/elixir_stream.ex

@@ -31,8 +31,14 @@ defmodule Boombox.InternalBin.ElixirStream do
           |> via_out(Pad.ref(:output, :audio))}
      end)

+    audio_options =
+      case Map.take(options, @options_audio_keys) do
+        audio_options when map_size(audio_options) == 3 -> audio_options


That 3 looks like a magic number to me, wouldn't it be better to match on particular structure of the audio_options map?

varsill · 2025-05-26T08:38:45Z

lib/boombox/internal_bin/hls.ex

@@ -61,6 +62,7 @@ defmodule Boombox.InternalBin.HLS do
              output_stream_format: %H264{alignment: :au, stream_structure: :avc3},
              transcoding_policy: transcoding_policy
            })
+            |> child(:hls_video_realtimer, Membrane.Realtimer)


I think it requires some further consideration. We currently use the default HTTPAdaptiveStream.SinkBin mode being :vod.
I am not sure if we want to generate VOD playlist in realtime. Perhaps we should add HLS specific option and either spawn SinkBin with :live mode and the realtimer, or spawn SinkBin with :vod and without the realtimer?

I think you're right, but I think it's out of scope for this PR, so I'll just revert it.

varsill · 2025-05-26T09:13:59Z

lib/boombox/server.ex

+  end
+
+  @doc """
+  Starts the server for more information see `GenServer.start/3`


Suggested change

Starts the server for more information see `GenServer.start/3`

Starts the server.

For more information see `GenServer.start/3`

lib/boombox/server.ex

FelonEkonom · 2025-05-28T08:52:10Z

lib/boombox/server.ex

+  def start_link(arg) do
+    GenServer.start_link(__MODULE__, arg)


Let's get rid of passing arg to Server.start_link because it is ignored in init callback

FelonEkonom · 2025-05-28T08:52:32Z

lib/boombox/server.ex

+  def start(arg) do
+    GenServer.start(__MODULE__, arg)


The same as in start_link

FelonEkonom · 2025-05-28T08:53:46Z

lib/boombox/server.ex

+  def consume_packet(server, packet) do
+    GenServer.call(server, {:consume_packet, packet})
+  end


Are you sure it should be a call, instead of cast or send?

Yes, the idea is that this call returns once Boombox is ready to consume a next packet, and is a packpressure mechanism

I get it, but be aware that it adds an additional complexity and it will behave badly when the CPU usage starts hitting max

FelonEkonom · 2025-05-28T09:03:31Z

lib/boombox/server.ex

+  @impl true
+  def handle_info({:call, sender, message}, state) do
+    {:reply, reply, state} = handle_call(message, sender, state)
+    send(sender, reply)
+    {:noreply, state}
+  end


Man, this looks weird

Yeah, but Pyrlang can communicate only through messages, there's no way to call the GenServer directly from there and I didn't want to use the internal GenServer message structures.

So let's move the implementation of these fake GenServer calls to the function that doesn't have handle_call in the name. It would be misleading to e.g. see errors from handle_call function that called by handle_info. What about handle_synchronous_request?

Beyond that, the second argument in GenServer handle_call isn't just pid where we should send response.

FelonEkonom · 2025-05-28T09:05:08Z

lib/boombox/server.ex

+  @impl true
+  def handle_info({:DOWN, _ref, :process, pid, :normal}, %State{boombox_pid: pid} = state) do
+    {:stop, :normal, state}
+  end


Let's stop Server also when reason is not :normal, we shouldn't ignore it

lib/boombox/application.ex

lib/boombox/server.ex

mix.exs

FelonEkonom · 2025-05-28T13:30:01Z

lib/boombox/application.ex


-          [Boombox.Server]
+  defp start_server() do
+    case System.fetch_env("NODE_TO_PING") do


It was supposed to be BOOMBOX_NODE_TO_PING

FelonEkonom · 2025-05-28T13:30:37Z

lib/boombox/server.ex

+  def start_link(_arg) do
+    GenServer.start_link(__MODULE__, nil)


I thought about def start_link() do

FelonEkonom · 2025-05-28T13:34:53Z

lib/boombox/server.ex

-  def handle_info({:DOWN, _ref, :process, pid, :normal}, %State{boombox_pid: pid} = state) do
-    {:stop, :normal, state}
+  def handle_info({:DOWN, _ref, :process, pid, reason}, %State{boombox_pid: pid} = state) do
+    {:stop, reason, state}


Suggested change

{:stop, reason, state}

reason =

case reason do

:normal -> :normal

reason -> {:boombox_crash, reason}

end

{:stop, reason, state}

FelonEkonom · 2025-05-28T14:35:30Z

lib/boombox/server.ex

  """
  @spec start(term()) :: GenServer.on_start()
-  def start(arg) do
-    GenServer.start(__MODULE__, arg)
+  def start(_arg) do


Suggested change

def start(_arg) do

def start() do

Noarkhh force-pushed the standalone-server branch 3 times, most recently from 93c744e to 2dcd808 Compare May 8, 2025 10:46

Noarkhh marked this pull request as ready for review May 8, 2025 10:46

Noarkhh requested review from mat-hek, FelonEkonom and varsill May 8, 2025 10:46

FelonEkonom reviewed May 9, 2025

View reviewed changes

Noarkhh force-pushed the standalone-server branch from 0363590 to 18be1b9 Compare May 13, 2025 12:17

Noarkhh requested a review from FelonEkonom May 13, 2025 13:04

Noarkhh added this to Smackore May 13, 2025

Noarkhh moved this to In Review in Smackore May 13, 2025

Noarkhh self-assigned this May 13, 2025

Noarkhh linked an issue May 13, 2025 that may be closed by this pull request

Boombox in Python MVP #71

Open

Noarkhh removed a link to an issue May 13, 2025

Boombox in Python MVP #71

Open

Noarkhh linked an issue May 13, 2025 that may be closed by this pull request

Boombox in Python MVP #71

Open

Noarkhh removed a link to an issue May 13, 2025

Boombox in Python MVP #71

Open

Noarkhh linked an issue May 13, 2025 that may be closed by this pull request

Boombox in Python MVP #71

Open

Base automatically changed from fix-specs to master May 15, 2025 14:44

Noarkhh added 11 commits May 21, 2025 15:48

Add realtimers before hls sink

6bd8d53

Clean up

5beed86

Fix for release, go back to elixir_stream

d3c91d4

Improve raw audio handling, fix demand eos bug

ba9c3a6

Start documenting

299ada6

Fix conflicts

5416d64

Fix min_by empty bug, add Application moduledoc

17bf4ae

Document the public API of Boombox.Server

116a0d6

Add more documentation

857cfd1

Make the server private

3a590ea

Apply reviewers suggestions

74dba9f

Noarkhh force-pushed the standalone-server branch from 18be1b9 to 74dba9f Compare May 21, 2025 13:53

mat-hek reviewed May 26, 2025

View reviewed changes

varsill reviewed May 26, 2025

View reviewed changes

FelonEkonom reviewed May 28, 2025

View reviewed changes

Apply reviewers suggestions

fc5da59

Noarkhh requested review from varsill, mat-hek and FelonEkonom May 28, 2025 12:43

FelonEkonom reviewed May 28, 2025

View reviewed changes

		Boombox application. When released as a standalone server will start Boombox.Server under the
		supervision tree.

	Starts the server for more information see `GenServer.start/3`
	Starts the server.
	For more information see `GenServer.start/3`

		def start_link(_arg) do
		GenServer.start_link(__MODULE__, nil)

-    {:stop, reason, state}
+    reason =
+      case reason do
+        :normal -> :normal
+        reason -> {:boombox_crash, reason}
+      end
+    {:stop, reason, state}

Standalone server #88

Are you sure you want to change the base?

Standalone server #88

Uh oh!

Conversation

Noarkhh commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This PR:

To do:

Uh oh!

FelonEkonom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Noarkhh May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FelonEkonom May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Noarkhh commented May 7, 2025 •

edited

Loading

Noarkhh May 13, 2025 •

edited

Loading

FelonEkonom May 28, 2025 •

edited

Loading