GNOME Bugzilla – Bug 764459
GstRTPBasedepayload fail to detect new stream after SSRC change
Last modified: 2016-04-03 08:50:11 UTC
Created attachment 325148 [details] [review]
the fix + test
There is a rare case when we start receiving the stream with the new SSRC and sequence numbers of the new stream don't differ much from the last received sequence number of the previous stream. In this case GstRTPBasepayload fails to detect that it is a new stream and can, theoretically, drop up to 99 packets. The proposed patch adds additional logic to handle this.
I think the patch is not complete though and have a suggestion to discuss. We have rtpjitterbuffer in the pipeline which handle the packet loss, reordering, duplication, new SSRCs, etc. Why should we have the similar logic in basedepayclass? It rather do harm than good. Correct me if I'm wrong, but if we decide to use rtpjitterbuffer, it is the only element which should be responsible for marking the packets with the DISCONT flag and pushing GAP events etc. in case of packetloss.
After a quick chat on IRC I found out there are use-cases when there is no rtpjitterbuffer in the pipeline. For this cases GstRTPBasepayload tries to be responsible for marking the packets with DISCONT flag and dropping possible duplicates. Ok, I got it.
So I the suggestion is to introduce the property to disable the logic, which can be used when there is upstream rtpjitterbuffer.
Can somebody make an example with a real life scenario, when we don't use rtpjitterbuffer and actually want GstRTPBasepayload to guess was there a missing packet, or a duplicate, or it as a new stream? I feel like I don't have a full picture in mind looking at the code.
Comment on attachment 325148 [details] [review]
the fix + test
Looks good but does not apply to latest GIT master.
(In reply to Mikhail Fludkov from comment #0)
> Can somebody make an example with a real life scenario, when we don't use
> rtpjitterbuffer and actually want GstRTPBasepayload to guess was there a
> missing packet, or a duplicate, or it as a new stream? I feel like I don't
> have a full picture in mind looking at the code.
People are using RTP with just an udpsrc and depayloader. Making this work somewhat OK is the point of this code.
Additionally we will need to detect disconts nonetheless in the depayloader if there's a rtpjitterbuffer. If it detects a discont, we need to detect that downstream again based on the gap in the seqnums.
Created attachment 325207 [details] [review]
the fix + test
Ok, let's try one more time. I've just pulled it from the master repo and rebased my patch on top of it.
Author: Mikhail Fludkov <email@example.com>
Date: Sat Apr 2 10:37:55 2016 +0200
rtpbasedepayload: look at ssrc before sequence numbers
Doing so prevents us dropping buffers in the rare, but possible, situations,
when the stream changes SSRC and new sequence numbers does not differ
much from the last sequence number from previous SSRC. For example:
ssrc - 0xaaaa 101,102,103,104 ssrc - 0xbbbb 102, 103, 104, 105...
In the scenario above we don't want to drop the first 3 packets of