Subject: [rohc] CDR review for draft-ietf-rohc-rfc3095bis-rohcv2-profiles-02.txt

From: Carl Knutsson <[email-masked]>
Date: Tue, 30 Oct 2007 16:39:29 +0100

Hi Kristofer, Ghyslain, rohcers,

I would like to discuss or see changes for the following issues:

1) In section "2. Terminology", "Sequentially Early packet" is defined but
   never used. Remove?

2) In section "2. Terminology", under ROHCv2 Packet Types: Do we have
   two or three different packet types? Is co_repair a different
   packet type.

3) MSN is used several times in before it is described in 6.3.1. It is
   however described in the framework, so it may be ok.
 
4) In section 4.2, under "IP Extension headers":  

    "under the life time of a context (CID)"

    Should (CID) really be there? It is an abbreviation for context
identifier.
    I suggest to change it to either "under the life time of a context"
    or "under the life time of a context (or a CID)"
    "context (CID)" is used in many places in the draft.
    
5) In section 5.1.2, under LSB encoded fields:

   "The interpretation interval offset... ..Its value is bound to the
    number of LSB compressed bits in the compressed header format, and
    grows with the number of bits transmitted."

    The offset doesn't necessarily grow with the number of bits.. If
    REORDERING_NONE is set for msn.

    I suggest rephrasing it to something like "typically grows" or
"normally grows"

6) In section 5.1.2 under the interpretation interval:

   "where (2^k-1) - p is the maximum positive delta, corresponding to
    the maximum number of consecutive losses for which the lsb
    encoding can recover the original value of the field"

   The ability to handle sequentially early packets is also affected
   by a positively affected by having a large positive delta.

8) Section 5.2.2:
  "All header formats carry a CRC and are context updating."
   Sequentially late packet are not context updating
   
9) Section 6.3.1

   "The MSN field has the following two functions:

   o  Differentiating between reference headers when receiving feedback
      data.
   o  Inferring the value of incrementing fields (e.g.  IPv4
      Identifier)."
   
  Detecting sequentially late packets is another one...  

10) Section 6.3.3: irregular chain item is used for the first
   time(except for in the terminology section under chaining)
   Maybe we should consider adding a reference to section 6.5.

11) In Section 6.6.12:

   "Therefore, the new value for the IP-ID is calculated according to the
   following formula:
      IP-ID = delta_msn + reference_IP_ID_value
   Where "delta_msn" is the difference is MSN between the reference
   value of MSN in the context and the value of the MSN decompressed
   from this packet, "previous_IP_ID_value" is the value of the IP-ID in
   the context."

   The formula above doesn't take work for swapped IP-ID. I suggest a
   change to something like this:

   "Therefore, the new value for the IP-ID is calculated according to the
   following formula:
     
      IP-ID = delta_msn + reference_IP_ID_value, for sequential IP-ID and,
      IP-ID = BYTE_SWAPPED(delta_msn + BYTE_SWAPPED(reference_IP_ID_value)),
      for sequential byte-swapped IP-ID.

   Where "delta_msn" is the difference is MSN between the reference
   value of MSN in the context and the value of the MSN decompressed
   from this packet, "previous_IP_ID_value" is the value of the IP-ID in
   the context."

12) In section 6.6.13:

    Last sentence, second paragraph:
    
    "corectly reconstruct the CC field."

    should be

    "correctly reconstruct the CC field.

13) In section 6.6.13.1, last paragraph, last sentence:

          "Previously unknown items are sent uncompressed in the list,
      while previously known items are only represented by an index
      pointing to the context."

      shouldn't it be something like:

      "pointing to the item stored in the context"  

14) Section 6.7

  I would like the arguments to be specified separately for compressor
  and decompressor. Most field is only written for compression some
arguments to encoding methods should be "set" differently depending on
if it is used by the compressor or the decompressor. The following
fields should have this separation:
 
  o outer_ip_flag for *_baseheader
    Should be undefined from the decompressor point of view.

  o ts_stride_value & time_stride_value in rtp and
     {rtp,udplite_rtp}_basheader. I beleive these should changed to
     control fields. See below point XXX.
      
 15) The "glue" between CO packet formats and FN is missing. There is
   nothing that connects rtp_baseheader as the FN encoding method for
   the base header field in the CO packet.  Some like this could be
   added to 6.8.2.3 or 6.8.2.4

    "The base header field can be encoded with the "*_base_header"
     encoding methods specified in section 6.8.2.4."

   The 'profile' argument is used to discriminate between profiles.    

16) Similar to problem as in 14) for IR packet. There is nothing that
maps the
   specific encoding method to the to the actual extension header
   (other than the name in a comment in the FN).

   I suggest adding either adding a mapping for the headers to the
   encoding method in section "6.5 Compressed header chains" or chaining
   the FN and add descriptions in section 6.7. I am fine with both.

   If changes are done in the FN and Section 6.7. A parameter
   'protocol' to the following enkoding functions: ipv4, ipv6, ah,
   mine, espnull, esp, udp, udplite, rtp ipv6_dest_hop etc. 'protocol'
   is taken from the the previous header's next_header/protocol field.

17) I would like to see an addition to draft were it specifies that the
   static chain must end with the header specified by the profile. I
   have not been able to find any such description in the draft.

18) In section "6.8.2.4 Header Formats in ROHC-FN", it would be a good
   idea to make the innermost ip_id_behavior control field global. So
   it will be in scope for both by encoding methods 'ipv4' and
   '*_baseheader'. Otherwise you would always have to send co_common
   after the initial IR packet for IPv4.

19) Maybe we should consider making ts_offset, ts_stride and
   time_stride global. To be in scope for both rtp() and rtp_base_header.
   Otherwise co_common must be sent after initial IR sequence.

20) I would like to see a marker field in both occurrences of
   pt_2_seq_both. I suggest changing ts_scaled from 8 to 7 bits and
   add a 1 bit marker field in this format.

21) pt_2_rnd is redundant (pt_0_crc7 can be used instead.) Remove
   compressed format from in encoding methods esp_baseheader,
   udp_baseheader, iponly_baseheader, udplite_baseheader.
   
22) Page 54, in encoding method 'gre':

   The format gre_static will always fail. The field 'sequence number'
   and 'checksum_and_res' the UNCOMPRESSED format have undefined/wrong
   ULENGTH attributes. I am not sure how the [0, 32] makes bindings
   (Either it is undefined or it could bind the wrong values to the
   THIS.ULENGTH).

   Add to COMPRESSED gre_static:
   ENFORCE((c_flag.UVALUE == 1 && checksum_and_res.ULENGTH == 32)
           || checksum_and_res.ULENGTH == 0);
   ENFORCE((s_flag.UVALUE == 1 && sequence_number.ULENGTH == 32)
           || sequence_number.ULENGTH == 0);

 23) Page 56, in encoding method 'mine':
    checksum =:= inferred_mine_header_checksum
    can be move to UNCOMPRESSED instead of DEFAULT.

Cheers,

/Calle "bättrevetaren"

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