Subject: [rohc] final follow-up review of last-call

From: Jani Juvan <[email-masked]>
Date: Fri, 18 Jan 2008 10:13:34 +0100

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-masked]
https://www1.ietf.org/mailman/listinfo/rohc

---

From: "Kristofer Sandlund" <[email-masked]>
Date: Fri, 25 Jan 2008 09:13:22 +0100

Hi,

thanks for a very thorough review.
Answers and suggested action to each comment inlined below.

Jani Juvan <mailto:[email-masked]> wrote on den 18 januari 2008
10:14 :

> Hi,
> 
> This is a joint review of
> draft-ietf-rohc-rfc3095bis-rohcv2-profiles -O4 by myself and Anders.
> 
> * comments: Issues we would like to mention that not
> necessarily require action.

NACK, this seems too obvious already, it is not either done for any
other
ROHC spec defining CRCs.

> co_repair added to make it obvious?

ACK, removed parenthesised text.

> 
> Section 6.8:
> Here the terms "packet types" and "packet formats" are
> defined, but they

ACK, this will be clarified, there was also some confusing use of these
so we moved most of it over to using "header" instead of "packet"

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

Disagree, it has more commonalities with CO than anything else, it is
better to make exceptions clear in the few cases where it "breaks".

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

The text below the figure says:
   A compressor MUST NOT use the Acknowledgment Number of the feedback
   to find the corresponding sent header when this option is present.

This covers how to interpret the situation you describe. No action.

ACK

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

ACK

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

ACK, changed wording slightly differently, though:
  CSRC     Contributing source. The RTP header contains an optional list
           of contributing sources.

> Should TTL be here?

ACK

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

ACK

>   "Robust header compression can be used over many

Changed wording to:
   Robust header compression can be used over different link
   technologies.

>   "state will be available" sounds strange.

NACK, this is as intended, it does not refer to any *specific* state,
it just says that some kind of state will be available.

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

ACK

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

Changed to:
   The compressor always starts with the assumption that the...

> 
> Section 6.5:
>   "ROHCv2 profiles compress---es--- extension headers as
> other headers,"

ACK

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

ACK

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

ACK

> 
> Section 6.8.2.1:
>   "common packet format+++s++ shared" and "It can update
> control---s--- fields"

ACK

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

Good catch, but the solution is actually the opposite: remove the
payload from the other formats in order to be consistent with
RFC4995 section 5.2.1. 
Also changed to "header" in more places in the draft

> defined to mean something else in the appendix.

Seems to matter very little, but ok, we'll call it RACH instead

>   "These are fields that change"

ACK

NACK. It is perfectly legal to fail decompression of CRC3 or CRC7
packets without going to a lower state. The only reason for dropping
back is when "context damage detected".

> This is not true for CO repair.

Exception for co_repair added

> This is not true for CO repair.

Exception for co_repair added

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

ACK

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

ACK, text updated. But we do not want to quote the text from 768 which
is
a bit hard to interpret without a lot more text from that RFC

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

ROHC-TCP carryover, changed numbered text to:
      2) When the list is unchanged, a compressed header that does not
      contain information about the list can be used.
      3) When the list changes, a compressed list is sent in the
      compressed header, including a representation of its structure and
      order.  Previously unknown items are sent uncompressed in the
      list, while previously known items are only represented by an
      index pointing to the item stored in the context.

> There is a parantheses missing in the ceiling calculation.

ACK

NACK, the MSN is handled differently than other fields, how it is
set for these profiles is handled by the english text, the fact that
it is incremented by one is neither included in the FN code.

> The global control field ip_id_behavior_innermost should be 2 bits

ACK

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

NACK, all the global control fields (except MSN) are such that:
a) They are "user-selected" for each input packet
b) They are calculated from another field

So there is no case that I can see where a default would have any
impact in the spec

> method. This problem affects all profiles and several

Changed dont_fragment to have the following UNCOMPRESSED section.

  UNCOMPRESSED {
    df [ 0, 1 ];
  }

Should fix the entire issue.

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

ACK

> ENFORCE(is_innermost == 0);

ACK

> The control field checksum_used should have static encoding

NACK, this field is always bound so a default has no meaning.

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

ACK

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

ACK to global field. NACK to default binding, see earlier comment

> Missing ENFORCE(profile_value == PROFILE_RTP_0107)

ACK

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

NACK, it is an error to have both tss=1 and tsc=1 and therefore no
format makes this a legal binding with the current ENFORCE.
We had to clarify this in 4815 for ROHCv1, and I'm not falling into
that trap any more :)

> This method should take ip_version as argument.

ACK

> "calling" method expects. 

ACK

> ip_version should not be an argument.

ACK

Ok, doesn't matter to me, so I can change it. 

> inferred_sequential_id_id, alternatively this could be done by

ACK

ACK

ACK

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

Ok, doesn't matter

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

Same as for 0x0101

> inferred_sequential_id_id, alternatively this could be done by

ACK

ACK

