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

[ErniTron]

Nico Sabbi wrote:
> 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
> 
> _______________________________________________
> MPlayer-dev-eng mailing list
> MPlayer-dev-eng at mplayerhq.hu
> http://mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
> 

Thanks for your review. Reply in short:

1) Yes, indentation is broken. But not that broken if you use ts=8 in 
vim. No problem to fix it in next version.

2) When packet is in sequence behaviour of patched code is exactly the 
same of old code in terms of buffer copy. Look at the old code:

int read_rtp_from_server(int fd, char *buffer, ...)
...
         getrtp2(fd, &rh, &data, &len);
...
         memcpy(buffer, data, len);

in fact getrtp2 has it's own static buf[1600] and rtp_read_from_server 
makes a copy of every packets to the caller buffer. My code does exactly 
the same when packets are in order. I agree with you that the copy can 
be saved, but I didn't dare to touch getrtp2 since it's a very old 
function that belongs to the original Dave Chapman dvbstream code and 
you can find it identical in other applications (rtpdump, etc...). I 
just stick to the new functionality.

3) For the sake of readability I wouldn't substitute repeated code with 
block and goto. But that's probably a matter of personal taste.

4) Why -3? It's a sort of heuristic threshold. I prefer to drop the very 
first packet and keep 4 more free slots in order to fill with new 
packets in sequence instead of dropping them while waiting for old 
packet to come.

Let me know if you agree with my considerations and I will proceed with 
a new revised patch.

Thanks again.

Erni