Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce project-wide MAX_CHANNELS in CMakeLists.txt #996

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Introduce project-wide MAX_CHANNELS in CMakeLists.txt #996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ option(SWIG_PYTHON "Enable SWIG Python bindings" OFF)
option(SWIG_RUBY "Enable SWIG Ruby bindings" OFF)
option(SWIG_TCL "Enable SWIG Tcl bindings" OFF)

set(MAX_CHANNELS 64)

if(WIN32)
option(WINDOWS_DEPLOY "Install exes/libs directly to prefix (no subdir /bin)" ON)
else()
Expand Down Expand Up @@ -484,6 +486,9 @@ if(SWIG_TCL)
find_package(TCL REQUIRED)
endif()

set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DMAX_CHANNELS=${MAX_CHANNELS}")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DMAX_CHANNELS=${MAX_CHANNELS}")

if(BUILD_DOCS)
find_package(Doxygen REQUIRED)

Expand Down
2 changes: 0 additions & 2 deletions src/modules/core/filter_audiomap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@
#include <stdlib.h>
#include <string.h>

#define MAX_CHANNELS 32

static int filter_get_audio(mlt_frame frame,
void **buffer,
mlt_audio_format *format,
Expand Down
1 change: 0 additions & 1 deletion src/modules/core/transition_mix.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
#include <stdlib.h>
#include <string.h>

#define MAX_CHANNELS (6)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change will result in higher heap allocation and higher loop counts in the code below. Rather than increasing the static value, should we change the code below to better handle a dynamic number of channels? I think we would just need to delay the heap allocations until after the audio has been received and the actual number of channels is known.

Copy link
Contributor Author

@Tjoppen Tjoppen Jun 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allocation isn't a problem thanks to overcommit. As for loop counts, they only increase if both channels_a and channels_b are greater than 6, which is broken in master anyway at the moment.

Supporting arbitrarily large numbers of channels would be nice, but increasing the setting in CMakeLists.txt and allocating upfront is far easier and again basically free thanks to overcommit. The only snag is the allocation being done by calloc() which may cause the allocation to actually occur due to touching all of the memory, unless libc's calloc() is clever enough to map all pages to a single page of all zeroes.

Even FFmpeg has a static limit, from libavcodec/internal.h: #define FF_SANE_NB_CHANNELS 512U

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main issue is that most NLE projects use multiple transition objects. In all MLT NLEs, not only there is a mix transition for every track but also every video transition to cross-fade audio. Then, consider a multitrack project with at least one video track with many A/V transitions. With this, the project requires much more memory. For example, let's say there are 100 mix transitions in the project, which is not unreasonable from my user support experience: before 439 MiB, after 4.6 GiB. So, I have to reject this.

#define MAX_SAMPLES (192000)
#define SAMPLE_BYTES(samples, channels) ((samples) * (channels) * sizeof(float))
#define MAX_BYTES SAMPLE_BYTES(MAX_SAMPLES, MAX_CHANNELS)
Expand Down
3 changes: 1 addition & 2 deletions src/modules/plus/ebur128/ebur128.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,12 +398,11 @@ void ebur128_get_version(int* major, int* minor, int* patch) {
*patch = EBUR128_VERSION_PATCH;
}

#define VALIDATE_MAX_CHANNELS (64)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not want to change this file because I want to keep it synchronized with the upstream project
https://github.com/jiixyj/libebur128

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the limit in libebur128 defined in a publicly accessible header? If so then we should use that rather than defining our own.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked; it's in ebur128.c so the answer is no.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait the whole of it is just incorporated as-is. Nevermind then. It seems it will successfully fail if the number of channels is greater than the limit.

#define VALIDATE_MAX_SAMPLERATE (2822400)

#define VALIDATE_CHANNELS_AND_SAMPLERATE(err) \
do { \
if (channels == 0 || channels > VALIDATE_MAX_CHANNELS) { \
if (channels == 0 || channels > MAX_CHANNELS) { \
return (err); \
} \
\
Expand Down
2 changes: 0 additions & 2 deletions src/modules/rubberband/filter_rbpitch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ typedef struct
uint64_t out_samples;
} private_data;

static const size_t MAX_CHANNELS = 10;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the rubberband library and it does not not have a limit on the channel count. I set this to 10 initially because that is the highest number of channels that I ever tested.


static int rbpitch_get_audio(mlt_frame frame,
void **buffer,
mlt_audio_format *format,
Expand Down
1 change: 0 additions & 1 deletion src/modules/spatialaudio/filter_ambisonic-decoder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
#include <framework/mlt.h>
#include <spatialaudio/Ambisonics.h>

static const auto MAX_CHANNELS = 6;
static const auto AMBISONICS_BLOCK_SIZE = 1024;
static const auto AMBISONICS_ORDER = 1;
static const auto AMBISONICS_1_CHANNELS = 4;
Expand Down
Loading