<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>I tried to put all your remarks from both of you into the code, but i have a few follow up questions. </div><div><br></div><div><div>Am 26.09.2013 um 22:12 schrieb Rémi Denis-Courmont:</div><blockquote type="cite"><div><blockquote type="cite"><font class="Apple-style-span" color="#000000"><br></font></blockquote><blockquote type="cite">+SOURCES_sofalizer = sofalizer.c<br></blockquote><blockquote type="cite">SOURCES_spatializer = \<br></blockquote><blockquote type="cite"><span class="Apple-tab-span" style="white-space: pre; ">   </span>spatializer/allpass.cpp spatializer/allpass.hpp \<br></blockquote><blockquote type="cite"><span class="Apple-tab-span" style="white-space: pre; ">   </span>spatializer/comb.cpp spatializer/comb.hpp \<br></blockquote><blockquote type="cite">@@ -27,7 +28,8 @@ audio_filter_LTLIBRARIES += \<br></blockquote><blockquote type="cite"><span class="Apple-tab-span" style="white-space: pre; ">     </span>libparam_eq_plugin.la \<br></blockquote><blockquote type="cite"><span class="Apple-tab-span" style="white-space: pre; ">     </span>libscaletempo_plugin.la \<br></blockquote><blockquote type="cite"><span class="Apple-tab-span" style="white-space: pre; ">   </span>libspatializer_plugin.la \<br></blockquote><blockquote type="cite">-<span class="Apple-tab-span" style="white-space: pre; "> </span>libstereo_widen_plugin.la<br></blockquote><blockquote type="cite">+<span class="Apple-tab-span" style="white-space: pre; ">  </span>libstereo_widen_plugin.la \<br></blockquote><blockquote type="cite">+<span class="Apple-tab-span" style="white-space: pre; ">        </span>libsofalizer_plugin.la<br></blockquote><br>Either the plugin is built always or it is triggered conditionally (in this <br>case by VLC_ADD_PLUGIN), but it cannot be both.<br></div></blockquote><div><br></div><div>I use now PKG_ENABLE_MODULES_VLC([SOFALIZER], [], [netcdf >= 4.1.3] ) ... in configure.ac as jean-baptiste suggested, but i need now these lines to build it. </div><br><blockquote type="cite"><div><br><blockquote type="cite">+static int Open( vlc_object_t *p_this )<br></blockquote><blockquote type="cite">+{<br></blockquote><blockquote type="cite">+    filter_t *p_filter = (filter_t *)p_this;<br></blockquote><blockquote type="cite">+    filter_sys_t *p_sys = p_filter->p_sys = malloc( sizeof( *p_sys ) );<br></blockquote><blockquote type="cite">+    if( unlikely( p_sys == NULL ) )<br></blockquote><blockquote type="cite">+        return VLC_ENOMEM;<br></blockquote><blockquote type="cite">+    p_sys->p_parent = p_filter;<br></blockquote><blockquote type="cite">+    vlc_object_t *p_aout = p_filter->p_parent;<br></blockquote><br>There is no warranty that the parent object is an audio output. It might be a <br>transcode plugin instance, say.<br><font class="Apple-style-span" color="#00761f"><br></font></div></blockquote><div><br></div><div>What would be a better name for the vlc_object_t *? Equalizer, compressor and spatializer use this name for the exact same purpose. </div><br><blockquote type="cite"><div><br><blockquote type="cite">+    /* Callbacks can call function LoadIR */<br></blockquote><blockquote type="cite">+    var_AddCallback( p_aout, "sofalizer-gain", GainCallback, p_sys );<br></blockquote><blockquote type="cite">+    var_AddCallback( p_aout, "sofalizer-rotation", RotationCallback,<br></blockquote><blockquote type="cite">p_sys);<br></blockquote><blockquote type="cite">+    var_AddCallback( p_aout, "sofalizer-elevation",<br></blockquote><blockquote type="cite">ElevationCallback, p_sys );<br></blockquote><blockquote type="cite">+    var_AddCallback( p_aout,<br></blockquote><blockquote type="cite">"sofalizer-switch", SwitchCallback, p_sys );<br></blockquote><blockquote type="cite">+    var_AddCallback( p_aout,<br></blockquote><blockquote type="cite">"sofalizer-select", SelectCallback, p_sys );<br></blockquote><blockquote type="cite">+    var_AddCallback( p_aout,<br></blockquote><blockquote type="cite">"sofalizer-radius", RadiusCallback, p_sys );<br></blockquote><blockquote type="cite">+    p_sys->b_Enable = true;<br></blockquote><br>You cannot write a value while it is potentially read (or written) by other <br>tasks.</div></blockquote><div><br></div><div>I deleted the whole variable b_Enable.  I lock with the mutex_lock all writing operations into p_sys except in the Open and DoWork function, but the Callbacks get now active after everything is done in the Open. There is one write in the DoWork to i_write, but only the DoWork function itselfs writes into this variable. So I think this can't be a problem any more.</div><div><br></div><br><blockquote type="cite"><div><blockquote type="cite">+        if( vlc_clone( &left_thread, (void *)&sofalizer_Convolute, (void<br></blockquote><blockquote type="cite">*)&t_data_l, VLC_THREAD_PRIORITY_HIGHEST ) ) goto out;<br></blockquote><blockquote type="cite">+        if(<br></blockquote><blockquote type="cite">vlc_clone( &right_thread, (void *)&sofalizer_Convolute, (void *)&t_data_r,<br></blockquote><blockquote type="cite">VLC_THREAD_PRIORITY_HIGHEST ) ) goto out;<br></blockquote><blockquote type="cite">+        vlc_join ( left_thread, NULL );<br></blockquote><blockquote type="cite">+        vlc_join ( right_thread, NULL );<br></blockquote><br>Threads are relatively cheap, but creating and destroying two per audio block <br>may be pushing it a bit...<br></div></blockquote><div><br></div><div>I nearly doubled the performance by putting the main computation of the the DoWork function into the two threads. Convolution in real time needs lots of computation time and the computed data for the right and left ear is independent from each other. </div><div><br></div><blockquote type="cite"><div><br><blockquote type="cite">+        p_sys->i_write = t_data_l.i_write;<br></blockquote><blockquote type="cite">+    }<br></blockquote><blockquote type="cite">+<br></blockquote><blockquote type="cite">+    if ( ( i_n_clippings_l + i_n_clippings_r ) > 0 )<br></blockquote><blockquote type="cite">+    {<br></blockquote><blockquote type="cite">+        msg_Err(p_filter, "%d of %d Samples in the Outputbuffer clipped.<br></blockquote><blockquote type="cite">Please reduce gain.", i_n_clippings_l + i_n_clippings_r,<br></blockquote><blockquote type="cite">p_out_buf->i_nb_samples * 2 );<br></blockquote><blockquote type="cite">+    }<br></blockquote><blockquote type="cite">+out: block_Release( p_in_buf );<br></blockquote><blockquote type="cite">+    return p_out_buf;<br></blockquote><blockquote type="cite">+}<br></blockquote><blockquote type="cite">+<br></blockquote><br>(...)<br></div></blockquote></div><div><br></div><div>Does (...) mean Trailing Spaces? I will run a script to remove them all next time, sorry for that.</div><div><br></div><br><div><div>Am 25.09.2013 um 15:50 schrieb Jean-Baptiste Kempf:</div><br><blockquote type="cite"><div><br><blockquote type="cite">+struct filter_sys_t<br></blockquote><blockquote type="cite">+{<br></blockquote><blockquote type="cite">+    filter_t *p_parent;<br></blockquote><blockquote type="cite">+    vlc_mutex_t lock;<br></blockquote><blockquote type="cite">+    bool b_Enable;<br></blockquote><br>Avoid that. You are destroying packing.<br><br><blockquote type="cite">+    float *p_speaker_pos;<br></blockquote><blockquote type="cite">+<br></blockquote><blockquote type="cite">+    /* Control variables */<br></blockquote><blockquote type="cite">+    float f_gain;<br></blockquote><blockquote type="cite">+    float f_rotation;<br></blockquote><blockquote type="cite">+    float f_elevation;<br></blockquote><blockquote type="cite">+    float f_radius;<br></blockquote><blockquote type="cite">+    int i_azimuth_array[4];<br></blockquote><blockquote type="cite">+    int i_elevation_array[4];<br></blockquote><blockquote type="cite">+    int i_switch;<br></blockquote><blockquote type="cite">+    bool b_mute;<br></blockquote><br>idem<br><br><blockquote type="cite">+    /* N of Channels to convolute */<br></blockquote><blockquote type="cite">+    int i_n_conv;<br></blockquote><blockquote type="cite">+    bool b_lfe;<br></blockquote><br>idem.<br><br><blockquote type="cite">+    /* Buffer variables */<br></blockquote><blockquote type="cite">+    float *p_ringbuffer_l;<br></blockquote><blockquote type="cite">+    float *p_ringbuffer_r;<br></blockquote><blockquote type="cite">+    int i_write;<br></blockquote><blockquote type="cite">+    int i_buffer_length;<br></blockquote><blockquote type="cite">+<br></blockquote><blockquote type="cite">+    /* NetCDF variables */<br></blockquote><blockquote type="cite">+    struct nc_sofa_t sofa[N_SOFA];<br></blockquote><br>And probably the same.<br></div></blockquote><div><br></div><div>I never had do worry about packing before, and i'm a little confused. bools in c are represented by int, so on 32bit all floats, ints and pointers should all be 4byte aligned. The struct is an array, so maybe also only a pointer? What can i do to make it better?</div><div><br></div><blockquote type="cite"><div><br><blockquote type="cite">+    int i_i_sofa;  /* Selected Sofa File */<br></blockquote><blockquote type="cite">+    int *p_delay_l;<br></blockquote><blockquote type="cite">+    int *p_delay_r;<br></blockquote><blockquote type="cite">+    float *p_ir_l;<br></blockquote><blockquote type="cite">+    float *p_ir_r;<br></blockquote><blockquote type="cite">+};<br></blockquote><br>Why so many pointers to float and int?<br><br></div></blockquote><div><br></div><div>All of these pointers are for arrays, whose lengths are dependent on the media and sofa file. It would be possible to merge the arrays p_sp_a, p_sp_e and p_sp_r into one array, but i think this wouldn't improve readability of the code. </div><br><blockquote type="cite"><div><br><blockquote type="cite">+#define HELP_TEXT N_( "SOFAlizer creates a virtual auditory display, i.e., virtual loudspeakers around the user for listening via headphones. The position of the virtual loudspeakers depends on the audio format of the input file (up to 8.1 supported). SOFAlizer filters audio channels with head-related transfer functions (HRTFs). SOFAlizer uses HRTFs stored in SOFA files (<a href="http://www.sofaconventions.org">www.sofaconventions.org</a>) following the SimpleFreeFieldHRIR Convention. A database of SOFA files can be found at www.sofacoustics.org.\nWith listener-specific HRTFs, the virtual loudspeakers can be rotated and elevated. With near-field HRTFs, the distance between the loudspeakers and the listener can be varied. SOFAlizer allows to load 5 different SOFA files and easily switch between them. The audio channels can also be presented from one of four pre-defined virtual positions.\nSOFAlizer is developed at the Acoustics Research Institute (ARI) of the Austrian Academy of Sciences." )<br></blockquote><br>Come on... This is way too long...<font class="Apple-style-span" color="#00761f"><br></font></div></blockquote><div><br></div>The second paragraph describes the function of the parameters, so i put it in the respective LONGTEXT descriptions instead of the add_help()<br><br><blockquote type="cite"><div><blockquote type="cite">+#define FILE1_NAME_TEXT N_( "SOFA file 1" )<br></blockquote><blockquote type="cite">+#define FILE2_NAME_TEXT N_( "SOFA file 2" )<br></blockquote><blockquote type="cite">+#define FILE3_NAME_TEXT N_( "SOFA file 3" )<br></blockquote><blockquote type="cite">+#define FILE4_NAME_TEXT N_( "SOFA file 4" )<br></blockquote><blockquote type="cite">+#define FILE5_NAME_TEXT N_( "SOFA file 5" )<br></blockquote><br>Useless strings addition.<br></div></blockquote><div><br></div><div>I'm not sure what you mean. Shouldn't i use the predefined strings here, but put them directly in add_loadfile()??</div><div><br></div><blockquote type="cite"><div><blockquote type="cite"><font class="Apple-style-span" color="#000000"><br></font></blockquote><blockquote type="cite">+    p_sys->i_elevation_array[3] = var_CreateGetInteger ( p_aout, "sofalizer-pos4-ele" );<br></blockquote><br>Use var_Inherit*<font class="Apple-style-span" color="#00761f"><br></font></div></blockquote><div><div><br></div></div><div>Can you please name me one ore two source files to learn the correct use of var_Inherit* ? If I use it, the callbacks from the interface don't work. I orientate on the compressor and spatializer, because they are closest...</div></div><div><br></div><div>Best Regards, Andi<br><br></div><br></body></html>