← Back to team overview

rohc team mailing list archive

Re: Support of RFC4996 RoHC-TCP



> > 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

> > ==== 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.


> > ==== 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.


> >   - 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.


> >   - 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.


> >   - 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]


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


> >   - 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

> >   - 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.


Attachment: signature.asc
Description: PGP signature

Follow ups