Skip to content

Commit

Permalink
Revert "Allow sending to separate payload types for each simulcast in…
Browse files Browse the repository at this point in the history 
…dex."

This reverts commit bcb19c0.

Reason for revert: speculative revert

Original change's description:
> Allow sending to separate payload types for each simulcast index.
>
> This change is for mixed-codec simulcast.
>
> By obtaining the payload type via RtpConfig::GetStreamConfig(),
> the correct payload type can be retrieved regardless of whether
> RtpConfig::stream_configs is initialized or not.
>
> Bug: webrtc:362277533
> Change-Id: I6b2a1ae66356b20a832565ce6729c3ce9e73a161
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/364760
> Reviewed-by: Danil Chapovalov <[email protected]>
> Commit-Queue: Florent Castelli <[email protected]>
> Reviewed-by: Florent Castelli <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#43197}

Bug: webrtc:362277533
Change-Id: I50ac1fa0d9963bf9796f8604542aef5cec653493
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/365161
Commit-Queue: Jeremy Leconte <[email protected]>
Reviewed-by: Danil Chapovalov <[email protected]>
Bot-Commit: [email protected] <[email protected]>
Cr-Commit-Position: refs/heads/main@{#43208}
  • Loading branch information
Jeremy Leconte authored and WebRTC LUCI CQ committed Oct 9, 2024
1 parent 042359f commit f95278f
Show file tree
Hide file tree
Showing 4 changed files with 10 additions and 155 deletions.
27 changes: 0 additions & 27 deletions call/rtp_config.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,31 +239,4 @@ std::optional<std::string> RtpConfig::GetRidForSsrc(uint32_t ssrc) const {
return std::nullopt;
}

RtpStreamConfig RtpConfig::GetStreamConfig(size_t index) const {
// GetStreamConfig function usually returns stream_configs[index], but if
// stream_configs is not initialized (i.e., index >= stream_configs.size()),
// it creates and returns an RtpStreamConfig using fields such as ssrcs, rids,
// payload_name, and payload_type from RtpConfig.
RTC_DCHECK_LT(index, ssrcs.size());
if (index < stream_configs.size()) {
return stream_configs[index];
}
RtpStreamConfig stream_config;
stream_config.ssrc = ssrcs[index];
if (index < rids.size()) {
stream_config.rid = rids[index];
}
stream_config.payload_name = payload_name;
stream_config.payload_type = payload_type;
stream_config.raw_payload = raw_payload;
if (!rtx.ssrcs.empty()) {
RTC_DCHECK_EQ(ssrcs.size(), rtx.ssrcs.size());
auto& stream_config_rtx = stream_config.rtx.emplace();
stream_config_rtx.ssrc = rtx.ssrcs[index];
stream_config_rtx.payload_type = rtx.payload_type;
}

return stream_config;
}

} // namespace webrtc
3 changes: 0 additions & 3 deletions call/rtp_config.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,9 +193,6 @@ struct RtpConfig {
uint32_t GetMediaSsrcAssociatedWithRtxSsrc(uint32_t rtx_ssrc) const;
uint32_t GetMediaSsrcAssociatedWithFlexfecSsrc(uint32_t flexfec_ssrc) const;
std::optional<std::string> GetRidForSsrc(uint32_t ssrc) const;

// Returns send config for RTP stream by provided simulcast `index`.
RtpStreamConfig GetStreamConfig(size_t index) const;
};
} // namespace webrtc
#endif // CALL_RTP_CONFIG_H_
22 changes: 8 additions & 14 deletions call/rtp_video_sender.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -470,14 +470,12 @@ RtpVideoSender::RtpVideoSender(
}

