rohc

c_rtp_decide_FO_packet

Asked by yiyuechan

Hello didier,

            in c_rtp_decide_FO_packet(), if (g_context->tmp.send_static && nr_sn_bits <= 14),you choose the "ROHC_PACKET_UOR_2_RTP",However, it maybe " the context contains at least one
IPv4 header with value(RND) = 0" in this "if"situation,which can not use the ROHC_PACKET_UOR_2_RTP according to RFC.
do you think so?
so I don't know how to choose.Thank you.

Best regards,

yiyuechan

Question information

Language:
English Edit question
Status:
Solved
For:
rohc Edit question
Assignee:
No assignee Edit question
Solved by:
Didier Barvaux
Solved:
Last query:
Last reply:
Revision history for this message
Best Didier Barvaux (didier-barvaux) said : 		#1

yiyuechan,

According to the code snippet that you included in your question, you seem to use a 1.7.x version of the ROHC library. The 1.7.x releases were released in 2014. They are very outdated. Please upgrade to the latest version 2.2.0.

Back on your problem. You are right. The code is incomplete. It should count the number of IPv4 headers with value(RND) = 0 and decide which UOR-2* packet to use in consequence. Such a code is available a few lines later : https://github.com/didier-barvaux/rohc/blob/1.7.x/src/comp/c_rtp.c#L571 . Newer versions up to 2.2.0 contain the very same mistake.

I was surprised not to have found such a bug since ages despite a large corpus of network flow for testing and fuzzing. So I analyzed the code further and I discovered that that code is in fact dead code :
 * The only STATIC field for the RFC3095-based profiles is the IPv4 Protocol or IPv6 Next Header field. A change of value requires an IR, IR-DYN or UOR-2 packet.
 * The Protocol/NH field is however part of the definition of one ROHC context, so a change of value would cause a change of context. A given context can never see a change of Protocol or NH.
 * The code that checks for Protocol/NH changes and forces an IR, IR-DYN or UOR-2 packet is therefore never used. This is dead code. It can be removed safely.

So, I removed the related code on the master branch (it will be released in the next 2.3.0 release): https://github.com/didier-barvaux/rohc/commit/7f2810441a89433261926e6269d122242706b6fa

Regards,
Didier

Revision history for this message
regulararmy (regulararmy) said : 		#2

Hello didier,

              And more, the ssrc is the static part of RTP packet, However, you count it as rtp_context>tmp.send_rtp_dynamic,is it rgiht?
              another, in changed_dynamic_one_hdr(),why think the "df" as nb_field, but think the "rnd""nbo" as nb_flags, think the "sid" as nothing,Thank you.

Best regards,
 regulararmy

Revision history for this message
yiyuechan (yiyuechan) said : 		#3

Thanks Didier Barvaux, that solved my question.

Revision history for this message
Didier Barvaux (didier-barvaux) said : 		#4

Hello,

Many thanks for your questions. You discover problems in legacy code, that's great! :-)

You are right about the RTP SSRC field. The field cannot change for one given ROHC stream, so the check for changes is useless. I removed it on master branch. See https://github.com/didier-barvaux/rohc/commit/ddd5abb6be35ec6bf2c83cd7684b37fa8093c1c0

The logic behind the DF, RND, NBO and SID fields is not defined well. Currently packet type IR-DYN is forced if there are 3 or more "dynamic fields" that changed. However, UO packets with Extension 3 can transmit changes for TTL, TOS, DF, NBO, RND. Those changes are counted as 4 changed fields, so UO with EXT3 is not used while it could be. It might save some bits. The logic behind dynamic fields should be reworked. I tried to do that on the master branch. See https://github.com/didier-barvaux/rohc/commit/ce2d85396ee0b18e7ce376d28aae12f1b83806a0

Please give the updated master branch a try, and tell me how it behaves for you.

Regards,
Didier