From cebf50ff75fc64f87f74f8ffe93c361d2e11b13b Mon Sep 17 00:00:00 2001 From: Florent Castelli Date: Thu, 3 May 2018 15:31:53 +0200 Subject: [PATCH] Reland "Implement RtpParameters.transaction_id for PC RtpSenderInterface" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 > Reviewed-by: Sami Kalliomäki > Reviewed-by: Taylor Brandstetter > Reviewed-by: Kári Helgason > Reviewed-by: Steve Anton > 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 Reviewed-by: Florent Castelli Cr-Commit-Position: refs/heads/master@{#23247} --- api/rtpparameters.h | 1 - api/rtpsenderinterface.h | 11 +- api/test/mock_rtpsender.h | 2 +- pc/rtpsender.cc | 48 ++++++- pc/rtpsender.h | 6 +- pc/rtpsenderreceiver_unittest.cc | 118 ++++++++++++++++++ pc/test/mock_rtpsenderinternal.h | 2 +- sdk/android/api/org/webrtc/RtpParameters.java | 15 ++- sdk/android/src/jni/pc/rtpparameters.cc | 6 +- .../PeerConnection/RTCRtpParameters.mm | 6 +- .../Headers/WebRTC/RTCRtpParameters.h | 3 + 11 files changed, 197 insertions(+), 21 deletions(-) diff --git a/api/rtpparameters.h b/api/rtpparameters.h index 12e041939f..e8ff11623c 100644 --- a/api/rtpparameters.h +++ b/api/rtpparameters.h @@ -546,7 +546,6 @@ struct RtpParameters { // 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 01279a55a0..8c7e751392 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 @@ class RtpSenderInterface : public rtc::RefCountInterface { // tracks. virtual std::vector 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(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 @@ BEGIN_SIGNALING_PROXY_MAP(RtpSender) PROXY_CONSTMETHOD0(cricket::MediaType, media_type) PROXY_CONSTMETHOD0(std::string, id) PROXY_CONSTMETHOD0(std::vector, stream_ids) - PROXY_CONSTMETHOD0(RtpParameters, GetParameters); + PROXY_METHOD0(RtpParameters, GetParameters); PROXY_METHOD1(RTCError, SetParameters, const RtpParameters&) PROXY_CONSTMETHOD0(rtc::scoped_refptr, GetDtmfSender); END_PROXY_MAP() diff --git a/api/test/mock_rtpsender.h b/api/test/mock_rtpsender.h index dda5f45675..22f391b86c 100644 --- a/api/test/mock_rtpsender.h +++ b/api/test/mock_rtpsender.h @@ -27,7 +27,7 @@ class MockRtpSender : public rtc::RefCountedObject { MOCK_CONST_METHOD0(media_type, cricket::MediaType()); MOCK_CONST_METHOD0(id, std::string()); MOCK_CONST_METHOD0(stream_ids, std::vector()); - MOCK_CONST_METHOD0(GetParameters, RtpParameters()); + MOCK_METHOD0(GetParameters, RtpParameters()); MOCK_METHOD1(SetParameters, RTCError(const RtpParameters&)); MOCK_CONST_METHOD0(GetDtmfSender, rtc::scoped_refptr()); }; diff --git a/pc/rtpsender.cc b/pc/rtpsender.cc index 5cfbb13637..3b0bbf886a 100644 --- a/pc/rtpsender.cc +++ b/pc/rtpsender.cc @@ -187,12 +187,15 @@ bool AudioRtpSender::SetTrack(MediaStreamTrackInterface* track) { return true; } -RtpParameters AudioRtpSender::GetParameters() const { +RtpParameters AudioRtpSender::GetParameters() { if (!media_channel_ || stopped_) { return RtpParameters(); } return worker_thread_->Invoke(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 @@ RTCError AudioRtpSender::SetParameters(const RtpParameters& parameters) { 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(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 @@ bool VideoRtpSender::SetTrack(MediaStreamTrackInterface* track) { return true; } -RtpParameters VideoRtpSender::GetParameters() const { +RtpParameters VideoRtpSender::GetParameters() { if (!media_channel_ || stopped_) { return RtpParameters(); } return worker_thread_->Invoke(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 @@ RTCError VideoRtpSender::SetParameters(const RtpParameters& parameters) { 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(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 ef41485898..4077b9ca1f 100644 --- a/pc/rtpsender.h +++ b/pc/rtpsender.h @@ -128,7 +128,7 @@ class AudioRtpSender : public DtmfProviderInterface, std::vector stream_ids() const override { return stream_ids_; } - RtpParameters GetParameters() const override; + RtpParameters GetParameters() override; RTCError SetParameters(const RtpParameters& parameters) override; rtc::scoped_refptr GetDtmfSender() const override; @@ -172,6 +172,7 @@ class AudioRtpSender : public DtmfProviderInterface, StatsCollector* stats_; rtc::scoped_refptr track_; rtc::scoped_refptr dtmf_sender_proxy_; + rtc::Optional last_transaction_id_; uint32_t ssrc_ = 0; bool cached_track_enabled_ = false; bool stopped_ = false; @@ -216,7 +217,7 @@ class VideoRtpSender : public ObserverInterface, std::vector stream_ids() const override { return stream_ids_; } - RtpParameters GetParameters() const override; + RtpParameters GetParameters() override; RTCError SetParameters(const RtpParameters& parameters) override; rtc::scoped_refptr GetDtmfSender() const override; @@ -253,6 +254,7 @@ class VideoRtpSender : public ObserverInterface, std::vector stream_ids_; cricket::VideoMediaChannel* media_channel_ = nullptr; rtc::scoped_refptr track_; + rtc::Optional 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 d9b543b8c2..b084b01e1a 100644 --- a/pc/rtpsenderreceiver_unittest.cc +++ b/pc/rtpsenderreceiver_unittest.cc @@ -606,6 +606,65 @@ TEST_F(RtpSenderReceiverTest, AudioSenderCanSetParameters) { 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 @@ TEST_F(RtpSenderReceiverTest, VideoSenderCanSetParameters) { 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 5988c7b3c1..3ddffec7a1 100644 --- a/pc/test/mock_rtpsenderinternal.h +++ b/pc/test/mock_rtpsenderinternal.h @@ -29,7 +29,7 @@ class MockRtpSenderInternal : public RtpSenderInternal { MOCK_CONST_METHOD0(media_type, cricket::MediaType()); MOCK_CONST_METHOD0(id, std::string()); MOCK_CONST_METHOD0(stream_ids, std::vector()); - MOCK_CONST_METHOD0(GetParameters, RtpParameters()); + MOCK_METHOD0(GetParameters, RtpParameters()); MOCK_METHOD1(SetParameters, RTCError(const RtpParameters&)); MOCK_CONST_METHOD0(GetDtmfSender, rtc::scoped_refptr()); diff --git a/sdk/android/api/org/webrtc/RtpParameters.java b/sdk/android/api/org/webrtc/RtpParameters.java index 634f3b3017..f2227ae53a 100644 --- a/sdk/android/api/org/webrtc/RtpParameters.java +++ b/sdk/android/api/org/webrtc/RtpParameters.java @@ -109,21 +109,24 @@ public class RtpParameters { } } + public final String transactionId; + public final List 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 codecs; - public RtpParameters() { - this.encodings = new ArrayList<>(); - this.codecs = new ArrayList<>(); + @CalledByNative + RtpParameters(String transactionId, List encodings, List codecs) { + this.transactionId = transactionId; + this.encodings = encodings; + this.codecs = codecs; } @CalledByNative - RtpParameters(List encodings, List 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 6a312b4865..a990934b01 100644 --- a/sdk/android/src/jni/pc/rtpparameters.cc +++ b/sdk/android/src/jni/pc/rtpparameters.cc @@ -59,6 +59,10 @@ RtpParameters JavaToNativeRtpParameters(JNIEnv* jni, const JavaRef& j_parameters) { RtpParameters parameters; + ScopedJavaLocalRef j_transaction_id = + Java_RtpParameters_getTransactionId(jni, j_parameters); + parameters.transaction_id = JavaToNativeString(jni, j_transaction_id); + // Convert encodings. ScopedJavaLocalRef j_encodings = Java_RtpParameters_getEncodings(jni, j_parameters); @@ -90,7 +94,7 @@ ScopedJavaLocalRef NativeToJavaRtpParameters( 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 5e791066df..d18eba6a3d 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 bdebf84884..b6e1f01a23 100644 --- a/sdk/objc/Framework/Headers/WebRTC/RTCRtpParameters.h +++ b/sdk/objc/Framework/Headers/WebRTC/RTCRtpParameters.h @@ -19,6 +19,9 @@ NS_ASSUME_NONNULL_BEGIN 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 *encodings;