From 14245cc9397284cabab5057c0c661953e2d0cdec Mon Sep 17 00:00:00 2001 From: nisse Date: Wed, 1 Feb 2017 08:10:36 -0800 Subject: [PATCH] Revert of Always call RemoteBitrateEstimator::IncomingPacket from Call. (patchset #9 id:160001 of https://codereview.webrtc.org/2659563002/ ) Reason for revert: This change causes excessive logging when running tests, and possibly also broke perf tests, see https://build.chromium.org/p/client.webrtc.perf/builders/Linux%20Trusty/builds/1040/steps/webrtc_perf_tests/logs/stdio Original issue's description: > Always call RemoteBitrateEstimator::IncomingPacket from Call. > > Delete the calls from RtpStreamReceiver (for video) and > AudioReceiveStream. > > BUG=webrtc:6847 > > Review-Url: https://codereview.webrtc.org/2659563002 > Cr-Commit-Position: refs/heads/master@{#16393} > Committed: https://chromium.googlesource.com/external/webrtc/+/6d4dd593a824cfc9c67f6e9fd3a08014d08350a0 TBR=stefan@webrtc.org,brandtr@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=webrtc:6847 Review-Url: https://codereview.webrtc.org/2668973003 Cr-Commit-Position: refs/heads/master@{#16400} --- webrtc/audio/audio_receive_stream.cc | 13 +++ webrtc/audio/audio_receive_stream_unittest.cc | 16 +++ webrtc/call/call.cc | 106 +++++------------- webrtc/video/rtp_stream_receiver.cc | 3 + 4 files changed, 62 insertions(+), 76 deletions(-) diff --git a/webrtc/audio/audio_receive_stream.cc b/webrtc/audio/audio_receive_stream.cc index 17da10f357..1f24b2ca27 100644 --- a/webrtc/audio/audio_receive_stream.cc +++ b/webrtc/audio/audio_receive_stream.cc @@ -330,6 +330,19 @@ bool AudioReceiveStream::DeliverRtp(const uint8_t* packet, return false; } + // Only forward if the parsed header has one of the headers necessary for + // bandwidth estimation. RTP timestamps has different rates for audio and + // video and shouldn't be mixed. + if (config_.rtp.transport_cc && + header.extension.hasTransportSequenceNumber) { + int64_t arrival_time_ms = rtc::TimeMillis(); + if (packet_time.timestamp >= 0) + arrival_time_ms = (packet_time.timestamp + 500) / 1000; + size_t payload_size = length - header.headerLength; + remote_bitrate_estimator_->IncomingPacket(arrival_time_ms, payload_size, + header); + } + return channel_proxy_->ReceivedRTPPacket(packet, length, packet_time); } diff --git a/webrtc/audio/audio_receive_stream_unittest.cc b/webrtc/audio/audio_receive_stream_unittest.cc index 8df66fe127..4c443e0f2e 100644 --- a/webrtc/audio/audio_receive_stream_unittest.cc +++ b/webrtc/audio/audio_receive_stream_unittest.cc @@ -248,6 +248,13 @@ TEST(AudioReceiveStreamTest, ConstructDestruct) { helper.config(), helper.audio_state(), helper.event_log()); } +MATCHER_P(VerifyHeaderExtension, expected_extension, "") { + return arg.extension.hasTransportSequenceNumber == + expected_extension.hasTransportSequenceNumber && + arg.extension.transportSequenceNumber == + expected_extension.transportSequenceNumber; +} + TEST(AudioReceiveStreamTest, ReceiveRtpPacket) { ConfigHelper helper; helper.config().rtp.transport_cc = true; @@ -260,6 +267,15 @@ TEST(AudioReceiveStreamTest, ReceiveRtpPacket) { std::vector rtp_packet = CreateRtpHeaderWithOneByteExtension( kTransportSequenceNumberId, kTransportSequenceNumberValue, 2); PacketTime packet_time(5678000, 0); + const size_t kExpectedHeaderLength = 20; + RTPHeaderExtension expected_extension; + expected_extension.hasTransportSequenceNumber = true; + expected_extension.transportSequenceNumber = kTransportSequenceNumberValue; + EXPECT_CALL(*helper.remote_bitrate_estimator(), + IncomingPacket(packet_time.timestamp / 1000, + rtp_packet.size() - kExpectedHeaderLength, + VerifyHeaderExtension(expected_extension))) + .Times(1); EXPECT_CALL(*helper.channel_proxy(), ReceivedRTPPacket(&rtp_packet[0], rtp_packet.size(), diff --git a/webrtc/call/call.cc b/webrtc/call/call.cc index 37b5c6a4fb..6aa564e95e 100644 --- a/webrtc/call/call.cc +++ b/webrtc/call/call.cc @@ -109,6 +109,8 @@ class Call : public webrtc::Call, // Implements RecoveredPacketReceiver. bool OnRecoveredPacket(const uint8_t* packet, size_t length) override; + void NotifyBweOfReceivedPacket(const RtpPacketReceived& packet); + void SetBitrateConfig( const webrtc::Call::Config::BitrateConfig& bitrate_config) override; @@ -143,9 +145,6 @@ class Call : public webrtc::Call, void ConfigureSync(const std::string& sync_group) EXCLUSIVE_LOCKS_REQUIRED(receive_crit_); - void NotifyBweOfReceivedPacket(const RtpPacketReceived& packet) - SHARED_LOCKS_REQUIRED(receive_crit_); - rtc::Optional ParseRtpPacket(const uint8_t* packet, size_t length, const PacketTime& packet_time) @@ -189,27 +188,12 @@ class Call : public webrtc::Call, std::map sync_stream_mapping_ GUARDED_BY(receive_crit_); - // This extra map is used for receive processing which is - // independent of media type. - - // TODO(nisse): In the RTP transport refactoring, we should have a - // single mapping from ssrc to a more abstract receive stream, with - // accessor methods for all configuration we need at this level. - struct ReceiveRtpConfig { - ReceiveRtpConfig() = default; // Needed by std::map - ReceiveRtpConfig(const std::vector& extensions, - bool transport_cc) - : extensions(extensions), transport_cc(transport_cc) {} - - // Registered RTP header extensions for each stream. Note that RTP header - // extensions are negotiated per track ("m= line") in the SDP, but we have - // no notion of tracks at the Call level. We therefore store the RTP header - // extensions per SSRC instead, which leads to some storage overhead. - RtpHeaderExtensionMap extensions; - // Set if the RTCP feedback message needed for send side BWE was negotiated. - bool transport_cc; - }; - std::map receive_rtp_config_ + // Registered RTP header extensions for each stream. + // Note that RTP header extensions are negotiated per track ("m= line") in the + // SDP, but we have no notion of tracks at the Call level. We therefore store + // the RTP header extensions per SSRC instead, which leads to some storage + // overhead. + std::map received_rtp_header_extensions_ GUARDED_BY(receive_crit_); std::unique_ptr send_crit_; @@ -373,9 +357,9 @@ rtc::Optional Call::ParseRtpPacket( if (!parsed_packet.Parse(packet, length)) return rtc::Optional(); - auto it = receive_rtp_config_.find(parsed_packet.Ssrc()); - if (it != receive_rtp_config_.end()) - parsed_packet.IdentifyExtensions(it->second.extensions); + auto it = received_rtp_header_extensions_.find(parsed_packet.Ssrc()); + if (it != received_rtp_header_extensions_.end()) + parsed_packet.IdentifyExtensions(it->second); int64_t arrival_time_ms; if (packet_time.timestamp != -1) { @@ -525,6 +509,7 @@ webrtc::AudioReceiveStream* Call::CreateAudioReceiveStream( event_log_->LogAudioReceiveStreamConfig(config); AudioReceiveStream* receive_stream = new AudioReceiveStream( &packet_router_, + // TODO(nisse): Used only when UseSendSideBwe(config) is true. congestion_controller_->GetRemoteBitrateEstimator(true), config, config_.audio_state, event_log_); { @@ -532,9 +517,6 @@ webrtc::AudioReceiveStream* Call::CreateAudioReceiveStream( RTC_DCHECK(audio_receive_ssrcs_.find(config.rtp.remote_ssrc) == audio_receive_ssrcs_.end()); audio_receive_ssrcs_[config.rtp.remote_ssrc] = receive_stream; - receive_rtp_config_[config.rtp.remote_ssrc] = - ReceiveRtpConfig(config.rtp.extensions, config.rtp.transport_cc); - ConfigureSync(config.sync_group); } { @@ -558,9 +540,8 @@ void Call::DestroyAudioReceiveStream( static_cast(receive_stream); { WriteLockScoped write_lock(*receive_crit_); - uint32_t ssrc = audio_receive_stream->config().rtp.remote_ssrc; - - size_t num_deleted = audio_receive_ssrcs_.erase(ssrc); + size_t num_deleted = audio_receive_ssrcs_.erase( + audio_receive_stream->config().rtp.remote_ssrc); RTC_DCHECK(num_deleted == 1); const std::string& sync_group = audio_receive_stream->config().sync_group; const auto it = sync_stream_mapping_.find(sync_group); @@ -569,7 +550,6 @@ void Call::DestroyAudioReceiveStream( sync_stream_mapping_.erase(it); ConfigureSync(sync_group); } - receive_rtp_config_.erase(ssrc); } UpdateAggregateNetworkState(); delete audio_receive_stream; @@ -662,22 +642,13 @@ webrtc::VideoReceiveStream* Call::CreateVideoReceiveStream( call_stats_.get(), &remb_); const webrtc::VideoReceiveStream::Config& config = receive_stream->config(); - ReceiveRtpConfig receive_config(config.rtp.extensions, - config.rtp.transport_cc); { WriteLockScoped write_lock(*receive_crit_); RTC_DCHECK(video_receive_ssrcs_.find(config.rtp.remote_ssrc) == video_receive_ssrcs_.end()); video_receive_ssrcs_[config.rtp.remote_ssrc] = receive_stream; - if (config.rtp.rtx_ssrc) { + if (config.rtp.rtx_ssrc) video_receive_ssrcs_[config.rtp.rtx_ssrc] = receive_stream; - // We record identical config for the rtx stream as for the main - // stream. Since the transport_cc negotiation is per payload - // type, we may get an incorrect value for the rtx stream, but - // that is unlikely to matter in practice. - receive_rtp_config_[config.rtp.rtx_ssrc] = receive_config; - } - receive_rtp_config_[config.rtp.remote_ssrc] = receive_config; video_receive_streams_.insert(receive_stream); ConfigureSync(config.sync_group); } @@ -703,8 +674,7 @@ void Call::DestroyVideoReceiveStream( if (receive_stream_impl != nullptr) RTC_DCHECK(receive_stream_impl == it->second); receive_stream_impl = it->second; - receive_rtp_config_.erase(it->first); - it = video_receive_ssrcs_.erase(it); + video_receive_ssrcs_.erase(it++); } else { ++it; } @@ -741,10 +711,10 @@ FlexfecReceiveStream* Call::CreateFlexfecReceiveStream( flexfec_receive_ssrcs_protection_.end()); flexfec_receive_ssrcs_protection_[config.remote_ssrc] = receive_stream; - RTC_DCHECK(receive_rtp_config_.find(config.remote_ssrc) == - receive_rtp_config_.end()); - receive_rtp_config_[config.remote_ssrc] = - ReceiveRtpConfig(config.rtp_header_extensions, config.transport_cc); + RTC_DCHECK(received_rtp_header_extensions_.find(config.remote_ssrc) == + received_rtp_header_extensions_.end()); + RtpHeaderExtensionMap rtp_header_extensions(config.rtp_header_extensions); + received_rtp_header_extensions_[config.remote_ssrc] = rtp_header_extensions; } // TODO(brandtr): Store config in RtcEventLog here. @@ -765,7 +735,7 @@ void Call::DestroyFlexfecReceiveStream(FlexfecReceiveStream* receive_stream) { WriteLockScoped write_lock(*receive_crit_); uint32_t ssrc = receive_stream_impl->GetConfig().remote_ssrc; - receive_rtp_config_.erase(ssrc); + received_rtp_header_extensions_.erase(ssrc); // Remove all SSRCs pointing to the FlexfecReceiveStreamImpl to be // destroyed. @@ -1138,20 +1108,12 @@ PacketReceiver::DeliveryStatus Call::DeliverRtp(MediaType media_type, size_t length, const PacketTime& packet_time) { TRACE_EVENT0("webrtc", "Call::DeliverRtp"); - - ReadLockScoped read_lock(*receive_crit_); - // TODO(nisse): We should parse the RTP header only here, and pass - // on parsed_packet to the receive streams. - rtc::Optional parsed_packet = - ParseRtpPacket(packet, length, packet_time); - - if (!parsed_packet) + // Minimum RTP header size. + if (length < 12) return DELIVERY_PACKET_ERROR; - NotifyBweOfReceivedPacket(*parsed_packet); - - uint32_t ssrc = parsed_packet->Ssrc(); - + uint32_t ssrc = ByteReader::ReadBigEndian(&packet[8]); + ReadLockScoped read_lock(*receive_crit_); if (media_type == MediaType::ANY || media_type == MediaType::AUDIO) { auto it = audio_receive_ssrcs_.find(ssrc); if (it != audio_receive_ssrcs_.end()) { @@ -1178,6 +1140,8 @@ PacketReceiver::DeliveryStatus Call::DeliverRtp(MediaType media_type, // not be parsed, as FlexFEC is oblivious to the semantic meaning of the // packet contents beyond the 12 byte RTP base header. The BWE is fed // information about these media packets from the regular media pipeline. + rtc::Optional parsed_packet = + ParseRtpPacket(packet, length, packet_time); if (parsed_packet) { auto it_bounds = flexfec_receive_ssrcs_media_.equal_range(ssrc); for (auto it = it_bounds.first; it != it_bounds.second; ++it) @@ -1191,7 +1155,10 @@ PacketReceiver::DeliveryStatus Call::DeliverRtp(MediaType media_type, if (media_type == MediaType::ANY || media_type == MediaType::VIDEO) { auto it = flexfec_receive_ssrcs_protection_.find(ssrc); if (it != flexfec_receive_ssrcs_protection_.end()) { + rtc::Optional parsed_packet = + ParseRtpPacket(packet, length, packet_time); if (parsed_packet) { + NotifyBweOfReceivedPacket(*parsed_packet); auto status = it->second->AddAndProcessReceivedPacket(*parsed_packet) ? DELIVERY_OK : DELIVERY_PACKET_ERROR; @@ -1231,21 +1198,8 @@ bool Call::OnRecoveredPacket(const uint8_t* packet, size_t length) { } void Call::NotifyBweOfReceivedPacket(const RtpPacketReceived& packet) { - auto it = receive_rtp_config_.find(packet.Ssrc()); - bool transport_cc = - (it != receive_rtp_config_.end()) && it->second.transport_cc; - RTPHeader header; packet.GetHeader(&header); - - // transport_cc represents the negotiation of the RTCP feedback - // message used for send side BWE. If it was negotiated but the - // corresponding RTP header extension is not present, or vice versa, - // bandwidth estimation is not correctly configured. - if (transport_cc != header.extension.hasTransportSequenceNumber) { - LOG(LS_ERROR) << "Inconsistent configuration of send side BWE."; - return; - } congestion_controller_->OnReceivedPacket(packet.arrival_time_ms(), packet.payload_size(), header); } diff --git a/webrtc/video/rtp_stream_receiver.cc b/webrtc/video/rtp_stream_receiver.cc index c1f50396ee..63c7d463c8 100644 --- a/webrtc/video/rtp_stream_receiver.cc +++ b/webrtc/video/rtp_stream_receiver.cc @@ -336,6 +336,7 @@ bool RtpStreamReceiver::DeliverRtp(const uint8_t* rtp_packet, &header)) { return false; } + size_t payload_length = rtp_packet_length - header.headerLength; int64_t arrival_time_ms; int64_t now_ms = clock_->TimeInMilliseconds(); if (packet_time.timestamp != -1) @@ -361,6 +362,8 @@ bool RtpStreamReceiver::DeliverRtp(const uint8_t* rtp_packet, } } + remote_bitrate_estimator_->IncomingPacket(arrival_time_ms, payload_length, + header); header.payload_type_frequency = kVideoPayloadTypeFrequency; bool in_order = IsPacketInOrder(header);