Make AudioSendStream to be OverheadObserver
In order to have correct overhead adjustments for ANA in AudioSendStream we need to know both transport and audio packetization overheads in AudioSendStream. This change makes AudioSendStream to be OverheadObserver instead of ChannelSend. This change is required for https://webrtc-review.googlesource.com/c/src/+/115082.
This change was also suggested earlier in TODO:
// TODO(solenberg): Make AudioSendStream an OverheadObserver instead.
https://cs.chromium.org/chromium/src/third_party/webrtc/audio/channel_send.cc?rcl=71b5a7df7794bbc4103296fcd8bd740acebdc901&l=1181
I think we should also consider moving TargetTransferRate observer to AudioSendStream. Since AudioSendStream owns encoder and configures ANA, it makes sense to consolidate all rate (and overhead) calculation there. Added TODO to clean it up in next chanelists.
Bug: webrtc:10150
Change-Id: I48791b998ea00ffde9ec75c6bca8b6dc83001b42
Reviewed-on: https://webrtc-review.googlesource.com/c/119121
Commit-Queue: Anton Sukhanov <sukhanov@webrtc.org>
Reviewed-by: Fredrik Solenberg <solenberg@webrtc.org>
Reviewed-by: Minyue Li <minyue@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26540}diff --git a/audio/audio_send_stream.cc b/audio/audio_send_stream.cc
index 544c936..baf51b2 100644
--- a/audio/audio_send_stream.cc
+++ b/audio/audio_send_stream.cc
@@ -111,6 +111,7 @@
voe::CreateChannelSend(worker_queue,
module_process_thread,
config.media_transport,
+ /*overhead_observer=*/this,
config.send_transport,
rtcp_rtt_stats,
event_log,
@@ -152,7 +153,15 @@
// should be no rtp_transport, and below check should be strengthened to XOR
// (either rtp_transport or media_transport but not both).
RTC_DCHECK(rtp_transport || config.media_transport);
-
+ if (config.media_transport) {
+ // TODO(sukhanov): Currently media transport audio overhead is considered
+ // constant, we will not get overhead_observer calls when using
+ // media_transport. In the future when we introduce RTP media transport we
+ // should make audio overhead interface consistent and work for both RTP and
+ // non-RTP implementations.
+ audio_overhead_per_packet_bytes_ =
+ config.media_transport->GetAudioPacketOverhead();
+ }
rtp_rtcp_module_ = channel_send_->GetRtpRtcp();
RTC_DCHECK(rtp_rtcp_module_);
@@ -480,9 +489,38 @@
}
}
-void AudioSendStream::SetTransportOverhead(int transport_overhead_per_packet) {
+void AudioSendStream::SetTransportOverhead(
+ int transport_overhead_per_packet_bytes) {
RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
- channel_send_->SetTransportOverhead(transport_overhead_per_packet);
+ rtc::CritScope cs(&overhead_per_packet_lock_);
+ transport_overhead_per_packet_bytes_ = transport_overhead_per_packet_bytes;
+ UpdateOverheadForEncoder();
+}
+
+void AudioSendStream::OnOverheadChanged(
+ size_t overhead_bytes_per_packet_bytes) {
+ rtc::CritScope cs(&overhead_per_packet_lock_);
+ audio_overhead_per_packet_bytes_ = overhead_bytes_per_packet_bytes;
+ UpdateOverheadForEncoder();
+}
+
+void AudioSendStream::UpdateOverheadForEncoder() {
+ const size_t overhead_per_packet_bytes = GetPerPacketOverheadBytes();
+ CallEncoder(channel_send_, [&](AudioEncoder* encoder) {
+ if (encoder) {
+ encoder->OnReceivedOverhead(overhead_per_packet_bytes);
+ }
+ });
+}
+
+size_t AudioSendStream::TestOnlyGetPerPacketOverheadBytes() const {
+ rtc::CritScope cs(&overhead_per_packet_lock_);
+ return GetPerPacketOverheadBytes();
+}
+
+size_t AudioSendStream::GetPerPacketOverheadBytes() const {
+ return transport_overhead_per_packet_bytes_ +
+ audio_overhead_per_packet_bytes_;
}
RtpState AudioSendStream::GetRtpState() const {
@@ -568,10 +606,18 @@
new_config.send_codec_spec->format.clockrate_hz);
}
+ // Set currently known overhead (used in ANA, opus only).
+ // If overhead changes later, it will be updated in UpdateOverheadForEncoder.
+ {
+ rtc::CritScope cs(&stream->overhead_per_packet_lock_);
+ encoder->OnReceivedOverhead(stream->GetPerPacketOverheadBytes());
+ }
+
stream->StoreEncoderProperties(encoder->SampleRateHz(),
encoder->NumChannels());
stream->channel_send_->SetEncoder(new_config.send_codec_spec->payload_type,
std::move(encoder));
+
return true;
}
@@ -619,6 +665,12 @@
ReconfigureANA(stream, new_config);
ReconfigureCNG(stream, new_config);
+ // Set currently known overhead (used in ANA, opus only).
+ {
+ rtc::CritScope cs(&stream->overhead_per_packet_lock_);
+ stream->UpdateOverheadForEncoder();
+ }
+
return true;
}
diff --git a/audio/audio_send_stream.h b/audio/audio_send_stream.h
index eb8c952..a1dab79 100644
--- a/audio/audio_send_stream.h
+++ b/audio/audio_send_stream.h
@@ -36,7 +36,8 @@
class AudioSendStream final : public webrtc::AudioSendStream,
public webrtc::BitrateAllocatorObserver,
- public webrtc::PacketFeedbackObserver {
+ public webrtc::PacketFeedbackObserver,
+ public webrtc::OverheadObserver {
public:
AudioSendStream(const webrtc::AudioSendStream::Config& config,
const rtc::scoped_refptr<webrtc::AudioState>& audio_state,
@@ -85,11 +86,19 @@
void OnPacketFeedbackVector(
const std::vector<PacketFeedback>& packet_feedback_vector) override;
- void SetTransportOverhead(int transport_overhead_per_packet);
+ void SetTransportOverhead(int transport_overhead_per_packet_bytes);
+
+ // OverheadObserver override reports audio packetization overhead from
+ // RTP/RTCP module or Media Transport.
+ void OnOverheadChanged(size_t overhead_bytes_per_packet_bytes) override;
RtpState GetRtpState() const;
const voe::ChannelSendInterface* GetChannel() const;
+ // Returns combined per-packet overhead.
+ size_t TestOnlyGetPerPacketOverheadBytes() const
+ RTC_LOCKS_EXCLUDED(overhead_per_packet_lock_);
+
private:
class TimedTransport;
@@ -116,6 +125,15 @@
double bitrate_priority);
void RemoveBitrateObserver();
+ // Sets per-packet overhead on encoded (for ANA) based on current known values
+ // of transport and packetization overheads.
+ void UpdateOverheadForEncoder()
+ RTC_EXCLUSIVE_LOCKS_REQUIRED(overhead_per_packet_lock_);
+
+ // Returns combined per-packet overhead.
+ size_t GetPerPacketOverheadBytes() const
+ RTC_EXCLUSIVE_LOCKS_REQUIRED(overhead_per_packet_lock_);
+
void RegisterCngPayloadType(int payload_type, int clockrate_hz);
rtc::ThreadChecker worker_thread_checker_;
@@ -156,6 +174,16 @@
const std::vector<RtpExtension>& extensions);
static int TransportSeqNumId(const Config& config);
+ rtc::CriticalSection overhead_per_packet_lock_;
+
+ // Current transport overhead (ICE, TURN, etc.)
+ size_t transport_overhead_per_packet_bytes_
+ RTC_GUARDED_BY(overhead_per_packet_lock_) = 0;
+
+ // Current audio packetization overhead (RTP or Media Transport).
+ size_t audio_overhead_per_packet_bytes_
+ RTC_GUARDED_BY(overhead_per_packet_lock_) = 0;
+
RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(AudioSendStream);
};
} // namespace internal
diff --git a/audio/audio_send_stream_unittest.cc b/audio/audio_send_stream_unittest.cc
index 5aaa07e..0f5edd7 100644
--- a/audio/audio_send_stream_unittest.cc
+++ b/audio/audio_send_stream_unittest.cc
@@ -520,9 +520,61 @@
helper.transport(), Ne(nullptr)))
.Times(1);
}
+
+ // ModifyEncoder will be called to re-set overhead.
+ EXPECT_CALL(*helper.channel_send(), ModifyEncoder(testing::_)).Times(1);
+
send_stream->Reconfigure(new_config);
}
+TEST(AudioSendStreamTest, OnTransportOverheadChanged) {
+ ConfigHelper helper(false, true);
+ auto send_stream = helper.CreateAudioSendStream();
+ auto new_config = helper.config();
+
+ // ModifyEncoder will be called on overhead change.
+ EXPECT_CALL(*helper.channel_send(), ModifyEncoder(testing::_)).Times(1);
+
+ const size_t transport_overhead_per_packet_bytes = 333;
+ send_stream->SetTransportOverhead(transport_overhead_per_packet_bytes);
+
+ EXPECT_EQ(transport_overhead_per_packet_bytes,
+ send_stream->TestOnlyGetPerPacketOverheadBytes());
+}
+
+TEST(AudioSendStreamTest, OnAudioOverheadChanged) {
+ ConfigHelper helper(false, true);
+ auto send_stream = helper.CreateAudioSendStream();
+ auto new_config = helper.config();
+
+ // ModifyEncoder will be called on overhead change.
+ EXPECT_CALL(*helper.channel_send(), ModifyEncoder(testing::_)).Times(1);
+
+ const size_t audio_overhead_per_packet_bytes = 555;
+ send_stream->OnOverheadChanged(audio_overhead_per_packet_bytes);
+ EXPECT_EQ(audio_overhead_per_packet_bytes,
+ send_stream->TestOnlyGetPerPacketOverheadBytes());
+}
+
+TEST(AudioSendStreamTest, OnAudioAndTransportOverheadChanged) {
+ ConfigHelper helper(false, true);
+ auto send_stream = helper.CreateAudioSendStream();
+ auto new_config = helper.config();
+
+ // ModifyEncoder will be called when each of overhead changes.
+ EXPECT_CALL(*helper.channel_send(), ModifyEncoder(testing::_)).Times(2);
+
+ const size_t transport_overhead_per_packet_bytes = 333;
+ send_stream->SetTransportOverhead(transport_overhead_per_packet_bytes);
+
+ const size_t audio_overhead_per_packet_bytes = 555;
+ send_stream->OnOverheadChanged(audio_overhead_per_packet_bytes);
+
+ EXPECT_EQ(
+ transport_overhead_per_packet_bytes + audio_overhead_per_packet_bytes,
+ send_stream->TestOnlyGetPerPacketOverheadBytes());
+}
+
// Validates that reconfiguring the AudioSendStream with a Frame encryptor
// correctly reconfigures on the object without crashing.
TEST(AudioSendStreamTest, ReconfigureWithFrameEncryptor) {
diff --git a/audio/channel_send.cc b/audio/channel_send.cc
index 00e48a6..dfbbb57 100644
--- a/audio/channel_send.cc
+++ b/audio/channel_send.cc
@@ -78,7 +78,6 @@
class ChannelSend
: public ChannelSendInterface,
- public OverheadObserver,
public AudioPacketizationCallback, // receive encoded packets from the
// ACM
public TargetTransferRateObserver {
@@ -90,6 +89,7 @@
ChannelSend(rtc::TaskQueue* encoder_queue,
ProcessThread* module_process_thread,
MediaTransportInterface* media_transport,
+ OverheadObserver* overhead_observer,
Transport* rtp_transport,
RtcpRttStats* rtcp_rtt_stats,
RtcEventLog* rtc_event_log,
@@ -160,8 +160,6 @@
// packet.
void ProcessAndEncodeAudio(std::unique_ptr<AudioFrame> audio_frame) override;
- void SetTransportOverhead(size_t transport_overhead_per_packet) override;
-
// The existence of this function alongside OnUplinkPacketLossRate is
// a compromise. We want the encoder to be agnostic of the PLR source, but
// we also don't want it to receive conflicting information from TWCC and
@@ -188,17 +186,11 @@
size_t payloadSize,
const RTPFragmentationHeader* fragmentation) override;
- // From OverheadObserver in the RTP/RTCP module
- void OnOverheadChanged(size_t overhead_bytes_per_packet) override;
-
void OnUplinkPacketLossRate(float packet_loss_rate);
bool InputMute() const;
int SetSendRtpHeaderExtension(bool enable, RTPExtensionType type, int id);
- void UpdateOverheadForEncoder()
- RTC_EXCLUSIVE_LOCKS_REQUIRED(overhead_per_packet_lock_);
-
int32_t SendRtpAudio(FrameType frameType,
uint8_t payloadType,
uint32_t timeStamp,
@@ -254,10 +246,7 @@
// TODO(henrika): can today be accessed on the main thread and on the
// task queue; hence potential race.
bool _includeAudioLevelIndication;
- size_t transport_overhead_per_packet_
- RTC_GUARDED_BY(overhead_per_packet_lock_);
- size_t rtp_overhead_per_packet_ RTC_GUARDED_BY(overhead_per_packet_lock_);
- rtc::CriticalSection overhead_per_packet_lock_;
+
// RtcpBandwidthObserver
const std::unique_ptr<VoERtcpObserver> rtcp_observer_;
@@ -636,6 +625,7 @@
ChannelSend::ChannelSend(rtc::TaskQueue* encoder_queue,
ProcessThread* module_process_thread,
MediaTransportInterface* media_transport,
+ OverheadObserver* overhead_observer,
Transport* rtp_transport,
RtcpRttStats* rtcp_rtt_stats,
RtcEventLog* rtc_event_log,
@@ -650,8 +640,6 @@
input_mute_(false),
previous_frame_muted_(false),
_includeAudioLevelIndication(false),
- transport_overhead_per_packet_(0),
- rtp_overhead_per_packet_(0),
rtcp_observer_(new VoERtcpObserver(this)),
feedback_observer_proxy_(new TransportFeedbackProxy()),
seq_num_allocator_proxy_(new TransportSequenceNumberProxy()),
@@ -677,7 +665,12 @@
// RTPMediaTransport. In this case it means that overhead and bandwidth
// observers should not be called when using media transport.
if (!media_transport_) {
- configuration.overhead_observer = this;
+ // TODO(sukhanov): Overhead observer is only needed for RTP path, because in
+ // media transport audio overhead is currently considered constant (see
+ // getter MediaTransportInterface::GetAudioPacketOverhead). In the future
+ // when we introduce RTP media transport we should make audio overhead
+ // interface consistent and work for both RTP and non-RTP implementations.
+ configuration.overhead_observer = overhead_observer;
configuration.bandwidth_callback = rtcp_observer_.get();
configuration.transport_feedback_callback = feedback_observer_proxy_.get();
}
@@ -704,7 +697,6 @@
if (media_transport_) {
RTC_DLOG(LS_INFO) << "Setting media_transport_ rate observers.";
media_transport_->AddTargetTransferRateObserver(this);
- OnOverheadChanged(media_transport_->GetAudioPacketOverhead());
} else {
RTC_DLOG(LS_INFO) << "Not setting media_transport_ rate observers.";
}
@@ -731,7 +723,6 @@
}
StopSend();
-
int error = audio_coding_->RegisterTransportCallback(NULL);
RTC_DCHECK_EQ(0, error);
@@ -814,7 +805,9 @@
void ChannelSend::ModifyEncoder(
rtc::FunctionView<void(std::unique_ptr<AudioEncoder>*)> modifier) {
- RTC_DCHECK_RUN_ON(&worker_thread_checker_);
+ // This method can be called on the worker thread, module process thread
+ // or network thread. Audio coding is thread safe, so we do not need to
+ // enforce the calling thread.
audio_coding_->ModifyEncoder(modifier);
}
@@ -1145,30 +1138,6 @@
_timeStamp += static_cast<uint32_t>(audio_input->samples_per_channel_);
}
-void ChannelSend::UpdateOverheadForEncoder() {
- size_t overhead_per_packet =
- transport_overhead_per_packet_ + rtp_overhead_per_packet_;
- audio_coding_->ModifyEncoder([&](std::unique_ptr<AudioEncoder>* encoder) {
- if (*encoder) {
- (*encoder)->OnReceivedOverhead(overhead_per_packet);
- }
- });
-}
-
-void ChannelSend::SetTransportOverhead(size_t transport_overhead_per_packet) {
- RTC_DCHECK_RUN_ON(&worker_thread_checker_);
- rtc::CritScope cs(&overhead_per_packet_lock_);
- transport_overhead_per_packet_ = transport_overhead_per_packet;
- UpdateOverheadForEncoder();
-}
-
-// TODO(solenberg): Make AudioSendStream an OverheadObserver instead.
-void ChannelSend::OnOverheadChanged(size_t overhead_bytes_per_packet) {
- rtc::CritScope cs(&overhead_per_packet_lock_);
- rtp_overhead_per_packet_ = overhead_bytes_per_packet;
- UpdateOverheadForEncoder();
-}
-
ANAStats ChannelSend::GetANAStatistics() const {
RTC_DCHECK_RUN_ON(&worker_thread_checker_);
return audio_coding_->GetANAStats();
@@ -1243,6 +1212,9 @@
}
}
+// TODO(sukhanov): Consider moving TargetTransferRate observer to
+// AudioSendStream. Since AudioSendStream owns encoder and configures ANA, it
+// makes sense to consolidate all rate (and overhead) calculation there.
void ChannelSend::OnTargetTransferRate(TargetTransferRate rate) {
RTC_DCHECK(media_transport_);
OnReceivedRtt(rate.network_estimate.round_trip_time.ms());
@@ -1264,6 +1236,7 @@
rtc::TaskQueue* encoder_queue,
ProcessThread* module_process_thread,
MediaTransportInterface* media_transport,
+ OverheadObserver* overhead_observer,
Transport* rtp_transport,
RtcpRttStats* rtcp_rtt_stats,
RtcEventLog* rtc_event_log,
@@ -1272,9 +1245,9 @@
bool extmap_allow_mixed,
int rtcp_report_interval_ms) {
return absl::make_unique<ChannelSend>(
- encoder_queue, module_process_thread, media_transport, rtp_transport,
- rtcp_rtt_stats, rtc_event_log, frame_encryptor, crypto_options,
- extmap_allow_mixed, rtcp_report_interval_ms);
+ encoder_queue, module_process_thread, media_transport, overhead_observer,
+ rtp_transport, rtcp_rtt_stats, rtc_event_log, frame_encryptor,
+ crypto_options, extmap_allow_mixed, rtcp_report_interval_ms);
}
} // namespace voe
diff --git a/audio/channel_send.h b/audio/channel_send.h
index ccd17df..148b1ff 100644
--- a/audio/channel_send.h
+++ b/audio/channel_send.h
@@ -89,7 +89,6 @@
virtual void ProcessAndEncodeAudio(
std::unique_ptr<AudioFrame> audio_frame) = 0;
- virtual void SetTransportOverhead(size_t transport_overhead_per_packet) = 0;
virtual RtpRtcp* GetRtpRtcp() const = 0;
virtual void OnTwccBasedUplinkPacketLossRate(float packet_loss_rate) = 0;
@@ -118,6 +117,7 @@
rtc::TaskQueue* encoder_queue,
ProcessThread* module_process_thread,
MediaTransportInterface* media_transport,
+ OverheadObserver* overhead_observer,
Transport* rtp_transport,
RtcpRttStats* rtcp_rtt_stats,
RtcEventLog* rtc_event_log,
