Subject: [rohc] rohc v2 cdr review

From: "Mark West" <[email-masked]>
Date: Fri, 2 Nov 2007 00:18:38 +0000

Hi all,

Ok, I finally got through the whole draft ;-) -- which I think is in
pretty good shape.  Personally, I think that this is a considerable
improvement on RFC 3095.  Anyway, such comments as I have are collated
below...

Cheers,

Mark.

General comment:

Having read this spec., I ended up feeling that I had seen quite a few
lower-case 'may', 'should', etc, terms and wondering whether or not
they should be in 'standards' upper-case form.  I've tried to pick up
the ones that jumped out most strongly at me, but I'm sure that there
were others...  (For example, the section on Feedback formats, which
is largely inherited from RFC 3095 is littered with SHOULDs and MUSTs;
but they're really rather sparse in the rest of the document.)

Specific comments:

2. Terminology
Chaining of Items

* chain an item for <<e.g.>> each header -- don't understand 'e.g.' in
this context

5.1.2. Tradeoff betwen robustness to losses and to reordering

* You can choose number of LSBs to cope with re-ordering, but it isn't
clear how you handle sequentially late 'static' fields?

5.2.1.2 Repair Context (RC) State

* Should you really allow an upward transition on any success -- by
the time you've tried 14 packets with CRC7, you've reduced its
error-detection strength to around that of CRC-3...

Feedback logic:

* "static context damage is assumed" -- would be good to give some
indication of what might cause such an assumption (I assume some
number of decompression failures?)

5.2.2. Decompressor Context Management

* "should be treated as a decompression failure when the decompressor
asserts the validity of its context." -- not sure I understand this!

* Static Context Damage Detection -- some guidance on this might be
useful?  (I realise that the aim is to simplify, but think that this
might be a bit too far in the other direction)

* When RoHCv2 profiles are used over a channel that cannot guarantee
in-order deliver, the decompressor <<may>> refrain -- 'MAY'?
'SHOULD'?

5.2.3. Feedback logic

* "the decompressor may refrain to send" -> "the decompressor may
refrain from sending"

* "the decompressor <<should>> limit" -- 'SHOULD'?

* "and <<should>> avoid sending unnecessary" -- 'SHOULD'?

6.2. Profile Operations

* are there any issues raised by using mixed v1/v2 profiles?  (I
assume not -- believe that this is implied by CID re-use description;
may not hurt to clarify)

6.3.1. MSN

* "The compressor should only initialise a new MSN for the initial IR
sent for a CID that corresponds to a context that is not already
associated with this profile" -- I think that I can parse that, but it
could perhaps be re-phrased to be a bit clearer?

* "it is initialised to a random value" -- 'MUST'?

* "is incremented by one" -- 'MUST'?

6.3.4. UDP-Lite Coverage Behavior

* "one of he" -> "one of the"

6.3.7. CRC-3 for Control Fields

* "the decompressor <<may>> still forward" -- 'MAY'?

6.6.8. Scaled RTP Timestamp Encoding

* "value different than" -> "value different from"

Again, feels like there should be more standards language in this
section (e.g. compresor cannot use scaled timestamp formats ->
compressor MUST NOT?)

6.6.8. timer_based_lsb

* Is it worth explicitly pointing out that a decompressor not
supporting TB compression SHOULD send CLOCK_RESOLUTION if it receipves
non-zero time_stride, but MUST send a resolution of 0.   (Or
something?)

6.6.11. control_crc3_encoding

* First use of 'binds' (w.r.t. formal notation)?  Is it worth
cross-referencing to the FN spec?

Again feels like I would expect more 'standards language', for example
in specifying the order of the fields?

6.6.12. inferred_sequential_ip_id

* Confusion between "reference_ip_id_value" and "previous_ip_id_value"?

6.6.13.2 Table-base Item Compression

* "The compressor may..." -- "MAY"?

6.6.13.3.

* "X: Indicates whether the item present" -- "X: Indicates whether the
item is present"

6.8.2.1. Design rationale

co_common

* "can not" -> "cannot"

co_repair

"fields are transmit" -> "fields are transmitted"

"format and contains" -> "format and it contains"

6.8.2.3.

esp_header (I think) has a 'discriminato' rather than a 'discriminator' in it...

6.9.1. Feedback Formats

* What's the difference between 'verified' and 'validated'??!

Appendix B.2  W-LSB

"compressor selects packet format" -> "compressor selects a packet format"

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

---

From: "Kristofer Sandlund" <[email-masked]>
Date: Fri, 2 Nov 2007 13:29:51 +0100

Hi Mark,

thanks for the review!

Answers inline.

Mark West <mailto:[email-masked]> wrote on den 2 november 2007
01:19 :

> improvement on RFC 3095.  Anyway, such comments as I have are
> collated below... 
> 
> Cheers,
> 
> Mark.
> 
> 
> General comment:
> 

We've tried to not overuse 2119-language, and limit it to where it:
* has some impact on interoperability 
  AND 
* there is a possibility that an implementation actually can have a 
  choice (i.e. no MUST on things that are just possible to do one way)

But it's always a difficult tradeoff.

> 
> 
> Specific comments:
> 
> 
> 2. Terminology
> Chaining of Items
> 
> * chain an item for <<e.g.>> each header -- don't understand 'e.g.'
> in this context 

Ok, this is ROHC-TCP cut&paste leftovers, where (as you know), we had 
irregular chains items for TCP options. I agree it does not make
any sense in this spec. Will remove the 'e.g.'

> clear how you handle sequentially late 'static' fields?

In this section, there's this paragraph "Seldom Chainging Fields", which
I think is what you're asking for (i.e. OA all the way).
Do you want some additional text?

> error-detection strength to around that of CRC-3...

I see what you're saying but:
- A compressor that has sent 14 CRC-7 packets (and to send 14 CRC-7
packets should normally mean that a lot of CRC-3 packets have come
in-between) and has not done a proper repair doesn't really seem to be
doing its job correctly (if unidir it should send a repetition during
this period, and if bidir, it should have gotten feedback)
- Even if this happens, you are likely to pretty soon drop back to RC
state again if your context is still invalid? So it's probably only a 
matter of one packet being pushed to upper layers
- The section is 'iInformative, so this is just guidance on how to
implement a decompressor, so there's nothing stopping you from being
more conservative in an implementation
- Do you have any suggestion for how we could define it differently?

> number of decompression failures?)

