Add disable_reservoir and cbr options for clean audio splicing#55
Merged
mat-hek merged 6 commits intomembraneframework:masterfrom Apr 16, 2026
Merged
Add disable_reservoir and cbr options for clean audio splicing#55mat-hek merged 6 commits intomembraneframework:masterfrom
mat-hek merged 6 commits intomembraneframework:masterfrom
Conversation
Adds a disable_reservoir boolean option (default: false) that calls lame_set_disable_reservoir(1) when enabled. This makes each MP3 frame self-contained (main_data_begin is always 0), producing constant-size CBR frames that can be cleanly spliced at any frame boundary. Useful for live streaming servers that concatenate audio from different sources (e.g., ad insertion) where bit reservoir dependencies between frames from different sources would cause decoder artifacts.
Two new boolean options (both default false): - disable_reservoir: calls lame_set_disable_reservoir(1) making each MP3 frame self-contained (main_data_begin always 0). Frames can be spliced at any boundary without decoder artifacts from bit reservoir dependencies between frames from different sources. - cbr: calls lame_set_VBR(vbr_off) to explicitly enforce constant bitrate mode. CBR is already the LAME default when a bitrate is set, but this option makes it explicit. Both options are useful for live streaming servers that concatenate audio from different sources (e.g., ad insertion).
New tests: - disable_reservoir: encodes PCM input, parses MP3 frames from output file, asserts main_data_begin == 0 for every frame (self-contained) - cbr: collects frame buffer sizes from Membrane encoder, asserts all frames are constant size (±1 byte for MP3 padding bit) Also regenerated test/fixtures/ref.mp3 for the current LAME build (previous reference was from a different build, causing a single-bit difference at byte 1064 — encoding decision variance, not a bug). All 6 tests now pass.
mat-hek
reviewed
Apr 15, 2026
Per review feedback, the disable_reservoir test now captures frame buffers via Membrane.Testing.Sink and checks main_data_begin on each buffer directly, matching the pattern used by the CBR test. Removed the file-based parse_mp3_frames/mp3_frame_size helpers that were no longer needed.
kookster
commented
Apr 15, 2026
mat-hek
reviewed
Apr 15, 2026
kookster
commented
Apr 15, 2026
| end | ||
| end | ||
|
|
||
| defp collect_frames(pid, acc) do |
Contributor
Author
There was a problem hiding this comment.
A common utility method now!
Member
|
Looks good, pls fix credo and we can merge ;) |
Contributor
Author
done! |
mat-hek
approved these changes
Apr 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds two new boolean options to the LAME encoder for live streaming use cases where audio from different sources is concatenated (e.g., ad insertion):
disable_reservoir(default:false)Calls
lame_set_disable_reservoir(1)making each MP3 frame self-contained (main_data_beginis always 0). Without this, the bit reservoir creates dependencies between adjacent frames — when audio from different sources is spliced together, the decoder sees invalid bit reservoir references from the previous source, causing audible glitches.cbr(default:false)Calls
lame_set_VBR(vbr_off)to explicitly enforce constant bitrate mode. CBR is already the LAME default when a bitrate is set, but this option makes it explicit.Changes
c_src/membrane_mp3_lame_plugin/encoder.c— addeddisable_reservoirandcbrparameters tocreate(), callslame_set_disable_reservoir()andlame_set_VBR(vbr_off)c_src/membrane_mp3_lame_plugin/encoder.spec.exs— updated Unifex spec with new parameterslib/membrane_mp3_lame/encoder.ex— addeddisable_reservoirandcbroptions with docs, passed toNative.create/5test/integration/integration_test.exs— two new tests:disable_reservoir: encodes PCM, parses all MP3 frames, assertsmain_data_begin == 0for every framecbr: collects frame buffer sizes, asserts constant size (±1 byte for padding bit)test/fixtures/ref.mp3— regenerated for current LAME buildUsage
Context
We're building a live streaming server with per-listener ad insertion using Membrane. The bit reservoir was causing audible glitches at ad→live transitions because frames from the ad audio had reservoir references that were invalid when followed by frames from the live stream. Disabling the reservoir eliminates this entirely.
Test results
All 6 tests pass (3 existing + 2 new + 1 with regenerated reference file).