<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; "><br><div><div>Am 21.05.2015 um 20:33 schrieb Rémi Denis-Courmont <<a href="mailto:remi@remlab.net">remi@remlab.net</a>>:</div><br class="Apple-interchange-newline"><blockquote type="cite">Le mardi 12 mai 2015, 12:44:41 Wolfgang Hrauda a écrit :<br><blockquote type="cite"><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<br>support for sofalizer module], [auto]) +<br>+dnl<br>dnl Libnotify notification plugin<br>dnl<br>PKG_ENABLE_MODULES_VLC([NOTIFY], [], [libnotify gtk+-2.0], [libnotify<br>notification], [auto])> <br></blockquote>Not standalone libraries.<br>Enable module based on library checks.<br></blockquote><br>We were advised to do so by Jean-Baptiste Kempf: „Please use just the PKG<br>check, in one line, for all the architectures.“<br>(<a href="https://mailman.videolan.org/pipermail/vlc-devel/2013-September/094739.htm">https://mailman.videolan.org/pipermail/vlc-devel/2013-September/094739.htm</a><br>l) That's why we are doing it this way. Could explain a bit more detailed<br>what you mean?<br></blockquote><br>Well, the way you patched configure.ac will become a problem if/when the netcdf <br>is reused in any other module. Of course, you could argue that this is a <br>bridge that has yet to be crossed.<br></blockquote><div>
        
        
        <p lang="en-US" style="margin-bottom: 0cm"><font color="#000000"><font face="Helvetica, serif">On
