From 7263c9a21f9fa6b6513852fde53da27f7cbff368 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 20 Nov 2024 09:47:50 +0100 Subject: [PATCH 01/32] Write separate module for diamonds detection WiP --- lib/membrane/core/diamond_detector.ex | 100 ++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 lib/membrane/core/diamond_detector.ex diff --git a/lib/membrane/core/diamond_detector.ex b/lib/membrane/core/diamond_detector.ex new file mode 100644 index 000000000..099d63c72 --- /dev/null +++ b/lib/membrane/core/diamond_detector.ex @@ -0,0 +1,100 @@ +# defmodule Membrane.Core.DiamondDetector do +# use GenServer + +# require Membrane.Core.Message, as: Message +# require Membrane.Core.Utils, as: Utils + +# @timer_interval 100 + +# defmodule State do +# use Bunch.Access + +# defstruct output_pads: %{}, +# input_pads: %{}, +# elements_effective_flow_control: %{}, +# updated_since_last_run?: false, +# timer_ref: nil +# end + +# @impl GenServer +# def init(_arg) do +# Utils.log_on_error do +# {:ok, %State{}} +# end +# end + +# @impl GenServer +# def handle_info(Message.new(:effective_flow_control_change, {efc, pid}), %State{} = state) do +# Utils.log_on_error do +# state = set_effective_flow_control(pid, efc, state) +# {:noreply, state} +# end +# end + +# @impl GenServer +# def handle_info(Message.new(:new_element, pid), %State{} = state) do +# Utils.log_on_error do +# Process.monitor(pid) +# state = set_effective_flow_control(pid, :push, state) +# {:noreply, state} +# end +# end + +# @impl GenServer +# def handle_info(Message.new(:new_link, {from, _from_flow_control, to, to_flow_control}), %State{} = state) do +# Utils.log_on_error do +# # if :push not in [from_flow_control, to_flow_control] +# state = +# %{state | updated_since_last_run?: true} +# |> Map.update!(:output_pads, &add_new_output_pad(&1, from, to, to_flow_control)) +# |> Map.update!(:input_pads, &add_new_input_pad(&1, from, to, to_flow_control)) +# |> ensure_timer_running() + +# {:noreply, state} +# end +# end + +# @impl GenServer +# def handle_info(Message.new(:delete_link, {from, to, to_flow_control}), %State{} = state) do +# state +# |> Map.update!(:output_pads, ) + +# end + +# @impl GenServer +# def handle_info(:do_algorithm, %State{} = state) do +# Utils.log_on_error do +# # do_algorithm +# {:noreply, %{state | timer_ref: nil, updated_since_last_run?: false}} +# end +# end + +# defp set_effective_flow_control(pid, efc, %State{} = state) do +# %{state | updated_since_last_run?: true} +# |> Map.update!(:elements_effective_flow_control, &Map.put(&1, pid, efc)) +# |> ensure_timer_running() +# end + +# defp ensure_timer_running(%State{timer_ref: nil} = state) do +# timer_ref = self() |> Process.send_after(:do_algorithm, @timer_interval) +# %{state | timer_ref: timer_ref} +# end + +# defp ensure_timer_running(state), do: state + +# defp add_new_output_pad(output_pads, from, to, to_flow_control) do +# entry = {to, to_flow_control} + +# output_pads +# |> Map.update(from, [entry], &[entry | &1]) +# end + +# defp add_new_input_pad(input_pads, from, to, to_flow_control) do +# entry = {from, to_flow_control} + +# input_pads +# |> Map.update(to, [entry], &1[entry | &1]) +# end + + +# end From f4bce2d387744b40370c983c138ff4109423e04b Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 20 Nov 2024 12:26:40 +0100 Subject: [PATCH 02/32] Implement parallel diamond detection WiP --- lib/membrane/core/bin.ex | 10 +++ .../core/bin/diamond_detection_controller.ex | 13 ++++ lib/membrane/core/diamond_detector.ex | 1 - lib/membrane/core/element.ex | 25 +++++++ .../element/diamond_detection_controller.ex | 73 +++++++++++++++++++ lib/membrane/core/element/state.ex | 8 +- .../parent/diamond_detection_controller.ex | 21 ++++++ lib/membrane/core/pipeline.ex | 10 +++ .../pipeline/diamond_detection_controller.ex | 27 +++++++ lib/membrane/core/pipeline/state.ex | 7 +- 10 files changed, 190 insertions(+), 5 deletions(-) create mode 100644 lib/membrane/core/bin/diamond_detection_controller.ex create mode 100644 lib/membrane/core/element/diamond_detection_controller.ex create mode 100644 lib/membrane/core/parent/diamond_detection_controller.ex create mode 100644 lib/membrane/core/pipeline/diamond_detection_controller.ex diff --git a/lib/membrane/core/bin.ex b/lib/membrane/core/bin.ex index 61b6862b0..dd87f2ddb 100644 --- a/lib/membrane/core/bin.ex +++ b/lib/membrane/core/bin.ex @@ -261,6 +261,16 @@ defmodule Membrane.Core.Bin do {:noreply, state} end + defp do_handle_info(Message.new(:start_diamond_detection), state) do + :ok = Parent.DiamondDetectionController.start_diamond_detection(state) + {:noreply, state} + end + + defp do_handle_info(Message.new(:trigger_diamond_detection), state) do + :ok = __MODULE__.DiamondDetectionController.trigger_diamond_detection(state) + {:noreply, state} + end + defp do_handle_info(Message.new(_type, _args, _opts) = message, _state) do raise Membrane.BinError, "Received invalid message #{inspect(message)}" end diff --git a/lib/membrane/core/bin/diamond_detection_controller.ex b/lib/membrane/core/bin/diamond_detection_controller.ex new file mode 100644 index 000000000..9bc66dec5 --- /dev/null +++ b/lib/membrane/core/bin/diamond_detection_controller.ex @@ -0,0 +1,13 @@ +defmodule Membrane.Core.Bin.DiamondDetectionController do + @moduledoc false + + require Membrane.Core.Message, as: Message + + alias Membrane.Core.Bin.State + + # todo: trigger DD on spawning new child & linking + @spec trigger_diamond_detection(State.t()) :: :ok + def trigger_diamond_detection(state) do + Message.send(state.parent, :trigger_diamond_detection) + end +end diff --git a/lib/membrane/core/diamond_detector.ex b/lib/membrane/core/diamond_detector.ex index 099d63c72..57700831c 100644 --- a/lib/membrane/core/diamond_detector.ex +++ b/lib/membrane/core/diamond_detector.ex @@ -96,5 +96,4 @@ # |> Map.update(to, [entry], &1[entry | &1]) # end - # end diff --git a/lib/membrane/core/element.ex b/lib/membrane/core/element.ex index 82d10c479..291fe1e9f 100644 --- a/lib/membrane/core/element.ex +++ b/lib/membrane/core/element.ex @@ -24,6 +24,7 @@ defmodule Membrane.Core.Element do alias Membrane.Core.Element.{ BufferController, DemandController, + DiamondDetectionController, EffectiveFlowController, EventController, LifecycleController, @@ -286,6 +287,30 @@ defmodule Membrane.Core.Element do {:noreply, state} end + defp do_handle_info(Message.new(:start_diamond_detection), state) do + :ok = DiamondDetectionController.start_diamond_detection(state) + {:noreply, state} + end + + defp do_handle_info( + Message.new(:diamond_detection, [diamond_detection_ref, diamond_detection_path]), + state + ) do + state = + DiamondDetectionController.continue_diamond_detection( + diamond_detection_ref, + diamond_detection_path, + state + ) + + {:noreply, state} + end + + defp do_handle_info(Message.new(:delete_diamond_detection_ref, diamond_detection_ref), state) do + state = DiamondDetectionController.delete_diamond_detection_ref(diamond_detection_ref, state) + {:noreply, state} + end + defp do_handle_info(Message.new(_type, _args, _opts) = message, _state) do raise Membrane.ElementError, "Received invalid message #{inspect(message)}" end diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex new file mode 100644 index 000000000..9f13d4f28 --- /dev/null +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -0,0 +1,73 @@ +defmodule Membrane.Core.Element.DiamondDetectionController do + @moduledoc false + + require Membrane.Core.Message, as: Message + require Membrane.Pad, as: Pad + + alias Membrane.Child + alias Membrane.Core.Element.State + alias Membrane.Element.PadData + + @type diamond_detection_path :: [{pid(), Child.name(), Pad.ref()}] + + @spec start_diamond_detection(State.t()) :: :ok + def start_diamond_detection(state) do + make_ref() + |> forward_diamond_detection([], state) + end + + @spec continue_diamond_detection(reference(), diamond_detection_path(), State.t()) :: State.t() + def continue_diamond_detection(diamond_detection_ref, diamond_detecton_path, state) do + delete_message = Message.new(:delete_diamond_detection_ref, diamond_detection_ref) + send_after_time = Membrane.Time.seconds(10) |> Membrane.Time.as_milliseconds(:round) + self() |> Process.send_after(delete_message, send_after_time) + + case Map.fetch(state.diamond_detection_ref_to_path, diamond_detection_ref) do + {:ok, _old_path} -> + # todo: log diamond if not cycle + state + + :error -> + :ok = forward_diamond_detection(diamond_detection_ref, diamond_detecton_path, state) + + state + |> put_in( + [:diamond_detection_ref_to_path, diamond_detection_ref], + diamond_detecton_path + ) + end + end + + @spec delete_diamond_detection_ref(reference(), State.t()) :: State.t() + def delete_diamond_detection_ref(diamond_detection_ref, state) do + state + |> Map.update!( + :diamond_detection_ref_to_path, + &Map.delete(&1, diamond_detection_ref) + ) + end + + @spec forward_diamond_detection(reference(), diamond_detection_path(), State.t()) :: :ok + defp forward_diamond_detection(diamond_detection_ref, diamond_detection_path, state) do + auto_pull_mode? = state.effective_flow_control == :pull + + state.pads_data + |> Enum.each(fn {pad_ref, pad_data} -> + if is_output_pull_pad(pad_data, auto_pull_mode?) do + diamond_detection_path = [{self(), state.name, pad_ref} | diamond_detection_path] + + Message.send( + pad_data.pid, + :diamond_detection, + [diamond_detection_ref, diamond_detection_path] + ) + end + end) + end + + defp is_output_pull_pad(%PadData{} = pad_data, auto_pull_mode?) do + pad_data.direction == :output and + (pad_data.flow_control == :manual or + (pad_data.flow_control == :auto and auto_pull_mode?)) + end +end diff --git a/lib/membrane/core/element/state.ex b/lib/membrane/core/element/state.ex index e7e6392f4..83c7faa44 100644 --- a/lib/membrane/core/element/state.ex +++ b/lib/membrane/core/element/state.ex @@ -9,7 +9,7 @@ defmodule Membrane.Core.Element.State do alias Membrane.{Clock, Element, Pad, Sync} alias Membrane.Core.Child.PadModel - alias Membrane.Core.Element.EffectiveFlowController + alias Membrane.Core.Element.{DiamondDetectionController, EffectiveFlowController} alias Membrane.Core.Timer require Membrane.Pad @@ -46,7 +46,10 @@ defmodule Membrane.Core.Element.State do stalker: Membrane.Core.Stalker.t(), satisfied_auto_output_pads: MapSet.t(), awaiting_auto_input_pads: MapSet.t(), - resume_delayed_demands_loop_in_mailbox?: boolean() + resume_delayed_demands_loop_in_mailbox?: boolean(), + diamond_detection_ref_to_path: %{ + optional(reference()) => DiamondDetectionController.diamond_detection_path() + } } # READ THIS BEFORE ADDING NEW FIELD!!! @@ -79,6 +82,7 @@ defmodule Membrane.Core.Element.State do handle_demand_loop_counter: 0, pads_to_snapshot: MapSet.new(), playback_queue: [], + diamond_detection_ref_to_path: %{}, pads_data: %{}, satisfied_auto_output_pads: MapSet.new(), awaiting_auto_input_pads: MapSet.new(), diff --git a/lib/membrane/core/parent/diamond_detection_controller.ex b/lib/membrane/core/parent/diamond_detection_controller.ex new file mode 100644 index 000000000..4b5692739 --- /dev/null +++ b/lib/membrane/core/parent/diamond_detection_controller.ex @@ -0,0 +1,21 @@ +defmodule Membrane.Core.Parent.DiamondDetectionController do + @moduledoc false + + require Membrane.Core.Message, as: Message + + alias Membrane.Core.Parent + + @spec start_diamond_detection(Parent.state()) :: :ok + def start_diamond_detection(state) do + children_with_input_pads = + state.links + |> MapSet.new(& &1.to.child) + + state.children + |> Enum.each(fn {child, child_data} -> + if child_data.component_type == :bin or not MapSet.member?(children_with_input_pads, child) do + Message.send(child_data.pid, :start_diamond_detection) + end + end) + end +end diff --git a/lib/membrane/core/pipeline.ex b/lib/membrane/core/pipeline.ex index 6f9fc95db..d81c5f41f 100644 --- a/lib/membrane/core/pipeline.ex +++ b/lib/membrane/core/pipeline.ex @@ -147,6 +147,16 @@ defmodule Membrane.Core.Pipeline do {:noreply, state} end + defp do_handle_info(Message.new(:start_diamond_detection), state) do + state = __MODULE__.DiamondDetectionController.start_diamond_detection(state) + {:noreply, state} + end + + defp do_handle_info(Message.new(:trigger_diamond_detection), state) do + state = __MODULE__.DiamondDetectionController.trigger_diamond_detection(state) + {:noreply, state} + end + defp do_handle_info(Message.new(_type, _args, _opts) = message, _state) do raise Membrane.PipelineError, "Received invalid message #{inspect(message)}" end diff --git a/lib/membrane/core/pipeline/diamond_detection_controller.ex b/lib/membrane/core/pipeline/diamond_detection_controller.ex new file mode 100644 index 000000000..a63bbceca --- /dev/null +++ b/lib/membrane/core/pipeline/diamond_detection_controller.ex @@ -0,0 +1,27 @@ +defmodule Membrane.Core.Pipeline.DiamondDetectionController do + @moduledoc false + + require Membrane.Core.Message, as: Message + + alias Membrane.Core.Parent + alias Membrane.Core.Pipeline.State + + @spec trigger_diamond_detection(State.t()) :: State.t() + def trigger_diamond_detection(%State{} = state) when state.diamond_detection_triggered? do + state + end + + def trigger_diamond_detection(%State{} = state) do + message = Message.new(:start_diamond_detection) + send_after_timeout = Membrane.Time.second() |> Membrane.Time.as_milliseconds(:round) + self() |> Process.send_after(message, send_after_timeout) + + %{state | diamond_detection_triggered?: true} + end + + @spec start_diamond_detection(State.t()) :: State.t() + def start_diamond_detection(%State{} = state) do + :ok = Parent.DiamondDetectionController.start_diamond_detection(state) + %{state | diamond_detection_triggered?: false} + end +end diff --git a/lib/membrane/core/pipeline/state.ex b/lib/membrane/core/pipeline/state.ex index 37f24ea2d..bd50663e2 100644 --- a/lib/membrane/core/pipeline/state.ex +++ b/lib/membrane/core/pipeline/state.ex @@ -34,7 +34,8 @@ defmodule Membrane.Core.Pipeline.State do setup_incomplete?: boolean(), stalker: Membrane.Core.Stalker.t(), subprocess_supervisor: pid(), - awaiting_setup_completition?: boolean() + awaiting_setup_completition?: boolean(), + diamond_detection_triggered?: boolean() } # READ THIS BEFORE ADDING NEW FIELD!!! @@ -58,5 +59,7 @@ defmodule Membrane.Core.Pipeline.State do stalker: nil, resource_guard: nil, subprocess_supervisor: nil, - awaiting_setup_completition?: false + awaiting_setup_completition?: false, + diamond_detection_triggered?: false + end From e9e8c77b6f4067cef74182f5d6184dcd6a8b9d30 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 20 Nov 2024 17:43:59 +0100 Subject: [PATCH 03/32] Small refactor --- lib/membrane/core/bin.ex | 20 +++++++------- .../core/bin/diamond_detection_controller.ex | 1 - lib/membrane/core/element.ex | 10 +++---- .../element/diamond_detection_controller.ex | 26 ++++++++++++------- .../core/parent/child_life_controller.ex | 15 +++++++++++ lib/membrane/core/pipeline.ex | 20 +++++++------- 6 files changed, 57 insertions(+), 35 deletions(-) diff --git a/lib/membrane/core/bin.ex b/lib/membrane/core/bin.ex index dd87f2ddb..56cd57525 100644 --- a/lib/membrane/core/bin.ex +++ b/lib/membrane/core/bin.ex @@ -239,6 +239,16 @@ defmodule Membrane.Core.Bin do {:noreply, state} end + defp do_handle_info(Message.new(:start_diamond_detection), state) do + :ok = Parent.DiamondDetectionController.start_diamond_detection(state) + {:noreply, state} + end + + defp do_handle_info(Message.new(:trigger_diamond_detection), state) do + :ok = __MODULE__.DiamondDetectionController.trigger_diamond_detection(state) + {:noreply, state} + end + defp do_handle_info(Message.new(:child_death, [name, reason]), state) do case Parent.ChildLifeController.handle_child_death(name, reason, state) do {:stop, reason, _state} -> ProcessHelper.notoelo(reason) @@ -261,16 +271,6 @@ defmodule Membrane.Core.Bin do {:noreply, state} end - defp do_handle_info(Message.new(:start_diamond_detection), state) do - :ok = Parent.DiamondDetectionController.start_diamond_detection(state) - {:noreply, state} - end - - defp do_handle_info(Message.new(:trigger_diamond_detection), state) do - :ok = __MODULE__.DiamondDetectionController.trigger_diamond_detection(state) - {:noreply, state} - end - defp do_handle_info(Message.new(_type, _args, _opts) = message, _state) do raise Membrane.BinError, "Received invalid message #{inspect(message)}" end diff --git a/lib/membrane/core/bin/diamond_detection_controller.ex b/lib/membrane/core/bin/diamond_detection_controller.ex index 9bc66dec5..e1e2444c7 100644 --- a/lib/membrane/core/bin/diamond_detection_controller.ex +++ b/lib/membrane/core/bin/diamond_detection_controller.ex @@ -5,7 +5,6 @@ defmodule Membrane.Core.Bin.DiamondDetectionController do alias Membrane.Core.Bin.State - # todo: trigger DD on spawning new child & linking @spec trigger_diamond_detection(State.t()) :: :ok def trigger_diamond_detection(state) do Message.send(state.parent, :trigger_diamond_detection) diff --git a/lib/membrane/core/element.ex b/lib/membrane/core/element.ex index 291fe1e9f..278169646 100644 --- a/lib/membrane/core/element.ex +++ b/lib/membrane/core/element.ex @@ -282,11 +282,6 @@ defmodule Membrane.Core.Element do {:noreply, state} end - defp do_handle_info(Message.new(:terminate), state) do - state = LifecycleController.handle_terminate_request(state) - {:noreply, state} - end - defp do_handle_info(Message.new(:start_diamond_detection), state) do :ok = DiamondDetectionController.start_diamond_detection(state) {:noreply, state} @@ -311,6 +306,11 @@ defmodule Membrane.Core.Element do {:noreply, state} end + defp do_handle_info(Message.new(:terminate), state) do + state = LifecycleController.handle_terminate_request(state) + {:noreply, state} + end + defp do_handle_info(Message.new(_type, _args, _opts) = message, _state) do raise Membrane.ElementError, "Received invalid message #{inspect(message)}" end diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex index 9f13d4f28..7ea9a0c1f 100644 --- a/lib/membrane/core/element/diamond_detection_controller.ex +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -18,16 +18,12 @@ defmodule Membrane.Core.Element.DiamondDetectionController do @spec continue_diamond_detection(reference(), diamond_detection_path(), State.t()) :: State.t() def continue_diamond_detection(diamond_detection_ref, diamond_detecton_path, state) do - delete_message = Message.new(:delete_diamond_detection_ref, diamond_detection_ref) - send_after_time = Membrane.Time.seconds(10) |> Membrane.Time.as_milliseconds(:round) - self() |> Process.send_after(delete_message, send_after_time) + cond do + not is_map_key(state.diamond_detection_ref_to_path, diamond_detection_ref) -> + delete_message = Message.new(:delete_diamond_detection_ref, diamond_detection_ref) + send_after_time = Membrane.Time.seconds(10) |> Membrane.Time.as_milliseconds(:round) + self() |> Process.send_after(delete_message, send_after_time) - case Map.fetch(state.diamond_detection_ref_to_path, diamond_detection_ref) do - {:ok, _old_path} -> - # todo: log diamond if not cycle - state - - :error -> :ok = forward_diamond_detection(diamond_detection_ref, diamond_detecton_path, state) state @@ -35,6 +31,13 @@ defmodule Membrane.Core.Element.DiamondDetectionController do [:diamond_detection_ref_to_path, diamond_detection_ref], diamond_detecton_path ) + + has_cycle?(diamond_detecton_path) -> + state + + true -> + # todo: log diamond + state end end @@ -70,4 +73,9 @@ defmodule Membrane.Core.Element.DiamondDetectionController do (pad_data.flow_control == :manual or (pad_data.flow_control == :auto and auto_pull_mode?)) end + + defp has_cycle?(diamond_detection_path) do + uniq_length = diamond_detection_path |> Enum.uniq() |> length() + uniq_length < length(diamond_detection_path) + end end diff --git a/lib/membrane/core/parent/child_life_controller.ex b/lib/membrane/core/parent/child_life_controller.ex index c9936ba8d..bba8bd501 100644 --- a/lib/membrane/core/parent/child_life_controller.ex +++ b/lib/membrane/core/parent/child_life_controller.ex @@ -103,6 +103,9 @@ defmodule Membrane.Core.Parent.ChildLifeController do """ @spec handle_spec(ChildrenSpec.t(), Parent.state()) :: Parent.state() | no_return() def handle_spec(spec, state) do + + + spec_ref = make_ref() canonical_spec = make_canonical(spec) @@ -162,11 +165,23 @@ defmodule Membrane.Core.Parent.ChildLifeController do dependent_specs: dependent_specs, awaiting_responses: MapSet.new() }) + |> trigger_diamond_detection() state = StartupUtils.exec_handle_spec_started(all_children_names, state) proceed_spec_startup(spec_ref, state) end + defp trigger_diamond_detection(state) do + cond do + Component.bin?(state) -> + :ok = Bin.DiamondDetectionController.trigger_diamond_detection(state) + state + + Component.pipeline?(state) -> + Pipeline.DiamondDetectionController.trigger_diamond_detection(state) + end + end + defp assert_all_static_pads_linked_in_spec!(children_definitions, links) do pads_to_link = links diff --git a/lib/membrane/core/pipeline.ex b/lib/membrane/core/pipeline.ex index d81c5f41f..c29de244a 100644 --- a/lib/membrane/core/pipeline.ex +++ b/lib/membrane/core/pipeline.ex @@ -130,6 +130,16 @@ defmodule Membrane.Core.Pipeline do {:noreply, state} end + defp do_handle_info(Message.new(:start_diamond_detection), state) do + state = __MODULE__.DiamondDetectionController.start_diamond_detection(state) + {:noreply, state} + end + + defp do_handle_info(Message.new(:trigger_diamond_detection), state) do + state = __MODULE__.DiamondDetectionController.trigger_diamond_detection(state) + {:noreply, state} + end + defp do_handle_info(Message.new(:initialized, child), state) do state = ChildLifeController.handle_child_initialized(child, state) {:noreply, state} @@ -147,16 +157,6 @@ defmodule Membrane.Core.Pipeline do {:noreply, state} end - defp do_handle_info(Message.new(:start_diamond_detection), state) do - state = __MODULE__.DiamondDetectionController.start_diamond_detection(state) - {:noreply, state} - end - - defp do_handle_info(Message.new(:trigger_diamond_detection), state) do - state = __MODULE__.DiamondDetectionController.trigger_diamond_detection(state) - {:noreply, state} - end - defp do_handle_info(Message.new(_type, _args, _opts) = message, _state) do raise Membrane.PipelineError, "Received invalid message #{inspect(message)}" end From cabbcfb89ac0114ebfb89f8f31c0ad021f6a2434 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 20 Nov 2024 17:48:20 +0100 Subject: [PATCH 04/32] Fix bug --- .../core/parent/diamond_detection_controller.ex | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/lib/membrane/core/parent/diamond_detection_controller.ex b/lib/membrane/core/parent/diamond_detection_controller.ex index 4b5692739..26e0de871 100644 --- a/lib/membrane/core/parent/diamond_detection_controller.ex +++ b/lib/membrane/core/parent/diamond_detection_controller.ex @@ -7,15 +7,9 @@ defmodule Membrane.Core.Parent.DiamondDetectionController do @spec start_diamond_detection(Parent.state()) :: :ok def start_diamond_detection(state) do - children_with_input_pads = - state.links - |> MapSet.new(& &1.to.child) - state.children - |> Enum.each(fn {child, child_data} -> - if child_data.component_type == :bin or not MapSet.member?(children_with_input_pads, child) do - Message.send(child_data.pid, :start_diamond_detection) - end + |> Enum.each(fn {_child, %{pid: child_pid}} -> + Message.send(child_pid, :start_diamond_detection) end) end end From a58b363b0c5fb01b54a4d3645bc090b09ff52a89 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Thu, 21 Nov 2024 12:09:05 +0100 Subject: [PATCH 05/32] Fix typo --- lib/membrane/core/bin/diamond_detection_controller.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/membrane/core/bin/diamond_detection_controller.ex b/lib/membrane/core/bin/diamond_detection_controller.ex index e1e2444c7..3d964ed01 100644 --- a/lib/membrane/core/bin/diamond_detection_controller.ex +++ b/lib/membrane/core/bin/diamond_detection_controller.ex @@ -7,6 +7,6 @@ defmodule Membrane.Core.Bin.DiamondDetectionController do @spec trigger_diamond_detection(State.t()) :: :ok def trigger_diamond_detection(state) do - Message.send(state.parent, :trigger_diamond_detection) + Message.send(state.parent_pid, :trigger_diamond_detection) end end From 5a76b10e25aae7f732a8c537550ada36ed687cf7 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Thu, 21 Nov 2024 12:14:51 +0100 Subject: [PATCH 06/32] Comment out triggering diamond detection in parents --- lib/membrane/core/bin.ex | 18 ++++---- .../core/bin/diamond_detection_controller.ex | 18 ++++---- .../core/parent/child_life_controller.ex | 24 +++++------ .../parent/diamond_detection_controller.ex | 24 +++++------ lib/membrane/core/pipeline.ex | 18 ++++---- .../pipeline/diamond_detection_controller.ex | 42 +++++++++---------- lib/membrane/core/pipeline/state.ex | 8 ++-- 7 files changed, 75 insertions(+), 77 deletions(-) diff --git a/lib/membrane/core/bin.ex b/lib/membrane/core/bin.ex index 56cd57525..1d5d995f5 100644 --- a/lib/membrane/core/bin.ex +++ b/lib/membrane/core/bin.ex @@ -239,15 +239,15 @@ defmodule Membrane.Core.Bin do {:noreply, state} end - defp do_handle_info(Message.new(:start_diamond_detection), state) do - :ok = Parent.DiamondDetectionController.start_diamond_detection(state) - {:noreply, state} - end - - defp do_handle_info(Message.new(:trigger_diamond_detection), state) do - :ok = __MODULE__.DiamondDetectionController.trigger_diamond_detection(state) - {:noreply, state} - end + # defp do_handle_info(Message.new(:start_diamond_detection), state) do + # :ok = Parent.DiamondDetectionController.start_diamond_detection(state) + # {:noreply, state} + # end + + # defp do_handle_info(Message.new(:trigger_diamond_detection), state) do + # :ok = __MODULE__.DiamondDetectionController.trigger_diamond_detection(state) + # {:noreply, state} + # end defp do_handle_info(Message.new(:child_death, [name, reason]), state) do case Parent.ChildLifeController.handle_child_death(name, reason, state) do diff --git a/lib/membrane/core/bin/diamond_detection_controller.ex b/lib/membrane/core/bin/diamond_detection_controller.ex index 3d964ed01..dd0130bdc 100644 --- a/lib/membrane/core/bin/diamond_detection_controller.ex +++ b/lib/membrane/core/bin/diamond_detection_controller.ex @@ -1,12 +1,12 @@ -defmodule Membrane.Core.Bin.DiamondDetectionController do - @moduledoc false +# defmodule Membrane.Core.Bin.DiamondDetectionController do +# @moduledoc false - require Membrane.Core.Message, as: Message +# require Membrane.Core.Message, as: Message - alias Membrane.Core.Bin.State +# alias Membrane.Core.Bin.State - @spec trigger_diamond_detection(State.t()) :: :ok - def trigger_diamond_detection(state) do - Message.send(state.parent_pid, :trigger_diamond_detection) - end -end +# @spec trigger_diamond_detection(State.t()) :: :ok +# def trigger_diamond_detection(state) do +# Message.send(state.parent_pid, :trigger_diamond_detection) +# end +# end diff --git a/lib/membrane/core/parent/child_life_controller.ex b/lib/membrane/core/parent/child_life_controller.ex index bba8bd501..c571bdf82 100644 --- a/lib/membrane/core/parent/child_life_controller.ex +++ b/lib/membrane/core/parent/child_life_controller.ex @@ -103,9 +103,6 @@ defmodule Membrane.Core.Parent.ChildLifeController do """ @spec handle_spec(ChildrenSpec.t(), Parent.state()) :: Parent.state() | no_return() def handle_spec(spec, state) do - - - spec_ref = make_ref() canonical_spec = make_canonical(spec) @@ -165,22 +162,23 @@ defmodule Membrane.Core.Parent.ChildLifeController do dependent_specs: dependent_specs, awaiting_responses: MapSet.new() }) - |> trigger_diamond_detection() + + # |> trigger_diamond_detection() state = StartupUtils.exec_handle_spec_started(all_children_names, state) proceed_spec_startup(spec_ref, state) end - defp trigger_diamond_detection(state) do - cond do - Component.bin?(state) -> - :ok = Bin.DiamondDetectionController.trigger_diamond_detection(state) - state + # defp trigger_diamond_detection(state) do + # cond do + # Component.bin?(state) -> + # :ok = Bin.DiamondDetectionController.trigger_diamond_detection(state) + # state - Component.pipeline?(state) -> - Pipeline.DiamondDetectionController.trigger_diamond_detection(state) - end - end + # Component.pipeline?(state) -> + # Pipeline.DiamondDetectionController.trigger_diamond_detection(state) + # end + # end defp assert_all_static_pads_linked_in_spec!(children_definitions, links) do pads_to_link = diff --git a/lib/membrane/core/parent/diamond_detection_controller.ex b/lib/membrane/core/parent/diamond_detection_controller.ex index 26e0de871..8c4e4a36c 100644 --- a/lib/membrane/core/parent/diamond_detection_controller.ex +++ b/lib/membrane/core/parent/diamond_detection_controller.ex @@ -1,15 +1,15 @@ -defmodule Membrane.Core.Parent.DiamondDetectionController do - @moduledoc false +# defmodule Membrane.Core.Parent.DiamondDetectionController do +# @moduledoc false - require Membrane.Core.Message, as: Message +# require Membrane.Core.Message, as: Message - alias Membrane.Core.Parent +# alias Membrane.Core.Parent - @spec start_diamond_detection(Parent.state()) :: :ok - def start_diamond_detection(state) do - state.children - |> Enum.each(fn {_child, %{pid: child_pid}} -> - Message.send(child_pid, :start_diamond_detection) - end) - end -end +# @spec start_diamond_detection(Parent.state()) :: :ok +# def start_diamond_detection(state) do +# state.children +# |> Enum.each(fn {_child, %{pid: child_pid}} -> +# Message.send(child_pid, :start_diamond_detection) +# end) +# end +# end diff --git a/lib/membrane/core/pipeline.ex b/lib/membrane/core/pipeline.ex index c29de244a..bb865541e 100644 --- a/lib/membrane/core/pipeline.ex +++ b/lib/membrane/core/pipeline.ex @@ -130,15 +130,15 @@ defmodule Membrane.Core.Pipeline do {:noreply, state} end - defp do_handle_info(Message.new(:start_diamond_detection), state) do - state = __MODULE__.DiamondDetectionController.start_diamond_detection(state) - {:noreply, state} - end - - defp do_handle_info(Message.new(:trigger_diamond_detection), state) do - state = __MODULE__.DiamondDetectionController.trigger_diamond_detection(state) - {:noreply, state} - end + # defp do_handle_info(Message.new(:start_diamond_detection), state) do + # state = __MODULE__.DiamondDetectionController.start_diamond_detection(state) + # {:noreply, state} + # end + + # defp do_handle_info(Message.new(:trigger_diamond_detection), state) do + # state = __MODULE__.DiamondDetectionController.trigger_diamond_detection(state) + # {:noreply, state} + # end defp do_handle_info(Message.new(:initialized, child), state) do state = ChildLifeController.handle_child_initialized(child, state) diff --git a/lib/membrane/core/pipeline/diamond_detection_controller.ex b/lib/membrane/core/pipeline/diamond_detection_controller.ex index a63bbceca..8a8e352e2 100644 --- a/lib/membrane/core/pipeline/diamond_detection_controller.ex +++ b/lib/membrane/core/pipeline/diamond_detection_controller.ex @@ -1,27 +1,27 @@ -defmodule Membrane.Core.Pipeline.DiamondDetectionController do - @moduledoc false +# defmodule Membrane.Core.Pipeline.DiamondDetectionController do +# @moduledoc false - require Membrane.Core.Message, as: Message +# require Membrane.Core.Message, as: Message - alias Membrane.Core.Parent - alias Membrane.Core.Pipeline.State +# alias Membrane.Core.Parent +# alias Membrane.Core.Pipeline.State - @spec trigger_diamond_detection(State.t()) :: State.t() - def trigger_diamond_detection(%State{} = state) when state.diamond_detection_triggered? do - state - end +# @spec trigger_diamond_detection(State.t()) :: State.t() +# def trigger_diamond_detection(%State{} = state) when state.diamond_detection_triggered? do +# state +# end - def trigger_diamond_detection(%State{} = state) do - message = Message.new(:start_diamond_detection) - send_after_timeout = Membrane.Time.second() |> Membrane.Time.as_milliseconds(:round) - self() |> Process.send_after(message, send_after_timeout) +# def trigger_diamond_detection(%State{} = state) do +# message = Message.new(:start_diamond_detection) +# send_after_timeout = Membrane.Time.second() |> Membrane.Time.as_milliseconds(:round) +# self() |> Process.send_after(message, send_after_timeout) - %{state | diamond_detection_triggered?: true} - end +# %{state | diamond_detection_triggered?: true} +# end - @spec start_diamond_detection(State.t()) :: State.t() - def start_diamond_detection(%State{} = state) do - :ok = Parent.DiamondDetectionController.start_diamond_detection(state) - %{state | diamond_detection_triggered?: false} - end -end +# @spec start_diamond_detection(State.t()) :: State.t() +# def start_diamond_detection(%State{} = state) do +# :ok = Parent.DiamondDetectionController.start_diamond_detection(state) +# %{state | diamond_detection_triggered?: false} +# end +# end diff --git a/lib/membrane/core/pipeline/state.ex b/lib/membrane/core/pipeline/state.ex index bd50663e2..85118aa70 100644 --- a/lib/membrane/core/pipeline/state.ex +++ b/lib/membrane/core/pipeline/state.ex @@ -34,8 +34,8 @@ defmodule Membrane.Core.Pipeline.State do setup_incomplete?: boolean(), stalker: Membrane.Core.Stalker.t(), subprocess_supervisor: pid(), - awaiting_setup_completition?: boolean(), - diamond_detection_triggered?: boolean() + awaiting_setup_completition?: boolean() + # diamond_detection_triggered?: boolean() } # READ THIS BEFORE ADDING NEW FIELD!!! @@ -59,7 +59,7 @@ defmodule Membrane.Core.Pipeline.State do stalker: nil, resource_guard: nil, subprocess_supervisor: nil, - awaiting_setup_completition?: false, - diamond_detection_triggered?: false + awaiting_setup_completition?: false + # diamond_detection_triggered?: false end From 0be84b9ef96be4937cbb23aef03b001bdbe9c97a Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Thu, 21 Nov 2024 14:58:04 +0100 Subject: [PATCH 07/32] Trigger diamon detection between elements WiP --- .../element/diamond_detection_controller.ex | 75 ++++++++++++++++++- lib/membrane/core/element/state.ex | 6 +- .../core/parent/child_life_controller.ex | 2 + 3 files changed, 78 insertions(+), 5 deletions(-) diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex index 7ea9a0c1f..edd32b2e0 100644 --- a/lib/membrane/core/element/diamond_detection_controller.ex +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -20,12 +20,12 @@ defmodule Membrane.Core.Element.DiamondDetectionController do def continue_diamond_detection(diamond_detection_ref, diamond_detecton_path, state) do cond do not is_map_key(state.diamond_detection_ref_to_path, diamond_detection_ref) -> - delete_message = Message.new(:delete_diamond_detection_ref, diamond_detection_ref) - send_after_time = Membrane.Time.seconds(10) |> Membrane.Time.as_milliseconds(:round) - self() |> Process.send_after(delete_message, send_after_time) - :ok = forward_diamond_detection(diamond_detection_ref, diamond_detecton_path, state) + :ok = + Message.new(:delete_diamond_detection_ref, diamond_detection_ref) + |> send_after_to_self() + state |> put_in( [:diamond_detection_ref_to_path, diamond_detection_ref], @@ -68,6 +68,15 @@ defmodule Membrane.Core.Element.DiamondDetectionController do end) end + defp forward_diamond_detection_trigger(trigger_ref, state) do + state.pads_data + |> Enum.each(fn {_pad_ref, %PadData{} = pad_data} -> + if pad_data.direction == :output and pad_data.flow_control != :push do + Message.send(pad_data.pid, :diamond_detection_trigger, trigger_ref) + end + end) + end + defp is_output_pull_pad(%PadData{} = pad_data, auto_pull_mode?) do pad_data.direction == :output and (pad_data.flow_control == :manual or @@ -78,4 +87,62 @@ defmodule Membrane.Core.Element.DiamondDetectionController do uniq_length = diamond_detection_path |> Enum.uniq() |> length() uniq_length < length(diamond_detection_path) end + + def start_diamond_detection_trigger(spec_ref, state) when map_size(state.pads_data) >= 2 do + handle_diamond_detection_trigger(spec_ref, state) + end + + def start_diamond_detection_trigger(_spec_ref, state), do: state + + def handle_diamond_detection_trigger(trigger_ref, %State{} = state) do + if MapSet.member?(state.diamond_detection_trigger_refs, trigger_ref), + do: state, + else: do_handle_diamond_detection_trigger(trigger_ref, state) + end + + defp do_handle_diamond_detection_trigger(trigger_ref, %State{} = state) do + state = + state + |> Map.update!(:diamond_detection_trigger_refs, &MapSet.put(&1, trigger_ref)) + + :ok = + Message.new(:delete_diamond_detection_trigger_ref, trigger_ref) + |> send_after_to_self() + + :ok = forward_diamond_detection_trigger(trigger_ref, state) + + if output_pull_arity(state) >= 2, + do: postpone_diamond_detection(state), + else: state + end + + defp postpone_diamond_detection(%State{} = state) when state.diamond_detection_postponed? do + state + end + + defp postpone_diamond_detection(%State{} = state) do + :ok = + Message.new(:start_diamond_detection) + |> send_after_to_self(1) + + %{state | diamond_detection_postponed?: true} + end + + def delete_diamond_detection_trigger_ref(trigger_ref, state) do + state + |> Map.update!(:diamond_detection_trigger_refs, &MapSet.delete(&1, trigger_ref)) + end + + defp output_pull_arity(state) do + auto_pull_mode? = state.effective_flow_control == :pull + + state.pads_data + |> Enum.count(fn {_pad_ref, pad_data} -> is_output_pull_pad(pad_data, auto_pull_mode?) end) + end + + defp send_after_to_self(message, seconds \\ 10) do + send_after_time = Membrane.Time.seconds(seconds) |> Membrane.Time.as_milliseconds(:round) + self() |> Process.send_after(message, send_after_time) + :ok + end end diff --git a/lib/membrane/core/element/state.ex b/lib/membrane/core/element/state.ex index 83c7faa44..d99155ac2 100644 --- a/lib/membrane/core/element/state.ex +++ b/lib/membrane/core/element/state.ex @@ -49,7 +49,9 @@ defmodule Membrane.Core.Element.State do resume_delayed_demands_loop_in_mailbox?: boolean(), diamond_detection_ref_to_path: %{ optional(reference()) => DiamondDetectionController.diamond_detection_path() - } + }, + diamond_detection_trigger_refs: MapSet.t(reference()), + diamond_detection_postponed?: boolean() } # READ THIS BEFORE ADDING NEW FIELD!!! @@ -83,6 +85,8 @@ defmodule Membrane.Core.Element.State do pads_to_snapshot: MapSet.new(), playback_queue: [], diamond_detection_ref_to_path: %{}, + diamond_detection_trigger_refs: MapSet.new(), + diamond_detection_postponed?: false, pads_data: %{}, satisfied_auto_output_pads: MapSet.new(), awaiting_auto_input_pads: MapSet.new(), diff --git a/lib/membrane/core/parent/child_life_controller.ex b/lib/membrane/core/parent/child_life_controller.ex index c571bdf82..f98db44fc 100644 --- a/lib/membrane/core/parent/child_life_controller.ex +++ b/lib/membrane/core/parent/child_life_controller.ex @@ -317,6 +317,8 @@ defmodule Membrane.Core.Parent.ChildLifeController do withl spec_data: {:ok, spec_data} <- Map.fetch(state.pending_specs, spec_ref), do: {spec_data, state} = do_proceed_spec_startup(spec_ref, spec_data, state), status: :ready <- spec_data.status do + + cleanup_spec_startup(spec_ref, state) else spec_data: :error -> state From 58c5384e2c78440b4288dce9d6070762f87f466c Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Fri, 22 Nov 2024 14:56:12 +0100 Subject: [PATCH 08/32] Trigger diamon detection between elements WiP --- lib/membrane/core/bin.ex | 10 ------- lib/membrane/core/element.ex | 15 +++++++++++ .../element/diamond_detection_controller.ex | 22 +++++++++------ .../core/parent/child_life_controller.ex | 25 +++++++---------- .../parent/diamond_detection_controller.ex | 26 +++++++++--------- lib/membrane/core/pipeline.ex | 10 ------- .../pipeline/diamond_detection_controller.ex | 27 ------------------- 7 files changed, 52 insertions(+), 83 deletions(-) delete mode 100644 lib/membrane/core/pipeline/diamond_detection_controller.ex diff --git a/lib/membrane/core/bin.ex b/lib/membrane/core/bin.ex index 1d5d995f5..61b6862b0 100644 --- a/lib/membrane/core/bin.ex +++ b/lib/membrane/core/bin.ex @@ -239,16 +239,6 @@ defmodule Membrane.Core.Bin do {:noreply, state} end - # defp do_handle_info(Message.new(:start_diamond_detection), state) do - # :ok = Parent.DiamondDetectionController.start_diamond_detection(state) - # {:noreply, state} - # end - - # defp do_handle_info(Message.new(:trigger_diamond_detection), state) do - # :ok = __MODULE__.DiamondDetectionController.trigger_diamond_detection(state) - # {:noreply, state} - # end - defp do_handle_info(Message.new(:child_death, [name, reason]), state) do case Parent.ChildLifeController.handle_child_death(name, reason, state) do {:stop, reason, _state} -> ProcessHelper.notoelo(reason) diff --git a/lib/membrane/core/element.ex b/lib/membrane/core/element.ex index 278169646..35a0be04e 100644 --- a/lib/membrane/core/element.ex +++ b/lib/membrane/core/element.ex @@ -306,6 +306,21 @@ defmodule Membrane.Core.Element do {:noreply, state} end + defp do_handle_info(Message.new(:start_diamond_detection_trigger, trigger_ref), state) do + state = DiamondDetectionController.start_diamond_detection_trigger(trigger_ref, state) + {:noreply, state} + end + + defp do_handle_info(Message.new(:diamond_detection_trigger, trigger_ref), state) do + state = DiamondDetectionController.handle_diamond_detection_trigger(trigger_ref, state) + {:noreply, state} + end + + defp do_handle_info(Message.new(:delete_diamond_detection_trigger_ref, trigger_ref), state) do + state = DiamondDetectionController.delete_diamond_detection_trigger_ref(trigger_ref, state) + {:noreply, state} + end + defp do_handle_info(Message.new(:terminate), state) do state = LifecycleController.handle_terminate_request(state) {:noreply, state} diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex index edd32b2e0..d14fd3e4e 100644 --- a/lib/membrane/core/element/diamond_detection_controller.ex +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -8,6 +8,8 @@ defmodule Membrane.Core.Element.DiamondDetectionController do alias Membrane.Core.Element.State alias Membrane.Element.PadData + # TODO: don't forward diamond detection and triggers in endpoints + @type diamond_detection_path :: [{pid(), Child.name(), Pad.ref()}] @spec start_diamond_detection(State.t()) :: :ok @@ -71,7 +73,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do defp forward_diamond_detection_trigger(trigger_ref, state) do state.pads_data |> Enum.each(fn {_pad_ref, %PadData{} = pad_data} -> - if pad_data.direction == :output and pad_data.flow_control != :push do + if pad_data.direction == :input and pad_data.flow_control != :push do Message.send(pad_data.pid, :diamond_detection_trigger, trigger_ref) end end) @@ -88,16 +90,20 @@ defmodule Membrane.Core.Element.DiamondDetectionController do uniq_length < length(diamond_detection_path) end - def start_diamond_detection_trigger(spec_ref, state) when map_size(state.pads_data) >= 2 do - handle_diamond_detection_trigger(spec_ref, state) + def start_diamond_detection_trigger(spec_ref, state) do + if map_size(state.pads_data) < 2 or + MapSet.member?(state.diamond_detection_trigger_refs, spec_ref) do + state + else + do_handle_diamond_detection_trigger(spec_ref, state) + end end - def start_diamond_detection_trigger(_spec_ref, state), do: state - def handle_diamond_detection_trigger(trigger_ref, %State{} = state) do - if MapSet.member?(state.diamond_detection_trigger_refs, trigger_ref), - do: state, - else: do_handle_diamond_detection_trigger(trigger_ref, state) + if state.type == :endpoint or + MapSet.member?(state.diamond_detection_trigger_refs, trigger_ref), + do: state, + else: do_handle_diamond_detection_trigger(trigger_ref, state) end defp do_handle_diamond_detection_trigger(trigger_ref, %State{} = state) do diff --git a/lib/membrane/core/parent/child_life_controller.ex b/lib/membrane/core/parent/child_life_controller.ex index f98db44fc..9b1ba56de 100644 --- a/lib/membrane/core/parent/child_life_controller.ex +++ b/lib/membrane/core/parent/child_life_controller.ex @@ -11,6 +11,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do ChildEntryParser, ChildrenModel, ClockHandler, + DiamondDetectionController, Link, SpecificationParser } @@ -163,23 +164,10 @@ defmodule Membrane.Core.Parent.ChildLifeController do awaiting_responses: MapSet.new() }) - # |> trigger_diamond_detection() - state = StartupUtils.exec_handle_spec_started(all_children_names, state) proceed_spec_startup(spec_ref, state) end - # defp trigger_diamond_detection(state) do - # cond do - # Component.bin?(state) -> - # :ok = Bin.DiamondDetectionController.trigger_diamond_detection(state) - # state - - # Component.pipeline?(state) -> - # Pipeline.DiamondDetectionController.trigger_diamond_detection(state) - # end - # end - defp assert_all_static_pads_linked_in_spec!(children_definitions, links) do pads_to_link = links @@ -317,8 +305,6 @@ defmodule Membrane.Core.Parent.ChildLifeController do withl spec_data: {:ok, spec_data} <- Map.fetch(state.pending_specs, spec_ref), do: {spec_data, state} = do_proceed_spec_startup(spec_ref, spec_data, state), status: :ready <- spec_data.status do - - cleanup_spec_startup(spec_ref, state) else spec_data: :error -> state @@ -440,7 +426,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do end end - defp do_proceed_spec_startup(_spec_ref, %{status: :ready} = spec_data, state) do + defp do_proceed_spec_startup(spec_ref, %{status: :ready} = spec_data, state) do state = Enum.reduce(spec_data.children_names, state, fn child, state -> %{pid: pid, terminating?: terminating?} = state.children[child] @@ -454,6 +440,13 @@ defmodule Membrane.Core.Parent.ChildLifeController do put_in(state.children[child].ready?, true) end) + spec_data.links_ids + |> Enum.map(&state.links[&1].from.name) + |> Enum.uniq() + |> Enum.each(fn child_name -> + DiamondDetectionController.start_diamond_detection_trigger(child_name, spec_ref, state) + end) + state = with %{playback: :playing} <- state do handle_children_playing(spec_data.children_names, state) diff --git a/lib/membrane/core/parent/diamond_detection_controller.ex b/lib/membrane/core/parent/diamond_detection_controller.ex index 8c4e4a36c..75ce95b4a 100644 --- a/lib/membrane/core/parent/diamond_detection_controller.ex +++ b/lib/membrane/core/parent/diamond_detection_controller.ex @@ -1,15 +1,17 @@ -# defmodule Membrane.Core.Parent.DiamondDetectionController do -# @moduledoc false +defmodule Membrane.Core.Parent.DiamondDetectionController do + @moduledoc false -# require Membrane.Core.Message, as: Message + require Membrane.Core.Message, as: Message -# alias Membrane.Core.Parent + alias Membrane.Child + alias Membrane.Core.Parent -# @spec start_diamond_detection(Parent.state()) :: :ok -# def start_diamond_detection(state) do -# state.children -# |> Enum.each(fn {_child, %{pid: child_pid}} -> -# Message.send(child_pid, :start_diamond_detection) -# end) -# end -# end + @spec start_diamond_detection_trigger(Child.name(), reference(), Parent.state()) :: :ok + def start_diamond_detection_trigger(child_name, trigger_ref, state) do + with %{component_type: :element, pid: pid} <- state.children[child_name] do + Message.send(pid, :start_diamond_detection_trigger, trigger_ref) + end + + :ok + end +end diff --git a/lib/membrane/core/pipeline.ex b/lib/membrane/core/pipeline.ex index bb865541e..6f9fc95db 100644 --- a/lib/membrane/core/pipeline.ex +++ b/lib/membrane/core/pipeline.ex @@ -130,16 +130,6 @@ defmodule Membrane.Core.Pipeline do {:noreply, state} end - # defp do_handle_info(Message.new(:start_diamond_detection), state) do - # state = __MODULE__.DiamondDetectionController.start_diamond_detection(state) - # {:noreply, state} - # end - - # defp do_handle_info(Message.new(:trigger_diamond_detection), state) do - # state = __MODULE__.DiamondDetectionController.trigger_diamond_detection(state) - # {:noreply, state} - # end - defp do_handle_info(Message.new(:initialized, child), state) do state = ChildLifeController.handle_child_initialized(child, state) {:noreply, state} diff --git a/lib/membrane/core/pipeline/diamond_detection_controller.ex b/lib/membrane/core/pipeline/diamond_detection_controller.ex deleted file mode 100644 index 8a8e352e2..000000000 --- a/lib/membrane/core/pipeline/diamond_detection_controller.ex +++ /dev/null @@ -1,27 +0,0 @@ -# defmodule Membrane.Core.Pipeline.DiamondDetectionController do -# @moduledoc false - -# require Membrane.Core.Message, as: Message - -# alias Membrane.Core.Parent -# alias Membrane.Core.Pipeline.State - -# @spec trigger_diamond_detection(State.t()) :: State.t() -# def trigger_diamond_detection(%State{} = state) when state.diamond_detection_triggered? do -# state -# end - -# def trigger_diamond_detection(%State{} = state) do -# message = Message.new(:start_diamond_detection) -# send_after_timeout = Membrane.Time.second() |> Membrane.Time.as_milliseconds(:round) -# self() |> Process.send_after(message, send_after_timeout) - -# %{state | diamond_detection_triggered?: true} -# end - -# @spec start_diamond_detection(State.t()) :: State.t() -# def start_diamond_detection(%State{} = state) do -# :ok = Parent.DiamondDetectionController.start_diamond_detection(state) -# %{state | diamond_detection_triggered?: false} -# end -# end From 7b5fdad667c03f8097b4a453e7cbe979fa41c2e5 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Fri, 22 Nov 2024 15:16:37 +0100 Subject: [PATCH 09/32] Fix typos --- lib/membrane/core/parent/child_life_controller.ex | 2 +- lib/membrane/core/parent/link_endpoint.ex | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/membrane/core/parent/child_life_controller.ex b/lib/membrane/core/parent/child_life_controller.ex index 9b1ba56de..13014be02 100644 --- a/lib/membrane/core/parent/child_life_controller.ex +++ b/lib/membrane/core/parent/child_life_controller.ex @@ -441,7 +441,7 @@ defmodule Membrane.Core.Parent.ChildLifeController do end) spec_data.links_ids - |> Enum.map(&state.links[&1].from.name) + |> Enum.map(&state.links[&1].from.child) |> Enum.uniq() |> Enum.each(fn child_name -> DiamondDetectionController.start_diamond_detection_trigger(child_name, spec_ref, state) diff --git a/lib/membrane/core/parent/link_endpoint.ex b/lib/membrane/core/parent/link_endpoint.ex index c828e8f8d..ca5cb1f27 100644 --- a/lib/membrane/core/parent/link_endpoint.ex +++ b/lib/membrane/core/parent/link_endpoint.ex @@ -3,14 +3,14 @@ defmodule Membrane.Core.Parent.Link.Endpoint do use Bunch.Access - alias Membrane.{Element, Pad} + alias Membrane.{Child, Pad} @enforce_keys [:child, :pad_spec] defstruct @enforce_keys ++ [pad_ref: nil, pid: nil, pad_props: [], pad_info: %{}, child_spec_ref: nil] @type t() :: %__MODULE__{ - child: Element.name() | {Membrane.Bin, :itself}, + child: Child.name() | {Membrane.Bin, :itself}, pad_spec: Pad.name() | Pad.ref(), pad_ref: Pad.ref(), pid: pid(), From 646898c0004a28bb42ed3c26ee64a5fc34c23822 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Fri, 22 Nov 2024 16:22:41 +0100 Subject: [PATCH 10/32] wip --- lib/membrane/core/element/diamond_detection_controller.ex | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex index d14fd3e4e..cd2bdd227 100644 --- a/lib/membrane/core/element/diamond_detection_controller.ex +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -59,7 +59,9 @@ defmodule Membrane.Core.Element.DiamondDetectionController do state.pads_data |> Enum.each(fn {pad_ref, pad_data} -> if is_output_pull_pad(pad_data, auto_pull_mode?) do - diamond_detection_path = [{self(), state.name, pad_ref} | diamond_detection_path] + name = state.name + # name = Membrane.ComponentPath.get() + diamond_detection_path = [{self(), name, pad_ref} | diamond_detection_path] Message.send( pad_data.pid, From 30b757ec0be2079e599a735e6a89aaf2ab37b6ac Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Mon, 25 Nov 2024 14:50:00 +0100 Subject: [PATCH 11/32] Log diamond wip --- lib/membrane/core/element.ex | 7 +- .../element/diamond_detection_controller.ex | 69 ++++++++++++++++--- 2 files changed, 64 insertions(+), 12 deletions(-) diff --git a/lib/membrane/core/element.ex b/lib/membrane/core/element.ex index 35a0be04e..b3966bb29 100644 --- a/lib/membrane/core/element.ex +++ b/lib/membrane/core/element.ex @@ -288,11 +288,16 @@ defmodule Membrane.Core.Element do end defp do_handle_info( - Message.new(:diamond_detection, [diamond_detection_ref, diamond_detection_path]), + Message.new(:diamond_detection, [ + input_pad_ref, + diamond_detection_ref, + diamond_detection_path + ]), state ) do state = DiamondDetectionController.continue_diamond_detection( + input_pad_ref, diamond_detection_ref, diamond_detection_path, state diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex index cd2bdd227..d65880b15 100644 --- a/lib/membrane/core/element/diamond_detection_controller.ex +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -2,24 +2,55 @@ defmodule Membrane.Core.Element.DiamondDetectionController do @moduledoc false require Membrane.Core.Message, as: Message + require Membrane.Logger require Membrane.Pad, as: Pad - alias Membrane.Child alias Membrane.Core.Element.State alias Membrane.Element.PadData - # TODO: don't forward diamond detection and triggers in endpoints + @component_path_suffix "__membrane_component_path_64_byte_suffix________________________" + @not_a_pad :__membrane_not_a_pad__ - @type diamond_detection_path :: [{pid(), Child.name(), Pad.ref()}] + @type diamond_detection_path_entry :: %{ + pid: pid(), + component_path: String.t(), + input_pad_ref: Pad.ref(), + output_pad_ref: Pad.ref() + } + + @type diamond_detection_path() :: [diamond_detection_path_entry()] @spec start_diamond_detection(State.t()) :: :ok def start_diamond_detection(state) do + diamond_detection_path = [ + %{ + pid: self(), + component_path: get_component_path(), + input_pad_ref: @not_a_pad, + output_pad_ref: nil + } + ] + make_ref() - |> forward_diamond_detection([], state) + |> forward_diamond_detection(diamond_detection_path, state) end - @spec continue_diamond_detection(reference(), diamond_detection_path(), State.t()) :: State.t() - def continue_diamond_detection(diamond_detection_ref, diamond_detecton_path, state) do + @spec continue_diamond_detection(Pad.ref(), reference(), diamond_detection_path(), State.t()) :: State.t() + def continue_diamond_detection( + input_pad_ref, + diamond_detection_ref, + diamond_detecton_path, + state + ) do + new_path_entry = %{ + pid: self(), + component_path: get_component_path(), + input_pad_ref: input_pad_ref, + output_pad_ref: @not_a_pad + } + + diamond_detecton_path = [new_path_entry | diamond_detecton_path] + cond do not is_map_key(state.diamond_detection_ref_to_path, diamond_detection_ref) -> :ok = forward_diamond_detection(diamond_detection_ref, diamond_detecton_path, state) @@ -37,6 +68,9 @@ defmodule Membrane.Core.Element.DiamondDetectionController do has_cycle?(diamond_detecton_path) -> state + have_common_prefix?(diamond_detecton_path, state.diamond_detection_ref_to_path[diamond_detection_ref]) -> + state + true -> # todo: log diamond state @@ -55,18 +89,18 @@ defmodule Membrane.Core.Element.DiamondDetectionController do @spec forward_diamond_detection(reference(), diamond_detection_path(), State.t()) :: :ok defp forward_diamond_detection(diamond_detection_ref, diamond_detection_path, state) do auto_pull_mode? = state.effective_flow_control == :pull + [current_entry | diamond_detection_path_tail] = diamond_detection_path state.pads_data |> Enum.each(fn {pad_ref, pad_data} -> if is_output_pull_pad(pad_data, auto_pull_mode?) do - name = state.name - # name = Membrane.ComponentPath.get() - diamond_detection_path = [{self(), name, pad_ref} | diamond_detection_path] + current_entry = %{current_entry | output_pad_ref: pad_ref} + diamond_detection_path = [current_entry | diamond_detection_path_tail] Message.send( pad_data.pid, :diamond_detection, - [diamond_detection_ref, diamond_detection_path] + [pad_data.other_ref, diamond_detection_ref, diamond_detection_path] ) end end) @@ -88,7 +122,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do end defp has_cycle?(diamond_detection_path) do - uniq_length = diamond_detection_path |> Enum.uniq() |> length() + uniq_length = diamond_detection_path |> Enum.uniq_by(& &1.pid) |> length() uniq_length < length(diamond_detection_path) end @@ -153,4 +187,17 @@ defmodule Membrane.Core.Element.DiamondDetectionController do self() |> Process.send_after(message, send_after_time) :ok end + + defp get_component_path() do + # adding @component_path_suffix to component path will cause that component path will + # always have more than 64 bytes, so it won't be copied during sending a message + (Membrane.ComponentPath.get() ++ [@component_path_suffix]) + |> Enum.join() + end + + defp have_common_prefix?(path_a, path_b), do: List.last(path_a) == List.last(path_b) + + defp log_diamond(path_a, path_b) do + + end end From 39030a76ab10e84ce8bd1e215167b87c2d76e2cc Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Tue, 26 Nov 2024 13:19:27 +0100 Subject: [PATCH 12/32] Add test wip --- .../element/diamond_detection_controller.ex | 52 +++++------ .../diamond_logger.ex | 13 +++ .../diamond_logger_impl.ex | 37 ++++++++ .../path_in_grapth.ex | 19 ++++ mix.exs | 1 + mix.lock | 2 + .../integration/diamond_detection_test.exs | 89 +++++++++++++++++++ 7 files changed, 187 insertions(+), 26 deletions(-) create mode 100644 lib/membrane/core/element/diamond_detection_controller/diamond_logger.ex create mode 100644 lib/membrane/core/element/diamond_detection_controller/diamond_logger_impl.ex create mode 100644 lib/membrane/core/element/diamond_detection_controller/path_in_grapth.ex create mode 100644 test/membrane/integration/diamond_detection_test.exs diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex index d65880b15..f2346c0a9 100644 --- a/lib/membrane/core/element/diamond_detection_controller.ex +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -5,51 +5,37 @@ defmodule Membrane.Core.Element.DiamondDetectionController do require Membrane.Logger require Membrane.Pad, as: Pad + alias __MODULE__.{DiamondLogger, PathInGraph} alias Membrane.Core.Element.State alias Membrane.Element.PadData @component_path_suffix "__membrane_component_path_64_byte_suffix________________________" - @not_a_pad :__membrane_not_a_pad__ - - @type diamond_detection_path_entry :: %{ - pid: pid(), - component_path: String.t(), - input_pad_ref: Pad.ref(), - output_pad_ref: Pad.ref() - } - - @type diamond_detection_path() :: [diamond_detection_path_entry()] @spec start_diamond_detection(State.t()) :: :ok def start_diamond_detection(state) do diamond_detection_path = [ - %{ - pid: self(), - component_path: get_component_path(), - input_pad_ref: @not_a_pad, - output_pad_ref: nil - } + %PathInGraph.Vertex{pid: self(), component_path: get_component_path()} ] make_ref() |> forward_diamond_detection(diamond_detection_path, state) end - @spec continue_diamond_detection(Pad.ref(), reference(), diamond_detection_path(), State.t()) :: State.t() + @spec continue_diamond_detection(Pad.ref(), reference(), PathInGraph.t(), State.t()) :: + State.t() def continue_diamond_detection( input_pad_ref, diamond_detection_ref, diamond_detecton_path, state ) do - new_path_entry = %{ + new_path_vertex = %PathInGraph.Vertex{ pid: self(), component_path: get_component_path(), - input_pad_ref: input_pad_ref, - output_pad_ref: @not_a_pad + input_pad_ref: input_pad_ref } - diamond_detecton_path = [new_path_entry | diamond_detecton_path] + diamond_detecton_path = [new_path_vertex | diamond_detecton_path] cond do not is_map_key(state.diamond_detection_ref_to_path, diamond_detection_ref) -> @@ -68,11 +54,19 @@ defmodule Membrane.Core.Element.DiamondDetectionController do has_cycle?(diamond_detecton_path) -> state - have_common_prefix?(diamond_detecton_path, state.diamond_detection_ref_to_path[diamond_detection_ref]) -> + have_common_prefix?( + diamond_detecton_path, + state.diamond_detection_ref_to_path[diamond_detection_ref] + ) -> state true -> - # todo: log diamond + :ok = + DiamondLogger.log_diamond( + diamond_detecton_path |> cut_suffixes_in_path(), + state.diamond_detection_ref_to_path[diamond_detection_ref] |> cut_suffixes_in_path() + ) + state end end @@ -86,7 +80,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do ) end - @spec forward_diamond_detection(reference(), diamond_detection_path(), State.t()) :: :ok + @spec forward_diamond_detection(reference(), PathInGraph.t(), State.t()) :: :ok defp forward_diamond_detection(diamond_detection_ref, diamond_detection_path, state) do auto_pull_mode? = state.effective_flow_control == :pull [current_entry | diamond_detection_path_tail] = diamond_detection_path @@ -126,6 +120,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do uniq_length < length(diamond_detection_path) end + @spec start_diamond_detection_trigger(reference(), State.t()) :: State.t() def start_diamond_detection_trigger(spec_ref, state) do if map_size(state.pads_data) < 2 or MapSet.member?(state.diamond_detection_trigger_refs, spec_ref) do @@ -135,6 +130,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do end end + @spec handle_diamond_detection_trigger(reference(), State.t()) :: State.t() def handle_diamond_detection_trigger(trigger_ref, %State{} = state) do if state.type == :endpoint or MapSet.member?(state.diamond_detection_trigger_refs, trigger_ref), @@ -170,6 +166,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do %{state | diamond_detection_postponed?: true} end + @spec delete_diamond_detection_trigger_ref(reference(), State.t()) :: State.t() def delete_diamond_detection_trigger_ref(trigger_ref, state) do state |> Map.update!(:diamond_detection_trigger_refs, &MapSet.delete(&1, trigger_ref)) @@ -197,7 +194,10 @@ defmodule Membrane.Core.Element.DiamondDetectionController do defp have_common_prefix?(path_a, path_b), do: List.last(path_a) == List.last(path_b) - defp log_diamond(path_a, path_b) do - + defp cut_suffixes_in_path(path_in_graph) do + path_in_graph + |> Enum.map( + &%{&1 | component_path: String.replace(&1.component_path, @component_path_suffix, "")} + ) end end diff --git a/lib/membrane/core/element/diamond_detection_controller/diamond_logger.ex b/lib/membrane/core/element/diamond_detection_controller/diamond_logger.ex new file mode 100644 index 000000000..0b97e2dcb --- /dev/null +++ b/lib/membrane/core/element/diamond_detection_controller/diamond_logger.ex @@ -0,0 +1,13 @@ +defmodule Membrane.Core.Element.DiamondDetectionController.DiamondLogger do + @moduledoc false + + alias Membrane.Core.Element.DiamondDetectionController.{ + DiamondLoggerImpl, + PathInGraph + } + + @callback log_diamond(PathInGraph.t(), PathInGraph.t()) :: :ok + + def log_diamond(path_a, path_b), do: impl().log_diamond(path_a, path_b) + defp impl(), do: Application.get_env(:membrane_core, :diamond_logger, DiamondLoggerImpl) +end diff --git a/lib/membrane/core/element/diamond_detection_controller/diamond_logger_impl.ex b/lib/membrane/core/element/diamond_detection_controller/diamond_logger_impl.ex new file mode 100644 index 000000000..6048f8849 --- /dev/null +++ b/lib/membrane/core/element/diamond_detection_controller/diamond_logger_impl.ex @@ -0,0 +1,37 @@ +defmodule Membrane.Core.Element.DiamondDetectionController.DiamondLoggerImpl do + @moduledoc false + + require Membrane.Logger + alias Membrane.Core.Element.DiamondDetectionController + alias Membrane.Core.Element.DiamondDetectionController.DiamondLogger + alias Membrane.Core.Element.DiamondDetectionController.PathInGraph.Vertex + + @behaviour DiamondLogger + + @impl DiamondLogger + def log_diamond(path_a, path_b) do + Membrane.Logger.debug(""" + DIAMOND + + Path A: + #{inspect_path(path_a)} + + Path B: + #{inspect_path(path_b)} + """) + + :ok + end + + defp inspect_path(path) do + path + |> Enum.reverse() + |> Enum.chunk_every(2, 1, :discard) + |> Enum.map_join("\n", fn [%Vertex{} = from, %Vertex{} = to] -> + """ + From #{from.component_path} via output pad #{inspect(from.output_pad_ref)} \ + to #{to.component_path} via input pad #{inspect(to.input_pad_ref)}. + """ + end) + end +end diff --git a/lib/membrane/core/element/diamond_detection_controller/path_in_grapth.ex b/lib/membrane/core/element/diamond_detection_controller/path_in_grapth.ex new file mode 100644 index 000000000..bf0b0c63c --- /dev/null +++ b/lib/membrane/core/element/diamond_detection_controller/path_in_grapth.ex @@ -0,0 +1,19 @@ +defmodule Membrane.Core.Element.DiamondDetectionController.PathInGraph do + @moduledoc false + + defmodule Vertex do + @moduledoc false + require Membrane.Pad, as: Pad + + defstruct [:pid, :component_path, :input_pad_ref, :output_pad_ref] + + @type t :: %__MODULE__{ + pid: pid(), + component_path: String.t(), + input_pad_ref: Pad.ref() | nil, + output_pad_ref: Pad.ref() | nil + } + end + + @type t :: [Vertex.t()] +end diff --git a/mix.exs b/mix.exs index c1fab8be5..77b38ce84 100644 --- a/mix.exs +++ b/mix.exs @@ -152,6 +152,7 @@ defmodule Membrane.Mixfile do {:credo, "~> 1.7", only: :dev, runtime: false}, # Testing {:mox, "~> 1.0", only: :test}, + {:mock, "~> 0.3.8", only: :test}, {:junit_formatter, "~> 3.1", only: :test}, {:excoveralls, "~> 0.14", only: :test} ] diff --git a/mix.lock b/mix.lock index d7ed3e717..c59ec0d56 100644 --- a/mix.lock +++ b/mix.lock @@ -15,6 +15,8 @@ "makeup_diff": {:hex, :makeup_diff, "0.1.1", "01498f8c95970081297837eaf4686b6f3813e535795b8421f15ace17a59aea37", [:mix], [{:makeup, "~> 1.0", [hex: :makeup, repo: "hexpm", optional: false]}], "hexpm", "fadb0bf014bd328badb7be986eadbce1a29955dd51c27a9e401c3045cf24184e"}, "makeup_elixir": {:hex, :makeup_elixir, "0.16.2", "627e84b8e8bf22e60a2579dad15067c755531fea049ae26ef1020cad58fe9578", [:mix], [{:makeup, "~> 1.0", [hex: :makeup, repo: "hexpm", optional: false]}, {:nimble_parsec, "~> 1.2.3 or ~> 1.3", [hex: :nimble_parsec, repo: "hexpm", optional: false]}], "hexpm", "41193978704763f6bbe6cc2758b84909e62984c7752b3784bd3c218bb341706b"}, "makeup_erlang": {:hex, :makeup_erlang, "1.0.1", "c7f58c120b2b5aa5fd80d540a89fdf866ed42f1f3994e4fe189abebeab610839", [:mix], [{:makeup, "~> 1.0", [hex: :makeup, repo: "hexpm", optional: false]}], "hexpm", "8a89a1eeccc2d798d6ea15496a6e4870b75e014d1af514b1b71fa33134f57814"}, + "meck": {:hex, :meck, "0.9.2", "85ccbab053f1db86c7ca240e9fc718170ee5bda03810a6292b5306bf31bae5f5", [:rebar3], [], "hexpm", "81344f561357dc40a8344afa53767c32669153355b626ea9fcbc8da6b3045826"}, + "mock": {:hex, :mock, "0.3.8", "7046a306b71db2488ef54395eeb74df0a7f335a7caca4a3d3875d1fc81c884dd", [:mix], [{:meck, "~> 0.9.2", [hex: :meck, repo: "hexpm", optional: false]}], "hexpm", "7fa82364c97617d79bb7d15571193fc0c4fe5afd0c932cef09426b3ee6fe2022"}, "mox": {:hex, :mox, "1.2.0", "a2cd96b4b80a3883e3100a221e8adc1b98e4c3a332a8fc434c39526babafd5b3", [:mix], [{:nimble_ownership, "~> 1.0", [hex: :nimble_ownership, repo: "hexpm", optional: false]}], "hexpm", "c7b92b3cc69ee24a7eeeaf944cd7be22013c52fcb580c1f33f50845ec821089a"}, "nimble_ownership": {:hex, :nimble_ownership, "1.0.0", "3f87744d42c21b2042a0aa1d48c83c77e6dd9dd357e425a038dd4b49ba8b79a1", [:mix], [], "hexpm", "7c16cc74f4e952464220a73055b557a273e8b1b7ace8489ec9d86e9ad56cb2cc"}, "nimble_parsec": {:hex, :nimble_parsec, "1.4.0", "51f9b613ea62cfa97b25ccc2c1b4216e81df970acd8e16e8d1bdc58fef21370d", [:mix], [], "hexpm", "9c565862810fb383e9838c1dd2d7d2c437b3d13b267414ba6af33e50d2d1cf28"}, diff --git a/test/membrane/integration/diamond_detection_test.exs b/test/membrane/integration/diamond_detection_test.exs new file mode 100644 index 000000000..4616a1e5b --- /dev/null +++ b/test/membrane/integration/diamond_detection_test.exs @@ -0,0 +1,89 @@ +defmodule Membrane.Integration.DiamondDetectionTest do + use ExUnit.Case, async: false + + import ExUnit.Assertions + import Membrane.ChildrenSpec + import Mock + + require Membrane.Pad, as: Pad + + alias Membrane.Core.Element.DiamondDetectionController.{DiamondLoggerImpl, PathInGraph} + alias Membrane.Core.Element.DiamondDetectionController.PathInGraph.Vertex + alias Membrane.Testing + + defmodule Node do + use Membrane.Filter + + def_input_pad :input, + accepted_format: _any, + availability: :on_request, + flow_control: :manual, + demand_unit: :buffers + + def_output_pad :output, + accepted_format: _any, + availability: :on_request, + flow_control: :manual + + @impl true + def handle_demand(_pad, _demand, _unit, _ctx, state), do: {[], state} + end + + test "simple scenario" do + with_mock DiamondLoggerImpl, log_diamond: fn _path_a, _path_b -> :ok end do + spec = [ + child(:source, Node) + |> via_out(Pad.ref(:output, 1)) + |> via_in(Pad.ref(:input, 1)) + |> child(:filter, Node) + |> via_out(Pad.ref(:output, 1)) + |> via_in(Pad.ref(:input, 1)) + |> child(:sink, Node), + get_child(:source) + |> via_out(Pad.ref(:output, 2)) + |> via_in(Pad.ref(:input, 2)) + |> get_child(:sink) + ] + + pipeline = Testing.Pipeline.start_link_supervised!(spec: spec) + + Process.sleep(1500) + + assert [{_element_pid, {DiamondLoggerImpl, :log_diamond, [path_a, path_b]}, :ok}] = + call_history(DiamondLoggerImpl) + + "#PID" <> inspected_pipeline = inspect(pipeline) + + [shorter_path, longer_path] = Enum.sort_by([path_a, path_b], &length/1) + + assert [ + %Vertex{ + component_path: ^inspected_pipeline <> "/:sink", + input_pad_ref: {Membrane.Pad, :input, 1} + }, + %Vertex{ + component_path: ^inspected_pipeline <> "/:filter", + input_pad_ref: {Membrane.Pad, :input, 1}, + output_pad_ref: {Membrane.Pad, :output, 1} + }, + %Vertex{ + component_path: ^inspected_pipeline <> "/:source", + output_pad_ref: {Membrane.Pad, :output, 1} + } + ] = longer_path + + assert [ + %Vertex{ + component_path: ^inspected_pipeline <> "/:sink", + input_pad_ref: {Membrane.Pad, :input, 2} + }, + %Vertex{ + component_path: ^inspected_pipeline <> "/:source", + output_pad_ref: {Membrane.Pad, :output, 2} + } + ] = shorter_path + + Testing.Pipeline.terminate(pipeline) + end + end +end From a3c6bdd7f70b1f236e0b04a5a9c65c8ec9e7d0b3 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Tue, 26 Nov 2024 19:05:57 +0100 Subject: [PATCH 13/32] Add more test cases wip --- .../diamond_logger.ex | 36 ++++++++-- .../diamond_logger_impl.ex | 59 ++++++++------- .../integration/diamond_detection_test.exs | 72 ++++++++++++++----- 3 files changed, 114 insertions(+), 53 deletions(-) diff --git a/lib/membrane/core/element/diamond_detection_controller/diamond_logger.ex b/lib/membrane/core/element/diamond_detection_controller/diamond_logger.ex index 0b97e2dcb..3206e1857 100644 --- a/lib/membrane/core/element/diamond_detection_controller/diamond_logger.ex +++ b/lib/membrane/core/element/diamond_detection_controller/diamond_logger.ex @@ -1,13 +1,35 @@ defmodule Membrane.Core.Element.DiamondDetectionController.DiamondLogger do @moduledoc false - alias Membrane.Core.Element.DiamondDetectionController.{ - DiamondLoggerImpl, - PathInGraph - } + require Membrane.Logger - @callback log_diamond(PathInGraph.t(), PathInGraph.t()) :: :ok + alias Membrane.Core.Element.DiamondDetectionController.PathInGraph + alias Membrane.Core.Element.DiamondDetectionController.PathInGraph.Vertex - def log_diamond(path_a, path_b), do: impl().log_diamond(path_a, path_b) - defp impl(), do: Application.get_env(:membrane_core, :diamond_logger, DiamondLoggerImpl) + @spec log_diamond(PathInGraph.t(), PathInGraph.t()) :: :ok + def log_diamond(path_a, path_b) do + Membrane.Logger.debug(""" + DIAMOND + + Path A: + #{inspect_path(path_a)} + + Path B: + #{inspect_path(path_b)} + """) + + :ok + end + + defp inspect_path(path) do + path + |> Enum.reverse() + |> Enum.chunk_every(2, 1, :discard) + |> Enum.map_join("\n", fn [%Vertex{} = from, %Vertex{} = to] -> + """ + From #{from.component_path} via output pad #{inspect(from.output_pad_ref)} \ + to #{to.component_path} via input pad #{inspect(to.input_pad_ref)}. + """ + end) + end end diff --git a/lib/membrane/core/element/diamond_detection_controller/diamond_logger_impl.ex b/lib/membrane/core/element/diamond_detection_controller/diamond_logger_impl.ex index 6048f8849..4020b8e3f 100644 --- a/lib/membrane/core/element/diamond_detection_controller/diamond_logger_impl.ex +++ b/lib/membrane/core/element/diamond_detection_controller/diamond_logger_impl.ex @@ -1,37 +1,36 @@ -defmodule Membrane.Core.Element.DiamondDetectionController.DiamondLoggerImpl do - @moduledoc false +# defmodule Membrane.Core.Element.DiamondDetectionController.DiamondLoggerImpl do +# @moduledoc false - require Membrane.Logger - alias Membrane.Core.Element.DiamondDetectionController - alias Membrane.Core.Element.DiamondDetectionController.DiamondLogger - alias Membrane.Core.Element.DiamondDetectionController.PathInGraph.Vertex +# require Membrane.Logger +# alias Membrane.Core.Element.DiamondDetectionController.DiamondLogger +# alias Membrane.Core.Element.DiamondDetectionController.PathInGraph.Vertex - @behaviour DiamondLogger +# @behaviour DiamondLogger - @impl DiamondLogger - def log_diamond(path_a, path_b) do - Membrane.Logger.debug(""" - DIAMOND +# @impl DiamondLogger +# def log_diamond(path_a, path_b) do +# Membrane.Logger.debug(""" +# DIAMOND - Path A: - #{inspect_path(path_a)} +# Path A: +# #{inspect_path(path_a)} - Path B: - #{inspect_path(path_b)} - """) +# Path B: +# #{inspect_path(path_b)} +# """) - :ok - end +# :ok +# end - defp inspect_path(path) do - path - |> Enum.reverse() - |> Enum.chunk_every(2, 1, :discard) - |> Enum.map_join("\n", fn [%Vertex{} = from, %Vertex{} = to] -> - """ - From #{from.component_path} via output pad #{inspect(from.output_pad_ref)} \ - to #{to.component_path} via input pad #{inspect(to.input_pad_ref)}. - """ - end) - end -end +# defp inspect_path(path) do +# path +# |> Enum.reverse() +# |> Enum.chunk_every(2, 1, :discard) +# |> Enum.map_join("\n", fn [%Vertex{} = from, %Vertex{} = to] -> +# """ +# From #{from.component_path} via output pad #{inspect(from.output_pad_ref)} \ +# to #{to.component_path} via input pad #{inspect(to.input_pad_ref)}. +# """ +# end) +# end +# end diff --git a/test/membrane/integration/diamond_detection_test.exs b/test/membrane/integration/diamond_detection_test.exs index 4616a1e5b..708a9ff3c 100644 --- a/test/membrane/integration/diamond_detection_test.exs +++ b/test/membrane/integration/diamond_detection_test.exs @@ -1,5 +1,5 @@ defmodule Membrane.Integration.DiamondDetectionTest do - use ExUnit.Case, async: false + use ExUnit.Case, async: true import ExUnit.Assertions import Membrane.ChildrenSpec @@ -7,7 +7,7 @@ defmodule Membrane.Integration.DiamondDetectionTest do require Membrane.Pad, as: Pad - alias Membrane.Core.Element.DiamondDetectionController.{DiamondLoggerImpl, PathInGraph} + alias Membrane.Core.Element.DiamondDetectionController.DiamondLogger alias Membrane.Core.Element.DiamondDetectionController.PathInGraph.Vertex alias Membrane.Testing @@ -29,60 +29,100 @@ defmodule Membrane.Integration.DiamondDetectionTest do def handle_demand(_pad, _demand, _unit, _ctx, state), do: {[], state} end - test "simple scenario" do - with_mock DiamondLoggerImpl, log_diamond: fn _path_a, _path_b -> :ok end do + test "logging a diamond" do + with_mock DiamondLogger, log_diamond: fn _path_a, _path_b -> :ok end do + # a -> b -> c and a -> c spec = [ - child(:source, Node) + child(:a, Node) |> via_out(Pad.ref(:output, 1)) |> via_in(Pad.ref(:input, 1)) - |> child(:filter, Node) + |> child(:b, Node) |> via_out(Pad.ref(:output, 1)) |> via_in(Pad.ref(:input, 1)) - |> child(:sink, Node), - get_child(:source) + |> child(:c, Node), + get_child(:a) |> via_out(Pad.ref(:output, 2)) |> via_in(Pad.ref(:input, 2)) - |> get_child(:sink) + |> get_child(:c) ] pipeline = Testing.Pipeline.start_link_supervised!(spec: spec) Process.sleep(1500) - assert [{_element_pid, {DiamondLoggerImpl, :log_diamond, [path_a, path_b]}, :ok}] = - call_history(DiamondLoggerImpl) + assert [{_element_pid, {DiamondLogger, :log_diamond, [path_a, path_b]}, :ok}] = + call_history(DiamondLogger) "#PID" <> inspected_pipeline = inspect(pipeline) [shorter_path, longer_path] = Enum.sort_by([path_a, path_b], &length/1) + # assert a -> b -> c assert [ %Vertex{ - component_path: ^inspected_pipeline <> "/:sink", + component_path: ^inspected_pipeline <> "/:c", input_pad_ref: {Membrane.Pad, :input, 1} }, %Vertex{ - component_path: ^inspected_pipeline <> "/:filter", + component_path: ^inspected_pipeline <> "/:b", input_pad_ref: {Membrane.Pad, :input, 1}, output_pad_ref: {Membrane.Pad, :output, 1} }, %Vertex{ - component_path: ^inspected_pipeline <> "/:source", + component_path: ^inspected_pipeline <> "/:a", output_pad_ref: {Membrane.Pad, :output, 1} } ] = longer_path + # assert a -> c assert [ %Vertex{ - component_path: ^inspected_pipeline <> "/:sink", + component_path: ^inspected_pipeline <> "/:c", input_pad_ref: {Membrane.Pad, :input, 2} }, %Vertex{ - component_path: ^inspected_pipeline <> "/:source", + component_path: ^inspected_pipeline <> "/:a", output_pad_ref: {Membrane.Pad, :output, 2} } ] = shorter_path + # d -> a -> b -> c and a -> c + spec = + child(:d, Node) + |> via_out(Pad.ref(:output, 3)) + |> via_in(Pad.ref(:input, 3)) + |> get_child(:a) + + Testing.Pipeline.execute_actions(pipeline, spec: spec) + + Process.sleep(1500) + + # adding this link shouldn't trigger logging a diamond + assert call_history(DiamondLogger) |> length() == 1 + + # d -> a -> b -> c and a -> c and d -> c + spec = + get_child(:d) + |> via_out(Pad.ref(:output, 4)) + |> via_in(Pad.ref(:input, 4)) + |> get_child(:c) + + Testing.Pipeline.execute_actions(pipeline, spec: spec) + + Process.sleep(1500) + + assert [_old_entry | new_entries] = call_history(DiamondLogger) + assert new_entries != [] + + new_entries + |> Enum.flat_map(fn {_element_pid, {DiamondLogger, :log_diamond, [path_a, path_b]}, :ok} -> + [path_a, path_b] + end) + |> Enum.each(fn path -> + assert %{component_path: ^inspected_pipeline <> "/:c"} = List.first(path) + assert %{component_path: ^inspected_pipeline <> "/:d"} = List.last(path) + end) + Testing.Pipeline.terminate(pipeline) end end From 9855e8ae22ec0573d84874e8978daa4d0db48dc9 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Tue, 26 Nov 2024 19:24:54 +0100 Subject: [PATCH 14/32] Refactor diamond detection code --- .../element/diamond_detection_controller.ex | 26 ++++++++------ .../diamond_logger.ex | 2 ++ .../diamond_logger_impl.ex | 36 ------------------- .../integration/diamond_detection_test.exs | 3 +- 4 files changed, 19 insertions(+), 48 deletions(-) delete mode 100644 lib/membrane/core/element/diamond_detection_controller/diamond_logger_impl.ex diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex index f2346c0a9..23d3b6d48 100644 --- a/lib/membrane/core/element/diamond_detection_controller.ex +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -6,10 +6,11 @@ defmodule Membrane.Core.Element.DiamondDetectionController do require Membrane.Pad, as: Pad alias __MODULE__.{DiamondLogger, PathInGraph} + alias __MODULE__.PathInGraph.Vertex alias Membrane.Core.Element.State alias Membrane.Element.PadData - @component_path_suffix "__membrane_component_path_64_byte_suffix________________________" + @component_path_prefix "__membrane_component_path_64_byte_prefix________________________" @spec start_diamond_detection(State.t()) :: :ok def start_diamond_detection(state) do @@ -61,11 +62,14 @@ defmodule Membrane.Core.Element.DiamondDetectionController do state true -> + old_diamond_detection_path = + state.diamond_detection_ref_to_path[diamond_detection_ref] + |> remove_component_path_prefix() + :ok = - DiamondLogger.log_diamond( - diamond_detecton_path |> cut_suffixes_in_path(), - state.diamond_detection_ref_to_path[diamond_detection_ref] |> cut_suffixes_in_path() - ) + diamond_detecton_path + |> remove_component_path_prefix() + |> DiamondLogger.log_diamond(old_diamond_detection_path) state end @@ -186,18 +190,18 @@ defmodule Membrane.Core.Element.DiamondDetectionController do end defp get_component_path() do - # adding @component_path_suffix to component path will cause that component path will + # adding @component_path_prefix to component path will cause that component path will # always have more than 64 bytes, so it won't be copied during sending a message - (Membrane.ComponentPath.get() ++ [@component_path_suffix]) + [@component_path_prefix | Membrane.ComponentPath.get()] |> Enum.join() end defp have_common_prefix?(path_a, path_b), do: List.last(path_a) == List.last(path_b) - defp cut_suffixes_in_path(path_in_graph) do + defp remove_component_path_prefix(path_in_graph) do path_in_graph - |> Enum.map( - &%{&1 | component_path: String.replace(&1.component_path, @component_path_suffix, "")} - ) + |> Enum.map(fn %Vertex{component_path: @component_path_prefix <> component_path} = vertex -> + %{vertex | component_path: component_path} + end) end end diff --git a/lib/membrane/core/element/diamond_detection_controller/diamond_logger.ex b/lib/membrane/core/element/diamond_detection_controller/diamond_logger.ex index 3206e1857..64e3fed55 100644 --- a/lib/membrane/core/element/diamond_detection_controller/diamond_logger.ex +++ b/lib/membrane/core/element/diamond_detection_controller/diamond_logger.ex @@ -6,6 +6,8 @@ defmodule Membrane.Core.Element.DiamondDetectionController.DiamondLogger do alias Membrane.Core.Element.DiamondDetectionController.PathInGraph alias Membrane.Core.Element.DiamondDetectionController.PathInGraph.Vertex + # logging a diamond is moved to the separate module due to testing + @spec log_diamond(PathInGraph.t(), PathInGraph.t()) :: :ok def log_diamond(path_a, path_b) do Membrane.Logger.debug(""" diff --git a/lib/membrane/core/element/diamond_detection_controller/diamond_logger_impl.ex b/lib/membrane/core/element/diamond_detection_controller/diamond_logger_impl.ex deleted file mode 100644 index 4020b8e3f..000000000 --- a/lib/membrane/core/element/diamond_detection_controller/diamond_logger_impl.ex +++ /dev/null @@ -1,36 +0,0 @@ -# defmodule Membrane.Core.Element.DiamondDetectionController.DiamondLoggerImpl do -# @moduledoc false - -# require Membrane.Logger -# alias Membrane.Core.Element.DiamondDetectionController.DiamondLogger -# alias Membrane.Core.Element.DiamondDetectionController.PathInGraph.Vertex - -# @behaviour DiamondLogger - -# @impl DiamondLogger -# def log_diamond(path_a, path_b) do -# Membrane.Logger.debug(""" -# DIAMOND - -# Path A: -# #{inspect_path(path_a)} - -# Path B: -# #{inspect_path(path_b)} -# """) - -# :ok -# end - -# defp inspect_path(path) do -# path -# |> Enum.reverse() -# |> Enum.chunk_every(2, 1, :discard) -# |> Enum.map_join("\n", fn [%Vertex{} = from, %Vertex{} = to] -> -# """ -# From #{from.component_path} via output pad #{inspect(from.output_pad_ref)} \ -# to #{to.component_path} via input pad #{inspect(to.input_pad_ref)}. -# """ -# end) -# end -# end diff --git a/test/membrane/integration/diamond_detection_test.exs b/test/membrane/integration/diamond_detection_test.exs index 708a9ff3c..38a936213 100644 --- a/test/membrane/integration/diamond_detection_test.exs +++ b/test/membrane/integration/diamond_detection_test.exs @@ -29,6 +29,7 @@ defmodule Membrane.Integration.DiamondDetectionTest do def handle_demand(_pad, _demand, _unit, _ctx, state), do: {[], state} end + @tag :xd test "logging a diamond" do with_mock DiamondLogger, log_diamond: fn _path_a, _path_b -> :ok end do # a -> b -> c and a -> c @@ -112,7 +113,7 @@ defmodule Membrane.Integration.DiamondDetectionTest do Process.sleep(1500) assert [_old_entry | new_entries] = call_history(DiamondLogger) - assert new_entries != [] + assert length(new_entries) in [1, 2] new_entries |> Enum.flat_map(fn {_element_pid, {DiamondLogger, :log_diamond, [path_a, path_b]}, :ok} -> From f5d694151564db68abdc96604928fe7af1630093 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Tue, 26 Nov 2024 19:30:54 +0100 Subject: [PATCH 15/32] Remove unused import --- test/membrane/integration/diamond_detection_test.exs | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/membrane/integration/diamond_detection_test.exs b/test/membrane/integration/diamond_detection_test.exs index 38a936213..ebf7c97f4 100644 --- a/test/membrane/integration/diamond_detection_test.exs +++ b/test/membrane/integration/diamond_detection_test.exs @@ -1,7 +1,6 @@ defmodule Membrane.Integration.DiamondDetectionTest do use ExUnit.Case, async: true - import ExUnit.Assertions import Membrane.ChildrenSpec import Mock @@ -29,7 +28,6 @@ defmodule Membrane.Integration.DiamondDetectionTest do def handle_demand(_pad, _demand, _unit, _ctx, state), do: {[], state} end - @tag :xd test "logging a diamond" do with_mock DiamondLogger, log_diamond: fn _path_a, _path_b -> :ok end do # a -> b -> c and a -> c From acbe87dfb228faf272eea11b30ead28b4fbdfd10 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 27 Nov 2024 10:22:57 +0100 Subject: [PATCH 16/32] Improve diamond log message, fix credo --- .../element/diamond_detection_controller.ex | 14 +++++------ .../diamond_logger.ex | 16 ++++++++---- lib/membrane/core/element/state.ex | 5 ++-- .../parent/diamond_detection_controller.ex | 4 +-- .../diamond_detection_test.exs | 25 ++++++++++--------- 5 files changed, 36 insertions(+), 28 deletions(-) rename test/membrane/{integration => }/diamond_detection_test.exs (83%) diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex index 23d3b6d48..f837ba7e7 100644 --- a/lib/membrane/core/element/diamond_detection_controller.ex +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -1,15 +1,15 @@ defmodule Membrane.Core.Element.DiamondDetectionController do @moduledoc false - require Membrane.Core.Message, as: Message - require Membrane.Logger - require Membrane.Pad, as: Pad - alias __MODULE__.{DiamondLogger, PathInGraph} alias __MODULE__.PathInGraph.Vertex alias Membrane.Core.Element.State alias Membrane.Element.PadData + require Membrane.Core.Message, as: Message + require Membrane.Logger + require Membrane.Pad, as: Pad + @component_path_prefix "__membrane_component_path_64_byte_prefix________________________" @spec start_diamond_detection(State.t()) :: :ok @@ -91,7 +91,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do state.pads_data |> Enum.each(fn {pad_ref, pad_data} -> - if is_output_pull_pad(pad_data, auto_pull_mode?) do + if output_pull_pad?(pad_data, auto_pull_mode?) do current_entry = %{current_entry | output_pad_ref: pad_ref} diamond_detection_path = [current_entry | diamond_detection_path_tail] @@ -113,7 +113,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do end) end - defp is_output_pull_pad(%PadData{} = pad_data, auto_pull_mode?) do + defp output_pull_pad?(%PadData{} = pad_data, auto_pull_mode?) do pad_data.direction == :output and (pad_data.flow_control == :manual or (pad_data.flow_control == :auto and auto_pull_mode?)) @@ -180,7 +180,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do auto_pull_mode? = state.effective_flow_control == :pull state.pads_data - |> Enum.count(fn {_pad_ref, pad_data} -> is_output_pull_pad(pad_data, auto_pull_mode?) end) + |> Enum.count(fn {_pad_ref, pad_data} -> output_pull_pad?(pad_data, auto_pull_mode?) end) end defp send_after_to_self(message, seconds \\ 10) do diff --git a/lib/membrane/core/element/diamond_detection_controller/diamond_logger.ex b/lib/membrane/core/element/diamond_detection_controller/diamond_logger.ex index 64e3fed55..928d87323 100644 --- a/lib/membrane/core/element/diamond_detection_controller/diamond_logger.ex +++ b/lib/membrane/core/element/diamond_detection_controller/diamond_logger.ex @@ -1,22 +1,28 @@ defmodule Membrane.Core.Element.DiamondDetectionController.DiamondLogger do @moduledoc false - require Membrane.Logger - alias Membrane.Core.Element.DiamondDetectionController.PathInGraph alias Membrane.Core.Element.DiamondDetectionController.PathInGraph.Vertex + require Membrane.Logger + # logging a diamond is moved to the separate module due to testing @spec log_diamond(PathInGraph.t(), PathInGraph.t()) :: :ok def log_diamond(path_a, path_b) do + from = List.last(path_a) |> Map.fetch!(:component_path) + to = List.first(path_a) |> Map.fetch!(:component_path) + Membrane.Logger.debug(""" - DIAMOND + Two paths from element #{from} to #{to} were detected, in which all pads are operating in pull \ + mode. With such a pipeline configuration, the membrane flow control mechanism may stop demanding \ + buffers. If you are debugging such an issue, keep in mind that input pads with `flow_control: :auto` \ + demand data when there is a demand for data on ALL output pads with `flow_control: :auto`. - Path A: + The first path from #{from} to #{to} leads: #{inspect_path(path_a)} - Path B: + The second path from #{from} to #{to} leads: #{inspect_path(path_b)} """) diff --git a/lib/membrane/core/element/state.ex b/lib/membrane/core/element/state.ex index d99155ac2..079523098 100644 --- a/lib/membrane/core/element/state.ex +++ b/lib/membrane/core/element/state.ex @@ -9,7 +9,8 @@ defmodule Membrane.Core.Element.State do alias Membrane.{Clock, Element, Pad, Sync} alias Membrane.Core.Child.PadModel - alias Membrane.Core.Element.{DiamondDetectionController, EffectiveFlowController} + alias Membrane.Core.Element.DiamondDetectionController.PathInGraph + alias Membrane.Core.Element.EffectiveFlowController alias Membrane.Core.Timer require Membrane.Pad @@ -48,7 +49,7 @@ defmodule Membrane.Core.Element.State do awaiting_auto_input_pads: MapSet.t(), resume_delayed_demands_loop_in_mailbox?: boolean(), diamond_detection_ref_to_path: %{ - optional(reference()) => DiamondDetectionController.diamond_detection_path() + optional(reference()) => PathInGraph.t() }, diamond_detection_trigger_refs: MapSet.t(reference()), diamond_detection_postponed?: boolean() diff --git a/lib/membrane/core/parent/diamond_detection_controller.ex b/lib/membrane/core/parent/diamond_detection_controller.ex index 75ce95b4a..131918dc3 100644 --- a/lib/membrane/core/parent/diamond_detection_controller.ex +++ b/lib/membrane/core/parent/diamond_detection_controller.ex @@ -1,11 +1,11 @@ defmodule Membrane.Core.Parent.DiamondDetectionController do @moduledoc false - require Membrane.Core.Message, as: Message - alias Membrane.Child alias Membrane.Core.Parent + require Membrane.Core.Message, as: Message + @spec start_diamond_detection_trigger(Child.name(), reference(), Parent.state()) :: :ok def start_diamond_detection_trigger(child_name, trigger_ref, state) do with %{component_type: :element, pid: pid} <- state.children[child_name] do diff --git a/test/membrane/integration/diamond_detection_test.exs b/test/membrane/diamond_detection_test.exs similarity index 83% rename from test/membrane/integration/diamond_detection_test.exs rename to test/membrane/diamond_detection_test.exs index ebf7c97f4..7f4c007fe 100644 --- a/test/membrane/integration/diamond_detection_test.exs +++ b/test/membrane/diamond_detection_test.exs @@ -1,15 +1,15 @@ -defmodule Membrane.Integration.DiamondDetectionTest do +defmodule Membrane.DiamondDetectionTest do use ExUnit.Case, async: true import Membrane.ChildrenSpec import Mock - require Membrane.Pad, as: Pad - alias Membrane.Core.Element.DiamondDetectionController.DiamondLogger alias Membrane.Core.Element.DiamondDetectionController.PathInGraph.Vertex alias Membrane.Testing + require Membrane.Pad, as: Pad + defmodule Node do use Membrane.Filter @@ -28,7 +28,7 @@ defmodule Membrane.Integration.DiamondDetectionTest do def handle_demand(_pad, _demand, _unit, _ctx, state), do: {[], state} end - test "logging a diamond" do + test "diamond detection algorithm" do with_mock DiamondLogger, log_diamond: fn _path_a, _path_b -> :ok end do # a -> b -> c and a -> c spec = [ @@ -52,23 +52,23 @@ defmodule Membrane.Integration.DiamondDetectionTest do assert [{_element_pid, {DiamondLogger, :log_diamond, [path_a, path_b]}, :ok}] = call_history(DiamondLogger) - "#PID" <> inspected_pipeline = inspect(pipeline) + "#PID" <> pipeline_id = inspect(pipeline) [shorter_path, longer_path] = Enum.sort_by([path_a, path_b], &length/1) # assert a -> b -> c assert [ %Vertex{ - component_path: ^inspected_pipeline <> "/:c", + component_path: ^pipeline_id <> "/:c", input_pad_ref: {Membrane.Pad, :input, 1} }, %Vertex{ - component_path: ^inspected_pipeline <> "/:b", + component_path: ^pipeline_id <> "/:b", input_pad_ref: {Membrane.Pad, :input, 1}, output_pad_ref: {Membrane.Pad, :output, 1} }, %Vertex{ - component_path: ^inspected_pipeline <> "/:a", + component_path: ^pipeline_id <> "/:a", output_pad_ref: {Membrane.Pad, :output, 1} } ] = longer_path @@ -76,11 +76,11 @@ defmodule Membrane.Integration.DiamondDetectionTest do # assert a -> c assert [ %Vertex{ - component_path: ^inspected_pipeline <> "/:c", + component_path: ^pipeline_id <> "/:c", input_pad_ref: {Membrane.Pad, :input, 2} }, %Vertex{ - component_path: ^inspected_pipeline <> "/:a", + component_path: ^pipeline_id <> "/:a", output_pad_ref: {Membrane.Pad, :output, 2} } ] = shorter_path @@ -110,6 +110,7 @@ defmodule Membrane.Integration.DiamondDetectionTest do Process.sleep(1500) + # there should be one or two new logged diamonds, depending on the race condition assert [_old_entry | new_entries] = call_history(DiamondLogger) assert length(new_entries) in [1, 2] @@ -118,8 +119,8 @@ defmodule Membrane.Integration.DiamondDetectionTest do [path_a, path_b] end) |> Enum.each(fn path -> - assert %{component_path: ^inspected_pipeline <> "/:c"} = List.first(path) - assert %{component_path: ^inspected_pipeline <> "/:d"} = List.last(path) + assert %{component_path: ^pipeline_id <> "/:c"} = List.first(path) + assert %{component_path: ^pipeline_id <> "/:d"} = List.last(path) end) Testing.Pipeline.terminate(pipeline) From f02d57e01f4702f4c549efc38b5d3da6be8fa808 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 27 Nov 2024 10:38:50 +0100 Subject: [PATCH 17/32] Remove leftover files --- .../core/bin/diamond_detection_controller.ex | 12 --- lib/membrane/core/diamond_detector.ex | 99 ------------------- 2 files changed, 111 deletions(-) delete mode 100644 lib/membrane/core/bin/diamond_detection_controller.ex delete mode 100644 lib/membrane/core/diamond_detector.ex diff --git a/lib/membrane/core/bin/diamond_detection_controller.ex b/lib/membrane/core/bin/diamond_detection_controller.ex deleted file mode 100644 index dd0130bdc..000000000 --- a/lib/membrane/core/bin/diamond_detection_controller.ex +++ /dev/null @@ -1,12 +0,0 @@ -# defmodule Membrane.Core.Bin.DiamondDetectionController do -# @moduledoc false - -# require Membrane.Core.Message, as: Message - -# alias Membrane.Core.Bin.State - -# @spec trigger_diamond_detection(State.t()) :: :ok -# def trigger_diamond_detection(state) do -# Message.send(state.parent_pid, :trigger_diamond_detection) -# end -# end diff --git a/lib/membrane/core/diamond_detector.ex b/lib/membrane/core/diamond_detector.ex deleted file mode 100644 index 57700831c..000000000 --- a/lib/membrane/core/diamond_detector.ex +++ /dev/null @@ -1,99 +0,0 @@ -# defmodule Membrane.Core.DiamondDetector do -# use GenServer - -# require Membrane.Core.Message, as: Message -# require Membrane.Core.Utils, as: Utils - -# @timer_interval 100 - -# defmodule State do -# use Bunch.Access - -# defstruct output_pads: %{}, -# input_pads: %{}, -# elements_effective_flow_control: %{}, -# updated_since_last_run?: false, -# timer_ref: nil -# end - -# @impl GenServer -# def init(_arg) do -# Utils.log_on_error do -# {:ok, %State{}} -# end -# end - -# @impl GenServer -# def handle_info(Message.new(:effective_flow_control_change, {efc, pid}), %State{} = state) do -# Utils.log_on_error do -# state = set_effective_flow_control(pid, efc, state) -# {:noreply, state} -# end -# end - -# @impl GenServer -# def handle_info(Message.new(:new_element, pid), %State{} = state) do -# Utils.log_on_error do -# Process.monitor(pid) -# state = set_effective_flow_control(pid, :push, state) -# {:noreply, state} -# end -# end - -# @impl GenServer -# def handle_info(Message.new(:new_link, {from, _from_flow_control, to, to_flow_control}), %State{} = state) do -# Utils.log_on_error do -# # if :push not in [from_flow_control, to_flow_control] -# state = -# %{state | updated_since_last_run?: true} -# |> Map.update!(:output_pads, &add_new_output_pad(&1, from, to, to_flow_control)) -# |> Map.update!(:input_pads, &add_new_input_pad(&1, from, to, to_flow_control)) -# |> ensure_timer_running() - -# {:noreply, state} -# end -# end - -# @impl GenServer -# def handle_info(Message.new(:delete_link, {from, to, to_flow_control}), %State{} = state) do -# state -# |> Map.update!(:output_pads, ) - -# end - -# @impl GenServer -# def handle_info(:do_algorithm, %State{} = state) do -# Utils.log_on_error do -# # do_algorithm -# {:noreply, %{state | timer_ref: nil, updated_since_last_run?: false}} -# end -# end - -# defp set_effective_flow_control(pid, efc, %State{} = state) do -# %{state | updated_since_last_run?: true} -# |> Map.update!(:elements_effective_flow_control, &Map.put(&1, pid, efc)) -# |> ensure_timer_running() -# end - -# defp ensure_timer_running(%State{timer_ref: nil} = state) do -# timer_ref = self() |> Process.send_after(:do_algorithm, @timer_interval) -# %{state | timer_ref: timer_ref} -# end - -# defp ensure_timer_running(state), do: state - -# defp add_new_output_pad(output_pads, from, to, to_flow_control) do -# entry = {to, to_flow_control} - -# output_pads -# |> Map.update(from, [entry], &[entry | &1]) -# end - -# defp add_new_input_pad(input_pads, from, to, to_flow_control) do -# entry = {from, to_flow_control} - -# input_pads -# |> Map.update(to, [entry], &1[entry | &1]) -# end - -# end From 35cf6559ea297dd79d5bdcd908b6cc7834b06d3e Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 27 Nov 2024 10:40:35 +0100 Subject: [PATCH 18/32] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 04d410f1e..52447447e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## 1.2.0 * Add `:max_instances` option for dynamic pads. [#876](https://github.com/membraneframework/membrane_core/pull/876) * Implement `Membrane.Connector`. [#904](https://github.com/membraneframework/membrane_core/pull/904) + * Implememnt diamonds detection. [#909](https://github.com/membraneframework/membrane_core/pull/909) ## 1.1.2 * Add new callback `handle_child_terminated/3` along with new assertions. [#894](https://github.com/membraneframework/membrane_core/pull/894) From c8b586a2289546bfb7b9480e9c63cebed8d47746 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 27 Nov 2024 10:42:40 +0100 Subject: [PATCH 19/32] Remove leftovers --- lib/membrane/core/pipeline/state.ex | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/membrane/core/pipeline/state.ex b/lib/membrane/core/pipeline/state.ex index 85118aa70..37f24ea2d 100644 --- a/lib/membrane/core/pipeline/state.ex +++ b/lib/membrane/core/pipeline/state.ex @@ -35,7 +35,6 @@ defmodule Membrane.Core.Pipeline.State do stalker: Membrane.Core.Stalker.t(), subprocess_supervisor: pid(), awaiting_setup_completition?: boolean() - # diamond_detection_triggered?: boolean() } # READ THIS BEFORE ADDING NEW FIELD!!! @@ -60,6 +59,4 @@ defmodule Membrane.Core.Pipeline.State do resource_guard: nil, subprocess_supervisor: nil, awaiting_setup_completition?: false - - # diamond_detection_triggered?: false end From 97d15cf122ab2b7f3e6fd3b1b3452e5e09f44f54 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 27 Nov 2024 11:01:46 +0100 Subject: [PATCH 20/32] Fix Connector test --- test/membrane/integration/connector_test.exs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/membrane/integration/connector_test.exs b/test/membrane/integration/connector_test.exs index cc6b7102e..59c8a683e 100644 --- a/test/membrane/integration/connector_test.exs +++ b/test/membrane/integration/connector_test.exs @@ -34,6 +34,8 @@ defmodule Membrane.Integration.ConnectorTest do }) ) + assert_child_playing(pipeline, :source) + data = generate_data(100, [:stream_format, :buffer, :event]) data From 63148036683dfb9431ac9a848fa26dd5459c460c Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 27 Nov 2024 11:13:17 +0100 Subject: [PATCH 21/32] Fix typo in the comment --- lib/membrane/core/element/diamond_detection_controller.ex | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex index f837ba7e7..cd3b5d1c9 100644 --- a/lib/membrane/core/element/diamond_detection_controller.ex +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -190,8 +190,8 @@ defmodule Membrane.Core.Element.DiamondDetectionController do end defp get_component_path() do - # adding @component_path_prefix to component path will cause that component path will - # always have more than 64 bytes, so it won't be copied during sending a message + # adding @component_path_prefix to component path causes that component path + # always has more than 64 bytes, so it won't be copied during sending a message [@component_path_prefix | Membrane.ComponentPath.get()] |> Enum.join() end From 1f941015b41456211bf1b2c31b3260587dcf0393 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 4 Dec 2024 11:18:26 +0100 Subject: [PATCH 22/32] Refactor State due to CR --- .../element/diamond_detection_controller.ex | 35 +++++++++++-------- lib/membrane/core/element/state.ex | 18 +++++----- 2 files changed, 31 insertions(+), 22 deletions(-) diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex index cd3b5d1c9..921843273 100644 --- a/lib/membrane/core/element/diamond_detection_controller.ex +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -39,7 +39,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do diamond_detecton_path = [new_path_vertex | diamond_detecton_path] cond do - not is_map_key(state.diamond_detection_ref_to_path, diamond_detection_ref) -> + not is_map_key(state.diamond_detection.ref_to_path, diamond_detection_ref) -> :ok = forward_diamond_detection(diamond_detection_ref, diamond_detecton_path, state) :ok = @@ -48,7 +48,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do state |> put_in( - [:diamond_detection_ref_to_path, diamond_detection_ref], + [:diamond_detection, :ref_to_path, diamond_detection_ref], diamond_detecton_path ) @@ -57,13 +57,13 @@ defmodule Membrane.Core.Element.DiamondDetectionController do have_common_prefix?( diamond_detecton_path, - state.diamond_detection_ref_to_path[diamond_detection_ref] + state.diamond_detection.ref_to_path[diamond_detection_ref] ) -> state true -> old_diamond_detection_path = - state.diamond_detection_ref_to_path[diamond_detection_ref] + state.diamond_detection.ref_to_path[diamond_detection_ref] |> remove_component_path_prefix() :ok = @@ -77,11 +77,11 @@ defmodule Membrane.Core.Element.DiamondDetectionController do @spec delete_diamond_detection_ref(reference(), State.t()) :: State.t() def delete_diamond_detection_ref(diamond_detection_ref, state) do + {_path, %State{} = state} = + state + |> pop_in([:diamond_detection, :ref_to_path, diamond_detection_ref]) + state - |> Map.update!( - :diamond_detection_ref_to_path, - &Map.delete(&1, diamond_detection_ref) - ) end @spec forward_diamond_detection(reference(), PathInGraph.t(), State.t()) :: :ok @@ -127,7 +127,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do @spec start_diamond_detection_trigger(reference(), State.t()) :: State.t() def start_diamond_detection_trigger(spec_ref, state) do if map_size(state.pads_data) < 2 or - MapSet.member?(state.diamond_detection_trigger_refs, spec_ref) do + MapSet.member?(state.diamond_detection.trigger_refs, spec_ref) do state else do_handle_diamond_detection_trigger(spec_ref, state) @@ -137,7 +137,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do @spec handle_diamond_detection_trigger(reference(), State.t()) :: State.t() def handle_diamond_detection_trigger(trigger_ref, %State{} = state) do if state.type == :endpoint or - MapSet.member?(state.diamond_detection_trigger_refs, trigger_ref), + MapSet.member?(state.diamond_detection.trigger_refs, trigger_ref), do: state, else: do_handle_diamond_detection_trigger(trigger_ref, state) end @@ -145,7 +145,10 @@ defmodule Membrane.Core.Element.DiamondDetectionController do defp do_handle_diamond_detection_trigger(trigger_ref, %State{} = state) do state = state - |> Map.update!(:diamond_detection_trigger_refs, &MapSet.put(&1, trigger_ref)) + |> update_in( + [:diamond_detection, :trigger_refs], + &MapSet.put(&1, trigger_ref) + ) :ok = Message.new(:delete_diamond_detection_trigger_ref, trigger_ref) @@ -158,7 +161,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do else: state end - defp postpone_diamond_detection(%State{} = state) when state.diamond_detection_postponed? do + defp postpone_diamond_detection(%State{} = state) when state.diamond_detection.postponed? do state end @@ -167,13 +170,17 @@ defmodule Membrane.Core.Element.DiamondDetectionController do Message.new(:start_diamond_detection) |> send_after_to_self(1) - %{state | diamond_detection_postponed?: true} + state + |> put_in([:diamond_detection, :postponed?], true) end @spec delete_diamond_detection_trigger_ref(reference(), State.t()) :: State.t() def delete_diamond_detection_trigger_ref(trigger_ref, state) do state - |> Map.update!(:diamond_detection_trigger_refs, &MapSet.delete(&1, trigger_ref)) + |> update_in( + [:diamond_detection, :trigger_refs], + &MapSet.delete(&1, trigger_ref) + ) end defp output_pull_arity(state) do diff --git a/lib/membrane/core/element/state.ex b/lib/membrane/core/element/state.ex index 079523098..57c1527c1 100644 --- a/lib/membrane/core/element/state.ex +++ b/lib/membrane/core/element/state.ex @@ -48,11 +48,11 @@ defmodule Membrane.Core.Element.State do satisfied_auto_output_pads: MapSet.t(), awaiting_auto_input_pads: MapSet.t(), resume_delayed_demands_loop_in_mailbox?: boolean(), - diamond_detection_ref_to_path: %{ - optional(reference()) => PathInGraph.t() - }, - diamond_detection_trigger_refs: MapSet.t(reference()), - diamond_detection_postponed?: boolean() + diamond_detection: %{ + ref_to_path: %{optional(reference()) => PathInGraph.t()}, + trigger_refs: MapSet.t(reference()), + postponed?: boolean() + } } # READ THIS BEFORE ADDING NEW FIELD!!! @@ -85,9 +85,11 @@ defmodule Membrane.Core.Element.State do handle_demand_loop_counter: 0, pads_to_snapshot: MapSet.new(), playback_queue: [], - diamond_detection_ref_to_path: %{}, - diamond_detection_trigger_refs: MapSet.new(), - diamond_detection_postponed?: false, + diamond_detection: %{ + ref_to_path: %{}, + trigger_refs: MapSet.new(), + postponed?: false + }, pads_data: %{}, satisfied_auto_output_pads: MapSet.new(), awaiting_auto_input_pads: MapSet.new(), From 090488af6ece28bcc4a2abf5a3b22c6fc960b281 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 4 Dec 2024 12:08:20 +0100 Subject: [PATCH 23/32] Refactor diamond detection messages --- lib/membrane/core/element.ex | 43 +---------- .../element/diamond_detection_controller.ex | 76 ++++++++++++++----- .../parent/diamond_detection_controller.ex | 3 +- 3 files changed, 62 insertions(+), 60 deletions(-) diff --git a/lib/membrane/core/element.ex b/lib/membrane/core/element.ex index b3966bb29..d5aaddac1 100644 --- a/lib/membrane/core/element.ex +++ b/lib/membrane/core/element.ex @@ -282,47 +282,8 @@ defmodule Membrane.Core.Element do {:noreply, state} end - defp do_handle_info(Message.new(:start_diamond_detection), state) do - :ok = DiamondDetectionController.start_diamond_detection(state) - {:noreply, state} - end - - defp do_handle_info( - Message.new(:diamond_detection, [ - input_pad_ref, - diamond_detection_ref, - diamond_detection_path - ]), - state - ) do - state = - DiamondDetectionController.continue_diamond_detection( - input_pad_ref, - diamond_detection_ref, - diamond_detection_path, - state - ) - - {:noreply, state} - end - - defp do_handle_info(Message.new(:delete_diamond_detection_ref, diamond_detection_ref), state) do - state = DiamondDetectionController.delete_diamond_detection_ref(diamond_detection_ref, state) - {:noreply, state} - end - - defp do_handle_info(Message.new(:start_diamond_detection_trigger, trigger_ref), state) do - state = DiamondDetectionController.start_diamond_detection_trigger(trigger_ref, state) - {:noreply, state} - end - - defp do_handle_info(Message.new(:diamond_detection_trigger, trigger_ref), state) do - state = DiamondDetectionController.handle_diamond_detection_trigger(trigger_ref, state) - {:noreply, state} - end - - defp do_handle_info(Message.new(:delete_diamond_detection_trigger_ref, trigger_ref), state) do - state = DiamondDetectionController.delete_diamond_detection_trigger_ref(trigger_ref, state) + defp do_handle_info( Message.new(:diamond_detection, message), state ) do + state = DiamondDetectionController.handle_diamond_detection_message(message, state) {:noreply, state} end diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex index 921843273..e7e5f4292 100644 --- a/lib/membrane/core/element/diamond_detection_controller.ex +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -12,8 +12,45 @@ defmodule Membrane.Core.Element.DiamondDetectionController do @component_path_prefix "__membrane_component_path_64_byte_prefix________________________" + @type diamond_detection_message() :: %{ + :type => + :start + | :start_trigger + | :diamond_detection + | :trigger + | :delete_ref + | :delete_trigger_ref, + optional(:ref) => reference(), + optional(:path) => PathInGraph.t(), + optional(:pad_ref) => Pad.ref() + } + + @spec handle_diamond_detection_message(diamond_detection_message(), State.t()) :: State.t() + def handle_diamond_detection_message(%{type: type} = message, state) do + case type do + :start -> + :ok = start_diamond_detection(state) + state + + :start_trigger -> + start_diamond_detection_trigger(message.ref, state) + + :diamond_detection -> + continue_diamond_detection(message.pad_ref, message.ref, message.path, state) + + :trigger -> + handle_diamond_detection_trigger(message.ref, state) + + :delete_ref -> + delete_diamond_detection_ref(message.ref, state) + + :delete_trigger_ref -> + delete_diamond_detection_trigger_ref(message.ref, state) + end + end + @spec start_diamond_detection(State.t()) :: :ok - def start_diamond_detection(state) do + defp start_diamond_detection(state) do diamond_detection_path = [ %PathInGraph.Vertex{pid: self(), component_path: get_component_path()} ] @@ -24,7 +61,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do @spec continue_diamond_detection(Pad.ref(), reference(), PathInGraph.t(), State.t()) :: State.t() - def continue_diamond_detection( + defp continue_diamond_detection( input_pad_ref, diamond_detection_ref, diamond_detecton_path, @@ -43,7 +80,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do :ok = forward_diamond_detection(diamond_detection_ref, diamond_detecton_path, state) :ok = - Message.new(:delete_diamond_detection_ref, diamond_detection_ref) + %{type: :delete_ref, ref: diamond_detection_ref} |> send_after_to_self() state @@ -76,7 +113,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do end @spec delete_diamond_detection_ref(reference(), State.t()) :: State.t() - def delete_diamond_detection_ref(diamond_detection_ref, state) do + defp delete_diamond_detection_ref(diamond_detection_ref, state) do {_path, %State{} = state} = state |> pop_in([:diamond_detection, :ref_to_path, diamond_detection_ref]) @@ -95,11 +132,14 @@ defmodule Membrane.Core.Element.DiamondDetectionController do current_entry = %{current_entry | output_pad_ref: pad_ref} diamond_detection_path = [current_entry | diamond_detection_path_tail] - Message.send( - pad_data.pid, - :diamond_detection, - [pad_data.other_ref, diamond_detection_ref, diamond_detection_path] - ) + message = %{ + type: :diamond_detection, + pad_ref: pad_data.other_ref, + ref: diamond_detection_ref, + path: diamond_detection_path + } + + Message.send(pad_data.pid, :diamond_detection, message) end end) end @@ -108,7 +148,8 @@ defmodule Membrane.Core.Element.DiamondDetectionController do state.pads_data |> Enum.each(fn {_pad_ref, %PadData{} = pad_data} -> if pad_data.direction == :input and pad_data.flow_control != :push do - Message.send(pad_data.pid, :diamond_detection_trigger, trigger_ref) + message = %{type: :trigger, ref: trigger_ref} + Message.send(pad_data.pid, :diamond_detection, message) end end) end @@ -125,7 +166,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do end @spec start_diamond_detection_trigger(reference(), State.t()) :: State.t() - def start_diamond_detection_trigger(spec_ref, state) do + defp start_diamond_detection_trigger(spec_ref, state) do if map_size(state.pads_data) < 2 or MapSet.member?(state.diamond_detection.trigger_refs, spec_ref) do state @@ -135,7 +176,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do end @spec handle_diamond_detection_trigger(reference(), State.t()) :: State.t() - def handle_diamond_detection_trigger(trigger_ref, %State{} = state) do + defp handle_diamond_detection_trigger(trigger_ref, %State{} = state) do if state.type == :endpoint or MapSet.member?(state.diamond_detection.trigger_refs, trigger_ref), do: state, @@ -151,7 +192,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do ) :ok = - Message.new(:delete_diamond_detection_trigger_ref, trigger_ref) + %{type: :delete_trigger_ref, ref: trigger_ref} |> send_after_to_self() :ok = forward_diamond_detection_trigger(trigger_ref, state) @@ -166,16 +207,14 @@ defmodule Membrane.Core.Element.DiamondDetectionController do end defp postpone_diamond_detection(%State{} = state) do - :ok = - Message.new(:start_diamond_detection) - |> send_after_to_self(1) + :ok = %{type: :start} |> send_after_to_self(1) state |> put_in([:diamond_detection, :postponed?], true) end @spec delete_diamond_detection_trigger_ref(reference(), State.t()) :: State.t() - def delete_diamond_detection_trigger_ref(trigger_ref, state) do + defp delete_diamond_detection_trigger_ref(trigger_ref, state) do state |> update_in( [:diamond_detection, :trigger_refs], @@ -190,8 +229,9 @@ defmodule Membrane.Core.Element.DiamondDetectionController do |> Enum.count(fn {_pad_ref, pad_data} -> output_pull_pad?(pad_data, auto_pull_mode?) end) end - defp send_after_to_self(message, seconds \\ 10) do + defp send_after_to_self(%{type: _type} = message, seconds \\ 10) do send_after_time = Membrane.Time.seconds(seconds) |> Membrane.Time.as_milliseconds(:round) + message = Message.new(:diamond_detection, message) self() |> Process.send_after(message, send_after_time) :ok end diff --git a/lib/membrane/core/parent/diamond_detection_controller.ex b/lib/membrane/core/parent/diamond_detection_controller.ex index 131918dc3..dd3488f3a 100644 --- a/lib/membrane/core/parent/diamond_detection_controller.ex +++ b/lib/membrane/core/parent/diamond_detection_controller.ex @@ -9,7 +9,8 @@ defmodule Membrane.Core.Parent.DiamondDetectionController do @spec start_diamond_detection_trigger(Child.name(), reference(), Parent.state()) :: :ok def start_diamond_detection_trigger(child_name, trigger_ref, state) do with %{component_type: :element, pid: pid} <- state.children[child_name] do - Message.send(pid, :start_diamond_detection_trigger, trigger_ref) + message = %{type: :start_trigger, ref: trigger_ref} + Message.send(pid, :diamond_detection, message) end :ok From 1d003bafa0572d0e3398ff9a07820c330638c928 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 4 Dec 2024 14:37:16 +0100 Subject: [PATCH 24/32] stop creating serialized component path every time --- lib/membrane/core/element.ex | 2 +- .../element/diamond_detection_controller.ex | 57 +++++++++++++------ lib/membrane/core/element/state.ex | 2 + 3 files changed, 43 insertions(+), 18 deletions(-) diff --git a/lib/membrane/core/element.ex b/lib/membrane/core/element.ex index d5aaddac1..0c639dacc 100644 --- a/lib/membrane/core/element.ex +++ b/lib/membrane/core/element.ex @@ -282,7 +282,7 @@ defmodule Membrane.Core.Element do {:noreply, state} end - defp do_handle_info( Message.new(:diamond_detection, message), state ) do + defp do_handle_info(Message.new(:diamond_detection, message), state) do state = DiamondDetectionController.handle_diamond_detection_message(message, state) {:noreply, state} end diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex index e7e5f4292..1d3be720b 100644 --- a/lib/membrane/core/element/diamond_detection_controller.ex +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -29,8 +29,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do def handle_diamond_detection_message(%{type: type} = message, state) do case type do :start -> - :ok = start_diamond_detection(state) - state + start_diamond_detection(state) :start_trigger -> start_diamond_detection_trigger(message.ref, state) @@ -49,27 +48,34 @@ defmodule Membrane.Core.Element.DiamondDetectionController do end end - @spec start_diamond_detection(State.t()) :: :ok + @spec start_diamond_detection(State.t()) :: State.t() defp start_diamond_detection(state) do + {component_path, state} = get_component_path(state) + diamond_detection_path = [ - %PathInGraph.Vertex{pid: self(), component_path: get_component_path()} + %PathInGraph.Vertex{pid: self(), component_path: component_path} ] - make_ref() - |> forward_diamond_detection(diamond_detection_path, state) + :ok = + make_ref() + |> forward_diamond_detection(diamond_detection_path, state) + + state end @spec continue_diamond_detection(Pad.ref(), reference(), PathInGraph.t(), State.t()) :: State.t() defp continue_diamond_detection( - input_pad_ref, - diamond_detection_ref, - diamond_detecton_path, - state - ) do + input_pad_ref, + diamond_detection_ref, + diamond_detecton_path, + state + ) do + {component_path, state} = get_component_path(state) + new_path_vertex = %PathInGraph.Vertex{ pid: self(), - component_path: get_component_path(), + component_path: component_path, input_pad_ref: input_pad_ref } @@ -236,11 +242,28 @@ defmodule Membrane.Core.Element.DiamondDetectionController do :ok end - defp get_component_path() do - # adding @component_path_prefix to component path causes that component path - # always has more than 64 bytes, so it won't be copied during sending a message - [@component_path_prefix | Membrane.ComponentPath.get()] - |> Enum.join() + defp get_component_path(state) do + case state.diamond_detection.serialized_component_path do + nil -> + # adding @component_path_prefix to component path causes that component path + # always has more than 64 bytes, so it won't be copied during sending a message + + component_path = + [@component_path_prefix | Membrane.ComponentPath.get()] + |> Enum.join() + + state = + state + |> put_in( + [:diamond_detection, :serialized_component_path], + component_path + ) + + {component_path, state} + + component_path -> + {component_path, state} + end end defp have_common_prefix?(path_a, path_b), do: List.last(path_a) == List.last(path_b) diff --git a/lib/membrane/core/element/state.ex b/lib/membrane/core/element/state.ex index 57c1527c1..a20daef2b 100644 --- a/lib/membrane/core/element/state.ex +++ b/lib/membrane/core/element/state.ex @@ -49,6 +49,7 @@ defmodule Membrane.Core.Element.State do awaiting_auto_input_pads: MapSet.t(), resume_delayed_demands_loop_in_mailbox?: boolean(), diamond_detection: %{ + serialized_component_path: String.t() | nil, ref_to_path: %{optional(reference()) => PathInGraph.t()}, trigger_refs: MapSet.t(reference()), postponed?: boolean() @@ -86,6 +87,7 @@ defmodule Membrane.Core.Element.State do pads_to_snapshot: MapSet.new(), playback_queue: [], diamond_detection: %{ + serialized_component_path: nil, ref_to_path: %{}, trigger_refs: MapSet.new(), postponed?: false From 501aeeedb3712fae56314ec755aeafc6e79b8261 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Thu, 5 Dec 2024 16:34:06 +0100 Subject: [PATCH 25/32] Write some comments describing how the diamond detection mechanism works --- .../element/diamond_detection_controller.ex | 71 +++++++++++++++++++ .../diamond_logger.ex | 4 +- 2 files changed, 73 insertions(+), 2 deletions(-) diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex index 1d3be720b..c20f2f2f6 100644 --- a/lib/membrane/core/element/diamond_detection_controller.ex +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -25,6 +25,77 @@ defmodule Membrane.Core.Element.DiamondDetectionController do optional(:pad_ref) => Pad.ref() } + # DESCRIPTION OF THE ALGORITHM OF FINDING DIAMONDS IN THE PIPELINE + + # Definitions: + + # diamond - directed graph, that has at least two distinct elements (sink and source) and + # has two vertex-disjoint paths from the source to the sink. + + # This algotithm takes the directed graph made by all Elements within single Pipeline and + # finds some diamond-subgraphs having all the edges (links) working in the :pull mode, + # it means in :manual flow control or in :auto flow controll if the effective flow control + # is set to :pull. + + # These diamonds can be dangerous when used with pull flow control, e.g. let's consider + # a pipeline, that contains: + # * MP4 demuxer that has two output pads + # * MKV muxer that is linked to both of them + # and let's assume, that MP4 container that is consumed by MP4 demuxer is unbalanced. + # If MKV muxer has pads working in pull mode, then demand on one pad will be satisified, + # but on the another won't, because the MP4 container is unbalanced. Then, if MP4 demuxer + # has pads in auto flow control and its effective flow control is set to :pull, it won't + # demand on the input, because one of the pads output with :auto flow control doesn't + # have positive demand, so the whole pipeline will get stuck and won't process more data. + + # The algorithms is made of two phases: (1) triggering and (2) proper searching. + + # (1) Triggering + + # Let's notice, that: + # * new diamond can be created only after linking new spec + # * if the new spec caused some new diamond to occur, this diamond will contain some of + # the links spawned in this spec + + # If the diamond contains a link, it must also contain an element which output pad takes + # is the part of this link. + + # After spec status is set to :done, parent component that returned the spec will trigger + # all elements, which output pads has been linked in this spec. Reference of the trigger + # is always set to the spec reference. + + # If the element is triggered with a specific reference for the first time, it does two + # things: + # * the element forwards the trigger with the same reference via all input pads working + # in the pull mode + # * if the element has at least two output pads working in the pull mode, it postpones + # a proper searching that will be spawned from itself. Time between postponing and the + # proper searching is one second. If in this time an element will be triggered once + # again with a different reference, it won't cause another postponing a proper + # searching, this means that at the time there is at most one proper searching + # postponed in the single element + + # (2) Proper searching: + + # Proper searching is started only in the elements that have at lest two output pads + # working in the pull mode. When an elements starts proper searching, it assings + # a new reference to it, different from the reference of the related trigger. + + # When a proper searching enters the element (no matter if it is the element that has + # hust started the proper searching, or maybe it was forwarded to it via link): + # * if the element sees the proper searching reference for the first time, then: + # - it forwards proper searching via all output pads working in the pull mode + # - when the proper searching is forwarded, it remembers the path in grapth through + # the elments that is has already passed + # * if the element has already seen the reference of proper searching, but there is + # a repeated element on the path, that proper searching traversed to this element, + # the element does nothing + # * if the element has already seen the reference of proper searching and the traversed + # path doesn't contain any repeated elements, it means that the current traversed path + # and the path that the proper searching traversed when it entered the element + # previous time together make a diamond. Then, the element logs the found diamond + # and doesn't forward proper searching fruther + @spec handle_diamond_detection_message(diamond_detection_message(), State.t()) :: State.t() def handle_diamond_detection_message(%{type: type} = message, state) do case type do diff --git a/lib/membrane/core/element/diamond_detection_controller/diamond_logger.ex b/lib/membrane/core/element/diamond_detection_controller/diamond_logger.ex index 928d87323..14eeaa8c7 100644 --- a/lib/membrane/core/element/diamond_detection_controller/diamond_logger.ex +++ b/lib/membrane/core/element/diamond_detection_controller/diamond_logger.ex @@ -35,8 +35,8 @@ defmodule Membrane.Core.Element.DiamondDetectionController.DiamondLogger do |> Enum.chunk_every(2, 1, :discard) |> Enum.map_join("\n", fn [%Vertex{} = from, %Vertex{} = to] -> """ - From #{from.component_path} via output pad #{inspect(from.output_pad_ref)} \ - to #{to.component_path} via input pad #{inspect(to.input_pad_ref)}. + * from #{from.component_path} via output pad #{inspect(from.output_pad_ref)} to \ + #{to.component_path} via input pad #{inspect(to.input_pad_ref)} """ end) end From 07fcfd12d9fe06f8ac9c3261cffa1ff23e3ba2c3 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Thu, 5 Dec 2024 16:34:49 +0100 Subject: [PATCH 26/32] Move the comments within the file --- .../element/diamond_detection_controller.ex | 49 ++++++++++--------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex index c20f2f2f6..20a9cedd1 100644 --- a/lib/membrane/core/element/diamond_detection_controller.ex +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -1,30 +1,6 @@ defmodule Membrane.Core.Element.DiamondDetectionController do @moduledoc false - alias __MODULE__.{DiamondLogger, PathInGraph} - alias __MODULE__.PathInGraph.Vertex - alias Membrane.Core.Element.State - alias Membrane.Element.PadData - - require Membrane.Core.Message, as: Message - require Membrane.Logger - require Membrane.Pad, as: Pad - - @component_path_prefix "__membrane_component_path_64_byte_prefix________________________" - - @type diamond_detection_message() :: %{ - :type => - :start - | :start_trigger - | :diamond_detection - | :trigger - | :delete_ref - | :delete_trigger_ref, - optional(:ref) => reference(), - optional(:path) => PathInGraph.t(), - optional(:pad_ref) => Pad.ref() - } - # DESCRIPTION OF THE ALGORITHM OF FINDING DIAMONDS IN THE PIPELINE # Definitions: @@ -96,6 +72,31 @@ defmodule Membrane.Core.Element.DiamondDetectionController do # previous time together make a diamond. Then, the element logs the found diamond # and doesn't forward proper searching fruther + alias __MODULE__.{DiamondLogger, PathInGraph} + alias __MODULE__.PathInGraph.Vertex + alias Membrane.Core.Element.State + alias Membrane.Element.PadData + + require Membrane.Core.Message, as: Message + require Membrane.Logger + require Membrane.Pad, as: Pad + + @component_path_prefix "__membrane_component_path_64_byte_prefix________________________" + + @type diamond_detection_message() :: %{ + :type => + :start + | :start_trigger + | :diamond_detection + | :trigger + | :delete_ref + | :delete_trigger_ref, + optional(:ref) => reference(), + optional(:path) => PathInGraph.t(), + optional(:pad_ref) => Pad.ref() + } + + @spec handle_diamond_detection_message(diamond_detection_message(), State.t()) :: State.t() def handle_diamond_detection_message(%{type: type} = message, state) do case type do From cc9039d0b55471ec62f7926f4d638a69b3e10cf6 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Fri, 6 Dec 2024 12:22:52 +0100 Subject: [PATCH 27/32] Fix typos in the algorithm description --- .../element/diamond_detection_controller.ex | 88 +++++++++---------- 1 file changed, 43 insertions(+), 45 deletions(-) diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex index 20a9cedd1..b5315f714 100644 --- a/lib/membrane/core/element/diamond_detection_controller.ex +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -5,72 +5,71 @@ defmodule Membrane.Core.Element.DiamondDetectionController do # Definitions: - # diamond - directed graph, that has at least two distinct elements (sink and source) and + # diamond - directed graph that has at least two distinct elements (sink and source) and # has two vertex-disjoint paths from the source to the sink. - # This algotithm takes the directed graph made by all Elements within single Pipeline and - # finds some diamond-subgraphs having all the edges (links) working in the :pull mode, - # it means in :manual flow control or in :auto flow controll if the effective flow control + # This algorithm takes the directed graph made by all elements within a single pipeline and + # finds some diamond-subgraphs where all the edges (links) work in the :pull mode, + # it means in :manual flow control or in :auto flow control if the effective flow control # is set to :pull. # These diamonds can be dangerous when used with pull flow control, e.g. let's consider - # a pipeline, that contains: - # * MP4 demuxer that has two output pads - # * MKV muxer that is linked to both of them - # and let's assume, that MP4 container that is consumed by MP4 demuxer is unbalanced. - # If MKV muxer has pads working in pull mode, then demand on one pad will be satisified, - # but on the another won't, because the MP4 container is unbalanced. Then, if MP4 demuxer + # a pipeline that contains: + # * MP4 demuxer that has two output pads + # * MKV muxer that is linked to both of them + # and let's assume that the MP4 container that is consumed by the MP4 demuxer is unbalanced. + # If the MKV muxer has pads working in pull mode, then demand on one pad will be satisfied, + # but on the other won't, because the MP4 container is unbalanced. Then, if MP4 demuxer # has pads in auto flow control and its effective flow control is set to :pull, it won't # demand on the input, because one of the pads output with :auto flow control doesn't # have positive demand, so the whole pipeline will get stuck and won't process more data. - # The algorithms is made of two phases: (1) triggering and (2) proper searching. + # The algorithm is made of two phases: (1) triggering and (2) proper searching. # (1) Triggering - # Let's notice, that: - # * new diamond can be created only after linking new spec - # * if the new spec caused some new diamond to occur, this diamond will contain some of - # the links spawned in this spec + # Let's notice that: + # * a new diamond can be created only after linking a new spec + # * if the new spec caused some new diamond to occur, this diamond will contain some of + # the links spawned in this spec + # If the diamond contains a link, it must also contain an element whose output pad + # is part of this link. - # If the diamond contains a link, it must also contain an element which output pad takes - # is the part of this link. - - # After spec status is set to :done, parent component that returned the spec will trigger - # all elements, which output pads has been linked in this spec. Reference of the trigger + # After the spec status is set to :done, the parent component that returned the spec will trigger + # all elements whose output pads have been linked in this spec. The reference of the trigger # is always set to the spec reference. # If the element is triggered with a specific reference for the first time, it does two # things: - # * the element forwards the trigger with the same reference via all input pads working - # in the pull mode - # * if the element has at least two output pads working in the pull mode, it postpones - # a proper searching that will be spawned from itself. Time between postponing and the - # proper searching is one second. If in this time an element will be triggered once - # again with a different reference, it won't cause another postponing a proper - # searching, this means that at the time there is at most one proper searching - # postponed in the single element + # * the element forwards the trigger with the same reference via all input pads working + # in the pull mode + # * if the element has at least two output pads working in the pull mode, it postpones + # the proper searching that will be spawned from itself. The time between postponing and the + # proper searching is one second. If during this time an element is triggered once + # again with a different reference, it won't cause another postponement of the proper + # searching, this means that at the time there is at most one proper searching + # postponed in the single element # (2) Proper searching: - # Proper searching is started only in the elements that have at lest two output pads - # working in the pull mode. When an elements starts proper searching, it assings + # Proper searching is started only in elements that have at least two output pads + # working in the pull mode. When an element starts proper searching, it assigns # a new reference to it, different from the reference of the related trigger. - # When a proper searching enters the element (no matter if it is the element that has - # hust started the proper searching, or maybe it was forwarded to it via link): - # * if the element sees the proper searching reference for the first time, then: - # - it forwards proper searching via all output pads working in the pull mode - # - when the proper searching is forwarded, it remembers the path in grapth through - # the elments that is has already passed - # * if the element has already seen the reference of proper searching, but there is - # a repeated element on the path, that proper searching traversed to this element, - # the element does nothing - # * if the element has already seen the reference of proper searching and the traversed - # path doesn't contain any repeated elements, it means that the current traversed path - # and the path that the proper searching traversed when it entered the element - # previous time together make a diamond. Then, the element logs the found diamond - # and doesn't forward proper searching fruther + # When proper searching enters the element (no matter if it is the element that has + # just started the proper searching, or maybe it was forwarded to it via a link): + # * if the element sees the proper searching reference for the first time, then: + # - it forwards proper searching via all output pads working in the pull mode + # - when proper searching is forwarded, it remembers the path in the graph through + # the elements that it has already passed + # * if the element has already seen the reference of proper searching, but there is + # a repeated element on the path that proper searching traversed to this element, + # the element does nothing + # * if the element has already seen the reference of proper searching and the traversed + # path doesn't contain any repeated elements, it means that the current traversed path + # and the path that the proper searching traversed when it entered the element + # the previous time together make a diamond. Then, the element logs the found diamond + # and doesn't forward proper searching further. alias __MODULE__.{DiamondLogger, PathInGraph} alias __MODULE__.PathInGraph.Vertex @@ -96,7 +95,6 @@ defmodule Membrane.Core.Element.DiamondDetectionController do optional(:pad_ref) => Pad.ref() } - @spec handle_diamond_detection_message(diamond_detection_message(), State.t()) :: State.t() def handle_diamond_detection_message(%{type: type} = message, state) do case type do From d364df905890c887c4ec90ce19ff867613af9d1f Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Mon, 16 Dec 2024 14:08:19 +0100 Subject: [PATCH 28/32] Implement CR suggestions WiP --- .../element/diamond_detection_controller.ex | 34 ++++++++++--------- .../diamond_detection_state.ex | 19 +++++++++++ lib/membrane/core/element/state.ex | 16 ++------- 3 files changed, 40 insertions(+), 29 deletions(-) create mode 100644 lib/membrane/core/element/diamond_detection_controller/diamond_detection_state.ex diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex index b5315f714..9f9256b65 100644 --- a/lib/membrane/core/element/diamond_detection_controller.ex +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -10,16 +10,17 @@ defmodule Membrane.Core.Element.DiamondDetectionController do # This algorithm takes the directed graph made by all elements within a single pipeline and # finds some diamond-subgraphs where all the edges (links) work in the :pull mode, - # it means in :manual flow control or in :auto flow control if the effective flow control - # is set to :pull. + # that is they're either in the :manual flow control or in the :auto flow control and the + # effective flow control is set to :pull. # These diamonds can be dangerous when used with pull flow control, e.g. let's consider # a pipeline that contains: # * MP4 demuxer that has two output pads # * MKV muxer that is linked to both of them - # and let's assume that the MP4 container that is consumed by the MP4 demuxer is unbalanced. + # and let's assume that the MP4 file that is consumed by the MP4 demuxer is unbalanced + # (audio and video streams are not interleaved, for example audio comes first and then video) # If the MKV muxer has pads working in pull mode, then demand on one pad will be satisfied, - # but on the other won't, because the MP4 container is unbalanced. Then, if MP4 demuxer + # but on the other won't, because the source MP4 file is unbalanced. Then, if the MP4 demuxer # has pads in auto flow control and its effective flow control is set to :pull, it won't # demand on the input, because one of the pads output with :auto flow control doesn't # have positive demand, so the whole pipeline will get stuck and won't process more data. @@ -30,14 +31,14 @@ defmodule Membrane.Core.Element.DiamondDetectionController do # Let's notice that: # * a new diamond can be created only after linking a new spec - # * if the new spec caused some new diamond to occur, this diamond will contain some of - # the links spawned in this spec + # * if the new spec created a new diamond, this diamond will contain some of + # the links spawned in this spec # If the diamond contains a link, it must also contain an element whose output pad # is part of this link. - # After the spec status is set to :done, the parent component that returned the spec will trigger - # all elements whose output pads have been linked in this spec. The reference of the trigger - # is always set to the spec reference. + # After the spec status is set to :done, the parent component that returned the spec will + # triggerall elements whose output pads have been linked in this spec. The reference of the + # trigger is always set to the spec reference. # If the element is triggered with a specific reference for the first time, it does two # things: @@ -152,7 +153,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do diamond_detecton_path = [new_path_vertex | diamond_detecton_path] cond do - not is_map_key(state.diamond_detection.ref_to_path, diamond_detection_ref) -> + not is_map_key(state.diamond_detection_state.ref_to_path, diamond_detection_ref) -> :ok = forward_diamond_detection(diamond_detection_ref, diamond_detecton_path, state) :ok = @@ -170,13 +171,13 @@ defmodule Membrane.Core.Element.DiamondDetectionController do have_common_prefix?( diamond_detecton_path, - state.diamond_detection.ref_to_path[diamond_detection_ref] + state.diamond_detection_state.ref_to_path[diamond_detection_ref] ) -> state true -> old_diamond_detection_path = - state.diamond_detection.ref_to_path[diamond_detection_ref] + state.diamond_detection_state.ref_to_path[diamond_detection_ref] |> remove_component_path_prefix() :ok = @@ -244,7 +245,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do @spec start_diamond_detection_trigger(reference(), State.t()) :: State.t() defp start_diamond_detection_trigger(spec_ref, state) do if map_size(state.pads_data) < 2 or - MapSet.member?(state.diamond_detection.trigger_refs, spec_ref) do + MapSet.member?(state.diamond_detection_state.trigger_refs, spec_ref) do state else do_handle_diamond_detection_trigger(spec_ref, state) @@ -254,7 +255,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do @spec handle_diamond_detection_trigger(reference(), State.t()) :: State.t() defp handle_diamond_detection_trigger(trigger_ref, %State{} = state) do if state.type == :endpoint or - MapSet.member?(state.diamond_detection.trigger_refs, trigger_ref), + MapSet.member?(state.diamond_detection_state.trigger_refs, trigger_ref), do: state, else: do_handle_diamond_detection_trigger(trigger_ref, state) end @@ -278,7 +279,8 @@ defmodule Membrane.Core.Element.DiamondDetectionController do else: state end - defp postpone_diamond_detection(%State{} = state) when state.diamond_detection.postponed? do + defp postpone_diamond_detection(%State{} = state) + when state.diamond_detection_state.postponed? do state end @@ -313,7 +315,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do end defp get_component_path(state) do - case state.diamond_detection.serialized_component_path do + case state.diamond_detection_state.serialized_component_path do nil -> # adding @component_path_prefix to component path causes that component path # always has more than 64 bytes, so it won't be copied during sending a message diff --git a/lib/membrane/core/element/diamond_detection_controller/diamond_detection_state.ex b/lib/membrane/core/element/diamond_detection_controller/diamond_detection_state.ex new file mode 100644 index 000000000..09d4f5e22 --- /dev/null +++ b/lib/membrane/core/element/diamond_detection_controller/diamond_detection_state.ex @@ -0,0 +1,19 @@ +defmodule Membrane.Core.Element.DiamondDetectionController.DiamondDatectionState do + @moduledoc false + + alias Membrane.Core.Element.DiamondDetectionController.PathInGraph + + defstruct [ + :serialized_component_path, + ref_to_path: %{}, + trigger_refs: MapSet.new(), + postponed?: false + ] + + @type t :: %__MODULE__{ + serialized_component_path: String.t() | nil, + ref_to_path: %{optional(reference()) => PathInGraph.t()}, + trigger_refs: MapSet.t(reference()), + postponed?: boolean() + } +end diff --git a/lib/membrane/core/element/state.ex b/lib/membrane/core/element/state.ex index a20daef2b..1c4d2d898 100644 --- a/lib/membrane/core/element/state.ex +++ b/lib/membrane/core/element/state.ex @@ -9,7 +9,7 @@ defmodule Membrane.Core.Element.State do alias Membrane.{Clock, Element, Pad, Sync} alias Membrane.Core.Child.PadModel - alias Membrane.Core.Element.DiamondDetectionController.PathInGraph + alias Membrane.Core.Element.DiamondDetectionController.DiamondDatectionState alias Membrane.Core.Element.EffectiveFlowController alias Membrane.Core.Timer @@ -48,12 +48,7 @@ defmodule Membrane.Core.Element.State do satisfied_auto_output_pads: MapSet.t(), awaiting_auto_input_pads: MapSet.t(), resume_delayed_demands_loop_in_mailbox?: boolean(), - diamond_detection: %{ - serialized_component_path: String.t() | nil, - ref_to_path: %{optional(reference()) => PathInGraph.t()}, - trigger_refs: MapSet.t(reference()), - postponed?: boolean() - } + diamond_detection_state: DiamondDatectionState.t() } # READ THIS BEFORE ADDING NEW FIELD!!! @@ -86,12 +81,7 @@ defmodule Membrane.Core.Element.State do handle_demand_loop_counter: 0, pads_to_snapshot: MapSet.new(), playback_queue: [], - diamond_detection: %{ - serialized_component_path: nil, - ref_to_path: %{}, - trigger_refs: MapSet.new(), - postponed?: false - }, + diamond_detection_state: %DiamondDatectionState{}, pads_data: %{}, satisfied_auto_output_pads: MapSet.new(), awaiting_auto_input_pads: MapSet.new(), From c158ee8c3824cdffeb27b8419351f78377b84834 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Mon, 16 Dec 2024 14:45:07 +0100 Subject: [PATCH 29/32] Implement CR suggestions WiP --- .../element/diamond_detection_controller.ex | 78 +++++++++---------- .../diamond_detection_state.ex | 2 + 2 files changed, 41 insertions(+), 39 deletions(-) diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex index 9f9256b65..38f2f1cd3 100644 --- a/lib/membrane/core/element/diamond_detection_controller.ex +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -85,11 +85,11 @@ defmodule Membrane.Core.Element.DiamondDetectionController do @type diamond_detection_message() :: %{ :type => - :start + :start_search + | :forward_search + | :delete_search_ref | :start_trigger - | :diamond_detection - | :trigger - | :delete_ref + | :forward_trigger | :delete_trigger_ref, optional(:ref) => reference(), optional(:path) => PathInGraph.t(), @@ -99,28 +99,28 @@ defmodule Membrane.Core.Element.DiamondDetectionController do @spec handle_diamond_detection_message(diamond_detection_message(), State.t()) :: State.t() def handle_diamond_detection_message(%{type: type} = message, state) do case type do - :start -> - start_diamond_detection(state) + :start_search -> + start_search(state) :start_trigger -> - start_diamond_detection_trigger(message.ref, state) + start_trigger(message.ref, state) - :diamond_detection -> - continue_diamond_detection(message.pad_ref, message.ref, message.path, state) + :forward_search -> + continue_search(message.pad_ref, message.ref, message.path, state) - :trigger -> - handle_diamond_detection_trigger(message.ref, state) + :forward_trigger -> + continue_trigger(message.ref, state) - :delete_ref -> - delete_diamond_detection_ref(message.ref, state) + :delete_search_ref -> + delete_search_ref(message.ref, state) :delete_trigger_ref -> - delete_diamond_detection_trigger_ref(message.ref, state) + delete_trigger_ref(message.ref, state) end end - @spec start_diamond_detection(State.t()) :: State.t() - defp start_diamond_detection(state) do + @spec start_search(State.t()) :: State.t() + defp start_search(state) do {component_path, state} = get_component_path(state) diamond_detection_path = [ @@ -134,9 +134,9 @@ defmodule Membrane.Core.Element.DiamondDetectionController do state end - @spec continue_diamond_detection(Pad.ref(), reference(), PathInGraph.t(), State.t()) :: + @spec continue_search(Pad.ref(), reference(), PathInGraph.t(), State.t()) :: State.t() - defp continue_diamond_detection( + defp continue_search( input_pad_ref, diamond_detection_ref, diamond_detecton_path, @@ -157,12 +157,12 @@ defmodule Membrane.Core.Element.DiamondDetectionController do :ok = forward_diamond_detection(diamond_detection_ref, diamond_detecton_path, state) :ok = - %{type: :delete_ref, ref: diamond_detection_ref} + %{type: :delete_search_ref, ref: diamond_detection_ref} |> send_after_to_self() state |> put_in( - [:diamond_detection, :ref_to_path, diamond_detection_ref], + [:diamond_detection_state, :ref_to_path, diamond_detection_ref], diamond_detecton_path ) @@ -189,11 +189,11 @@ defmodule Membrane.Core.Element.DiamondDetectionController do end end - @spec delete_diamond_detection_ref(reference(), State.t()) :: State.t() - defp delete_diamond_detection_ref(diamond_detection_ref, state) do + @spec delete_search_ref(reference(), State.t()) :: State.t() + defp delete_search_ref(diamond_detection_ref, state) do {_path, %State{} = state} = state - |> pop_in([:diamond_detection, :ref_to_path, diamond_detection_ref]) + |> pop_in([:diamond_detection_state, :ref_to_path, diamond_detection_ref]) state end @@ -210,7 +210,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do diamond_detection_path = [current_entry | diamond_detection_path_tail] message = %{ - type: :diamond_detection, + type: :forward_search, pad_ref: pad_data.other_ref, ref: diamond_detection_ref, path: diamond_detection_path @@ -225,7 +225,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do state.pads_data |> Enum.each(fn {_pad_ref, %PadData{} = pad_data} -> if pad_data.direction == :input and pad_data.flow_control != :push do - message = %{type: :trigger, ref: trigger_ref} + message = %{type: :forward_trigger, ref: trigger_ref} Message.send(pad_data.pid, :diamond_detection, message) end end) @@ -242,29 +242,29 @@ defmodule Membrane.Core.Element.DiamondDetectionController do uniq_length < length(diamond_detection_path) end - @spec start_diamond_detection_trigger(reference(), State.t()) :: State.t() - defp start_diamond_detection_trigger(spec_ref, state) do + @spec start_trigger(reference(), State.t()) :: State.t() + defp start_trigger(spec_ref, state) do if map_size(state.pads_data) < 2 or MapSet.member?(state.diamond_detection_state.trigger_refs, spec_ref) do state else - do_handle_diamond_detection_trigger(spec_ref, state) + do_continue_trigger(spec_ref, state) end end - @spec handle_diamond_detection_trigger(reference(), State.t()) :: State.t() - defp handle_diamond_detection_trigger(trigger_ref, %State{} = state) do + @spec continue_trigger(reference(), State.t()) :: State.t() + defp continue_trigger(trigger_ref, %State{} = state) do if state.type == :endpoint or MapSet.member?(state.diamond_detection_state.trigger_refs, trigger_ref), do: state, - else: do_handle_diamond_detection_trigger(trigger_ref, state) + else: do_continue_trigger(trigger_ref, state) end - defp do_handle_diamond_detection_trigger(trigger_ref, %State{} = state) do + defp do_continue_trigger(trigger_ref, %State{} = state) do state = state |> update_in( - [:diamond_detection, :trigger_refs], + [:diamond_detection_state, :trigger_refs], &MapSet.put(&1, trigger_ref) ) @@ -285,17 +285,17 @@ defmodule Membrane.Core.Element.DiamondDetectionController do end defp postpone_diamond_detection(%State{} = state) do - :ok = %{type: :start} |> send_after_to_self(1) + :ok = %{type: :start_search} |> send_after_to_self(1) state - |> put_in([:diamond_detection, :postponed?], true) + |> put_in([:diamond_detection_state, :postponed?], true) end - @spec delete_diamond_detection_trigger_ref(reference(), State.t()) :: State.t() - defp delete_diamond_detection_trigger_ref(trigger_ref, state) do + @spec delete_trigger_ref(reference(), State.t()) :: State.t() + defp delete_trigger_ref(trigger_ref, state) do state |> update_in( - [:diamond_detection, :trigger_refs], + [:diamond_detection_state, :trigger_refs], &MapSet.delete(&1, trigger_ref) ) end @@ -327,7 +327,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do state = state |> put_in( - [:diamond_detection, :serialized_component_path], + [:diamond_detection_state, :serialized_component_path], component_path ) diff --git a/lib/membrane/core/element/diamond_detection_controller/diamond_detection_state.ex b/lib/membrane/core/element/diamond_detection_controller/diamond_detection_state.ex index 09d4f5e22..bade2c96d 100644 --- a/lib/membrane/core/element/diamond_detection_controller/diamond_detection_state.ex +++ b/lib/membrane/core/element/diamond_detection_controller/diamond_detection_state.ex @@ -1,6 +1,8 @@ defmodule Membrane.Core.Element.DiamondDetectionController.DiamondDatectionState do @moduledoc false + use Bunch.Access + alias Membrane.Core.Element.DiamondDetectionController.PathInGraph defstruct [ From 8df71f851274543f55088f9aa5162bdc5fdf7227 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Mon, 16 Dec 2024 14:57:33 +0100 Subject: [PATCH 30/32] Further refactor --- .../element/diamond_detection_controller.ex | 38 +++++++++---------- .../integration/toilet_forwarding_test.exs | 2 +- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex index 38f2f1cd3..d80deaf3a 100644 --- a/lib/membrane/core/element/diamond_detection_controller.ex +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -86,10 +86,10 @@ defmodule Membrane.Core.Element.DiamondDetectionController do @type diamond_detection_message() :: %{ :type => :start_search - | :forward_search + | :search | :delete_search_ref | :start_trigger - | :forward_trigger + | :trigger | :delete_trigger_ref, optional(:ref) => reference(), optional(:path) => PathInGraph.t(), @@ -105,11 +105,11 @@ defmodule Membrane.Core.Element.DiamondDetectionController do :start_trigger -> start_trigger(message.ref, state) - :forward_search -> - continue_search(message.pad_ref, message.ref, message.path, state) + :search -> + handle_and_forward_search(message.pad_ref, message.ref, message.path, state) - :forward_trigger -> - continue_trigger(message.ref, state) + :trigger -> + handle_and_forward_trigger(message.ref, state) :delete_search_ref -> delete_search_ref(message.ref, state) @@ -129,14 +129,14 @@ defmodule Membrane.Core.Element.DiamondDetectionController do :ok = make_ref() - |> forward_diamond_detection(diamond_detection_path, state) + |> forward_search(diamond_detection_path, state) state end - @spec continue_search(Pad.ref(), reference(), PathInGraph.t(), State.t()) :: + @spec handle_and_forward_search(Pad.ref(), reference(), PathInGraph.t(), State.t()) :: State.t() - defp continue_search( + defp handle_and_forward_search( input_pad_ref, diamond_detection_ref, diamond_detecton_path, @@ -154,7 +154,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do cond do not is_map_key(state.diamond_detection_state.ref_to_path, diamond_detection_ref) -> - :ok = forward_diamond_detection(diamond_detection_ref, diamond_detecton_path, state) + :ok = forward_search(diamond_detection_ref, diamond_detecton_path, state) :ok = %{type: :delete_search_ref, ref: diamond_detection_ref} @@ -198,8 +198,8 @@ defmodule Membrane.Core.Element.DiamondDetectionController do state end - @spec forward_diamond_detection(reference(), PathInGraph.t(), State.t()) :: :ok - defp forward_diamond_detection(diamond_detection_ref, diamond_detection_path, state) do + @spec forward_search(reference(), PathInGraph.t(), State.t()) :: :ok + defp forward_search(diamond_detection_ref, diamond_detection_path, state) do auto_pull_mode? = state.effective_flow_control == :pull [current_entry | diamond_detection_path_tail] = diamond_detection_path @@ -210,7 +210,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do diamond_detection_path = [current_entry | diamond_detection_path_tail] message = %{ - type: :forward_search, + type: :search, pad_ref: pad_data.other_ref, ref: diamond_detection_ref, path: diamond_detection_path @@ -225,7 +225,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do state.pads_data |> Enum.each(fn {_pad_ref, %PadData{} = pad_data} -> if pad_data.direction == :input and pad_data.flow_control != :push do - message = %{type: :forward_trigger, ref: trigger_ref} + message = %{type: :trigger, ref: trigger_ref} Message.send(pad_data.pid, :diamond_detection, message) end end) @@ -248,19 +248,19 @@ defmodule Membrane.Core.Element.DiamondDetectionController do MapSet.member?(state.diamond_detection_state.trigger_refs, spec_ref) do state else - do_continue_trigger(spec_ref, state) + do_handle_and_forward_trigger(spec_ref, state) end end - @spec continue_trigger(reference(), State.t()) :: State.t() - defp continue_trigger(trigger_ref, %State{} = state) do + @spec handle_and_forward_trigger(reference(), State.t()) :: State.t() + defp handle_and_forward_trigger(trigger_ref, %State{} = state) do if state.type == :endpoint or MapSet.member?(state.diamond_detection_state.trigger_refs, trigger_ref), do: state, - else: do_continue_trigger(trigger_ref, state) + else: do_handle_and_forward_trigger(trigger_ref, state) end - defp do_continue_trigger(trigger_ref, %State{} = state) do + defp do_handle_and_forward_trigger(trigger_ref, %State{} = state) do state = state |> update_in( diff --git a/test/membrane/integration/toilet_forwarding_test.exs b/test/membrane/integration/toilet_forwarding_test.exs index 37097add8..580823a2a 100644 --- a/test/membrane/integration/toilet_forwarding_test.exs +++ b/test/membrane/integration/toilet_forwarding_test.exs @@ -13,7 +13,7 @@ defmodule Membrane.Integration.ToiletForwardingTest do end defmodule AutoFilter do - use Membrane.Filter + use Membrane.Filter, flow_control_hints?: false def_input_pad :input, availability: :on_request, accepted_format: _any, flow_control: :auto def_output_pad :output, availability: :on_request, accepted_format: _any, flow_control: :auto From 0bff0604a5b9569fb655bb8caf4e8dd3e9093881 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Mon, 16 Dec 2024 15:04:01 +0100 Subject: [PATCH 31/32] Algorithm description refactor --- .../element/diamond_detection_controller.ex | 31 ++++++++++--------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex index d80deaf3a..575abdf01 100644 --- a/lib/membrane/core/element/diamond_detection_controller.ex +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -37,18 +37,18 @@ defmodule Membrane.Core.Element.DiamondDetectionController do # is part of this link. # After the spec status is set to :done, the parent component that returned the spec will - # triggerall elements whose output pads have been linked in this spec. The reference of the - # trigger is always set to the spec reference. + # triggerall elements whose output pads have been linked in this spec (`type: :start_trigger`). + # The reference of the trigger is always set to the spec reference. - # If the element is triggered with a specific reference for the first time, it does two - # things: + # If the element is triggered with a specific reference (`type: :trigger`) for the first time, + # it does two things: # * the element forwards the trigger with the same reference via all input pads working - # in the pull mode + # in the pull mode (`type: :trigger`) # * if the element has at least two output pads working in the pull mode, it postpones - # the proper searching that will be spawned from itself. The time between postponing and the - # proper searching is one second. If during this time an element is triggered once - # again with a different reference, it won't cause another postponement of the proper - # searching, this means that at the time there is at most one proper searching + # the proper searching that will be spawned from itself (`type: :start_search`). The time + # between postponing and the proper searching is one second. If during this time an element + # is triggered once again with a different reference, it won't cause another postponement + # of the proper searching, this means that at the time there is at most one proper searching # postponed in the single element # (2) Proper searching: @@ -61,6 +61,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do # just started the proper searching, or maybe it was forwarded to it via a link): # * if the element sees the proper searching reference for the first time, then: # - it forwards proper searching via all output pads working in the pull mode + # (`type: :search`) # - when proper searching is forwarded, it remembers the path in the graph through # the elements that it has already passed # * if the element has already seen the reference of proper searching, but there is @@ -102,18 +103,18 @@ defmodule Membrane.Core.Element.DiamondDetectionController do :start_search -> start_search(state) - :start_trigger -> - start_trigger(message.ref, state) - :search -> handle_and_forward_search(message.pad_ref, message.ref, message.path, state) - :trigger -> - handle_and_forward_trigger(message.ref, state) - :delete_search_ref -> delete_search_ref(message.ref, state) + :start_trigger -> + start_trigger(message.ref, state) + + :trigger -> + handle_and_forward_trigger(message.ref, state) + :delete_trigger_ref -> delete_trigger_ref(message.ref, state) end From 2dff094c3562bf9d290451855b17d815165141f6 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Tue, 17 Dec 2024 11:45:52 +0100 Subject: [PATCH 32/32] Implement suggestions from CR --- .../element/diamond_detection_controller.ex | 54 +++---------------- .../diamond_detection_state.ex | 10 ++-- ...effective_flow_control_resolution_test.exs | 2 +- 3 files changed, 12 insertions(+), 54 deletions(-) diff --git a/lib/membrane/core/element/diamond_detection_controller.ex b/lib/membrane/core/element/diamond_detection_controller.ex index 575abdf01..7e4b5650a 100644 --- a/lib/membrane/core/element/diamond_detection_controller.ex +++ b/lib/membrane/core/element/diamond_detection_controller.ex @@ -74,7 +74,6 @@ defmodule Membrane.Core.Element.DiamondDetectionController do # and doesn't forward proper searching further. alias __MODULE__.{DiamondLogger, PathInGraph} - alias __MODULE__.PathInGraph.Vertex alias Membrane.Core.Element.State alias Membrane.Element.PadData @@ -82,8 +81,6 @@ defmodule Membrane.Core.Element.DiamondDetectionController do require Membrane.Logger require Membrane.Pad, as: Pad - @component_path_prefix "__membrane_component_path_64_byte_prefix________________________" - @type diamond_detection_message() :: %{ :type => :start_search @@ -101,7 +98,8 @@ defmodule Membrane.Core.Element.DiamondDetectionController do def handle_diamond_detection_message(%{type: type} = message, state) do case type do :start_search -> - start_search(state) + :ok = start_search(state) + state :search -> handle_and_forward_search(message.pad_ref, message.ref, message.path, state) @@ -120,9 +118,9 @@ defmodule Membrane.Core.Element.DiamondDetectionController do end end - @spec start_search(State.t()) :: State.t() + @spec start_search(State.t()) :: :ok defp start_search(state) do - {component_path, state} = get_component_path(state) + component_path = Membrane.ComponentPath.get_formatted() diamond_detection_path = [ %PathInGraph.Vertex{pid: self(), component_path: component_path} @@ -132,7 +130,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do make_ref() |> forward_search(diamond_detection_path, state) - state + :ok end @spec handle_and_forward_search(Pad.ref(), reference(), PathInGraph.t(), State.t()) :: @@ -143,7 +141,7 @@ defmodule Membrane.Core.Element.DiamondDetectionController do diamond_detecton_path, state ) do - {component_path, state} = get_component_path(state) + component_path = Membrane.ComponentPath.get_formatted() new_path_vertex = %PathInGraph.Vertex{ pid: self(), @@ -177,14 +175,9 @@ defmodule Membrane.Core.Element.DiamondDetectionController do state true -> - old_diamond_detection_path = - state.diamond_detection_state.ref_to_path[diamond_detection_ref] - |> remove_component_path_prefix() - :ok = - diamond_detecton_path - |> remove_component_path_prefix() - |> DiamondLogger.log_diamond(old_diamond_detection_path) + state.diamond_detection_state.ref_to_path[diamond_detection_ref] + |> DiamondLogger.log_diamond(diamond_detecton_path) state end @@ -315,36 +308,5 @@ defmodule Membrane.Core.Element.DiamondDetectionController do :ok end - defp get_component_path(state) do - case state.diamond_detection_state.serialized_component_path do - nil -> - # adding @component_path_prefix to component path causes that component path - # always has more than 64 bytes, so it won't be copied during sending a message - - component_path = - [@component_path_prefix | Membrane.ComponentPath.get()] - |> Enum.join() - - state = - state - |> put_in( - [:diamond_detection_state, :serialized_component_path], - component_path - ) - - {component_path, state} - - component_path -> - {component_path, state} - end - end - defp have_common_prefix?(path_a, path_b), do: List.last(path_a) == List.last(path_b) - - defp remove_component_path_prefix(path_in_graph) do - path_in_graph - |> Enum.map(fn %Vertex{component_path: @component_path_prefix <> component_path} = vertex -> - %{vertex | component_path: component_path} - end) - end end diff --git a/lib/membrane/core/element/diamond_detection_controller/diamond_detection_state.ex b/lib/membrane/core/element/diamond_detection_controller/diamond_detection_state.ex index bade2c96d..da2545177 100644 --- a/lib/membrane/core/element/diamond_detection_controller/diamond_detection_state.ex +++ b/lib/membrane/core/element/diamond_detection_controller/diamond_detection_state.ex @@ -5,15 +5,11 @@ defmodule Membrane.Core.Element.DiamondDetectionController.DiamondDatectionState alias Membrane.Core.Element.DiamondDetectionController.PathInGraph - defstruct [ - :serialized_component_path, - ref_to_path: %{}, - trigger_refs: MapSet.new(), - postponed?: false - ] + defstruct ref_to_path: %{}, + trigger_refs: MapSet.new(), + postponed?: false @type t :: %__MODULE__{ - serialized_component_path: String.t() | nil, ref_to_path: %{optional(reference()) => PathInGraph.t()}, trigger_refs: MapSet.t(reference()), postponed?: boolean() diff --git a/test/membrane/integration/effective_flow_control_resolution_test.exs b/test/membrane/integration/effective_flow_control_resolution_test.exs index 894207fef..ca95853ed 100644 --- a/test/membrane/integration/effective_flow_control_resolution_test.exs +++ b/test/membrane/integration/effective_flow_control_resolution_test.exs @@ -7,7 +7,7 @@ defmodule Membrane.Integration.EffectiveFlowControlResolutionTest do alias Membrane.Testing defmodule AutoFilter do - use Membrane.Filter + use Membrane.Filter, flow_control_hints?: false def_input_pad :input, availability: :on_request, accepted_format: _any def_output_pad :output, availability: :on_request, accepted_format: _any