Implement bin #68

FelonEkonom · 2025-03-07T16:06:45Z

closes #8

mat-hek

Overall looks good, let's add the docs ;)

mat-hek · 2025-03-28T09:30:52Z

lib/boombox/bin.ex

+  @type input() ::
+          (path_or_uri :: String.t())
+          | {:mp4, location :: String.t(), transport: :file | :http}
+          | {:webrtc, Boombox.webrtc_signaling()}
+          | {:whip, uri :: String.t(), token: String.t()}
+          | {:rtmp, (uri :: String.t()) | (client_handler :: pid)}
+          | {:rtsp, url :: String.t()}
+          | {:rtp, Boombox.in_rtp_opts()}
+          | {:stream, pid(), Boombox.in_stream_opts()}
+
+  @type output ::
+          (path_or_uri :: String.t())
+          | {path_or_uri :: String.t(), [Boombox.force_transcoding()]}
+          | {:mp4, location :: String.t()}
+          | {:mp4, location :: String.t(), [Boombox.force_transcoding()]}
+          | {:webrtc, Boombox.webrtc_signaling()}
+          | {:webrtc, Boombox.webrtc_signaling(), [Boombox.force_transcoding()]}
+          | {:whip, uri :: String.t(), [{:token, String.t()} | {bandit_option :: atom(), term()}]}
+          | {:hls, location :: String.t()}
+          | {:hls, location :: String.t(), [Boombox.force_transcoding()]}
+          | {:rtp, Boombox.out_rtp_opts()}
+          | {:stream, pid(), Boombox.out_stream_opts()}


how does it differ from Boombox.input()/output()?

Boombox has {:stream, options}
Boombox.Bin has {:stream, pid(), options}

As we spoke, let's create a private bin to handle this

mat-hek · 2025-03-28T09:38:57Z

lib/boombox/bin.ex

+  def handle_playing(_ctx, state), do: {[], state}
+
+  @impl true
+  def handle_pad_added(pad_ref, ctx, state) when state.input == :membrane_pad do


let's throw a readable error when someone specifies a different input and links some pads

FelonEkonom · 2025-03-28T13:18:11Z

Overall looks good, let's add the docs ;)

I would add the docs after adding output pads, maybe the API will change till then and I hope I will have a better idea how to fix issue with passing {:stream, ...} option to Boombox.Bin

varsill · 2025-03-31T14:40:49Z

lib/boombox.ex

-    opts
-    |> Enum.split_with(fn {key, _value} -> key == :force_transcoding end)
-  end
+  defp resolve_stream_endpoint(endpont, _parent), do: endpont


Suggested change

defp resolve_stream_endpoint(endpont, _parent), do: endpont

defp resolve_stream_endpoint(endpoint, _parent), do: endpoint

varsill · 2025-03-31T15:08:24Z

test/boombox_bin_test.exs

