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

From: "Kristofer Sandlund" <[email-masked]>
Date: Thu, 1 Nov 2007 16:24:29 +0100

Hi Calle,

thanks for the review. Answers inline.

/Kristofer & Ghyslain

Carl Knutsson <mailto:[email-masked]> wrote on den 30 oktober 2007 16:39 :

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

Well, no. See the answer for comment 6) below.

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

co_repair is a CO packet, so I think the current text is correct.
No change.

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

Also, it is expanded on first use, so I think it is fine. No change.

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

I think the current text is ok and prefer that to changing it.

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

We suggest the following resolution instead:
      "Its value is ++typically++ bound to the
      number of LSB compressed bits in the compressed header format, and
      ++thus++ grows with the number of bits transmitted."

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

The definion of "Seq Early" says that an early packet is treated the
same as a loss, so with that definion left in the draft, it should
cover your request here.

Where did comment 7) go? ;)

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

See resolution proposed to Rohit's comment.

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

Don't agree. For example, profiles 0x0101 0x0103 and 0x0107 cannot
_reliably_ know the difference between on-link reordering and prelink 
reordering based on the MSN, so the fact that you can sometimes do
this is more of a side-effect than it is a "function" of the MSN.
No change.

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

We'd prefer to avoid forward references, and as you point out, it is
already listed in terminology => no change

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

ok, we need to do some fix here, but in your above suggestion,
what do you mean by "BYTE_SWAPPED"? Do you mean to add some encoding
method in FN, do you mean to define it elsewhere in the engligh text
or something else?
Please elaborate on your proposal.

> 
> 12) In section 6.6.13:
> 
>     Last sentence, second paragraph:
> 
>     "corectly reconstruct the CC field."
> 
>     should be
> 
>     "correctly reconstruct the CC field.

ok

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

ok

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

I don't think this needs to be done if we do the change you propse
in 18) and 19) => no change

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

Ok, I think I see what you're getting at.
We've added a table mapping profile names to encoding method
names.

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

Ok. Same as above, we've added a table mapping header types to
FN names.

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

I think the text in 6.5 already handles this when it talks about
something like "one for each compressible header" => No change.

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

Hmm... 
Ok, I guess we can do that, it adds a bunch of extra code, but I
guess it may improve readability.

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

same as 18. Fixed.

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

Ok, fixed

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

Good catch, this was really old leftovers. Due to this, I also
updated the discriminators slightly. Fixed

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

I don't agree that this is anything that *fails*, but if you think
it improves readability, I can add this. Fixed

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

I don't see what gain this would give, so I keep it as-is.

/Kristofer

> Cheers,
> /Calle "bättrevetaren"

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

---

From: Carl Knutsson <[email-masked]>
Date: Thu, 01 Nov 2007 18:03:29 +0100

Hi Kristofer,

Comments inline..

> co_repair is a CO packet, so I think the current text is correct.
Agreed
> Also, it is expanded on first use, so I think it is fine. No change.
Agreed
> I think the current text is ok and prefer that to changing it.
If everyone agrees, I think it is ok to keep it.
>       ++thus++ grows with the number of bits transmitted."
I am fine with this text.
> same as a loss, so with that definion left in the draft, it should
I am not sure about this. The section  is called "tradeoff between
reordering and loss"  and the text above gives the impression that
increasing the interpretation offset will make it better to handle
reordering better. It does, but only to the point where the offset is
half the interval. At least  if sequentially late packet are allowed to
 update the context.

I just want to make the working group aware of this. If someone
has an opinion, speak up or the text will stay as it is.
> Where did comment 7) go? ;)
till fanders på ren svenska...:)
> See resolution proposed to Rohit's comment.
Agreed ...and sorry about bringing it up again!
> this is more of a side-effect than it is a "function" of the MSN.
Ok, I can agree with the text..
> already listed in terminology => no change
Agreed.
> Please elaborate on your proposal.
I will get back to you with a better suggestion...
> We've added a table mapping profile names to encoding method
I table will do, but I just wanted to map the base header
field in BN to any of the *_baseheader encoding method. 

