Reland "Implement RtpParameters.transaction_id for PC RtpSenderInterface"
This is a reland of 5faf36ef3c582350fba5ef97a3549e440d81a283
The issue in Chrome has been fixed and this should be safe to reland.
TBR=deadbeef
Original change's description:
> Implement RtpParameters.transaction_id for PC RtpSenderInterface
>
> The transaction_id field should be refreshed for every getParameters()
> call and checked at each setParameters() call.
> This also checks that getParameters() was ever called to return a proper
> error code.
>
> Bug: webrtc:7580
> Change-Id: I6c6fe289542e486fc422cdc61577982b0529d4c1
> Reviewed-on: https://webrtc-review.googlesource.com/70820
> Commit-Queue: Florent Castelli <orphis@webrtc.org>
> Reviewed-by: Sami Kalliomäki <sakal@webrtc.org>
> Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
> Reviewed-by: Kári Helgason <kthelgason@webrtc.org>
> Reviewed-by: Steve Anton <steveanton@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#23120}
Bug: webrtc:7580
Change-Id: Iabd41fb21afdf452c039d5513824ae334f8d1d3f
Reviewed-on: https://webrtc-review.googlesource.com/76980
Commit-Queue: Florent Castelli <orphis@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23247}diff --git a/api/rtpparameters.h b/api/rtpparameters.h
index 12e0419..e8ff116 100644
--- a/api/rtpparameters.h
+++ b/api/rtpparameters.h
@@ -546,7 +546,6 @@
// Used when calling getParameters/setParameters with a PeerConnection
// RtpSender, to ensure that outdated parameters are not unintentionally
// applied successfully.
- // TODO(deadbeef): Not implemented.
std::string transaction_id;
// Value to use for MID RTP header extension.
diff --git a/api/rtpsenderinterface.h b/api/rtpsenderinterface.h
index 01279a5..8c7e751 100644
--- a/api/rtpsenderinterface.h
+++ b/api/rtpsenderinterface.h
@@ -23,6 +23,7 @@
#include "api/proxy.h"
#include "api/rtcerror.h"
#include "api/rtpparameters.h"
+#include "rtc_base/deprecation.h"
#include "rtc_base/refcount.h"
#include "rtc_base/scoped_ref_ptr.h"
@@ -53,7 +54,13 @@
// tracks.
virtual std::vector<std::string> stream_ids() const = 0;
- virtual RtpParameters GetParameters() const = 0;
+ // TODO(orphis): Transitional implementation
+ // Remove the const implementation and make the non-const pure virtual once
+ // when external code depending on this has updated
+ virtual RtpParameters GetParameters() { return RtpParameters(); }
+ RTC_DEPRECATED virtual RtpParameters GetParameters() const {
+ return const_cast<RtpSenderInterface*>(this)->GetParameters();
+ }
// Note that only a subset of the parameters can currently be changed. See
// rtpparameters.h
virtual RTCError SetParameters(const RtpParameters& parameters) = 0;
@@ -76,7 +83,7 @@
PROXY_CONSTMETHOD0(cricket::MediaType, media_type)
PROXY_CONSTMETHOD0(std::string, id)
PROXY_CONSTMETHOD0(std::vector<std::string>, stream_ids)
- PROXY_CONSTMETHOD0(RtpParameters, GetParameters);
+ PROXY_METHOD0(RtpParameters, GetParameters);
PROXY_METHOD1(RTCError, SetParameters, const RtpParameters&)
PROXY_CONSTMETHOD0(rtc::scoped_refptr<DtmfSenderInterface>, GetDtmfSender);
END_PROXY_MAP()
diff --git a/api/test/mock_rtpsender.h b/api/test/mock_rtpsender.h
index dda5f45..22f391b 100644
--- a/api/test/mock_rtpsender.h
+++ b/api/test/mock_rtpsender.h
@@ -27,7 +27,7 @@
MOCK_CONST_METHOD0(media_type, cricket::MediaType());
MOCK_CONST_METHOD0(id, std::string());
MOCK_CONST_METHOD0(stream_ids, std::vector<std::string>());
- MOCK_CONST_METHOD0(GetParameters, RtpParameters());
+ MOCK_METHOD0(GetParameters, RtpParameters());
MOCK_METHOD1(SetParameters, RTCError(const RtpParameters&));
MOCK_CONST_METHOD0(GetDtmfSender, rtc::scoped_refptr<DtmfSenderInterface>());
};
diff --git a/pc/rtpsender.cc b/pc/rtpsender.cc
index 5cfbb13..3b0bbf8 100644
--- a/pc/rtpsender.cc
+++ b/pc/rtpsender.cc
@@ -187,12 +187,15 @@
return true;
}
-RtpParameters AudioRtpSender::GetParameters() const {
+RtpParameters AudioRtpSender::GetParameters() {
if (!media_channel_ || stopped_) {
return RtpParameters();
}
return worker_thread_->Invoke<RtpParameters>(RTC_FROM_HERE, [&] {
- return media_channel_->GetRtpSendParameters(ssrc_);
+ RtpParameters result = media_channel_->GetRtpSendParameters(ssrc_);
+ last_transaction_id_ = rtc::CreateRandomUuid();
+ result.transaction_id = last_transaction_id_.value();
+ return result;
});
}
@@ -201,8 +204,23 @@
if (!media_channel_ || stopped_) {
return RTCError(RTCErrorType::INVALID_STATE);
}
+ if (!last_transaction_id_) {
+ LOG_AND_RETURN_ERROR(
+ RTCErrorType::INVALID_STATE,
+ "Failed to set parameters since getParameters() has never been called"
+ " on this sender");
+ }
+ if (last_transaction_id_ != parameters.transaction_id) {
+ LOG_AND_RETURN_ERROR(
+ RTCErrorType::INVALID_MODIFICATION,
+ "Failed to set parameters since the transaction_id doesn't match"
+ " the last value returned from getParameters()");
+ }
+
return worker_thread_->Invoke<RTCError>(RTC_FROM_HERE, [&] {
- return media_channel_->SetRtpSendParameters(ssrc_, parameters);
+ RTCError result = media_channel_->SetRtpSendParameters(ssrc_, parameters);
+ last_transaction_id_.reset();
+ return result;
});
}
@@ -373,12 +391,15 @@
return true;
}
-RtpParameters VideoRtpSender::GetParameters() const {
+RtpParameters VideoRtpSender::GetParameters() {
if (!media_channel_ || stopped_) {
return RtpParameters();
}
return worker_thread_->Invoke<RtpParameters>(RTC_FROM_HERE, [&] {
- return media_channel_->GetRtpSendParameters(ssrc_);
+ RtpParameters result = media_channel_->GetRtpSendParameters(ssrc_);
+ last_transaction_id_ = rtc::CreateRandomUuid();
+ result.transaction_id = last_transaction_id_.value();
+ return result;
});
}
@@ -387,8 +408,23 @@
if (!media_channel_ || stopped_) {
return RTCError(RTCErrorType::INVALID_STATE);
}
+ if (!last_transaction_id_) {
+ LOG_AND_RETURN_ERROR(
+ RTCErrorType::INVALID_STATE,
+ "Failed to set parameters since getParameters() has never been called"
+ " on this sender");
+ }
+ if (last_transaction_id_ != parameters.transaction_id) {
+ LOG_AND_RETURN_ERROR(
+ RTCErrorType::INVALID_MODIFICATION,
+ "Failed to set parameters since the transaction_id doesn't match"
+ " the last value returned from getParameters()");
+ }
+
return worker_thread_->Invoke<RTCError>(RTC_FROM_HERE, [&] {
- return media_channel_->SetRtpSendParameters(ssrc_, parameters);
+ RTCError result = media_channel_->SetRtpSendParameters(ssrc_, parameters);
+ last_transaction_id_.reset();
+ return result;
});
}
diff --git a/pc/rtpsender.h b/pc/rtpsender.h
index ef41485..4077b9c 100644
--- a/pc/rtpsender.h
+++ b/pc/rtpsender.h
@@ -128,7 +128,7 @@
std::vector<std::string> stream_ids() const override { return stream_ids_; }
- RtpParameters GetParameters() const override;
+ RtpParameters GetParameters() override;
RTCError SetParameters(const RtpParameters& parameters) override;
rtc::scoped_refptr<DtmfSenderInterface> GetDtmfSender() const override;
@@ -172,6 +172,7 @@
StatsCollector* stats_;
rtc::scoped_refptr<AudioTrackInterface> track_;
rtc::scoped_refptr<DtmfSenderInterface> dtmf_sender_proxy_;
+ rtc::Optional<std::string> last_transaction_id_;
uint32_t ssrc_ = 0;
bool cached_track_enabled_ = false;
bool stopped_ = false;
@@ -216,7 +217,7 @@
std::vector<std::string> stream_ids() const override { return stream_ids_; }
- RtpParameters GetParameters() const override;
+ RtpParameters GetParameters() override;
RTCError SetParameters(const RtpParameters& parameters) override;
rtc::scoped_refptr<DtmfSenderInterface> GetDtmfSender() const override;
@@ -253,6 +254,7 @@
std::vector<std::string> stream_ids_;
cricket::VideoMediaChannel* media_channel_ = nullptr;
rtc::scoped_refptr<VideoTrackInterface> track_;
+ rtc::Optional<std::string> last_transaction_id_;
uint32_t ssrc_ = 0;
VideoTrackInterface::ContentHint cached_track_content_hint_ =
VideoTrackInterface::ContentHint::kNone;
diff --git a/pc/rtpsenderreceiver_unittest.cc b/pc/rtpsenderreceiver_unittest.cc
index d9b543b..b084b01 100644
--- a/pc/rtpsenderreceiver_unittest.cc
+++ b/pc/rtpsenderreceiver_unittest.cc
@@ -606,6 +606,65 @@
DestroyAudioRtpSender();
}
+TEST_F(RtpSenderReceiverTest,
+ AudioSenderMustCallGetParametersBeforeSetParameters) {
+ CreateAudioRtpSender();
+
+ RtpParameters params;
+ RTCError result = audio_rtp_sender_->SetParameters(params);
+ EXPECT_EQ(RTCErrorType::INVALID_STATE, result.type());
+
+ DestroyAudioRtpSender();
+}
+
+TEST_F(RtpSenderReceiverTest,
+ AudioSenderSetParametersInvalidatesTransactionId) {
+ CreateAudioRtpSender();
+
+ RtpParameters params = audio_rtp_sender_->GetParameters();
+ EXPECT_EQ(1u, params.encodings.size());
+ EXPECT_TRUE(audio_rtp_sender_->SetParameters(params).ok());
+ RTCError result = audio_rtp_sender_->SetParameters(params);
+ EXPECT_EQ(RTCErrorType::INVALID_STATE, result.type());
+
+ DestroyAudioRtpSender();
+}
+
+TEST_F(RtpSenderReceiverTest, AudioSenderDetectTransactionIdModification) {
+ CreateAudioRtpSender();
+
+ RtpParameters params = audio_rtp_sender_->GetParameters();
+ params.transaction_id = "";
+ RTCError result = audio_rtp_sender_->SetParameters(params);
+ EXPECT_EQ(RTCErrorType::INVALID_MODIFICATION, result.type());
+
+ DestroyAudioRtpSender();
+}
+
+TEST_F(RtpSenderReceiverTest, AudioSenderCheckTransactionIdRefresh) {
+ CreateAudioRtpSender();
+
+ RtpParameters params = audio_rtp_sender_->GetParameters();
+ EXPECT_NE(params.transaction_id.size(), 0);
+ auto saved_transaction_id = params.transaction_id;
+ params = audio_rtp_sender_->GetParameters();
+ EXPECT_NE(saved_transaction_id, params.transaction_id);
+
+ DestroyAudioRtpSender();
+}
+
+TEST_F(RtpSenderReceiverTest, AudioSenderSetParametersOldValueFail) {
+ CreateAudioRtpSender();
+
+ RtpParameters params = audio_rtp_sender_->GetParameters();
+ RtpParameters second_params = audio_rtp_sender_->GetParameters();
+
+ RTCError result = audio_rtp_sender_->SetParameters(params);
+ EXPECT_EQ(RTCErrorType::INVALID_MODIFICATION, result.type());
+
+ DestroyAudioRtpSender();
+}
+
TEST_F(RtpSenderReceiverTest, SetAudioMaxSendBitrate) {
CreateAudioRtpSender();
@@ -664,6 +723,65 @@
DestroyVideoRtpSender();
}
+TEST_F(RtpSenderReceiverTest,
+ VideoSenderMustCallGetParametersBeforeSetParameters) {
+ CreateVideoRtpSender();
+
+ RtpParameters params;
+ RTCError result = video_rtp_sender_->SetParameters(params);
+ EXPECT_EQ(RTCErrorType::INVALID_STATE, result.type());
+
+ DestroyVideoRtpSender();
+}
+
+TEST_F(RtpSenderReceiverTest,
+ VideoSenderSetParametersInvalidatesTransactionId) {
+ CreateVideoRtpSender();
+
+ RtpParameters params = video_rtp_sender_->GetParameters();
+ EXPECT_EQ(1u, params.encodings.size());
+ EXPECT_TRUE(video_rtp_sender_->SetParameters(params).ok());
+ RTCError result = video_rtp_sender_->SetParameters(params);
+ EXPECT_EQ(RTCErrorType::INVALID_STATE, result.type());
+
+ DestroyVideoRtpSender();
+}
+
+TEST_F(RtpSenderReceiverTest, VideoSenderDetectTransactionIdModification) {
+ CreateVideoRtpSender();
+
+ RtpParameters params = video_rtp_sender_->GetParameters();
+ params.transaction_id = "";
+ RTCError result = video_rtp_sender_->SetParameters(params);
+ EXPECT_EQ(RTCErrorType::INVALID_MODIFICATION, result.type());
+
+ DestroyVideoRtpSender();
+}
+
+TEST_F(RtpSenderReceiverTest, VideoSenderCheckTransactionIdRefresh) {
+ CreateVideoRtpSender();
+
+ RtpParameters params = video_rtp_sender_->GetParameters();
+ EXPECT_NE(params.transaction_id.size(), 0);
+ auto saved_transaction_id = params.transaction_id;
+ params = video_rtp_sender_->GetParameters();
+ EXPECT_NE(saved_transaction_id, params.transaction_id);
+
+ DestroyVideoRtpSender();
+}
+
+TEST_F(RtpSenderReceiverTest, VideoSenderSetParametersOldValueFail) {
+ CreateVideoRtpSender();
+
+ RtpParameters params = video_rtp_sender_->GetParameters();
+ RtpParameters second_params = video_rtp_sender_->GetParameters();
+
+ RTCError result = video_rtp_sender_->SetParameters(params);
+ EXPECT_EQ(RTCErrorType::INVALID_MODIFICATION, result.type());
+
+ DestroyVideoRtpSender();
+}
+
TEST_F(RtpSenderReceiverTest, SetVideoMaxSendBitrate) {
CreateVideoRtpSender();
diff --git a/pc/test/mock_rtpsenderinternal.h b/pc/test/mock_rtpsenderinternal.h
index 5988c7b..3ddffec 100644
--- a/pc/test/mock_rtpsenderinternal.h
+++ b/pc/test/mock_rtpsenderinternal.h
@@ -29,7 +29,7 @@
MOCK_CONST_METHOD0(media_type, cricket::MediaType());
MOCK_CONST_METHOD0(id, std::string());
MOCK_CONST_METHOD0(stream_ids, std::vector<std::string>());
- MOCK_CONST_METHOD0(GetParameters, RtpParameters());
+ MOCK_METHOD0(GetParameters, RtpParameters());
MOCK_METHOD1(SetParameters, RTCError(const RtpParameters&));
MOCK_CONST_METHOD0(GetDtmfSender, rtc::scoped_refptr<DtmfSenderInterface>());
diff --git a/sdk/android/api/org/webrtc/RtpParameters.java b/sdk/android/api/org/webrtc/RtpParameters.java
index 634f3b3..f2227ae 100644
--- a/sdk/android/api/org/webrtc/RtpParameters.java
+++ b/sdk/android/api/org/webrtc/RtpParameters.java
@@ -109,21 +109,24 @@
}
}
+ public final String transactionId;
+
public final List<Encoding> encodings;
// Codec parameters can't currently be changed between getParameters and
// setParameters. Though in the future it will be possible to reorder them or
// remove them.
public final List<Codec> codecs;
- public RtpParameters() {
- this.encodings = new ArrayList<>();
- this.codecs = new ArrayList<>();
+ @CalledByNative
+ RtpParameters(String transactionId, List<Encoding> encodings, List<Codec> codecs) {
+ this.transactionId = transactionId;
+ this.encodings = encodings;
+ this.codecs = codecs;
}
@CalledByNative
- RtpParameters(List<Encoding> encodings, List<Codec> codecs) {
- this.encodings = encodings;
- this.codecs = codecs;
+ String getTransactionId() {
+ return transactionId;
}
@CalledByNative
diff --git a/sdk/android/src/jni/pc/rtpparameters.cc b/sdk/android/src/jni/pc/rtpparameters.cc
index 6a312b4..a990934 100644
--- a/sdk/android/src/jni/pc/rtpparameters.cc
+++ b/sdk/android/src/jni/pc/rtpparameters.cc
@@ -59,6 +59,10 @@
const JavaRef<jobject>& j_parameters) {
RtpParameters parameters;
+ ScopedJavaLocalRef<jstring> j_transaction_id =
+ Java_RtpParameters_getTransactionId(jni, j_parameters);
+ parameters.transaction_id = JavaToNativeString(jni, j_transaction_id);
+
// Convert encodings.
ScopedJavaLocalRef<jobject> j_encodings =
Java_RtpParameters_getEncodings(jni, j_parameters);
@@ -90,7 +94,7 @@
JNIEnv* env,
const RtpParameters& parameters) {
return Java_RtpParameters_Constructor(
- env,
+ env, NativeToJavaString(env, parameters.transaction_id),
NativeToJavaList(env, parameters.encodings,
&NativeToJavaRtpEncodingParameter),
NativeToJavaList(env, parameters.codecs, &NativeToJavaRtpCodecParameter));
diff --git a/sdk/objc/Framework/Classes/PeerConnection/RTCRtpParameters.mm b/sdk/objc/Framework/Classes/PeerConnection/RTCRtpParameters.mm
index 5e79106..d18eba6 100644
--- a/sdk/objc/Framework/Classes/PeerConnection/RTCRtpParameters.mm
+++ b/sdk/objc/Framework/Classes/PeerConnection/RTCRtpParameters.mm
@@ -10,11 +10,13 @@
#import "RTCRtpParameters+Private.h"
+#import "NSString+StdString.h"
#import "RTCRtpCodecParameters+Private.h"
#import "RTCRtpEncodingParameters+Private.h"
@implementation RTCRtpParameters
+@synthesize transactionId = _transactionId;
@synthesize encodings = _encodings;
@synthesize codecs = _codecs;
@@ -25,6 +27,7 @@
- (instancetype)initWithNativeParameters:
(const webrtc::RtpParameters &)nativeParameters {
if (self = [self init]) {
+ _transactionId = [NSString stringForStdString:nativeParameters.transaction_id];
NSMutableArray *encodings = [[NSMutableArray alloc] init];
for (const auto &encoding : nativeParameters.encodings) {
[encodings addObject:[[RTCRtpEncodingParameters alloc]
@@ -43,7 +46,8 @@
}
- (webrtc::RtpParameters)nativeParameters {
- webrtc::RtpParameters parameters;
+ webrtc::RtpParameters parameters;
+ parameters.transaction_id = [NSString stdStringForString:_transactionId];
for (RTCRtpEncodingParameters *encoding in _encodings) {
parameters.encodings.push_back(encoding.nativeParameters);
}
diff --git a/sdk/objc/Framework/Headers/WebRTC/RTCRtpParameters.h b/sdk/objc/Framework/Headers/WebRTC/RTCRtpParameters.h
index bdebf84..b6e1f01 100644
--- a/sdk/objc/Framework/Headers/WebRTC/RTCRtpParameters.h
+++ b/sdk/objc/Framework/Headers/WebRTC/RTCRtpParameters.h
@@ -19,6 +19,9 @@
RTC_EXPORT
@interface RTCRtpParameters : NSObject
+/** A unique identifier for the last set of parameters applied. */
+@property(nonatomic, copy) NSString *transactionId;
+
/** The currently active encodings in the order of preference. */
@property(nonatomic, copy) NSArray<RTCRtpEncodingParameters *> *encodings;
