Subject: Comments on Appendix A of draft-ietf-avt-rtp-new-04

From: Bill Fenner <[email-masked]>
Date: Tue, 21 Sep 1999 13:21:15 -0700

Hi,

  While using the UCL RTP library, which is heavily based upon
Appendix A of draft-ietf-avt-rtp-new-04, I found the following problems.
The first two are major and must be fixed; the others are just nits.

1. A single packet loss is always reported during the first reporting
interval.  I believe that this is because the fencepost error arising
from subtracting two sequence numbers is corrected for in two places:
	a) in A.2's init_seq(), setting s->base_seq to seq - 1.
	b) when calculating the number of expected packets in A.3.

Removing the -1 from the initialization of base_seq fixes this problem
(and eliminates possible problems when seq == 0), and as far as I can
tell is the correct solution.

2. Section A.3 says to clamp the loss at 0xffffff for negative loss.
0xffffff is -1; the smallest negative number in 24 bits is 0x800000.
It might be better to write the values in decimal; +8388607 to -8388608,
or at least change the 0xffffff to 0x800000 in the existing sentence.

Should the wording about clamping be stronger than a SHOULD?  I guess
the wraparound is so far out of the expected operating conditions that
it's OK to not necessarily address it, since who really cares whether
you lost 8 million packets or got 8 million extra.

3. The setting of s->bad_seq is a little non-obvious:

	s->bad_seq = (seq + 1) & (RTP_SEQ_MOD-1);

Since there's no pressing need to optimize this particular case, I think
it would be more obvious to actually use the mod operator:

	s->bad_seq = (seq + 1) % RTP_SEQ_MOD;

It's much more obvious [to me, at least] what's going on in the latter
code fragment.  (Understanding exactly what the first one does relies on
knowing the value of RTP_SEQ_MOD, while the second is obvious.  Plus, the
constant is named *_MOD, so it's fairly natural to use the mod operator
with it.)

4. On the packet that brings a source out of probation, s->max_seq is
updated twice.  It's set to seq both times, so there's no inconsistency,
but it might be worth moving the update of s->max_seq to after the
"if (s->probation == 0)" condition.  (I told you it was minor =)

5. Formatting nit: the length[] arg to rtp_write_sdes() in section A.4
should be indented consistently with the other arguments.

  Bill