REFAC(client): Introduce and use AudioPreprocessor

[davidebeatrici]

No description provided.

[Hartmnt]

So the point of this is to rip the speex pre-processor code out of AudioInput.cpp and abstract it, such that it can be swapped for a different pre-processor in the future, right?

[Krzmbrzl]

I'm not a big fan of using fixed-width integers without need. Imo, all the integers in the interface should be of type int (or unsigned int) to let the compiler use the native integer size for the given architecture.
Explicitly encoding the integer's width only provides extra information to someone reading the code that they don't need because in this case here it is completely irrelevant as all integers that are expected to be passed around are well below the maximum possible value of an int on all platforms that Mumble can be compiled on. This only causes head-scratching when someone has to refactor the code as they will start wondering where exactly the requirement for a 32-bit integer is coming from and why e.g. a 64-bit integer can't be used.

All-in-all though, LGTM

[Krzmbrzl]

@Hartmnt the immediate goal is to be able to reuse it to implement #6428 but of course the possibility to more easily drop in a different audio processor backend is a nice benefit too :)

[davidebeatrici]

@Hartmnt the immediate goal is to be able to reuse it to implement #6428 but of course the possibility to more easily drop in a different audio processor backend is a nice benefit too :)

Correct. If in the future we want to implement another preprocessor, this new class is a good abstraction base.

I'm not a big fan of using fixed-width integers without need. Imo, all the integers in the interface should be of type int (or unsigned int) to let the compiler use the native integer size for the given architecture. Explicitly encoding the integer's width only provides extra information to someone reading the code that they don't need because in this case here it is completely irrelevant as all integers that are expected to be passed around are well below the maximum possible value of an int on all platforms that Mumble can be compiled on. This only causes head-scratching when someone has to refactor the code as they will start wondering where exactly the requirement for a 32-bit integer is coming from and why e.g. a 64-bit integer can't be used.

speex_preprocess_ctl() treats the argument as a pointer to int32_t, except special cases (e.g. float). Ideally we should use the narrowest data type possible based on the range of values that is accepted by a specific call.

Documentation for each function would also help understanding that.

As for the use of int and unsigned int: it indeed ensures that the variable is the default size for the given architecture, but you don't gain any added performance from that. You actually risk potentially wasting memory.

[Krzmbrzl]

but you don't gain any added performance from that. You actually risk potentially wasting memory.

That's not true in the general case but that's also not my point. My point is completely about readability of the code and how much information to put into tue source code. Ideally, we only want relevant information in the source code and if the exact size of an integer is not important, adding the size into the source code decreased the signal-to-noise ratio, making the code harder to parse and modify.

[davidebeatrici]

Technically speaking the size of a variable is always important. Good practice generally consists in defaulting to the smallest size that can hold the biggest value you expect to be assigned to the variable.

There are of course some exceptions, such as when designing a public API. In that case you have to consider the future proofing aspect and thus may want to go with a bigger size even if not required right now.

[Krzmbrzl]

Technically speaking the size of a variable is always important. Good practice generally consists in defaulting to the smallest size that can hold the biggest value you expect to be assigned to the variable.

I don't think always choosing the smallest possible size does any good (almost never do the logical range of valid inputs coincide with the range of possible values that can be encoded with an integer of a given size) and as long as you do that, the size loses importance for 99% of functions as arguments typically range in the order of 100s to maybe 1000s which fits into pretty much all integer sizes.

[davidebeatrici]

If you're referring to the readability aspect then no, knowing the size of a variable doesn't tell you the actual value range that is expected. It's still a hint though.

[davidebeatrici]

@Krzmbrzl

Digest: sha256:0f969ceabb97af1232ceffeac308fbdee9f0cd0113289ca3ead6e01af9e8d6df
Status: Downloaded newer image for ghcr.io/jidicula/clang-format:10
clang-format version 10.0.0-4ubuntu1 
./src/mumble/AudioPreprocessor.h:203:46: error: code should be clang-formatted [-Wclang-format-violations]
        AudioPreprocessor(const AudioPreprocessor &)            = delete;
                                                    ^
Failed on file: ./src/mumble/AudioPreprocessor.h

This looks wrong to me... can we update the clang-format image?

[Krzmbrzl]

This looks wrong to me... can we update the clang-format image?

Only if you find a way to not break existing PRs (aka: a way to automatically adapt PRs to new formatting standards). I wanted to look into a script/bot that can do this for a while but never actually came around to implementing it. I'm sure it wouldn't be too complicated given that clang-format can format diffs but it'd have to be implemented in a way that makes it easy to use on all existing open PRs.

[davidebeatrici]

Formatted with clang-format 10, in order for CI to succeed.