Detect diamonds #909

FelonEkonom · 2024-11-21T10:42:27Z

Closes #873

mat-hek · 2024-11-28T12:49:37Z

lib/membrane/core/element/diamond_detection_controller.ex

+    # 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


I'm not convinced it's worth the hassle. Did you do any benchmarks? BTW, it seems we construct this 'serialized' component path every time we need it - I'd do that once and keep it in state.

I see it's now constructed once, but I'm still unconvinced about prepending the prefix. Handling ref-counted binaries strains GC, and copying may be more efficient if the binary is small, so I wouldn't force either way, at least without benchmarks.

mat-hek · 2024-11-28T12:51:28Z

lib/membrane/core/element/diamond_detection_controller.ex

@@ -0,0 +1,207 @@
+defmodule Membrane.Core.Element.DiamondDetectionController do
+  @moduledoc false
+


Let's describe how the algorithm works

mat-hek · 2024-11-28T12:53:25Z

lib/membrane/core/element.ex

+  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)
+    {:noreply, state}
+  end


These messages seem quite internal to the diamond detection. I'd make them a single message and dispatch within DiamondDetectionController

mat-hek · 2024-11-28T12:54:13Z

lib/membrane/core/element/state.ex

+          diamond_detection_ref_to_path: %{
+            optional(reference()) => PathInGraph.t()
+          },
+          diamond_detection_trigger_refs: MapSet.t(reference()),
+          diamond_detection_postponed?: boolean()


let's put these into a single diamond_detection map/struct

mat-hek · 2024-12-10T13:56:02Z

lib/membrane/core/element/diamond_detection_controller.ex

+  # it means in :manual flow control or in :auto flow control if the effective flow control
+  # is set to :pull.


Suggested change

# 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.

mat-hek · 2024-12-10T13:58:01Z

lib/membrane/core/element/diamond_detection_controller.ex

+  # 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.


Suggested change

# 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)

mat-hek · 2024-12-10T13:59:12Z

lib/membrane/core/element/diamond_detection_controller.ex

+  #   * 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


Suggested change

# 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

mat-hek · 2024-12-10T14:00:47Z

lib/membrane/core/element/diamond_detection_controller.ex

+
+  # 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


Suggested change

# * if the new spec caused some new diamond to occur, this diamond will contain some of

# * if the new spec created a new diamond, this diamond will contain some of

mat-hek · 2024-12-10T14:02:25Z

lib/membrane/core/element/diamond_detection_controller.ex

+  # 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 algorithm is made of two phases: (1) triggering and (2) proper searching.


why proper searching? :P

It is supposed to be a translation of "właściwe szukanie"

Got it, 'proper' is more like 'correct'. The best term I know would be 'actual search', though I'd just go for 'search', since the whole process is called 'detection', not 'search', so there's no other 'search' unless I'm missing something ;)

mat-hek · 2024-12-10T14:07:53Z

lib/membrane/core/element/diamond_detection_controller.ex

+            :start
+            | :start_trigger
+            | :diamond_detection
+            | :trigger
+            | :delete_ref
+            | :delete_trigger_ref,


I'm not a fan of these names :P there's start, start_trigger and trigger, hard to tell the difference. diamond_detection is pretty generic. They should at least correspond to the terms used in the algorithm description (proper search?), ideally be mentioned there

mat-hek · 2024-12-10T14:14:23Z

lib/membrane/core/element/diamond_detection_controller.ex

+    # 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


I see it's now constructed once, but I'm still unconvinced about prepending the prefix. Handling ref-counted binaries strains GC, and copying may be more efficient if the binary is small, so I wouldn't force either way, at least without benchmarks.

mat-hek · 2024-12-10T14:19:05Z

lib/membrane/core/element/state.ex

+              serialized_component_path: nil,
+              ref_to_path: %{},
+              trigger_refs: MapSet.new(),
+              postponed?: false


NIT: You can move the type and initialization to the DiamondDetection module

mat-hek · 2024-12-19T11:07:06Z

lib/membrane/core/element/diamond_detection_controller.ex

-  # 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 (`type: :start_trigger`).


Suggested change

# triggerall elements whose output pads have been linked in this spec (`type: :start_trigger`).

