From 15f7e0c571ba0480e1c14522149f84e04b131e6a Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Mon, 29 Jul 2024 13:31:12 -0400 Subject: [PATCH 1/4] fix nack pli overwrite bug (#2035) * fix nack pli overwrite bug * create generic strings instead of hard coding * remove stale comment in tests --- .../PeerConnection/SessionDescription.c | 2 ++ tst/SdpApiTest.cpp | 26 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/src/source/PeerConnection/SessionDescription.c b/src/source/PeerConnection/SessionDescription.c index 2ca79fbf83..3f208da140 100644 --- a/src/source/PeerConnection/SessionDescription.c +++ b/src/source/PeerConnection/SessionDescription.c @@ -679,6 +679,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"); @@ -787,6 +788,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 index 2ac27c316b..890db5a168 100644 --- 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"; }; /* @@ -352,6 +357,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); } @@ -422,6 +434,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); } @@ -453,6 +472,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); } From c69769a88d89275bc54bb2e31326c216781b05b5 Mon Sep 17 00:00:00 2001 From: Stefan Kieszkowski <85728496+stefankiesz@users.noreply.github.com> Date: Wed, 20 Nov 2024 12:42:17 -0800 Subject: [PATCH 2/4] Check that the sdp lines are not missing, append SDP_LINE_SEPARATOR to lines checked --- tst/SdpApiTest.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tst/SdpApiTest.cpp b/tst/SdpApiTest.cpp index 890db5a168..846e918cc6 100644 --- a/tst/SdpApiTest.cpp +++ b/tst/SdpApiTest.cpp @@ -13,10 +13,10 @@ 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"; + const std::string m_rtcp_h264_nack_line = "a=rtcp-fb:" + std::to_string(DEFAULT_PAYLOAD_H264) + " nack" + SDP_LINE_SEPARATOR; + const std::string m_rtcp_h264_nack_pli_line = "a=rtcp-fb:" + std::to_string(DEFAULT_PAYLOAD_H264) + " nack pli" + SDP_LINE_SEPARATOR; + const std::string m_rtcp_h265_nack_line = "a=rtcp-fb:" + std::to_string(DEFAULT_PAYLOAD_H265) + " nack" + SDP_LINE_SEPARATOR; + const std::string m_rtcp_h265_nack_pli_line = "a=rtcp-fb:" + std::to_string(DEFAULT_PAYLOAD_H265) + " nack pli" + SDP_LINE_SEPARATOR; }; /* @@ -363,6 +363,8 @@ TEST_F(SdpApiTest, populateSingleMediaSection_TestTxSendRecv) 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); + EXPECT_NE(posPliOnly, std::string::npos); + EXPECT_NE(posPliNack, std::string::npos); closePeerConnection(offerPc); freePeerConnection(&offerPc); @@ -440,6 +442,8 @@ TEST_F(SdpApiTest, populateSingleMediaSection_TestTxSendOnly) 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); + EXPECT_NE(posPliOnly, std::string::npos); + EXPECT_NE(posPliNack, std::string::npos); closePeerConnection(offerPc); freePeerConnection(&offerPc); @@ -478,6 +482,8 @@ TEST_F(SdpApiTest, populateSingleMediaSection_TestTxSendOnly_H265) 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); + EXPECT_NE(posPliOnly, std::string::npos); + EXPECT_NE(posPliNack, std::string::npos); closePeerConnection(offerPc); freePeerConnection(&offerPc); From db1e3bf84f22498dd222df1f9917689816a3f5c7 Mon Sep 17 00:00:00 2001 From: Stefan Kieszkowski <85728496+stefankiesz@users.noreply.github.com> Date: Wed, 20 Nov 2024 14:47:26 -0800 Subject: [PATCH 3/4] Remove duplicate session attribute lines, add missing attribute name --- src/source/PeerConnection/SessionDescription.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/source/PeerConnection/SessionDescription.c b/src/source/PeerConnection/SessionDescription.c index 3f208da140..b11e83b53f 100644 --- a/src/source/PeerConnection/SessionDescription.c +++ b/src/source/PeerConnection/SessionDescription.c @@ -680,21 +680,13 @@ STATUS populateSingleMediaSection(PKvsPeerConnection pKvsPeerConnection, PKvsRtp 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++; + + STRCPY(pSdpMediaDescription->sdpAttributes[attributeCount].attributeName, "rtcp-fb"); 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"); attributeCount++; - STRCPY(pSdpMediaDescription->sdpAttributes[attributeCount].attributeName, "rtcp-fb"); - SPRINTF(pSdpMediaDescription->sdpAttributes[attributeCount].attributeValue, "%" PRId64 " nack", payloadType); - SPRINTF(pSdpMediaDescription->sdpAttributes[attributeCount].attributeValue, "%" PRId64 " nack pli", payloadType); - attributeCount++; - - STRCPY(pSdpMediaDescription->sdpAttributes[attributeCount].attributeName, "rtcp-fb"); - SPRINTF(pSdpMediaDescription->sdpAttributes[attributeCount].attributeValue, "%" PRId64 " nack", payloadType); - SPRINTF(pSdpMediaDescription->sdpAttributes[attributeCount].attributeValue, "%" PRId64 " nack pli", payloadType); - attributeCount++; - // TODO: If level asymmetry is allowed, consider sending back DEFAULT_H264_FMTP instead of the received fmtp value. if (currentFmtp != NULL) { STRCPY(pSdpMediaDescription->sdpAttributes[attributeCount].attributeName, "fmtp"); @@ -789,6 +781,8 @@ STATUS populateSingleMediaSection(PKvsPeerConnection pKvsPeerConnection, PKvsRtp 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++; + + STRCPY(pSdpMediaDescription->sdpAttributes[attributeCount].attributeName, "rtcp-fb"); 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"); From 2f693cfb6b42408ec26c0300870e3b0df5002089 Mon Sep 17 00:00:00 2001 From: Stefan Kieszkowski <85728496+stefankiesz@users.noreply.github.com> Date: Wed, 20 Nov 2024 15:07:14 -0800 Subject: [PATCH 4/4] Clang format --- src/source/PeerConnection/SessionDescription.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/source/PeerConnection/SessionDescription.c b/src/source/PeerConnection/SessionDescription.c index b11e83b53f..2173ae3e3e 100644 --- a/src/source/PeerConnection/SessionDescription.c +++ b/src/source/PeerConnection/SessionDescription.c @@ -680,7 +680,7 @@ STATUS populateSingleMediaSection(PKvsPeerConnection pKvsPeerConnection, PKvsRtp 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++; - + STRCPY(pSdpMediaDescription->sdpAttributes[attributeCount].attributeName, "rtcp-fb"); amountWritten = SNPRINTF(pSdpMediaDescription->sdpAttributes[attributeCount].attributeValue, SIZEOF(pSdpMediaDescription->sdpAttributes[attributeCount].attributeValue), "%" PRId64 " nack pli", payloadType);