..pick any *_baseheader of choice to encode the base
header field in CO header.

The *_baseheader encoding methods in the FN part, will
fail if used with the wrong profile.
   
> Ok. Same as above, we've added a table mapping header types to
Agreed.
> something like "one for each compressible header" => No change.
Ok, I give in :). I have read again and I can agree with the current text.
> guess it may improve readability.
I think it will improve readabillity...to avoid the slightest possibillity
of other interpretations then the one that we have consensus
for in the working group.
>>    rtp_base_header. Otherwise co_common must be sent after initial IR
Agreed.
>>    add a 1 bit marker field in this format.
Thanks
> updated the discriminators slightly. Fixed
:)
> I don't see what gain this would give, so I keep it as-is.
I guess it is a matter of taste.. It will not change the
encoding method at all.

It looked like you used it as a coding convention to
only have fields that can be set by the compressed
formats in the DEFAULT block. And have field encoded
for all compressed formats in UNCOMPRESSED.
That is why I commented it...

It is correct FN code, so I have no problem with keeping
it as-is.

/Calle

---

From: Carl Knutsson <[email-masked]>
Date: Wed, 14 Nov 2007 11:41:10 +0100

Hi Kristofer, Ghyslain,

Comments for 6.6.12 inline...

>> Please elaborate on your proposal.

Instead of changing the formula (my first suggestion), I suggest adding
 the following text last in the paragraph.

"For swapped IP-ID behavior, "previous_IP_ID_value" and IP-ID
 are byte-swapped with regard to the corresponding fields in the
 context."

Cheers,

/Calle

---

From: "Kristofer Sandlund" <[email-masked]>
Date: Wed, 14 Nov 2007 13:18:32 +0100

Carl Knutsson <mailto:[email-masked]> wrote on den 14 november
2007 11:41 :

>>>>    "Therefore, the new value for the IP-ID is calculated according
>>>>    to the following formula:
>>>>    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:
>>>> 
>>>>    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."
>>>> 
>>> method in FN, do you mean to define it elsewhere in the engligh
>>> text or something else? Please elaborate on your proposal.
> 
> Instead of changing the formula (my first suggestion), I suggest
>  adding the following text last in the paragraph.
> 

Ok, good suggestion, slight modification to tie it to the FN a bit more:
       For byteswapped IP-ID behavior (i.e. when
ip_id_behavior_innermost 
       is set to IP_ID_BEHAVIOR_SEQUENTIAL_SWAPPED), 
       "reference_IP_ID_value" and "IP-ID" are byte-swapped with regard
       to the corresponding fields in the context.

I'll add this to the draft.

/Kristofer

---

From: Carl Knutsson <[email-masked]>
Date: Wed, 14 Nov 2007 14:49:49 +0100

> Carl Knutsson <mailto:[email-masked]> wrote on den 14 november

Agreed. Your modification makes it better.

Cheers,

/Calle (medl. i klubben-för-inbördes-beundran:)

---

From: Carl Knutsson <[email-masked]>
Date: Wed, 14 Nov 2007 15:37:31 +0100

Hi rohcers,

The last-call for draft-ietf-rohc-rfc3095bis-profiles-03.txt has ended.
I want to thank all reviewers that sent feedback to the rohc-list during
the last-call. A special thanks to the committed document reviewers,
Haipeng Jin, Mark West and Robert Finking for their reviews.

We will continue with the discussion of the remaining issues. When WG
consensus is reached, the authors will present an updated version
(-03.txt).

After the submit of new version, we will either have a short review
period or another x week Last-Call with CDRs, depending on the amount of
changes in the new version.

Keep up the good work!

Cheers,

/Carl Knutsson (ROHC WG chair)