Make WebRtcSessionDescriptionFactory depend on SdpOfferAnswerHandler
This factory is only used by SdpOfferAnswerHandler, so it should not
need to depend on PeerConnection.
Bug: webrtc:11995
Change-Id: Ib27d9d9fdf440be7db8890bf0e7520d0c67bde22
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/189780
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#32460}
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 9f3f6ba..8a72bcf 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -581,21 +581,10 @@
audio_options_.audio_jitter_buffer_enable_rtx_handling =
configuration.audio_jitter_buffer_enable_rtx_handling;
- // Whether the certificate generator/certificate is null or not determines
- // what PeerConnectionDescriptionFactory will do, so make sure that we give it
- // the right instructions by clearing the variables if needed.
- if (!dtls_enabled_) {
- dependencies.cert_generator.reset();
- certificate = nullptr;
- } else if (certificate) {
- // Favor generated certificate over the certificate generator.
- dependencies.cert_generator.reset();
- }
-
auto webrtc_session_desc_factory =
std::make_unique<WebRtcSessionDescriptionFactory>(
- signaling_thread(), channel_manager(), this, session_id(),
- std::move(dependencies.cert_generator), certificate,
+ signaling_thread(), channel_manager(), &sdp_handler_, session_id(),
+ dtls_enabled_, std::move(dependencies.cert_generator), certificate,
&ssrc_generator_);
webrtc_session_desc_factory->SignalCertificateReady.connect(
this, &PeerConnection::OnCertificateReady);
diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h
index da8b3a2..0880c1d 100644
--- a/pc/sdp_offer_answer.h
+++ b/pc/sdp_offer_answer.h
@@ -76,6 +76,7 @@
class RtcEventLog;
class RtpTransmissionManager;
class TransceiverList;
+class WebRtcSessionDescriptionFactory;
// SdpOfferAnswerHandler is a component
// of the PeerConnection object as defined
@@ -118,6 +119,8 @@
const SessionDescriptionInterface* pending_local_description() const;
const SessionDescriptionInterface* pending_remote_description() const;
+ JsepTransportController* transport_controller();
+
void RestartIce();
// JSEP01
@@ -528,7 +531,6 @@
cricket::ChannelManager* channel_manager() const;
TransceiverList* transceivers();
const TransceiverList* transceivers() const;
- JsepTransportController* transport_controller();
DataChannelController* data_channel_controller();
const DataChannelController* data_channel_controller() const;
cricket::PortAllocator* port_allocator();
diff --git a/pc/webrtc_session_description_factory.cc b/pc/webrtc_session_description_factory.cc
index aaef7fd..2409805 100644
--- a/pc/webrtc_session_description_factory.cc
+++ b/pc/webrtc_session_description_factory.cc
@@ -11,7 +11,7 @@
#include "pc/webrtc_session_description_factory.h"
#include <stddef.h>
-
+#include <algorithm>
#include <memory>
#include <string>
#include <utility>
@@ -22,6 +22,8 @@
#include "api/jsep.h"
#include "api/jsep_session_description.h"
#include "api/rtc_error.h"
+#include "pc/jsep_transport_controller.h"
+#include "pc/sdp_offer_answer.h"
#include "pc/session_description.h"
#include "rtc_base/checks.h"
#include "rtc_base/location.h"
@@ -125,8 +127,9 @@
WebRtcSessionDescriptionFactory::WebRtcSessionDescriptionFactory(
rtc::Thread* signaling_thread,
cricket::ChannelManager* channel_manager,
- PeerConnectionInternal* pc,
+ SdpOfferAnswerHandler* sdp_handler,
const std::string& session_id,
+ bool dtls_enabled,
std::unique_ptr<rtc::RTCCertificateGeneratorInterface> cert_generator,
const rtc::scoped_refptr<rtc::RTCCertificate>& certificate,
UniqueRandomIdGenerator* ssrc_generator)
@@ -139,20 +142,20 @@
// to just use a random number as session id and start version from
// |kInitSessionVersion|.
session_version_(kInitSessionVersion),
- cert_generator_(std::move(cert_generator)),
- pc_(pc),
+ cert_generator_(dtls_enabled ? std::move(cert_generator) : nullptr),
+ sdp_handler_(sdp_handler),
session_id_(session_id),
certificate_request_state_(CERTIFICATE_NOT_NEEDED) {
RTC_DCHECK(signaling_thread_);
- RTC_DCHECK(!(cert_generator_ && certificate));
- bool dtls_enabled = cert_generator_ || certificate;
- // SRTP-SDES is disabled if DTLS is on.
- SetSdesPolicy(dtls_enabled ? cricket::SEC_DISABLED : cricket::SEC_REQUIRED);
+
if (!dtls_enabled) {
+ SetSdesPolicy(cricket::SEC_REQUIRED);
RTC_LOG(LS_VERBOSE) << "DTLS-SRTP disabled.";
return;
}
+ // SRTP-SDES is disabled if DTLS is on.
+ SetSdesPolicy(cricket::SEC_DISABLED);
if (certificate) {
// Use |certificate|.
certificate_request_state_ = CERTIFICATE_WAITING;
@@ -252,13 +255,13 @@
PostCreateSessionDescriptionFailed(observer, error);
return;
}
- if (!pc_->remote_description()) {
+ if (!sdp_handler_->remote_description()) {
error += " can't be called before SetRemoteDescription.";
RTC_LOG(LS_ERROR) << error;
PostCreateSessionDescriptionFailed(observer, error);
return;
}
- if (pc_->remote_description()->GetType() != SdpType::kOffer) {
+ if (sdp_handler_->remote_description()->GetType() != SdpType::kOffer) {
error += " failed because remote_description is not an offer.";
RTC_LOG(LS_ERROR) << error;
PostCreateSessionDescriptionFailed(observer, error);
@@ -325,12 +328,12 @@
void WebRtcSessionDescriptionFactory::InternalCreateOffer(
CreateSessionDescriptionRequest request) {
- if (pc_->local_description()) {
+ if (sdp_handler_->local_description()) {
// If the needs-ice-restart flag is set as described by JSEP, we should
// generate an offer with a new ufrag/password to trigger an ICE restart.
for (cricket::MediaDescriptionOptions& options :
request.options.media_description_options) {
- if (pc_->NeedsIceRestart(options.mid)) {
+ if (sdp_handler_->transport_controller()->NeedsIceRestart(options.mid)) {
options.transport_options.ice_restart = true;
}
}
@@ -338,9 +341,10 @@
std::unique_ptr<cricket::SessionDescription> desc =
session_desc_factory_.CreateOffer(
- request.options, pc_->local_description()
- ? pc_->local_description()->description()
- : nullptr);
+ request.options,
+ sdp_handler_->local_description()
+ ? sdp_handler_->local_description()->description()
+ : nullptr);
if (!desc) {
PostCreateSessionDescriptionFailed(request.observer,
"Failed to initialize the offer.");
@@ -360,11 +364,11 @@
auto offer = std::make_unique<JsepSessionDescription>(
SdpType::kOffer, std::move(desc), session_id_,
rtc::ToString(session_version_++));
- if (pc_->local_description()) {
+ if (sdp_handler_->local_description()) {
for (const cricket::MediaDescriptionOptions& options :
request.options.media_description_options) {
if (!options.transport_options.ice_restart) {
- CopyCandidatesFromSessionDescription(pc_->local_description(),
+ CopyCandidatesFromSessionDescription(sdp_handler_->local_description(),
options.mid, offer.get());
}
}
@@ -374,31 +378,34 @@
void WebRtcSessionDescriptionFactory::InternalCreateAnswer(
CreateSessionDescriptionRequest request) {
- if (pc_->remote_description()) {
+ if (sdp_handler_->remote_description()) {
for (cricket::MediaDescriptionOptions& options :
request.options.media_description_options) {
// According to http://tools.ietf.org/html/rfc5245#section-9.2.1.1
// an answer should also contain new ICE ufrag and password if an offer
// has been received with new ufrag and password.
options.transport_options.ice_restart =
- pc_->IceRestartPending(options.mid);
- // We should pass the current SSL role to the transport description
+ sdp_handler_->IceRestartPending(options.mid);
+ // We should pass the current DTLS role to the transport description
// factory, if there is already an existing ongoing session.
- rtc::SSLRole ssl_role;
- if (pc_->GetSslRole(options.mid, &ssl_role)) {
+ absl::optional<rtc::SSLRole> dtls_role =
+ sdp_handler_->transport_controller()->GetDtlsRole(options.mid);
+ if (dtls_role) {
options.transport_options.prefer_passive_role =
- (rtc::SSL_SERVER == ssl_role);
+ (rtc::SSL_SERVER == *dtls_role);
}
}
}
std::unique_ptr<cricket::SessionDescription> desc =
session_desc_factory_.CreateAnswer(
- pc_->remote_description() ? pc_->remote_description()->description()
- : nullptr,
+ sdp_handler_->remote_description()
+ ? sdp_handler_->remote_description()->description()
+ : nullptr,
request.options,
- pc_->local_description() ? pc_->local_description()->description()
- : nullptr);
+ sdp_handler_->local_description()
+ ? sdp_handler_->local_description()->description()
+ : nullptr);
if (!desc) {
PostCreateSessionDescriptionFailed(request.observer,
"Failed to initialize the answer.");
@@ -416,13 +423,13 @@
auto answer = std::make_unique<JsepSessionDescription>(
SdpType::kAnswer, std::move(desc), session_id_,
rtc::ToString(session_version_++));
- if (pc_->local_description()) {
+ if (sdp_handler_->local_description()) {
// Include all local ICE candidates in the SessionDescription unless
// the remote peer has requested an ICE restart.
for (const cricket::MediaDescriptionOptions& options :
request.options.media_description_options) {
if (!options.transport_options.ice_restart) {
- CopyCandidatesFromSessionDescription(pc_->local_description(),
+ CopyCandidatesFromSessionDescription(sdp_handler_->local_description(),
options.mid, answer.get());
}
}
diff --git a/pc/webrtc_session_description_factory.h b/pc/webrtc_session_description_factory.h
index f70b847..21d2a13 100644
--- a/pc/webrtc_session_description_factory.h
+++ b/pc/webrtc_session_description_factory.h
@@ -12,7 +12,6 @@
#define PC_WEBRTC_SESSION_DESCRIPTION_FACTORY_H_
#include <stdint.h>
-
#include <memory>
#include <queue>
#include <string>
@@ -22,18 +21,25 @@
#include "api/scoped_refptr.h"
#include "p2p/base/transport_description.h"
#include "p2p/base/transport_description_factory.h"
+#include "pc/channel_manager.h"
#include "pc/media_session.h"
-#include "pc/peer_connection_internal.h"
+#include "pc/sdp_offer_answer.h"
#include "rtc_base/constructor_magic.h"
#include "rtc_base/message_handler.h"
#include "rtc_base/rtc_certificate.h"
#include "rtc_base/rtc_certificate_generator.h"
#include "rtc_base/third_party/sigslot/sigslot.h"
#include "rtc_base/thread.h"
+#include "rtc_base/thread_message.h"
#include "rtc_base/unique_id_generator.h"
namespace webrtc {
+// Forward declaration is necessary because there's a circular dependency
+// between this class and SdpOfferAnswerHandler.
+// TODO(https://bugs.webrtc.org/12060): Break the dependency.
+class SdpOfferAnswerHandler;
+
// DTLS certificate request callback class.
class WebRtcCertificateGeneratorCallback
: public rtc::RTCCertificateGeneratorCallback,
@@ -80,8 +86,9 @@
WebRtcSessionDescriptionFactory(
rtc::Thread* signaling_thread,
cricket::ChannelManager* channel_manager,
- PeerConnectionInternal* pc,
+ SdpOfferAnswerHandler* sdp_handler,
const std::string& session_id,
+ bool dtls_enabled,
std::unique_ptr<rtc::RTCCertificateGeneratorInterface> cert_generator,
const rtc::scoped_refptr<rtc::RTCCertificate>& certificate,
rtc::UniqueRandomIdGenerator* ssrc_generator);
@@ -151,9 +158,7 @@
cricket::MediaSessionDescriptionFactory session_desc_factory_;
uint64_t session_version_;
const std::unique_ptr<rtc::RTCCertificateGeneratorInterface> cert_generator_;
- // TODO(jiayl): remove the dependency on peer connection once bug 2264 is
- // fixed.
- PeerConnectionInternal* const pc_;
+ SdpOfferAnswerHandler* sdp_handler_;
const std::string session_id_;
CertificateRequestState certificate_request_state_;
