[vlc-devel] Re: RTP reordering - a different approach

Jean-Paul Saman jean-paul.saman at planet.nl
Sat Oct 8 00:28:24 CEST 2005


Marian,

As promised here my update.

The 'stream.diff' solves the synchro warning I believe.
The 'rtp_reorder-3.patch' is almost there, but suffers from some 
problems still

Using --rtp-late 0 makes the behaviour the same as the normal case.
Any other value makes it take the rtp_reordering code when a packet has 
been lost.

I know it is not very much optimized now, but it is a first start. When 
I get this algorithm correct, then it is time to optimize a few things.

Grt,
Jean-Paul Saman.

Marian Durkovic wrote:

>Hi Jean-Paul,
>
>  of course, I'm fully aware that stream.c is not the right place for
>reordering, but I was actually trying to verify if the synchro is 
>happier when releasing the packets in equal time intervals - and it is.
>
>The code could be moved elsewhere if needed - but if your patch for synchro 
>solves the problem, there's of course no need to mess with reordering in
>stream.c
>
>  The additions in block_t are however a different story - what we have in
>the buffer looks like this:
>
>   RTP fixed header (12 bytes) |  RTP variable header (0-x bytes) | PAYLOAD
>
>  So once you move the pointer to the payload:
>
>    p_block->i_buffer -= i_skip;
>    p_block->p_buffer += i_skip;
>
>there's no way back to the RTP header fields (and you need seqence numbers 
>in both rtp_ChainInsert as well as rtp_ChainSend). 
>
>If you don't move the pointer before rtp_ChainInsert, you're not able 
>to release the chain at once - you must reparse the header fields for 
>every block again; also you have to recompute i_seqno in every pass 
>through the loops.
>
>Looking at block_t definition, it already contains:
>
>i_flags: may not always be set (ie could be 0, even for a key frame
>         it depends where you receive the buffer (before/after a packetizer
>         and the demux/packetizer implementations.
>#define BLOCK_FLAG_TYPE_I        0x0002
>#define BLOCK_FLAG_TYPE_P        0x0004
>#define BLOCK_FLAG_TYPE_B        0x0008
>i_length; length in microseond of the packet, can be null except in the
>          sout where it is mandatory
>i_samples; /* Used for audio */
>i_rate;   
>
>I don't think we'd do much harm by adding i_seqno and i_pcr, since
>there are already payload-specific attributes... The code could be
>more effective if we could just fetch the values from the block_t 
>structure and don't have to recompute them on every pass through 
>the loops and once again before sending.
>
>
>	With kind regards,
>
>		M.
>    
>
>On Thu, Oct 06, 2005 at 11:07:17PM +0200, Jean-Paul Saman wrote:
>  
>
>>Sorry, but we cannot reuse a block_t and stream.c to store protocol 
>>dependend information. It is a generic layer for handling data. Even 
>>JPEG, PNG, XML, etc.
>>
>>Using it for other then general data, would make VLC less clean in its 
>>design and probably it will start suffering from the same problems 
>>FENICE, LIVE, MPLAYER and XINE have.  A BAD DESIGN !
>>
>>Well that is my 2 cts worth.
>>
>>I already have a solution for the synchro problem. It is not that hard 
>>and requires a small generic change in stream.c. I'll try to sent a 
>>patch to you this weekend.
>>
>>Thanks for all the help.
>>
>>Grtz,
>>Jean-Paul Saman.
>>
>>Marian Durkovic wrote:
>>
>>    
>>
>>>Hi Jean-Paul,
>>>
>>>
>>> further experimenting with RTP reordering, I came to an idea for
>>>different approach:
>>>
>>> src/input/stream.c implements a buffer 100 msec of RTP stream (or 32 KB
>>>of data). This might be a good place to implement RTP reordering 
>>>without the synchro problems I've described in my previous Email.
>>>
>>> Since the buffer is already available, the implementation is
>>>quite straightforward:
>>>
>>>- UDP access needs to store RTP sequence number into p_block->i_seqno and 
>>>return packets as they're coming in
>>>
>>>- in src/input/stream.c a modified version of rtp_ChainInsert walks
>>>the buffer from top down and puts the RTP packet to the correct place
>>>(most of the time just appending it to the end of buffer)
>>>
>>>I tried to quickly implement this and it works very smoothly - there are
>>>no more synchro problems, since for every incoming packet exactly one 
>>>packet is delivered from the bottom of buffer to the next vlc parts (demux,
>>>decoder, etc.) So there are no gaps, nor many packets sent at once and
>>>the synchro is happy seeing the obvious stream of equally spaced packets.  
>>>
>>>The basic idea looks like this - what do you think?
>>>
>>>#define STREAM_CACHE_PREBUFFER_SIZE (300000)
>>>/* allow 100 msec buffering also for HDTV streams @ 20 Mbps */
>>>/* Maximum time we take to pre-buffer - not changed */
>>>#define STREAM_CACHE_PREBUFFER_LENGTH (100*1000) 
>>>
>>>static int AStreamRefillBlock( stream_t *s )
>>>
>>>[snip]
>>>
>>>  /* Append the block */
>>>  i_tmp = p_sys->block.p_last->i_seqno - b->i_seqno;
>>>
>>>  if ( i_tmp < 2000 )
>>>  {
>>>
>>>      /* packet is older than top of buffer - let's try to insert
>>>      it to the correct place by walking the buffer from top down
>>>      upto the read pointer ( p_sys->block.p_current ) */
>>>
>>>      block_t *p_tmp = p_sys->block.p_last;
>>>      block_t *p_prev = NULL;
>>>
>>>      while ( 1 )
>>>      {
>>>          i_tmp = b->i_seqno - p_tmp->i_seqno;
>>>          if( !i_tmp )
>>>          {   /* block already exists */
>>>              msg_Dbg(p_access, ">> trashing duplicate (%d)", b->i_seqno 
>>>              );
>>>              block_Release( b );
>>>              b = NULL;
>>>              break;
>>>          }
>>>          if ( i_tmp < 2000 )
>>>          {   /* insert after this block ( b->i_seqno > p_tmp->i_seqno ) 
>>>          */
>>>              msg_Dbg(p_access, ">> insert after %p(%d)==%p(%d)", p_tmp,
>>>                      p_tmp->i_seqno,  b, b->i_seqno );
>>>              p_tmp->p_next = b;
>>>              b->p_prev = p_tmp;
>>>              if (p_prev)
>>>              {
>>>                  p_prev->p_prev = b;
>>>                  b->p_next = p_prev;
>>>              }
>>>              else
>>>              {
>>>                  p_sys->block.p_last = b;
>>>                  p_sys->block.pp_last = &b->p_next;
>>>              }
>>>          break;
>>>          }
>>>          if ( p_tmp == p_sys->block.p_current )
>>>          {   /* insert into the bottom of chain */
>>>              msg_Dbg(p_access, ">> prepend %p(%d)==%p(%d)", b,
>>>                      b->i_seqno, p_tmp, p_tmp->i_seqno );
>>>              b->p_prev = p_tmp->p_prev;
>>>              p_tmp->p_prev = b;
>>>              b->p_next = p_tmp;
>>>              p_sys->block.p_current = b;
>>>              if (b->p_prev)
>>>              {
>>>                  b->p_prev->p_next = b;
>>>              }
>>>              break;
>>>          }
>>>          p_prev = p_tmp;
>>>          p_tmp = p_tmp->p_prev;
>>>      }
>>>      if ( !b ) goto again;
>>>  }
>>>  else
>>>  {
>>>
>>>      /* packet is newer - simply append it to the end of queue and
>>>         don't care about possible gap - if the missing packet(s) 
>>>         arrive later, they will be inserted to the correct place */
>>>
>>>      b->p_prev = p_sys->block.p_last;
>>>      p_sys->block.p_last = b;
>>>      *p_sys->block.pp_last = b;
>>>      p_sys->block.pp_last = &b->p_next;
>>>  }
>>>
>>>  p_sys->block.i_size += b->i_buffer;
>>>
>>>  /* Fix p_current */
>>>  if( p_sys->block.p_current == NULL )
>>>      p_sys->block.p_current = b;
>>>
>>>  /* Update stat */
>>>  p_sys->stat.i_bytes += b->i_buffer;
>>>  p_sys->stat.i_read_time += i_stop - i_start;
>>>  p_sys->stat.i_read_count++;
>>>
>>>  return VLC_SUCCESS;
>>>
>>>
>>>
>>>	With kind regards,
>>>
>>>		M.
>>>
>>>On Sun, Oct 02, 2005 at 10:29:59PM +0200, Jean-Paul Saman wrote:
>>>
>>>
>>>      
>>>
>>>>Marian,
>>>>
>>>>Thanks for helping out. However there are a few problems with your patch.
>>>>
>>>>Marian Durkovic wrote:
>>>>
>>>>  
>>>>
>>>>        
>>>>
>>>>>Hi again,
>>>>>
>>>>>here is the new version, with this additional fixes:
>>>>>
>>>>>- optimised rtp_ChainInsert, in most cases we need to add packet to the
>>>>>end of queue so we must walk from top down
>>>>>- added seqno & pcr handling in rtp_Chain
>>>>>- added limit of 100 packets if cur_seqno < i_expected  (the streamer
>>>>>might restart, change seqnos etc.) 
>>>>>
>>>>>These however need changes in struct block_t  (include/vlc_block.h)
>>>>>to add the folowing members:
>>>>>
>>>>>block_t     *p_prev;
>>>>>int         i_seqno;     /* Used for RTP */
>>>>>uint32_t    i_last_pcr;
>>>>>
>>>>>
>>>>>    
>>>>>
>>>>>          
>>>>>
>>>>A block_t doesn't know anything about the content of the payload (so it 
>>>>can be Ogg, MP3, MPEG1/2/4, A52, ARAW, DTS, etc.). So adding knowledge 
>>>>about a codec to it is a bad idea, hence i_seqno and i_last_pcr cannot 
>>>>be applied to it. About the p_prev that is probably a good idea ;-)
>>>>
>>>>  
>>>>
>>>>        
>>>>
>>>>>-replaced all expressions if ( seqno > i_cur )  etc.
>>>>>
>>>>>I've also found the reason for problems with returning NULL from udp
>>>>>input, and not accepting chain of blocks - both are located in
>>>>>src/input/stream.c:
>>>>>
>>>>>#define STREAM_DATA_WAIT 40000       /* Time between before a pf_block 
>>>>>retry */
>>>>>
>>>>>- this means vlc waits 40 msec before retrying after NULL
>>>>>
>>>>>    /* Append the block */
>>>>>    p_sys->block.i_size += b->i_buffer;
>>>>>    *p_sys->block.pp_last = b;
>>>>>
>>>>>- this only accepts 1 block, not the chain of blocks.
>>>>>
>>>>>
>>>>>    
>>>>>
>>>>>          
>>>>>
>>>>Good catch! I am experimenting with this right now and it seems to 
>>>>improve things a bit. However I have not tested all your modifications 
>>>>yet.
>>>>
>>>>  
>>>>
>>>>        
>>>>
>>>>>I'm not sure whether I can rewrite those without breaking other parts of 
>>>>>vlc
>>>>>- could you perhaps take care of those?
>>>>>
>>>>>
>>>>>    
>>>>>
>>>>>          
>>>>>
>>>>looking into it.
>>>>
>>>>  
>>>>
>>>>        
>>>>
>
>  
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: rtp_reorder-3.patch
Type: text/x-patch
Size: 16554 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20051008/bfa80c3b/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: stream.diff
Type: text/x-patch
Size: 2148 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20051008/bfa80c3b/attachment-0001.bin>


More information about the vlc-devel mailing list