bool fec_enabled = false;
for (size_t i = 0; i < rtp_streams_.size(); i++) {
const RtpStreamSender& stream = rtp_streams_[i];
for (const RtpStreamSender& stream : rtp_streams_) {
// Simulcast has one module for each layer. Set the CNAME on all modules.
stream.rtp_rtcp->SetCNAME(rtp_config_.c_name.c_str());
stream.rtp_rtcp->SetMaxRtpPacketSize(rtp_config_.max_packet_size);
stream.rtp_rtcp->RegisterSendPayloadFrequency(
rtp_config_.GetStreamConfig(i).payload_type,
kVideoPayloadTypeFrequency);
stream.rtp_rtcp->RegisterSendPayloadFrequency(rtp_config_.payload_type,
kVideoPayloadTypeFrequency);
if (stream.fec_generator != nullptr) {
fec_enabled = true;
}
Expand Down Expand Up @@ -578,7 +576,7 @@ EncodedImageCallback::Result RtpVideoSender::OnEncodedImage(
// knowledge of the offset to a single place.
if (!rtp_streams_[simulcast_index].rtp_rtcp->OnSendingRtpFrame(
encoded_image.RtpTimestamp(), encoded_image.capture_time_ms_,
rtp_config_.GetStreamConfig(simulcast_index).payload_type,
rtp_config_.payload_type,
encoded_image._frameType == VideoFrameType::kVideoFrameKey)) {
// The payload router could be active but this module isn't sending.
return Result(Result::ERROR_SEND_FAILED);
Expand Down Expand Up @@ -618,8 +616,7 @@ EncodedImageCallback::Result RtpVideoSender::OnEncodedImage(

bool send_result =
rtp_streams_[simulcast_index].sender_video->SendEncodedImage(
rtp_config_.GetStreamConfig(simulcast_index).payload_type,
codec_type_, rtp_timestamp, encoded_image,
rtp_config_.payload_type, codec_type_, rtp_timestamp, encoded_image,
params_[simulcast_index].GetRtpVideoHeader(
encoded_image, codec_specific_info, frame_id),
expected_retransmission_time);
Expand Down Expand Up @@ -757,12 +754,9 @@ void RtpVideoSender::ConfigureSsrcs(

// Configure RTX payload types.
RTC_DCHECK_GE(rtp_config_.rtx.payload_type, 0);
for (size_t i = 0; i < rtp_streams_.size(); ++i) {
const RtpStreamSender& stream = rtp_streams_[i];
RtpStreamConfig stream_config = rtp_config_.GetStreamConfig(i);
RTC_DCHECK(stream_config.rtx);
stream.rtp_rtcp->SetRtxSendPayloadType(stream_config.rtx->payload_type,
stream_config.payload_type);
for (const RtpStreamSender& stream : rtp_streams_) {
stream.rtp_rtcp->SetRtxSendPayloadType(rtp_config_.rtx.payload_type,
rtp_config_.payload_type);
stream.rtp_rtcp->SetRtxSendStatus(kRtxRetransmitted |
kRtxRedundantPayloads);
}
Expand Down
113 changes: 2 additions & 111 deletions call/rtp_video_sender_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ using ::testing::SaveArg;
using ::testing::SizeIs;

const int8_t kPayloadType = 96;
const int8_t kPayloadType2 = 98;
const uint32_t kSsrc1 = 12345;
const uint32_t kSsrc2 = 23456;
const uint32_t kRtxSsrc1 = 34567;
Expand Down Expand Up @@ -132,8 +131,7 @@ VideoSendStream::Config CreateVideoSendStreamConfig(
Transport* transport,
const std::vector<uint32_t>& ssrcs,
const std::vector<uint32_t>& rtx_ssrcs,
int payload_type,
rtc::ArrayView<const int> payload_types) {
int payload_type) {
VideoSendStream::Config config(transport);
config.rtp.ssrcs = ssrcs;
config.rtp.rtx.ssrcs = rtx_ssrcs;
Expand All @@ -145,20 +143,6 @@ VideoSendStream::Config CreateVideoSendStreamConfig(
config.rtp.extensions.emplace_back(RtpDependencyDescriptorExtension::Uri(),
kDependencyDescriptorExtensionId);
config.rtp.extmap_allow_mixed = true;

if (!payload_types.empty()) {
RTC_CHECK_EQ(payload_types.size(), ssrcs.size());
for (size_t i = 0; i < ssrcs.size(); ++i) {
auto& stream_config = config.rtp.stream_configs.emplace_back();
stream_config.ssrc = ssrcs[i];
stream_config.payload_type = payload_types[i];
if (i < rtx_ssrcs.size()) {
auto& rtx = stream_config.rtx.emplace();
rtx.ssrc = rtx_ssrcs[i];
rtx.payload_type = payload_types[i] + 1;
}
}
}
return config;
}

Expand All @@ -171,7 +155,6 @@ class RtpVideoSenderTestFixture {
const std::map<uint32_t, RtpPayloadState>& suspended_payload_states,
FrameCountObserver* frame_count_observer,
rtc::scoped_refptr<FrameTransformerInterface> frame_transformer,
const std::vector<int>& payload_types,
const FieldTrialsView* field_trials = nullptr)
: time_controller_(Timestamp::Millis(1000000)),
env_(CreateEnvironment(&field_trials_,
Expand All @@ -181,8 +164,7 @@ class RtpVideoSenderTestFixture {
config_(CreateVideoSendStreamConfig(&transport_,
ssrcs,
rtx_ssrcs,
payload_type,
payload_types)),
payload_type)),
bitrate_config_(GetBitrateConfig()),
transport_controller_(
RtpTransportConfig{.env = env_, .bitrate_config = bitrate_config_}),
Expand All @@ -204,22 +186,6 @@ class RtpVideoSenderTestFixture {
std::make_unique<FecControllerDefault>(env_), nullptr, CryptoOptions{},
frame_transformer);
}
RtpVideoSenderTestFixture(
const std::vector<uint32_t>& ssrcs,
const std::vector<uint32_t>& rtx_ssrcs,
int payload_type,
const std::map<uint32_t, RtpPayloadState>& suspended_payload_states,
FrameCountObserver* frame_count_observer,
rtc::scoped_refptr<FrameTransformerInterface> frame_transformer,
const FieldTrialsView* field_trials = nullptr)
: RtpVideoSenderTestFixture(ssrcs,
rtx_ssrcs,
payload_type,
suspended_payload_states,
frame_count_observer,
frame_transformer,
/*payload_types=*/{},
field_trials) {}

RtpVideoSenderTestFixture(
const std::vector<uint32_t>& ssrcs,
Expand All @@ -234,7 +200,6 @@ class RtpVideoSenderTestFixture {
suspended_payload_states,
frame_count_observer,
/*frame_transformer=*/nullptr,
/*payload_types=*/{},
field_trials) {}

RtpVideoSenderTestFixture(
Expand All @@ -249,7 +214,6 @@ class RtpVideoSenderTestFixture {
suspended_payload_states,
/*frame_count_observer=*/nullptr,
/*frame_transformer=*/nullptr,
/*payload_types=*/{},
field_trials) {}

~RtpVideoSenderTestFixture() { SetSending(false); }
Expand Down Expand Up @@ -987,79 +951,6 @@ TEST(RtpVideoSenderTest,
EXPECT_EQ(dd_s1.frame_number(), 1002);
}

TEST(RtpVideoSenderTest, MixedCodecSimulcastPayloadType) {
// When multiple payload types are set, verify that the payload type switches
// corresponding to the simulcast index.
RtpVideoSenderTestFixture test({kSsrc1, kSsrc2}, {kRtxSsrc1, kRtxSsrc2},
kPayloadType, {}, nullptr, nullptr,
{kPayloadType, kPayloadType2});
test.SetSending(true);

std::vector<uint16_t> rtp_sequence_numbers;
std::vector<RtpPacket> sent_packets;
EXPECT_CALL(test.transport(), SendRtp)
.Times(3)
.WillRepeatedly([&](rtc::ArrayView<const uint8_t> packet,
const PacketOptions& options) -> bool {
RtpPacket& rtp_packet = sent_packets.emplace_back();
EXPECT_TRUE(rtp_packet.Parse(packet));
rtp_sequence_numbers.push_back(rtp_packet.SequenceNumber());
return true;
});

const uint8_t kPayload[1] = {'a'};
EncodedImage encoded_image;
encoded_image.SetEncodedData(
EncodedImageBuffer::Create(kPayload, sizeof(kPayload)));

CodecSpecificInfo codec_specific;
codec_specific.codecType = VideoCodecType::kVideoCodecVP8;

encoded_image.SetSimulcastIndex(0);
ASSERT_EQ(test.router()->OnEncodedImage(encoded_image, &codec_specific).error,
EncodedImageCallback::Result::OK);
ASSERT_EQ(test.router()->OnEncodedImage(encoded_image, &codec_specific).error,
EncodedImageCallback::Result::OK);
encoded_image.SetSimulcastIndex(1);
ASSERT_EQ(test.router()->OnEncodedImage(encoded_image, &codec_specific).error,
EncodedImageCallback::Result::OK);

test.AdvanceTime(TimeDelta::Millis(33));
ASSERT_THAT(sent_packets, SizeIs(3));
EXPECT_EQ(sent_packets[0].PayloadType(), kPayloadType);
EXPECT_EQ(sent_packets[1].PayloadType(), kPayloadType);
EXPECT_EQ(sent_packets[2].PayloadType(), kPayloadType2);

// Verify that NACK is sent to the RTX payload type corresponding to the
// payload type.
rtcp::Nack nack1, nack2;
nack1.SetMediaSsrc(kSsrc1);
nack2.SetMediaSsrc(kSsrc2);
nack1.SetPacketIds({rtp_sequence_numbers[0], rtp_sequence_numbers[1]});
nack2.SetPacketIds({rtp_sequence_numbers[2]});
rtc::Buffer nack_buffer1 = nack1.Build();
rtc::Buffer nack_buffer2 = nack2.Build();

std::vector<RtpPacket> sent_rtx_packets;
EXPECT_CALL(test.transport(), SendRtp)
.Times(3)
.WillRepeatedly([&](rtc::ArrayView<const uint8_t> packet,
const PacketOptions& options) {
RtpPacket& rtp_packet = sent_rtx_packets.emplace_back();
EXPECT_TRUE(rtp_packet.Parse(packet));
return true;
});
test.router()->DeliverRtcp(nack_buffer1.data(), nack_buffer1.size());
test.router()->DeliverRtcp(nack_buffer2.data(), nack_buffer2.size());

test.AdvanceTime(TimeDelta::Millis(33));

ASSERT_THAT(sent_rtx_packets, SizeIs(3));
EXPECT_EQ(sent_rtx_packets[0].PayloadType(), kPayloadType + 1);
EXPECT_EQ(sent_rtx_packets[1].PayloadType(), kPayloadType + 1);
EXPECT_EQ(sent_rtx_packets[2].PayloadType(), kPayloadType2 + 1);
}

TEST(RtpVideoSenderTest,
SupportsDependencyDescriptorForVp8NotProvidedByEncoder) {
constexpr uint8_t kPayload[1] = {'a'};
Expand Down

0 comments on commit f95278f

Please sign in to comment.