It is defined in 5.2.2 (there's a forward reference under the state
machine figure). Sufficient?

> asserts the validity of its context." -- not sure I understand this!

Not sure I understand what you don't understand :)
The intent is to say that such a CRC failure should be counted
as a 'regular CRC failure' when considering if one should drop to RC,
so there's nothing special about this. Do you have any improvement
suggestion.

> might be a bit too far in the other direction)

I would prefer to keep it as-is, but if you have some suggestions 
for what you would like it to say, we can add them

> in-order deliver, the decompressor <<may>> refrain -- 'MAY'? 'SHOULD'?
> 

The whole of section 5 is informative, so there should be no 2119
language here.

> * "the decompressor may refrain to send" -> "the decompressor may

ok
 
> * "the decompressor <<should>> limit" -- 'SHOULD'?

informative section

> * "and <<should>> avoid sending unnecessary" -- 'SHOULD'?

informative section

> assume not -- believe that this is implied by CID re-use description;

As far as I remember, there should not be any issues with using (for
example) 0x0101 and 0x0002 on the same channel.
Maybe we can add some text, I need to think about this a bit more.

> could perhaps be re-phrased to be a bit clearer?

Can you suggest some text that would make it clearer for you? I can
parse that text, so I'm not sure how to make it clearer.

> * "it is initialised to a random value" -- 'MUST'?

This would not break interoperability, so what good would a MUST do?
Maybe a SHOULD, though. I'll check what Ghyslain thinks.

> * "is incremented by one" -- 'MUST'?

Possibly. Again, it does not break interoperability, but breaking this
reduce compression efficiency, so maybe a SHOULD is is order.
But I think I prefer the non-2119 language....

> * "one of he" -> "one of the"

ok

> * "the decompressor <<may>> still forward" -- 'MAY'?

ok

> * "value different than" -> "value different from"

ok

