[MPlayer-dev-eng] [PATCH] RTP Reorder - fix out-of-sequence packets in rtp streaming

[Nico Sabbi]

ErniTron wrote:

> Hello List,
>
> Attached is an RTP reordering algorithm based on cache. It adds the
> basic feature of rtp sequence reordering as found in VLC and other
> streaming players. Patch affects only "mplayer rtp://" streaming.
>
> The problem arises also in broadband networks where UDP packet delivery
> is not guaranteed. We have noticed in a production environment where RTP
> packets can be scrambled or re-issued.
>
> The following patch just caches out-of-order RTP packets and recovers
> from scrambled or re-issued packets. Cannot recover lost packets, of
> course.
>

Hi,
I looked at the code and, apart from the indentation broken as hell and 
a couple of things that could
be improved, it looked mostly good to me (code like this is really 
needed for rtp).


> Algorithm is pretty simple and code is neat (I hope).
> When packets follows in order, behaviour is the same as previous MPlayer


not exactly: see below

> versions. If an out-of-sequence packet is found it is put in cache 
> waiting for other packets 'till the order is restored or packets limit 
> is reached.
>
> Number of packet in cache is configurable via MAXRTPPACKETSIN as in:
> #define MAXRTPPACKETSIN 32
> which also is the maximum distance between scrambled packets (it seems 
> a lot but we have found that case in a production env.)
>
> Cost of patch is an extension of DATA section of 32*2048=64KB for cache.
>
> Testing the code is not that easy unless you have access to an heavy 
> traffic network. I can assure patch was EXTENSIVELY tested and 
> considered STABLE.
>
> Best Regards,
> Erni Tron
>
>

[...]

>+    newseq = seq - rtpbuf.seq[rtpbuf.first];
>+
>+    if ((newseq == 0) || is_first)
>+    {
>  
>

this is a packet in order, right?

>+        is_first = 0;
>+
>+        //mp_msg(MSGT_NETWORK, MSGL_DBG4, "RTP (seq[%d]=%d seq=%d, newseq=%d)\n", rtpbuf.first, rtpbuf.seq[rtpbuf.first], seq, newseq);
>+        memcpy (buffer, data, length);
>  
>
this is a bounce buffer; it costs time and can be avoided reading 
directly in "buffer"
(yes, I know that previously it was done inefficiently, too) len will 
have to be set to 0
if the packet in not in sequence and must be cached

>+
>+        rtpbuf.first = ( 1 + rtpbuf.first ) % MAXRTPPACKETSIN;
>+        rtpbuf.seq[rtpbuf.first] = ++seq;
>+
>+	return length;
>+    }
>+
>  
>
[...]

>+
>+        mp_msg(MSGT_NETWORK, MSGL_ERR,  "Underrun(seq[%d]=%d seq=%d, newseq=%d)\n", rtpbuf.first, rtpbuf.seq[rtpbuf.first], seq, newseq);
>+
>+        memcpy (buffer, data, length);
>+        rtp_cache_reset(seq);
>+        return length;
>+    }
>  
>

same as above; besides, you could add a block at the end of this 
function to handle the "in sequence"
case and goto label, rather than repeating many times the same code

>+
>+    // If we have empty buffer we loop to fill it
>+    for (i=0; i < MAXRTPPACKETSIN -3; i++) {
>  
>

why  -3?


Overall is looks mostly good to me.
We are in a bugfixing situation, so committing this code, even is fixed, 
is not
allowed now; after release some developer (or me) will consider your new 
and improved
patch for inclusion.
In the meantime thanks.

    Nico