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

From: Limin Wang <lance.lmwang@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@gmail.com>
---
 libavformat/rtpdec.c | 1 +
 1 file changed, 1 insertion(+)

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

> From: Limin Wang <lance.lmwang@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@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

On Wed, Oct 14, 2020 at 10:56:33PM +0300, Martin Storsjö wrote:
> On Wed, 14 Oct 2020, lance.lmwang@gmail.com wrote:
> 
> > From: Limin Wang <lance.lmwang@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@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.

yes, my use case is the rtp input is restarted, so the next packets are
discontinued and dropped which isn't expected behavior. I'll consider more
about the case your raised. 


> 
> // Martin
>