Implement REMB Handling #201

Sean-Der · 2020-02-20T08:25:11Z

Add new callback to RtcRtpTransceiver 'onBandwidthEstimation'.
This is now fired everytime we get an REMB packet. In the future we will
also fire this for sender side estimation, but at this time nothing is
implemented.

Resolves #114

codecov-io · 2020-02-20T21:51:23Z

Codecov Report

❗ No coverage uploaded for pull request base (master@cafebc8). Click here to learn what that means.
The diff coverage is 46.77%.

@@            Coverage Diff            @@
##             master     #201   +/-   ##
=========================================
  Coverage          ?   86.64%           
=========================================
  Files             ?       33           
  Lines             ?     7772           
  Branches          ?        0           
=========================================
  Hits              ?     6734           
  Misses            ?     1038           
  Partials          ?        0

Impacted Files	Coverage Δ
src/source/PeerConnection/Rtp.c	`72.66% <0%> (ø)`
src/source/Rtcp/RtcpPacket.c	`100% <100%> (ø)`
src/source/PeerConnection/Rtcp.c	`28.94% <3.7%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cafebc8...9aeaea1. Read the comment docs.

MushMal

Mostly minor comments but please take a look at the BO issue

MushMal · 2020-02-20T21:42:27Z

samples/Common.c

@@ -383,6 +383,10 @@ STATUS createSampleStreamingSession(PSampleConfiguration pSampleConfiguration, P
                              NULL,
                              &pSampleStreamingSession->pVideoRtcRtpTransceiver));

+    CHK_STATUS(transceiverOnBandwidthEstimation(pSampleStreamingSession->pVideoRtcRtpTransceiver,


MINOR: Suggest to think through how the bandwidth handler could differentiate between the transceivers. Either have different handlers for audio and video or in this simple case customData could be the differentiator (bad).

I think users will pass a handle to their encoders as customData, and go from there. If I was doing GStreamer I would pass a *GSTElement and have it configure everytime.

MushMal · 2020-02-20T22:00:11Z

src/include/com/amazonaws/kinesis/video/webrtcclient/Include.h

+ * RtcOnBandwidthEstimation is a KVS specific method
+ *
+ */
+typedef VOID (*RtcOnBandwidthEstimation)(UINT64, DOUBLE);


One more important question. Can the comment contain the link or something to the RFC describing the DOUBLE?

In https://tools.ietf.org/html/draft-alvestrand-rmcat-remb-00#section-2.2 it is given as a mantissa + exponent. I chose DOUBLE just because of the return value of ldexp

MushMal · 2020-02-20T22:03:50Z

src/source/PeerConnection/Rtcp.c

@@ -15,6 +15,11 @@ STATUS onRtcpPacket(PKvsPeerConnection pKvsPeerConnection, PBYTE pBuff, UINT32 b

        if (rtcpPacket.header.packetType == RTCP_PACKET_TYPE_GENERIC_RTP_FEEDBACK && rtcpPacket.header.receptionReportCount == RTCP_FEEDBACK_MESSAGE_TYPE_NACK) {
            CHK_STATUS(resendPacketOnNack(&rtcpPacket, pKvsPeerConnection));
+        } else if (rtcpPacket.header.packetType == RTCP_PACKET_TYPE_PAYLOAD_SPECIFIC_FEEDBACK &&
+                   rtcpPacket.header.receptionReportCount == RTCP_FEEDBACK_MESSAGE_TYPE_APPLICATION_LAYER_FEEDBACK &&
+                   isRembPacket(rtcpPacket.payload, rtcpPacket.payloadLength) == STATUS_SUCCESS)


CHK_STATUS(isRembPacket(...))

I don't want it to set the return value. We can have some packets that are this value, and then we just want to skip (but not actually an error). I want to keep iterating the compound packet.

Oh, of course. You could use STATUS_SUCCEEDED(isRembPacket(...)). Not a big issue

src/source/PeerConnection/Rtcp.c

MushMal · 2020-02-20T22:07:28Z

src/source/PeerConnection/Rtcp.c

+    UINT64 item;
+
+    CHK_STATUS(rembValueGet(pRtcpPacket->payload, pRtcpPacket->payloadLength, &maximumBitRate, NULL, &ssrcListLen));
+    CHK(NULL != (pSsrcList = (PUINT32) MEMCALLOC(1, SIZEOF(UINT32) * ssrcListLen)), STATUS_STORE_OUT_OF_MEMORY);


How big do you expect the buffer to be? If this is anything less than 100K you can simply use stack buffer without dynamic alloc

255 * uint32_t

You ok with me doing a uint32_t ssrcList[255] = {}?

MushMal · 2020-02-20T22:08:02Z

src/source/PeerConnection/Rtcp.c

+    }
+
+CleanUp:
+    SAFE_MEMFREE(pSsrcList);


In case of on-the-stack buffer you don't need this line

src/source/PeerConnection/Rtcp.h

src/source/Rtcp/RtcpPacket.c

MushMal · 2020-02-20T22:09:14Z

src/source/Rtcp/RtcpPacket.c

+    mantissa &= RTCP_PACKET_REMB_MANTISSA_BITMASK;
+
+    exponent = pPayload[RTCP_PACKET_REMB_IDENTIFIER_OFFSET + 5] >> 2;
+    maximumBitRate = ldexp(mantissa, exponent);


We should think of adding these things to PICs #commondefs

src/source/Rtcp/RtcpPacket.c

Add new callback to RtcRtpTransceiver 'onBandwidthEstimation'. This is now fired everytime we get an REMB packet. In the future we will also fire this for sender side estimation, but at this time nothing is implemented. Resolves #114

MushMal · 2020-02-20T23:08:59Z

src/source/PeerConnection/Rtcp.c

@@ -15,6 +15,11 @@ STATUS onRtcpPacket(PKvsPeerConnection pKvsPeerConnection, PBYTE pBuff, UINT32 b

        if (rtcpPacket.header.packetType == RTCP_PACKET_TYPE_GENERIC_RTP_FEEDBACK && rtcpPacket.header.receptionReportCount == RTCP_FEEDBACK_MESSAGE_TYPE_NACK) {
            CHK_STATUS(resendPacketOnNack(&rtcpPacket, pKvsPeerConnection));
+        } else if (rtcpPacket.header.packetType == RTCP_PACKET_TYPE_PAYLOAD_SPECIFIC_FEEDBACK &&
+                   rtcpPacket.header.receptionReportCount == RTCP_FEEDBACK_MESSAGE_TYPE_APPLICATION_LAYER_FEEDBACK &&
+                   isRembPacket(rtcpPacket.payload, rtcpPacket.payloadLength) == STATUS_SUCCESS)


Oh, of course. You could use STATUS_SUCCEEDED(isRembPacket(...)). Not a big issue

alexw91 · 2020-02-20T23:12:43Z

Codecov Report

Merging #201 into master will decrease coverage by 0.31%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
- Coverage   86.95%   86.64%   -0.32%     
==========================================
  Files          33       33              
  Lines        7698     7773      +75     
==========================================
+ Hits         6694     6735      +41     
- Misses       1004     1038      +34

Impacted Files	Coverage Δ
src/source/Ice/Network.c	`70.89% <0.00%> (ø)`	⬆️
src/source/Ice/TurnConnection.c	`91.82% <0.00%> (ø)`	⬆️
src/source/Ice/ConnectionListener.c	`90.50% <0.00%> (ø)`	⬆️
src/source/Ice/IceAgent.c	`93.70% <0.00%> (-0.11%)`	⬇️
src/source/Ice/IceUtils.c	`96.80% <0.00%> (ø)`	⬆️
src/source/Ice/IceAgentStateMachine.c	`83.33% <0.00%> (+0.67%)`	⬆️
src/source/PeerConnection/PeerConnection.c	`93.69% <0.00%> (+0.42%)`	⬆️
src/source/PeerConnection/Rtp.c	`72.66% <0.00%> (-2.70%)`	⬇️
src/source/Ice/SocketConnection.c	`82.68% <0.00%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cafebc8...9aeaea1. Read the comment docs.

