fix nack pli overwrite bug (#2035)

* fix nack pli overwrite bug * create generic strings instead of hard coding * remove stale comment in tests
awslabs · Jul 29, 2024 · 29201e3 · 29201e3
1 parent 09e3e01
commit 29201e3
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 0 deletions.
diff --git a/src/source/PeerConnection/SessionDescription.c b/src/source/PeerConnection/SessionDescription.c
@@ -665,6 +665,7 @@ STATUS populateSingleMediaSection(PKvsPeerConnection pKvsPeerConnection, PKvsRtp
         amountWritten = SNPRINTF(pSdpMediaDescription->sdpAttributes[attributeCount].attributeValue,
                                  SIZEOF(pSdpMediaDescription->sdpAttributes[attributeCount].attributeValue), "%" PRId64 " nack", payloadType);
         CHK_ERR(amountWritten > 0, STATUS_INTERNAL_ERROR, "Full H264 rtcp-fb nack value could not be written");
+        attributeCount++;
         amountWritten = SNPRINTF(pSdpMediaDescription->sdpAttributes[attributeCount].attributeValue,
                                  SIZEOF(pSdpMediaDescription->sdpAttributes[attributeCount].attributeValue), "%" PRId64 " nack pli", payloadType);
         CHK_ERR(amountWritten > 0, STATUS_INTERNAL_ERROR, "Full H264 rtcp-fb nack-pli value could not be written");
@@ -763,6 +764,7 @@ STATUS populateSingleMediaSection(PKvsPeerConnection pKvsPeerConnection, PKvsRtp
         amountWritten = SNPRINTF(pSdpMediaDescription->sdpAttributes[attributeCount].attributeValue,
                                  SIZEOF(pSdpMediaDescription->sdpAttributes[attributeCount].attributeValue), "%" PRId64 " nack", payloadType);
         CHK_ERR(amountWritten > 0, STATUS_INTERNAL_ERROR, "Full H265 rtcp-fb nack value could not be written");
+        attributeCount++;
         amountWritten = SNPRINTF(pSdpMediaDescription->sdpAttributes[attributeCount].attributeValue,
                                  SIZEOF(pSdpMediaDescription->sdpAttributes[attributeCount].attributeValue), "%" PRId64 " nack pli", payloadType);
         CHK_ERR(amountWritten > 0, STATUS_INTERNAL_ERROR, "Full H265 rtcp-fb nack-pli value could not be written");

diff --git a/tst/SdpApiTest.cpp b/tst/SdpApiTest.cpp
@@ -12,6 +12,11 @@ namespace video {
 namespace webrtcclient {
 
 class SdpApiTest : public WebRtcClientTestBase {
+  public:
+    const std::string m_rtcp_h264_nack_line = "a=rtcp-fb:" + std::to_string(DEFAULT_PAYLOAD_H264) + " nack";
+    const std::string m_rtcp_h264_nack_pli_line = "a=rtcp-fb:" + std::to_string(DEFAULT_PAYLOAD_H264) + " nack pli";
+    const std::string m_rtcp_h265_nack_line = "a=rtcp-fb:" + std::to_string(DEFAULT_PAYLOAD_H265) + " nack";
+    const std::string m_rtcp_h265_nack_pli_line = "a=rtcp-fb:" + std::to_string(DEFAULT_PAYLOAD_H265) + " nack pli";
 };
 
 /*
@@ -407,6 +412,13 @@ TEST_F(SdpApiTest, populateSingleMediaSection_TestTxSendRecv)
     EXPECT_EQ(STATUS_SUCCESS, createOffer(offerPc, &sessionDescriptionInit));
     EXPECT_PRED_FORMAT2(testing::IsSubstring, "sendrecv", sessionDescriptionInit.sdp);
 
+    std::string offerSdp(sessionDescriptionInit.sdp);
+
+    // check nack and nack pli lines
+    std::string::size_type posPliOnly = offerSdp.find(m_rtcp_h264_nack_line);
+    std::string::size_type posPliNack = offerSdp.find(m_rtcp_h264_nack_pli_line);
+    EXPECT_NE(posPliOnly, posPliNack);
+
     closePeerConnection(offerPc);
     freePeerConnection(&offerPc);
 }
@@ -477,6 +489,13 @@ TEST_F(SdpApiTest, populateSingleMediaSection_TestTxSendOnly)
     EXPECT_EQ(STATUS_SUCCESS, createOffer(offerPc, &sessionDescriptionInit));
     EXPECT_PRED_FORMAT2(testing::IsSubstring, "sendonly", sessionDescriptionInit.sdp);
 
+    std::string offerSdp(sessionDescriptionInit.sdp);
+
+    // check nack and nack pli lines
+    std::string::size_type posPliOnly = offerSdp.find(m_rtcp_h264_nack_line);
+    std::string::size_type posPliNack = offerSdp.find(m_rtcp_h264_nack_pli_line);
+    EXPECT_NE(posPliOnly, posPliNack);
+
     closePeerConnection(offerPc);
     freePeerConnection(&offerPc);
 }
@@ -508,6 +527,13 @@ TEST_F(SdpApiTest, populateSingleMediaSection_TestTxSendOnly_H265)
     EXPECT_EQ(STATUS_SUCCESS, createOffer(offerPc, &sessionDescriptionInit));
     EXPECT_PRED_FORMAT2(testing::IsSubstring, "sendonly", sessionDescriptionInit.sdp);
 
+    std::string offerSdp(sessionDescriptionInit.sdp);
+
+    // check nack and nack pli lines
+    std::string::size_type posPliOnly = offerSdp.find(m_rtcp_h265_nack_line);
+    std::string::size_type posPliNack = offerSdp.find(m_rtcp_h265_nack_pli_line);
+    EXPECT_NE(posPliOnly, posPliNack);
+
     closePeerConnection(offerPc);
     freePeerConnection(&offerPc);
 }