[vlc-devel] [PATCH] Implementation of Dozer streaming protocol

[Wilawar]

When I started going through this code for real, I realized it was not only a ton of work to review this code
diligently (as I had feared), it was even more. ;) So, I could have read through it from top to bottom and see 
how far I could get, but instead I opted to focus on the fec functions in the input module as I had already
started with one of those; however, I do have remarks about other parts I’ve read.
I inserted line breaks at a bit over 100 colums here (I think the maximum is 108), let me know how readable it is.
I wasn’t able to find the preview function here …

== General remarks, not specific to any function in particular ==
I don’t think attaching generic language type shorthands to variable names (such as psz) is really useful.
What I use myself and don’t object are shorthands for the _semantic_ type variables have.
I’ve noticed that they aren’t used everywhere, which makes their use all the more suspicious.
As far as I’m concerned, all of them could be removed. (Have fun! :P)

The names you give to variables are very short and very nondescriptive throughout.
I know other people code like this, but I do want to express my discontent over this.

You did use 'const' in some places, as a search has revealed to me, but it seems to
be scattered only around randomly (probably where you borrowed code from other files? :P).
I suggest using const where applicable, as I’ve demonstrated in my fec_xor replacement code.

I don’t like empty lines everywhere. I do know others do, but I wanted to mention it at least.

> new_hdr->seq = htons(new_idx);
As you’re trying to establish a new protocol, you should consider taking the chance to define it in terms
of the now-ubiquitous little-endian, this will help you save conversions everywhere.
Especially since you keep repeating the same conversions. There is hope any good compiler’s optimizer
would take them out, but it seems unwise to rely on it. There’s also danger of forgetting it at some point.
Does VLC support any BE architectures? If so, there would still have to be some guarding function to be
used for accesses, of course, even if the protocol was changed.
Did you think about converting all fields for which this matters just once, where the packets are received?
You could get rid of all access guards then. This seems like the most sane solution to me and could even
be faster than the current way of doing it.

Many functions (fec_try_recovery is exemplary for this) are written in C89 style, with the declarations at the
start of scopes. There is no need for it, really, j-b said that C99 is ok to use. (You also don’t keep to it in
all places, there are functions where you strayed from that path).

C99 has constants. There’s absolutely no need to use defines everywhere anymore. Use it.

== Other stuff I picked up ==
> For example, for a 1Mbps stream with the default 10x5 matrix with 30%
> overhead, we would have a 500ms buffer and and an adequate recovery of
> up to 3% packet loss.
You should probably put an explanation somewhere how you’ve gotten to this conclusion,
it doesn’t strike me as obvious.

In struct access_sys_t:
> int fd;
> int timeout;
> size_t fifo_size;
> block_fifo_t *fifo;
> vlc_sem_t semaphore;
> vlc_thread_t thread;
Windows doesn’t have fd-s (I don’t know how it’s handled inside VLC). HANDLE is commonly given as void*.
Neither 'semaphore' nor 'thread' tell much about the use of those variables.
The type 'int' for the timeout is due to the UNIX time legacy, I guess.
The name 'fifo' is, not explained and does’t give off much information.

> static block_t *BlockUDP( access_t *, bool * );
> static int Control( access_t *, int, va_list );
> static void* ThreadRead( void *data );
If you repeated the parameter names there, chances would be higher that IDE-s displayed them,
I guess. Although, while I would advocate for the repetition in general, they don’t seem particularly
descriptive here.
BlockUDP suggests to me that the function would do something to make UDP connections get
denied. You should probably rename it as even glancing over the implementation, I cannot say for sure what
it’s supposed to do. (Will it fetch the next block? Why does it have UDP in the name although
I don’t see any UDP functions there?)

One from the output module:
> #define COLS_TEXT N_("FEC 2022 Columns")
> #define COLS_LONGTEXT N_( \
>     "Number of Columns to use for the FEC 2022 matrix. Default is 10 cols and" \
>     " 5 rows for 30% overhead and about 3% packet loss recovery" )
> 
> #define ROWS_TEXT N_("FEC 2022 Rows")
> #define ROWS_LONGTEXT N_( \
>     "Number of Rows to use for the FEC 2022 matrix. Default is 5 rows and" \
>     " 10 cols for 30% overhead and about 3% packet loss recovery" )
Repetition in the description. No need to advertise as much. :P

And something else:
> static block_t *process_fec_row(sout_access_out_t *p_access, block_t *p_buffer, uint64_t seq)
> static block_t *process_fec_col(sout_access_out_t *p_access, block_t *p_buffer, uint64_t seq)
Why is the type for seq uint64_t here but only uint16_t in the input module? Doesn’t make sense.

