From 38cc1d6b31eff566651bc1d09b8a4e9093bcb90d Mon Sep 17 00:00:00 2001 From: nisse Date: Mon, 13 Feb 2017 05:59:46 -0800 Subject: [PATCH] Replace RtpStreamReceiver::DeliverRtp with OnRtpPacket. This avoids redoing RTP header parsing already done in Call, for video. The next step is to convert other types of receive streams, i.e., audio and flexfec, to use a compatible OnRtpPacket method. We can then introduce a shared base interface, and simplify media-independent receive processing in Call. BUG=webrtc:7135 Review-Url: https://codereview.webrtc.org/2681673004 Cr-Commit-Position: refs/heads/master@{#16583} --- webrtc/call/BUILD.gn | 1 - webrtc/call/call.cc | 39 +++----- webrtc/call/packet_injection_tests.cc | 93 ------------------- .../source/rtp_format_h264_unittest.cc | 6 ++ webrtc/video/rtp_stream_receiver.cc | 50 +++++----- webrtc/video/rtp_stream_receiver.h | 7 +- webrtc/video/video_receive_stream.cc | 6 +- webrtc/video/video_receive_stream.h | 6 +- 8 files changed, 55 insertions(+), 153 deletions(-) delete mode 100644 webrtc/call/packet_injection_tests.cc diff --git a/webrtc/call/BUILD.gn b/webrtc/call/BUILD.gn index 382ae2e66b..680ec6bd2f 100644 --- a/webrtc/call/BUILD.gn +++ b/webrtc/call/BUILD.gn @@ -61,7 +61,6 @@ if (rtc_include_tests) { "bitrate_estimator_tests.cc", "call_unittest.cc", "flexfec_receive_stream_unittest.cc", - "packet_injection_tests.cc", ] deps = [ ":call", diff --git a/webrtc/call/call.cc b/webrtc/call/call.cc index e21b0762fe..ec8053768d 100644 --- a/webrtc/call/call.cc +++ b/webrtc/call/call.cc @@ -1211,35 +1211,26 @@ PacketReceiver::DeliveryStatus Call::DeliverRtp(MediaType media_type, if (it != video_receive_ssrcs_.end()) { received_bytes_per_second_counter_.Add(static_cast(length)); received_video_bytes_per_second_counter_.Add(static_cast(length)); - // TODO(brandtr): Notify the BWE of received media packets here. - auto status = it->second->DeliverRtp(packet, length, packet_time) - ? DELIVERY_OK - : DELIVERY_PACKET_ERROR; - // Deliver media packets to FlexFEC subsystem. RTP header extensions need - // 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. - if (parsed_packet) { - auto it_bounds = flexfec_receive_ssrcs_media_.equal_range(ssrc); - for (auto it = it_bounds.first; it != it_bounds.second; ++it) - it->second->AddAndProcessReceivedPacket(*parsed_packet); - } - if (status == DELIVERY_OK) - event_log_->LogRtpHeader(kIncomingPacket, media_type, packet, length); - return status; + it->second->OnRtpPacket(*parsed_packet); + + // Deliver media packets to FlexFEC subsystem. + auto it_bounds = flexfec_receive_ssrcs_media_.equal_range(ssrc); + for (auto it = it_bounds.first; it != it_bounds.second; ++it) + it->second->AddAndProcessReceivedPacket(*parsed_packet); + + event_log_->LogRtpHeader(kIncomingPacket, media_type, packet, length); + return DELIVERY_OK; } } if (media_type == MediaType::ANY || media_type == MediaType::VIDEO) { auto it = flexfec_receive_ssrcs_protection_.find(ssrc); if (it != flexfec_receive_ssrcs_protection_.end()) { - if (parsed_packet) { - auto status = it->second->AddAndProcessReceivedPacket(*parsed_packet) - ? DELIVERY_OK - : DELIVERY_PACKET_ERROR; - if (status == DELIVERY_OK) - event_log_->LogRtpHeader(kIncomingPacket, media_type, packet, length); - return status; - } + auto status = it->second->AddAndProcessReceivedPacket(*parsed_packet) + ? DELIVERY_OK + : DELIVERY_PACKET_ERROR; + if (status == DELIVERY_OK) + event_log_->LogRtpHeader(kIncomingPacket, media_type, packet, length); + return status; } } return DELIVERY_UNKNOWN_SSRC; diff --git a/webrtc/call/packet_injection_tests.cc b/webrtc/call/packet_injection_tests.cc deleted file mode 100644 index 10de8f6e87..0000000000 --- a/webrtc/call/packet_injection_tests.cc +++ /dev/null @@ -1,93 +0,0 @@ -/* - * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#include - -#include "webrtc/logging/rtc_event_log/rtc_event_log.h" -#include "webrtc/test/call_test.h" -#include "webrtc/test/gtest.h" -#include "webrtc/test/null_transport.h" - -namespace webrtc { - -class PacketInjectionTest : public test::CallTest { - protected: - enum class CodecType { - kVp8, - kH264, - }; - - PacketInjectionTest() : rtp_header_parser_(RtpHeaderParser::Create()) {} - - void InjectIncorrectPacket(CodecType codec_type, - uint8_t packet_type, - const uint8_t* packet, - size_t length); - - std::unique_ptr rtp_header_parser_; -}; - -void PacketInjectionTest::InjectIncorrectPacket(CodecType codec_type, - uint8_t payload_type, - const uint8_t* packet, - size_t length) { - CreateSenderCall(Call::Config(&event_log_)); - CreateReceiverCall(Call::Config(&event_log_)); - - test::NullTransport null_transport; - CreateSendConfig(1, 0, 0, &null_transport); - CreateMatchingReceiveConfigs(&null_transport); - video_receive_configs_[0].decoders[0].payload_type = payload_type; - switch (codec_type) { - case CodecType::kVp8: - video_receive_configs_[0].decoders[0].payload_name = "VP8"; - break; - case CodecType::kH264: - video_receive_configs_[0].decoders[0].payload_name = "H264"; - break; - } - CreateVideoStreams(); - - RTPHeader header; - EXPECT_TRUE(rtp_header_parser_->Parse(packet, length, &header)); - EXPECT_EQ(kVideoSendSsrcs[0], header.ssrc) - << "Packet should have configured SSRC to not be dropped early."; - EXPECT_EQ(payload_type, header.payloadType); - Start(); - EXPECT_EQ(PacketReceiver::DELIVERY_PACKET_ERROR, - receiver_call_->Receiver()->DeliverPacket(MediaType::VIDEO, packet, - length, PacketTime())); - Stop(); - - DestroyStreams(); -} - -TEST_F(PacketInjectionTest, StapAPacketWithTruncatedNalUnits) { - const uint8_t kPacket[] = {0x80, - 0xE5, - 0xE6, - 0x0, - 0x0, - 0xED, - 0x23, - 0x4, - 0x00, - 0xC0, - 0xFF, - 0xED, - 0x58, - 0xCB, - 0xED, - 0xDF}; - - InjectIncorrectPacket(CodecType::kH264, 101, kPacket, sizeof(kPacket)); -} - -} // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc index f22f108262..22f050b8d0 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc @@ -867,6 +867,12 @@ TEST_F(RtpDepacketizerH264Test, TestTruncatedSingleStapANalu) { EXPECT_FALSE(depacketizer_->Parse(&payload, kPayload, sizeof(kPayload))); } +TEST_F(RtpDepacketizerH264Test, TestStapAPacketWithTruncatedNalUnits) { + const uint8_t kPayload[] = { 0x58, 0xCB, 0xED, 0xDF}; + RtpDepacketizer::ParsedPayload payload; + EXPECT_FALSE(depacketizer_->Parse(&payload, kPayload, sizeof(kPayload))); +} + TEST_F(RtpDepacketizerH264Test, TestTruncationJustAfterSingleStapANalu) { const uint8_t kPayload[] = {0x38, 0x27, 0x27}; RtpDepacketizer::ParsedPayload payload; diff --git a/webrtc/video/rtp_stream_receiver.cc b/webrtc/video/rtp_stream_receiver.cc index d49d0959f3..cb1ffc9044 100644 --- a/webrtc/video/rtp_stream_receiver.cc +++ b/webrtc/video/rtp_stream_receiver.cc @@ -26,6 +26,8 @@ #include "webrtc/modules/rtp_rtcp/include/rtp_receiver.h" #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp.h" #include "webrtc/modules/rtp_rtcp/include/ulpfec_receiver.h" +#include "webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h" +#include "webrtc/modules/rtp_rtcp/source/rtp_packet_received.h" #include "webrtc/modules/video_coding/frame_object.h" #include "webrtc/modules/video_coding/h264_sprop_parameter_sets.h" #include "webrtc/modules/video_coding/h264_sps_pps_tracker.h" @@ -316,57 +318,54 @@ void RtpStreamReceiver::OnIncomingSSRCChanged(const uint32_t ssrc) { rtp_rtcp_->SetRemoteSSRC(ssrc); } -bool RtpStreamReceiver::DeliverRtp(const uint8_t* rtp_packet, - size_t rtp_packet_length, - const PacketTime& packet_time) { +void RtpStreamReceiver::OnRtpPacket(const RtpPacketReceived& packet) { { rtc::CritScope lock(&receive_cs_); if (!receiving_) { - return false; + return; } } - RTPHeader header; - if (!rtp_header_parser_->Parse(rtp_packet, rtp_packet_length, - &header)) { - return false; - } - int64_t arrival_time_ms; int64_t now_ms = clock_->TimeInMilliseconds(); - if (packet_time.timestamp != -1) - arrival_time_ms = (packet_time.timestamp + 500) / 1000; - else - arrival_time_ms = now_ms; { // Periodically log the RTP header of incoming packets. rtc::CritScope lock(&receive_cs_); if (now_ms - last_packet_log_ms_ > kPacketLogIntervalMs) { std::stringstream ss; - ss << "Packet received on SSRC: " << header.ssrc << " with payload type: " - << static_cast(header.payloadType) << ", timestamp: " - << header.timestamp << ", sequence number: " << header.sequenceNumber - << ", arrival time: " << arrival_time_ms; - if (header.extension.hasTransmissionTimeOffset) - ss << ", toffset: " << header.extension.transmissionTimeOffset; - if (header.extension.hasAbsoluteSendTime) - ss << ", abs send time: " << header.extension.absoluteSendTime; + ss << "Packet received on SSRC: " << packet.Ssrc() + << " with payload type: " << static_cast(packet.PayloadType()) + << ", timestamp: " << packet.Timestamp() + << ", sequence number: " << packet.SequenceNumber() + << ", arrival time: " << packet.arrival_time_ms(); + int32_t time_offset; + if (packet.GetExtension(&time_offset)) { + ss << ", toffset: " << time_offset; + } + uint32_t send_time; + if (packet.GetExtension(&send_time)) { + ss << ", abs send time: " << send_time; + } LOG(LS_INFO) << ss.str(); last_packet_log_ms_ = now_ms; } } + // TODO(nisse): Delete use of GetHeader, but needs refactoring of + // ReceivePacket and IncomingPacket methods below. + RTPHeader header; + packet.GetHeader(&header); + header.payload_type_frequency = kVideoPayloadTypeFrequency; bool in_order = IsPacketInOrder(header); rtp_payload_registry_.SetIncomingPayloadType(header); - bool ret = ReceivePacket(rtp_packet, rtp_packet_length, header, in_order); + ReceivePacket(packet.data(), packet.size(), header, in_order); // Update receive statistics after ReceivePacket. // Receive statistics will be reset if the payload type changes (make sure // that the first packet is included in the stats). rtp_receive_statistics_->IncomingPacket( - header, rtp_packet_length, IsPacketRetransmitted(header, in_order)); - return ret; + header, packet.size(), IsPacketRetransmitted(header, in_order)); } int32_t RtpStreamReceiver::RequestKeyFrame() { @@ -424,6 +423,7 @@ void RtpStreamReceiver::OnRttUpdate(int64_t avg_rtt_ms, int64_t max_rtt_ms) { nack_module_->UpdateRtt(max_rtt_ms); } +// TODO(nisse): Drop return value. bool RtpStreamReceiver::ReceivePacket(const uint8_t* packet, size_t packet_length, const RTPHeader& header, diff --git a/webrtc/video/rtp_stream_receiver.h b/webrtc/video/rtp_stream_receiver.h index dd217899f2..c6531ccb9b 100644 --- a/webrtc/video/rtp_stream_receiver.h +++ b/webrtc/video/rtp_stream_receiver.h @@ -44,6 +44,7 @@ class ReceiveStatisticsProxy; class RemoteNtpTimeEstimator; class RtcpRttStats; class RtpHeaderParser; +class RtpPacketReceived; class RTPPayloadRegistry; class RtpReceiver; class Transport; @@ -89,9 +90,6 @@ class RtpStreamReceiver : public RtpData, void StartReceive(); void StopReceive(); - bool DeliverRtp(const uint8_t* rtp_packet, - size_t rtp_packet_length, - const PacketTime& packet_time); bool DeliverRtcp(const uint8_t* rtcp_packet, size_t rtcp_packet_length); void FrameContinuous(uint16_t seq_num); @@ -100,6 +98,9 @@ class RtpStreamReceiver : public RtpData, void SignalNetworkState(NetworkState state); + // TODO(nisse): Intended to be part of an RtpPacketReceiver interface. + void OnRtpPacket(const RtpPacketReceived& packet); + // Implements RtpData. int32_t OnReceivedPayloadData(const uint8_t* payload_data, size_t payload_size, diff --git a/webrtc/video/video_receive_stream.cc b/webrtc/video/video_receive_stream.cc index 50c47e5a06..7d8cfef21d 100644 --- a/webrtc/video/video_receive_stream.cc +++ b/webrtc/video/video_receive_stream.cc @@ -256,10 +256,8 @@ bool VideoReceiveStream::DeliverRtcp(const uint8_t* packet, size_t length) { return rtp_stream_receiver_.DeliverRtcp(packet, length); } -bool VideoReceiveStream::DeliverRtp(const uint8_t* packet, - size_t length, - const PacketTime& packet_time) { - return rtp_stream_receiver_.DeliverRtp(packet, length, packet_time); +void VideoReceiveStream::OnRtpPacket(const RtpPacketReceived& packet) { + rtp_stream_receiver_.OnRtpPacket(packet); } bool VideoReceiveStream::OnRecoveredPacket(const uint8_t* packet, diff --git a/webrtc/video/video_receive_stream.h b/webrtc/video/video_receive_stream.h index a6ee31ee42..8c13ccd966 100644 --- a/webrtc/video/video_receive_stream.h +++ b/webrtc/video/video_receive_stream.h @@ -62,9 +62,6 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream, void SignalNetworkState(NetworkState state); bool DeliverRtcp(const uint8_t* packet, size_t length); - bool DeliverRtp(const uint8_t* packet, - size_t length, - const PacketTime& packet_time); bool OnRecoveredPacket(const uint8_t* packet, size_t length); @@ -84,6 +81,9 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream, void EnableEncodedFrameRecording(rtc::PlatformFile file, size_t byte_limit) override; + // TODO(nisse): Intended to be part of an RtpPacketReceiver interface. + void OnRtpPacket(const RtpPacketReceived& packet); + // Implements rtc::VideoSinkInterface. void OnFrame(const VideoFrame& video_frame) override;