diff --git a/c_src/membrane_mp3_lame_plugin/encoder.c b/c_src/membrane_mp3_lame_plugin/encoder.c index 58cad29..730f29f 100644 --- a/c_src/membrane_mp3_lame_plugin/encoder.c +++ b/c_src/membrane_mp3_lame_plugin/encoder.c @@ -21,7 +21,7 @@ void handle_destroy_state(UnifexEnv *env, State *state) { } } -UNIFEX_TERM create(UnifexEnv *env, int channels, int bitrate, int quality) { +UNIFEX_TERM create(UnifexEnv *env, int channels, int bitrate, int quality, int disable_reservoir, int cbr) { UNIFEX_TERM result; State *state = unifex_alloc_state(env); state->lame_state = NULL; @@ -40,6 +40,14 @@ UNIFEX_TERM create(UnifexEnv *env, int channels, int bitrate, int quality) { lame_set_brate(lame_state, bitrate); lame_set_quality(lame_state, quality); + if (cbr) { + lame_set_VBR(lame_state, vbr_off); + } + + if (disable_reservoir) { + lame_set_disable_reservoir(lame_state, 1); + } + if (lame_init_params(lame_state) < 0) { result = create_result_error(env, "lame_init"); goto create_exit; diff --git a/c_src/membrane_mp3_lame_plugin/encoder.spec.exs b/c_src/membrane_mp3_lame_plugin/encoder.spec.exs index 5240293..1237dfb 100644 --- a/c_src/membrane_mp3_lame_plugin/encoder.spec.exs +++ b/c_src/membrane_mp3_lame_plugin/encoder.spec.exs @@ -1,6 +1,6 @@ module Membrane.MP3.Lame.Encoder.Native -spec create(channels :: int, bitrate :: int, quality :: int) :: +spec create(channels :: int, bitrate :: int, quality :: int, disable_reservoir :: bool, cbr :: bool) :: {:ok :: label, state} | {:error :: label, reason :: atom} spec encode_frame(buffer :: payload, state) :: diff --git a/lib/membrane_mp3_lame/encoder.ex b/lib/membrane_mp3_lame/encoder.ex index 0f68015..eab66d1 100644 --- a/lib/membrane_mp3_lame/encoder.ex +++ b/lib/membrane_mp3_lame/encoder.ex @@ -53,6 +53,27 @@ defmodule Membrane.MP3.Lame.Encoder do * `5` - good quality, fast * `7` - ok quality, really fast """ + ], + disable_reservoir: [ + spec: boolean(), + default: false, + description: """ + When set to true, disables the LAME bit reservoir. This makes each + MP3 frame self-contained (main_data_begin is always 0), so frames + can be cleanly spliced at any boundary without decoder artifacts. + + Useful for live streaming with ad insertion where audio from different + sources is concatenated. + """ + ], + cbr: [ + spec: boolean(), + default: false, + description: """ + When set to true, explicitly enforces constant bitrate (CBR) mode. + CBR is already the LAME default when a bitrate is set, but this + option makes it explicit via `lame_set_VBR(vbr_off)`. + """ ] @impl true @@ -75,7 +96,9 @@ defmodule Membrane.MP3.Lame.Encoder do Native.create( @channels, state.options.bitrate, - quality_val + quality_val, + state.options.disable_reservoir, + state.options.cbr ) do {[], %{state | native: native}} else diff --git a/test/fixtures/ref.mp3 b/test/fixtures/ref.mp3 index 58685c2..c44aa97 100644 Binary files a/test/fixtures/ref.mp3 and b/test/fixtures/ref.mp3 differ diff --git a/test/integration/integration_test.exs b/test/integration/integration_test.exs index 51b9af1..1ff82e9 100644 --- a/test/integration/integration_test.exs +++ b/test/integration/integration_test.exs @@ -1,4 +1,8 @@ defmodule Membrane.MP3.Lame.Encoder.IntegrationTest do + @moduledoc """ + Integration tests for the Membrane MP3 LAME encoder plugin. + """ + use ExUnit.Case import Membrane.Testing.Assertions import Membrane.ChildrenSpec @@ -116,7 +120,93 @@ defmodule Membrane.MP3.Lame.Encoder.IntegrationTest do Pipeline.terminate(pid) end - describe "Encoder forwards timestamps corretly" do + describe "disable_reservoir option" do + test "produces frames with main_data_begin always 0" do + pid = + Pipeline.start_link_supervised!( + spec: + child(:file_src, %Membrane.File.Source{chunk_size: 4096, location: @in_path}) + |> child(:parser, %Membrane.RawAudioParser{ + stream_format: %Membrane.RawAudio{ + sample_format: :s32le, + sample_rate: 44_100, + channels: 2 + }, + overwrite_pts?: true + }) + |> child(:encoder, %Membrane.MP3.Lame.Encoder{disable_reservoir: true}) + |> child(:sink, Membrane.Testing.Sink) + ) + + # Collect frames and extract main_data_begin from each + frames = collect_frames(pid, []) |> Enum.map(&extract_main_data_begin/1) + + assert frames != [], "Expected at least one MP3 frame" + + for {main_data_begin, idx} <- Enum.with_index(frames) do + assert main_data_begin == 0, + "Frame #{idx} has main_data_begin=#{main_data_begin}, expected 0" + end + end + end + + describe "cbr option" do + test "produces constant-size frame buffers" do + pid = + Pipeline.start_link_supervised!( + spec: + child(:file_src, %Membrane.File.Source{chunk_size: 4096, location: @in_path}) + |> child(:parser, %Membrane.RawAudioParser{ + stream_format: %Membrane.RawAudio{ + sample_format: :s32le, + sample_rate: 44_100, + channels: 2 + }, + overwrite_pts?: true + }) + |> child(:encoder, %Membrane.MP3.Lame.Encoder{cbr: true, disable_reservoir: true}) + |> child(:sink, Membrane.Testing.Sink) + ) + + # Collect frame sizes from sink buffers + frame_sizes = collect_frames(pid, []) |> Enum.map(&byte_size/1) + + assert length(frame_sizes) > 1, "Expected multiple MP3 frames, got #{length(frame_sizes)}" + + # CBR frames should be constant size, with at most 1-byte variation + # from the MP3 padding bit (used to maintain exact average bitrate). + # Allow the last frame to differ (flush frame may be shorter). + main_sizes = Enum.drop(frame_sizes, -1) + {min_size, max_size} = Enum.min_max(main_sizes) + + assert max_size - min_size <= 1, + "Expected constant frame size (±1 byte padding), got range #{min_size}..#{max_size}: #{inspect(Enum.frequencies(main_sizes))}" + end + end + + defp collect_frames(pid, acc) do + receive do + {Pipeline, ^pid, + {:handle_child_notification, {{:buffer, %Buffer{payload: payload}}, :sink}}} -> + collect_frames(pid, [payload | acc]) + + {Pipeline, ^pid, {:handle_child_notification, {{:end_of_stream, :input}, :sink}}} -> + Enum.reverse(acc) + after + 5_000 -> Enum.reverse(acc) + end + end + + # Extract main_data_begin from the first 9 bits after the 4-byte MP3 header + defp extract_main_data_begin( + <<_header::binary-size(4), main_data_begin::size(9), _remaining::bitstring>> + ) do + main_data_begin + end + + defp extract_main_data_begin(_data), do: :not_mp3 + + describe "Encoder forwards timestamps correctly" do test "when one input buffer contains exactly one MP3 frame" do perform_timestamp_test(@raw_frame_size, :one_to_one) end