From 945b7d8e31a63e4be70ba41a0064189034ebf341 Mon Sep 17 00:00:00 2001 From: Bjorn Terelius Date: Mon, 19 Oct 2020 19:01:21 +0200 Subject: [PATCH] Add test for logging of large compound RTCP packets. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: chromium:1134107 Change-Id: Ic6ce50d33700c05733747584ce45480660cf64c9 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/188583 Reviewed-by: Elad Alon Commit-Queue: Björn Terelius Cr-Commit-Position: refs/heads/master@{#32445} --- .../encoder/rtc_event_log_encoder_unittest.cc | 60 +++++++++++++++++++ logging/rtc_event_log/logged_events.cc | 8 ++- logging/rtc_event_log/logged_events.h | 14 ++--- logging/rtc_event_log/rtc_event_log_parser.cc | 40 ++++--------- logging/rtc_event_log/rtc_event_log_parser.h | 6 +- 5 files changed, 85 insertions(+), 43 deletions(-) diff --git a/logging/rtc_event_log/encoder/rtc_event_log_encoder_unittest.cc b/logging/rtc_event_log/encoder/rtc_event_log_encoder_unittest.cc index 009acbd5d6..76740a4b04 100644 --- a/logging/rtc_event_log/encoder/rtc_event_log_encoder_unittest.cc +++ b/logging/rtc_event_log/encoder/rtc_event_log_encoder_unittest.cc @@ -36,6 +36,7 @@ #include "logging/rtc_event_log/rtc_event_log_unittest_helper.h" #include "modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor_config.h" #include "modules/remote_bitrate_estimator/include/bwe_defines.h" +#include "modules/rtp_rtcp/source/rtcp_packet/bye.h" #include "modules/rtp_rtcp/source/rtp_header_extensions.h" #include "rtc_base/fake_clock.h" #include "rtc_base/random.h" @@ -1242,4 +1243,63 @@ INSTANTIATE_TEST_SUITE_P( /* Event count: */ ::testing::Values(1, 2, 10, 100), /* Repeated fields: */ ::testing::Bool())); +class RtcEventLogEncoderSimpleTest + : public ::testing::TestWithParam { + protected: + RtcEventLogEncoderSimpleTest() : encoding_(GetParam()) { + switch (encoding_) { + case RtcEventLog::EncodingType::Legacy: + encoder_ = std::make_unique(); + break; + case RtcEventLog::EncodingType::NewFormat: + encoder_ = std::make_unique(); + break; + } + } + ~RtcEventLogEncoderSimpleTest() override = default; + + std::deque> history_; + std::unique_ptr encoder_; + ParsedRtcEventLog parsed_log_; + const RtcEventLog::EncodingType encoding_; +}; + +TEST_P(RtcEventLogEncoderSimpleTest, RtcEventLargeCompoundRtcpPacketIncoming) { + // Create a compound packet containing multiple Bye messages. + rtc::Buffer packet; + size_t index = 0; + for (int i = 0; i < 8; i++) { + rtcp::Bye bye; + std::string reason(255, 'a'); // Add some arbitrary data. + bye.SetReason(reason); + bye.SetSenderSsrc(0x12345678); + packet.SetSize(packet.size() + bye.BlockLength()); + bool created = + bye.Create(packet.data(), &index, packet.capacity(), nullptr); + ASSERT_TRUE(created); + ASSERT_EQ(index, packet.size()); + } + + EXPECT_GT(packet.size(), static_cast(IP_PACKET_SIZE)); + auto event = std::make_unique(packet); + history_.push_back(event->Copy()); + std::string encoded = encoder_->EncodeBatch(history_.begin(), history_.end()); + + ParsedRtcEventLog::ParseStatus status = parsed_log_.ParseString(encoded); + ASSERT_TRUE(status.ok()) << status.message(); + + const auto& incoming_rtcp_packets = parsed_log_.incoming_rtcp_packets(); + ASSERT_EQ(incoming_rtcp_packets.size(), 1u); + ASSERT_EQ(incoming_rtcp_packets[0].rtcp.raw_data.size(), packet.size()); + EXPECT_EQ(memcmp(incoming_rtcp_packets[0].rtcp.raw_data.data(), packet.data(), + packet.size()), + 0); +} + +INSTANTIATE_TEST_SUITE_P( + LargeCompoundRtcp, + RtcEventLogEncoderSimpleTest, + ::testing::Values(RtcEventLog::EncodingType::Legacy, + RtcEventLog::EncodingType::NewFormat)); + } // namespace webrtc diff --git a/logging/rtc_event_log/logged_events.cc b/logging/rtc_event_log/logged_events.cc index 3cba8bab21..dd0a8aae2a 100644 --- a/logging/rtc_event_log/logged_events.cc +++ b/logging/rtc_event_log/logged_events.cc @@ -41,15 +41,17 @@ LoggedPacketInfo::LoggedPacketInfo(const LoggedPacketInfo&) = default; LoggedPacketInfo::~LoggedPacketInfo() {} LoggedRtcpPacket::LoggedRtcpPacket(int64_t timestamp_us, - const uint8_t* packet, - size_t total_length) - : timestamp_us(timestamp_us), raw_data(packet, packet + total_length) {} + const std::vector& packet) + : timestamp_us(timestamp_us), raw_data(packet) {} + LoggedRtcpPacket::LoggedRtcpPacket(int64_t timestamp_us, const std::string& packet) : timestamp_us(timestamp_us), raw_data(packet.size()) { memcpy(raw_data.data(), packet.data(), packet.size()); } + LoggedRtcpPacket::LoggedRtcpPacket(const LoggedRtcpPacket& rhs) = default; + LoggedRtcpPacket::~LoggedRtcpPacket() = default; } // namespace webrtc diff --git a/logging/rtc_event_log/logged_events.h b/logging/rtc_event_log/logged_events.h index 4bd33f62bd..192f7cf818 100644 --- a/logging/rtc_event_log/logged_events.h +++ b/logging/rtc_event_log/logged_events.h @@ -309,9 +309,7 @@ struct LoggedRtpPacketOutgoing { }; struct LoggedRtcpPacket { - LoggedRtcpPacket(int64_t timestamp_us, - const uint8_t* packet, - size_t total_length); + LoggedRtcpPacket(int64_t timestamp_us, const std::vector& packet); LoggedRtcpPacket(int64_t timestamp_us, const std::string& packet); LoggedRtcpPacket(const LoggedRtcpPacket&); ~LoggedRtcpPacket(); @@ -325,9 +323,8 @@ struct LoggedRtcpPacket { struct LoggedRtcpPacketIncoming { LoggedRtcpPacketIncoming(int64_t timestamp_us, - const uint8_t* packet, - size_t total_length) - : rtcp(timestamp_us, packet, total_length) {} + const std::vector& packet) + : rtcp(timestamp_us, packet) {} LoggedRtcpPacketIncoming(uint64_t timestamp_us, const std::string& packet) : rtcp(timestamp_us, packet) {} @@ -339,9 +336,8 @@ struct LoggedRtcpPacketIncoming { struct LoggedRtcpPacketOutgoing { LoggedRtcpPacketOutgoing(int64_t timestamp_us, - const uint8_t* packet, - size_t total_length) - : rtcp(timestamp_us, packet, total_length) {} + const std::vector& packet) + : rtcp(timestamp_us, packet) {} LoggedRtcpPacketOutgoing(uint64_t timestamp_us, const std::string& packet) : rtcp(timestamp_us, packet) {} diff --git a/logging/rtc_event_log/rtc_event_log_parser.cc b/logging/rtc_event_log/rtc_event_log_parser.cc index 67b1c0d6e6..a49af871fc 100644 --- a/logging/rtc_event_log/rtc_event_log_parser.cc +++ b/logging/rtc_event_log/rtc_event_log_parser.cc @@ -678,10 +678,8 @@ ParsedRtcEventLog::ParseStatus StoreRtcpPackets( raw_packet_values[i])) { continue; } - const size_t data_size = raw_packet_values[i].size(); - const uint8_t* data = - reinterpret_cast(raw_packet_values[i].data()); - rtcp_packets->emplace_back(1000 * timestamp_ms, data, data_size); + std::string data(raw_packet_values[i]); + rtcp_packets->emplace_back(1000 * timestamp_ms, data); } return ParsedRtcEventLog::ParseStatus::Success(); } @@ -1093,8 +1091,7 @@ void ParsedRtcEventLog::Clear() { video_recv_configs_.clear(); video_send_configs_.clear(); - memset(last_incoming_rtcp_packet_, 0, IP_PACKET_SIZE); - last_incoming_rtcp_packet_length_ = 0; + last_incoming_rtcp_packet_.clear(); first_timestamp_ = std::numeric_limits::max(); last_timestamp_ = std::numeric_limits::min(); @@ -1476,27 +1473,23 @@ ParsedRtcEventLog::ParseStatus ParsedRtcEventLog::StoreParsedLegacyEvent( } case rtclog::Event::RTCP_EVENT: { PacketDirection direction; - uint8_t packet[IP_PACKET_SIZE]; - size_t total_length; - auto status = GetRtcpPacket(event, &direction, packet, &total_length); + std::vector packet; + auto status = GetRtcpPacket(event, &direction, &packet); RTC_RETURN_IF_ERROR(status); RTC_PARSE_CHECK_OR_RETURN(event.has_timestamp_us()); int64_t timestamp_us = event.timestamp_us(); - RTC_PARSE_CHECK_OR_RETURN_LE(total_length, IP_PACKET_SIZE); if (direction == kIncomingPacket) { // Currently incoming RTCP packets are logged twice, both for audio and // video. Only act on one of them. Compare against the previous parsed // incoming RTCP packet. - if (total_length == last_incoming_rtcp_packet_length_ && - memcmp(last_incoming_rtcp_packet_, packet, total_length) == 0) + if (packet == last_incoming_rtcp_packet_) break; incoming_rtcp_packets_.push_back( - LoggedRtcpPacketIncoming(timestamp_us, packet, total_length)); - last_incoming_rtcp_packet_length_ = total_length; - memcpy(last_incoming_rtcp_packet_, packet, total_length); + LoggedRtcpPacketIncoming(timestamp_us, packet)); + last_incoming_rtcp_packet_ = packet; } else { outgoing_rtcp_packets_.push_back( - LoggedRtcpPacketOutgoing(timestamp_us, packet, total_length)); + LoggedRtcpPacketOutgoing(timestamp_us, packet)); } break; } @@ -1655,12 +1648,10 @@ const RtpHeaderExtensionMap* ParsedRtcEventLog::GetRtpHeaderExtensionMap( return nullptr; } -// The packet must have space for at least IP_PACKET_SIZE bytes. ParsedRtcEventLog::ParseStatus ParsedRtcEventLog::GetRtcpPacket( const rtclog::Event& event, PacketDirection* incoming, - uint8_t* packet, - size_t* length) const { + std::vector* packet) const { RTC_PARSE_CHECK_OR_RETURN(event.has_type()); RTC_PARSE_CHECK_OR_RETURN_EQ(event.type(), rtclog::Event::RTCP_EVENT); RTC_PARSE_CHECK_OR_RETURN(event.has_rtcp_packet()); @@ -1670,16 +1661,11 @@ ParsedRtcEventLog::ParseStatus ParsedRtcEventLog::GetRtcpPacket( if (incoming != nullptr) { *incoming = rtcp_packet.incoming() ? kIncomingPacket : kOutgoingPacket; } - // Get packet length. - RTC_PARSE_CHECK_OR_RETURN(rtcp_packet.has_packet_data()); - if (length != nullptr) { - *length = rtcp_packet.packet_data().size(); - } // Get packet contents. + RTC_PARSE_CHECK_OR_RETURN(rtcp_packet.has_packet_data()); if (packet != nullptr) { - RTC_PARSE_CHECK_OR_RETURN_LE(rtcp_packet.packet_data().size(), - static_cast(IP_PACKET_SIZE)); - memcpy(packet, rtcp_packet.packet_data().data(), + packet->resize(rtcp_packet.packet_data().size()); + memcpy(packet->data(), rtcp_packet.packet_data().data(), rtcp_packet.packet_data().size()); } return ParseStatus::Success(); diff --git a/logging/rtc_event_log/rtc_event_log_parser.h b/logging/rtc_event_log/rtc_event_log_parser.h index 712510b2a4..dce075aff2 100644 --- a/logging/rtc_event_log/rtc_event_log_parser.h +++ b/logging/rtc_event_log/rtc_event_log_parser.h @@ -670,8 +670,7 @@ class ParsedRtcEventLog { // NB: The packet must have space for at least IP_PACKET_SIZE bytes. ParseStatus GetRtcpPacket(const rtclog::Event& event, PacketDirection* incoming, - uint8_t* packet, - size_t* length) const; + std::vector* packet) const; ParseStatusOr GetVideoReceiveConfig( const rtclog::Event& event) const; @@ -873,8 +872,7 @@ class ParsedRtcEventLog { std::vector route_change_events_; std::vector remote_estimate_events_; - uint8_t last_incoming_rtcp_packet_[IP_PACKET_SIZE]; - uint8_t last_incoming_rtcp_packet_length_; + std::vector last_incoming_rtcp_packet_; int64_t first_timestamp_; int64_t last_timestamp_;