diff mbox series

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

Message ID 1602686103-3427-1-git-send-email-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/6] avformat/rtpdec: update the previous with new seq
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished

Commit Message

Limin Wang Oct. 14, 2020, 2:34 p.m. UTC
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(+)

Comments

Martin Storsjö Oct. 14, 2020, 7:56 p.m. UTC | #1
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
Limin Wang Oct. 15, 2020, 1:37 a.m. UTC | #2
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
>
diff mbox series

Patch

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;
             return -1;
         } else if (diff <= 1) {
             /* Correct packet */