[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