[Gen-art] RE: Gen-ART LC review of draft-ietf-rohc-rtp-impl-guide-21.txt

Spencer,

Thanks for this thorough review, responses can be found inline.

Cheers,
/L-E

> There are several places in the draft (section 3.3 and 4.1
> are examples, but I noted others) where the "INCOMPLETE",
> "INCORRECT", "INCORRECT AND INVALIDATED", "CORRECTED",
> "FORMAL ADDITION" notation is not used, and the text doesn't
> say anything using 2119, but it seems to be changing the
> protocol. Given that the implementer will have two
> specifications open at the same time, being absolutely clear
> about what the text says NOW seems pretty important.

I think this has also been the guiding principle when writing
the draft, let's look at the specific cases.

> Abstract
> 
>    RFC 3095 defines the RObust Header Compression (ROHC)
>    framework and profiles for IP, UDP, RTP, and ESP. Some
>    parts of the specification
> 
> Editorial: IP and UDP are probably OK, but ESP should
> certainly be expanded on first use, and RTP is an "almost
> certainly expand on first use". 

Agree!  (now I also noticed UDP-Lite was mis-spelled "UPD")

> 1. Introduction and terminology
> 
>    RFC 3095 [1] defines the RObust Header Compression (ROHC)
>    framework and profiles for IP [8][9], UDP [10], RTP [11],
>    and ESP [12]. 
> 
> Editorial: same comment for RTP and ESP expansion on first
> use as in abstract.

Agree! 

>    This document summarizes identified issues and provides
>    corrections needed for implementations of RFC 3095 to
>    interoperate, i.e. it constitutes an update to RFC 3095.
>    This document also provides other clarifications related
>    to common misinterpretations of the specification. When
>    referring to RFC 3095, this document should therefore
>    also be referenced.
> 
> Editorial: "when referring to RFC 3095, this document should
> therefore also be referenced." is probably correct but easy to
> misinterpret, I think. Perhaps something like "References to
> RFC 3095 should also include this document."

OK!

>    When a section of this document makes formal corrections,
>    additions or invalidations to text in RFC 3095, this is
>    clearly summarized. The text from RFC 3095 that is being
>    addressed is given and labeled "INCOMPLETE", "INCORRECT"
>    or "INCORRECT AND INVALIDATED", followed by the correct
>    text labeled "CORRECTED", where applicable. When a formal
>    addition is provided, it is given with the label "FORMAL
>    ADDITION".
> 
> Technical Comment: I'm not sure "formal addition" is a widely
> used term - at least, not widely used enough to allow circular
> self-definition. Perhaps "When text is added that does not
> simply correct text appearing in a previous specification, it
> is given with the label "FORMAL ADDITION"." No problem with
> using the label elsewhere in the document, it just needs to
> be described more clearly here.

Agree, and your suggestions seem to be fine!

> 2.3. CRC coverage in CRC feedback options
> 
>    Section RFC3095-5.7.6.3 is incomplete, as it does not
>    mention how the "size" field is handled when calculating
>    the 8-bit CRC used in the CRC feedback option. Since the
>    "size" field can be considered an extension of the "code"
>    field, it must be treated in the same way.
> 
> Editorial: if the specification is being corrected based on
> this consideration, perhaps it should be "field is considered
> an extension".

OK, I would even remove "considered".

> 2.4. CRC coverage of the ESP NULL header
> 
>    Section RFC3095-5.8.7 gives the CRC coverage of the ESP NULL
>    [13] header as "Entire ESP header". This must be interpreted
>    as including only the initial part of the header (i.e. SPI
>    and Sequence number), and not the trailer part at the end of
>    the payload. Therefore, the ESP NULL header has the same CRC
>    coverage as the ESP header used in the ESP profile (section
>    RFC3095-5.7.7.7). 
> 
> Technical Comment: Doesn't this actually need a text correction?
> I'm reading this section as saying ["Entire" must be interpreted
> as "only the initial part"], and if I'm intrpreting this correctly,
> the statement doesn't work for me.

We do not consider this a correction, only an observation, making
clear the only reasonable interpretation. "Entire header" excludes
"trailer", in this regard RFC 3095 and RFC 4303 (2406) are
consistent.

> 3.1. Feedback during mode transition to U- and O-mode
> 
>    These enhanced procedures should be considered only as a
>    possible improvement to a decompressor implementation, since
>    interoperability is not affected in any way. A decompressor
>    implemented according to the optimized procedures will
>    interoperate with an RFC3095 compressor, as well as a
>    decompressor implemented according to the procedures
>    described in RFC3095 does.
> 
> Editorial: The last sentence reads oddly to me. It would read
> more clearly if the sentence ended "will interoperate with an
> RFC3095 compressor.", without the last clause, but even removing
> the final "does" would be clearer.

