[rmcat] Adam Roach's Discuss on draft-ietf-rmcat-scream-cc-12: (with DISCUSS and COMMENT)

[Adam Roach <[e-mail]>]

Adam Roach has entered the following ballot position for
draft-ietf-rmcat-scream-cc-12: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)


Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-rmcat-scream-cc/



----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

I'm confused about whether the text in this document is intended to form a
normative description of SCReAM. The document contains the following statement:

   Note that the pseudo code does not show all
   details for reasons of readability, the reader is encouraged to look
   into the C++ code in [SCReAM-CPP-implementation] for the details.

This effectively states that the cited C++ code forms the normative
specification of the SCReAM algorithm, and that this document is a
non-normative companion to help understand the normative code.

If this is the case, then:

- The [SCReAM-CPP-implementation] reference needs to be moved from "Informative
References" to "Normative References",

- The abstract and introduction need to make it much clearer that the normative
definition of the SCReAM algorithm is a body of C++ code rather than the prose
and psuedocode in this document, and

- We need to coordinate with the RFC editor to ensure proper archival of the
code at [SCReAM-CPP-implementation]. At this time, github.com does not meet the
standards of archival quality that the RFC series is expected to meet.

If the C++ implementation is *not* the normative definition of SCReAM, then the
psuedocode and definitions in this document need to be complete and sufficient
to implement the algorithm; and, in particular, it cannot omit algorithm
details "for reasons of readability."


----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Section 5 indicates:

   o  Support for alternate ECN semantics: This specification adopts the
      proposal in [I-D.ietf-tcpm-alternativebackoff-ecn] to reduce the
      congestion window less when ECN based congestion events are
      detected.

This needs some clarification. While the psuedocode in section 4.1.2.2 has two
different code paths for ECN- versus non-ECN-congestion, they differ only in
terms of whether they reduce the CWND according to BETA_LOSS versus BETA_ECN.
Section 4.1.1.1 defines the RECOMMENDED value for both of these constants as
0.8. If these are the same value, then treatment of ECN will be identical to
treatment of loss, right?

I suspect that either (a) one of these values was intended to be different than
the other, or (b) I've missed some additional ECN-related handling that
provides differential treatment. If neither is true, please amend the statement
in Section 5 to be more accurate (i.e.: the algorithm supports differential
handling, but the normatively recommended configuration does not provide it).

___

The document talks extensively about ECN, without ever making it clear whether
the SCReAM algorithm works without ECN. The final paragraph of section 4.2.1
sort of implies that it is optional; but this is very late in the document, and
it isn't very explicit. I would suggest adding text to the introduction that
indicates that the algorithm can take advantage of ECN information when it is
present, but that it does not require ECN to work properly.

___

Minor editorial comments follow.

Section 1.2:

   the RTP queue is kept short (preferably empty).  In addition the
   output from a video encoder is rarely constant bitrate, static
   content (talking heads) for instance gives almost zero video rate.

I think you mean "bit rate" rather than "video rate."

Section 4.1.1.1:

   QDELAY_WEIGHT (0.1)
     Averaging factor for qdelay_fraction_avg.

   QDELAY_TREND_TH (0.2)
     Averaging factor for qdelay_fraction_avg.

   QDELAY_TREND_TH (0.2)
     Averaging factor for qdelay_fraction_avg.

All three of these appear to have the same definition, and the last two appear
to have the same name.

Please expand ECN and EWMA on first use.