From 3aa25de346661e61d1f443a65e2fb938e2c822b3 Mon Sep 17 00:00:00 2001 From: "pwestin@webrtc.org" Date: Thu, 5 Jan 2012 08:40:56 +0000 Subject: [PATCH] Bugfix OnNetworkChanged not triggered for RTCP compund messages if TMMBR is higher than last value. TBR=mflodman Review URL: http://webrtc-codereview.appspot.com/342001 git-svn-id: http://webrtc.googlecode.com/svn/trunk@1344 4adac7df-926f-26a2-2b94-8c16560cd09d --- src/modules/rtp_rtcp/source/rtcp_receiver.cc | 67 ++++++++++---------- src/modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 48 +++++--------- src/modules/rtp_rtcp/source/rtp_rtcp_impl.h | 5 +- 3 files changed, 51 insertions(+), 69 deletions(-) diff --git a/src/modules/rtp_rtcp/source/rtcp_receiver.cc b/src/modules/rtp_rtcp/source/rtcp_receiver.cc index 7c209271b..6021955d3 100644 --- a/src/modules/rtp_rtcp/source/rtcp_receiver.cc +++ b/src/modules/rtp_rtcp/source/rtcp_receiver.cc @@ -1301,29 +1301,37 @@ RTCPReceiver::OnReceivedReferencePictureSelectionIndication(const WebRtc_UWord64 } // Holding no Critical section -void -RTCPReceiver::TriggerCallbacksFromRTCPPacket(RTCPPacketInformation& rtcpPacketInformation) +void RTCPReceiver::TriggerCallbacksFromRTCPPacket( + RTCPPacketInformation& rtcpPacketInformation) { - // callback if SR or RR + // Process TMMBR and REMB first to avoid multiple callbacks + // to OnNetworkChanged. + if (rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpTmmbr) + { + WEBRTC_TRACE(kTraceStateInfo, kTraceRtpRtcp, _id, + "SIG [RTCP] Incoming TMMBR to id:%d", _id); + + // Might trigger a OnReceivedBandwidthEstimateUpdate. + _rtpRtcp.OnReceivedTMMBR(); + } + if (rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpRemb) + { + WEBRTC_TRACE(kTraceStateInfo, kTraceRtpRtcp, _id, + "SIG [RTCP] Incoming REMB to id:%d", _id); + + // We need to bounce this to the default channel. + _rtpRtcp.OnReceivedEstimatedMaxBitrate( + rtcpPacketInformation.receiverEstimatedMaxBitrate); + } if (rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpSr || rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpRr) { - if(rtcpPacketInformation.reportBlock) + if (rtcpPacketInformation.reportBlock) { - // We only want to trigger one OnNetworkChanged callback per RTCP - // packet. The callback is triggered by a SR, RR, REMB or TMMBR, so - // we don't want to trigger one from here if the packet also - // contains a REMB or TMMBR block. - bool triggerCallback = true; - if (rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpRemb || - rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpTmmbr) { - triggerCallback = false; - } _rtpRtcp.OnPacketLossStatisticsUpdate( rtcpPacketInformation.fractionLost, rtcpPacketInformation.roundTripTime, - rtcpPacketInformation.lastReceivedExtendedHighSeqNum, - triggerCallback); + rtcpPacketInformation.lastReceivedExtendedHighSeqNum); } } if (rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpSr) @@ -1338,27 +1346,24 @@ RTCPReceiver::TriggerCallbacksFromRTCPPacket(RTCPPacketInformation& rtcpPacketIn { if (rtcpPacketInformation.nackSequenceNumbersLength > 0) { - WEBRTC_TRACE(kTraceStateInfo, kTraceRtpRtcp, _id, "SIG [RTCP] Incoming NACK to id:%d", _id); - _rtpRtcp.OnReceivedNACK(rtcpPacketInformation.nackSequenceNumbersLength, - rtcpPacketInformation.nackSequenceNumbers); + WEBRTC_TRACE(kTraceStateInfo, kTraceRtpRtcp, _id, + "SIG [RTCP] Incoming NACK to id:%d", _id); + _rtpRtcp.OnReceivedNACK( + rtcpPacketInformation.nackSequenceNumbersLength, + rtcpPacketInformation.nackSequenceNumbers); } } - if (rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpTmmbr) - { - WEBRTC_TRACE(kTraceStateInfo, kTraceRtpRtcp, _id, "SIG [RTCP] Incoming TMMBR to id:%d", _id); - - // might trigger a OnReceivedBandwidthEstimateUpdate - _rtpRtcp.OnReceivedTMMBR(); - } if ((rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpPli) || (rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpFir)) { if (rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpPli) { - WEBRTC_TRACE(kTraceStateInfo, kTraceRtpRtcp, _id, "SIG [RTCP] Incoming PLI to id:%d", _id); - }else + WEBRTC_TRACE(kTraceStateInfo, kTraceRtpRtcp, _id, + "SIG [RTCP] Incoming PLI to id:%d", _id); + } else { - WEBRTC_TRACE(kTraceStateInfo, kTraceRtpRtcp, _id, "SIG [RTCP] Incoming FIR to id:%d", _id); + WEBRTC_TRACE(kTraceStateInfo, kTraceRtpRtcp, _id, + "SIG [RTCP] Incoming FIR to id:%d", _id); } _rtpRtcp.OnReceivedIntraFrameRequest(&_rtpRtcp); } @@ -1368,12 +1373,6 @@ RTCPReceiver::TriggerCallbacksFromRTCPPacket(RTCPPacketInformation& rtcpPacketIn _rtpRtcp.OnReceivedSliceLossIndication( rtcpPacketInformation.sliPictureId); } - if (rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpRemb) - { - // We need to bounce this to the default channel. - _rtpRtcp.OnReceivedEstimatedMaxBitrate( - rtcpPacketInformation.receiverEstimatedMaxBitrate); - } if (rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpRpsi) { // we need use a bounce it up to handle default channel diff --git a/src/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/src/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 03a66904d..5a33f0d9c 100644 --- a/src/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/src/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -2719,7 +2719,7 @@ void ModuleRtpRtcpImpl::OnReceivedBandwidthEstimateUpdate( // We received a TMMBR const bool defaultInstance(_childModules.empty() ? false : true); if (defaultInstance) { - ProcessDefaultModuleBandwidth(true); + ProcessDefaultModuleBandwidth(); return; } if (_audio) { @@ -2760,8 +2760,7 @@ void ModuleRtpRtcpImpl::OnReceivedBandwidthEstimateUpdate( void ModuleRtpRtcpImpl::OnPacketLossStatisticsUpdate( const WebRtc_UWord8 fractionLost, const WebRtc_UWord16 roundTripTime, - const WebRtc_UWord32 lastReceivedExtendedHighSeqNum, - bool triggerOnNetworkChanged) { + const WebRtc_UWord32 lastReceivedExtendedHighSeqNum) { const bool defaultInstance(_childModules.empty() ? false : true); if (!defaultInstance) { @@ -2797,22 +2796,16 @@ void ModuleRtpRtcpImpl::OnPacketLossStatisticsUpdate( _defaultModule->OnPacketLossStatisticsUpdate( loss, // send in the filtered loss roundTripTime, - lastReceivedExtendedHighSeqNum, - triggerOnNetworkChanged); + lastReceivedExtendedHighSeqNum); } return; } - // No default module check if we should trigger OnNetworkChanged - // via video callback - if (triggerOnNetworkChanged) - { - _rtpReceiver.UpdateBandwidthManagement(newBitrate, - fractionLost, - roundTripTime); - } + _rtpReceiver.UpdateBandwidthManagement(newBitrate, + fractionLost, + roundTripTime); } else { if (!_simulcast) { - ProcessDefaultModuleBandwidth(triggerOnNetworkChanged); + ProcessDefaultModuleBandwidth(); } else { // default and simulcast WebRtc_UWord32 newBitrate = 0; @@ -2831,14 +2824,9 @@ void ModuleRtpRtcpImpl::OnPacketLossStatisticsUpdate( return; } _rtpSender.SetTargetSendBitrate(newBitrate); - // check if we should trigger OnNetworkChanged - // via video callback - if (triggerOnNetworkChanged) - { - _rtpReceiver.UpdateBandwidthManagement(newBitrate, - loss, - roundTripTime); - } + _rtpReceiver.UpdateBandwidthManagement(newBitrate, + loss, + roundTripTime); // sanity if (_sendVideoCodec.codecType == kVideoCodecUnknown) { return; @@ -2875,8 +2863,7 @@ void ModuleRtpRtcpImpl::OnPacketLossStatisticsUpdate( } } -void ModuleRtpRtcpImpl::ProcessDefaultModuleBandwidth( - bool triggerOnNetworkChanged) { +void ModuleRtpRtcpImpl::ProcessDefaultModuleBandwidth() { WebRtc_UWord32 minBitrateBps = 0xffffffff; WebRtc_UWord32 maxBitrateBps = 0; @@ -2933,14 +2920,11 @@ void ModuleRtpRtcpImpl::ProcessDefaultModuleBandwidth( } _bandwidthManagement.SetSendBitrate(minBitrateBps, 0, 0); - if (triggerOnNetworkChanged) { - // Update default module bitrate. Don't care about min max. - // Check if we should trigger OnNetworkChanged via video callback - WebRtc_UWord8 fractionLostAvg = WebRtc_UWord8(fractionLostAcc / count); - _rtpReceiver.UpdateBandwidthManagement(minBitrateBps, - fractionLostAvg , - maxRoundTripTime); - } + // Update default module bitrate. Don't care about min max. + WebRtc_UWord8 fractionLostAvg = WebRtc_UWord8(fractionLostAcc / count); + _rtpReceiver.UpdateBandwidthManagement(minBitrateBps, + fractionLostAvg , + maxRoundTripTime); } void ModuleRtpRtcpImpl::OnRequestSendReport() { diff --git a/src/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/src/modules/rtp_rtcp/source/rtp_rtcp_impl.h index e8222cf73..cec02cdeb 100644 --- a/src/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/src/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -508,8 +508,7 @@ public: void OnPacketLossStatisticsUpdate( const WebRtc_UWord8 fractionLost, const WebRtc_UWord16 roundTripTime, - const WebRtc_UWord32 lastReceivedExtendedHighSeqNum, - bool triggerOnNetworkChanged); + const WebRtc_UWord32 lastReceivedExtendedHighSeqNum); void OnReceivedTMMBR(); @@ -562,7 +561,7 @@ protected: RtpRtcpClock& _clock; private: void SendKeyFrame(); - void ProcessDefaultModuleBandwidth(bool triggerOnNetworkChanged); + void ProcessDefaultModuleBandwidth(); WebRtc_Word32 _id; const bool _audio;