Makes sense I think.

I think I proposed that to Wim a few months ago and there were reasons not to do that.

The patch needs to be seen in context of https://bugzilla.gnome.org/show_bug.cgi?id=769768, which has lots of tests and explanations.

In short, to do good RTX, you need a good estimate of the round trip time (rtt), and to get a good rtt estimate you need to be able to differentiate a packet that has been requested and arrived, and one that has been requested, but where the original is just coming in a bit late. (due to jitter).

If we add a flag, we may want something a bit more generic to also include packets re-created using generic FEC.

(In reply to Olivier Crête from comment #4)
> If we add a flag, we may want something a bit more generic to also include
> packets re-created using generic FEC.

Our main use-case is to provide the jitterbuffer with much better statistics on all things rtx (and also other things like the internal jitter calculation), which again leads directly to much improved rtx. For this case, a FEC recovered packets would not be the same thing, and using the same flag for the two would lower the usefulness of it.

(In reply to Håvard Graff (hgr) from comment #5)
> (In reply to Olivier Crête from comment #4)
> > If we add a flag, we may want something a bit more generic to also include
> > packets re-created using generic FEC.
> 
> Our main use-case is to provide the jitterbuffer with much better statistics
> on all things rtx (and also other things like the internal jitter
> calculation), which again leads directly to much improved rtx. For this
> case, a FEC recovered packets would not be the same thing, and using the
> same flag for the two would lower the usefulness of it.

But adding another flag for packets re-created using FEC would be a good idea, and maybe some check (GST_RTP_BUFFER_IS_RECREATED?) that would test positive for both?

I think Sebastian suggested such a flag as well a while back but after discussion with Wim it was rejected. I don't remember all the details (we'll have to dig deep in our irc logs), but it may have been because it's not possible to reliably distinguish whether a packet has been re-sent or not. The packet coming in might just be the original one that came in late, no? So the conclusion was that these packets should not be excluded from the calculations. But I might be making this up, Sebastian and/or Wim will hopefully remember the full details.

(In reply to Tim-Philipp Müller from comment #7)
> I think Sebastian suggested such a flag as well a while back but after
> discussion with Wim it was rejected. I don't remember all the details (we'll
> have to dig deep in our irc logs), but it may have been because it's not
> possible to reliably distinguish whether a packet has been re-sent or not.
> The packet coming in might just be the original one that came in late, no?
> So the conclusion was that these packets should not be excluded from the
> calculations. But I might be making this up, Sebastian and/or Wim will
> hopefully remember the full details.

https://tools.ietf.org/html/rfc4588 explains why RTX needs its own SSRC and payload number, so when a packet is requested for retransmission, the packet that arrives is basically the original packet wrapped with a new RTP-header. The rtxreceive elements then "unpacks" the original packet, and from that time on it is indistinguishable from the original. So in order to be able to tell that this packet was once a RTX packet, a buffer flag felt like the right choice.

What you are describing was exactly what the jitterbuffer was suffering from, that the rtx-stats (like round-trip time) was being updated based on the packet arriving, but having to way to distinguish the original from the rtx that stats would often become very skewed, and rtx would suffer as a result. With the flag, the whole system becomes a lot more solid, and rtx is suddenly performing very well.

The problem here was two:
- you don't know, in general, if something is a retransmission or not. There are cases where retransmissions are sent with the same SSRC, etc. The Kurento guys always told me that this is how a) they implement it and b) Firefox does it. As such, this flag would only be an indication but not always correct
- are retransmissions really to be considered not part of the RTT? Arguably they are just like reordered packets. The packet just took that long to arrive, for whatever reason


Also see https://bugzilla.gnome.org/show_bug.cgi?id=747922 , where I added exactly the same flag ;)

(In reply to Sebastian Dröge (slomo) from comment #9)
> The problem here was two:
> - you don't know, in general, if something is a retransmission or not. There
> are cases where retransmissions are sent with the same SSRC, etc. The
> Kurento guys always told me that this is how a) they implement it and b)
> Firefox does it. As such, this flag would only be an indication but not
> always correct

You will always know if the packet used to be a RFC 4588 packet. That RFC even specifies why it is a terrible idea to re-send the same packet: (from https://tools.ietf.org/html/rfc4588#section-3)

  "One approach may be to retransmit the RTP packet with its original
   sequence number and send original and retransmission packets in the
   same RTP stream.  The retransmission packet would then be identical
   to the original RTP packet, i.e., the same header (and thus same
   sequence number) and the same payload.  However, such an approach is
   not acceptable because it would corrupt the RTCP statistics.  As a
   consequence, requirement 1 would not be met.  Correct RTCP statistics
   require that for every RTP packet within the RTP stream, the sequence
   number be increased by one."


> - are retransmissions really to be considered not part of the RTT? Arguably
> they are just like reordered packets. The packet just took that long to
> arrive, for whatever reason
> 
Easy example: You have just fired of a RTX request for a packet, but the original packet arrives 1 nanosecond later. According to the previous jitterbuffer calculation, with no way of telling the original apart from the retransmitted, your RTT is now 1 nanosecond...

Let's quote Wim about this issue: <wtay> slomo, I don't remember, but a flag sounds good to me

Review of attachment 333149 [details] [review]:

::: gst-libs/gst/rtp/gstrtpbuffer.h
@@ +155,3 @@
+ * @GST_RTP_BUFFER_FLAG_RETRANSMISSION: The #GstBuffer is a retranmission
+ *                                      packet.
+ * @GST_VIDEO_BUFFER_FLAG_LAST:         Offset to define more flags.

You probably mean GST_RTP_BUFFER_FLAG_LAST

After some more though, I think we should merge this. FEC can wait until we have a decent implementation.

I also noticed that this uses the same "bitspace" as the GST_VIDEO_BUFFER_FLAG_*, which means that potentially a RTP buffer containing vidoe data could have both. I'd suggest just restricting RTP flags to application/x-rtp and video flags to video/* caps, and document that as such.

(In reply to Olivier Crête from comment #13)
> After some more though, I think we should merge this. FEC can wait until we
> have a decent implementation.
> 
> I also noticed that this uses the same "bitspace" as the
> GST_VIDEO_BUFFER_FLAG_*, which means that potentially a RTP buffer
> containing vidoe data could have both. I'd suggest just restricting RTP
> flags to application/x-rtp and video flags to video/* caps, and document
> that as such.

I thought that was always the intention anyway. Someone wants to make a patch to clarify in the docs?

Håvard, please update this patch. Thanks! :)

And also a new one to clarify the video buffer flags as you now conflict with them

Created attachment 334588 [details] [review]
update patch with fixed flag and caps doc

Created attachment 334589 [details] [review]
specify caps for GST_VIDEO_BUFFER_FLAG

Review of attachment 334588 [details] [review]:

::: gst-libs/gst/rtp/gstrtpbuffer.h
@@ +159,3 @@
+ * Additional RTP buffer flags. These flags can potentially be used on any
+ * buffers carrying RTP packets.
+ * Note that these are only valid for #GstCaps of type: application/x-rtp (x-rtcp).

Maybe write out application/x-rtcp. But worth mentioning that the reason for this is that they might conflict with other flags, e.g. GstVideoBufferFlags

Review of attachment 334589 [details] [review]:

::: gst-libs/gst/video/video-frame.h
@@ +156,3 @@
  * Additional video buffer flags. These flags can potentially be used on any
  * buffers carrying video data - even encoded data.
+ * Note that these are only valid for #GstCaps of type: video/x-raw.

, because of potential conflicts with other extended GstBufferFlag.

commit 4b854b3440c4917272a8909c6685169b9cc404dc
Author: Havard Graff <havard.graff@gmail.com>
Date:   Thu Sep 1 09:59:06 2016 +0200

    video-frame: GST_VIDEO_BUFFER_FLAG are only valid for video/x-raw caps
    
    https://bugzilla.gnome.org/show_bug.cgi?id=769771

commit c726e0ab6d730d6d6096f45be41406c780d47450
Author: Havard Graff <havard.graff@gmail.com>
Date:   Thu Sep 1 09:57:33 2016 +0200

    rtpbuffer: Add buffer flag RETRANSMISSION
    
    Useful for elements to know if a buffer is a retransmitted RTP packet.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=769771