> section (e.g. compresor cannot use scaled timestamp formats ->

probably ok, need to think about this.

> non-zero time_stride, but MUST send a resolution of 0.   (Or

The following is from 6.6.9, and doesn't that say what you're
asking for already?

                                                                  The
   decompressor SHOULD send the CLOCK_RESOLUTION option if it receives a
   non-zero time_stride value and it has not previously informed the
   compressor that it supports timer-based compression using the
   CLOCK_RESOLUTION option with a non-zero value.

> cross-referencing to the FN spec?

Since the FN is a normative reference, I think the use of 'binds'
should be fine.

> in specifying the order of the fields?

So, in this case, there's nothing that can be chosen at all, this is
just a statement of how things actually work, so I don't think 2119
language is appropriate. But maybe we just have different views on
how to best utilize 2119?

> * Confusion between "reference_ip_id_value" and

ok

> * "The compressor may..." -- "MAY"?

ok, that seems appropriate

> 
> 6.6.13.3.
> 
> * "X: Indicates whether the item present" -- "X: Indicates whether
> the item is present" 

ok

> 6.8.2.1. Design rationale

ok

> "fields are transmit" -> "fields are transmitted"

Ahh... but this is my signature :) 
This is something I constantly do wrong, in every spec or report I
ever write, damnit!
Will fix.

> "format and contains" -> "format and it contains"

ok

> esp_header (I think) has a 'discriminato' rather than a

ok

> * What's the difference between 'verified' and 'validated'??!

It took you all the way to 6.9.1 to notice this? :)
These terms are used throughout the entire document, and they
are defined in RFC4995. So I think it is fine as it is.

> "compressor selects packet format" -> "compressor selects a

ok

/Kristofer

---

From: "Mark West" <[email-masked]>
Date: Mon, 12 Nov 2007 21:13:58 +0000

Hi Kristofer,

Some comments on your answers (I've deleted most of the
non-controversial stuff)...

Cheers,

Mark.

-- "the innocent, the beautiful and rohc contributors have no enemy
but time" (with apologies to W.B.Yeats)

> But it's always a difficult tradeoff.

As to your first point, I completely agree (yet probably failed to
keep it in mind all the time when reviewing the draft).  I do agree
with your second point, although it's perhaps harder to see that there
is 'only one choice'.  I'll bear it in mind when answering your
answers!

> Do you want some additional text?

Where a 'static' field is sequentally *late*, it will decode to the
wrong value unless (i) the decompressor stores a history are matches
it against the MSN; or (ii) you simply discard the packet on detection
that it is sequentially late (which could be assumed to be done by the
CRC, but then there is no 'reordering specific' stuff being done).
Basically, 'static' fields are not of themselves completely
'self-correcting', like LSB fields are with the appropriate
interpretation interval.  Or am I missing something?

> - Do you have any suggestion for how we could define it differently?

Ok, that largely makes sense.  And I'm sure that in part it's just me
getting my head around the gulf between the prescriptions in 3095 and
this version...

... my only question would be whether it is useful, as a compressor
implementer, to have some idea of how a decompressor is expected to
behave?  (This may affect the choice of packet formats -- if a
decompressor requires 2 CRC-7s to succeed, the compressor has to
provide them, or the decompressor will keep tossing packets.)  I'm
prepared to accept that I'm worrying about nothing, though.

> machine figure). Sufficient?

Yes

> so there's nothing special about this. Do you have any improvement

It's quite clear on re-reading, so I'm now not sure what I don't
understand... leave it as it is! ;-)

> for what you would like it to say, we can add them

I suppose that I was thinking of the 'k out of n' decompression
failures stuff.  But, on balance, I'm inclined to agree that they're
not necessary.

> The whole of section 5 is informative, so there should be no 2119

Fair point (other similar comments on Section 5 agreed with and deleted!)

> Maybe we can add some text, I need to think about this a bit more.

I'll leave it up to you -- I'm not too fussed either way.

> parse that text, so I'm not sure how to make it clearer.

I'll try -- assuming that I understand exactly what this is trying to say:

"The compressor only initialises the MSN for a context when that
context is first created or when the profile associated with a context
changes"

(see also below, regarding how necessary this is)

> Maybe a SHOULD, though. I'll check what Ghyslain thinks.

Ok, I agree that a 'MUST' is wrong.  But then, if it doesn't affect
anything, why say anything about it at all?!