Ok, agree!

> 3.1.1. Mode transition procedures allowing sparse feedback
> 
>    This enhanced transition, where feedback need not be sent for
>    every decompressed packet, does however introduce some
>    considerations in case feedback messages would be lost.
>    Specifically, there is a risk for a deadlock situation when
>    a transition from R-mode is performed in case no feedback
>    message successfully reaches the compressor and
> 
> Editorial: s/in case/in cases where/, or even "performed, no
> feedback message successfully reaches".

Ok!

>    the transition is not complete. For transition between
>    U-mode and O- mode, there is also a small risk for reduced
>    compression efficiency. 
> 
>    To avoid this, the decompressor MUST continue to send
>    feedback at least periodically, also when in Pending
>    transition state. This is

> Editorial: I THINK the "also" can be deleted, but would
> appreciate author confirmation.

Since this rule already applies to the "Initiated" state, it
makes sense to keep "also" as it is.

>       While D_TRANS is I, the decompressor sends a NACK or ACK
>       carrying a CRC option for each packet received. When D_TRANS
>       is set to P, the decompressor do not have to send a NACK or
>       ACK for each packet received, but it MUST continue to send
>       feedback with some periodicity, and all feedback packets sent
>       MUST include the CRC
> 
> Technical Comment: I understand that the appropriate periodicity
> can be very application-dependent, but is it possible to place any
> maximum on periodicity? ("MUST send feedback every two thousand
> years" seems to violate the spirit of this description :-)

Your comment is reasonable. Still, I think this should stay as it is,
for two reasons:
1) It is in line with how 3095 is otherwise written
2) Any number we would pick would be either pointless (too big), or
   possible to question under certain assumptions about link 
   characteristics. The protocol is based on this, that is all the 
   implementer needs to know, but the number is not part of the
   protocol. Performance is completely dependent on implementation
   choices, not for just this mechanism.

> 3.1.2. Transition from Reliable to Optimistic mode
> 
>    The enhanced procedure for transition from Reliable to
>    Optimistic mode is shown below: 
> 
> Editorial: I didn't see any text that described what the
> enhancement actually was - just a sentence would keep the reader
> from having to dig the difference out without guidance.
> 
> 3.1.3. Transition to Unidirectional mode
> 
>    The enhanced procedure for transition to Unidirectional
>    mode is shown on the following figure:
> 
> Editorial: Again, a one-sentence summary of the enhancement
> would be nice here.

3.1 is about this enhancement, where 3.1.1 defines the conceptual
parts of the enhancement, and 3.1.2-3.1.3 the resulting transition
diagrams. I think this is rather well structured. 

> 3.3. Packet decoding during mode transition
> 
>    The purpose of a mode transition is to ensure that the
>    compressor and the decompressor coherently move from one mode
>    of operation to another using a three-way handshake. At one
>    point during the mode transition, the decompressor acknowledges
>    the reception of one (or more) IR, IR-DYN or UOR-2 packet(s)
>    that have mode bits set to the new mode. Packets of type 0 or 1
>    that are received up to this point are decompressed using the
>    old mode, while afterwards they are decompressed using the new
>    mode. If the enhanced transition procedures described in section
>    3.1 are used, the setting of the D_TRANS parameter to P
>    represents this breakpoint. The successful decompression of a
>    packet of type 0 or type 1 completes the mode transition. 
> 
> Technical Comment: Section 3.3 seems very reasonable, but
> does it clarify, or change, or ... - what is the relationship 
> between this text and text in previous Proposed Standards?

The section only provides clarifications to RFC 3095, it does not
present a change of any kind.

> 4. Timestamp encoding
> 
> 4.1. Encoding used for compressed TS bits
> 
>    RTP Timestamp values (TS) are always encoded using W-LSB 
>    encoding, both when sent scaled and when sent unscaled. When no
>    TS bits are transmitted in a compressed packet, TS is always
>    scaled. If a compressed packet carries an extension 3 and
>    field(Tsc)=0, the compressed packet must thus always carry
>    unscaled TS bits. For TS values sent in Extension 3, W-LSB
>    encoded values are sent using the self-describing variable-
>    length format (section RFC3095-4.5.6), and this applies to both
>    scaled and unscaled values.
> 
> Technical Comment: Again, Section 4.1 seems very reasonable,
> but does it clarify, or change, or ... - what is the relationship
> between this text and text in previous Proposed Standards?

