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

[Ingemar Johansson S <[e-mail]>]

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

/Ingemar

> -----Original Message-----
> From: Adam Roach [mailto:[e-mail]]
> Sent: den 26 oktober 2017 07:52
> To: The IESG <[e-mail]>
> Cc: [e-mail]; Martin Stiemerling
> <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]
> Subject: Adam Roach's Discuss on draft-ietf-rmcat-scream-cc-12: (with
> DISCUSS and COMMENT)
> 
> 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."

[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 ?


> 
> 
> ----------------------------------------------------------------------
> 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).
[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==

> 
> ___
> 
> 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."
[IJ] "video bit rate" is more proper
 
> 
> 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.
[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.
> 
> Please expand ECN and EWMA on first use.
>