RE: final follow-up review of last-call

Hi,

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

Jani Juvan <mailto:[email protected]> 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.
> 
> 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?

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

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

ACK, removed parenthesised text.

> 
> Section 6.8:
> Here the terms "packet types" and "packet formats" are
> defined, but they
> are used arbitrarily throughout the draft.

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"

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

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

> 
> Section 6.9.2.2:
> There is no mention of how an ACK with an ACKNUMBER-NOT-VALID option
> is to be interpreted. 

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.


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

ACK

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

ACK

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

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

> 
> Section 3:
> Should TTL be here?

ACK

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

ACK

> 
> Section 4.3:
>   "Robust header compression can be used over many
> type+++s+++ of link
>    technologies.

Changed wording to:
   Robust header compression can be used over different 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.
> 

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

> 
> Section 6.6.8:
>   "This section provide+++s+++ additional details..."

ACK

> 
> Section 6.6.8:
>   "...use the new scaling factor once it has ---have--- enough  
> confidence..." 

ACK

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

ACK

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

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

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

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

> 
> PATTERN: "These field that change"
> Replace with:
>   "These fields change"
> or:
>   "These are fields that change"

ACK

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

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

> 
> Section 6.5:
>   "For each compressed packet, the irregular chain is appended at.."
> This is not true for CO repair.

Exception for co_repair added

> 
> Section 6.5:
>   "The irregular chain is used in all CO packets"
> This is not true for CO repair.

Exception for co_repair added

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

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

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

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

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.

> Appendix B:
> There is a parantheses missing in the ceiling calculation.
> 

ACK

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

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
> instead of 16. 

ACK

> 
> All global control fields should have static encoding by
> default. This
> 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

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

Changed dont_fragment to have the following UNCOMPRESSED section.

  UNCOMPRESSED {
    df [ 0, 1 ];
  }

Should fix the entire issue.

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

ACK

> 
> COMPRESSED ipv4_outer_dynamic:
> The first ENFORCE statement should only be
> ENFORCE(is_innermost == 0);

ACK

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

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

> 
> COMPRESSED udp_regular_dynamic
> The second ENFORCE statement should be ENFORE(checksum_used.UVALUE =
> (checksum.UVALUE != 0)). 

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

> 
> COMPRESSED udp_lite_regular_dynamic
> Missing ENFORCE(profile_value == PROFILE_RTP_0107)
> 

ACK

> variable_scaled_timestamp()
> for the compressed not_present format the ENFORCE statement should
> 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 :)

> 
> profile_1_7_flags1_enc()
> This method should take ip_version as argument.

ACK

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

ACK

> 
> profile_7_flags2_enc()
> ip_version should not be an argument.
> 

ACK

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

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

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

ACK

> 
> ====================
> = IP ONLY PROFILE
> ====================
> 
> pt_0_crc7 has msn incorrectly encoded as msn_lsb(7), it should be
> msn_lsb(6). 

ACK

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

ACK

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

Ok, doesn't matter

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

Same as for 0x0101

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

ACK

> 
> ====================
> = UDP-LITE PROFILE
> ====================
> 
> pt_0_crc7 has msn incorrectly encoded as msn_lsb(7), it should be
> msn_lsb(6). 

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