[Bug 794909] ulpfecdec: output perfect seqnums

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=794909

--- Comment #4 from Mathieu Duponchelle <mduponchelle1 at gmail.com> ---
(In reply to Mikhail Fludkov from comment #3)
> Given how weird ULPFEC beast is. The better approach is for the depayloader
> (or decoder) to take care of that. What you are trying to achieve in this
> patch should be done codec by codec basis. There is no generic solution to
> that. For example, VP8 & VP9 should be able to figure out if the
> discontinuity happened in the middle of the frame or between the frames. If
> it happened between the frames picture_id should be used to understand if
> the lost packet was a media packet or FEC (see
> https://github.com/pexip/gst-plugins-good/commit/
> 3c67c9528e9f4cd87708abefd9e967fbcc4a4e08).

I do not think I agree: it should not be the job of the depayloader / decoder
to sort out the good from the bad DISCONT flags, most of them are simply not
equipped to do so. Those, like vp8 / vp9, that have a more reliable mechanism
available (eg picture-id), should indeed use it instead, but that patch does
not prevent this.

My idea here is that as basedepayloader will transform gaps in the seqnums into
DISCONT flags set on the buffers, if the decoder element knows these gaps are
not a reliable DISCONT indicator it is its job to hide them.

> 
> The patch introduces more harm than good IMO. If you really wanna go for it,
> make this behavior disabled by default and configured by a property. 

I don't agree either: without this patch, we can end up producing a fully
repaired stream, with each depayloaded buffer marked as DISCONT. That's an
objectively broken behaviour, which I don't think we want to allow at all, or
even be the default one.

> 
> BTW. It hides discontinuity in sequence numbers by changing sequnce number
> domain of the media stream, but it does not do the same for so for the
> GstRTPPacketLost events.

Well spotted, I think it should either drop these lost events entirely, or at
least remove their seqnums, if it's not considered a mandatory field of this
event's structure.

Note that I agree that none of this is ideal, but I think it is making the most
of a poorly-designed protocol, as Sebastian said flexfec will most likely save
us all :)

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.