[vlc-devel] [PATCH] Implementation of Dozer streaming protocol: rework 1

[Wilawar]

I don’t know why you still put the loop counter in fec_xor at the
function body’s scope, but other than that, I guess I’m fine with the
code now. The check for 'f2->i_buffer != 0' is rather indirect and well,
the whole function is not written in my style, but it doesn’t seem like
my style is liked by many, either. ;) The comments are probably OK.

I noticed that you added the variable names to the function
declarations, as requested. Thanks.

> if (unlikely(htons(new_hdr->size) != (new->i_buffer - sizeof(struct
> dozer_hdr)))) {
>     msg_Err(access, "Dozer Recovery FAILED. Invalid size %u for
>     recovered packet.",
>             ntohs(new_hdr->size));
I told you the BE protocol definition was going to cause problems … :P
(htons instead of ntohs)


> msg_Dbg(access, "Dozer Packet Recovered. Cur win: %u idx: %u buffer
> size: %lu size  \
>         stored: %u", ntohs(win),  new_idx, new->i_buffer, ntohs(new_hdr-
>         >size));
This one looks bogous as well, why should 'win' have to be converted?
(Line 425)

> if ((new_idx == sys->fec_last_rx + 1U)) {
>     sys->fec_last_rx++;
> }
>
> while (sys->fec_last_rx < (sys->n_tot - 1) && (sys->matrix[sys-
> >fec_last_rx + 1])) {
>     sys->fec_last_rx++;
> }
I assume you’re doing this to keep track of how many packets have
already been registered, but could you please state this definitely in
the source?

fec_rebuild_packet still has the variable 'i' placed at the function
body scope …
fec_try_recovery is unchanged?

Semantically, it doesn’t make sense to call a function with the name
fec_try_recovery from a function that pretends to initialize FEC stuff
(process_fec_initials ->*), some name adjustments (or even refactorings)
might be needed. (*-> This name is also bad. I realize English might not
be the language you’re the most proficient in (nor is it mine), but that
name just doesn’t really seem to make sense. It’s not totally off, it
does convey the notions that something is changed in the packet, that it
has something to do with FEC and that it is to be done at the beginning,
but, it’s just fuzzy and not quite on the point. I hope you understand.
It’s the case for a few other names, too. It really doesn’t help that
the function will do different things based on external state – it will
either just fill in the counts or it will allocate memory and initialize
it in a specific way. I couldn’t come up with any better one after a few
tries, though.)


Well, I don’t feel like continuing now (there have already been two or
three days between starting to write this mail (the top part) and
sending it ;) ), but honestly, scrolling through it, it doesn’t seem
like you have changed any other parts of the code I had looked at,
anyways. (No offense on my part, although given the two NAKs that have
arrived in the meantime, it doesn’t seem like you’ll be able to dodge
improving those pieces of code :P )

-- 
http://www.fastmail.com - The professional email service

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20161019/7c044cfa/attachment.html>