Subject: [rmcat] Report on our experience implementing the feedback format in ns3-rmcat

From: Sergio Mena <[email-masked]>
Date: Thu, 28 Jun 2018 10:11:38 +0200

Hello,

An action item we took during rmcat meeting at IETF 101 was to try an 
implementation of the feedback format message 
(draft-ietf-avtcore-cc-feedback-message-01) in the ns3-rmcat simulation, 
which we open-sourced last year (https://github.com/cisco/ns3-rmcat).

We are done with such experimental implementation and would like to 
report on a few items that popped up during this exercise:

* Let me start with a very minor thing. In page 5, 1st paragraph, the 
draft reads "[...] to generate the NTP timestamp field in RTCP Sender 
Report (SR) and Receiver Report (RR) packets.". I've looked into RFC 
3550 and there does not seem to be an NTP timestamp field in RR packets 
(there is one, though, in SR packets). Maybe I'm missing a further RFC 
that updates/extends the RR packet format?

* While carrying out the implementation, we realized that the 
deserialization could be optimized, assuming the header is read 
sequentially upon deserializing.
   Since the Report Timestamp (RTS) appears _after_ the Report Blocks, 
the deserializing code needs to read the ATO fields and keep them 
unmodified until the RTS is read; only then we iterate a second time 
over all Report Blocks (and Metric Blocks) to calculate the actual 
timestamp of each Metric Block. If the RTS appeared before the Report 
Blocks (possibly renaming it to Base Timestamp or something similar) we 
could save the second iteration loop, as the timestamp calculation could 
be done as part of the iteration loop that deserializes the Report Blocks.

* Consider storing "end_seq + 1", rather than "end_seq" in the feedback 
header. It might be a bit less obvious for the human eye that is reading 
the draft, but the code that results is easier to read. Let me just 
illustrate this with an example we found during the implementation: 
'for' loops. As the sequence can wrap around, the stop condition for a 
loop (iterating over all Metric Blocks of a Report Block) will tend to 
be "i != end_seq + 1" rather than "i <= end_seq". Maybe I'm too biased 
toward C++ STL, where iterator.end() denotes we are PAST the end 
element, but, please give it a thought.

* The draft says that, in the case there are no metrics to report for an 
SSRC one SHOULD send a regular SR/RR packet. That is fine. However, both 
the sender and receiver's logic could be greatly simplified if we were 
able to send a Report Block denoting nothing was received for that SSRC 
(as an allowed alternative to regular SR/RR packets) since all the 
feedback information would be located in one place (the FB packet). In 
order to do this, consider our suggestion of storing "end_seq + 1" 
rather than "end_seq". In that case, if begin_seq == end_seq, we know 
there was nothing to report on that SSRC. Moreover, no change in the 
serialization/deserialization logic would be necessary.

* Finally, the most important suggestion for us. The draft specifies the 
constraints on sequence numbers of two consecutive feedback packets. 
Basically, the draft says that the sequence numbers between two 
consecutive reports for an SSRC should not be more than 1 quadrant apart 
(quadrant = 1/4 of the sequence space). This makes a lot of sense. 
However we strongly believe that the same restriction should be enforced 
WITHIN a report: the difference between begin_seq and end_seq must be 
less than 1 quadrant (if not less!). In other words: what am I supposed 
to do with a report that spans sequence IDs more than 1 quadrant apart? 
We know that would be too big a packet to be practical. An advantage of 
making such (huge) packets illegal is that we could easily and safely 
detect which sequence number is the beginning and which one is the end 
without resorting to their receive timestamps (which anyway is not 
trivial if the network reorders packets).
   Currently, the part of our code that deals with figuring out begin 
and end sequences (CCFeedbackHeader::CalculateBeginStopSeq) is the most 
complex by far. Adding the proposed constraint to the draft would 
simplify this part of the code drastically.

Note that we are sending these comments just to the rmcat list, so as to 
avoid unnecessary noise. Please feel free to forward elsewhere (e.g. 
avtcore) if you think it can be helpful to do so.

We hope these suggestions, based on our implementation experience, are 
useful to improve the quality of the draft.

Thanks,

Sergio

---

From: Colin Perkins <[email-masked]>
Date: Thu, 28 Jun 2018 10:24:59 +0100

Hi,

[As individual]

Thanks for the feedback!

> On 28 Jun 2018, at 09:11, Sergio Mena <semena=[email-masked]> wrote:
> 
> 
> Hello,
> 
> An action item we took during rmcat meeting at IETF 101 was to try an implementation of the feedback format message (draft-ietf-avtcore-cc-feedback-message-01) in the ns3-rmcat simulation, which we open-sourced last year (https://github.com/cisco/ns3-rmcat).
> 
> We are done with such experimental implementation and would like to report on a few items that popped up during this exercise:
> 
> * Let me start with a very minor thing. In page 5, 1st paragraph, the draft reads "[...] to generate the NTP timestamp field in RTCP Sender Report (SR) and Receiver Report (RR) packets.". I've looked into RFC 3550 and there does not seem to be an NTP timestamp field in RR packets (there is one, though, in SR packets). Maybe I’m missing a further RFC that updates/extends the RR packet format?

No, that’s a bug in the draft; the timestamp is in the SR packets.

> * While carrying out the implementation, we realized that the deserialization could be optimized, assuming the header is read sequentially upon deserializing.
>   Since the Report Timestamp (RTS) appears _after_ the Report Blocks, the deserializing code needs to read the ATO fields and keep them unmodified until the RTS is read; only then we iterate a second time over all Report Blocks (and Metric Blocks) to calculate the actual timestamp of each Metric Block. If the RTS appeared before the Report Blocks (possibly renaming it to Base Timestamp or something similar) we could save the second iteration loop, as the timestamp calculation could be done as part of the iteration loop that deserializes the Report Blocks.

I agree that would be desirable. Unfortunately, if we’re to be able to send this as an RTP/AVPF congestion control feedback packet, then the per-SSRC reports – each starting with the SSRC of the stream on which it’s reporting – have to follow the SSRC of the RTCP packet sender, so we can’t do this optimisation. The second paragraph after Figure 1 tries to explain this, but is perhaps unclear?

> * Consider storing "end_seq + 1", rather than "end_seq" in the feedback header. It might be a bit less obvious for the human eye that is reading the draft, but the code that results is easier to read. Let me just illustrate this with an example we found during the implementation: 'for' loops. As the sequence can wrap around, the stop condition for a loop (iterating over all Metric Blocks of a Report Block) will tend to be "i != end_seq + 1" rather than "i <= end_seq". Maybe I’m too biased toward C++ STL, where iterator.end() denotes we are PAST the end element, but, please give it a thought.

Seems reasonable to me.

> * The draft says that, in the case there are no metrics to report for an SSRC one SHOULD send a regular SR/RR packet. That is fine. However, both the sender and receiver's logic could be greatly simplified if we were able to send a Report Block denoting nothing was received for that SSRC (as an allowed alternative to regular SR/RR packets) since all the feedback information would be located in one place (the FB packet). In order to do this, consider our suggestion of storing "end_seq + 1" rather than "end_seq". In that case, if begin_seq == end_seq, we know there was nothing to report on that SSRC. Moreover, no change in the serialization/deserialization logic would be necessary.

The regular SR/RR packets need to be sent, so this would have to be an addition to them, rather than an alternative, but otherwise I like the idea. 

> * Finally, the most important suggestion for us. The draft specifies the constraints on sequence numbers of two consecutive feedback packets. Basically, the draft says that the sequence numbers between two consecutive reports for an SSRC should not be more than 1 quadrant apart (quadrant = 1/4 of the sequence space). This makes a lot of sense. However we strongly believe that the same restriction should be enforced WITHIN a report: the difference between begin_seq and end_seq must be less than 1 quadrant (if not less!). In other words: what am I supposed to do with a report that spans sequence IDs more than 1 quadrant apart? We know that would be too big a packet to be practical. An advantage of making such (huge) packets illegal is that we could easily and safely detect which sequence number is the beginning and which one is the end without resorting to their receive timestamps (which anyway is not trivial if the network reorders packets).
>   Currently, the part of our code that deals with figuring out begin and end sequences (CCFeedbackHeader::CalculateBeginStopSeq) is the most complex by far. Adding the proposed constraint to the draft would simplify this part of the code drastically.

Seems reasonable. 

> Note that we are sending these comments just to the rmcat list, so as to avoid unnecessary noise. Please feel free to forward elsewhere (e.g. avtcore) if you think it can be helpful to do so.
> 
> We hope these suggestions, based on our implementation experience, are useful to improve the quality of the draft.

Yes - thank you!

-- 
Colin Perkins
https://csperkins.org/