one hand, yes, it is a bridge that has yet to be crossed. On the
other hand, what exactly would be a better way to patch configure.ac?</font></font></p></div><br><blockquote type="cite"><br><blockquote type="cite"><br><blockquote type="cite"><blockquote type="cite">+/**********************************************************************<br>******* + * sofalizer.c : SOFAlizer plugin to use SOFA files in vlc<br>+<br>************************************************************************<br>*****> <br></blockquote>Where else ?<br></blockquote><br>Done. Updated short description.<br><br><blockquote type="cite"><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><br>Actually, SOFAlizer is a project of the Acoustics Research Institute (ARI).<br>Piotr Majdak is the project coordinator at ARI. I now clarified this in the<br>text.<br></blockquote><br>I doubt that Mr. Majdak will continue as the project coordinator of the VLC <br>SOFalizer plugin going forward. It´s totally fine to give credit where due...<br>But in 5 years, if someone reads that line, they might think that they need to <br>contact him to change the VLC plugin code. And that just would not be right.<br></blockquote><div>
        
        
        <p lang="en-US" style="margin-bottom: 0cm"><font color="#000000"><font face="Helvetica, serif">Actually,
the work on this project is done by different audio engineering
students (the above mentioned Andreas Fuchs and me, so far) under the
supervision of Mr Majdak.</font></font></p><p lang="en-US" style="margin-bottom: 0cm"><font color="#000000"><font face="Helvetica, serif">The
students will change, but Mr Majdak will remain as the supervisor, so
I think it is the most sustainable solution to also list him.</font></font></p></div><div><br></div><div><br></div><br><blockquote type="cite"><blockquote type="cite"><blockquote type="cite"><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<br>receiver */ +};<br></blockquote><br>struct naming is _s.<br></blockquote><br>We were following the code conventions found in the developer's corner of<br>VLC (<a href="https://wiki.videolan.org/Code_Conventions/">https://wiki.videolan.org/Code_Conventions/</a>). It says: „If a variable<br>has no basic type (for instance a complex structure), don't put any prefix<br>(except p_ if it's a pointer).“ I'm not sure what to do now, please advise.<br></blockquote><br>I would advise to avoid _t as it violates the POSIX specification. And yes, I <br>know there are plenty of violations in the existing code, but lets not add <br>more.<br></blockquote><div>
        
        
        <p lang="en-US" style="margin-bottom: 0cm"><font color="#000000"><font face="Helvetica, serif">Ok,
I changed all struct names to „*_s“ (except filter_sys_t, because
it is declared externally).</font></font></p></div><div><br></div><br><blockquote type="cite"><br><blockquote type="cite"><blockquote type="cite"><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><br>Could you explain what you mean by “use format string”? We oriented us<br>towards the use of strings in the other audio modules.<br></blockquote><br>Indeed, you cannot use format strings here. But as others have pointed out, <br>having three identical configuration entries is arbitrary and questionable.<br></blockquote><div><br></div><div>
        
        
        <p lang="en-US" style="margin-bottom: 0cm"><font color="#000000"><font face="Helvetica, serif">I
think that there is a kind of misunderstanding of what SOFAlizer is
actually doing. So, we try to explain: When you listen to music or
watch a movie, SOFAlizer creates a virtual audio environment for the
content. Similarly to various settings in an equalizer, you probably
want to have various environments and switch between them on-the-fly.
The environments are stored in pre-calculated files called “SOFA
files”. Thus, we need entries for at several (least three) files,
otherwise the SOFAlizer module gets boring. Does it help?</font></font></p></div><div><br></div><br><blockquote type="cite"><blockquote type="cite"><blockquote type="cite"><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><br>Agreed, changed to „Use SOFA file number:“<br></blockquote><br>You are supposed to provide a description of what the option does.<br></blockquote><div>
        
        
        <p lang="en-US" style="margin-bottom: 0cm"><font color="#000000"><font face="Helvetica, serif">Agreed,
changed to „Select SOFA file used for rendering:“.</font></font></p></div><div><br></div><br><blockquote type="cite"><br><blockquote type="cite"><blockquote type="cite"><blockquote type="cite">+static int LoadSofa ( filter_t *p_filter, char *c_filename, /* loads one<br>file*/ +                      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><br>Are you addressing the prefixes? We were following the code conventions<br>found in the developer's corner of VLC<br>(<a href="https://wiki.videolan.org/Code_Conventions/">https://wiki.videolan.org/Code_Conventions/</a>): „That is, we have the<br>following prefixes: i_ for integers etc.“<br>Please confirm that you are proposing to violate the Code Conventions. We<br>will do accordingely.<br></blockquote><br>I would advise to side step the problem by not using the Hungarian notation at <br>all. But if you use it, please stick to the VLC convention. In particular, <br>psz_ for nul-terminated strings.<br></blockquote><div>
        
        
        <p lang="en-US" style="margin-bottom: 0cm"><font color="#000000"><font face="Helvetica, serif">Agreed,
I renamed all strings to psz_.</font></font></p><p lang="en-US" style="margin-bottom: 0cm"><span style="font-family: Helvetica, serif; ">I
now also realized that we misunderstood the handling of prefixes for
pointers to integers, floats etc. I changed variable names
accordingly. </span></p><div><br></div></div><br><blockquote type="cite"><br><blockquote type="cite"><blockquote type="cite"><blockquote type="cite">+    for( int ii = 0; ii<i_n_dims; ii++ ) /* go through all dimensions of<br>file */ +    {<br>+        nc_inq_dim( i_ncid, ii, c_dim_names[ii], &i_dim_length[ii] ); /*<br>get dimensions */ +        if ( !strncmp("M", c_dim_names[ii], 1 ) ) /*<br>get ID of dimension "M" */ +            i_m_dim_id = ii;<br>+        if ( !strncmp("N", c_dim_names[ii], 1 ) ) /* get ID of dimension<br>"N" */ +            i_n_dim_id = ii;<br>+    }<br></blockquote><br>ii ??<br></blockquote><br>„ii“ has the advantage that you can search for it within the code. If you<br>search for „i“, you will obviously get a lot of results you don't want. I<br>admit that its use here is not consistent with the rest of the source code,<br>therefore I changed it to „i“.<br></blockquote><br>Yes but this is confusing. "i" is well known to mean index in this context, <br>not "ii". If you don´t like "i", then use "idx" or "it" or something.<br></blockquote><div>
        
        
        <p lang="en-US" style="margin-bottom: 0cm"><font color="#000000"><font face="Helvetica, serif">Ok.
I have already changed it to „i“.</font></font></p></div><div><br></div><br><br><blockquote type="cite"><blockquote type="cite"><blockquote type="cite"><blockquote type="cite">+    p_sys->i_buffer_length = pow(2, ceil(log( i_n_max )/ log(2) ) ); /*<br>buffer length as power of 2 */> <br></blockquote>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><br>Note the ceil in the code. So, we first do log2 then ceil (to obtain an<br>integer power of 2), then power of 2. For example, if one impulse response<br>is 200 samples long, we want the ringbuffer to be 256 samples for<br>computational efficiency.<br></blockquote><br>François´s point was that you should use the log2() function.<br><br>But actually, if you only care about the integral part of the binary log, then <br>either frexp(), or casting to unsigned integer and clz() would be faster.<br><br><br>By the way, please use the "float" functions with the f-suffix when dealing with <br>"float" values. Double precision is a big waste of CPU power.<br></blockquote><div>
        
        
        <p lang="en-US" style="margin-bottom: 0cm"><font color="#000000"><font face="Helvetica, serif">I
changed the log2 thing to using clz(), thanks.</font></font></p><p lang="en-US" style="margin-bottom: 0cm"><span style="font-family: Helvetica, serif; ">Also,
I now use „float“ functions where possible and reasonable.</span></p><div><br></div></div><br><blockquote type="cite"><br><blockquote type="cite"><blockquote type="cite"><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><br>I'm not sure what you mean!? In case that the second thread fails cloning,<br>would I have to join the first thread and then “goto out”?<br></blockquote><br>Yes. Joining a non-existent thread is undefined (and will typically crash).<br></blockquote><div>
        
        
        <p lang="en-US" style="margin-bottom: 0cm">Agreed, if second thread
fails cloning, I now join first thread and then „goto out“.</p><div><br></div><div><br></div><div>Thanks for commenting! Also see our updated patch which I just posted.</div></div><div>Wolfgang</div><div><br></div><br><blockquote type="cite">-- <br>Rémi Denis-Courmont<br><a href="http://www.remlab.net/">http://www.remlab.net/</a><br><br></blockquote></div><br></body></html>