GNOME Bugzilla – Bug 794909
ulpfecdec: output perfect seqnums
Last modified: 2018-04-19 16:19:43 UTC
See commit message
Created attachment 370451 [details] [review] ulpfecdec: output perfect seqnums ULP FEC, as defined in RFC 5109, has the protected and protection packets sharing the same ssrc, and a different payload type, and implies rewriting the seqnums of the protected stream when encoding the protection packets. This has the unfortunate drawback of not being able to tell whether a lost packet was a protection packet. rtpbasedepayload relies on gaps in the seqnums to set the DISCONT flag on buffers it outputs. Before that commit, this created two problems: * The protection packets don't make it as far as the depayloader, which means it will mark buffers as DISCONT every time the previous packets were protected * While we could work around the previous issue by looking at the protection packets ignored and dropped in rtpptdemux, we would still mark buffers as DISCONT when a FEC packet was lost, as we cannot know that it was indeed a FEC packet, even though this should have no impact on the decoding of the stream With this commit, we consider that when using ULPFEC, gaps in the seqnums are not a reliable indicator of whether buffers should be marked as DISCONT or not, and thus rewrite the seqnums on the decoding side as well to form a perfect sequence, this obviously doesn't prevent the jitterbuffer from doing its job as the ulpfec decoder is downstream from it.
Generally makes sense to me, however a "big enough" seqnum gap should probably still cause DISCONT and our new seqnum to have a discont too? Also rtpjitterbuffer would still send LOST events for missing packets here, or not? So a RTX+FEC solution would potentially cause FEC packets to be re-requested? Let's hope soon enough we can move to flexfec :)
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). 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. 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.
(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 :)
(In reply to Sebastian Dröge (slomo) from comment #2) > Generally makes sense to me, however a "big enough" seqnum gap should > probably still cause DISCONT and our new seqnum to have a discont too? > Right, forgot about that, will fix > Also rtpjitterbuffer would still send LOST events for missing packets here, > or not? So a RTX+FEC solution would potentially cause FEC packets to be > re-requested? > RTX + FEC will work just as before, as the retransmission requests will occur on the unmodified seqnums. > > Let's hope soon enough we can move to flexfec :) ++
@(In reply to Mathieu Duponchelle from comment #4) >I think it should either drop these lost events entirely In that case, decoders will have no idea that they are dealing with a corrupt stream. It is a too harsh approach. VP8 decoder is particularly dumb, not able to detect that the stream is corrupted. > 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. You are right here. I guess the main reason why I said it brings more harm than good that is because its hard for me to foresee potential issues with all the elements based on basedepayload & video/audiodecoders. Hiding all gaps in sequence numbers will backfire in some scenarios. Then let's modify the patch to reflect lost events we receive in new sequence numbers and vice versa - rewrite sequence number in lost events. For example: We receive packets: #0 #2 #3 lostevent#4 #5 We push out: #100 #101 #102 lostevent#103 #104 By doing that we hid that there was dropped FEC packet #1, but we didn't hide gap in seqnums because of real packet loss. We still have lost events with new sequence numbers. As you have said, FEC encoder is rewriting sequence numbers. In that case, FEC decoder will behave symmetrically to the encoder. Which is nice.
Found one issue with the patch. rtp_storage_put_recovered_packet should be called with the packet that has "untouched" sequence number. The packet we put back to the storage can be used in the future to recover other packets, we can't not mess with its content. The sequence should be: put back "untouched" packet to the storage first, make the buffer writeable and modify seqnum after.
I'll sum up the discussion Mikhail and I had: We both agree that: 1) Seqnums should be rewritten, because rtpbasedepayload can't know whether there is a jitterbuffer upstream which will take care of setting the DISCONT flags 2) There needs to be some indication that *something* was lost sent downstream, I initially assumed the vp8 depayloader for example would be capable of determining this without relying on this information at all, thanks to the picture-id and the M and S bits, but it isn't enough for the case where a packet goes missing between these two markers. The remaining bit on which we disagree is whether there should be gaps at all in the output seqnums: I think that there shouldn't be, because ulpfecdec has to cope with rtpbasepayload's behaviour, and adding a property there to disable it would just be messy and annoying to discover Mikhail argues that it feels weird, I agree with that, but think we don't really have a choice if we want things to work out of the box. I propose the following: Keep the patch as is, and simply remove the seqnum from the PacketLost events we propagate. If need be, we can update downstream elements that would expect a seqnum there, but for example the default implementation of handle_packet_loss in rtpbasedepayload does not use that field at all.
Created attachment 370612 [details] [review] ulpfecdec: output perfect seqnums ULP FEC, as defined in RFC 5109, has the protected and protection packets sharing the same ssrc, and a different payload type, and implies rewriting the seqnums of the protected stream when encoding the protection packets. This has the unfortunate drawback of not being able to tell whether a lost packet was a protection packet. rtpbasedepayload relies on gaps in the seqnums to set the DISCONT flag on buffers it outputs. Before that commit, this created two problems: * The protection packets don't make it as far as the depayloader, which means it will mark buffers as DISCONT every time the previous packets were protected * While we could work around the previous issue by looking at the protection packets ignored and dropped in rtpptdemux, we would still mark buffers as DISCONT when a FEC packet was lost, as we cannot know that it was indeed a FEC packet, even though this should have no impact on the decoding of the stream With this commit, we consider that when using ULPFEC, gaps in the seqnums are not a reliable indicator of whether buffers should be marked as DISCONT or not, and thus rewrite the seqnums on the decoding side as well to form a perfect sequence, this obviously doesn't prevent the jitterbuffer from doing its job as the ulpfec decoder is downstream from it.
Created attachment 370613 [details] [review] rtpbasedepayload: condition the sending of gap events The default implementation for packet loss handling previously always sent a gap event. While this is correct as long as we know the packet that was lost was actually a media packet, with ULPFEC this becomes a bit more complicated, as we do not know whether the packet that was lost was a FEC packet, in which case it is better to not actually send any gap events in the default implementation. Some payloaders can be more clever about, for example VP8 can use the picture-id, and the M and S bits to determine whether the missing packet was inside an encoded frame or outside, and thus whether if it was a media packet or a FEC packet, which is why ulpfecdec still lets these lost events go through, though stripping them of their seqnum, and appending a new "might-have-been-fec" field to them. This is all a bit terrible, but necessary to have ULPFEC integrate properly with the rest of our RTP stack.
Created attachment 370614 [details] [review] ulpfecdec: output perfect seqnums ULP FEC, as defined in RFC 5109, has the protected and protection packets sharing the same ssrc, and a different payload type, and implies rewriting the seqnums of the protected stream when encoding the protection packets. This has the unfortunate drawback of not being able to tell whether a lost packet was a protection packet. rtpbasedepayload relies on gaps in the seqnums to set the DISCONT flag on buffers it outputs. Before that commit, this created two problems: * The protection packets don't make it as far as the depayloader, which means it will mark buffers as DISCONT every time the previous packets were protected * While we could work around the previous issue by looking at the protection packets ignored and dropped in rtpptdemux, we would still mark buffers as DISCONT when a FEC packet was lost, as we cannot know that it was indeed a FEC packet, even though this should have no impact on the decoding of the stream With this commit, we consider that when using ULPFEC, gaps in the seqnums are not a reliable indicator of whether buffers should be marked as DISCONT or not, and thus rewrite the seqnums on the decoding side as well to form a perfect sequence, this obviously doesn't prevent the jitterbuffer from doing its job as the ulpfec decoder is downstream from it.
Seems all good to me, Mikhail?
Yes, I agree. Seems good to me too.
Although tests of the new behavior in basedepayload and ulpfecdec are missing.
Comment on attachment 370613 [details] [review] rtpbasedepayload: condition the sending of gap events Attachment 370613 [details] pushed as 8467939 - rtpbasedepayload: condition the sending of gap events
Attachment 370614 [details] pushed as 90f5ae8 - ulpfecdec: output perfect seqnums
Mikhail, I pushed with tests updated for both basedepayload and ulpfecdec, thanks!