<p>Hi Daniele and Sergio,</p>
<p>I have not compiled your implementation, so the opinions expressed in this reply is my immediate reactions to reading your patch here on the mailing-list.</p>
<p>I am aware that you have used other code within VLC as inspiration for your implementation, but be careful using old code as a guideline (some stuff are simply legacy, and are not using features that are now available).</p>
<hr style="height:1px;margin-bottom:20px;background-color:#ddd;color:#ddd" />
<p>On 2016-09-30 14:14, Daniele Lacamera wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> Added access + access_output modules implementing Dozer streaming protocol.
A lua plugin for wireshark dissector is available at
https://gist.github.com/danielinux/d9e854962e88b33ee77d41009fc0497f
access/Makefile.am | 4
access/dozer.c | 723
++++++++++++++++++++++++++++++++++++++++++++++
access_output/Makefile.am | 5
access_output/dozer.c | 673 ++++++++++++++++++++++++++++++++++++++++++
4 files changed, 1404 insertions(+), 1 deletion(-)</code></pre>
</blockquote>
<hr style="height:1px;margin-bottom:20px;background-color:#ddd;color:#ddd" />
<h3 id="code-duplication">code duplication</h3>
<p>The implementation contains <em>a lot</em> of code-duplication, which can be gotten rid of with a little bit of code refactoring.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + p_sys->fec_rows = calloc(p_sys->i_rows * sizeof(block_t *), 1);
+ if (!p_sys->fec_rows) {
+ msg_Err( p_access, "cannot allocate FEC matrix" );
+ block_FifoRelease( p_sys->p_fifo );
+ block_FifoRelease( p_sys->p_empty_blocks );
+ net_Close (i_handle);
+ free (p_sys);
+ return VLC_EGENERIC;
+ }
+ p_sys->fec_cols = calloc(p_sys->i_cols * sizeof(block_t *), 1);
+ if (!p_sys->fec_cols) {
+ free(p_sys->fec_rows);
+ msg_Err( p_access, "cannot allocate FEC matrix" );
+ block_FifoRelease( p_sys->p_fifo );
+ block_FifoRelease( p_sys->p_empty_blocks );
+ net_Close (i_handle);
+ free (p_sys);
+ return VLC_EGENERIC;
+ }
+ if( vlc_clone( &p_sys->thread, ThreadWrite, p_access,
+ VLC_THREAD_PRIORITY_HIGHEST ) )
+ {
+ msg_Err( p_access, "cannot spawn sout access thread" );
+ block_FifoRelease( p_sys->p_fifo );
+ block_FifoRelease( p_sys->p_empty_blocks );
+ net_Close (i_handle);
+ free (p_sys);
+ free(p_sys->fec_rows);
+ free(p_sys->fec_cols);
+ return VLC_EGENERIC;
+ }</code></pre>
</blockquote>
<p>A very good example is the one pasted above, from within the implementation of <code>modules/access_output/dozer.c:Open</code>.</p>
<hr style="height:1px;margin-bottom:20px;background-color:#ddd;color:#ddd" />
<h3 id="unchecked-return-values">unchecked return-values</h3>
<p>The error-checking within the implementation is very inconsistent. In some parts of the code everything looks fine, whereas in others the *error-checking’ is lacking completely.</p>
<p>As an example, in the below <code>p_buffer</code> can potentially be <code>NULL</code> if <code>block_Alloc</code> or <code>block_Realloc</code> fails (for whatever reason).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +static block_t *NewUDPPacket( sout_access_out_t *p_access, mtime_t i_dts)
+{
...
+ if( block_FifoCount( p_sys->p_empty_blocks ) == 0 )
+ {
+ p_buffer = block_Alloc( p_sys->i_mtu + sizeof(struct dozer_hdr));
+ }
+ else
+ {
+ p_buffer = block_FifoGet(p_sys->p_empty_blocks );
+ p_buffer->i_flags = 0;
+ p_buffer = block_Realloc( p_buffer, 0, p_sys->i_mtu + sizeof(struct dozer_hdr));
+ }
+
+ p_buffer->i_dts = i_dts;
+ p_buffer->i_buffer = 0;
+
+ return p_buffer;
+}</code></pre>
</blockquote>
<p>A few notes:</p>
<ul>
<li><p><em>helper-utilities</em> created with <em>vlc</em> functions ending in “<code>New</code>” can most often fail (given that they allocate something), as such their <em>return-value</em> should be checked. As an example <code>block_FifoNew</code>.</p></li>
<li><p>most functions related to releasing resources aquired by <em>helper-utilities</em> within <em>vlc</em> <strong>cannot</strong> be invoked with <code>NULL</code>.</p></li>
<li><p>make use of <code>likely</code> vs <code>unlikely</code> where applicable.</p></li>
<li><p>take advantage of the fact that <code>free</code> on <code>NULL</code> is <em>well-defined</em>.</p></li>
</ul>
<hr style="height:1px;margin-bottom:20px;background-color:#ddd;color:#ddd" />
<h3 id="do-not-use-verbose-logging-during-open">Do not use verbose logging during <code>Open</code></h3>
<p>There might be other modules than your own that can handle a certain entity, and if such actually can open a resource where yours does not work, usage of <code>msg_Err</code> during <code>Open</code> is very distracting (and as such not encouraged).</p>
<hr style="height:1px;margin-bottom:20px;background-color:#ddd;color:#ddd" />
<h3 id="log-messages-should-only-contain-relevant-information">Log messages should only contain relevant information</h3>
<p>Please do not provide unnecessary information in your diagnostics. There is absolutely no point in padding your message, like in the below example.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +static void process_fec_initials(access_t *access, block_t *pkt)
+{
...
+ if ((sys->n_cols > 0) && (sys->n_rows > 0)) {
+ sys->n_tot = sys->n_cols * sys->n_rows;
+ msg_Dbg(access, "******* Initialized Dozer FEC recovery matrix %ux%u", sys->n_cols, sys->n_rows);</code></pre>
</blockquote>
<hr style="height:1px;margin-bottom:20px;background-color:#ddd;color:#ddd" />
<h3 id="indentation">Indentation</h3>
<p>Do not use tabs to indent your code, instead use an appropriate amount of spaces.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + vlc_fifo_Lock(sys->fifo);
+ fifo_enqueue(access, pkt);
+ vlc_sem_post(&sys->semaphore);
+ vlc_fifo_Unlock(sys->fifo);</code></pre>
</blockquote>
<p>It is especially discourage to mix usage of spaces and tabs, and I reckon most of the places that caught my attention in regards of weird indentation has been hidden by the editor of choice because of the mixing.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + free( psz_name );
+ if( sys->fd == -1 )
+ {
+ msg_Err( p_access, "cannot open socket" );
+ goto error;
+ }</code></pre>
</blockquote>
<p>I guess the below is an attempt to have the <em>argument-list</em> of the individual functions line-up, but as shown below.. it doesn’t really look that good.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +static ssize_t Write ( sout_access_out_t *, block_t * );
+static int Seek ( sout_access_out_t *, off_t );
+static int Control( sout_access_out_t *, int, va_list );</code></pre>
</blockquote>
<hr style="height:1px;margin-bottom:20px;background-color:#ddd;color:#ddd" />
<h3 id="use-helpers-available-within-vlc">Use helpers available within VLC</h3>
<p>One suitable example would be using <code>vlc_UrlParse</code> (and relevant functions), instead of reinventing the wheel to parse things manually, in <code>modules/access/dozer.c:Open</code>.</p>
<hr style="height:1px;margin-bottom:20px;background-color:#ddd;color:#ddd" />
<h3 id="attribute-packed">Attribute <code>packed</code></h3>
<p>This is actually very important given that it is directly related to the correctness and robustness of the implementation, so think about this in full.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +struct __attribute__((packed)) dozer_hdr {
+ uint16_t signature;
+ uint16_t type;
+ uint16_t seq;
+ uint16_t win;
+ uint16_t count;
+ uint16_t size;
+};</code></pre>
</blockquote>
<p>Instead of relying on <em>compiler-specific</em> behavior in terms of attributes (ie. potentially running into builds where things does not work as intended, or does not build at all), use the provided helpers to read/write <code>N</code> bits of data.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + struct dozer_hdr *new_hdr = (struct dozer_hdr *)new->p_buffer;
+ access_sys_t *sys = access->p_sys;
+
+ new_hdr->type = FEC_TYPE_DATA;
+ new_hdr->seq = htons(new_idx);
+ new_hdr->win = htons(win);
+ new_hdr->count = 0;</code></pre>
</blockquote>
<p>See the helpers available in <code>include/vlc_common.h</code> for more information.</p>
<hr style="height:1px;margin-bottom:20px;background-color:#ddd;color:#ddd" />
<h3 id="write-code-that-is-consise-and-easy-to-read">Write code that is consise and easy to read</h3>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +static void fec_xor(block_t *dst, block_t *f1, block_t *f2)
+{
+ uint32_t len, len1, len2;
+ uint32_t i;
+
+
+ len1 = f1->i_buffer;
+ len2 = f2->i_buffer;
+ len = len1;
+
+ if (len2 > len1)
+ len = len2;
+
+ if (len < 10)
+ return;
+
+ for(i = 10; i < len; i++) {
+ if (i >= len1) {
+ dst->p_buffer[i] = f2->p_buffer[i];
+ } else if (i >= len2) {
+ dst->p_buffer[i] = f1->p_buffer[i];
+ }
+ else dst->p_buffer[i] = f1->p_buffer[i] ^ f2->p_buffer[i];
+ }
+ dst->i_buffer = len;
+}</code></pre>
</blockquote>
<p>The above can be written <em>sooo</em> much cleaner, and I give the above as an example to try and point out that a different mindset might be necessary when writing code.</p>
<hr style="height:1px;margin-bottom:20px;background-color:#ddd;color:#ddd" />
<h3 id="potential-division-by-zero">potential division-by-zero</h3>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +static block_t *process_fec_row(sout_access_out_t *p_access, block_t *p_buffer, uint64_t seq)
+{
+ sout_access_out_sys_t *p_sys = p_access->p_sys;
+ uint16_t win = seq / p_sys->i_fecsize;
+ uint16_t iseq = seq % p_sys->i_fecsize;
+ uint16_t row = iseq / p_sys->i_cols;
+ uint16_t col = iseq % p_sys->i_cols;
...
+}</code></pre>
</blockquote>
<p>There are several places where unexpected input will lead to the implementation doing mathematically undefined stuff, such as <em>division-by-zero</em>.</p>
<p>See the above for one of the many places.</p>
<h3 id="unnecessary-casts">Unnecessary casts</h3>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + case STREAM_GET_PTS_DELAY:
+ pi_64 = (int64_t*)va_arg( args, int64_t * );
+ *pi_64 = INT64_C(1000)</code></pre>
</blockquote>
<p><code>va_arg( args, int64_t* )</code> does not require a cast, the yield type is already <code>int64_t*</code>. The above is just one of several places where unnecessary casts are used.</p>
<hr style="height:1px;margin-bottom:20px;background-color:#ddd;color:#ddd" />
<hr style="height:1px;margin-bottom:20px;background-color:#ddd;color:#ddd" />
<p>Other than what is mentioned further up in this email, there are places where your implementation will potentially crash (given the “right” <em>input-data</em>).</p>
<p>I vote that you fix the code-duplication and other stuff, and most of these cases will expose themselves during clean-up. What has been pointed out should be enough to get you started on a little <em>“clean-up mission”</em>.</p>
<p>Best of luck!</p>
<p>[ resending this email given that it seems like my previous one did not get through ]</p>