From d44ce0563f39b5357e050a57cbdf91e34d45bdaa Mon Sep 17 00:00:00 2001 From: nisse Date: Mon, 6 Feb 2017 02:23:00 -0800 Subject: [PATCH] Reland of Always call RemoteBitrateEstimator::IncomingPacket from Call. (patchset #1 id:1 of https://codereview.webrtc.org/2668973003/ ) Reason for revert: Intending to fix issues and reland. Original issue's description: > 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} > Committed: https://chromium.googlesource.com/external/webrtc/+/14245cc9397284cabab5057c0c661953e2d0cdec TBR=stefan@webrtc.org,brandtr@webrtc.org BUG=webrtc:6847 Review-Url: https://codereview.webrtc.org/2673523003 Cr-Commit-Position: refs/heads/master@{#16440} --- webrtc/audio/audio_receive_stream.cc | 13 -- webrtc/audio/audio_receive_stream_unittest.cc | 16 --- webrtc/call/call.cc | 124 +++++++++++++----- webrtc/video/rtp_stream_receiver.cc | 3 - 4 files changed, 91 insertions(+), 65 deletions(-) diff --git a/webrtc/audio/audio_receive_stream.cc b/webrtc/audio/audio_receive_stream.cc index 1f24b2ca27..17da10f357 100644 --- a/webrtc/audio/audio_receive_stream.cc +++ b/webrtc/audio/audio_receive_stream.cc @@ -330,19 +330,6 @@ 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 4c443e0f2e..8df66fe127 100644 --- a/webrtc/audio/audio_receive_stream_unittest.cc +++ b/webrtc/audio/audio_receive_stream_unittest.cc @@ -248,13 +248,6 @@ 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; @@ -267,15 +260,6 @@ 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 6aa564e95e..6fd0f6d74a 100644 --- a/webrtc/call/call.cc +++ b/webrtc/call/call.cc @@ -109,8 +109,6 @@ 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; @@ -145,6 +143,10 @@ class Call : public webrtc::Call, void ConfigureSync(const std::string& sync_group) EXCLUSIVE_LOCKS_REQUIRED(receive_crit_); + void NotifyBweOfReceivedPacket(const RtpPacketReceived& packet, + MediaType media_type) + SHARED_LOCKS_REQUIRED(receive_crit_); + rtc::Optional ParseRtpPacket(const uint8_t* packet, size_t length, const PacketTime& packet_time) @@ -188,12 +190,27 @@ class Call : public webrtc::Call, std::map sync_stream_mapping_ GUARDED_BY(receive_crit_); - // 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_ + // 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_ GUARDED_BY(receive_crit_); std::unique_ptr send_crit_; @@ -357,9 +374,9 @@ rtc::Optional Call::ParseRtpPacket( if (!parsed_packet.Parse(packet, length)) return rtc::Optional(); - auto it = received_rtp_header_extensions_.find(parsed_packet.Ssrc()); - if (it != received_rtp_header_extensions_.end()) - parsed_packet.IdentifyExtensions(it->second); + auto it = receive_rtp_config_.find(parsed_packet.Ssrc()); + if (it != receive_rtp_config_.end()) + parsed_packet.IdentifyExtensions(it->second.extensions); int64_t arrival_time_ms; if (packet_time.timestamp != -1) { @@ -509,7 +526,6 @@ 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_); { @@ -517,6 +533,9 @@ 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); } { @@ -540,8 +559,9 @@ void Call::DestroyAudioReceiveStream( static_cast(receive_stream); { WriteLockScoped write_lock(*receive_crit_); - size_t num_deleted = audio_receive_ssrcs_.erase( - audio_receive_stream->config().rtp.remote_ssrc); + uint32_t ssrc = audio_receive_stream->config().rtp.remote_ssrc; + + size_t num_deleted = audio_receive_ssrcs_.erase(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); @@ -550,6 +570,7 @@ void Call::DestroyAudioReceiveStream( sync_stream_mapping_.erase(it); ConfigureSync(sync_group); } + receive_rtp_config_.erase(ssrc); } UpdateAggregateNetworkState(); delete audio_receive_stream; @@ -642,13 +663,22 @@ 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); } @@ -674,7 +704,8 @@ void Call::DestroyVideoReceiveStream( if (receive_stream_impl != nullptr) RTC_DCHECK(receive_stream_impl == it->second); receive_stream_impl = it->second; - video_receive_ssrcs_.erase(it++); + receive_rtp_config_.erase(it->first); + it = video_receive_ssrcs_.erase(it); } else { ++it; } @@ -711,10 +742,10 @@ FlexfecReceiveStream* Call::CreateFlexfecReceiveStream( flexfec_receive_ssrcs_protection_.end()); flexfec_receive_ssrcs_protection_[config.remote_ssrc] = receive_stream; - 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; + 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); } // TODO(brandtr): Store config in RtcEventLog here. @@ -735,7 +766,7 @@ void Call::DestroyFlexfecReceiveStream(FlexfecReceiveStream* receive_stream) { WriteLockScoped write_lock(*receive_crit_); uint32_t ssrc = receive_stream_impl->GetConfig().remote_ssrc; - received_rtp_header_extensions_.erase(ssrc); + receive_rtp_config_.erase(ssrc); // Remove all SSRCs pointing to the FlexfecReceiveStreamImpl to be // destroyed. @@ -1108,12 +1139,20 @@ PacketReceiver::DeliveryStatus Call::DeliverRtp(MediaType media_type, size_t length, const PacketTime& packet_time) { TRACE_EVENT0("webrtc", "Call::DeliverRtp"); - // Minimum RTP header size. - if (length < 12) + + 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) return DELIVERY_PACKET_ERROR; - uint32_t ssrc = ByteReader::ReadBigEndian(&packet[8]); - ReadLockScoped read_lock(*receive_crit_); + NotifyBweOfReceivedPacket(*parsed_packet, media_type); + + uint32_t ssrc = parsed_packet->Ssrc(); + if (media_type == MediaType::ANY || media_type == MediaType::AUDIO) { auto it = audio_receive_ssrcs_.find(ssrc); if (it != audio_receive_ssrcs_.end()) { @@ -1140,8 +1179,6 @@ 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) @@ -1155,10 +1192,7 @@ 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; @@ -1197,11 +1231,35 @@ bool Call::OnRecoveredPacket(const uint8_t* packet, size_t length) { return it->second->OnRecoveredPacket(packet, length); } -void Call::NotifyBweOfReceivedPacket(const RtpPacketReceived& packet) { +void Call::NotifyBweOfReceivedPacket(const RtpPacketReceived& packet, + MediaType media_type) { + 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); - congestion_controller_->OnReceivedPacket(packet.arrival_time_ms(), - packet.payload_size(), header); + + if (!transport_cc && header.extension.hasTransportSequenceNumber) { + // Inconsistent configuration of send side BWE. Do nothing. + // TODO(nisse): Without this check, we may produce RTCP feedback + // packets even when not negotiated. But it would be cleaner to + // move the check down to RTCPSender::SendFeedbackPacket, which + // would also help the PacketRouter to select an appropriate rtp + // module in the case that some, but not all, have RTCP feedback + // enabled. + return; + } + // For audio, we only support send side BWE. + // TODO(nisse): Tests passes MediaType::ANY, see + // FakeNetworkPipe::Process. We need to treat that as video. Tests + // should be fixed to use the same MediaType as the production code. + if (media_type != MediaType::AUDIO || + (transport_cc && header.extension.hasTransportSequenceNumber)) { + congestion_controller_->OnReceivedPacket( + packet.arrival_time_ms(), packet.payload_size() + packet.padding_size(), + header); + } } } // namespace internal diff --git a/webrtc/video/rtp_stream_receiver.cc b/webrtc/video/rtp_stream_receiver.cc index 8064e876be..50c50d08ce 100644 --- a/webrtc/video/rtp_stream_receiver.cc +++ b/webrtc/video/rtp_stream_receiver.cc @@ -322,7 +322,6 @@ 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) @@ -348,8 +347,6 @@ 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);