diff --git a/modules/rtp_rtcp/source/rtcp_sender.cc b/modules/rtp_rtcp/source/rtcp_sender.cc index 4674315d25..bfddc422f0 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/modules/rtp_rtcp/source/rtcp_sender.cc @@ -891,29 +891,40 @@ void RTCPSender::SetVideoBitrateAllocation( // Check if this allocation is first ever, or has a different set of // spatial/temporal layers signaled and enabled, if so trigger an rtcp report // as soon as possible. - if (HasNewLayerStructure(bitrate)) { + absl::optional new_bitrate = + CheckAndUpdateLayerStructure(bitrate); + if (new_bitrate) { + video_bitrate_allocation_ = *new_bitrate; next_time_to_send_rtcp_ = clock_->TimeInMilliseconds(); + } else { + video_bitrate_allocation_ = bitrate; } - video_bitrate_allocation_ = bitrate; send_video_bitrate_allocation_ = true; SetFlag(kRtcpAnyExtendedReports, true); } -bool RTCPSender::HasNewLayerStructure( +absl::optional RTCPSender::CheckAndUpdateLayerStructure( const VideoBitrateAllocation& bitrate) const { + absl::optional updated_bitrate; for (size_t si = 0; si < kMaxSpatialLayers; ++si) { for (size_t ti = 0; ti < kMaxTemporalStreams; ++ti) { - if (bitrate.HasBitrate(si, ti) != - video_bitrate_allocation_.HasBitrate(si, ti) || - (bitrate.GetBitrate(si, ti) == 0) != - (video_bitrate_allocation_.GetBitrate(si, ti) == 0)) { - return true; + if (!updated_bitrate && + (bitrate.HasBitrate(si, ti) != + video_bitrate_allocation_.HasBitrate(si, ti) || + (bitrate.GetBitrate(si, ti) == 0) != + (video_bitrate_allocation_.GetBitrate(si, ti) == 0))) { + updated_bitrate = bitrate; + } + if (video_bitrate_allocation_.GetBitrate(si, ti) > 0 && + bitrate.GetBitrate(si, ti) == 0) { + // Make sure this stream disabling is explicitly signaled. + updated_bitrate->SetBitrate(si, ti, 0); } } } - return false; + return updated_bitrate; } bool RTCPSender::SendFeedbackPacket(const rtcp::TransportFeedback& packet) { diff --git a/modules/rtp_rtcp/source/rtcp_sender.h b/modules/rtp_rtcp/source/rtcp_sender.h index 10cc1ec465..c9220dd35c 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.h +++ b/modules/rtp_rtcp/source/rtcp_sender.h @@ -244,7 +244,8 @@ class RTCPSender { RTC_GUARDED_BY(critical_section_rtcp_sender_); bool send_video_bitrate_allocation_ RTC_GUARDED_BY(critical_section_rtcp_sender_); - bool HasNewLayerStructure(const VideoBitrateAllocation& bitrate) const + absl::optional CheckAndUpdateLayerStructure( + const VideoBitrateAllocation& bitrate) const RTC_EXCLUSIVE_LOCKS_REQUIRED(critical_section_rtcp_sender_); void SetFlag(uint32_t type, bool is_volatile) diff --git a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc index 850d48760e..bcffc81213 100644 --- a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc @@ -681,4 +681,41 @@ TEST_F(RtcpSenderTest, SendImmediateXrWithTargetBitrate) { EXPECT_TRUE(rtcp_sender_->TimeToSendRTCPReport(false)); } +TEST_F(RtcpSenderTest, SendTargetBitrateExplicitZeroOnStreamRemoval) { + // Set up and send a bitrate allocation with two layers. + + rtcp_sender_->SetRTCPStatus(RtcpMode::kCompound); + VideoBitrateAllocation allocation; + allocation.SetBitrate(0, 0, 100000); + allocation.SetBitrate(1, 0, 200000); + rtcp_sender_->SetVideoBitrateAllocation(allocation); + EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpReport)); + absl::optional target_bitrate = + parser()->xr()->target_bitrate(); + ASSERT_TRUE(target_bitrate); + std::vector bitrates = + target_bitrate->GetTargetBitrates(); + ASSERT_EQ(2u, bitrates.size()); + EXPECT_EQ(bitrates[0].target_bitrate_kbps, + allocation.GetBitrate(0, 0) / 1000); + EXPECT_EQ(bitrates[1].target_bitrate_kbps, + allocation.GetBitrate(1, 0) / 1000); + + // Create a new allocation, where the second stream is no longer available. + VideoBitrateAllocation new_allocation; + new_allocation.SetBitrate(0, 0, 150000); + rtcp_sender_->SetVideoBitrateAllocation(new_allocation); + EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpReport)); + target_bitrate = parser()->xr()->target_bitrate(); + ASSERT_TRUE(target_bitrate); + bitrates = target_bitrate->GetTargetBitrates(); + + // Two bitrates should still be set, with an explicit entry indicating the + // removed stream is gone. + ASSERT_EQ(2u, bitrates.size()); + EXPECT_EQ(bitrates[0].target_bitrate_kbps, + new_allocation.GetBitrate(0, 0) / 1000); + EXPECT_EQ(bitrates[1].target_bitrate_kbps, 0u); +} + } // namespace webrtc