From 2ff259141ab9e68587ed7728682e97e3fc148d75 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Wed, 9 Apr 2025 16:40:14 +0200 Subject: [PATCH 1/7] Implement transcoding_policy option --- lib/transcoder.ex | 39 +++++++++++++++++++++++++-------------- lib/transcoder/audio.ex | 23 ++++++++++++++++------- lib/transcoder/video.ex | 22 +++++++++++++++------- test/integration_test.exs | 31 +++++++++++++++++++++++++++---- 4 files changed, 83 insertions(+), 32 deletions(-) diff --git a/lib/transcoder.ex b/lib/transcoder.ex index 3a6886f..2fa29c0 100644 --- a/lib/transcoder.ex +++ b/lib/transcoder.ex @@ -69,16 +69,27 @@ defmodule Membrane.Transcoder do and is supposed to return the desired output stream format or its module. """ ], - force_transcoding?: [ - spec: boolean() | (stream_format() -> boolean()), - default: false, + transcoding_policy: [ + spec: :always | :if_needed | :never, + default: :if_needed, description: """ - If set to `true`, the input media stream will be decoded and encoded, even - if the input stream format and the output stream format are the same type. + Specifies, when transcoding should be appliead. Can be either: - * a boolean, - * a function that receives the input stream format and returns a boolean. + * an atom: `:always`, `:if_needed` (default) or `:never`, + * a function that receives the input stream format and returns an atom. + + If set to `:always`, the input media stream will be decoded and encoded, even + if the input stream format and the output stream format are the same type. + + If set to `:if_needed`, the input media stream will be transcoded only if the input + stream format and the output stream format are different types. + This is the default behavior. + + If set to `:never`, the input media stream won't be neither deocded nor transcoded. + Changing alignment, encapsulation or stream structure is still possible. This option + is helpful when you want to ensure that #{inspect(__MODULE__)} will not use too much + of resources, e.g. CPU or memory. """ ] @@ -109,8 +120,8 @@ defmodule Membrane.Transcoder do |> resolve_output_stream_format() state = - with %{force_transcoding?: f} when is_function(f) <- state do - %{state | force_transcoding?: f.(format)} + with %{transcoding_policy: f} when is_function(f) <- state do + %{state | transcoding_policy: f.(format)} end spec = @@ -118,7 +129,7 @@ defmodule Membrane.Transcoder do |> plug_transcoding( format, state.output_stream_format, - state.force_transcoding? + state.transcoding_policy ) |> get_child(:output_funnel) @@ -160,15 +171,15 @@ defmodule Membrane.Transcoder do end end - defp plug_transcoding(builder, input_format, output_format, force_transcoding?) + defp plug_transcoding(builder, input_format, output_format, transcoding_policy) when Audio.is_audio_format(input_format) do builder - |> Audio.plug_audio_transcoding(input_format, output_format, force_transcoding?) + |> Audio.plug_audio_transcoding(input_format, output_format, transcoding_policy) end - defp plug_transcoding(builder, input_format, output_format, force_transcoding?) + defp plug_transcoding(builder, input_format, output_format, transcoding_policy) when Video.is_video_format(input_format) do builder - |> Video.plug_video_transcoding(input_format, output_format, force_transcoding?) + |> Video.plug_video_transcoding(input_format, output_format, transcoding_policy) end end diff --git a/lib/transcoder/audio.ex b/lib/transcoder/audio.ex index 074ffe7..b418fa5 100644 --- a/lib/transcoder/audio.ex +++ b/lib/transcoder/audio.ex @@ -35,17 +35,18 @@ defmodule Membrane.Transcoder.Audio do audio_stream_format(), boolean() ) :: ChildrenSpec.builder() - def plug_audio_transcoding(builder, input_format, output_format, force_transcoding?) + def plug_audio_transcoding(builder, input_format, output_format, transcoding_policy) when is_audio_format(input_format) and is_audio_format(output_format) do - do_plug_audio_transcoding(builder, input_format, output_format, force_transcoding?) + do_plug_audio_transcoding(builder, input_format, output_format, transcoding_policy) end defp do_plug_audio_transcoding( builder, %format_module{}, %format_module{}, - false = _force_transcoding? - ) do + transcoding_policy + ) + when transcoding_policy in [:if_needed, :never] do Membrane.Logger.debug(""" This bin will only forward buffers, as the input stream format is the same as the output stream format. """) @@ -57,12 +58,20 @@ defmodule Membrane.Transcoder.Audio do builder, %RemoteStream{content_format: Opus}, %Opus{}, - false = _force_transcoding? - ) do + transcoding_policy + ) + when transcoding_policy in [:if_needed, :never] do builder |> child(:opus_parser, Opus.Parser) end - defp do_plug_audio_transcoding(builder, input_format, output_format, _force_transcoding?) do + defp do_plug_audio_transcoding(_builder, input_format, output_format, :never) do + raise """ + Cannot convert input format #{inspect(input_format)} to output format #{inspect(output_format)} \ + with :transcoding_policy option set to :never. + """ + end + + defp do_plug_audio_transcoding(builder, input_format, output_format, _transcoding_policy) do builder |> maybe_plug_parser(input_format) |> maybe_plug_decoder(input_format) diff --git a/lib/transcoder/video.ex b/lib/transcoder/video.ex index 933e6fb..3dc53d7 100644 --- a/lib/transcoder/video.ex +++ b/lib/transcoder/video.ex @@ -19,18 +19,18 @@ defmodule Membrane.Transcoder.Video do video_stream_format(), boolean() ) :: ChildrenSpec.builder() - def plug_video_transcoding(builder, input_format, output_format, force_transcoding?) + def plug_video_transcoding(builder, input_format, output_format, transcoding_policy) when is_video_format(input_format) and is_video_format(output_format) do - do_plug_video_transcoding(builder, input_format, output_format, force_transcoding?) + do_plug_video_transcoding(builder, input_format, output_format, transcoding_policy) end defp do_plug_video_transcoding( builder, %h26x{}, %h26x{} = output_format, - false = _force_transcoding? + transcoding_policy ) - when h26x in [H264, H265] do + when h26x in [H264, H265] and transcoding_policy in [:if_needed, :never] do parser = h26x |> Module.concat(Parser) @@ -46,8 +46,9 @@ defmodule Membrane.Transcoder.Video do builder, %format_module{}, %format_module{}, - false = _force_transcoding? - ) do + transcoding_policy + ) + when transcoding_policy in [:if_needed, :never] do Membrane.Logger.debug(""" This bin will only forward buffers, as the input stream format is the same type as the output stream format. """) @@ -55,7 +56,14 @@ defmodule Membrane.Transcoder.Video do builder end - defp do_plug_video_transcoding(builder, input_format, output_format, _force_transcoding?) do + defp do_plug_video_transcoding(_builder, input_format, output_format, :never) do + raise """ + Cannot convert input format #{inspect(input_format)} to output format #{inspect(output_format)} \ + with :transcoding_policy option set to :never. + """ + end + + defp do_plug_video_transcoding(builder, input_format, output_format, _transcoding_policy) do builder |> maybe_plug_parser_and_decoder(input_format) |> maybe_plug_encoder_and_parser(output_format) diff --git a/test/integration_test.exs b/test/integration_test.exs index e633cbf..cd5b5ec 100644 --- a/test/integration_test.exs +++ b/test/integration_test.exs @@ -67,14 +67,14 @@ defmodule Membrane.Transcoder.IntegrationTest do do: {[stream_format: {:output, state.format}], state} end - test "if encoder and decoder are spawned or not, depending on the value of `force_transcoding?` option" do + test "if encoder and decoder are spawned or not, depending on the value of `transcoding_policy` option" do for format <- [%AAC{channels: 1}, %H264{alignment: :au, stream_structure: :annexb}], - force_transcoding? <- [true, false] do + transcoding_policy <- [:always, :if_needed, :never] do spec = child(:source, %FormatSource{format: format}) |> child(:transcoder, %Membrane.Transcoder{ output_stream_format: format, - force_transcoding?: force_transcoding? + transcoding_policy: transcoding_policy }) |> child(:sink, Testing.Sink) @@ -89,7 +89,7 @@ defmodule Membrane.Transcoder.IntegrationTest do |> Enum.each(fn child_name -> get_child_result = Testing.Pipeline.get_child_pid(pipeline, [:transcoder, child_name]) - if force_transcoding? do + if transcoding_policy == :always do assert {:ok, child_pid} = get_child_result assert is_pid(child_pid) else @@ -100,4 +100,27 @@ defmodule Membrane.Transcoder.IntegrationTest do Testing.Pipeline.terminate(pipeline) end end + + @tag :xd + test "if transcoder raises `transcoding_policy` is set to `:never` and formats don't match" do + spec = + child(:source, %FormatSource{format: %H264{alignment: :au, stream_structure: :annexb}}) + |> child(:transcoder, %Membrane.Transcoder{ + output_stream_format: VP8, + transcoding_policy: :never + }) + |> child(:sink, Testing.Sink) + + {:ok, pipeline, supervisor} = Testing.Pipeline.start_supervised() + # supervisor_ref = Process.monitor(supervisor) + pipeline_ref = Process.monitor(pipeline) + + Testing.Pipeline.execute_actions(pipeline, spec: spec) + + assert_receive {:DOWN, ^pipeline_ref, :process, _pid, + {:membrane_child_crash, :transcoder, + {%RuntimeError{}, _transcoder_stacktrace}}} + + # assert_receive {:DOWN, ^supervisor_ref, :process, _pid, _reason} + end end From b2cc3b1f59ed8b95089a3f7313892155c4e762f4 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Thu, 10 Apr 2025 09:28:28 +0200 Subject: [PATCH 2/7] Start capturing logs --- test/integration_test.exs | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/test/integration_test.exs b/test/integration_test.exs index cd5b5ec..007b8b5 100644 --- a/test/integration_test.exs +++ b/test/integration_test.exs @@ -70,10 +70,16 @@ defmodule Membrane.Transcoder.IntegrationTest do test "if encoder and decoder are spawned or not, depending on the value of `transcoding_policy` option" do for format <- [%AAC{channels: 1}, %H264{alignment: :au, stream_structure: :annexb}], transcoding_policy <- [:always, :if_needed, :never] do + output_format = + case format do + %H264{} -> %H264{format | stream_structure: :avc1} + %AAC{} -> format + end + spec = child(:source, %FormatSource{format: format}) |> child(:transcoder, %Membrane.Transcoder{ - output_stream_format: format, + output_stream_format: output_format, transcoding_policy: transcoding_policy }) |> child(:sink, Testing.Sink) @@ -101,8 +107,7 @@ defmodule Membrane.Transcoder.IntegrationTest do end end - @tag :xd - test "if transcoder raises `transcoding_policy` is set to `:never` and formats don't match" do + test "if transcoder raises when `transcoding_policy` is set to `:never` and formats don't match" do spec = child(:source, %FormatSource{format: %H264{alignment: :au, stream_structure: :annexb}}) |> child(:transcoder, %Membrane.Transcoder{ @@ -111,16 +116,15 @@ defmodule Membrane.Transcoder.IntegrationTest do }) |> child(:sink, Testing.Sink) - {:ok, pipeline, supervisor} = Testing.Pipeline.start_supervised() - # supervisor_ref = Process.monitor(supervisor) + {:ok, supervisor, pipeline} = Testing.Pipeline.start(spec: []) + supervisor_ref = Process.monitor(supervisor) pipeline_ref = Process.monitor(pipeline) Testing.Pipeline.execute_actions(pipeline, spec: spec) assert_receive {:DOWN, ^pipeline_ref, :process, _pid, - {:membrane_child_crash, :transcoder, - {%RuntimeError{}, _transcoder_stacktrace}}} + {:membrane_child_crash, :transcoder, {%RuntimeError{}, _stacktrace}}} - # assert_receive {:DOWN, ^supervisor_ref, :process, _pid, _reason} + assert_receive {:DOWN, ^supervisor_ref, :process, _pid, _reason} end end From 784e2f3f3bf75a81de38ed0e0647d89af9737be1 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Thu, 10 Apr 2025 09:35:29 +0200 Subject: [PATCH 3/7] Fix typo --- lib/transcoder.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/transcoder.ex b/lib/transcoder.ex index 2fa29c0..951a1f8 100644 --- a/lib/transcoder.ex +++ b/lib/transcoder.ex @@ -86,7 +86,7 @@ defmodule Membrane.Transcoder do stream format and the output stream format are different types. This is the default behavior. - If set to `:never`, the input media stream won't be neither deocded nor transcoded. + If set to `:never`, the input media stream won't be neither decoded nor transcoded. Changing alignment, encapsulation or stream structure is still possible. This option is helpful when you want to ensure that #{inspect(__MODULE__)} will not use too much of resources, e.g. CPU or memory. From b8976673ddfafd794e84fb07706130ebff328a52 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Thu, 10 Apr 2025 09:36:22 +0200 Subject: [PATCH 4/7] Bump version to 0.3.0 --- README.md | 2 +- mix.exs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 50681c7..511cde0 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ The package can be installed by adding `membrane_transcoder_plugin` to your list ```elixir def deps do [ - {:membrane_transcoder_plugin, "~> 0.2.1"} + {:membrane_transcoder_plugin, "~> 0.3.0"} ] end ``` diff --git a/mix.exs b/mix.exs index 956221f..498db77 100644 --- a/mix.exs +++ b/mix.exs @@ -1,7 +1,7 @@ defmodule Membrane.Transcoder.Plugin.Mixfile do use Mix.Project - @version "0.2.1" + @version "0.3.0" @github_url "https://github.com/membraneframework/membrane_transcoder_plugin" def project do From 11b60c4ff2c34996463fb8c28cc245642608b9c2 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Thu, 10 Apr 2025 09:38:28 +0200 Subject: [PATCH 5/7] Fix typespec --- lib/transcoder.ex | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/transcoder.ex b/lib/transcoder.ex index 951a1f8..99e020a 100644 --- a/lib/transcoder.ex +++ b/lib/transcoder.ex @@ -70,7 +70,11 @@ defmodule Membrane.Transcoder do """ ], transcoding_policy: [ - spec: :always | :if_needed | :never, + spec: + :always + | :if_needed + | :never + | (stream_format() -> :always | :if_needed | :never), default: :if_needed, description: """ Specifies, when transcoding should be appliead. From b8e23d3633bb1654f6ed2c32436fff96e882944c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Feliks=20Pobiedzi=C5=84ski?= <38541925+FelonEkonom@users.noreply.github.com> Date: Thu, 10 Apr 2025 11:17:07 +0200 Subject: [PATCH 6/7] Update lib/transcoder.ex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Łukasz Kita --- lib/transcoder.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/transcoder.ex b/lib/transcoder.ex index 99e020a..700603a 100644 --- a/lib/transcoder.ex +++ b/lib/transcoder.ex @@ -77,7 +77,7 @@ defmodule Membrane.Transcoder do | (stream_format() -> :always | :if_needed | :never), default: :if_needed, description: """ - Specifies, when transcoding should be appliead. + Specifies, when transcoding should be applied. Can be either: * an atom: `:always`, `:if_needed` (default) or `:never`, From 311569431fc78a0fba73a5e4abd1c3e1c0d534a3 Mon Sep 17 00:00:00 2001 From: "feliks.pobiedzinski@swmansion.com" Date: Thu, 10 Apr 2025 11:27:59 +0200 Subject: [PATCH 7/7] Implement suggestion from CR --- lib/transcoder.ex | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/transcoder.ex b/lib/transcoder.ex index 700603a..2c0ba0b 100644 --- a/lib/transcoder.ex +++ b/lib/transcoder.ex @@ -81,7 +81,8 @@ defmodule Membrane.Transcoder do Can be either: * an atom: `:always`, `:if_needed` (default) or `:never`, - * a function that receives the input stream format and returns an atom. + * a function that receives the input stream format and returns either `:always`, + `:if_needed` or `:never`. If set to `:always`, the input media stream will be decoded and encoded, even if the input stream format and the output stream format are the same type. @@ -90,10 +91,13 @@ defmodule Membrane.Transcoder do stream format and the output stream format are different types. This is the default behavior. - If set to `:never`, the input media stream won't be neither decoded nor transcoded. + If set to `:never`, the input media stream won't be neither decoded nor encoded. Changing alignment, encapsulation or stream structure is still possible. This option is helpful when you want to ensure that #{inspect(__MODULE__)} will not use too much of resources, e.g. CPU or memory. + + If the transition from the input stream format to the output stream format is not + possible without decoding or encoding the stream, an error will be raised. """ ]