From 346094cb01ef2ffbf0398f465d61c9a4f77b465c Mon Sep 17 00:00:00 2001 From: "sprang@webrtc.org" Date: Tue, 18 Feb 2014 08:40:33 +0000 Subject: [PATCH] Incorrect overhead calculation when using FEC + RTP extension headers. When frames are fragmented inte multiple RTP packets in order to not exceed a maximum packet size, the header overhead calculation must take into account that FEC redundancy packets may use more than the 12 bytes of the basic RTP header. For example, a csrc list or extension headers may be present. BUG=2899 R=phoglund@webrtc.org, stefan@webrtc.org Review URL: https://webrtc-codereview.appspot.com/8769005 git-svn-id: http://webrtc.googlecode.com/svn/trunk@5562 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../rtp_rtcp/interface/rtp_rtcp_defines.h | 3 + .../modules/rtp_rtcp/source/fec_test_helper.h | 3 - .../source/forward_error_correction.cc | 4 +- webrtc/modules/rtp_rtcp/source/rtp_sender.cc | 6 +- .../rtp_rtcp/source/rtp_sender_video.cc | 10 +- .../test/configurable_frame_size_encoder.cc | 4 +- webrtc/video/video_send_stream_tests.cc | 168 +++++++++++++++--- 7 files changed, 161 insertions(+), 37 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/interface/rtp_rtcp_defines.h b/webrtc/modules/rtp_rtcp/interface/rtp_rtcp_defines.h index b66e927ae..6f99f938d 100644 --- a/webrtc/modules/rtp_rtcp/interface/rtp_rtcp_defines.h +++ b/webrtc/modules/rtp_rtcp/interface/rtp_rtcp_defines.h @@ -27,6 +27,9 @@ namespace webrtc { const int kVideoPayloadTypeFrequency = 90000; +// Minimum RTP header size in bytes. +const uint8_t kRtpHeaderSize = 12; + struct AudioPayload { uint32_t frequency; diff --git a/webrtc/modules/rtp_rtcp/source/fec_test_helper.h b/webrtc/modules/rtp_rtcp/source/fec_test_helper.h index e3c3581be..e6426ea7e 100644 --- a/webrtc/modules/rtp_rtcp/source/fec_test_helper.h +++ b/webrtc/modules/rtp_rtcp/source/fec_test_helper.h @@ -16,9 +16,6 @@ namespace webrtc { -enum { - kRtpHeaderSize = 12 -}; enum { kFecPayloadType = 96 }; diff --git a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc b/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc index 189e1b052..af2cb9e83 100644 --- a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc +++ b/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc @@ -17,15 +17,13 @@ #include #include +#include "webrtc/modules/rtp_rtcp/interface/rtp_rtcp_defines.h" #include "webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.h" #include "webrtc/modules/rtp_rtcp/source/rtp_utility.h" #include "webrtc/system_wrappers/interface/trace.h" namespace webrtc { -// Minimum RTP header size in bytes. -const uint8_t kRtpHeaderSize = 12; - // FEC header size in bytes. const uint8_t kFecHeaderSize = 10; diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc index 0929fd967..0711356e7 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc @@ -301,9 +301,9 @@ uint16_t RTPSender::MaxDataPayloadLength() const { if (audio_configured_) { return max_payload_length_ - RTPHeaderLength(); } else { - return max_payload_length_ - RTPHeaderLength() - - video_->FECPacketOverhead() - ((rtx_) ? 2 : 0); - // Include the FEC/ULP/RED overhead. + return max_payload_length_ - RTPHeaderLength() // RTP overhead. + - video_->FECPacketOverhead() // FEC/ULP/RED overhead. + - ((rtx_) ? 2 : 0); // RTX overhead. } } diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc index 7b36f7cce..10bc252bb 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -14,6 +14,7 @@ #include #include +#include "webrtc/modules/rtp_rtcp/interface/rtp_rtcp_defines.h" #include "webrtc/modules/rtp_rtcp/source/producer_fec.h" #include "webrtc/modules/rtp_rtcp/source/rtp_format_video_generic.h" #include "webrtc/modules/rtp_rtcp/source/rtp_format_vp8.h" @@ -253,8 +254,13 @@ RTPSenderVideo::FECPacketOverhead() const { if (_fecEnabled) { - return ForwardErrorCorrection::PacketOverhead() + - REDForFECHeaderLength; + // Overhead is FEC headers plus RED for FEC header plus anything in RTP + // header beyond the 12 bytes base header (CSRC list, extensions...) + // This reason for the header extensions to be included here is that + // from an FEC viewpoint, they are part of the payload to be protected. + // (The base RTP header is already protected by the FEC header.) + return ForwardErrorCorrection::PacketOverhead() + REDForFECHeaderLength + + (_rtpSender.RTPHeaderLength() - kRtpHeaderSize); } return 0; } diff --git a/webrtc/test/configurable_frame_size_encoder.cc b/webrtc/test/configurable_frame_size_encoder.cc index 0046f5613..b246da357 100644 --- a/webrtc/test/configurable_frame_size_encoder.cc +++ b/webrtc/test/configurable_frame_size_encoder.cc @@ -49,7 +49,9 @@ int32_t ConfigurableFrameSizeEncoder::Encode( encodedImage._timeStamp = inputImage.timestamp(); encodedImage.capture_time_ms_ = inputImage.render_time_ms(); RTPFragmentationHeader* fragmentation = NULL; - callback_->Encoded(encodedImage, codecSpecificInfo, fragmentation); + CodecSpecificInfo specific; + memset(&specific, 0, sizeof(specific)); + callback_->Encoded(encodedImage, &specific, fragmentation); return WEBRTC_VIDEO_CODEC_OK; } diff --git a/webrtc/video/video_send_stream_tests.cc b/webrtc/video/video_send_stream_tests.cc index d3333e0f3..f0c190ea3 100644 --- a/webrtc/video/video_send_stream_tests.cc +++ b/webrtc/video/video_send_stream_tests.cc @@ -33,6 +33,8 @@ namespace webrtc { +enum VideoFormat { kGeneric, kVP8, }; + class VideoSendStreamTest : public ::testing::Test { public: VideoSendStreamTest() @@ -75,6 +77,8 @@ class VideoSendStreamTest : public ::testing::Test { uint8_t retransmit_payload_type, bool enable_pacing); + void TestPacketFragmentationSize(VideoFormat format, bool with_fec); + void SendsSetSsrcs(size_t num_ssrcs, bool send_single_ssrc_first); enum { kNumSendSsrcs = 3 }; @@ -587,44 +591,84 @@ TEST_F(VideoSendStreamTest, RetransmitsNackOverRtxWithPacing) { TestNackRetransmission(kSendRtxSsrc, kSendRtxPayloadType, true); } -TEST_F(VideoSendStreamTest, FragmentsAccordingToMaxPacketSize) { +void VideoSendStreamTest::TestPacketFragmentationSize(VideoFormat format, + bool with_fec) { + static const int kRedPayloadType = 118; + static const int kUlpfecPayloadType = 119; // Observer that verifies that the expected number of packets and bytes // arrive for each frame size, from start_size to stop_size. class FrameFragmentationObserver : public test::RtpRtcpObserver, public EncodedFrameObserver { public: - FrameFragmentationObserver(size_t max_packet_size, + FrameFragmentationObserver(uint32_t max_packet_size, uint32_t start_size, uint32_t stop_size, - test::ConfigurableFrameSizeEncoder* encoder) - : RtpRtcpObserver(30 * 1000), + test::ConfigurableFrameSizeEncoder* encoder, + bool test_generic_packetization, + bool use_fec) + : RtpRtcpObserver(120 * 1000), // Timeout after two minutes. + transport_adapter_(SendTransport()), + encoder_(encoder), max_packet_size_(max_packet_size), + stop_size_(stop_size), + test_generic_packetization_(test_generic_packetization), + use_fec_(use_fec), + packet_count_(0), accumulated_size_(0), accumulated_payload_(0), - stop_size_(stop_size), + fec_packet_received_(false), current_size_rtp_(start_size), - current_size_frame_(start_size), - encoder_(encoder) { + current_size_frame_(start_size) { // Fragmentation required, this test doesn't make sense without it. assert(stop_size > max_packet_size); + transport_adapter_.Enable(); } - virtual Action OnSendRtp(const uint8_t* packet, size_t length) OVERRIDE { + virtual Action OnSendRtp(const uint8_t* packet, size_t size) OVERRIDE { + uint32_t length = static_cast(size); RTPHeader header; - EXPECT_TRUE(parser_->Parse(packet, static_cast(length), &header)); + EXPECT_TRUE(parser_->Parse(packet, length, &header)); EXPECT_LE(length, max_packet_size_); + if (use_fec_) { + uint8_t payload_type = packet[header.headerLength]; + bool is_fec = header.payloadType == kRedPayloadType && + payload_type == kUlpfecPayloadType; + if (is_fec) { + fec_packet_received_ = true; + return SEND_PACKET; + } + } + accumulated_size_ += length; - // Payload size = packet size - minus RTP header, padding and one byte - // generic header. - accumulated_payload_ += - length - (header.headerLength + header.paddingLength + 1); + + if (use_fec_) + TriggerLossReport(header); + + if (test_generic_packetization_) { + uint32_t overhead = header.headerLength + header.paddingLength + + (1 /* Generic header */); + if (use_fec_) + overhead += 1; // RED for FEC header. + accumulated_payload_ += length - overhead; + } // Marker bit set indicates last packet of a frame. if (header.markerBit) { + if (use_fec_ && accumulated_payload_ == current_size_rtp_ - 1) { + // With FEC enabled, frame size is incremented asynchronously, so + // "old" frames one byte too small may arrive. Accept, but don't + // increase expected frame size. + accumulated_size_ = 0; + accumulated_payload_ = 0; + return SEND_PACKET; + } + EXPECT_GE(accumulated_size_, current_size_rtp_); - EXPECT_EQ(accumulated_payload_, current_size_rtp_); + if (test_generic_packetization_) { + EXPECT_EQ(current_size_rtp_, accumulated_payload_); + } // Last packet of frame; reset counters. accumulated_size_ = 0; @@ -633,32 +677,68 @@ TEST_F(VideoSendStreamTest, FragmentsAccordingToMaxPacketSize) { // Done! (Don't increase size again, might arrive more @ stop_size). observation_complete_->Set(); } else { - // Increase next expected frame size. - ++current_size_rtp_; + // Increase next expected frame size. If testing with FEC, make sure + // a FEC packet has been received for this frame size before + // proceeding, to make sure that redundancy packets don't exceed + // size limit. + if (!use_fec_) { + ++current_size_rtp_; + } else if (fec_packet_received_) { + fec_packet_received_ = false; + ++current_size_rtp_; + ++current_size_frame_; + } } } return SEND_PACKET; } + void TriggerLossReport(const RTPHeader& header) { + // Send lossy receive reports to trigger FEC enabling. + if (packet_count_++ % 2 != 0) { + // Receive statistics reporting having lost 50% of the packets. + FakeReceiveStatistics lossy_receive_stats( + kSendSsrc, header.sequenceNumber, packet_count_ / 2, 127); + RTCPSender rtcp_sender( + 0, false, Clock::GetRealTimeClock(), &lossy_receive_stats); + EXPECT_EQ(0, rtcp_sender.RegisterSendTransport(&transport_adapter_)); + + rtcp_sender.SetRTCPStatus(kRtcpNonCompound); + rtcp_sender.SetRemoteSSRC(kSendSsrc); + + RTCPSender::FeedbackState feedback_state; + + EXPECT_EQ(0, rtcp_sender.SendRTCP(feedback_state, kRtcpRr)); + } + } + virtual void EncodedFrameCallback(const EncodedFrame& encoded_frame) { // Increase frame size for next encoded frame, in the context of the // encoder thread. - if (current_size_frame_ < stop_size_) { + if (!use_fec_ && + current_size_frame_.Value() < static_cast(stop_size_)) { ++current_size_frame_; } - encoder_->SetFrameSize(current_size_frame_); + encoder_->SetFrameSize(current_size_frame_.Value()); } private: - size_t max_packet_size_; - size_t accumulated_size_; - size_t accumulated_payload_; + internal::TransportAdapter transport_adapter_; + test::ConfigurableFrameSizeEncoder* const encoder_; + + const uint32_t max_packet_size_; + const uint32_t stop_size_; + const bool test_generic_packetization_; + const bool use_fec_; + + uint32_t packet_count_; + uint32_t accumulated_size_; + uint32_t accumulated_payload_; + bool fec_packet_received_; - uint32_t stop_size_; uint32_t current_size_rtp_; - uint32_t current_size_frame_; - test::ConfigurableFrameSizeEncoder* encoder_; + Atomic32 current_size_frame_; }; // Use a fake encoder to output a frame of every size in the range [90, 290], @@ -668,21 +748,59 @@ TEST_F(VideoSendStreamTest, FragmentsAccordingToMaxPacketSize) { static const uint32_t start = 90; static const uint32_t stop = 290; + // Don't auto increment if FEC is used; continue sending frame size until + // a FEC packet has been received. test::ConfigurableFrameSizeEncoder encoder(stop); encoder.SetFrameSize(start); - FrameFragmentationObserver observer(kMaxPacketSize, start, stop, &encoder); + FrameFragmentationObserver observer( + kMaxPacketSize, start, stop, &encoder, format == kGeneric, with_fec); Call::Config call_config(observer.SendTransport()); scoped_ptr call(Call::Create(call_config)); + observer.SetReceivers(call->Receiver(), NULL); + VideoSendStream::Config send_config = GetSendTestConfig(call.get(), 1); + if (with_fec) { + send_config.rtp.fec.red_payload_type = kRedPayloadType; + send_config.rtp.fec.ulpfec_payload_type = kUlpfecPayloadType; + } + + if (format == kVP8) { + strcpy(send_config.codec.plName, "VP8"); + send_config.codec.codecType = kVideoCodecVP8; + } + send_config.pacing = false; send_config.encoder = &encoder; send_config.rtp.max_packet_size = kMaxPacketSize; send_config.post_encode_callback = &observer; + // Add an extension header, to make the RTP header larger than the base + // length of 12 bytes. + static const uint8_t kAbsSendTimeExtensionId = 13; + send_config.rtp.extensions.push_back( + RtpExtension(RtpExtension::kAbsSendTime, kAbsSendTimeExtensionId)); + RunSendTest(call.get(), send_config, &observer); } +// TODO(sprang): Is there any way of speeding up these tests? +TEST_F(VideoSendStreamTest, FragmentsGenericAccordingToMaxPacketSize) { + TestPacketFragmentationSize(kGeneric, false); +} + +TEST_F(VideoSendStreamTest, FragmentsGenericAccordingToMaxPacketSizeWithFec) { + TestPacketFragmentationSize(kGeneric, true); +} + +TEST_F(VideoSendStreamTest, FragmentsVp8AccordingToMaxPacketSize) { + TestPacketFragmentationSize(kVP8, false); +} + +TEST_F(VideoSendStreamTest, FragmentsVp8AccordingToMaxPacketSizeWithFec) { + TestPacketFragmentationSize(kVP8, true); +} + TEST_F(VideoSendStreamTest, CanChangeSendCodec) { static const uint8_t kFirstPayloadType = 121; static const uint8_t kSecondPayloadType = 122;