+    if video_format != nil do
+      Compare.compare(out_file, "test/fixtures/ref_bun10s_aac.mp4",
+        kinds: [:video],
+        subject_tracks_number: tracks_number


It's quite long, but how about:

Suggested change

subject_tracks_number: tracks_number

expected_subject_tracks_number: tracks_number

?

varsill · 2025-04-01T08:50:17Z

lib/boombox/pad.ex

+    raise """
+    Boombox.Bin pad #{inspect(pad_ref)} was added while the Boombox.Bin playback \
+    is already :playing. All Boombox.Bin pads have to be linked before it enters \
+    :playing playback. To achieve so, link all pads of Boombox.Bin in the same spec \


Suggested change

:playing playback. To achieve so, link all pads of Boombox.Bin in the same spec \

:playing playback. To achieve it, link all the pads of Boombox.Bin in the same spec \

varsill · 2025-04-01T08:52:35Z

lib/boombox/pad.ex

+  @spec create_input(CallbackContext.t()) :: Ready.t() | Wait.t() | no_return()
+  def create_input(ctx) when ctx.playback == :playing and ctx.pads == %{} do
+    raise """
+    Cannot create input of type #{inspect(__MODULE__)}, while there are no pads. \


Suggested change

Cannot create input of type #{inspect(__MODULE__)}, while there are no pads. \

Cannot create input of type #{inspect(__MODULE__)}, as there are no pads available. \

varsill · 2025-04-01T09:15:39Z

lib/boombox/bin.ex

+    :ok = maybe_log_transcoding_related_warning(state.input, state.output)
+
+    with :membrane_pad <- state.input, {:stream, _pid, _opts} <- state.output do
+      raise "Boombox bin cannot have pad input and stream output at the same time"


As discussed, please remember to change these raise messages once the code is separated into public bin and private bin

mat-hek

As always, add some examples to examples.livemd

mat-hek · 2025-04-22T15:01:47Z

lib/boombox/bin.ex

+    pads
+    |> Enum.find(fn {Pad.ref(name, _id), %{options: options}} ->
+      name == :output and
+        (Bunch.listify(options.codec) -- @codecs[options.kind]) -- [nil] != []


I find this hard to read, maybe refactor to Enum.all?

mat-hek · 2025-04-22T15:05:19Z

test/support/compare.ex

+      # <<<<<<< HEAD
+      #       assert abs(length(ref_audio_bufs) - length(sub_audio_bufs)) <= 1
+
+      #       Enum.zip(sub_audio_bufs, ref_audio_bufs)
+      #       |> Enum.each(fn {sub, ref} ->
+      #         # The results differ between operating systems
+      #         # and subsequent runs due to transcoding.
+      #         # The threshold here is obtained empirically and may need
+      #         # to be adjusted, or a better metric should be used.
+      #         boundary = options[:audio_error_bounadry] || 30_000
+      #         assert samples_min_squared_error(sub.payload, ref.payload, 16) < boundary
+      #       end)
+      # =======
+      #       assert samples_min_squared_error(sub_audio, ref_audio, 16) < 30_000
+      # >>>>>>> origin/master


mat-hek · 2025-04-22T15:10:53Z

lib/boombox/bin.ex

+  If you use it as a Membrane Source, you have to specify `:input` option.
+  If you use it as a Membrane Sink, you have to specify `:output` option.
+  Boombox.Bin cannot be used as both Source and Sink at the same time.


Maybe mention what pads they need to link too, and provide some example, like child(%Boombox.Bin{input: "file.mp4"}) |> via_out(:output, options: [kind: :video]) |> ...

lib/boombox/bin.ex

mat-hek · 2025-04-22T15:30:19Z

lib/boombox/internal_bin.ex

+  @type input() ::
+          (path_or_uri :: String.t())
+          | {:mp4, location :: String.t(), transport: :file | :http}
+          | {:webrtc, Boombox.webrtc_signaling()}
+          | {:whip, uri :: String.t(), token: String.t()}
+          | {:rtmp, (uri :: String.t()) | (client_handler :: pid)}
+          | {:rtsp, url :: String.t()}
+          | {:rtp, Boombox.in_rtp_opts()}
+          | {:stream, pid(), Boombox.in_stream_opts()}
+          | :membrane_pad
+
+  @type output ::
+          (path_or_uri :: String.t())
+          | {path_or_uri :: String.t(), [Boombox.transcoding_policy()]}
+          | {:mp4, location :: String.t()}
+          | {:mp4, location :: String.t(), [Boombox.transcoding_policy()]}
+          | {:webrtc, Boombox.webrtc_signaling()}
+          | {:webrtc, Boombox.webrtc_signaling(), [Boombox.transcoding_policy()]}
+          | {:whip, uri :: String.t(), [{:token, String.t()} | {bandit_option :: atom(), term()}]}
+          | {:hls, location :: String.t()}
+          | {:hls, location :: String.t(), [Boombox.transcoding_policy()]}
+          | {:rtp, Boombox.out_rtp_opts()}
+          | {:stream, pid(), Boombox.out_stream_opts()}
+          | :membrane_pad


still duplicated

Co-authored-by: Mateusz Front <[email protected]>

varsill · 2025-04-28T09:40:40Z

lib/boombox/bin.ex

+        If set to `:always`, the media stream will be decoded and/or encoded, even if the
+        format of the stream arriving at the #{inspect(__MODULE__)} endpoint matches the \
+        output pad codec.


How about adding some example when :always is helpful, i.e. when we want to "fix" the stream or support the keyframe requests? (We have a similar examples for the :never option)

varsill · 2025-04-28T09:45:02Z

lib/boombox/bin.ex

+      #{inspect(__MODULE__)} cannot accept input and output options at the same time, but both were provided.\
+      Input: #{inspect(opts.input)}, output: #{inspect(opts.output)}.


That message only describes a situation when nil_opts_number == 0, how about nil_opts_number == 2?

varsill · 2025-04-28T09:50:53Z

lib/boombox/hls.ex

@@ -4,18 +4,18 @@ defmodule Boombox.HLS do
  import Membrane.ChildrenSpec

  require Membrane.Pad, as: Pad
-  alias Boombox.Pipeline.Ready
+  alias Boombox.InternalBin.Ready


Are you sure Ready and Wait should be in InternalBin, since they are use outside of the InternalBin namespace as well?

they are used only by modules that are used only inside internal bin
I can move these modules inside internal_bin directory to make it more clear

test/boombox_bin_test.exs

Co-authored-by: Łukasz Kita <[email protected]>

lib/boombox/bin.ex

FelonEkonom self-assigned this Mar 7, 2025

FelonEkonom added this to Smackore Mar 7, 2025

FelonEkonom moved this to In Progress in Smackore Mar 7, 2025

FelonEkonom added 2 commits March 20, 2025 12:05

Add pad

1522cc2

Split boombox pipeline into bin and pipeline

e3055ba

FelonEkonom force-pushed the implement-bin branch from 4fdcd93 to e3055ba Compare March 20, 2025 11:30

FelonEkonom added 3 commits March 20, 2025 12:34

add pad.create_input in bin

5fdd41c

Fix bug in bin

b402d2f

Write test wip

7909366

FelonEkonom force-pushed the implement-bin branch from 80db650 to 7909366 Compare March 20, 2025 15:27

FelonEkonom added 5 commits March 21, 2025 16:19

Write tests wip

982256e

Make test work :)

db7c910

Refactor bin test

7d0c81f

Add new test

cd3abbe

Parse options in boombox bin as in boombox script

2c7290c

darthez removed this from Smackore Mar 25, 2025

Refactor parsing boombox opts

c147283

FelonEkonom marked this pull request as ready for review March 26, 2025 14:04

FelonEkonom requested a review from mat-hek March 26, 2025 14:04

FelonEkonom added 4 commits March 27, 2025 09:51

Improve error messages

bf2ebf4

Add stream process to stream endpoint:

a8c321c

Small warning improvemenet

1413523

Fix credo

a4250ac

mat-hek reviewed Mar 28, 2025

View reviewed changes

mat-hek requested a review from varsill March 28, 2025 09:42

Implement CR suggestion

f8bfb1b

FelonEkonom requested a review from mat-hek March 28, 2025 13:19

Fix typo

2bed374

varsill reviewed Apr 1, 2025

View reviewed changes

FelonEkonom added 9 commits April 11, 2025 17:02

Negotiate webrtc tracks depending on pad codec

72b7e89

Fix flaky test

e927968

force_transcoding option -> transcoding_policy

831f322

Write documentation for Boombox Bin

465d0b3

Fix typos in docs

5db395b

Fix test

d3ff5f8

Fix credo

e153306

Fix typo

ebc235d

Remove leftovers

294c204

FelonEkonom requested a review from mat-hek April 14, 2025 15:15

mat-hek requested a review from varsill April 22, 2025 14:55

mat-hek reviewed Apr 22, 2025

View reviewed changes

FelonEkonom and others added 4 commits April 24, 2025 17:42

Update lib/boombox/bin.ex

d41a0fe

Co-authored-by: Mateusz Front <[email protected]>

Implement suggestions from CR

5ed7870

Add example

e0e6c48

Restore example setup

b0b8154

FelonEkonom requested a review from mat-hek April 25, 2025 09:38

mat-hek approved these changes Apr 25, 2025

View reviewed changes

varsill reviewed Apr 28, 2025

View reviewed changes

FelonEkonom and others added 5 commits April 29, 2025 11:18

Update test/boombox_bin_test.exs

041bda3

Co-authored-by: Łukasz Kita <[email protected]>

Refactor directories wip

46a3277

Refactor directories layout

e804425

Merge remote-tracking branch 'origin/implement-bin' into implement-bin

b3b2691

Improve pad docs

7ec5022

FelonEkonom requested a review from varsill April 29, 2025 09:42

varsill reviewed Apr 29, 2025

View reviewed changes

lib/boombox/bin.ex Outdated Show resolved Hide resolved

varsill approved these changes Apr 29, 2025

View reviewed changes

Update lib/boombox/bin.ex

c8bfa25

FelonEkonom merged commit 0794dfb into master Apr 29, 2025
2 of 3 checks passed

FelonEkonom deleted the implement-bin branch April 29, 2025 09:59

	defp resolve_stream_endpoint(endpont, _parent), do: endpont
	defp resolve_stream_endpoint(endpoint, _parent), do: endpoint

	subject_tracks_number: tracks_number
	expected_subject_tracks_number: tracks_number

	:playing playback. To achieve so, link all pads of Boombox.Bin in the same spec \
	:playing playback. To achieve it, link all the pads of Boombox.Bin in the same spec \

	Cannot create input of type #{inspect(__MODULE__)}, while there are no pads. \
	Cannot create input of type #{inspect(__MODULE__)}, as there are no pads available. \

		#{inspect(__MODULE__)} cannot accept input and output options at the same time, but both were provided.\
		Input: #{inspect(opts.input)}, output: #{inspect(opts.output)}.

Implement bin #68

Implement bin #68

Uh oh!

Conversation

FelonEkonom commented Mar 7, 2025 • edited by darthez Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mat-hek 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

mat-hek 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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FelonEkonom commented Mar 7, 2025 •

edited by darthez

Loading

FelonEkonom commented Mar 28, 2025 •

edited

Loading