> But I think I prefer the non-2119 language....

Again, agree that a 'MUST' is wrong.  For my money, it's a finely
balanced call on 'SHOULD' -- given the defiination of SHOULD to "mean
that there  may exist valid reasons in particular circumstances to
ignore a particular item, but the full implications must be understood
and carefully weighed before choosing a different course."  I think
that being clear about the implications on compression efficiency is
useful, but that you are quite right to argue that there is no
interoperability issue.  I'm happy to leave it to your judgement.

> probably ok, need to think about this.

If you are happy to assert that it is *impossible* for the compressor
to send scaled timestamp formats in this case, then you may will be
correct to write it as you have done (by your second observation on my
general comment).  Otherwise, this would obviously (?) be an
interoperability issue..?

>    CLOCK_RESOLUTION option with a non-zero value.

I guess what I'm asking is: should the decompressor respond to a
non-zero time_stride if it does not support timer-based compression,
or should it simply ignore it (assuming that it has sent no
CLOCK_RESOLUTION option already)?  This doesn't seem to be a big deal,
but there seems to be no guidance about which way (send 0, or don't
send anything) is the 'preferred' way to deal with it...

ok

> how to best utilize 2119?

On reflection, I think you're right.  (Maybe I'm just conditioned by
RoHC v1 to expect more standards language...)

> This is something I constantly do wrong, in every spec or report I

Always worth placing a deliberate mistake to see if the reviewers have
actually read it! ;-)

> are defined in RFC4995. So I think it is fine as it is.

You know how slow I can be ;-)

Agree -- fine as it is!

---

From: "Kristofer Sandlund" <[email-masked]>
Date: Tue, 13 Nov 2007 16:11:56 +0100

Hi Mark,

commenting on some final issues.

Mark West <mailto:[email-masked]> wrote on den 12 november
2007 22:14 :

>>> * You can choose number of LSBs to cope with re-ordering, but it
>>> isn't clear how you handle sequentially late 'static' fields?
>> 
>> In this section, there's this paragraph "Seldom Chainging Fields",
>> which I think is what you're asking for (i.e. OA all the way).

You're of course correct in the above, but the following text is in
the upcoming -03 (it was updated based on other comments to the list)

   When ROHCv2 profiles are used over a channel that cannot guarantee
   in-order delivery, the decompressor may refrain from updating its
   context with the content of a sequentially late packet that is
   successfully decompressed.  This is to avoid updating the context
   with information that is older than what the decompressor already has
   in its context.

So that is one way to avoid problems with late packets.
The other thing is the text I referred to:

   Seldom changing fields

      The optimistic approach (Section 5.1.1) provides the upper limit
      for the maximum reordering depth for seldom changing fields.

So, if each static update is repeated for as long as the max reordering
depth, then the last packet of one update sequence is "guaranteed"
to arrive after any packet sent before this update. Although some
of these packets may get discarded, the context will be up-to-date
at the end of the sequence.
(a bit hard to explain, but I hope it came out right)

>> in-between) and has not done a proper repair doesn't really seem to
>> be doing its job correctly (if unidir it should send a repetition
>> during this period, and if bidir, it should have gotten feedback)

I don't see this as an issue, the compressor should always have this
vague "confidence" in all it does and I think that is enough from
the spec. So if a decompressor has these requirements, this
decompressor has to understand that it risks a later repair....
 
>>> 
>>> 6.3.1. MSN
>>> 
>>> * "The compressor should only initialise a new MSN for the initial
>>> IR sent for a CID that corresponds to a context that is not already
>>> associated with this profile" -- I think that I can parse that, but
>>> it could perhaps be re-phrased to be a bit clearer?
>> 
> context is first created or when the profile associated with a
> context changes"

ok, I'm fine with this suggestion. Will update
 
> anything, why say anything about it at all?!

I think this came out of some very old ROHC-TCP discussion about
security and randomized initial sequence numbers and we just ported 
it to these profiles as well. To me, it doesn't really matter, but
it serves as a way to say that "any number will do", and if it serves
some security purpose as well, then I say it can stay.
Might change it to SHOULD though....

>> Possibly. Again, it does not break interoperability, but breaking
>> this reduce compression efficiency, so maybe a SHOULD is is order.

