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

[Richard Mudgett]

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

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


Patch Set 13:

(5 comments)

* The hrirs_left.h, hrirs_right.h, and impulses.h files seem to be duplicates of the bridge_softmix/include versions.

* Can those data include files be regenerated so that you don't have 19 thousand character long lines?

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

PS13, Line 57: struct softmix_stats {
             :         /*! Each index represents a sample rate used above the internal rate. */
             :         unsigned int sample_rates[16];
             :         /*! Each index represents the number of channels using the same index in the sample_rates array.  */
             :         unsigned int num_channels[16];
             :         /*! The number of channels above the internal sample rate */
             :         unsigned int num_above_internal_rate;
             :         /*! The number of channels at the internal sample rate */
             :         unsigned int num_at_internal_rate;
             :         /*! The absolute highest sample rate preferred by any channel in the bridge */
             :         unsigned int highest_supported_rate;
             :         /*! Is the sample rate locked by the bridge, if so what is that rate.*/
             :         unsigned int locked_rate;
Guidelines: Use tabs to indent not spaces.


PS13, Line 72: struct softmix_translate_helper_entry {
             :         int num_times_requested; /*!< Once this entry is no longer requested, free the trans_pvt
             :                                       and re-init if it was usable. */
             :         struct ast_format *dst_format; /*!< The destination format for this helper */
             :         struct ast_trans_pvt *trans_pvt; /*!< the translator for this slot. */
             :         struct ast_frame *out_frame; /*!< The output frame from the last translation */
             :         AST_LIST_ENTRY(softmix_translate_helper_entry) entry;
             : };
             : 
             : struct softmix_translate_helper {
             :         struct ast_format *slin_src; /*!< the source format expected for all the translators */
             :         AST_LIST_HEAD_NOLOCK(, softmix_translate_helper_entry) entries;
             : };
Guidelines: Use tabs to indent not spaces.


PS13, Line 208: 	AST_LIST_TRAVERSE(&trans_helper->entries, entry, entry) {
              : 		if (sc->binaural == 0) {
Should skip the entire loop if sc->binaural is zero.


PS13, Line 1062: #ifdef HAVE_FFTW3
               : 			if (bridge->softmix.binaural_active.active && softmix_data->convolve.binaural_active && sc->binaural) {
               : 				create_binaural_frame(bridge_channel, sc, bin_buf, ann_buf, softmix_datalen, softmix_samples, buf);
               : 			} else {
               : #endif
               : 				sc->write_frame.datalen = softmix_datalen;
               : 				sc->write_frame.samples = softmix_samples;
               : 				memcpy(sc->final_buf, buf, softmix_datalen);
               : #ifdef HAVE_FFTW3
               : 			}
               : #endif
This conditional code can be simplified:

#ifdef HAVE_FFTW3
  if () {
     ...
  } else
#endif
  {
     ...
  }


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

PS13, Line 176: 	unsigned int binaural_suspended:1;
              : 	/*! TRUE if a change of binaural positions has to be performed. */
              : 	unsigned int binaural_pos_change:1;
These two flags should be in the bridge_softmix bridge_channel->tech_pvt data (struct softmix_channel) not in the public struct.

Also neither of these flags seem to really be used.  They are tested or set to zero but nothing sets them non-zero.


-- 
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: 13
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: Frank Haase <fra.haase at googlemail.com>
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-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes