From 6893f3c6f0da0c63685289c5e0d1985430ed1ca5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Thu, 31 Jan 2019 08:56:26 +0100 Subject: [PATCH] Move ownership of PlayoutDelayOracle Moved from RtpSender to RtpSenderVideo, since currently the PlayoutDelay extension is used for video only, and configured via RTPVideoHeader. Bug: webrtc:7135 Change-Id: Idfcc90cefea83e40a4e79164d7914cdcd50e41fe Reviewed-on: https://webrtc-review.googlesource.com/c/120357 Reviewed-by: Danil Chapovalov Commit-Queue: Niels Moller Cr-Commit-Position: refs/heads/master@{#26484} --- modules/rtp_rtcp/source/rtp_sender.cc | 26 ++------------- modules/rtp_rtcp/source/rtp_sender.h | 8 ++--- modules/rtp_rtcp/source/rtp_sender_video.cc | 37 +++++++++++++-------- modules/rtp_rtcp/source/rtp_sender_video.h | 9 +++++ 4 files changed, 37 insertions(+), 43 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index b2b6e7cd26..7f42a62e96 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -23,7 +23,6 @@ #include "modules/remote_bitrate_estimator/test/bwe_test_logging.h" #include "modules/rtp_rtcp/include/rtp_cvo.h" #include "modules/rtp_rtcp/source/byte_io.h" -#include "modules/rtp_rtcp/source/playout_delay_oracle.h" #include "modules/rtp_rtcp/source/rtp_generic_frame_descriptor_extension.h" #include "modules/rtp_rtcp/source/rtp_header_extensions.h" #include "modules/rtp_rtcp/source/rtp_packet_to_send.h" @@ -344,7 +343,6 @@ bool RTPSender::SendOutgoingData(FrameType frame_type, const RTPVideoHeader* rtp_header, uint32_t* transport_frame_id_out, int64_t expected_retransmission_time_ms) { - uint32_t ssrc; uint16_t sequence_number; uint32_t rtp_timestamp; { @@ -352,7 +350,6 @@ bool RTPSender::SendOutgoingData(FrameType frame_type, rtc::CritScope lock(&send_critsect_); RTC_DCHECK(ssrc_); - ssrc = *ssrc_; sequence_number = sequence_number_; rtp_timestamp = timestamp_offset_ + capture_timestamp; if (transport_frame_id_out) @@ -388,20 +385,6 @@ bool RTPSender::SendOutgoingData(FrameType frame_type, if (frame_type == kEmptyFrame) return true; - if (rtp_header) { - // TODO(nisse): This way of using PlayoutDelayOracle is a bit awkward. The - // intended way to use it is to call PlayoutDelayToSend at the place where - // the extension is written into the packet, and OnSentPacket later, after - // the final allocation of the sequence number. But currently the - // extension is set in AllocatePacket, where the RTPVideoHeader isn't - // available, so it always calls PlayoutDelayToSend with {-1,-1}. Hence, - // we have to use OnSentPacket early, and record both contents of the - // extension and the sequence number. - playout_delay_oracle_.OnSentPacket( - sequence_number, - playout_delay_oracle_.PlayoutDelayToSend(rtp_header->playout_delay)); - } - result = video_->SendVideo(frame_type, payload_type, rtp_timestamp, capture_time_ms, payload_data, payload_size, fragmentation, rtp_header, @@ -675,8 +658,7 @@ void RTPSender::OnReceivedRtcpReportBlocks( for (const RTCPReportBlock& report_block : report_blocks) { if (ssrc == report_block.source_ssrc) { - playout_delay_oracle_.OnReceivedAck( - report_block.extended_highest_sequence_number); + video_->OnReceivedAck(report_block.extended_highest_sequence_number); } } } @@ -1075,11 +1057,7 @@ std::unique_ptr RTPSender::AllocatePacket() const { packet->ReserveExtension(); packet->ReserveExtension(); packet->ReserveExtension(); - absl::optional playout_delay = - playout_delay_oracle_.PlayoutDelayToSend({-1, -1}); - if (playout_delay) { - packet->SetExtension(*playout_delay); - } + if (!mid_.empty()) { // This is a no-op if the MID header extension is not registered. packet->SetExtension(mid_); diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index d0bc1c3469..401a8ff067 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -25,7 +25,6 @@ #include "modules/rtp_rtcp/include/flexfec_sender.h" #include "modules/rtp_rtcp/include/rtp_header_extension_map.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" -#include "modules/rtp_rtcp/source/playout_delay_oracle.h" #include "modules/rtp_rtcp/source/rtp_packet_history.h" #include "modules/rtp_rtcp/source/rtp_rtcp_config.h" #include "rtc_base/constructor_magic.h" @@ -183,6 +182,7 @@ class RTPSender { absl::optional FlexfecSsrc() const; + // Sends packet to |transport_| or to the pacer, depending on configuration. bool SendToNetwork(std::unique_ptr packet, StorageType storage, RtpPacketSender::Priority priority); @@ -239,6 +239,7 @@ class RTPSender { std::unique_ptr BuildRtxPacket( const RtpPacketToSend& packet); + // Sends packet on to |transport_|, leaving the RTP module. bool SendPacketToNetwork(const RtpPacketToSend& packet, const PacketOptions& options, const PacedPacketInfo& pacing_info); @@ -286,11 +287,6 @@ class RTPSender { RtpHeaderExtensionMap rtp_header_extension_map_ RTC_GUARDED_BY(send_critsect_); - // Tracks the current request for playout delay limits from application - // and decides whether the current RTP frame should include the playout - // delay extension on header. - PlayoutDelayOracle playout_delay_oracle_; - RtpPacketHistory packet_history_; // TODO(brandtr): Remove |flexfec_packet_history_| when the FlexfecSender // is hooked up to the PacedSender. diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index 60ffd32acf..92805f3ccc 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -53,6 +53,7 @@ void BuildRedPayload(const RtpPacketToSend& media_packet, } void AddRtpHeaderExtensions(const RTPVideoHeader& video_header, + const absl::optional& playout_delay, FrameType frame_type, bool set_video_rotation, bool set_color_space, @@ -78,6 +79,10 @@ void AddRtpHeaderExtensions(const RTPVideoHeader& video_header, video_header.video_timing.flags != VideoSendTiming::kInvalid) packet->SetExtension(video_header.video_timing); + // If transmitted, add to all packets; ack logic depends on this. + if (playout_delay) { + packet->SetExtension(*playout_delay); + } if (video_header.generic) { RtpGenericFrameDescriptor generic_descriptor; generic_descriptor.SetFirstPacketInSubFrame(first_packet); @@ -378,6 +383,9 @@ bool RTPSenderVideo::SendVideo(FrameType frame_type, int32_t retransmission_settings; bool set_video_rotation; bool set_color_space = false; + + const absl::optional playout_delay = + playout_delay_oracle_.PlayoutDelayToSend(video_header->playout_delay); { rtc::CritScope cs(&crit_); // According to @@ -441,19 +449,18 @@ bool RTPSenderVideo::SendVideo(FrameType frame_type, auto middle_packet = absl::make_unique(*single_packet); auto last_packet = absl::make_unique(*single_packet); // Simplest way to estimate how much extensions would occupy is to set them. - AddRtpHeaderExtensions(*video_header, frame_type, set_video_rotation, - set_color_space, /*first=*/true, /*last=*/true, - single_packet.get()); - AddRtpHeaderExtensions(*video_header, frame_type, set_video_rotation, - set_color_space, /*first=*/true, /*last=*/false, - first_packet.get()); - AddRtpHeaderExtensions(*video_header, frame_type, set_video_rotation, - set_color_space, /*first=*/false, /*last=*/false, - middle_packet.get()); - AddRtpHeaderExtensions(*video_header, frame_type, set_video_rotation, - set_color_space, /*first=*/false, /*last=*/true, - last_packet.get()); - + AddRtpHeaderExtensions(*video_header, playout_delay, frame_type, + set_video_rotation, set_color_space, /*first=*/true, + /*last=*/true, single_packet.get()); + AddRtpHeaderExtensions(*video_header, playout_delay, frame_type, + set_video_rotation, set_color_space, /*first=*/true, + /*last=*/false, first_packet.get()); + AddRtpHeaderExtensions(*video_header, playout_delay, frame_type, + set_video_rotation, set_color_space, /*first=*/false, + /*last=*/false, middle_packet.get()); + AddRtpHeaderExtensions(*video_header, playout_delay, frame_type, + set_video_rotation, set_color_space, /*first=*/false, + /*last=*/true, last_packet.get()); RTC_DCHECK_GT(packet_capacity, single_packet->headers_size()); RTC_DCHECK_GT(packet_capacity, first_packet->headers_size()); RTC_DCHECK_GT(packet_capacity, middle_packet->headers_size()); @@ -582,6 +589,10 @@ bool RTPSenderVideo::SendVideo(FrameType frame_type, return false; packetized_payload_size += packet->payload_size(); + if (i == 0) { + playout_delay_oracle_.OnSentPacket(packet->SequenceNumber(), + playout_delay); + } // No FEC protection for upper temporal layers, if used. bool protect_packet = temporal_id == 0 || temporal_id == kNoTemporalIdx; diff --git a/modules/rtp_rtcp/source/rtp_sender_video.h b/modules/rtp_rtcp/source/rtp_sender_video.h index ea5865b002..bba549bddb 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.h +++ b/modules/rtp_rtcp/source/rtp_sender_video.h @@ -19,6 +19,7 @@ #include "common_types.h" // NOLINT(build/include) #include "modules/rtp_rtcp/include/flexfec_sender.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" +#include "modules/rtp_rtcp/source/playout_delay_oracle.h" #include "modules/rtp_rtcp/source/rtp_rtcp_config.h" #include "modules/rtp_rtcp/source/rtp_sender.h" #include "modules/rtp_rtcp/source/ulpfec_generator.h" @@ -71,6 +72,10 @@ class RTPSenderVideo { uint32_t FecOverheadRate() const; uint32_t PacketizationOverheadBps() const; + void OnReceivedAck(int64_t extended_highest_sequence_number) { + playout_delay_oracle_.OnReceivedAck(extended_highest_sequence_number); + } + protected: static uint8_t GetTemporalId(const RTPVideoHeader& header); StorageType GetStorageType(uint8_t temporal_id, @@ -135,6 +140,10 @@ class RTPSenderVideo { VideoRotation last_rotation_ RTC_GUARDED_BY(crit_); absl::optional last_color_space_ RTC_GUARDED_BY(crit_); bool transmit_color_space_next_frame_ RTC_GUARDED_BY(crit_); + // Tracks the current request for playout delay limits from application + // and decides whether the current RTP frame should include the playout + // delay extension on header. + PlayoutDelayOracle playout_delay_oracle_; // RED/ULPFEC. int red_payload_type_ RTC_GUARDED_BY(crit_);