== To the fec functions ==
I wrote a replacement for the fec_xor function. I realize my replacement is way heavier and more
verbose, but in my opinion, this is just what it should have been. Feel free to use it or not.
It will have to be adapted, of course, as I left the parameter names unchanged and also didn’t bother
to look up what the proper replacement for assert is in VLC’s context (as you should already know it)

I couldn’t deduce what sys->fec_last_rx was to be used for. Adding some comments would help.
> while (sys->fec_last_rx < (sys->n_tot - 1) && (sys->matrix[sys->fec_last_rx + 1]) ) {
>     sys->fec_last_rx++;
> }
Comment, pls? Seems like it’s checking for gaps in the population of the packet matrix and setting
fec_last_rx to the highest numbered unpopulated entry (for whichever reason)
!!The most current packet it knows about

I already spoke above about the placement of declarations in fec_try_recovery.

> if (pkt_win != sys->fec_cur_win) {
>     block_Release(pkt);
>     return;
> }
You’re dealing with packets in units of a certain size (the FEC context size, I suppose, the matrix)?
Fine, care to document this anywhere? ;)
In addition, if I’m not mistaken, there should be at least two recovery windows active during
the transition period from one to the other* (which is most of the time), more if
packet recovery rate can be traded for higher latency. In that context, he snippet doesn’t seem right.
Feel free to ignore this remark if you are sure this bit of the logic is correct.

> sys->fec_cols[pkt_seq] = pkt;
pkt_seq has never been checked. Vulnerability reports incoming …

> new = block_Alloc(sys->mtu);
> memset(new->p_buffer, 0, sys->mtu);
> new->i_buffer = 0;
> fec_xor(new, new, pkt);
I already told you in IRC that this doesn’t make sense and memcpy should instead be used.

> if (pkt_type == FEC_TYPE_FEC_COL) {
> 	sys->fec_cols[pkt_seq] = pkt;
> 	for (i = 0; i < sys->n_rows; i++) {
> 		idx = i * sys->n_cols + pkt_seq;
> 		if (!sys->matrix[idx]) {
> 			found++;
> 			rec_r = i;
> 			new_idx = idx;
> 		}
> 	}
> 	if (found == 1) {
> 		new = block_Alloc(sys->mtu);
> 		memset(new->p_buffer, 0, sys->mtu);
> 		new->i_buffer = 0;
> 		fec_xor(new, new, pkt);
> 		for (i = 0; i < sys->n_rows; i++) {
> 			if (i != (uint32_t)rec_r) {
> 				fec_xor(new, new, sys->matrix[i * sys->n_cols + pkt_seq]);
> 			}
> 		}
I think I see what this code should do. If there is exactly one entry missing in
the matrix, it is recovered by XOR-ing with the other entries. I suggest two
refinements: Rename the variable 'found' to 'missing' or 'NrOfMissingEntriesInLine'
or another name that describes better what it is there for and don’t attempt to
recover the added error correction data. (It’s only the payload data that matters)
There is no reason to have rec_r and rec_c as only one of them will be used at any time.

> /* If found and recovered... */
> if (new) {
>     fec_found_and_recovered(access, new, new_idx, ntohs(hdr->win));
> }
Oh, so contrary to what I assumed, this number isn’t checked either.
If anyone were looking to revive the ping of death, they should try hooking into Dozer streams.
Would be fun to shut down half of your viewerbase’s video players with a single malformed packet …

In process_fec_initials:
> uint16_t pkt_type   = ntohs(hdr->type);
> uint16_t pkt_count   = ntohs(hdr->count);
>
> if (pkt_type == FEC_TYPE_FEC_COL) {
>     sys->n_cols = pkt_count;
> }
> if (pkt_type == FEC_TYPE_FEC_ROW) {
>     sys->n_rows = pkt_count;
> }
Yeah, no safety or sanity checks there either.
Also, as I don’t want to read through the other code, I cannot know, but does this mean
that the two sizes will be transmitted in two separate packets? Please make sure not to make this
unnecessarily inefficient …

> sys->fec_initialized = 1;
Seems like you could use n_tot to check whether it’s initialized. I’m not sure you should, though,
such reuse can end up confusing. Maybe n_tot would have to be renamed then.

recover_frame has only stuff I already wrote about. Otherwise, it looks somewhat sane.

Regarding process_fec, signature is better abbreviated as 'sig' not 'sign':
> uint16_t pkt_sign   = ntohs(hdr->signature);
If you don’t decide to do the order conversion once (as I’ve suggested above), consider to at least
keep data-independent constants in converted form. (The packet types and SIGNATURE, from what
I’ve seen so far; there might be others)

* There is this bit in frame_to_matrix, but I don’t see how it’d work out. Isn’t the comparison backwards?
> win = ntohs(hdr->win);
> if (sys->fec_cur_win != win) {
>    if (((uint16_t)(sys->fec_cur_win - win)) > 3) {
>        /* Next win started. */

-- 
http://www.fastmail.com - IMAP accessible web-mail