codecov-io · 2020-02-20T23:31:11Z

Codecov Report

❗ No coverage uploaded for pull request base (master@cafebc8). Click here to learn what that means.
The diff coverage is 47.45%.

@@            Coverage Diff            @@
##             master     #201   +/-   ##
=========================================
  Coverage          ?   86.67%           
=========================================
  Files             ?       33           
  Lines             ?     7769           
  Branches          ?        0           
=========================================
  Hits              ?     6734           
  Misses            ?     1035           
  Partials          ?        0

Impacted Files	Coverage Δ
src/source/PeerConnection/Rtp.c	`72.66% <0%> (ø)`
src/source/Rtcp/RtcpPacket.c	`100% <100%> (ø)`
src/source/PeerConnection/Rtcp.c	`30.55% <4%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cafebc8...9aeaea1. Read the comment docs.

Sean-Der requested review from disa6302, MushMal, zhiyua-git and chehefen February 20, 2020 08:25

Sean-Der force-pushed the issue-114 branch 2 times, most recently from d1ea31c to d734710 Compare February 20, 2020 21:43

MushMal reviewed Feb 20, 2020

View reviewed changes

Sean-Der force-pushed the issue-114 branch 2 times, most recently from 5cd45dc to 8aba01d Compare February 20, 2020 22:48

Implement REMB Handling

9aeaea1

Add new callback to RtcRtpTransceiver 'onBandwidthEstimation'. This is now fired everytime we get an REMB packet. In the future we will also fire this for sender side estimation, but at this time nothing is implemented. Resolves #114

Sean-Der force-pushed the issue-114 branch from 8aba01d to 9aeaea1 Compare February 20, 2020 23:05

Sean-Der requested a review from MushMal February 20, 2020 23:08

MushMal approved these changes Feb 20, 2020

View reviewed changes

Sean-Der merged commit 2a690f1 into master Feb 21, 2020

Sean-Der deleted the issue-114 branch February 21, 2020 00:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement REMB Handling #201

Implement REMB Handling #201

Sean-Der commented Feb 20, 2020

codecov-io commented Feb 20, 2020 •

edited

Loading

MushMal left a comment

MushMal Feb 20, 2020

Sean-Der Feb 20, 2020

MushMal Feb 20, 2020

Sean-Der Feb 20, 2020

MushMal Feb 20, 2020

Sean-Der Feb 20, 2020

MushMal Feb 20, 2020

MushMal Feb 20, 2020

Sean-Der Feb 20, 2020

Sean-Der Feb 20, 2020

MushMal Feb 20, 2020

MushMal Feb 20, 2020

MushMal Feb 20, 2020

alexw91 commented Feb 20, 2020

codecov-io commented Feb 20, 2020

Implement REMB Handling #201

Implement REMB Handling #201

Conversation

Sean-Der commented Feb 20, 2020

codecov-io commented Feb 20, 2020 • edited Loading

Codecov Report

MushMal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexw91 commented Feb 20, 2020

Codecov Report

codecov-io commented Feb 20, 2020

Codecov Report

codecov-io commented Feb 20, 2020 •

edited

Loading