RTCError as return type for PeerConnectionInterface::SetConfiguration

Bug: None
Change-Id: I6dd7378ceac617e29945d72906cb8e2e0bd49538
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/149166
Reviewed-by: Kári Helgason <kthelgason@webrtc.org>
Reviewed-by: Sami Kalliomäki <sakal@webrtc.org>
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Commit-Queue: Niels Moller <nisse@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28910}
This commit is contained in:
Niels Möller 2019-08-19 09:58:17 +02:00 committed by Commit Bot
parent 7627fdd68a
commit 2579f0c584
12 changed files with 84 additions and 102 deletions

View File

@ -135,9 +135,9 @@ bool PeerConnectionInterface::SetConfiguration(
return false;
}
bool PeerConnectionInterface::SetConfiguration(
RTCError PeerConnectionInterface::SetConfiguration(
const PeerConnectionInterface::RTCConfiguration& config) {
return false;
return RTCError();
}
bool PeerConnectionInterface::RemoveIceCandidates(

View File

@ -996,15 +996,15 @@ class RTC_EXPORT PeerConnectionInterface : public rtc::RefCountInterface {
// - INVALID_PARAMETER if a TURN server is missing |username| or |password|.
// - INTERNAL_ERROR if an unexpected error occurred.
//
// TODO(deadbeef): Make this pure virtual once all Chrome subclasses of
// PeerConnectionInterface implement it.
// TODO(nisse): Deprecated, migrate to the method with an RTCError return
// value, then delete this one.
virtual bool SetConfiguration(
const PeerConnectionInterface::RTCConfiguration& config,
RTCError* error);
// Version without error output param for backwards compatibility.
// TODO(deadbeef): Remove once chromium is updated.
virtual bool SetConfiguration(
// TODO(nisse): Make this pure virtual once all Chrome subclasses of
// PeerConnectionInterface implement it.
virtual RTCError SetConfiguration(
const PeerConnectionInterface::RTCConfiguration& config);
// Provides a remote candidate to the ICE Agent.
@ -1100,7 +1100,7 @@ class RTC_EXPORT PeerConnectionInterface : public rtc::RefCountInterface {
virtual bool StartRtcEventLog(std::unique_ptr<RtcEventLogOutput> output);
// Stops logging the RtcEventLog.
// TODO(ivoc): Make this pure virtual when Chrome is updated.
// TODO(ivoc): Make this pure virtual when Chrome is updat ed.
virtual void StopRtcEventLog() {}
// Terminates all media, closes the transports, and in general releases any

View File

@ -112,7 +112,7 @@ PROXY_METHOD2(bool,
SetConfiguration,
const PeerConnectionInterface::RTCConfiguration&,
RTCError*)
PROXY_METHOD1(bool,
PROXY_METHOD1(RTCError,
SetConfiguration,
const PeerConnectionInterface::RTCConfiguration&)
PROXY_METHOD1(bool, AddIceCandidate, const IceCandidateInterface*)

View File

@ -111,7 +111,7 @@ class MockPeerConnectionInterface
bool(const PeerConnectionInterface::RTCConfiguration&,
RTCError*));
MOCK_METHOD1(SetConfiguration,
bool(const PeerConnectionInterface::RTCConfiguration&));
RTCError(const PeerConnectionInterface::RTCConfiguration&));
MOCK_METHOD1(AddIceCandidate, bool(const IceCandidateInterface*));
MOCK_METHOD1(RemoveIceCandidates,
bool(const std::vector<cricket::Candidate>&));

View File

@ -256,22 +256,6 @@ uint32_t ConvertIceTransportTypeToCandidateFilter(
return cricket::CF_NONE;
}
// Helper to set an error and return from a method.
bool SafeSetError(webrtc::RTCErrorType type, webrtc::RTCError* error) {
if (error) {
error->set_type(type);
}
return type == webrtc::RTCErrorType::NONE;
}
bool SafeSetError(webrtc::RTCError error, webrtc::RTCError* error_out) {
bool ok = error.ok();
if (error_out) {
*error_out = std::move(error);
}
return ok;
}
std::string GetSignalingStateString(
PeerConnectionInterface::SignalingState state) {
switch (state) {
@ -3469,12 +3453,22 @@ PeerConnectionInterface::RTCConfiguration PeerConnection::GetConfiguration() {
bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration,
RTCError* error) {
RTCError result = SetConfiguration(configuration);
bool success = result.ok();
if (error) {
*error = std::move(result);
}
return success;
}
RTCError PeerConnection::SetConfiguration(
const RTCConfiguration& configuration) {
RTC_DCHECK_RUN_ON(signaling_thread());
RTC_DCHECK_RUNS_SERIALIZED(&use_media_transport_race_checker_);
TRACE_EVENT0("webrtc", "PeerConnection::SetConfiguration");
if (IsClosed()) {
RTC_LOG(LS_ERROR) << "SetConfiguration: PeerConnection is closed.";
return SafeSetError(RTCErrorType::INVALID_STATE, error);
LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_STATE,
"SetConfiguration: PeerConnection is closed.");
}
// According to JSEP, after setLocalDescription, changing the candidate pool
@ -3482,60 +3476,60 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration,
// in new candidates being gathered.
if (local_description() && configuration.ice_candidate_pool_size !=
configuration_.ice_candidate_pool_size) {
RTC_LOG(LS_ERROR) << "Can't change candidate pool size after calling "
"SetLocalDescription.";
return SafeSetError(RTCErrorType::INVALID_MODIFICATION, error);
LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_MODIFICATION,
"Can't change candidate pool size after calling "
"SetLocalDescription.");
}
if (local_description() &&
configuration.use_media_transport != configuration_.use_media_transport) {
RTC_LOG(LS_ERROR) << "Can't change media_transport after calling "
"SetLocalDescription.";
return SafeSetError(RTCErrorType::INVALID_MODIFICATION, error);
LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_MODIFICATION,
"Can't change media_transport after calling "
"SetLocalDescription.");
}
if (remote_description() &&
configuration.use_media_transport != configuration_.use_media_transport) {
RTC_LOG(LS_ERROR) << "Can't change media_transport after calling "
"SetRemoteDescription.";
return SafeSetError(RTCErrorType::INVALID_MODIFICATION, error);
LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_MODIFICATION,
"Can't change media_transport after calling "
"SetRemoteDescription.");
}
if (local_description() &&
configuration.use_media_transport_for_data_channels !=
configuration_.use_media_transport_for_data_channels) {
RTC_LOG(LS_ERROR) << "Can't change media_transport_for_data_channels "
"after calling SetLocalDescription.";
return SafeSetError(RTCErrorType::INVALID_MODIFICATION, error);
LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_MODIFICATION,
"Can't change media_transport_for_data_channels "
"after calling SetLocalDescription.");
}
if (remote_description() &&
configuration.use_media_transport_for_data_channels !=
configuration_.use_media_transport_for_data_channels) {
RTC_LOG(LS_ERROR) << "Can't change media_transport_for_data_channels "
"after calling SetRemoteDescription.";
return SafeSetError(RTCErrorType::INVALID_MODIFICATION, error);
LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_MODIFICATION,
"Can't change media_transport_for_data_channels "
"after calling SetRemoteDescription.");
}
if (local_description() &&
configuration.crypto_options != configuration_.crypto_options) {
RTC_LOG(LS_ERROR) << "Can't change crypto_options after calling "
"SetLocalDescription.";
return SafeSetError(RTCErrorType::INVALID_MODIFICATION, error);
LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_MODIFICATION,
"Can't change crypto_options after calling "
"SetLocalDescription.");
}
if (local_description() && configuration.use_datagram_transport !=
configuration_.use_datagram_transport) {
RTC_LOG(LS_ERROR) << "Can't change use_datagram_transport "
"after calling SetLocalDescription.";
return SafeSetError(RTCErrorType::INVALID_MODIFICATION, error);
LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_MODIFICATION,
"Can't change use_datagram_transport "
"after calling SetLocalDescription.");
}
if (remote_description() && configuration.use_datagram_transport !=
configuration_.use_datagram_transport) {
RTC_LOG(LS_ERROR) << "Can't change use_datagram_transport "
"after calling SetRemoteDescription.";
return SafeSetError(RTCErrorType::INVALID_MODIFICATION, error);
LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_MODIFICATION,
"Can't change use_datagram_transport "
"after calling SetRemoteDescription.");
}
if (configuration.use_media_transport_for_data_channels ||
@ -3578,21 +3572,21 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration,
configuration.use_media_transport_for_data_channels;
modified_config.use_datagram_transport = configuration.use_datagram_transport;
if (configuration != modified_config) {
RTC_LOG(LS_ERROR) << "Modifying the configuration in an unsupported way.";
return SafeSetError(RTCErrorType::INVALID_MODIFICATION, error);
LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_MODIFICATION,
"Modifying the configuration in an unsupported way.");
}
// Validate the modified configuration.
RTCError validate_error = ValidateConfiguration(modified_config);
if (!validate_error.ok()) {
return SafeSetError(std::move(validate_error), error);
return validate_error;
}
// Note that this isn't possible through chromium, since it's an unsigned
// short in WebIDL.
if (configuration.ice_candidate_pool_size < 0 ||
configuration.ice_candidate_pool_size > static_cast<int>(UINT16_MAX)) {
return SafeSetError(RTCErrorType::INVALID_RANGE, error);
return RTCError(RTCErrorType::INVALID_RANGE);
}
// Parse ICE servers before hopping to network thread.
@ -3601,7 +3595,7 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration,
RTCErrorType parse_error =
ParseIceServers(configuration.servers, &stun_servers, &turn_servers);
if (parse_error != RTCErrorType::NONE) {
return SafeSetError(parse_error, error);
return RTCError(parse_error);
}
// Note if STUN or TURN servers were supplied.
if (!stun_servers.empty()) {
@ -3621,8 +3615,8 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration,
modified_config.turn_customizer,
modified_config.stun_candidate_keepalive_interval,
static_cast<bool>(local_description())))) {
RTC_LOG(LS_ERROR) << "Failed to apply configuration to PortAllocator.";
return SafeSetError(RTCErrorType::INTERNAL_ERROR, error);
LOG_AND_RETURN_ERROR(RTCErrorType::INTERNAL_ERROR,
"Failed to apply configuration to PortAllocator.");
}
// As described in JSEP, calling setConfiguration with new ICE servers or
@ -3652,7 +3646,7 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration,
configuration_ = modified_config;
use_media_transport_ = configuration.use_media_transport;
return SafeSetError(RTCErrorType::NONE, error);
return RTCError::OK();
}
bool PeerConnection::AddIceCandidate(

View File

@ -211,10 +211,8 @@ class PeerConnection : public PeerConnectionInternal,
bool SetConfiguration(
const PeerConnectionInterface::RTCConfiguration& configuration,
RTCError* error) override;
bool SetConfiguration(
const PeerConnectionInterface::RTCConfiguration& configuration) override {
return SetConfiguration(configuration, nullptr);
}
RTCError SetConfiguration(
const PeerConnectionInterface::RTCConfiguration& configuration) override;
bool AddIceCandidate(const IceCandidateInterface* candidate) override;
bool RemoveIceCandidates(
const std::vector<cricket::Candidate>& candidates) override;

View File

@ -587,9 +587,7 @@ TEST_F(PeerConnectionUsageHistogramTest, FingerprintStunTurnInReconfiguration) {
configuration.servers.push_back(server);
auto caller = CreatePeerConnection();
ASSERT_TRUE(caller);
RTCError error;
caller->pc()->SetConfiguration(configuration, &error);
ASSERT_TRUE(error.ok());
ASSERT_TRUE(caller->pc()->SetConfiguration(configuration).ok());
caller->pc()->Close();
int expected_fingerprint =
MakeUsageFingerprint({PeerConnection::UsageEvent::STUN_SERVER_ADDED,

View File

@ -1229,8 +1229,7 @@ TEST_F(PeerConnectionIceConfigTest, SetStunCandidateKeepaliveInterval) {
port_allocator_->stun_candidate_keepalive_interval();
EXPECT_EQ(actual_stun_keepalive_interval.value_or(-1), 123);
config.stun_candidate_keepalive_interval = 321;
RTCError error;
pc_->SetConfiguration(config, &error);
ASSERT_TRUE(pc_->SetConfiguration(config).ok());
actual_stun_keepalive_interval =
port_allocator_->stun_candidate_keepalive_interval();
EXPECT_EQ(actual_stun_keepalive_interval.value_or(-1), 321);

View File

@ -1423,7 +1423,7 @@ TEST_P(PeerConnectionInterfaceTest, GetConfigurationAfterSetConfiguration) {
config.type = PeerConnectionInterface::kRelay;
config.use_media_transport = true;
config.use_media_transport_for_data_channels = true;
EXPECT_TRUE(pc_->SetConfiguration(config));
EXPECT_TRUE(pc_->SetConfiguration(config).ok());
PeerConnectionInterface::RTCConfiguration returned_config =
pc_->GetConfiguration();
@ -1438,7 +1438,7 @@ TEST_P(PeerConnectionInterfaceTest, SetConfigurationFailsAfterClose) {
pc_->Close();
EXPECT_FALSE(
pc_->SetConfiguration(PeerConnectionInterface::RTCConfiguration()));
pc_->SetConfiguration(PeerConnectionInterface::RTCConfiguration()).ok());
}
TEST_F(PeerConnectionInterfaceTestPlanB, AddStreams) {
@ -2429,7 +2429,7 @@ TEST_P(PeerConnectionInterfaceTest, SetConfigurationChangesIceServers) {
PeerConnectionInterface::IceServer server;
server.uri = "stun:test_hostname";
config.servers.push_back(server);
EXPECT_TRUE(pc_->SetConfiguration(config));
EXPECT_TRUE(pc_->SetConfiguration(config).ok());
EXPECT_EQ(1u, port_allocator_->stun_servers().size());
EXPECT_EQ("test_hostname",
@ -2440,7 +2440,7 @@ TEST_P(PeerConnectionInterfaceTest, SetConfigurationChangesCandidateFilter) {
CreatePeerConnection();
PeerConnectionInterface::RTCConfiguration config = pc_->GetConfiguration();
config.type = PeerConnectionInterface::kRelay;
EXPECT_TRUE(pc_->SetConfiguration(config));
EXPECT_TRUE(pc_->SetConfiguration(config).ok());
EXPECT_EQ(cricket::CF_RELAY, port_allocator_->candidate_filter());
}
@ -2452,7 +2452,7 @@ TEST_P(PeerConnectionInterfaceTest, SetConfigurationChangesPruneTurnPortsFlag) {
EXPECT_FALSE(port_allocator_->prune_turn_ports());
config.prune_turn_ports = true;
EXPECT_TRUE(pc_->SetConfiguration(config));
EXPECT_TRUE(pc_->SetConfiguration(config).ok());
EXPECT_TRUE(port_allocator_->prune_turn_ports());
}
@ -2465,7 +2465,7 @@ TEST_P(PeerConnectionInterfaceTest, SetConfigurationChangesIceCheckInterval) {
CreatePeerConnection(config);
config = pc_->GetConfiguration();
config.ice_check_min_interval = 100;
EXPECT_TRUE(pc_->SetConfiguration(config));
EXPECT_TRUE(pc_->SetConfiguration(config).ok());
config = pc_->GetConfiguration();
EXPECT_EQ(config.ice_check_min_interval, 100);
}
@ -2479,7 +2479,7 @@ TEST_P(PeerConnectionInterfaceTest,
EXPECT_FALSE(config.surface_ice_candidates_on_ice_transport_type_changed);
config.surface_ice_candidates_on_ice_transport_type_changed = true;
EXPECT_TRUE(pc_->SetConfiguration(config));
EXPECT_TRUE(pc_->SetConfiguration(config).ok());
config = pc_->GetConfiguration();
EXPECT_TRUE(config.surface_ice_candidates_on_ice_transport_type_changed);
}
@ -2495,7 +2495,7 @@ TEST_P(PeerConnectionInterfaceTest,
server.uri = kStunAddressOnly;
config.servers.push_back(server);
config.type = PeerConnectionInterface::kRelay;
EXPECT_TRUE(pc_->SetConfiguration(config));
EXPECT_TRUE(pc_->SetConfiguration(config).ok());
const cricket::FakePortAllocatorSession* session =
static_cast<const cricket::FakePortAllocatorSession*>(
@ -2512,18 +2512,17 @@ TEST_P(PeerConnectionInterfaceTest,
// Start by setting a size of 1.
PeerConnectionInterface::RTCConfiguration config = pc_->GetConfiguration();
config.ice_candidate_pool_size = 1;
EXPECT_TRUE(pc_->SetConfiguration(config));
EXPECT_TRUE(pc_->SetConfiguration(config).ok());
// Set remote offer; can still change pool size at this point.
CreateOfferAsRemoteDescription();
config.ice_candidate_pool_size = 2;
EXPECT_TRUE(pc_->SetConfiguration(config));
EXPECT_TRUE(pc_->SetConfiguration(config).ok());
// Set local answer; now it's too late.
CreateAnswerAsLocalDescription();
config.ice_candidate_pool_size = 3;
RTCError error;
EXPECT_FALSE(pc_->SetConfiguration(config, &error));
RTCError error = pc_->SetConfiguration(config);
EXPECT_EQ(RTCErrorType::INVALID_MODIFICATION, error.type());
}
@ -2536,7 +2535,7 @@ TEST_P(PeerConnectionInterfaceTest,
// Set a larger-than-necessary size.
PeerConnectionInterface::RTCConfiguration config = pc_->GetConfiguration();
config.ice_candidate_pool_size = 4;
EXPECT_TRUE(pc_->SetConfiguration(config));
EXPECT_TRUE(pc_->SetConfiguration(config).ok());
// Do offer/answer.
CreateOfferAsRemoteDescription();
@ -2555,7 +2554,7 @@ TEST_P(PeerConnectionInterfaceTest, PooledSessionsDiscardedAfterClose) {
PeerConnectionInterface::RTCConfiguration config = pc_->GetConfiguration();
config.ice_candidate_pool_size = 3;
EXPECT_TRUE(pc_->SetConfiguration(config));
EXPECT_TRUE(pc_->SetConfiguration(config).ok());
pc_->Close();
// Expect no pooled sessions to be left.
@ -2578,22 +2577,19 @@ TEST_P(PeerConnectionInterfaceTest,
pc_->GetConfiguration();
modified_config.bundle_policy =
PeerConnectionInterface::kBundlePolicyMaxBundle;
RTCError error;
EXPECT_FALSE(pc_->SetConfiguration(modified_config, &error));
RTCError error = pc_->SetConfiguration(modified_config);
EXPECT_EQ(RTCErrorType::INVALID_MODIFICATION, error.type());
modified_config = pc_->GetConfiguration();
modified_config.rtcp_mux_policy =
PeerConnectionInterface::kRtcpMuxPolicyRequire;
error.set_type(RTCErrorType::NONE);
EXPECT_FALSE(pc_->SetConfiguration(modified_config, &error));
error = pc_->SetConfiguration(modified_config);
EXPECT_EQ(RTCErrorType::INVALID_MODIFICATION, error.type());
modified_config = pc_->GetConfiguration();
modified_config.continual_gathering_policy =
PeerConnectionInterface::GATHER_CONTINUALLY;
error.set_type(RTCErrorType::NONE);
EXPECT_FALSE(pc_->SetConfiguration(modified_config, &error));
error = pc_->SetConfiguration(modified_config);
EXPECT_EQ(RTCErrorType::INVALID_MODIFICATION, error.type());
}
@ -2606,13 +2602,11 @@ TEST_P(PeerConnectionInterfaceTest,
config = pc_->GetConfiguration();
config.ice_candidate_pool_size = -1;
RTCError error;
EXPECT_FALSE(pc_->SetConfiguration(config, &error));
RTCError error = pc_->SetConfiguration(config);
EXPECT_EQ(RTCErrorType::INVALID_RANGE, error.type());
config.ice_candidate_pool_size = INT_MAX;
error.set_type(RTCErrorType::NONE);
EXPECT_FALSE(pc_->SetConfiguration(config, &error));
error = pc_->SetConfiguration(config);
EXPECT_EQ(RTCErrorType::INVALID_RANGE, error.type());
}
@ -2627,8 +2621,7 @@ TEST_P(PeerConnectionInterfaceTest,
PeerConnectionInterface::IceServer bad_server;
bad_server.uri = "stunn:www.example.com";
config.servers.push_back(bad_server);
RTCError error;
EXPECT_FALSE(pc_->SetConfiguration(config, &error));
RTCError error = pc_->SetConfiguration(config);
EXPECT_EQ(RTCErrorType::SYNTAX_ERROR, error.type());
}
@ -3294,7 +3287,7 @@ TEST_P(PeerConnectionInterfaceTest, SetConfigurationCausingIceRestart) {
// Change ICE policy, which should trigger an ICE restart on the next offer.
config.type = PeerConnectionInterface::kAll;
EXPECT_TRUE(pc_->SetConfiguration(config));
EXPECT_TRUE(pc_->SetConfiguration(config).ok());
CreateOfferAsLocalDescription();
// Grab the new ufrags.
@ -3328,7 +3321,7 @@ TEST_P(PeerConnectionInterfaceTest, SetConfigurationNotCausingIceRestart) {
// Call SetConfiguration with a config identical to what the PC was
// constructed with.
EXPECT_TRUE(pc_->SetConfiguration(config));
EXPECT_TRUE(pc_->SetConfiguration(config).ok());
CreateOfferAsLocalDescription();
// Grab the new ufrags.
@ -3359,7 +3352,7 @@ TEST_P(PeerConnectionInterfaceTest, SetConfigurationCausingPartialIceRestart) {
// Change ICE policy, which should set the "needs-ice-restart" flag.
config.type = PeerConnectionInterface::kAll;
EXPECT_TRUE(pc_->SetConfiguration(config));
EXPECT_TRUE(pc_->SetConfiguration(config).ok());
// Do ICE restart for the first m= section, initiated by remote peer.
std::unique_ptr<webrtc::SessionDescriptionInterface> remote_offer(

View File

@ -170,9 +170,9 @@ class FakePeerConnectionBase : public PeerConnectionInternal {
return false;
}
bool SetConfiguration(
RTCError SetConfiguration(
const PeerConnectionInterface::RTCConfiguration& config) override {
return false;
return RTCError();
}
bool AddIceCandidate(const IceCandidateInterface* candidate) override {

View File

@ -592,7 +592,7 @@ static jboolean JNI_PeerConnection_SetConfiguration(
if (owned_pc->constraints()) {
CopyConstraintsIntoRtcConfiguration(owned_pc->constraints(), &rtc_config);
}
return owned_pc->pc()->SetConfiguration(rtc_config);
return owned_pc->pc()->SetConfiguration(rtc_config).ok();
}
static jboolean JNI_PeerConnection_AddIceCandidate(

View File

@ -378,7 +378,7 @@ void PeerConnectionDelegateAdapter::OnRemoveTrack(
}
CopyConstraintsIntoRtcConfiguration(_nativeConstraints.get(),
config.get());
return _peerConnection->SetConfiguration(*config);
return _peerConnection->SetConfiguration(*config).ok();
}
- (RTCConfiguration *)configuration {