How to integrate RTCP Report Send?

[ispysoftware]

I need to add dynamic bitrate adjustment based on RTCP reports. I can see incoming reports from the browser but no outgoing reports from SipSorcery. There's SendRtcpReport in MediaStream but nothing seems to be hooked up to it - is this something the RTCSession should be periodically calling? I need reports sent so i can calculate Round Trip Time values for bitrate adjustments.

[sipsorcery]

RTCP sender and receiver eports should be being sent if your app is sending/receviing any audio/video RTP packets, see here.

The SendRTCPReport methods on the RTPSession & MediaStream classes provide the ability to send custom RTCP reports. One use case being to send feedback reports. It's up to the app to wire up any custom RTCP reports it wants to send.

[ispysoftware]

Thanks - you've been really helpful - you should add a sponsor tab to the repo - i'd happily sponsor you.
Like
https://github.com/Ruslan-B/FFmpeg.AutoGen

[ispysoftware]

In my case I am only sending audio and video, not receiving audio or video so HasVideo is returning false as RemoteTrack is null

public bool HasVideo
{
    get
    {
        return LocalTrack != null && LocalTrack.StreamStatus != MediaStreamStatusEnum.Inactive
          && RemoteTrack != null && RemoteTrack.StreamStatus != MediaStreamStatusEnum.Inactive;
    }
}

This means that Start doesn't start RTCP because HasVideo is false

foreach (var videoStream in VideoStreamList)
{
    if (videoStream.HasVideo && videoStream.RtcpSession != null && videoStream.LocalTrack.StreamStatus != MediaStreamStatusEnum.Inactive)
    {
        // The local video track may have been disabled if there were no matching capabilities with
        // the remote party.
        videoStream.RtcpSession.Start();
    }
}

I can work around this by directly starting rtcp on my video track

_pc.VideoRtcpSession.OnReportReadyToSend += VideoRtcpSession_OnReportReadyToSend;
_pc.VideoRtcpSession.Start();

but this looks like a bug to me - it shouldn't rely on there being both local and remote streams.

Should be

public bool HasVideo
{
    get
    {
        return (LocalTrack != null && LocalTrack.StreamStatus != MediaStreamStatusEnum.Inactive)
          ||  (RemoteTrack != null && RemoteTrack.StreamStatus != MediaStreamStatusEnum.Inactive);
    }
}

should be the same with audiostream - i'll do a pull request

[sipsorcery]

I've had a c;loser look at this and I think there's something fundamentally wrong with the MediaStream/AudioStream/VideoStream classes. They now have their own RTCP session instances which are different to the main Peer Connection RTCP session instance. I can't actually work out how it's meant to work (I didn't add the Stream classes). I suspect some major surgery will be required which could take a while.

[ispysoftware]

I put a breakpoint in the RTCPSession constructor and it's called once for video and once for audio - seems ok to me

[sipsorcery]

Yes, but the wiring up of the sending/receiving of RTP packets now looks broken. The RTPSession has its own RTCPSession and now there are additional ones introduced by the MediaStream classes. I'm finding it very confusing to work out what's going on. From my understanding the MediaStream and MediaStreamTrack shouldn't be involved down to the RTP/RTCP level. They are meant to be higher level abstractions.

[sipsorcery]

This issue should be fixed now. The event handler to send an RTCP report was missing. I've added it back now.

The refactor referred to above does desperately need to be done but it's a much bigger job and this one line fix should get things working for now.

[ispysoftware]

OK makes sense - if people have hooked up a send already it'll be sending them twice - not sure if that will cause an issue or not.

I think you also need to remove the event handler in CloseRtcpSession

mediaStream.RtcpSession.OnReportReadyToSend -= SendRtcpReport;

[sipsorcery]

It's not meant to be wired up externally. It was meant to be done as part of the internal plumbing. It was previoulsy in place but got missed in the MediaStream refactor.

The removal of the event handler is deliberately omitted so the RTCP BYE can be sent.