Membrane Live dev #188

Karolk99 · 2022-11-04T10:43:33Z

after merging this PR draft (#182) will be cloused.

mickel8

I didn't manage to take a look at HLS Endpoint itself but will do this tomorrow

lib/membrane_rtc_engine/engine.ex

mix.exs

lib/membrane_rtc_engine/endpoints/hls/ffmpeg_filter.ex

test/membrane_rtc_engine/hls_endpoint_test.exs

test/hls_reference/single-track-h264/g2gCZAAFdmlkZW9tAAAADXRlc3QtdHJhY2staWQ.m3u8

test/membrane_rtc_engine/hls_endpoint_test.exs

Rados13

Also, hls tests fail on CI, so my question is whether these tests are deterministic?

lib/membrane_rtc_engine/endpoints/hls_endpoint.ex

lib/membrane_rtc_engine/endpoints/hls/ffmpeg_filter.ex

test/hls_reference/single-track-h264/g2gCZAAFdmlkZW9tAAAADXRlc3QtdHJhY2staWQ.m3u8

test/support/file_endpoint.ex

lib/membrane_rtc_engine/engine.ex

mickel8 · 2022-11-14T22:18:11Z

lib/membrane_rtc_engine/endpoints/hls/ffmpeg_filter.ex

+  Returns FFmpeg filtergraph that is used to create compositor output.
+  https://ffmpeg.org/ffmpeg-filters.html#toc-Filtergraph-description
+
+  Used ffmpeg options:


That's much better 🙂

lib/membrane_rtc_engine/endpoints/hls/compositor_config.ex

lib/membrane_rtc_engine/endpoints/hls_endpoint.ex

mickel8 · 2022-12-05T12:47:59Z

Unpinning myself from CR as the whole development is done in PRs to this branch

codecov · 2023-02-03T15:26:14Z

Codecov Report

Merging #188 (688705f) into master (d5c4263) will increase coverage by 2.82%.
The diff coverage is 74.44%.

❗ Current head 688705f differs from pull request most recent head dcffa6e. Consider uploading reports for the commit dcffa6e to get more accurate results

@@            Coverage Diff             @@
##           master     #188      +/-   ##
==========================================
+ Coverage   56.73%   59.55%   +2.82%     
==========================================
  Files          35       38       +3     
  Lines        1641     1721      +80     
==========================================
+ Hits          931     1025      +94     
+ Misses        710      696      -14

Impacted Files	Coverage Δ
...embrane_rtc_engine/endpoints/webrtc/media_event.ex	`5.35% <ø> (ø)`
...ne_rtc_engine/endpoints/webrtc/variant_selector.ex	`84.61% <ø> (+1.53%)`	⬆️
...b/membrane_rtc_engine/endpoints/webrtc_endpoint.ex	`0.00% <0.00%> (ø)`
lib/membrane_rtc_engine/tees/filter_tee.ex	`0.00% <0.00%> (ø)`
lib/membrane_rtc_engine/track.ex	`77.77% <ø> (ø)`
lib/membrane_rtc_engine/utils.ex	`12.12% <ø> (ø)`
...rane_rtc_engine/endpoints/webrtc/track_receiver.ex	`50.00% <11.11%> (-2.88%)`	⬇️
...mbrane_rtc_engine/endpoints/webrtc/track_sender.ex	`71.42% <37.50%> (+0.29%)`	⬆️
...ne_rtc_engine/endpoints/hls/mobile_layout_maker.ex	`60.00% <60.00%> (ø)`
test/support/test_sink.ex	`80.00% <62.50%> (+1.42%)`	⬆️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5c4263...dcffa6e. Read the comment docs.

Rados13 · 2023-02-10T06:44:10Z

lib/membrane_rtc_engine/endpoints/hls/caps_updater.ex

+    new_buffers =
+      state
+      |> get_in([:buffers, state.update_queue])
+      |> then(&[buffer | &1])
+
+    state = put_in(state, [:buffers, state.update_queue], new_buffers)


Suggested change

new_buffers =

state

|> get_in([:buffers, state.update_queue])

|> then(&[buffer | &1])

state = put_in(state, [:buffers, state.update_queue], new_buffers)

state = update_in(state,[:buffers, state.update_queue],&[buffer | &1])

Rados13 · 2023-02-10T06:48:32Z

lib/membrane_rtc_engine/endpoints/hls/caps_updater.ex

+    buffers =
+      state
+      |> get_in([:buffers, state.update_queue])
+      |> Enum.reverse()
+
+    actions = [buffer: {:output, buffers}] ++ maybe_notify_end_of_stream(state)
+
+    new_buffers =
+      state
+      |> Map.get(:buffers)
+      |> Map.delete(state.update_queue)
+
+    {actions, %{state | buffers: new_buffers, update_queue: state.update_queue - 1}}


Suggested change

buffers =

state

|> get_in([:buffers, state.update_queue])

|> Enum.reverse()

actions = [buffer: {:output, buffers}] ++ maybe_notify_end_of_stream(state)

new_buffers =

state

|> Map.get(:buffers)

|> Map.delete(state.update_queue)

{actions, %{state | buffers: new_buffers, update_queue: state.update_queue - 1}}

{buffers,state} = pop_in([:buffers, state.update_queue])

buffers = Enum.reverse(buffers)

actions = [buffer: {:output, buffers}] ++ maybe_notify_end_of_stream(state)

{actions, %{state | update_queue: state.update_queue - 1}}

Rados13 · 2023-02-10T06:52:51Z

lib/membrane_rtc_engine/endpoints/hls/config/audio_mixer_config.ex

+  """
+
+  @typedoc """
+  * `channels` - number of channels inside a frame. Default value is 1.


I think a frame is a term related to video and doesn't have much sense in the context of audio, but I am maybe wrong.

The same term is widely used in AudioRaw documentation.

Rados13 · 2023-02-10T07:24:11Z

lib/membrane_rtc_engine/endpoints/hls/config/sink_bin_config.ex

+    @typedoc """
+      To read more about config options go to module Membrane.HTTPAdaptiveStream.SinkBin and read options descriptions.
+    """
+
+    alias Membrane.HTTPAdaptiveStream.{Manifest, Storage}


Suggested change

@typedoc """

To read more about config options go to module Membrane.HTTPAdaptiveStream.SinkBin and read options descriptions.

"""

alias Membrane.HTTPAdaptiveStream.{Manifest, Storage}

alias Membrane.HTTPAdaptiveStream.{Manifest, Storage}

@typedoc """

To read more about config options go to module Membrane.HTTPAdaptiveStream.SinkBin and read options descriptions.

"""

Rados13 · 2023-02-10T07:25:38Z

lib/membrane_rtc_engine/endpoints/hls/config/sink_bin_config.ex

+
+    @type t() :: %__MODULE__{
+            manifest_name: String.t(),
+            manifest_module: (Path.t() -> module),


This type doesn't match the default value, isn't there some error?

Rados13 · 2023-02-10T08:40:53Z

lib/membrane_rtc_engine/endpoints/hls_endpoint.ex

+          stream_beginning: nil,
+          video_layout_state: nil,
+          stream_ids: MapSet.new(),
+          video_layout_tracks_added: %{}


Why do we need this map and where do we put anything in it?

Rados13 · 2023-02-10T09:03:22Z

lib/membrane_rtc_engine/endpoints/hls/caps_updater.ex

@@ -0,0 +1,73 @@
+defmodule Membrane.RTC.Engine.Endpoint.HLS.StreamFormatUpdater do


Outdated file name.

Rados13 · 2023-02-10T09:16:51Z

lib/membrane_rtc_engine/endpoints/hls_endpoint.ex

+    end
+
+    defp get_track_offset(%{stream_beginning: nil} = state),
+      do: {0, Map.put(state, :stream_beginning, System.monotonic_time())}


Suggested change

do: {0, Map.put(state, :stream_beginning, System.monotonic_time())}

do: {0, %{state | stream_beginning: System.monotonic_time()})}

Rados13 · 2023-02-10T09:23:55Z

mix.exs

+      {:membrane_audio_mix_plugin, "~> 0.12.0", optional: true},
+      {:membrane_generator_plugin, "~> 0.8.0", optional: true},
+      {:membrane_realtimer_plugin, "~> 0.6.0", optional: true},
+      {:membrane_audio_filler_plugin, "~> 0.1.0"},


Why isn't this dependency optional?

Rados13 · 2023-02-10T09:35:50Z

test/membrane_rtc_engine/hls_endpoint_test.exs

+      for output_file <- output_files do
+        output_path = Path.join(output_dir, output_file)
+        %{size: size} = File.stat!(output_path)
+        assert size > 0
+      end


We use this fragment in multiple tests could we move it to a function?

Rados13 · 2023-02-14T08:54:13Z

lib/membrane_rtc_engine/endpoints/hls/config/mixer_config.ex

-  @moduledoc """
-  Module representing mixer configuration for the HLS endpoint.
-  """
+if Code.ensure_loaded?(Membrane.RawAudio) do


Shouldn't we for consistency add also check for RawVideo?

Rados13 · 2023-02-14T09:25:34Z

lib/membrane_rtc_engine/endpoints/hls_endpoint.ex

@@ -148,7 +148,6 @@ if Enum.all?(
          tracks: %{},
          stream_beginning: nil,
          video_layout_state: nil,
-          stream_ids: MapSet.new(),
          video_layout_tracks_added: %{}


As mentioned in our talk I think we should add a test for this whether we properly call functions from VideoLayoutMaker behavior.

tests added

… config

mickel8

Looks really cool 👍 I had a couple of suggestions regarding the API

lib/membrane_rtc_engine/endpoints/hls/config/audio_mixer_config.ex

lib/membrane_rtc_engine/endpoints/hls/config/compositor_config.ex

lib/membrane_rtc_engine/endpoints/hls/config/mixer_config.ex

lib/membrane_rtc_engine/endpoints/hls/video_layout_maker.ex

mickel8 · 2023-02-15T20:47:09Z

lib/membrane_rtc_engine/endpoints/hls/config/sink_bin_config.ex

+    alias Membrane.HTTPAdaptiveStream.{Manifest, Storage}
+
+    @typedoc """
+      To read more about config options go to module Membrane.HTTPAdaptiveStream.SinkBin and read options descriptions.


Suggested change

To read more about config options go to module Membrane.HTTPAdaptiveStream.SinkBin and read options descriptions.

To read more about config options go to module `Membrane.HTTPAdaptiveStream.SinkBin` and read options descriptions.

lib/membrane_rtc_engine/endpoints/hls/config/sink_bin_config.ex

mickel8 · 2023-02-15T20:49:38Z

lib/membrane_rtc_engine/endpoints/hls/mobile_layout_maker.ex

+    Module representing function for updating video layout for the HLS stream.
+
+    1) Only main presenter
+         _ _ _ _
+        |       |
+        |       |
+        |       |
+        |       |
+        |       |
+         - - - -
+    2) Main presenter and one side presenter
+         _ _ _ _
+        |       |
+        |       |
+        |       |
+         - -    |
+        |   |   |
+         - - - -
+
+    3) Main presenter and two side presenters
+         _ _ _ _
+        |       |
+        |       |
+        |       |
+         - - - -
+        |   |   |
+         - - - -


Really cool docs 👍

test/support/file_endpoint.ex

mickel8 · 2023-02-16T16:09:41Z

lib/membrane_rtc_engine/endpoints/hls_endpoint.ex

-                  default: SinkBinConfig,
+                hls_config: [
+                  spec: HLSConfig.t(),
+                  default: HLSConfig,


Suggested change

default: HLSConfig,

default: %HLSConfig{},

Karolk99 requested a review from mickel8 as a code owner November 4, 2022 10:43

Karolk99 requested a review from Rados13 November 4, 2022 10:43

Karolk99 marked this pull request as draft November 4, 2022 10:44

mickel8 suggested changes Nov 7, 2022

View reviewed changes

Rados13 reviewed Nov 9, 2022

View reviewed changes

lib/membrane_rtc_engine/endpoints/hls_endpoint.ex Outdated Show resolved Hide resolved

lib/membrane_rtc_engine/endpoints/hls_endpoint.ex Outdated Show resolved Hide resolved

lib/membrane_rtc_engine/endpoints/hls_endpoint.ex Outdated Show resolved Hide resolved

mickel8 suggested changes Nov 9, 2022

View reviewed changes

lib/membrane_rtc_engine/endpoints/hls_endpoint.ex Outdated Show resolved Hide resolved

lib/membrane_rtc_engine/endpoints/hls_endpoint.ex Outdated Show resolved Hide resolved

Karolk99 force-pushed the add-compositor-with-synchronization branch 7 times, most recently from cbab3f3 to b1395a9 Compare November 9, 2022 16:13

Karolk99 requested review from mickel8 and Rados13 November 9, 2022 16:34

Karolk99 changed the title ~~Add compositor and audio mixer to hls~~ Membrane Live dev Nov 10, 2022

mickel8 reviewed Nov 14, 2022

View reviewed changes

mickel8 suggested changes Nov 14, 2022

View reviewed changes

test/support/file_endpoint.ex Outdated Show resolved Hide resolved

lib/membrane_rtc_engine/engine.ex Outdated Show resolved Hide resolved

Karolk99 requested a review from mickel8 November 14, 2022 16:30

mickel8 suggested changes Nov 14, 2022

View reviewed changes

Karolk99 requested a review from mickel8 November 15, 2022 09:49

Rados13 reviewed Nov 15, 2022

View reviewed changes

lib/membrane_rtc_engine/endpoints/hls_endpoint.ex Outdated Show resolved Hide resolved

Karolk99 requested a review from Rados13 November 15, 2022 13:14

Rados13 previously approved these changes Nov 17, 2022

View reviewed changes

LVala force-pushed the add-compositor-with-synchronization branch from fd74911 to 48b71a4 Compare November 22, 2022 13:55

roznawsk force-pushed the add-compositor-with-synchronization branch from 48b71a4 to bd5f80b Compare November 23, 2022 10:41

mickel8 removed their request for review December 5, 2022 12:46

Karolk99 added 2 commits December 19, 2022 10:12

Add compositor and audio mixer to hls

0f9c4cd

Add hls stream without compositor

a2772cb

Karolk99 requested review from Rados13 and LVala February 3, 2023 15:23

mickel8 force-pushed the core-v0.11 branch from 5eefd91 to 74f188b Compare February 7, 2023 16:50

Rados13 reviewed Feb 10, 2023

View reviewed changes

Karolk99 requested a review from Rados13 February 13, 2023 14:25

Karolk99 force-pushed the add-compositor-with-synchronization branch 4 times, most recently from e1fd1e0 to 2d2c779 Compare February 13, 2023 15:03

Change requests

a7887b5

Karolk99 force-pushed the add-compositor-with-synchronization branch from 2d2c779 to a7887b5 Compare February 13, 2023 15:25

Rados13 reviewed Feb 14, 2023

View reviewed changes

Karolk99 added 4 commits February 14, 2023 11:30

Add test for hls notification 'update_layou'

64a090d

Merge branch 'core-v0.11' into add-compositor-with-synchronization

9cd47aa

mix format

59e14ea

Fix integration tests and look check for RawVideo dependency in mixer…

930dfe9

… config

Karolk99 requested a review from Rados13 February 14, 2023 15:40

Rados13 approved these changes Feb 15, 2023

View reviewed changes

mickel8 suggested changes Feb 15, 2023

View reviewed changes

mickel8 force-pushed the core-v0.11 branch from 07ff440 to d55ed83 Compare February 16, 2023 15:37

Requested changes

df5e4f2

mickel8 marked this pull request as ready for review February 16, 2023 15:54

mickel8 approved these changes Feb 16, 2023

View reviewed changes

Karolk99 and others added 2 commits February 16, 2023 17:18

Requested changes

688705f

Merge branch 'core-v0.11' into add-compositor-with-synchronization

dcffa6e

Base automatically changed from core-v0.11 to master February 16, 2023 16:23

mickel8 changed the base branch from master to core-v0.11 February 16, 2023 16:28

mickel8 merged commit 0512d7c into core-v0.11 Feb 16, 2023

mickel8 deleted the add-compositor-with-synchronization branch February 16, 2023 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Membrane Live dev #188

Membrane Live dev #188

Karolk99 commented Nov 4, 2022

mickel8 left a comment

Rados13 left a comment

mickel8 Nov 14, 2022

mickel8 commented Dec 5, 2022

codecov bot commented Feb 3, 2023 •

edited

Loading

Rados13 Feb 10, 2023

Rados13 Feb 10, 2023

Rados13 Feb 10, 2023

Karolk99 Feb 13, 2023

Rados13 Feb 10, 2023

Rados13 Feb 10, 2023

Rados13 Feb 10, 2023

Rados13 Feb 10, 2023

Rados13 Feb 10, 2023

Rados13 Feb 10, 2023

Rados13 Feb 10, 2023

Rados13 Feb 14, 2023

Rados13 Feb 14, 2023

Karolk99 Feb 14, 2023

mickel8 left a comment

mickel8 Feb 15, 2023

mickel8 Feb 15, 2023

mickel8 Feb 16, 2023

		@@ -0,0 +1,73 @@
		defmodule Membrane.RTC.Engine.Endpoint.HLS.StreamFormatUpdater do

	do: {0, Map.put(state, :stream_beginning, System.monotonic_time())}
	do: {0, %{state \| stream_beginning: System.monotonic_time()})}

	To read more about config options go to module Membrane.HTTPAdaptiveStream.SinkBin and read options descriptions.
	To read more about config options go to module `Membrane.HTTPAdaptiveStream.SinkBin` and read options descriptions.

Membrane Live dev #188

Membrane Live dev #188

Conversation

Karolk99 commented Nov 4, 2022

mickel8 left a comment

Choose a reason for hiding this comment

Rados13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickel8 commented Dec 5, 2022

codecov bot commented Feb 3, 2023 • edited Loading

Codecov Report

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

mickel8 left a 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

codecov bot commented Feb 3, 2023 •

edited

Loading