From defa7a80491c8598d74ba2895bdb73d7d1e674f3 Mon Sep 17 00:00:00 2001 From: Henrik Lundin Date: Tue, 3 Jul 2018 13:07:30 +0200 Subject: [PATCH] NetEq: Handle nested RED packets This CL makes NetEq handle nested RED packets without crashing. Nested RED packets mean that the block PT (see https://tools.ietf.org/html/rfc2198.html#section-3) in the RED packet is also set to the RED PT. This implies a nested RED packet, which is not supported. Instead, all payloads in a RED packet that have the RED PT will be discarded. Bug: chromium:851662 Change-Id: I86ec257e60fb8076e3574ac5a4a1ca50196f6b34 Reviewed-on: https://webrtc-review.googlesource.com/86824 Commit-Queue: Henrik Lundin Reviewed-by: Ivo Creusen Cr-Commit-Position: refs/heads/master@{#23825} --- .../neteq/mock/mock_red_payload_splitter.h | 4 ++-- modules/audio_coding/neteq/neteq_impl.cc | 3 +++ .../neteq/red_payload_splitter.cc | 9 +++---- .../audio_coding/neteq/red_payload_splitter.h | 7 +++--- .../neteq/red_payload_splitter_unittest.cc | 24 +++++++++++++++++++ 5 files changed, 37 insertions(+), 10 deletions(-) diff --git a/modules/audio_coding/neteq/mock/mock_red_payload_splitter.h b/modules/audio_coding/neteq/mock/mock_red_payload_splitter.h index 27a2276664..426c467afa 100644 --- a/modules/audio_coding/neteq/mock/mock_red_payload_splitter.h +++ b/modules/audio_coding/neteq/mock/mock_red_payload_splitter.h @@ -21,8 +21,8 @@ class MockRedPayloadSplitter : public RedPayloadSplitter { public: MOCK_METHOD1(SplitRed, bool(PacketList* packet_list)); MOCK_METHOD2(CheckRedPayloads, - int(PacketList* packet_list, - const DecoderDatabase& decoder_database)); + void(PacketList* packet_list, + const DecoderDatabase& decoder_database)); }; } // namespace webrtc diff --git a/modules/audio_coding/neteq/neteq_impl.cc b/modules/audio_coding/neteq/neteq_impl.cc index 4630448efd..0ef32638d2 100644 --- a/modules/audio_coding/neteq/neteq_impl.cc +++ b/modules/audio_coding/neteq/neteq_impl.cc @@ -612,6 +612,9 @@ int NetEqImpl::InsertPacketInternal(const RTPHeader& rtp_header, // Only accept a few RED payloads of the same type as the main data, // DTMF events and CNG. red_payload_splitter_->CheckRedPayloads(&packet_list, *decoder_database_); + if (packet_list.empty()) { + return kRedundancySplitError; + } } // Check payload types. diff --git a/modules/audio_coding/neteq/red_payload_splitter.cc b/modules/audio_coding/neteq/red_payload_splitter.cc index 85e399c863..f5435e8eab 100644 --- a/modules/audio_coding/neteq/red_payload_splitter.cc +++ b/modules/audio_coding/neteq/red_payload_splitter.cc @@ -130,13 +130,16 @@ bool RedPayloadSplitter::SplitRed(PacketList* packet_list) { return ret; } -int RedPayloadSplitter::CheckRedPayloads( +void RedPayloadSplitter::CheckRedPayloads( PacketList* packet_list, const DecoderDatabase& decoder_database) { int main_payload_type = -1; - int num_deleted_packets = 0; for (auto it = packet_list->begin(); it != packet_list->end(); /* */) { uint8_t this_payload_type = it->payload_type; + if (decoder_database.IsRed(this_payload_type)) { + it = packet_list->erase(it); + continue; + } if (!decoder_database.IsDtmf(this_payload_type) && !decoder_database.IsComfortNoise(this_payload_type)) { if (main_payload_type == -1) { @@ -149,14 +152,12 @@ int RedPayloadSplitter::CheckRedPayloads( // moves the iterator |it| to the next packet in the list. Thus, we // do not have to increment it manually. it = packet_list->erase(it); - ++num_deleted_packets; continue; } } } ++it; } - return num_deleted_packets; } } // namespace webrtc diff --git a/modules/audio_coding/neteq/red_payload_splitter.h b/modules/audio_coding/neteq/red_payload_splitter.h index 1475b1b4a1..5e239ddc70 100644 --- a/modules/audio_coding/neteq/red_payload_splitter.h +++ b/modules/audio_coding/neteq/red_payload_splitter.h @@ -38,10 +38,9 @@ class RedPayloadSplitter { // Checks all packets in |packet_list|. Packets that are DTMF events or // comfort noise payloads are kept. Except that, only one single payload type - // is accepted. Any packet with another payload type is discarded. Returns - // the number of discarded packets. - virtual int CheckRedPayloads(PacketList* packet_list, - const DecoderDatabase& decoder_database); + // is accepted. Any packet with another payload type is discarded. + virtual void CheckRedPayloads(PacketList* packet_list, + const DecoderDatabase& decoder_database); private: RTC_DISALLOW_COPY_AND_ASSIGN(RedPayloadSplitter); diff --git a/modules/audio_coding/neteq/red_payload_splitter_unittest.cc b/modules/audio_coding/neteq/red_payload_splitter_unittest.cc index 73cd66cac1..52fc1ba177 100644 --- a/modules/audio_coding/neteq/red_payload_splitter_unittest.cc +++ b/modules/audio_coding/neteq/red_payload_splitter_unittest.cc @@ -319,6 +319,30 @@ TEST(RedPayloadSplitter, CheckRedPayloads) { EXPECT_TRUE(packet_list.empty()); } +// This test creates a RED packet where the payloads also have the payload type +// for RED. That is, some kind of weird nested RED packet. This is not supported +// and the splitter should discard all packets. +TEST(RedPayloadSplitter, CheckRedPayloadsRecursiveRed) { + PacketList packet_list; + for (uint8_t i = 0; i <= 3; ++i) { + // Create packet with RED payload type, payload length 10 bytes, all 0. + packet_list.push_back(CreatePacket(kRedPayloadType, 10, 0)); + } + + // Use a real DecoderDatabase object here instead of a mock, since it is + // easier to just register the payload types and let the actual implementation + // do its job. + DecoderDatabase decoder_database( + new rtc::RefCountedObject, absl::nullopt); + decoder_database.RegisterPayload(kRedPayloadType, NetEqDecoder::kDecoderRED, + "red"); + + RedPayloadSplitter splitter; + splitter.CheckRedPayloads(&packet_list, decoder_database); + + EXPECT_TRUE(packet_list.empty()); // Should have dropped all packets. +} + // Packet A is split into A1, A2 and A3. But the length parameter is off, so // the last payloads should be discarded. TEST(RedPayloadSplitter, WrongPayloadLength) {