Subject: [rohc] RE: Followup on CDR comments

From: "Kristofer Sandlund" <[email-masked]>
Date: Fri, 11 Jan 2008 09:56:34 +0100

Hi Calle,

sorry for the delayed answer.

I can agree that the definition of the external parameters could be
a bit clearer, so here's a bunch of suggested clarifications, all
related to the external parameters in one way or the other.
The basic idea is that I will add global control fields for most
the these parameters which they bind to in order to make them act more
consistently, so that all of them have the behavior that:
"on the compressor side, the parameter is set to a user-selected value
on input and gets bound to a control field or something else inside
the FN code. on the decompressor side, the value is set to undefined
by the user and gets bound by the FN code"
Here's a list of proposed changes:

(A) There was some leftover naming of the ip_id-related parameters,
which I fixed. Also a couple of naming convention oddities with
_flag and_indicator were fixed

(B) For is_innermost,
- Added a CONTROL field for ipv4 and ipv6 
- Bind this field in the static or dynamic chain. For clarity, I also
  added adding an explicit flag in the static chains for v4 and v6. We 
  already have one in the _endpoint version and in the other versions,
  we anyway have reserved bits that can be used to bind the parameter. 
  This is bits-on-the-wire change, but since it is just a reserved bit
  there's no cost, and it helps simplifying the FN code quite a bit.
- Clarify the text definition of this one to clearly point out that
  it is the innermost of the _compressible_ IP headers (in the case of
  IP-only profile this removes ambiguity)

(C) Made 'profile' into a global control field and just bind it in
basically every header. This one has a slight difference to the other
fields since for the static chain, the paraemter should be set on
the decomressor to the value from the IR packet (so this will be noted
in the text). This would not be a bit-change, only code change.
Also added text for this in the definition of the parameter:
     When processing the static chain at the
     decompressor, this parameter is set to the value of the profile
     field in the IR header

(D) Updated the text def for the external parameters by adding
the following:
     The following applies for all parameters listed below: 
     At the compressor, the value of the parameter is set according
     to the recommendations for each parameter. At the decompressor,
     the value of the parameter is set to undefined and will get bound
     by encoding methods, except where otehrwise noted.
The last part is to allow for a clarification for the profile parameter
when processing the static chain (as listed above)

Also clarified the text on outer_ip_flag slightly

I will submit a rev -04 of the draft with these changes later today
unless there are objections so that one can be reviewed before the
deadline next friday.

BR,
  Kristofer

Carl Knutsson <mailto:[email-masked]> wrote on den 21 december 2007 10:52 :

> Hi Kristofer, Ghyslain
> 
> All my comments have been updated according to what was agreed, except
> issue 14). After inspecting the solution for in version *-03 I still
> consider 14) being an unsolved issue. Making control fields global
> (issue 18, 19) will not explain how the parameters should be set to
> describe compression and decompression.
> 
> Many of the encoding method arguments represent
> implementation specific
> choice for the compressor. These values or parameters must treated
> differently in the decompressor compared to the compressor. Sometimes
> they must be set to <undefined>, for the encoding method to correctly
> describe the decompression. Otherwise the some of the
> compressed formats
> will fail because of the ENFORCE statements in the
> *baseheader encoding
> method. In some cases they must be set differently depending
> on the type
> of chain item it is parsing (decompressing).
> 
> Example:
> 
> If 'reorder_ratio_value' in rtp_baseheader must be "undefined" to
> correctly describe decompression of the co_common packet(it
> will be set
> by the encoding method itself).
> 
>     "reorder_ratio_value:
> 
>          Set to a 2-bit integer value, using one of the
> constants whose
>          name begins with the prefix REORDERING_ and as defined in   
> Section 6.8.2.4." 
> 
> The text above only describes this from the compressor point
> of view. If
>  you follow the text above. If the 'reorder_ratio_value' is SET at all
> the co_common format will the fail if it reorder_ratio field
> is present
> and containing a different UVALUE.
> 
> CONTROL {
>  ENFORCE(reorder_ratio.UVALUE == reorder_ratio_value); }
> 
> The following parameters are implementation specific choice in the
> compressor and should be or set to undefined in the
> decompressor or set
> by the encoding method itself:
> 
> For encoding method *baseheader:
> o reordering_ratio
> o ip_id_behavior
> o coverage_behavior_value
> o ts_stride_value
> o time_stride_value
> o outer_ip_flag
> 
> All these parameters represent a compressor choice.
> 
> How the parameter "ip_id_behavior" is set in the decompressor depends
> whether irregular chain or dynamic chain is decompressed. If irregular
> chain is decompressed, this value must be set. That is, if the
> decompressor should be able to parse the irregular chain (IN). For
> dynamic chain it will be given by the compressed format (OUT) (to be
> used later in irregular chain). This is also the case for
> 'is_innermost'. When parsing the static chain for an IR packet the
> innermost header is unknown and will be "set" by the encoding method
> itself. However, in the irregular chain "is_innermost" most
> be know when
> chain is parsed.
> 
> parameter "profile" is ok!
> 
> Do you agree? Any comments?
> 
> 
> Cheers,
> 
> /Calle
> 
> -------8<--------------8<--------------8<--------------8<-----
> ---------8<-------
>>> 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
> -------8<--------------8<--------------8<--------------8<-----
> ---------8<-------
>>> 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.
> 
> -------8<--------------8<--------------8<--------------8<-----
> ---------8<-------
> 
> 
> Carl Knutsson wrote:
>> 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)
>> 
>> Carl Knutsson wrote:
>>> Kristofer Sandlund wrote:
>>>> Carl Knutsson <mailto:[email-masked]> wrote on den 14
>>>> november 2007 11:41 : 
>>>> 
>>>>> Hi Kristofer, Ghyslain,
>>>>> 
>>>>> Comments for 6.6.12 inline...
>>>>> 
>>>>>>>> 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. 
>>>>> 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
>>>> 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.
>>> Agreed. Your modification makes it better.
>>> 
>>> Cheers,
>>> 
>>> /Calle (medl. i klubben-för-inbördes-beundran:)
>>> 
>>> _______________________________________________
>>> Rohc mailing list
>>> [email-masked]
>>> https://www1.ietf.org/mailman/listinfo/rohc

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

---

From: Carl Knutsson <[email-masked]>
Date: Fri, 11 Jan 2008 11:11:54 +0100

Hi Kristofer,

I agree with you suggestion. With these changes I consider my issue 14)
to be solved. When the new draft is published I will look into the details.

/Calle