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.

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

Introduce project-wide MAX_CHANNELS in CMakeLists.txt #996

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

Conversation

Tjoppen
Copy link
Contributor

@Tjoppen Tjoppen commented Jun 19, 2024

This is an alternative to PR #995

The current value of 64 is taken from ebur128.
Having the same value across the entire project reduces user surprise.
@Tjoppen
Copy link
Contributor Author

Tjoppen commented Jun 19, 2024

I wasn't able to properly test that filter_ambisonic-decoder.cpp compiles due to its C++ API having changed (or Debian having an ancient version), but not removing MAX_CHANNELS in its leads to an earlier compilation failure as expected.

Copy link
Member

@bmatherly bmatherly left a comment

Choose a reason for hiding this comment

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

I do not feel strongly about this pull request or the alternate that was submitted. However, I would lean slightly toward our current approach which is to allow each service to define their own channel count limit based on practical limitation or real limitations passed down by other libraries. Also, if a service has a limit defined, it would be good if someone had tested the service at that limit - which would not be the case with this patch. Users could submit pull requests to increase the limit for a specific service when they have tested it at that limit.

@@ -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.

@@ -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

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.

@@ -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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants