Implement REMB Handling by Sean-Der · Pull Request #201 · awslabs/amazon-kinesis-video-streams-webrtc-sdk-c

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

                              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

+ * 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

            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

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

+    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

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


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

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

+    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

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

            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 MushMal, chehefen, disa6302 and zhiyua-git 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

Conversation

Sean-Der commented Feb 20, 2020

Uh oh!

codecov-io commented Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

MushMal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexw91 commented Feb 20, 2020

Codecov Report

Uh oh!

codecov-io commented Feb 20, 2020

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Feb 20, 2020 •

edited

Loading