rohc team mailing list archive

[Didier Barvaux <didier@xxxxxxxxxxx>]

FWX,

> > I must say that I didn't test it for the moment, as there is some
> > work to get it up to date with the current trunk (my fault). I'll
> > do the merge ASAP. I'll do it in a dedicated branch, then when it
> > will be ready we will merge it in the trunk.
> >
> 
> -->> Ok. No Problem.

I propose you that I perform the merge with current trunk if you
didn't already started the work. I know the changes that I did on the
trunk, so that should be easier for me. Is it OK for you?

When the branch will be created, we could improve the code step by step
together.


> > ==== common ====
> >
> > => src/common/ipproto.h:
> >
> >   - why a (partial) list of IP protocol numbers?
> 
> -->> Because others IP protocol numbers are defined in other include 
> files, with compiler for example. Here are only those not found
> elsewhere.

OK. I see you used the array for header parsing. We could discuss this
further when merging header parsing.


> > => tcp_profile/common/tcp.h:
> >
> >   - why are the *_(TO|FROM)_MPTR macro for?
> 
> -->> For ARM support, because of alignment problem.

OK, why not using the ARM ones for all platforms?


> >   - why defining the base_header_ip_t, multi_ptr_t, WB_t and LWB_t
> >     unions?
> 
> -->> With this method, you have a single pointer to a lot of
> different types of datas. In the IP header, you don't know, at the
> begin, what you'll find : TCP, RTP, etc... WB_t and LWB_t are types
> defined for use when you need to access datas as bytes table or as
> word (dword).

OK. I used to define a "unsigned char *data" variable that I cast to the
needed header (struct iphdr *, struct udphdr *...) when needed.


> > => src/common/trace.[ch]:
> >
> >   - I think I will enhance src/common/rohc_traces.[ch] with that
> > files, are you OK?
> >
> 
> -->> Ok. At the begining, it was just an internal tool for debug. But
> it may help others.

Fine.


> > ==== compression ====
> >
> > => src/comp/c_rohc_lib.[ch]
> >
> >   - The filename seems too general, tcp_encoding.[ch] seems better.
> >     The methods defined in RFC 4997 could even be split in a new
> > file named rfc4997_encoding.[ch].
> 
> -->> Ok for rfc4997_encoding.[ch]. And I'll create also 
> rfc5225_encoding.[ch] for RoHCv2 encoding methods.

Fine.


> >   - c_lsb(): according to RFC 4997 section 4.11.5, it seems that the
> >     'lsb' encoding scheme matches the W-LSB one already defined in
> >     src/common/wlsb.c.
> 
> -->> Perhaps, but I prefer use my code, for the moment.
>
> >   - c_ip_id_lsb(): could be merged with code from c_generic.c
> 
> -->> Same answer. My code is more short and quick than the other...

My intention was not to replace the code immediately :) But we'll have
to merge the code later...


> >   - lsb_masks: I prefer avoiding global variables...
> 
> -->> Ok, but how to use same variables in compression and
> decompression source files, without duplicate them ? Put them in a C
> file in common ?

Put a function in a .c file in common, and then use that function in
the comp/decomp parts. The variable can then be local to the function.


> >   - unit tests should be written for these encoding functions
> 
> -->> I'm agree with you. But all existing tests are passed ok.

Not breaking existing tests is a great thing, but my experience with
RFC 3095 encoding/decoding methods told me to write tests for "basic"
operations, because it is easier to debug basic operation than a
non-reproducible compression/decompression problem in a several-million
streams of packets.

No hurry here, we could add these tests step by step.


> >   - c_tcp_create(): the code that parse IP headers in IP/UDP/RTP
> >     profiles could probably be re-used (and maybe enhanced if
> > needed)
> 
> -->> Yes, it will be a good idea, in rohc_compress.c for example, or 
> c_generic.c. I'll try to write a good code, that can be enhanced.

I meant that the IP/UDP/RTP profiles already have code for parsing
uncompressed headers. Your code does the same. They could be merged.


> >   - the call to random() must be replaced by a call to the dedicated
> >     callback function in the library
> 
> -->> Which callback function?

