Subject: [rohc] Followup on CDR comments

From: Carl Knutsson <[email-masked]>
Date: Fri, 21 Dec 2007 10:52:00 +0100

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