Again, the section only provides clarifications to RFC 3095, it does
not present a change of any kind.

> 4.2. (De)compression of TS without transmitted TS bits
> 
>    When ROHC RTP operate using its most efficient packet types,
>     
> Editorial: s/operate/operates/

Ok!

>    from packet type identification and the error detection
>    CRC, only RTP sequence number (SN) bits have to be transmitted
>    in RTP compressed
> 
> Editorial: "must be transmitted", or "are transmitted"?

Agree, "are transmitted"

>    headers. All other fields are then omitted either because they 
>    are unchanged or because they can be reconstructed through a
>    function from the SN (i.e. by combining the transmitted SN bits
>    with state information from the context). Fields that can be
>    inferred from the SN are the IP Identification (IP-ID) and the
>    RTP Timestamp (TS).
> 
>    IP-ID compression and decompression, both with and without
>    transmitted IP-ID bits in the compressed header, are well
>    defined in section RFC3095-4.5.5 (see section 8.2). However, for
>    TS it is only defined how to decompress based on actual TS bits
>    in the compressed header, either scaled or unscaled, but not how
>    to infer the TS from the SN. This section specifies how the
>    scaled TS is decompressed when no TS bits are received in the
>    compressed header.
> 
> Editorial: the last two sentences weren't easy for me to
> understand. Perhaps, "RFC3095-4.5.5 defines decompression when TS
> bits are received in the compressed header, but does not specify
> how to infer the TS from the SN when no TS bits are received in the
> compressed header."?

I do not see the problem myself, but the text can probably be improved.
However, the suggested text does not point out that TS encoding, in
contrast to IP-ID, is missing for the "no bits" case in 3095. Further, 
4.5.5 is not the section that defines TS encoding for sent bits, only
IP-ID encoding.

What about:
"For the TS field, however, RFC 3095 only defines how to decompress
based on actual TS bits in the compressed header, either scaled or 
unscaled, but not how to infer the TS from the SN when there are no TS
bits present in the compressed header. This section therefore specifies
how the scaled TS is decompressed when no TS bits are received in the
compressed header."

> 4.4.1. TS_STRIDE for scaled timestamp encoding
> 
>    The relation between TS and TS_SCALED, given by the following
>    equality in section RFC3095-4.5.3, defines the mathematical
>    meaning of TS_STRIDE: 
> 
>       TS = TS_SCALED * TS_STRIDE + TS_OFFSET
> 
>    TS_SCALED is incorrectly written as TS / TS_STRIDE in the
>    compression step following the above core equality. This formula
>    is incorrect both because it excludes TS_OFFSET and because it
>    would prevent a TS_STRIDE value of 0, which is an alternative not
>    excluded by the definition or by the core equality above. If "/"
>    were a generally unambiguously defined operation meaning "the
>    integral part of the result from dividing X by Y", the absence of
>    TS_OFFSET could be explained, but the formula would still lack a
>    proper output for TS_STRIDE equal to 0. The formula of "2.
>    Compression" is thus invalid. 
> 
> Technical Comment: I could probably figure out the "correct
> formula in the compression step following the core equality", but
> that would require me to look at both documents and try to figure it
> out. Since you guys already know the right answer, could you say
> "the CORRECTED formula for the compression step is (whatever it
> is)" in the text?

The correct formula is actually the same, but with the following two
assumptions:
1) "/" means "the integral part of the result from dividing X by Y"
2) The formula is not applicable for TS_STRIDE=0 (with TS_STRIDE=0,
   TS is never sent scaled).

I propose a re-write as follows:

    TS_SCALED is incompletely written as TS / TS_STRIDE in the
    compression step following the above core equality. This formula
    is incorrect both because it excludes TS_OFFSET and because it
    would prevent a TS_STRIDE value of 0, which is an alternative not
    excluded by the definition or by the core equality above. If "/"
    were a generally unambiguously defined operation meaning "the
    integral part of the result from dividing X by Y", the absence of
    TS_OFFSET could be explained, but the formula would still lack a
    proper output for TS_STRIDE equal to 0. The formula of "2.
    Compression" is thus valid only with the following requirements:
    a) "/" means "the integral part of the result from dividing X by Y"
    b) TS_STRIDE>0 (TS is never sent scaled when TS_STRIDE=0) 

