Created attachment 304657 [details] [review]
rtpsession: Implement sending of reduced size RTCP packets

Some IRC discussion around this:

<slomo_> wtay: https://bugzilla.gnome.org/show_bug.cgi?id=750456 do you have an opinion? whether caps for negotiation, or a property? (or both, but no)
<wtay> slomo_, if it's in the SDP, caps
<wtay> slomo_, property not so much..
<slomo_> wtay: how would it look like in the caps? a=rtcp-rsize becomes rtcp-rsize=true?
<wtay> slomo_, a-rtcp-rsize=1 or something, I think it starts with a-
<slomo_> wtay: ok, i'll check the code :) we also have trr-int, which should override the rtcp-min-interval property btw...
<wtay> slomo_, but it's possible that it doesn't like attributes without =
<slomo_> wtay: there's also the problem that you would probably like to override rtcp-rsize from the application, if you detect that something in the middle is dropping the packets
<slomo_> so we probably also need a property additionally, maybe with an enum that takes 3 values ;)
<wtay> slomo_, maybe
<ocrete> slomo_: shouldnt rtpsession try to detect if something is dropping the packets ?
<slomo_> ocrete: that's not really trivial unfortunately. how do you detect that a fir was successful? only the depayloader knows, or the decoder
<slomo_> but ideally yes
<ocrete> yes, it's indeed non-trivial, I hoped you had smart ideas ;)
<slomo_> that's why i was thinking of putting this responsibility to whatever is around rtpbin, so that it can check if keyframes are arriving, etc :)
<wtay> it looks like a=rtcp-rsize will result in a caps entry a-rtcp-rsize and an empty string
<slomo_> ocrete: we could add something to the depayloaders to have them send an event upstream whenever they detect that some rtcp thing they did was successful or not ;)
<ocrete> and rtx
<slomo_> that's something rtpjitterbuffer could detect, yes
<slomo_> ocrete: so basically we could have a backlog of event seqnums in rtpsession for rtcp requests, and requesters should tell us with another event when they were successful or timed out
<ocrete> btw, I think it shuld be a property... the rule I tried to use was, if it's per payload, put it in the caps, if it's per session, as a property
<slomo_> makes sense

I also think that a property is better than putting it on caps. I haven't looked at the code in deeps, specially the new conditions you added, but the idea seems OK.

About the problem of detecting that remote is not processing them, I think is a complex task becase there are different types of packages and not all of them can be checked in the same way. Fir and PLI could be easily checked by depayloaders, but what about application packages (ie: chrome REMB packages)?

From my perspective, properties in general have a nicer interface and you don't have to write the code to create a capsfilter, the caps and set them. But ultimately it seems like an implementation detail. I have nothing against stuff in the SDP that can potentially be renegotiated being in caps. And I suppose many things do relate to caps.

I think was Olivier said makes sense - payload stuff describes a data format which is what we would normally have in caps. Session stuff describes how those payloads should be managed - they affect behaviours rather than data formats. So it perhaps makes sense if those are properties.

Then this particular one is a bit tricky because it affects the session but it also affects the data formatting for RTCP packets. :)

So, a property it is then. Should we just merge it as is, and let applications worry about only enabling it when they can? Maybe we later get an idea how to properly implement heuristics, but for now it seems ok to just let the application worry about it.

I think that it's OK letting applications enable it when they want, implement heuristics will complicate the issue and as this is something that should be negotiated in the SDP is not estrange to have to enable it with a property.

Let's merge this after 1.6 then? Any objections?

The proposed patch seems to fit well with our use of Reduced-Size RTCP also. Specifically we would like to send non-standard Reduced-Size RTCP Feedback messages at any time (e.g. MS-RTP) by connecting to 'on-sending-rtcp' and add the GstRTCPPacket before sending. This should allow for, when early feedback is requested from the signal 'send-rtcp' and no PLI/FIR/NACKs are pending, that 'on-sending-rtcp' is emitted with an empty buffer so that the GstRTCPPacket can be added. This seems to be the case with the patch.

In the same scenario if there is no signal handler, does the patch have an issue where we can end up trying to send an empty buffer?

I have not tested any of this yet in GStreamer 1.x, but will do that in not too long and try to add some element tests.

(In reply to Stian Selnes (stianse) from comment #8)
> The proposed patch seems to fit well with our use of Reduced-Size RTCP also.

I'll merge it now, waited here long enough :) Patches are not wine

> Specifically we would like to send non-standard Reduced-Size RTCP Feedback
> messages at any time (e.g. MS-RTP) by connecting to 'on-sending-rtcp' and
> add the GstRTCPPacket before sending. This should allow for, when early
> feedback is requested from the signal 'send-rtcp' and no PLI/FIR/NACKs are
> pending, that 'on-sending-rtcp' is emitted with an empty buffer so that the
> GstRTCPPacket can be added. This seems to be the case with the patch.

So that part is ok for you? Great!

> In the same scenario if there is no signal handler, does the patch have an
> issue where we can end up trying to send an empty buffer?

The return value of on-sending-rtcp can be used here. We only send an RTCP packet if a) it got contents before on-sending-rtcp or b) on-sending-rtcp returned TRUE.

If there was no content before and on-sending-rtcp returns FALSE, no packet will be sent.

> I have not tested any of this yet in GStreamer 1.x, but will do that in not
> too long and try to add some element tests.

Let us know how it goes :)

Comment on attachment 304657 [details] [review]
rtpsession: Implement sending of reduced size RTCP packets

commit f09da189aa9fdb8b50e7ab33e69a477e78d98e9f
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Fri Jun 5 17:20:33 2015 +0200

    rtpsession: Implement sending of reduced size RTCP packets
    
    https://bugzilla.gnome.org/show_bug.cgi?id=750456