[vlc-devel] [PATCH] Implementation of Dozer streaming protocol
Filip Roséen
filip at atch.se
Sat Oct 1 21:52:54 CEST 2016
Hi Daniele and Sergio,
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.
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).
----------------------------------------------------------------------
On 2016-09-30 14:14, Daniele Lacamera wrote:
> 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 duplication
----------------------------------------------------------------------
The implementation contains *a lot* of code-duplication, which can be
gotten rid of with a little bit of code refactoring.
> + 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;
> + }
A very good example is the one pasted above, from within the
implementation of `modules/access_output/dozer.c:Open`.
----------------------------------------------------------------------
unchecked return-values
----------------------------------------------------------------------
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.
As an example, in the below `p_buffer` can potentially be `NULL` if
`block_Alloc` or `block_Realloc` fails (for whatever reason).
> +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;
> +}
A few notes:
- *helper-utilities* created with *vlc* functions ending in "`New`"
can most often fail (given that they allocate something), as such
their *return-value* should be checked. As an example
`block_FifoNew`.
- most functions related to releasing resources aquired by
*helper-utilities* within *vlc* **cannot** be invoked with `NULL`.
- make use of `likely` vs `unlikely` where applicable.
- take advantage of the fact that `free` on `NULL` is *well-defined*.
----------------------------------------------------------------------
Do not use verbose logging during `Open`
----------------------------------------------------------------------
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 `msg_Err` during `Open` is very distracting (and as
such not encouraged).
----------------------------------------------------------------------
Log messages should only contain relevant information
----------------------------------------------------------------------
Please do not provide unnecessary information in your diagnostics.
There is absolutely no point in padding your message, like in the
below example.
> +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);
----------------------------------------------------------------------
Indentation
----------------------------------------------------------------------
Do not use tabs to indent your code, instead use an appropriate amount
of spaces.
> + vlc_fifo_Lock(sys->fifo);
> + fifo_enqueue(access, pkt);
> + vlc_sem_post(&sys->semaphore);
> + vlc_fifo_Unlock(sys->fifo);
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.
> + free( psz_name );
> + if( sys->fd == -1 )
> + {
> + msg_Err( p_access, "cannot open socket" );
> + goto error;
> + }
I guess the below is an attempt to have the *argument-list* of the
individual functions line-up, but as shown below.. it doesn't really look
that good.
> +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 );
----------------------------------------------------------------------
Use helpers available within VLC
----------------------------------------------------------------------
One suitable example would be using `vlc_UrlParse` (and relevant
functions), instead of reinventing the wheel to parse things manually,
in `modules/access/dozer.c:Open`.
----------------------------------------------------------------------
Attribute `packed`
----------------------------------------------------------------------
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.
> +struct __attribute__((packed)) dozer_hdr {
> + uint16_t signature;
> + uint16_t type;
> + uint16_t seq;
> + uint16_t win;
> + uint16_t count;
> + uint16_t size;
> +};
Instead of relying on *compiler-specific* 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 `N` bits of data.
> + 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;
See the helpers available in `include/vlc_common.h` for more
information.
----------------------------------------------------------------------
Write code that is consise and easy to read
----------------------------------------------------------------------
> +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;
> +}
The above can be written *sooo* 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.
----------------------------------------------------------------------
potential division-by-zero
----------------------------------------------------------------------
> +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;
> ...
> +}
There are several places where unexpected input will lead to the
implementation doing mathematically undefined stuff, such as
*division-by-zero*.
See the above for one of the many places.
Unnecessary casts
----------------------------------------------------------------------
> + case STREAM_GET_PTS_DELAY:
> + pi_64 = (int64_t*)va_arg( args, int64_t * );
> + *pi_64 = INT64_C(1000)
`va_arg( args, int64_t* )` does not require a cast, the yield type is
already `int64_t*`. The above is just one of several places where
unnecessary casts are used.
----------------------------------------------------------------------
----------------------------------------------------------------------
Other than what is mentioned further up in this email, there are
places where your implementation will potentially crash (given the
"right" *input-data*).
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 *"clean-up
mission"*.
Best of luck!
[ resending this email given that it seems like my previous one did
not get through ]
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20161001/dca65dd1/attachment.html>
More information about the vlc-devel
mailing list