rtpbasedepayload: Drop gap events before first buffer

Stian Selnes @stianse submitted a patch:

Patch 337855, "rtpbasedepayload: Drop gap events before first buffer":
rtpbasedepayload-Drop-gap-events-before-first-buff.patch

Nicolas Dufresne @ndufresne said:

Won't that break sub-title streams sent to a muxer ?

Olivier Crête @ocrete said:

I think that can also break "playback" cases where you want to unblock the sink based on GAP events. Maybe the decoder should just forward the GAP events instead of trying to do concealment before the first buffer.

Stian Selnes @stianse said:

Nicolas, Olivier, I'm not familiar with the use cases you describe. But let me describe more in detail what the patch does, because the commit message perhaps wasn't specific enough.

The patch only changes the behavior of gst_rtp_base_depayload_packet_lost() which is called when the rtpjitterbuffer sends an GstRTPPacketLost event downstream about actually lost RTP packets. If no packets are lost this code path will not be entered. The gap event is dropped only if the depayloader has not yet sent a segment event downstream, which sounds reasonable since the gap event's timestamp and duration has little value without the segment event.

Does this behavior conflict with what you're describing?

Nicolas Dufresne @ndufresne said:

Sounds like a really familiar issue. Some code will craft a caps + segment for that purpose. The question is did sparse streams worked before ? Could we mux a stream with one stream sending only gaps ? (that last one is more due to CollectPads then this, but sending gaps is the old way to fix this). We should probably write a unit test for that.

Tim Müller @tpm said:

There should already be an input segment at this point, so the main problem is the caps event, right?

As far as I can tell, rtpjitterbuffer only sends packetlost events when it detects that packets are actually lost. It won't send these for sparse streams, an it won't send any if it doesn't receive packets initially.

The patch seems fine to me, but we could limit it to audio/video if needed, but it's not clear to me that that's needed, as per above.

We know that muxers don't handle live inputs well. So any pipeline with collectpads-based muxers is already pretty much broken for this use case, so this particular patch won't make much difference to them.

Also, for most muxers it's more important to get accurate caps once, and getting incomplete caps or changing caps later is a big no-no, so making up some bogus caps seems very suboptimal for that use case.

We did make sure to pass through gap events so that e.g. audiosinks could preroll and start up in cases where there's no audio at the beginning, but I believe this is only relevant for non-live pipelines?