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

From: Adam Roach <[email-masked]>
Date: Wed, 25 Oct 2017 22:51:53 -0700

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.

---

From: Ingemar Johansson S <[email-masked]>
Date: Thu, 26 Oct 2017 09:01:11 +0000

Hi
And thanks for the review, comments inline marked [IJ]

/Ingemar

> -----Original Message-----
> From: Adam Roach [mailto:[email-masked]]
> Sent: den 26 oktober 2017 07:52
> To: The IESG <[email-masked]>
> Cc: [email-masked]; Martin Stiemerling
> <[email-masked]>; [email-masked]; [email-masked];
> [email-masked]
> Subject: Adam Roach's Discuss on draft-ietf-rmcat-scream-cc-12: (with
> DISCUSS and COMMENT)
> 
> 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.)
> 
> 
> specification of the SCReAM algorithm, and that this document is a non-
> normative companion to help understand the normative code.
> 
> - 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
> 
> 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."

[IJ] Understand the concern. 
The pseudocode is normative as is and the "for reasons of readability" quote is actually note needed. The C++ code reference is that as I personally like to see the real running code in order to fully understand the function, and to be able to do fast experiments.
The C++ code contains optimizations, for instance it is not necessary to re-compute the pacing rate and the congestion window for each received ACK and that reduces complexity at very high bitrates. The code also contains optimizations for how bytes in flight is computed
In addition there is some code to increase the stability for the cases where packets arrive in bursts at the receiving end. This occasionally happens with LTE access and the fix limits how much the congestion window can increase. The fix does not make much difference and is not critical for the function of SCReAM.
Also there is some extra code in the congestion window update function that improves the performance at very low bitrates (less than 100kbps). Again this code is not critical for the function in SCReAM.

If possible I would like to have a refence the C++ code, with the necessary changes such as removal of "for reasons of readability" and making it more clear that the pseudo code and the draft text constitute a normative specification. It is however not a MUST that the C++ code is referenced to in the draft and it cannot be completely ruled out that parts of the code can change over time, the C++ code has now also gained sufficient traction to live on its own.  

What do you think is the best way forward here ?

> 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
> 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).
[IJ] Good catch, it should actually be BETA_ECN = 0.9, will fix this. With that said, these values are recommended but in a real implementation they may eventually differ as more experiments and testing is needed.

> 
> ___
> 
> 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.
[IJ] OK, good point, will add this last in the introduction 
=BEGIN=
SCReAM can take advantage of ECN (Explicit Congestion Notification) in cases where ECN is supported by the network and the hosts. ECN is however not a requirement for the basic congestion control functionality in SCReAM.
==END==

> I think you mean "bit rate" rather than "video rate."
[IJ] "video bit rate" is more proper
 
>      Averaging factor for qdelay_fraction_avg.
[IJ] Will remove one duplicate and rewrite as 
    QDELAY_WEIGHT (0.1)
      Averaging factor for qdelay_fraction_avg.
 
    QDELAY_TREND_TH (0.2)
      Threshold for the detection of incipient congestion.

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

---

From: Adam Roach <[email-masked]>
Date: Thu, 26 Oct 2017 08:19:12 -0500

Responses below.

> What do you think is the best way forward here ?

Thanks for clarifying. Assuming that the only differences between the 
psuedocode in the draft and the C++ implementation is this small list of 
optimizations, I think you'll want to replace the text I quote above 
with something more like:

"As mentioned in section 7, calculation of qdelay may benefit from 
additional optimizations for handling of very high and very low 
bitrates, and from additional damping to handle periodic packet bursts. 
Some such optimizations are implemented in [SCReAM-CPP-implementation], 
but they do not form part of the specification of SCReAM at this time."

And then add text to section 7 suggesting additional experimentation 
with qdelay calculations.

Your remaining edits look good to me. Thanks.

/a