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 <henrik.lundin@webrtc.org>
Reviewed-by: Ivo Creusen <ivoc@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23825}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 27a2276..426c467 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 @@
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 4630448..0ef3263 100644
--- a/modules/audio_coding/neteq/neteq_impl.cc
+++ b/modules/audio_coding/neteq/neteq_impl.cc
@@ -612,6 +612,9 @@
// 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 85e399c..f5435e8 100644
--- a/modules/audio_coding/neteq/red_payload_splitter.cc
+++ b/modules/audio_coding/neteq/red_payload_splitter.cc
@@ -130,13 +130,16 @@
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 @@
// 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 1475b1b..5e239dd 100644
--- a/modules/audio_coding/neteq/red_payload_splitter.h
+++ b/modules/audio_coding/neteq/red_payload_splitter.h
@@ -38,10 +38,9 @@
// 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 73cd66c..52fc1ba 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 @@
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<MockAudioDecoderFactory>, 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) {