From 89f64b994f03d4b249c102be2988bd9b6435f2c6 Mon Sep 17 00:00:00 2001 From: Jakob Ivarsson Date: Wed, 7 Jun 2023 19:18:18 +0200 Subject: [PATCH] Make packet info optional and only set for primary packets in NetEq. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Header metadata such as audio level and capture time doesn't make sense for redundant payloads (i.e. RED and inband-FEC). It is assumed that one of the parsed payload timestamps will correspond to the RTP header timestamp. This fixes a bug where capture time and CSRCs were not set after parsing RED packets. CreateRedPayload test function is adapted from red_payload_splitter_unittest.cc Bug: webrtc:15185 Change-Id: Iba58772499b6d760f516854999b60511896b053c Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/305700 Reviewed-by: Henrik Lundin Commit-Queue: Jakob Ivarsson‎ Cr-Commit-Position: refs/heads/main@{#40240} --- modules/audio_coding/neteq/neteq_impl.cc | 18 +++-- .../audio_coding/neteq/neteq_impl_unittest.cc | 80 +++++++++++++++++++ modules/audio_coding/neteq/packet.h | 2 +- .../neteq/red_payload_splitter.cc | 9 +-- 4 files changed, 92 insertions(+), 17 deletions(-) diff --git a/modules/audio_coding/neteq/neteq_impl.cc b/modules/audio_coding/neteq/neteq_impl.cc index 113215db8e..52e8cbad3a 100644 --- a/modules/audio_coding/neteq/neteq_impl.cc +++ b/modules/audio_coding/neteq/neteq_impl.cc @@ -506,14 +506,13 @@ int NetEqImpl::InsertPacketInternal(const RTPHeader& rtp_header, PacketList packet_list; // Insert packet in a packet list. - packet_list.push_back([&rtp_header, &payload, &receive_time] { + packet_list.push_back([&rtp_header, &payload] { // Convert to Packet. Packet packet; packet.payload_type = rtp_header.payloadType; packet.sequence_number = rtp_header.sequenceNumber; packet.timestamp = rtp_header.timestamp; packet.payload.SetData(payload.data(), payload.size()); - packet.packet_info = RtpPacketInfo(rtp_header, receive_time); // Waiting time will be set upon inserting the packet in the buffer. RTC_DCHECK(!packet.waiting_time); return packet; @@ -630,10 +629,9 @@ int NetEqImpl::InsertPacketInternal(const RTPHeader& rtp_header, parsed_packet_list.splice(parsed_packet_list.end(), packet_list, packet_list.begin()); } else { - const auto sequence_number = packet.sequence_number; - const auto payload_type = packet.payload_type; + const uint16_t sequence_number = packet.sequence_number; + const uint8_t payload_type = packet.payload_type; const Packet::Priority original_priority = packet.priority; - const auto& packet_info = packet.packet_info; auto packet_from_result = [&](AudioDecoder::ParseResult& result) { Packet new_packet; new_packet.sequence_number = sequence_number; @@ -641,7 +639,10 @@ int NetEqImpl::InsertPacketInternal(const RTPHeader& rtp_header, new_packet.timestamp = result.timestamp; new_packet.priority.codec_level = result.priority; new_packet.priority.red_level = original_priority.red_level; - new_packet.packet_info = packet_info; + // Only associate the header information with the primary packet. + if (new_packet.timestamp == rtp_header.timestamp) { + new_packet.packet_info = RtpPacketInfo(rtp_header, receive_time); + } new_packet.frame = std::move(result.frame); return new_packet; }; @@ -1474,8 +1475,9 @@ int NetEqImpl::DecodeLoop(PacketList* packet_list, auto opt_result = packet_list->front().frame->Decode( rtc::ArrayView(&decoded_buffer_[*decoded_length], decoded_buffer_length_ - *decoded_length)); - last_decoded_packet_infos_.push_back( - std::move(packet_list->front().packet_info)); + if (packet_list->front().packet_info) { + last_decoded_packet_infos_.push_back(*packet_list->front().packet_info); + } packet_list->pop_front(); if (opt_result) { const auto& result = *opt_result; diff --git a/modules/audio_coding/neteq/neteq_impl_unittest.cc b/modules/audio_coding/neteq/neteq_impl_unittest.cc index f631937535..e61cd52502 100644 --- a/modules/audio_coding/neteq/neteq_impl_unittest.cc +++ b/modules/audio_coding/neteq/neteq_impl_unittest.cc @@ -725,6 +725,86 @@ TEST_F(NetEqImplTest, FirstPacketUnknown) { } } +std::vector CreateRedPayload(size_t num_payloads, + int payload_type, + int payload_size, + int timestamp_offset) { + constexpr int kRedHeaderLength = 4; + const size_t size = + payload_size + 1 + (num_payloads - 1) * (payload_size + kRedHeaderLength); + std::vector payload(size, 0); + uint8_t* payload_ptr = payload.data(); + for (size_t i = 0; i < num_payloads; ++i) { + // Write the RED headers. + if (i == num_payloads - 1) { + // Special case for last payload. + *payload_ptr = payload_type & 0x7F; // F = 0; + ++payload_ptr; + break; + } + *payload_ptr = payload_type & 0x7F; + // Not the last block; set F = 1. + *payload_ptr |= 0x80; + ++payload_ptr; + const int this_offset = + rtc::checked_cast((num_payloads - i - 1) * timestamp_offset); + *payload_ptr = this_offset >> 6; + ++payload_ptr; + RTC_DCHECK_LE(payload_size, 1023); // Max length described by 10 bits. + *payload_ptr = ((this_offset & 0x3F) << 2) | (payload_size >> 8); + ++payload_ptr; + *payload_ptr = payload_size & 0xFF; + ++payload_ptr; + } + return payload; +} + +TEST_F(NetEqImplTest, InsertRedPayload) { + UseNoMocks(); + CreateInstance(); + constexpr int kRedPayloadType = 7; + neteq_->RegisterPayloadType(kRedPayloadType, SdpAudioFormat("red", 8000, 1)); + constexpr int kPayloadType = 8; + neteq_->RegisterPayloadType(kPayloadType, SdpAudioFormat("l16", 8000, 1)); + size_t frame_size = 80; // 10 ms. + size_t payload_size = frame_size * 2; + std::vector payload = + CreateRedPayload(3, kPayloadType, payload_size, frame_size); + RTPHeader header; + header.payloadType = kRedPayloadType; + header.sequenceNumber = 0x1234; + header.timestamp = 0x12345678; + header.ssrc = 0x87654321; + AbsoluteCaptureTime capture_time; + capture_time.absolute_capture_timestamp = 1234; + header.extension.absolute_capture_time = capture_time; + header.extension.audioLevel = 12; + header.extension.hasAudioLevel = true; + header.numCSRCs = 1; + header.arrOfCSRCs[0] = 123; + neteq_->InsertPacket(header, payload); + AudioFrame frame; + bool muted; + neteq_->GetAudio(&frame, &muted); + // TODO(jakobi): Find a better way to test that the correct packet is decoded + // than using the timestamp. The fixed NetEq delay is an implementation + // detail that should not be tested. + constexpr int kNetEqFixedDelay = 5; + EXPECT_EQ(frame.timestamp_, + header.timestamp - frame_size * 2 - kNetEqFixedDelay); + EXPECT_TRUE(frame.packet_infos_.empty()); + neteq_->GetAudio(&frame, &muted); + EXPECT_EQ(frame.timestamp_, header.timestamp - frame_size - kNetEqFixedDelay); + EXPECT_TRUE(frame.packet_infos_.empty()); + neteq_->GetAudio(&frame, &muted); + EXPECT_EQ(frame.timestamp_, header.timestamp - kNetEqFixedDelay); + EXPECT_EQ(frame.packet_infos_.size(), 1u); + EXPECT_EQ(frame.packet_infos_.front().absolute_capture_time(), capture_time); + EXPECT_EQ(frame.packet_infos_.front().audio_level(), + header.extension.audioLevel); + EXPECT_EQ(frame.packet_infos_.front().csrcs()[0], header.arrOfCSRCs[0]); +} + // This test verifies that audio interruption is not logged for the initial // PLC period before the first packet is deocoded. // TODO(henrik.lundin) Maybe move this test to neteq_network_stats_unittest.cc. diff --git a/modules/audio_coding/neteq/packet.h b/modules/audio_coding/neteq/packet.h index 0c6f204edb..795c36dc12 100644 --- a/modules/audio_coding/neteq/packet.h +++ b/modules/audio_coding/neteq/packet.h @@ -74,7 +74,7 @@ struct Packet { // Datagram excluding RTP header and header extension. rtc::Buffer payload; Priority priority; - RtpPacketInfo packet_info; + absl::optional packet_info; std::unique_ptr waiting_time; std::unique_ptr frame; diff --git a/modules/audio_coding/neteq/red_payload_splitter.cc b/modules/audio_coding/neteq/red_payload_splitter.cc index cec9f2f8a0..992cd28e62 100644 --- a/modules/audio_coding/neteq/red_payload_splitter.cc +++ b/modules/audio_coding/neteq/red_payload_splitter.cc @@ -39,7 +39,7 @@ bool RedPayloadSplitter::SplitRed(PacketList* packet_list) { bool ret = true; PacketList::iterator it = packet_list->begin(); while (it != packet_list->end()) { - const Packet& red_packet = *it; + Packet& red_packet = *it; RTC_DCHECK(!red_packet.payload.empty()); const uint8_t* payload_ptr = red_packet.payload.data(); size_t payload_length = red_packet.payload.size(); @@ -132,13 +132,6 @@ bool RedPayloadSplitter::SplitRed(PacketList* packet_list) { new_packet.priority.red_level = rtc::dchecked_cast((new_headers.size() - 1) - i); new_packet.payload.SetData(payload_ptr, payload_length); - new_packet.packet_info = RtpPacketInfo( - /*ssrc=*/red_packet.packet_info.ssrc(), - /*csrcs=*/std::vector(), - /*rtp_timestamp=*/new_packet.timestamp, - /*receive_time=*/red_packet.packet_info.receive_time()); - new_packet.packet_info.set_audio_level( - red_packet.packet_info.audio_level()); new_packets.push_front(std::move(new_packet)); payload_ptr += payload_length; }