[FFmpeg-devel] [PATCH 1/6] avformat/rtpdec: update the previous with new seq

[Martin Storsjö]

On Wed, 14 Oct 2020, lance.lmwang at gmail.com wrote:

> From: Limin Wang <lance.lmwang at gmail.com>
>
> If the rtp input is discontinued, we'll drop all the new received packets now,
> It's better to update the previous seq with the new seq so that the following
> packets will not be dropped always.
>
> Signed-off-by: Limin Wang <lance.lmwang at gmail.com>
> ---
> libavformat/rtpdec.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/libavformat/rtpdec.c b/libavformat/rtpdec.c
> index 3d5b200..de79536 100644
> --- a/libavformat/rtpdec.c
> +++ b/libavformat/rtpdec.c
> @@ -833,6 +833,7 @@ static int rtp_parse_one_packet(RTPDemuxContext *s, AVPacket *pkt,
>             /* Packet older than the previously emitted one, drop */
>             av_log(s->ic, AV_LOG_WARNING,
>                    "RTP: dropping old packet received too late\n");
> +            s->seq = seq;

I'm not sure if this is better, depending on the usecase.

The existing logic tries to make sure that packets that have been skipped 
really aren't output, as that should make for a less disruptive data 
stream.

Consider if you're receiving packets in this order:

0 1 2 3 6 7 8 9 10 11 12 13 14 15 16 4 5 17 18 19 ...

When receiving 0-3 everything runs fine. Packets 6 and onwards are 
enqueued. If the queue size limit hits before packets 4-5 arrive (say with 
a queue size limit of 5 packets), we start outputting packets 6, 7 etc to 
the parser/decoder. When packets 4 and 5 arrive late, they're skipped 
completely. With your patch, packet 4 would be skipped but 5 would be 
included.

Thus, I think this sequence is better:

0 1 2 3 6 7 8 9 10 11 ...

Than this:

0 1 2 3 6 7 8 9 4 5 10 11 ..

The first sequence has got one discontinuity, the other one has three 
discontinuities.

I do see your point that there's an issue if the the sequence jumps to a 
different point (over 32k packets ahead from the current numbrer), and it 
would be good to handle that in some way, but the suggested patch does 
regress the cases the existing code tries to handle.

// Martin