From a740142398c87f72ff6a9860874a0f963916d37a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Fri, 13 Sep 2019 14:18:58 +0200 Subject: [PATCH] Refactor LossNotificationController to not use VCMPacket Bug: None Change-Id: I15e1b3405c6538dd22aaeb125751c158c069478a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/152384 Reviewed-by: Ilya Nikolaevskiy Reviewed-by: Philip Eliasson Commit-Queue: Niels Moller Cr-Commit-Position: refs/heads/master@{#29193} --- .../loss_notification_controller.cc | 29 +++++------- .../loss_notification_controller.h | 5 +- .../loss_notification_controller_unittest.cc | 46 ++++++++----------- video/rtp_video_stream_receiver.cc | 8 +++- 4 files changed, 41 insertions(+), 47 deletions(-) diff --git a/modules/video_coding/loss_notification_controller.cc b/modules/video_coding/loss_notification_controller.cc index 6389fd0454..20752f8a07 100644 --- a/modules/video_coding/loss_notification_controller.cc +++ b/modules/video_coding/loss_notification_controller.cc @@ -43,19 +43,15 @@ LossNotificationController::LossNotificationController( LossNotificationController::~LossNotificationController() = default; -void LossNotificationController::OnReceivedPacket(const VCMPacket& packet) { +void LossNotificationController::OnReceivedPacket( + uint16_t rtp_seq_num, + const RtpGenericFrameDescriptor& generic_descriptor) { RTC_DCHECK_RUN_ON(&sequence_checker_); - if (!packet.generic_descriptor) { - RTC_LOG(LS_WARNING) << "Generic frame descriptor missing. Buggy remote? " - "Misconfigured local?"; - return; - } - // Ignore repeated or reordered packets. // TODO(bugs.webrtc.org/10336): Handle packet reordering. if (last_received_seq_num_ && - !AheadOf(packet.seqNum, *last_received_seq_num_)) { + !AheadOf(rtp_seq_num, *last_received_seq_num_)) { return; } @@ -63,12 +59,12 @@ void LossNotificationController::OnReceivedPacket(const VCMPacket& packet) { const bool seq_num_gap = last_received_seq_num_ && - packet.seqNum != static_cast(*last_received_seq_num_ + 1u); + rtp_seq_num != static_cast(*last_received_seq_num_ + 1u); - last_received_seq_num_ = packet.seqNum; + last_received_seq_num_ = rtp_seq_num; - if (packet.generic_descriptor->FirstPacketInSubFrame()) { - const uint16_t frame_id = packet.generic_descriptor->FrameId(); + if (generic_descriptor.FirstPacketInSubFrame()) { + const uint16_t frame_id = generic_descriptor.FrameId(); const int64_t unwrapped_frame_id = frame_id_unwrapper_.Unwrap(frame_id); // Ignore repeated or reordered frames. @@ -83,7 +79,7 @@ void LossNotificationController::OnReceivedPacket(const VCMPacket& packet) { last_received_unwrapped_frame_id_ = unwrapped_frame_id; const bool intra_frame = - packet.generic_descriptor->FrameDependenciesDiffs().empty(); + generic_descriptor.FrameDependenciesDiffs().empty(); // Generic Frame Descriptor does not current allow us to distinguish // whether an intra frame is a key frame. // We therefore assume all intra frames are key frames. @@ -98,11 +94,10 @@ void LossNotificationController::OnReceivedPacket(const VCMPacket& packet) { current_frame_potentially_decodable_ = true; } else { const bool all_dependencies_decodable = AllDependenciesDecodable( - unwrapped_frame_id, - packet.generic_descriptor->FrameDependenciesDiffs()); + unwrapped_frame_id, generic_descriptor.FrameDependenciesDiffs()); current_frame_potentially_decodable_ = all_dependencies_decodable; if (seq_num_gap || !current_frame_potentially_decodable_) { - HandleLoss(packet.seqNum, current_frame_potentially_decodable_); + HandleLoss(rtp_seq_num, current_frame_potentially_decodable_); } } } else if (seq_num_gap || !current_frame_potentially_decodable_) { @@ -111,7 +106,7 @@ void LossNotificationController::OnReceivedPacket(const VCMPacket& packet) { // even if only one of its packets is lost. We do this because the bigger // the frame, the more likely it is to be non-discardable, and therefore // the more robust we wish to be to loss of the feedback messages. - HandleLoss(packet.seqNum, false); + HandleLoss(rtp_seq_num, false); } } diff --git a/modules/video_coding/loss_notification_controller.h b/modules/video_coding/loss_notification_controller.h index 09f4fef180..6fc5eb858c 100644 --- a/modules/video_coding/loss_notification_controller.h +++ b/modules/video_coding/loss_notification_controller.h @@ -15,7 +15,7 @@ #include "absl/types/optional.h" #include "modules/include/module_common_types.h" -#include "modules/video_coding/packet.h" +#include "modules/rtp_rtcp/source/rtp_generic_frame_descriptor.h" #include "rtc_base/numerics/sequence_number_util.h" #include "rtc_base/synchronization/sequence_checker.h" @@ -28,7 +28,8 @@ class LossNotificationController { ~LossNotificationController(); // An RTP packet was received from the network. - void OnReceivedPacket(const VCMPacket& packet); + void OnReceivedPacket(uint16_t sequence_number, + const RtpGenericFrameDescriptor& generic_descriptor); // A frame was assembled from packets previously received. // (Should be called even if the frame was composed of a single packet.) diff --git a/modules/video_coding/loss_notification_controller_unittest.cc b/modules/video_coding/loss_notification_controller_unittest.cc index 590cc7716d..62ff889b50 100644 --- a/modules/video_coding/loss_notification_controller_unittest.cc +++ b/modules/video_coding/loss_notification_controller_unittest.cc @@ -20,7 +20,14 @@ namespace webrtc { namespace { -VCMPacket CreatePacket( + +// The information about an RTP packet that is relevant in these tests. +struct Packet { + uint16_t seq_num; + RtpGenericFrameDescriptor descriptor; +}; + +Packet CreatePacket( bool first_in_frame, bool last_in_frame, uint16_t seq_num, @@ -40,24 +47,21 @@ VCMPacket CreatePacket( } } - VCMPacket packet; - packet.seqNum = seq_num; - packet.generic_descriptor = frame_descriptor; - return packet; + return Packet{seq_num, frame_descriptor}; } class PacketStreamCreator final { public: PacketStreamCreator() : seq_num_(0), frame_id_(0), next_is_key_frame_(true) {} - VCMPacket NextPacket() { + Packet NextPacket() { std::vector ref_frame_ids; if (!next_is_key_frame_) { ref_frame_ids.push_back(frame_id_ - 1); } - VCMPacket packet = CreatePacket(true, true, seq_num_++, frame_id_++, - next_is_key_frame_, ref_frame_ids); + Packet packet = CreatePacket(true, true, seq_num_++, frame_id_++, + next_is_key_frame_, ref_frame_ids); next_is_key_frame_ = false; @@ -104,16 +108,15 @@ class LossNotificationControllerBaseTest : public ::testing::Test, decodability_flag); } - void OnReceivedPacket(const VCMPacket& packet) { + void OnReceivedPacket(const Packet& packet) { EXPECT_FALSE(LastKeyFrameRequest()); EXPECT_FALSE(LastLossNotification()); - if (packet.generic_descriptor && - packet.generic_descriptor->FirstPacketInSubFrame()) { + if (packet.descriptor.FirstPacketInSubFrame()) { previous_first_packet_in_frame_ = packet; } - uut_.OnReceivedPacket(packet); + uut_.OnReceivedPacket(packet.seq_num, packet.descriptor); } void OnAssembledFrame(uint16_t first_seq_num, @@ -124,7 +127,7 @@ class LossNotificationControllerBaseTest : public ::testing::Test, ASSERT_TRUE(previous_first_packet_in_frame_); const RtpGenericFrameDescriptor& frame_descriptor = - previous_first_packet_in_frame_->generic_descriptor.value(); + previous_first_packet_in_frame_->descriptor; uut_.OnAssembledFrame(first_seq_num, frame_id, discardable, frame_descriptor.FrameDependenciesDiffs()); @@ -197,7 +200,7 @@ class LossNotificationControllerBaseTest : public ::testing::Test, // of a subsequent frame, OnAssembledFrame is not called, and so this is // note read. Therefore, it's not a problem if it is not cleared when // the frame changes.) - absl::optional previous_first_packet_in_frame_; + absl::optional previous_first_packet_in_frame_; }; class LossNotificationControllerTest @@ -331,8 +334,8 @@ TEST_P(LossNotificationControllerTest, RepeatedPacketsAreIgnored) { const auto key_frame_packet = packet_stream.NextPacket(); OnReceivedPacket(key_frame_packet); - OnAssembledFrame(key_frame_packet.seqNum, - key_frame_packet.generic_descriptor->FrameId(), false); + OnAssembledFrame(key_frame_packet.seq_num, + key_frame_packet.descriptor.FrameId(), false); const bool gap = Bool<0>(); @@ -346,21 +349,12 @@ TEST_P(LossNotificationControllerTest, RepeatedPacketsAreIgnored) { if (gap) { // Loss notification issued because of the gap. This is not the focus of // the test. - ExpectLossNotification(key_frame_packet.seqNum, repeated_packet.seqNum, + ExpectLossNotification(key_frame_packet.seq_num, repeated_packet.seq_num, false); } OnReceivedPacket(repeated_packet); } -// Frames without the generic frame descriptor cannot be properly handled, -// but must not induce a crash. -TEST_F(LossNotificationControllerTest, - IgnoreFramesWithoutGenericFrameDescriptor) { - auto packet = CreatePacket(true, true, 1, 0, true); - packet.generic_descriptor.reset(); - OnReceivedPacket(packet); -} - class LossNotificationControllerTestDecodabilityFlag : public LossNotificationControllerBaseTest { protected: diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc index 87c664e1e2..24fa41a24b 100644 --- a/video/rtp_video_stream_receiver.cc +++ b/video/rtp_video_stream_receiver.cc @@ -333,10 +333,14 @@ int32_t RtpVideoStreamReceiver::OnReceivedPayloadData( if (loss_notification_controller_) { if (is_recovered) { // TODO(bugs.webrtc.org/10336): Implement support for reordering. - RTC_LOG(LS_WARNING) + RTC_LOG(LS_INFO) << "LossNotificationController does not support reordering."; + } else if (!generic_descriptor) { + RTC_LOG(LS_WARNING) << "LossNotificationController requires generic " + "frame descriptor, but it is missing."; } else { - loss_notification_controller_->OnReceivedPacket(packet); + loss_notification_controller_->OnReceivedPacket(rtp_header.sequenceNumber, + *generic_descriptor); } }