Subject: Re: [rmcat] Review of draft-ietf-rmcat-scream-cc-06

From: David Hayes <[email-masked]>
Date: Mon, 28 Nov 2016 15:36:44 +0100

Hi Ingemar,

Thank you for taking the time to do this. It is really appreciated.

We will fix the nits and add an early brief description of the grouping,
as you suggest to make it easier to follow. Thanks!

Some background regarding the incremental computation details:
Originally these were not in the draft. As a test for the document, we
asked Kristian (now one of the authors) to implement the mechanism using
only the draft. His feedback was that the incremental computation
details would make it much easier for an implementer to get a good
implementation up and running quickly. For this reason we added them.

Many thanks,

David

On Mon, Oct 24 2016 at 21:46, Ingemar Johansson S <[email-masked]> wrote:

> Hi
>
> Many thanks for the review, most of the editorial comments are accepted, 
> some comments and suggested text changes inline, marked [IJ]
>
> /Ingemar
>
>> -----Original Message-----
>> From: David Hayes [mailto:[email-masked]]
>> Sent: den 20 oktober 2016 18:35
>> To: [email-masked]
>> Subject: [rmcat] Review of draft-ietf-rmcat-scream-cc-06
>> 
>> Hi all,
>> 
>> Below is my review of draft-ietf-rmcat-scream-cc-06. I hope the comments
>> and corrections are helpful.
>> 
>> Kind regards,
>> 
>> David
>> 
>> General comments:
>> 
>> It would be better not to use the term "bandwidth", especially in the context
>> of this draft where it often refers to LTE. Capacity or bit rate, where
>> appropriate, make an unambiguous replacement.
> [IJ] OK, it sounds quite reasonable
>
>> 
>> The algorithm description has many constant numerical values, e.g.: 50, 1.5,
>> 1.25, 5, etc. It may be better if some of these were parameters.
> [IJ] Yes, will give it a shot, main problem is that sometimes find it had to find the proper names
>
>
>> 
>> Some descriptions could be more concise.
>> 
>> 
>> Section by section comments:
>> 
>> 1. Introduction
>> 
>> Applications don't necessarily need to "have" congestion control schemes.
>> They may use transports that have them. Perhaps it is better to use the word
>> "employ" instead of "have" in this context.
>> 
>> "the service *it* provides" --- it is ambiguous in this sentence.
>> 
>> "but also to ensure the function of the currently deployed Internet" --- this is
>> vague. Perhaps this phrase is not needed.
> [IJ] Perhaps rewrite as
> Applications that are deployed in the Internet must employ congestion
>    control, to achieve robust performance and to avoid congestion collapse.
>> 
>> "over a large span in available bandwidth" -> "over a large dynamic range"
> [IJ] OK
>> 
>> "what is used in a new delay based rate adaptation algorithm, LEDBAT" ->
>> "what is used in the LEDBAT based rate adaptation algorithm"
> [IJ] OK
>> 
>> SCREaAM is not an entirely self clocked protocol. It augments self clocking
>> with pacing and a minimum send rate. Perhaps this should be mentioned up
>> front.
> [IJ] OK will add it 
>> 
>> 
>> 1.1 Wireless (LTE) access properties
>> 
>> "(in one RTT)" -> ("within one RTT)"
>> 
>> 
>> 1.2 Why is it a self-clocked algorithm?
>> 
>> The first paragraph is too long and difficult to read. I suggest starting as
>> follows:
>> 
>> "Self-clocked congestion control algorithms provide a benefit over rate based
>> counterparts in that the former consists of two adaption mechanisms:"
>> 
>> Then a numbered list of the two mechanisms.
> [IJ] OK
>> 
>> 
>> "the outcome is still that there is only" -> "there is still only"
> [IJ] OK
>> 
>> 
>> 3.  Overview of SCReAM Algorithm
>> 
>> [PACKET_CONSERVATION] reference does not seem to be listed fully in the
>> bibliography.
> [IJ] Good observation, must have been trashed...
>> 
>> "the sender in feedback packets, the sender keeps" -> "the sender in
>> feedback packets. The sender keeps"
>> 
>> "amount of bytes" -> "number of bytes"
>> 
>> "All this implements a congestion control that follows the packet
>> conservation principle." -> ""  (unnecessary repetition)
> [IJ] ACK to all the above
>
>> 
>> "The fact that SCReAM follows ... makes it as safe ..." -> I'm not sure that a
>> statement like this is needed.
> [IJ] I can remove it, recall though that it was added because somebody asked a question related to this...? 
>
>> 
>> 
>> "No additional circuit breaker mechanisms are necessary with SCReAM as
>> the ACK-clocking automatically falls back to a very low transmission  rate (1
>> RTP packet/200ms) when the acknowledgements no longer arrive at  the
>> sender." --- I'm not sure how the need or not for a circuit breaker
>> mechanism relates to SCReAM being able to keep sending even when it  gets
>> no ACKs.
> [IJ] I can probably remove the sentence altogether, it is not needed for the draft.
>
>> 
>> 
>> "Furthermore, high packet loss rates reduces the congestion value to very
>> low values and thus a low transmission rate." --- This wording here doesn't
>> make sense to me. I think the sentence can probably be removed.
> [IJ] OK
>> 
>> 
>> 
>> "LEDBAT is a congestion control..." --- keep with previous sentence.
> [IJ] OK
>> 
>> 
>> "LEDBAT ensures that the end-to-end latency is kept low" --- This is not what
>> was found by Ros and Welzl (see
>> http://home.ifi.uio.no/michawe/research/publications/ledbat-impact-
>> letters.pdf).
> [IJ] Have missed this paper, thanks for the heads up.
> Yes, the base delay history is an issue. The big difference with using LEDBAT in the SCReAM context lies in the fact that the source is rate limited and that it is required that 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.
> This gives two properties
> 1) There is always a certain probability that SCReAM is short of data to transmit, which means that the network queue will run empty every once in a while.
> 2) The max video bitrate can be lower than the link capacity. If the max video bitrate is 5Mbps and the capacity is 10Mbps then we get an empty queue. 
> It is sufficient that any of the two conditions above is fulfilled to make the base delay update properly.
> As regards to the issue with shortlived competing flows, the case here is that these shortlived flows will case the ACK-clocking in SCReAM to slow down with the result that an RTP queue is built up, which will result in a reduced video bitrate. SCReAM will thus yield more to competing short lived flows than what is the case with traditional use of LEDBAT. 
> Perhaps this should be explained in the draft.
>
>
>> 
>> 
>> "concept work with conversational media.  In a few words they are:" ->
>> "concept work with conversational media:"
>> 
>> 
>> "is limited by the amount of actual bytes" -> "is limited by the actual
>> number of bytes"
>> 
>> 
>> "Fast increase for quicker bitrate increase. It makes" -> "Fast increase
>> makes"
>> 
>> "this to enable a" -> "this enables a"
>> 
>> "constitutes mainly three parts:" -> "consists of three main parts:"
>> 
>> "All these" -> "All of these"
> [IJ] OK to all the above
>
>> 
>> 
>> 3.2.  Sender Transmission Control
>> 
>> "Packet pacing limits the packet transmission rate, given by the
>> estimated link throughput, this has the effect that even if" -> "Packet
>> pacing limits the packet transmission rate given by the estimated link
>> throughput. Even if"
>> 
>> 3.3.  Media Rate Control
>> 
>> "queued up in" -> "queued in"
>> 
>> "such as the case" -> "such as in the case"
>> 
>> "discarding of encoded" -> "discarding encoded"
>> 
>> " RTP queues are drained quickly or simply that stale RTP packets are
>> removed from the queue.  Frame skipping means that the frame rate is
>> temporarily" -> " RTP queues are drained quickly. Stale RTP packets are
>> removed from the
>> queue.  Frame skipping results in the frame rate being temporarily"
>> 
> [IJ] OK to all the above
>
>> "design consideration" -- Do you mean policy?
> [IJ] Yes.
>> 
>> 
>> 4.1.  SCReAM Sender
>> 
>> "is a split between" -> "is split between"
>> 
>> "(a.k.a stream)" -> "(or stream)"
>> 
>> "different streams and then act like" -> "different streams and act
>> like"
>> 
>> "controls the media bitrate (3)" --- I think you mean "provides a target
>> rate for the media encoder (3)"
>> 
>> "takes care of the transmission of RTP packets to be
>> written to the UDP socket" -> "sends the RTP packets to the UDP socket"
>> 
>> "and is allowed to be transmitted if the number of bytes in flight is
>> less " -> "and is limited so that the number of bytes in flight is less"
>> 
> [IJ] OK to all the above
>
>> 
>> 4.1.1.1.  Constants
>> 
>> QDELAY_TARGET_LO (0.1s) and QDELAY_TARGET_HI (0.4s) to not match the
>> 50-100ms mentioned earlier.
> [IJ] OK, needs a clarification, the QDELAY_TARGET_HI value is an upper limit to how much the target delay can be increased in order to cope with competing loss based flows. It is not recommended to set the target delay this high otherwise as it increases e2e delay and makes the rate control and congestion control loop sluggish
>> 
>> 
>> "H264 and VP8 have however given" -> "H264 and VP8 indicate"
> [IJ] OK
>
>> 
>> The last 4 constants do not start a new line for the description, and
>> should to be consistent.
> [IJ] OK
>> 
>> 4.1.1.2.  State variables
>> 
>> "computed similar to method depicted in [RFC6298]" -> "computed with a
>> similar method depicted to that in [RFC6298]"
> [IJ] OK
>> 
>> 
>> 4.1.2.  Network congestion control
>> 
>> "it contains two main functions" -> "it contains two main functions:"
>> 
>> "i.e how many bytes that have been transmitted but not yet acknowledged"
>> -> "" (unnecessary repetition)
>> 
> [IJ] OK
>
>> 
>> "Unlike TCP, SCReAM is not a byte oriented protocol, rather it is an
>> RTP packet oriented protocol.  Thus a list of transmitted RTP packets
>> and their respective transmission times (wall-clock time) is kept for
>> further calculation.  The congestion control is however based on
>> transmitted and acknowledged bytes." --- This paragraph needs
>> work. SCReAM is a byte oriented congestion control algorithm. The
>> paragraph seems to contradict itself as it is written.
> [IJ] You are right , what about.
> "SCReAM is a byte protocol congestion control protocol, where the number of bytes transmitted is inferred from the size of the transmitted RTP packets. Thus a list of transmitted RTP packets and their respective transmission times (wall-clock time) is kept for further calculation."
>
>> 
>> 
>> "The qdelay fraction is sampled every 50ms" --- Previously it was
>> mentioned that there were occasions where packets could be sent once
>> every 200ms. Is this a problem here? Perhaps "The 50ms sampling is a
>> simplification a ..." is addressing this, though I think it would be
>> made clearer if the paragraph was reworded.
> [IJ] Yes, in practice this does not seem to cause any problem. I will see if I can improve this para.
>
>> 
>> "SCReAM on the other hand has a source which rate is limited to a value
>> close to the available transmit rate and often below said value," ->
>> "SCReAM on the other hand has a source whose rate is limited to a value
>> close to the available transmit rate and often below that value,"
> [IJ] OK
>> 
>> 
>> 4.1.2.2.  Competing flows compensation
>> 
>> "It is desired avoid" -> "It is desirable to avoid"
>> 
>> "positives and negatives but it manages competing FTP flows reasonably
>> well at the same time as it has proven to avoid accidental increased
>> qdelay target relatively well in simulated LTE test cases." ->
>> "positives and negatives. In the simulated LTE test cases it manages
>> competing FTP flows reasonably well at the same time as generally
>> avoiding accidental increases in the qdelay target."
>> 
>> "The algorithm can however accidentally increase the qdelay target and
>> cause self-inflicted congestion in certain cases, therefore it is
>> recommended to turn off the algorithm if is unlikely that competing
>> flows will occur over the same bottleneck." --- This is unclear and
>> needs rewriting.
> [IJ] Ok, try with 
> "The algorithm can however accidentally increase the qdelay target and cause self-inflicted congestion in certain cases. It is therefore recommended that the algorithm described in this section is turned off it is unlikely that competing flows occur over the same bottleneck. 
> "
>
>
>> 
>> 
>> 4.1.2.3.  Lost packets detection -> Lost packet detection
>> 
>> "Lost packets detection" -> "Lost packet detection"
>> 
>> "to avoid that packet reordering triggers" -> "to avoid packet
>> reordering triggering"
> [IJ] OK
>
>> 
>> 
>> 4.1.2.4.  Send window calculation
>> 
>> "A strict rule as the one given above will have the effect that the
>> media bitrate will have difficulties to increase as the congestion
>> window puts a too hard restriction on the media frame size variation.
>> This can lead to" -> "A strict rule can lead to"
>> 
>> "will further prevent bitrate increase" -> "will prevent bitrate
>> increase"
>> 
>> "bitrate to increase fast when" -> "bitrate to increase quickly when"
> [IJ] OK
>
>> 
>> 
>> 4.1.3.  Media rate control
>> 
>> "considered in the media rate control, this to" -> "considered in the
>> media rate control to"
>> 
>> "capacity and that the flow is starved out by other, more opportunistic
>> traffic, on the other hand a too aggressive setting leads to extra
>> jitter." -> "capacity leading to the flow being starved out by other,
>> more opportunistic traffic. On the other hand too aggressive a setting
>> leads to extra jitter."
>> 
>> "to avoid that RTP packets are queued" -> "to avoid RTP packets being
>> queued"
>> 
>> "RTP packets in cases the network" -> "RTP packets in cases where the
>> network"
>> 
>> "pro actively avoids that RTP packets are queued" -> "pro actively
>> avoids  RTP packets being queued"
>
> [IJ] OK
>
>> 
>> 
>> 4.1.3.1.  FEC and packet overhead considerations
>> 
>> "The target bitrate given by SCReAM depicts the bitrate including RTP"
>> -> "The target bitrate given by SCReAM includes RTP"
> [IJ] OK
>> 
>> **To me the congestion controller should be taking into account the
>>   codec, not the other way around.
> [IJ] Yes, perhaps. My thinking here is that SCReAM may not know how much extra FEC overhead there is. The Video encoder may produce a variable FEC overhead depending on the amount of packet loss. It  can be tricky (?) for a box outside the video encoder to infer this information from the RTP packet stream. The Video coder should however have an exact knowledge about this  
> I guess I here lack the expert knowledge to determine what is the most correct.
>
>> 
>> 
>> "compensate moderate errors." -> "compensate for moderate errors."
>> 
>> "Under-compensation of the overhead has the effect that the jitter will
>> increase somewhat while overcompensation will have the effect that the
>> bottleneck link becomes under-utilized." -> "Under-compensation of the
>> overhead has the effect of increasing jitter while overcompensation will
>> have the effect of causing the bottleneck link to become
>> under-utilized."
> [IJ] OK
>> 
>> 
>> 4.2.  SCReAM Receiver
>> 
>> "receiver will simply" -> "receiver must"
>> 
>> "via RTCP"->"via an RTCP"
> [IJ] OK
>> 
>> 5.  Discussion
>> 
>> "as is the case with LEDBAT [RFC6817].  Section A.2 in said RFC however" ->
>> "as
>> is the case with LEDBAT [RFC6817].  Section A.2 in [RFC6817]"
> [IJ OK
>> 
>> 
>> 
>> 10.  Security Considerations
>> 
>> "Furthermore, as SCReAM is self-clocked, a malicious middlebox can drop
>> RTCP feedback packets and thus cause the self-clocking in SCReAM to
>> stall." --- yes, but the self-clocking attack problem is mitigated by
>> the minimum rate paced packets that SCReAM sends when there are no
>> ACKs.
> [IJ] OK, will add that.
>> 
>> 
>> --
>> David Hayes
>> 

-- 
David Hayes