final follow-up review of last-call

Hi,

This is a joint review of draft-ietf-rohc-rfc3095bis-rohcv2-profiles
-O4 by myself and Anders.

We have divided our comments into four sections:

* comments: Issues we would like to mention that not necessarily
require action.

* typos: Issues affecting readability.

* errors: Issues that require action.

* formal notation: All issues related to the formal notation part.

regards,

Jani Juvan & Anders Edqvist

====================

Comments

====================

Section 6.6.11:

There is no mention of byte-order for fields that are more than one

byte long, does this need to be mentioned?

Section 6.6.13.5:

"A compressed list that is part of the dynamic chain (i.e. in IR

packets) must have all its list items present, ..."

As this applies to co_repair too, perhaps the "ie" could be removed or

co_repair added to make it obvious?

Section 6.8:

Here the terms "packet types" and "packet formats" are defined, but
they

are used arbitrarily throughout the draft.

Section 6.8.2:

The co_repair packet format causes exceptions to most co packet

generalizations. It might have been easier to separate it as a
different

packet type.

Section 6.9.2.2:

There is no mention of how an ACK with an ACKNUMBER-NOT-VALID option

is to be interpreted.

====================

Typos, grammar and similar:

====================

Either write RFC with space or without space before number. Not both!

(I do not mean [bracket] references)

"RFC XXX" or "RFCXXX"

Section 2, Chaining of Items:

replace:

"Chaining is achieved by appending to the chain an item for each
header

in their order of appearance in the uncompressed packet."

with:

"Chaining is achieved by appending an item to the chain for each
header

in their order of appearance in the uncompressed packet."

Section 3:

CSRC

"CSRC Contributing source. Optional list of CSRCs in RTP header."

This feels like a circual reference, could it be replaced with:

"CSRC Contributing source. The RTP header contains a list of

contributing sources"

Section 3:

Should TTL be here?

Section 3:

For TOS and TC is says octet, could we stick to field instead?

Section 4.3:

"Robust header compression can be used over many type+++s+++ of link

technologies.

Section 5.1.1:

"If the decompressor successfully receives any of the headers

containing this update, ???the???state will be available for

the decompressor to process smaller compressed headers."

"state will be available" sounds strange.

Section 5.1.3:

"In other words, the responsibility of the compressor are to
ensure.."

Should be

"In other words, the responsibilities of the compressor are to
ensure.."

or

"In other words, the responsibility of the compressor is to
ensure.."

Section 6.2:

"The compressor always start assuming that the decompressor will
not"

Replace with

"The compressor always starts by assuming that the decompressor will
not"

Section 6.5:

"ROHCv2 profiles compress---es--- extension headers as other
headers,"

Section 6.6.8:

"This section provide+++s+++ additional details..."

Section 6.6.8:

"...use the new scaling factor once it has ---have--- enough

confidence..."

Section 6.8.2.1:

"common packet format+++s++ shared" and "It can update control---s---

fields"

Section 6.8.2.3:

It would be better to show the packet format instead of the header

format (i.e. add the payload) just like for IR and co_repair.

Appendix A:

RARELY CHANGING: Since RC is already used to mean Repair

Context State and defined so under Acronyms I'd rather see it not

defined to mean something else in the appendix.

PATTERN: "These field that change"

Replace with:

"These fields change"

or:

"These are fields that change"

====================

Errors

====================

Section 5.2.1:

There are transition arrows missing in the state machine, shouldn't

FC state have transistion !CRC3 !CRC7, like RC has?

Section 6.5:

"For each compressed packet, the irregular chain is appended at.."

This is not true for CO repair.

Section 6.5:

"The irregular chain is used in all CO packets"

This is not true for CO repair.

Section 6.5:

"The format for the irregular chain item for the outer IP header is

also determined using one flag for TTL/Hop Limit and one for TOS/TC."

not true any more. replace by:

"using one flag for TTL/Hop Limit and TOS/TC."

Section 6.6.3 inferred_udp_length :

"The decompressor infers the value of the UDP length field as being

the size of +++the UDP header and+++ the UDP payload."

Further, why not cite RFC768 as done in 6.6.4 and 6.6.7?

Section 6.6.13.1:

"2) When the structure of the list is unchanged, no information

about the list is sent in compressed headers."

This is not true for CO repair were it is always sent.

Appendix B:

There is a parantheses missing in the ceiling calculation.

====================

The formal notation part

====================

MSN is not initialized in FN for profiles 0x0102, 0x0104 and 0x0108

(6.3.1 says it should be initialized to a random value).

The global control field ip_id_behavior_innermost should be 2 bits

instead of 16.

All global control fields should have static encoding by default. This

to ensure that they can keep their previous values between packets.

There is a problem with how DF is handled in IPv6: Since DF has

ULENGTH==0 for ipv6 but methods like dont_fragment and

profile_1_7_flags_enc has a DF with ULENGTH==1 in its UNCOMPRESSED

format the binding will not be awesome. The concatenated list of

flags will simply not match as intended to the bitpattern in the

method. This problem affects all profiles and several encoding methods.

COMPRESSED ipv4_regular_innermost_dynamic:

It should be a && instead of the || in the first ENFORCE
statement.

COMPRESSED ipv4_outer_dynamic:

The first ENFORCE statement should only be ENFORCE(is_innermost == 0);

udp()

The control field checksum_used should have static encoding by default.

COMPRESSED udp_regular_dynamic

The second ENFORCE statement should be ENFORE(checksum_used.UVALUE =

(checksum.UVALUE != 0)).

udp_lite()

coverage_behavior should be a global control field and should be given

static encoding by default.

The same also applies for udp_lite/rtp.

COMPRESSED udp_lite_regular_dynamic

Missing ENFORCE(profile_value == PROFILE_RTP_0107)

variable_scaled_timestamp()

for the compressed not_present format the ENFORCE statement should

be ENFORCE(tsc_flag == 0), the other is unnecessary.

profile_1_7_flags1_enc()

This method should take ip_version as argument.

profile_1_flags2_enc()

Should remove ip_version from arguments.

The order of the fields in the UNCOMPRESSED format should be changed

to the order of the COMPRESSED format "present". This is what the

"calling" method expects.

profile_7_flags2_enc()

ip_version should not be an argument.

====================

= RTP PROFILE

====================

It would ease implementation if pt_1_rnd and pt_1_seq_ts looked the

same (i.e. move the marker bit in pt_1_rnd).

In pt_1_seq_ts and pt_2_seq_ts ip_id should be encoded as

inferred_sequential_id_id, alternatively this could be done by

default.

====================

= IP ONLY PROFILE

====================

pt_0_crc7 has msn incorrectly encoded as msn_lsb(7), it should be

msn_lsb(6).

====================

= UDP-LITE/RTP PROFILE

====================

co_common, the order of the items in the concatenated list bound to

profile_7_flags2_enc does not match the UNCOMPRESSED format of that

method. switch places for tis_indicator and pt_indicator.

co_common, I'd prefer to see 8 instead of ttl_hopl.ULENGTH in the

ttl_hopl binding.

It would ease implementation if pt_1_rnd and pt_1_seq_ts looked the

same (i.e. move the marker bit in pt_1_rnd).

In pt_1_seq_ts and pt_2_seq_ts ip_id should be encoded as

inferred_sequential_id_id, alternatively this could be done by

default.

====================

= UDP-LITE PROFILE

====================

pt_0_crc7 has msn incorrectly encoded as msn_lsb(7), it should be

msn_lsb(6).

_______________________________________________
Rohc mailing list
[email protected]
https://www1.ietf.org/mailman/listinfo/rohc