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

[George Joseph]

George Joseph has posted comments on this change.

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


Patch Set 3: Code-Review-1

(12 comments)

I'm not an expert in this area so my comments are mainly around code organization.  My biggest concern is that bridge_softmix has grown 54%.

https://gerrit.asterisk.org/#/c/3524/3/apps/app_confbridge.c
File apps/app_confbridge.c:

Line 1281:  * \return A pointer to the conference bridge struct, or NULL if the conference room wasn't found.
You should make a note that you expect binaural_active.settings_lock to be locked before this call is made.


Line 1351: 		
whitespace


Line 1352: 		/* Release the lock that the mixing loop can start working. */
It might be good to note where this was locked since it's not in this file.


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

I'm really concerned that the size of this file has grown 54% for a feature that's not exactly mainstream.  All the ifdefs also make it hard to follow the code.

Do you think there could be a better way to organize this?  Maybe even splitting the file into different parts and moving the common stuff into a separate file?


https://gerrit.asterisk.org/#/c/3524/3/include/asterisk/bridge.h
File include/asterisk/bridge.h:

Various whitespace errors


https://gerrit.asterisk.org/#/c/3524/3/include/asterisk/bridge_channel.h
File include/asterisk/bridge_channel.h:

PS3, Line 130: 	/*! TRUE if binaural is suspended. */
             : 	unsigned int binaural_suspended:1;
             : 	/*! TRUE if a change of binaural positions has to be performed. */
             : 	unsigned int binaural_pos_change:1;
Since this is a public structure, moving these to the end will allow the patch to be cherry-picked into 13 and 14 if it makes sense.


https://gerrit.asterisk.org/#/c/3524/3/include/asterisk/hrirs_left.h
File include/asterisk/hrirs_left.h:

At a minimum you'll need to provide a file header with the appropriate copyright, license and attribution.


https://gerrit.asterisk.org/#/c/3524/3/include/asterisk/hrirs_right.h
File include/asterisk/hrirs_right.h:

At a minimum you'll need to provide a file header with the appropriate copyright, license and attribution.


https://gerrit.asterisk.org/#/c/3524/3/include/asterisk/impulses.h
File include/asterisk/impulses.h:

You should add a file header here including your copyright notice.

EDIT: actually you could place the contents of the hrirs files here and save having to do those as well.


Line 24: int positions[IMPULSE_SIZE] = {90,80,100,70,110,60,120,50,130,40,140,20,160,0,180,85,95,75,105,65,115,55,125,45,135,30,150,10,170,87,93,82,98,77,103,72,108,67,113,62,118,57,123,52,128,47,133,42,138,35,145,25,155,15,165,5,175,88,92,86,83,97,81,99,78,102,76,104,73,94,107,71,109,68,112,66,114,63,117,61,119,58,122,56,124,53,127,51,129,48,132,46,134,43,137,41,139,37,143,32,148,27,153,22,158,17,163,12,168,7,96,173,2,178,89,91,84,79,101,74,106,69,111,64,116,59,121,54,126,49,131,44,136,38,142,36,144,33,147,31,149,28,152,26,154,23,157,21,159,162,16,164,13,167,11,169,8,172,6,174,3,177,1,179,39,141,34,146,29,151,24,156,19,161,14,166,9,171,4,18,176};
This is a very long line.  :)


https://gerrit.asterisk.org/#/c/3524/3/include/asterisk/interleaved_stereo.h
File include/asterisk/interleaved_stereo.h:

You should pull this up into bridge_softmix since that's the only place it's used.


PS3, Line 10: PLACEHOLDER_STEREO_CODEC
What's the purpose of this placeholder?


-- 
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: 3
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Frank Haase <fra.haase at googlemail.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-HasComments: Yes