We will include the changes we have agreed to and post a new version on 
monday unless someone strongly disagrees with some thing. I believe the 
only change in the bits-on-the-wire is the move of the marker/ts field
requested for implementation simplicitly, the rest of the changes
are just clarifications/type fixes so although many small fixes, I the
technical impact is low.

Anyway, thanks again for a very detailed review, which helped a lot with
shaping up the remaining nits in the draft.

BR,
  Kristofer & Ghyslain

---

From: Jani Juvan <[email-masked]>
Date: Fri, 25 Jan 2008 13:49:22 +0100

Hi,

We agree and accept to all your changes with these exceptions (rest of
the text is cut out to limit the size):

Kristofer Sandlund wrote:

====================
Errors
====================

Section 5.2.1:

We were looking for an arrow from FC to FC for !CRC3 and !CRC7. But
feel free to omit it anyway.

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

ACK

The sentence following that correction still has a reference to "these
flags" in the draft of the new draft.

We also noticed that the description of the Dynamic chain in the same
section incorrectly states: "The dynamic chain is only used in the IR
header format."

The formal notation part
====================

Does this really follow formal notation? English text can be used to
describe encoding methods and parameters, but just change values of
control fields, really? Shouldn't it have been a parameter? Why not do
this for more control fields then?

All global control fields should have static encoding by
default. This
to ensure that they can keep their previous values between packets.

As an example: At the decompressor the reorder_ratio_value parameter
for the baseheader is undefined, the control field is also undefined
and thus msn_lsb fails for some compressed header formats (not
co_repair). In our understanding you don't remember values between
packets unless they are static encoded.

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

UNCOMPRESSED {
df [ 0, 1 ];
}

The same change needs to be done to "callers" of dont_fragment. For
instance "profile_1_7_flags1_enc" still returns DF as 1 bit... same
applies for profile_2_3_4_flags_enc and profile_8_flags_enc.

udp()
The control field checksum_used should have static encoding
by default.

It is not bound for the decompressor for CO headers (except co_repair)
when parsing the irregular chain.

Little misunderstanding here, we meant that the tss_flag is irrelevant
as long as tsc_flag is zero. Because that is exactly what the current
ENFORCE says. Either way works.

Regards,

Jani and Anders

---

From: "Kristofer Sandlund" <[email-masked]>
Date: Fri, 25 Jan 2008 16:31:08 +0100

Hi,

further discussion inline.

Jani Juvan <mailto:[email-masked]> wrote on den 25 januari 2008
13:49 :

> Hi,
> 
> We agree and accept to all your changes with these exceptions
> (rest of the text is cut out to limit the size):
> 
> We were looking for an arrow from FC to FC for !CRC3 and
> !CRC7. But feel free to omit it anyway.
> 

We will omit it. We feel the diagram is ok as it is.

> The sentence following that correction still has a reference
> to "these flags" in the draft of the new draft.
> We also noticed that the description of the Dynamic chain in
> the same section incorrectly states: "The dynamic chain is
> only used in the IR header format."
> 

ACK, fixed the text.

> Does this really follow formal notation? English text can be
> used to describe encoding methods and parameters, but just
> change values of control fields, really? Shouldn't it have
> been a parameter? Why not do this for more control fields then?
> 

In section 6.3, there is normative text for non-RTP profiles about
how the MSN behaves. For RTP profiles, teh FN is clear about 
behavior. We feel that the specification is then formally and 
normatively complete and correct with respect to the handling of 
the MSN. So our preferred resolution here would be to do nothing,
it is also the simplest alternative for the specification itself.

default.
> 		This to ensure that they can keep their previous
> values between packets.
> 
> 
> 
> As an example: At the decompressor the reorder_ratio_value
> parameter for the baseheader is undefined, the control field
> is also undefined and thus msn_lsb fails for some compressed
> header formats (not co_repair). In our understanding you
> don't remember values between packets unless they are static encoded.
> 

Not sure this is necessary, but we can add a DEFAULT
section on the global scope. However, this is not supported by the 
FN in RFC 4997, so we would have to add a definition that explains 
this additional FN feature. Doing anything else would make the FN 
code become incredibly ugly, and we think that this is unnecessary.

> 		method. This problem affects all profiles and several
encoding
> methods. 
> The same change needs to be done to "callers" of
> dont_fragment. For instance "profile_1_7_flags1_enc" still
> returns DF as 1 bit... same applies for
> profile_2_3_4_flags_enc and profile_8_flags_enc.

NACK. These encodings only "return" the CLENGTH of 1 bit, which is
what we want, it never re-binds ULENGTH to 1 bit for IPv6.
No change.

> It is not bound for the decompressor for CO headers (except
> co_repair) when parsing the irregular chain.

NACK. The irregular chain binds the UVALUE with the ENFORCE, and the
ULENGTH gets bound in the CONTROL section, so everything is bound
by the current code. No change.

> Little misunderstanding here, we meant that the tss_flag is
> irrelevant as long as tsc_flag is zero. Because that is
> exactly what the current ENFORCE says. Either way works.

No change it is, then.

BR,
    Kristofer & Ghyslain