<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Thanks again, Francois. We have now finally posted an updated patch containing corrections based on your comments plus new functionality (resampling of HRTFs and performance improvement by using fast convolution). See my posts from today (May 12th).<div><br></div><div>I answer your comments below. Some answers just confirm our agreement, some explain our point of view and/or raise follow-up questions.</div><div>We'd appreciate if you could answer our follow-up questions! Thank you!</div><div><br></div><div><br></div><div><div><div>Am 14.04.2015 um 18:11 schrieb Francois Cartegnie <<a href="mailto:fcvlcdev@free.fr">fcvlcdev@free.fr</a>>:</div><br class="Apple-interchange-newline"><blockquote type="cite">Le 16/02/2015 13:28, Wolfgang Hrauda a écrit :<br><blockquote type="cite"> dnl<br>+dnl NetCDF library for SOFAlizer plugin<br>+dnl<br>+PKG_ENABLE_MODULES_VLC([SOFALIZER], [], [netcdf >= 4.1.1], [netcdf support for sofalizer module], [auto])<br>+<br>+dnl<br> dnl Libnotify notification plugin<br> dnl<br> PKG_ENABLE_MODULES_VLC([NOTIFY], [], [libnotify gtk+-2.0], [libnotify notification], [auto])<br></blockquote><br>Not standalone libraries.<br>Enable module based on library checks.<br></blockquote><div>
<p style="margin-bottom: 0cm"><font color="#000000">We
were advised to do so by Jean-Baptiste Kempf: „Please use just the
PKG check, in one line, for all the architectures.“
(<a href="https://mailman.videolan.org/pipermail/vlc-devel/2013-September/094739.html">https://mailman.videolan.org/pipermail/vlc-devel/2013-September/094739.html</a>)
That's why we are doing it this way.</font></p><p style="margin-bottom: 0cm"><font color="#000000">Could
explain a bit more detailed what you mean?</font></p></div><br><blockquote type="cite"><br><blockquote type="cite">+/*****************************************************************************<br>+ * sofalizer.c : SOFAlizer plugin to use SOFA files in vlc<br>+ *****************************************************************************<br></blockquote><br>Where else ?<br></blockquote><div>Done. Updated
short description.</div>
<br><blockquote type="cite"><br><blockquote type="cite">+ * Copyright (C) 2013-2015 Andreas Fuchs, Wolfgang Hrauda, ARI<br></blockquote><br>No contact for "ARI".<br><br><blockquote type="cite">+ * Project coordinator: Piotr Majdak <<a href="mailto:piotr@majdak.at">piotr@majdak.at</a>><br></blockquote><br>We don't care.<br></blockquote><div>
<p style="margin-bottom: 0cm"><font color="#000000">Actually,
SOFAlizer is a project of the Acoustics Research Institute (ARI).
Piotr Majdak is the project coordinator at ARI. I now clarified this
in the text.</font></p></div><br><blockquote type="cite"><br><blockquote type="cite">+<br>+struct nc_sofa_t /* contains data of one SOFA file */<br>+{<br>+ int i_ncid; /* netCDF ID of the opened SOFA file */<br>+ int i_n_samples; /* length of one impulse response (IR) */<br>+ int i_m_dim; /* number of measurement positions */<br>+ int *p_data_delay; /* broadband delay of IR, dimension [I R] or [M R] */<br>+ /* all measurement positions for each receiver (i.e. ear): */<br>+ float *p_sp_a; /* azimuth angles */<br>+ float *p_sp_e; /* elevation angles */<br>+ float *p_sp_r; /* radii */<br>+ float *p_data_ir; /* IRs at each measurement position for each receiver */<br>+};<br></blockquote><br>struct naming is _s.<br></blockquote><div>
<p style="margin-bottom: 0cm"><font color="#000000">We
were following the code conventions found in the developer's corner
of VLC (<a href="https://wiki.videolan.org/Code_Conventions/">https://wiki.videolan.org/Code_Conventions/</a>).</font></p><p style="margin-bottom: 0cm"><font color="#000000">It
says: „</font>If a variable has no basic type (for
instance a complex structure), don't put any prefix (except <tt>p_</tt>
if it's a pointer).<font color="#000000">“
</font><font color="#000000">I'm
not sure what to do now, please advise.</font></p><p style="margin-bottom: 0cm"><br>
</p></div><br><blockquote type="cite"><br><blockquote type="cite">+#define FILE1_NAME_TEXT N_( "SOFA file 1" )<br>+#define FILE2_NAME_TEXT N_( "SOFA file 2" )<br>+#define FILE3_NAME_TEXT N_( "SOFA file 3" )<br></blockquote><br>Don't put pressure on translators. Use format string where possible.<br>Same for others strings.<br></blockquote><div>
<p style="margin-bottom: 0cm"><font color="#000000"><font size="3"><span lang="en-US"><span style="background: #ffffff">Could
you explain what you mean by “use format string”? </span></span><span lang="en-US"><span style="background: #ffffff">We
oriented us towards the use of strings in the other </span></span><span lang="en-US"><span style="background: #ffffff">audio
</span></span><span lang="en-US"><span style="background: #ffffff">modules.</span></span></font></font></p></div><br><blockquote type="cite"><br><blockquote type="cite">+#define SELECT_VALUE_TEXT N_( "Select SOFA file" )<br></blockquote><br>You are supposed to provide text, not UI actions.<br></blockquote><div>
<p style="margin-bottom: 0cm"><font color="#000000">Agreed,
changed to „Use SOFA file number:“</font></p><div><br></div></div><blockquote type="cite"><br><blockquote type="cite">+#define ROTATION_VALUE_TEXT N_( "Rotation [°]" )<br></blockquote><br>You are supposed to provide text, no symbols.<br></blockquote><div>
<p style="margin-bottom: 0cm"><font color="#000000">Agreed,
changed.</font></p></div><br><blockquote type="cite"><blockquote type="cite">+vlc_module_begin ()<br>+ set_description( N_("SOFAlizer") )<br>+ set_shortname( N_("SOFAlizer") )<br></blockquote><br>No translation for that.<br></blockquote><div>
<p style="margin-bottom: 0cm"><font color="#000000">Agreed,
changed.</font></p></div><br><blockquote type="cite"><blockquote type="cite">+<br>+static int LoadSofa ( filter_t *p_filter, char *c_filename, /* loads one file*/<br>+ int i_i_sofa , int *p_samplingrate)<br></blockquote><br>*c_<br>i_i_<br>*p_<br><br>not really matching code writing guidelines. Same everywhere.<br></blockquote><div>
<p style="margin-bottom: 0cm"><font color="#000000">Are
you addressing the prefixes? We were following the code conventions
found in the developer's corner of VLC
(<a href="https://wiki.videolan.org/Code_Conventions/">https://wiki.videolan.org/Code_Conventions/</a>):</font></p><p style="margin-bottom: 0cm"><font color="#000000">„</font>That
is, we have the following prefixes: <tt>i_</tt> for integers etc.“</p><p style="margin-bottom: 0cm">Please confirm that you are proposing
to violate the Code Conventions. We will do accordingely.</p></div><div><br></div><br><blockquote type="cite"><br><blockquote type="cite">+ for( int ii = 0; ii<i_n_dims; ii++ ) /* go through all dimensions of file */<br>+ {<br>+ nc_inq_dim( i_ncid, ii, c_dim_names[ii], &i_dim_length[ii] ); /* get dimensions */<br>+ if ( !strncmp("M", c_dim_names[ii], 1 ) ) /* get ID of dimension "M" */<br>+ i_m_dim_id = ii;<br>+ if ( !strncmp("N", c_dim_names[ii], 1 ) ) /* get ID of dimension "N" */<br>+ i_n_dim_id = ii;<br>+ }<br></blockquote><br>ii ??<br></blockquote><div>
<p style="margin-bottom: 0cm"><font color="#000000">„<font face="Helvetica, serif"><font size="3">ii“
has the advantage that you can search for it within the code. If you
search for „i“, you will obviously get a lot of results you don't
want.</font></font></font></p><p style="margin-bottom: 0cm"><font color="#000000"><font face="Helvetica, serif">I
admit that its use here is not consistent with the rest of the source
code, therefore I changed it to „i“.</font></font></p></div><br><blockquote type="cite"><br><blockquote type="cite">+<br>+ /* -- allocate memory for one value for each measurement position: -- */<br>+ int *p_data_delay = p_sys->sofa[i_i_sofa].p_data_delay =<br>+ calloc ( sizeof( int ) , i_m_dim * 2 );<br></blockquote><br>"man calloc"<br></blockquote><div>
<p style="margin-bottom: 0cm"><font color="#000000"><font face="Helvetica, serif">Agreed,
changed order of arguments.</font></font></p></div><br><blockquote type="cite"><br><blockquote type="cite">+ float *p_sp_a = p_sys->sofa[i_i_sofa].p_sp_a = malloc( sizeof(float) * i_m_dim);<br>+ float *p_sp_e = p_sys->sofa[i_i_sofa].p_sp_e = malloc( sizeof(float) * i_m_dim);<br>+ float *p_sp_r = p_sys->sofa[i_i_sofa].p_sp_r = malloc( sizeof(float) * i_m_dim);<br>+ /* delay values are required for each ear and each measurement position: */<br>+ float *p_data_ir = p_sys->sofa[i_i_sofa].p_data_ir =<br>+ malloc( sizeof( float ) * 2 * i_m_dim * i_n_samples );<br>+<br>+ if ( !p_data_delay || !p_sp_a || !p_sp_e || !p_sp_r || !p_data_ir )<br>+ { /* if memory could not be allocated */<br></blockquote><br>Partial allocation = LEAK<br></blockquote><div>
<p lang="en-US" style="margin-bottom: 0cm; "><font color="#000000"><font face="Helvetica, Arial, sans-serif"><font size="3"><span style="background: #ffffff">When
an error occurs, the function CloseSofa() (which is called within
that corresponding if statement) correctly frees all the allocated
memory associated with the currently loaded file, even on partial
allocation.</span></font></font></font></p><p lang="en-US" style="margin-bottom: 0cm; "><font color="#000000"><font face="Helvetica, Arial, sans-serif"><font size="3"><span style="background: #ffffff">However,
there was a problem with an uninitialized id variable, but I fixed
that. Please give me a note if this solved the problem you
addressed.</span></font></font></font></p></div><br><blockquote type="cite"><br><blockquote type="cite">+ /* Data.Delay dimension check */<br>+ /* dimension of Data.Delay is [I R]: */<br>+ if ( !strncmp ( psz_data_delay_dim_name, "I\0", 2 ) )<br></blockquote><br>String is already zero terminated.<br></blockquote><div>
<p lang="en-US" style="margin-bottom: 0cm; "><font color="#000000"><font face="Helvetica, Arial, sans-serif"><font size="3"><span style="background: #ffffff">Agreed,
changed.</span></font></font></font></p></div><br><blockquote type="cite"><blockquote type="cite">+static int GainCallback( vlc_object_t *p_this, char const *psz_var,<br>+ vlc_value_t oldval, vlc_value_t newval, void *p_data )<br>+{<br>+ VLC_UNUSED(p_this); VLC_UNUSED(psz_var); VLC_UNUSED(oldval);<br></blockquote><br>UNUSED USED..<br></blockquote><div>
<p lang="en-US" style="margin-bottom: 0cm; "><font color="#000000"><font face="Helvetica, Arial, sans-serif"><font size="3"><span style="background: #ffffff">Agreed,
removed VLC_UNUSED(p_this).</span></font></font></font></p></div><br><blockquote type="cite"><br><blockquote type="cite">+ /* get user settings */<br>+ p_sys->f_rotation = abs ( ( - (int) var_CreateGetFloat ( p_out, "sofalizer-rotation" ) + 720 ) % 360 );<br>+ p_sys->i_i_sofa = (int) (var_CreateGetFloat ( p_out, "sofalizer-select" ) ) - 1;<br>+ p_sys->i_switch = (int) ( var_CreateGetFloat ( p_out, "sofalizer-switch" ) );<br>+ p_sys->f_gain = var_CreateGetFloat( p_out, "sofalizer-gain" );<br>+ p_sys->f_elevation = var_CreateGetFloat( p_out, "sofalizer-elevation" );<br>+ p_sys->f_radius = var_CreateGetFloat( p_out, "sofalizer-radius");<br>+<br></blockquote><br><blockquote type="cite">+ p_sys->i_buffer_length = pow(2, ceil(log( i_n_max )/ log(2) ) ); /* buffer length as power of 2 */<br></blockquote><br>1 << ceil(log( i_n_max )/ log(2) )<br><br>Maybe I'm wrong but:<br><br>log( i_n_max ) / log(2) = log2(i_n_max)<br>doing a power of 2 of log2 ?<br></blockquote><div>
<p style="margin-bottom: 0cm"><font color="#000000"><font face="Helvetica, serif"><font face="Helvetica, Arial, sans-serif"><span lang="en-US"><span style="background-color: rgb(255, 255, 255); ">Note
the ceil in the code. </span></span></font>So, we first do log2
then ceil (to obtain an integer power of 2), then power of 2. For
example, if one impulse response is 200 samples long, we want the
ringbuffer to be 256 samples for computational efficiency.</font></font></p></div><br><blockquote type="cite"><br><blockquote type="cite">+ p_sys->p_ringbuffer_l = calloc( p_sys->i_buffer_length * i_input_nb, sizeof( float ) );<br>+ p_sys->p_ringbuffer_r = calloc( p_sys->i_buffer_length * i_input_nb, sizeof( float ) );<br></blockquote><br>man calloc again<br></blockquote><div>
<p style="margin-bottom: 0cm"><font color="#000000"><font face="Helvetica, serif">What's
wrong here? I couldn't figure out, despite reading documentation of
calloc.</font></font></p></div><br><blockquote type="cite"><br><blockquote type="cite">+ if ( !p_sys->p_ir_l || !p_sys->p_ir_r || !p_sys->p_delay_l ||<br>+ !p_sys->p_delay_r || !p_sys->p_ringbuffer_l || !p_sys->p_ringbuffer_r<br>+ || !p_sys->p_speaker_pos )<br>+ {<br></blockquote><br>re-leak<br></blockquote><div>
<p style="margin-bottom: 0cm"><font color="#000000"><font face="Helvetica, serif">If
you refer to a partial allocation problem again, the free'ing is
correctly done by the functions FreeAllSofa and FreeFilter in the
error handling.</font></font></p><p style="margin-bottom: 0cm"><font color="#000000"><font face="Helvetica, serif">Note
that while re-checking the code, we found an initialization problem
which we have resolved now. However, we cannot find any leak (we
reconfirmed this with a simulated partial allocation and valgrind. No
leak).</font></font></p></div><br><blockquote type="cite"><br><blockquote type="cite">+ memset( (float *)p_out_buf->p_buffer , 0 , sizeof( float ) * p_in_buf->i_nb_samples * 2 );<br></blockquote><br>no cast<br></blockquote><div>
<p style="margin-bottom: 0cm"><font color="#000000"><font face="Helvetica, serif">Basically,
the out buffer is always cast to float* (also e.g. in the compressor
module). However, for setting the memory to 0 it doesn't matter
anyway, so I removed the casting to float.</font></font></p></div><br><blockquote type="cite"><br><blockquote type="cite">+ }<br>+ else /* do the actual convolution for left and right channel */<br>+ {<br>+ if( vlc_clone( &left_thread, (void *)&sofalizer_Convolute,<br>+ (void *)&t_data_l, VLC_THREAD_PRIORITY_HIGHEST ) ) goto out;<br></blockquote><br>I doubt someone would allow highest priority<br></blockquote><div>
<p style="margin-bottom: 0cm"><font color="#000000"><font face="Helvetica, serif">Agreed,
changed priority to PRIORITY_AUDIO.</font></font></p></div><br><blockquote type="cite"><br><blockquote type="cite">+ if( vlc_clone( &right_thread, (void *)&sofalizer_Convolute,<br>+ (void *)&t_data_r, VLC_THREAD_PRIORITY_HIGHEST ) ) goto out;<br>+ vlc_join ( left_thread, NULL );<br>+ vlc_join ( right_thread, NULL );<br>+ p_sys->i_write = t_data_l.i_write;<br>+ }<br></blockquote><br>And if second thread fails cloning ?<br></blockquote><div>
<p lang="en-US" style="margin-bottom: 0cm"><font color="#000000"><font face="Helvetica, serif">I'm
not sure what you mean!? In case that the second thread fails
cloning, would I have to join the first thread and then “goto out”?</font></font></p><div><br></div><div><br></div><div>Best,</div><div>Wolfgang</div></div><div><br></div><div><br></div><blockquote type="cite"><br><br>Francois<br>_______________________________________________<br>vlc-devel mailing list<br>To unsubscribe or modify your subscription options:<br><a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a><br></blockquote></div><br></div></body></html>