I added it after your patch proposal. It is a user callback that allows
an application to specify its own random function. It allows the
library to be more platform-independent.

See
http://rohc-lib.org/doc/rohc-doc-1.5.0/group__rohc__comp.html#gad4f6281794cb8db147dc64abbd8874d1


> >   - c_tcp_check_context(): the code from c_generic could be re-used
> 
> -->> Can you be more precise?

The *_check_context() functions of the IP/UDP/RTP profiles partly do
the same job. They share code in common (the rtp one calls the udp one
for example). The TCP function could call the IP function for a part of
its job.


> >   - rohc_v2_code_dynamic_ipv6_option_part(): why "v2" ?
> 
> -->> For historical reason... I code these functions for RoHC V2 
> support, then copy them in the TCP support. Code is the same.

OK.


> >   - tcp_changed_tcp_dynamic(): unused at the moment
> 
> -->> Yes, because I need your help to code this function. (see DBX 
> comment inside).

What do you need?


> >   - c_ts_lsb(): could probably be moved in c_rohc_lib.[ch]
> 
> -->> It is more easy to have this function near the code where they
> are called. And they are specific to TCP compression. May be moved
> when code will be exact. But it is not urgent.

Of course, it is not the first task to do :)


> >   - tcp_compress_tcp_options(): goto is OK only for error handling,
> > the function should be reworked to get rid of
> >     'new_index_with_compressed_value' and
> > 'same_index_without_value'.
> 
> -->> I have not enough experience to change this code. But why not in 
> the future.

We'll see later how to do this.


> >   - d_lsb(): seems very basic in comparison to the the lsb_decode
> > module.
> 
> -->> Yes, but give me an example where there is a problem ...

We discussed about this some time ago in a private discussion. If I
remember correctly, I gave you an example. However I have to check that
to be sure...


> >   - The filename seems too general, tcp_decoding.[ch] seems better.
> >     The methods defined in RFC 4997 could even be split in a new
> > file named rfc4997_encoding.[ch].
> 
> -->> I prefer rfc4997_decoding.[ch], according with 
> comp/rfc4997_encoding.[ch]

OK.


> >   - d_ip_id_lsb(): could be merged with ip_id_offset_decode module
> 
> -->> Later, if possible.

Yes.


> >   - my_ip_fast_csum(): duplicate ip_fast_csum() from src/common/ip.h
> 
> -->> Yes, but without alignment problem with ARM. Shorter and more 
> quick. The other code doesn't work with all platforms. I'll suggest a 
> patch to use my code, rather then the other.

Yes, we could add that code in ip.h for the ARM platform. There is
already one optimized version for x86.


> >   - d_tcp_decode_ir(): should be merged with d_tcp_decode()
> > (because of a change between 1.4.0 and 1.5.0 releases, my fault...)
> 
> -->> No idea. I'll look this point.
> 
> >   - tcp_get_static_size(), tcp_detect_ir_size(),
> >     tcp_detect_ir_dyn_size(), and tcp_detect_tcp_dynamic_size():
> > should be removed (because of a change between 1.4.0 and 1.5.0
> > releases, my fault...)
> 
> -->> Same answer.

See my first comment in this email. I can do it if you're not on it
already.

  
> >   - PACKET_TCP_* should be added to enum rohc_packet_t in
> > rohc_packets.h
> 
> -->> There is different types of TCP packets. Not usefull, because
> only used in d_tcp.c file.

Other packet types are profile-specific (UOR-2-TS/ID and UO-1-TS/ID are
defined only for RTP). Having only one enum allows sharing some code
with other profiles.

 
> >   - d_tcp_decode_CO(): the rnd_X/seq_X detection could be put in a
> >     tcp_detect_packet_type() function.
> 
> -->> Perhaps.
> 
> >   - d_tcp_decode_CO(): goto is OK only for error handling, the
> > function should be reworked
> 
> -->> I'm agree with you. I'm waiting for tests to do that.

Yes, of course.


Regards,
Didier

Attachment: signature.asc
Description: PGP signature