> 4.4.2. TS wraparound with scaled timestamp encoding
> 
>    Section RFC3095-4.5.3 states in point 4 and 5 that the compressor
>    is not required to initialize TS_OFFSET at wraparound, but that it
>    is required to increase the number of bits sent for the scaled TS
>    value when there is a TS wraparound. The decompressor is also
>    required to detect and cope with TS wraparound, including updating
>    TS_OFFSET.
> 
>    This method is not interoperable and not robust. The gain is also
>    insignificant, as TS wraparound happens very seldom. Therefore, the
>    compressor reinitializes TS_OFFSET upon TS wraparound, by sending 
>    unscaled TS. 
> 
> Technical Comment: is this "the compressor MUST reinitialize
> TS_OFFSET"?

This is indeed covered by the complete rewrite provided by section
4.4.3, which formally summarizes the resulting changes from 4.4.1 and
4.4.2. In 4.4.3 it says, SHOULD, and we should then adjust the last
sentence of 4.4.2 to say:

"Therefore, the compressor should reinitialize TS_OFFSET upon TS
wraparound, by sending unscaled TS."

> 4.7. Using timer-based compression
> 
>    Whether to use timer-based compression or not is controlled by the
>    TIME_STRIDE control field, which can be set either by an IR, an IR-
>    DYN, or by a compressed packet with extension 3. Before timer-based
>    compression can be used, the decompressor has to inform the
>    compressor (on a per-channel basis) about its clock resolution by
>    sending a CLOCK feedback option for any CID on the channel. The
>    compressor can then initiate timer-based compression by sending
>    (on a per-context basis) a non-zero TIME_STRIDE to the
>    decompressor. First
> 
> Editorial: s/First when/When/
> 
>    when the compressor is confident that the decompressor has received
>    the TIME_STRIDE value, it can switch to timer-based compression.

Ok!

> 5.3. Bit masks in list compression
> 
>    The decompressor MUST NOT modify items with an index not covered by
>    the 7-bit mask, when a 7-bit mask is received for a reference list
>    that contains more than 7 items.
> 
> Technical Comment: is this adding a MUST NOT that isn't in
> the original specification? If so, should it be flagged as a FORMAL
> ADDITION?

Well, no, the MUST NOT is actually just clarifying a direct consequence
of what RFC 3095 specifies, one can not do otherwise.

> 5.6. Translation tables and indexes for IP extension headers
> 
>    When content of extension headers changes, an implementation can
>    choose to either use a different index, or update the existing one.
>    Some extensions can be compressed away also when some fields
>    change,
> 
> Editorial: s/be compressed away also/also be compressed away/

That would change the meaning of the sentence. Maybe:
"Some extensions can be compressed away even when some..."

> 6.1. Implicit updates
> 
>    A context updating packet that contains compressed sequence number
>    information may also carry information about other fields; in such
>    case, these fields are updated according to the content of the
> 
> Editorial: s/case/cases/

Ok!

>    An updating packet thus updates the reference values of all header
>    fields, either explicitly or implicitly, with an exception for the
> 
> Editorial: s/with an exception for/except for/
> 
>    UO-1-ID packet (see section 6.2 of this document). In UO-mode, all
>    packets are updating packets, while in R-mode all packets with a
>    CRC are updating packets.

Ok!

> 7.2. CID/context re-use
> 
<snip>
> 
>    How context resources are re-used is in RFC 3095 [1] and subsequent
>    ROHC standards left unspecified and up to implementation. 
> 
> Editorial: s/in RFC 3095 [1] and subsequent ROHC standards left 
> unspecified/left unspecified in RFC 3095 [1] and subsequent ROHC
> standards/

Almost ok, I'll make it:
"How context resources are re-used is left unspecified in RFC 3095 [1]
and subsequent 3095-based ROHC specifications." 

> 8.6. Multiple CRC options in one feedback packet
> 
>    Although it is not useful to have more than one single CRC
>    option in a feedback packet, having multiple CRC options is still
>    allowed. If multiple CRC options are included, all such CRC options 
>    MUST be identical, as they will be calculated over the same header, 
>    the compressor SHOULD otherwise discard the feedback packet.
> 
> Technical Comment: Why SHOULD here? Is there a reason besides "not
> supporting this specification" that the compressor would not discard
> the feedback packet? If so, providing the reason why this is not
> a MUST would be nice.

OK, since RFC 3095 already makes it a MUST to discard the feedback if
one CRC option fails, I will not even argue for this SHOULD. To be
consistent, it has to say MUST. Thanks for catching this!

> 8.10. Expecting UOR-2 ACKs in O-mode
> 
>    It is thus RECOMMENDED to not use of the optional ACK mechanism in
> 
> Editorial: s/RECOMMENDED to not/NOT RECOMMENDED to/

Ok!

_______________________________________________
Gen-art mailing list
Gen-art@ietf.org
https://www1.ietf.org/mailman/listinfo/gen-art