# trigger all elements whose output pads have been linked in this spec (`type: :start_trigger`).

mat-hek · 2024-12-19T11:10:46Z

lib/membrane/core/element/diamond_detection_controller.ex

+  #     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:


Seems like 'proper' is still there, can we make it just 'searching'?

FelonEkonom added 4 commits November 20, 2024 09:47

Write separate module for diamonds detection WiP

7263c9a

Implement parallel diamond detection WiP

f4bce2d

Small refactor

e9e8c77

Fix bug

cabbcfb

FelonEkonom self-assigned this Nov 21, 2024

FelonEkonom added 12 commits November 21, 2024 12:09

Fix typo

a58b363

Comment out triggering diamond detection in parents

5a76b10

Trigger diamon detection between elements WiP

0be84b9

Trigger diamon detection between elements WiP

58c5384

Fix typos

7b5fdad

wip

646898c

Log diamond wip

30b757e

Add test wip

39030a7

Add more test cases wip

a3c6bdd

Refactor diamond detection code

9855e8a

Remove unused import

f5d6941

Improve diamond log message, fix credo

acbe87d

FelonEkonom marked this pull request as ready for review November 27, 2024 09:34

FelonEkonom requested a review from mat-hek as a code owner November 27, 2024 09:34

FelonEkonom added 6 commits November 27, 2024 10:38

Remove leftover files

f02d57e

Merge remote-tracking branch 'origin/master' into pull-diamonds

2327b24

Update changelog

35cf655

Remove leftovers

c8b586a

Fix Connector test

97d15cf

Fix typo in the comment

6314803

mat-hek requested changes Nov 28, 2024

View reviewed changes

FelonEkonom added 3 commits December 4, 2024 11:18

Refactor State due to CR

1f94101

Refactor diamond detection messages

090488a

stop creating serialized component path every time

1d003ba

FelonEkonom force-pushed the pull-diamonds branch from 97730c6 to 1d003ba Compare December 4, 2024 13:42

FelonEkonom added 3 commits December 5, 2024 16:34

Write some comments describing how the diamond detection mechanism works

501aeee

Move the comments within the file

07fcfd1

Fix typos in the algorithm description

cc9039d

FelonEkonom requested a review from mat-hek December 6, 2024 11:25

mat-hek reviewed Dec 10, 2024

View reviewed changes

FelonEkonom added 5 commits December 16, 2024 14:08

Implement CR suggestions WiP

d364df9

Implement CR suggestions WiP

c158ee8

Further refactor

8df71f8

Algorithm description refactor

0bff060

Implement suggestions from CR

2dff094

FelonEkonom requested a review from mat-hek December 17, 2024 10:46

mat-hek reviewed Dec 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect diamonds #909

Detect diamonds #909

FelonEkonom commented Nov 21, 2024 •

edited

Loading

mat-hek Nov 28, 2024

mat-hek Dec 10, 2024

mat-hek Nov 28, 2024

mat-hek Nov 28, 2024

mat-hek Nov 28, 2024

mat-hek Dec 10, 2024

mat-hek Dec 10, 2024

mat-hek Dec 10, 2024

mat-hek Dec 10, 2024

mat-hek Dec 10, 2024

FelonEkonom Dec 12, 2024

mat-hek Dec 13, 2024

mat-hek Dec 10, 2024

mat-hek Dec 10, 2024

mat-hek Dec 10, 2024

mat-hek Dec 19, 2024

mat-hek Dec 19, 2024

		# 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

		@@ -0,0 +1,207 @@
		defmodule Membrane.Core.Element.DiamondDetectionController do
		@moduledoc false

		# it means in :manual flow control or in :auto flow control if the effective flow control
		# is set to :pull.

	# 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)

	# 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

	# * if the new spec caused some new diamond to occur, this diamond will contain some of
	# * if the new spec created a new diamond, this diamond will contain some of

	# triggerall elements whose output pads have been linked in this spec (`type: :start_trigger`).
	# trigger all elements whose output pads have been linked in this spec (`type: :start_trigger`).

Detect diamonds #909

Are you sure you want to change the base?

Detect diamonds #909

Conversation

FelonEkonom commented Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FelonEkonom commented Nov 21, 2024 •

edited

Loading