[FFmpeg-devel] [RFC] rtpdec: Reordering RTP packets

[Ronald S. Bultje]

Hi,

On Tue, May 25, 2010 at 6:33 PM, Martin Storsj? <martin at martin.st> wrote:
> On Wed, 26 May 2010, Michael Niedermayer wrote:
>> On Wed, May 26, 2010 at 12:19:55AM +0300, Martin Storsj? wrote:
>> > On Tue, 25 May 2010, Michael Niedermayer wrote:
>> >
>> > > very low delay is absolutely mandatory for some applications and refusing
>> > > to return the correct packet if you have 2 packets is just insane.
>> >
>> > Well, that's what the code currently does, too.
>> >
>> > The code we've proposed up til now doesn't make the "minimum delay" case
>> > any different from what it does now, if the reordering is disabled.
>> >
>> > > i dont object to a max_delay (in milli second or such)
>> > > i object to the code returning the wrong packet of several
>> > > that it has immedeately available if max_delay=0 or for example
>> > > if there are 5 packets in the kernel buffer and max_delay=1 to then
>> > > limit reordering to 1 packet (and drop 3 as they kernel buffer overflows
>> > > till the next call of av_read_frame())
>> >
>> > I tried implementing something like the thing I outlined above, and this
>> > should handle these cases where all the necessary packets are in kernel
>> > buffers but in incorrect orders.
>> >
>> > That is, first we try to receive as many packets as we need to get the
>> > correct one using nonblocking reads. If the correct one isn't available at
>> > all, then we return the first one of the packets in the reorder queues
>> > when max_delay has expired.
>> >
>> > The attached patches does this - they're untested, but try to show roughly
>> > how this could be implemented.
>> [...]
>> > @@ -1806,9 +1807,27 @@ static int rtsp_fetch_packet(AVFormatContext *s, AVPacket *pkt)
>> > ?#endif
>> > ? ? ?case RTSP_LOWER_TRANSPORT_UDP:
>> > ? ? ?case RTSP_LOWER_TRANSPORT_UDP_MULTICAST:
>> > - ? ? ? ?len = udp_read_packet(s, &rtsp_st, buf, sizeof(buf), 1);
>> > + ? ? ? ?len = udp_read_packet(s, &rtsp_st, buf, sizeof(buf), wait);
>> > ? ? ? ? ?if (len >=0 && rtsp_st->transport_priv && rt->transport == RTSP_TRANSPORT_RTP)
>> > ? ? ? ? ? ? ?rtp_check_and_send_back_rr(rtsp_st->transport_priv, len);
>> > + ? ? ? ?if (len == FF_NETERROR(ETIMEDOUT) && !wait) {
>> > + ? ? ? ? ? ?/* No more queued packets in the kernel buffers */
>> > + ? ? ? ? ? ?if (rt->transport == RTSP_TRANSPORT_RTP && s->max_delay <= 0) {
>> > + ? ? ? ? ? ? ? ?/* max_delay expired, try to flush one of the queued packets,
>>
>> s->max_delay <= 0 and max_delay expired dont sound like they match
>> or are you intending to decrease s->max_delay ?
>
> The comment is a bit badly worded. I meant "max_delay has elapsed".
>
> So if the correct packet wasn't found when reading nonblocking, wait for
> more packets up to s->max_delay, after that has elapsed (more precisely,
> after receiving the next packet after max_delay has elapsed, since I
> didn't write any code to abort waiting for packets after an exact time)
> just try to flush out the first packet from the queues, even if we haven't
> got the one we're looking for.

That sounds OK to me. Is this still being worked on?

Ronald