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

From: David Hayes <[email-masked]>
Date: Thu, 20 Oct 2016 18:34:35 +0200

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.

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.

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.

"over a large span in available bandwidth" -> "over a large dynamic
range"

"what is used in a new delay based rate adaptation algorithm, LEDBAT" ->
"what is used in the LEDBAT based rate adaptation algorithm"

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.

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.

"the outcome is still that there is only" -> "there is still only"

3.  Overview of SCReAM Algorithm

[PACKET_CONSERVATION] reference does not seem to be listed fully in the
bibliography.

"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)

"The fact that SCReAM follows ... makes it as safe ..." -> I'm not sure
that a statement like this is needed.

"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.

"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.

"LEDBAT is a congestion control..." --- keep with previous sentence.

"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).

"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"

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"

"design consideration" -- Do you mean policy?

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"

4.1.1.1.  Constants

QDELAY_TARGET_LO (0.1s) and QDELAY_TARGET_HI (0.4s) to not match the
50-100ms mentioned earlier.

"H264 and VP8 have however given" -> "H264 and VP8 indicate"

The last 4 constants do not start a new line for the description, and
should to be consistent.

4.1.1.2.  State variables

"computed similar to method depicted in [RFC6298]" -> "computed with a
similar method depicted to that in [RFC6298]"

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)

"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.

"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.

"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,"

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.

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"

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"

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"

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"

**To me the congestion controller should be taking into account the
  codec, not the other way around.

"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."

4.2.  SCReAM Receiver

"receiver will simply" -> "receiver must"

"via RTCP"->"via an RTCP"

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]"

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.

-- 
David Hayes

---

From: Ingemar Johansson S <[email-masked]>
Date: Mon, 24 Oct 2016 19:46:05 +0000

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,
> 
> 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

> 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.
> 
[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 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 
> 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:"
> 
[IJ] OK
> "the outcome is still that there is only" -> "there is still only"
[IJ] OK
> [PACKET_CONSERVATION] reference does not seem to be listed fully in the
[IJ] Good observation, must have been trashed...
> "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...? 

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

> "All these" -> "All of these"
[IJ] OK to all the above

> queue.  Frame skipping results in the frame rate being temporarily"
[IJ] OK to all the above

> "design consideration" -- Do you mean policy?
[IJ] Yes.
> less " -> "and is limited so that the number of bytes in flight is less"
[IJ] OK to all the above

> 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
[IJ] OK
> similar method depicted to that in [RFC6298]"
[IJ] OK
> -> "" (unnecessary repetition)
[IJ] OK

> 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."

> 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.

> close to the available transmit rate and often below that value,"
[IJ] OK
> flows will occur over the same bottleneck." --- This is unclear and
[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. 
"

> "to avoid that packet reordering triggers" -> "to avoid packet
[IJ] OK

> "bitrate to increase fast when" -> "bitrate to increase quickly when"
[IJ] OK

> avoids  RTP packets being queued"

[IJ] OK

> -> "The target bitrate given by SCReAM includes RTP"
[IJ] OK
>   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.

> have the effect of causing the bottleneck link to become
[IJ] OK
> "via RTCP"->"via an RTCP"
[IJ] OK
> is the case with LEDBAT [RFC6817].  Section A.2 in [RFC6817]"
[IJ OK
> the minimum rate paced packets that SCReAM sends when there are no
[IJ] OK, will add that.