Re: Followup on CDR comments

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

Kristofer Sandlund wrote:
> 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 protected]> 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 protected]> 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 protected]
>>>> https://www1.ietf.org/mailman/listinfo/rohc