Agreed. Changed it to SHOULD.

>>> supporting TB compression SHOULD send CLOCK_RESOLUTION if it
>>> receipves non-zero time_stride, but MUST send a resolution of 0.  
>>> (Or something?)
>> 
>> The following is from 6.6.9, and doesn't that say what you're asking
>> for already? 
>> 
>> 
>       The
>>    decompressor SHOULD send the CLOCK_RESOLUTION option if it
>>    receives a non-zero time_stride value and it has not previously
>>    informed the compressor that it supports timer-based compression
>>    using the CLOCK_RESOLUTION option with a non-zero value.
>> 
> 

Quoting from the same place as above, but with a bit more text.
My comments inside the text.

   The control field time_stride controls whether or not the
   timer_based_lsb method is used in the CO header.  The decompressor
   SHOULD send the CLOCK_RESOLUTION option if it receives a non-zero
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
So, whether it supports TBC or not, it SHOULD still send
CLOCK_RESOLUTION
(i.e. it should implement CLOCK_RES for value zero unless it has strong
reasons to know this will never be a problem)

   time_stride value and it has not previously informed the compressor
   that it supports timer-based compression using the CLOCK_RESOLUTION
   option with a non-zero value.  Whether the CLOCK_RESOLUTION is set to
                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   a non-zero value or to a zero value is up to the implementation.
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
And what it sets inside the CLOCK_RES is optional, it may be zero and
it may be non-zero, both independent if it has implemented support
for it or not.

So, again, I think the text already handles your question, or otherwise,
I'm being really thick.

All other issues seemed to have been resolved already.
Thanks again for your time reviewing the draft!

/Kristofer

---

From: "Mark West" <[email-masked]>
Date: Wed, 14 Nov 2007 21:26:25 +0000

Hi Kristofer,

> commenting on some final issues.

hopefully -- I think we're down to one minor quibble ;-)

See inline...

Cheers,

Mark.

>       for the maximum reordering depth for seldom changing fields.

Ok, I like this text -- great!

> decompressor has to understand that it risks a later repair....

I agree that the compressor needs 'confidence' in everything,
although I still think that this is in part based on its model of
decompressor behaviour.  So I still have a slight concern about this.
However, you've obviously tried very hard to walk a fine balance and
not over-specify.  I think that's good, too; so overall I'm happy to
leave this as it is.

> Might change it to SHOULD though....

Ok - I'll leave that with you; I'm happy either way...

> > send anything) is the 'preferred' way to deal with it...

I think that the text says:

The decompressor SHOULD send CLOCK_RESOLUTION IF
(1) it receives a non-zero time-stride AND
(2) it has not sent CLOCK_RES with a value != 0

So it does not cover the case where the decompressor receives a
non-zero time-stride and has never sent CLOCK_RES (or has only sent
CLOCK_RES of 0)

Yup, it's really picky!  But some guidance would be nice: "If the
decompressor has never sent a CLOCK_RESOLUTION option and does not
wish to use timer-based compression, it need not send
CLOCK_RESOLUTION" or change the text to say, "if it receives a
non-zero time_stride value and it has not previously sent the
CLOCK_RESOLUTION option".  Or something?

Or are we both missing something?!

---

From: "Kristofer Sandlund" <[email-masked]>
Date: Wed, 21 Nov 2007 12:39:53 +0100

Hi Mark,

w.r.t. the final 'quibble', Haipeng had a similar comment in his
review and Ghyslain proposed a text solution in his mail
earlier today, i.e.:

  The decompressor SHOULD send the CLOCK_RESOLUTION option with a
  zero value, if:
  - it receives a non-zero time_stride value, and
  - it has not previously sent a CLOCK_RESOLUTION feedback with a
    non-zero value. 
  This is to allow compression to recover from the case where a
  compressor erroneously activates timer-based compression.

I think that should satisfy your concern.

/Kristofer

Mark West <mailto:[email-masked]> wrote on den 14 november
2007 22:26 :

>>> CLOCK_RESOLUTION option already)?  This doesn't seem to be a big
>>> deal, but there seems to be no guidance about which way (send 0, or
>>> don't send anything) is the 'preferred' way to deal with it...
>> 
>