[Asterisk-code-review] Binaural synthesis (confbridge): Adds binaural synthesis to ... (asterisk[master])

[Joshua Colp]

Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/3524 )

Change subject: Binaural synthesis (confbridge): Adds binaural synthesis to bridge_softmix.
......................................................................


Patch Set 7: Code-Review-1

(13 comments)

I've got a few more findings, and the findings from George are still applicable. Do you need some help to take care of them?

https://gerrit.asterisk.org/#/c/3524/7/bridges/bridge_softmix.c
File bridges/bridge_softmix.c:

Line 286: 	if (softmix_data->convolve.chan_size > 1) {
You can reduce nesting by returning early


Line 302: 		ast_log(LOG_ERROR, "Invalid sample size (%d) for binaural convole.\n", in_sample_size);
Log messages currently do not contain any identifying information such as bridge identifier or channel identifier. If multiple are in use this can make it difficult to know what is going on and can confuse users. This comment applies to all the newly added log messages.


Line 376: 	channel->fftw_in = fftw_alloc_real(hrtf_len + 1);
Is it possible for any allocations to fail here? Does that need to be checked for and handled?


Line 414: 	data->cchan_pair = ast_malloc(sizeof(struct convolve_channel_pair *) * CONVOLVE_CHANNEL_PREALLOC);
I think a vector could be used for this instead, which is common code and handles all of the management. It would simplify the code a bit.


Line 416: 		data->cchan_pair[i] = ast_malloc(sizeof(struct convolve_channel_pair));
How do both of these handle memory allocation failures? Will things subsequently crash?


Line 917: 		if (strncmp(ast_channel_name(bridge_channel->chan), "CBAnn", 5 * sizeof(char)) != 0) {
No need to do the * sizeof(char). 5 is fine.


PS7, Line 1291: analyse_softmix
Is it possible to determine from the data in softmix_data whether binaural is active or not so this does not need to be changed?


PS7, Line 1398: mixing_array_destroy
If chan_pairs is guaranteed to be NULL when binaural is not active you don't have to change this function signature and have the if check.


Line 1448: 			if (bridge_channel->binaural_pos_change) {
You can reduce nesting by checking for !bridge_channel->binaural_pos_change and doing continue


PS7, Line 1463: 	if (bridge->softmix.binaural_active.active && softmix_data->convolve.binaural_active) {
              : 		if (softmix_samples % DEFAULT_SAMPLE_SIZE == 0) {
You can actually reduce this to a single if statement that then returns earlier. Reduces nesting.


PS7, Line 1783: 	ast_bridge_unlock(bridge);
              : 	ast_mutex_lock(&bridge->softmix.binaural_active.settings_lock);
              : 	ast_mutex_unlock(&bridge->softmix.binaural_active.settings_lock);
              : 	ast_bridge_lock(bridge);
As I mention down below I don't think this method of synchronization will work for everything.


Line 1864: 	ast_mutex_init(&bridge->softmix.binaural_active.settings_lock);
This mutex lock is never destroyed. On some systems this can cause a memory leak.


Line 1865: 	ast_mutex_lock(&bridge->softmix.binaural_active.settings_lock);
This requires a user of the bridge_softmix module to know that they have to unlock this. This is not a very good requirement to place upon every user of the API, in fact right now if ARI was being used settings_lock would never get unlocked and stuff would not work.

Is there any other way that synchronization can be done?


-- 
To view, visit https://gerrit.asterisk.org/3524
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iecdb381b6adc17c961049658678f6219adae1ddf
Gerrit-PatchSet: 7
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Frank Haase <fra.haase at googlemail.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Dennis Guse <dennis.guse at alumni.tu-berlin.de>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>
Gerrit-HasComments: Yes