From a55644813876d6e00fe4255c0e9d210d11a60caf Mon Sep 17 00:00:00 2001 From: Benjamin Wright Date: Wed, 3 Apr 2019 10:44:18 -0700 Subject: [PATCH] Don't recreate the VideoReceiveStream on SetFrameDecryptor in the MediaEngine. This change introduces new logic to allow the injection of the FrameDecryptor into an arbitrary already running VideoReceiveStream without resetting it. It does this by taking advantage of the BufferedFrameDecryptor which will forcefully be created regardless of whether a FrameDecryptor is passed in during construction of the VideoReceiver if the crypto_option.require_frame_encryption is true. By allowing the BufferedFrameDecryptor to swap out which FrameDecryptor it uses this allows the Receiver to switch decryptors without resetting the stream. This is intended to mostly be used when you set your FrameDecryptor at a point post creation for the first time. Bug: webrtc:10416 Change-Id: If656b2acc447e2e77537cfa394729e5c3a8b660a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/130361 Commit-Queue: Stefan Holmer Reviewed-by: Stefan Holmer Reviewed-by: Jonas Oreland Reviewed-by: Niels Moller Reviewed-by: Rasmus Brandt Reviewed-by: Philip Eliasson Cr-Commit-Position: refs/heads/master@{#27458} --- call/call.cc | 28 +--------------------- call/video_receive_stream.h | 5 ++++ media/engine/fake_webrtc_call.h | 3 +++ media/engine/webrtc_video_engine.cc | 11 +++------ video/buffered_frame_decryptor.cc | 16 ++++++++----- video/buffered_frame_decryptor.h | 12 +++++++--- video/buffered_frame_decryptor_unittest.cc | 27 +++++++++++++++++++-- video/rtp_video_stream_receiver.cc | 19 ++++++++++++--- video/rtp_video_stream_receiver.h | 5 ++++ video/video_receive_stream.cc | 5 ++++ video/video_receive_stream.h | 3 +++ 11 files changed, 85 insertions(+), 49 deletions(-) diff --git a/call/call.cc b/call/call.cc index e458c48391..1e8d3a9309 100644 --- a/call/call.cc +++ b/call/call.cc @@ -404,8 +404,6 @@ class Call final : public webrtc::Call, MediaTransportInterface* media_transport_ RTC_GUARDED_BY(&target_observer_crit_) = nullptr; - const bool field_trial_webrtc_video_buffer_packets_with_unknown_ssrc_; - RTC_DISALLOW_COPY_AND_ASSIGN(Call); }; } // namespace internal @@ -488,10 +486,7 @@ Call::Call(Clock* clock, receive_side_cc_(clock_, transport_send->packet_router()), receive_time_calculator_(ReceiveTimeCalculator::CreateFromFieldTrial()), video_send_delay_stats_(new SendDelayStats(clock_)), - start_ms_(clock_->TimeInMilliseconds()), - field_trial_webrtc_video_buffer_packets_with_unknown_ssrc_( - webrtc::field_trial::IsEnabled( - "WebRTC-Video-BufferPacketsWithUnknownSsrc")) { + start_ms_(clock_->TimeInMilliseconds()) { RTC_DCHECK(config.event_log != nullptr); transport_send_ = std::move(transport_send); transport_send_ptr_ = transport_send_.get(); @@ -1416,27 +1411,6 @@ PacketReceiver::DeliveryStatus Call::DeliverRtp(MediaType media_type, return DELIVERY_UNKNOWN_SSRC; } - if (field_trial_webrtc_video_buffer_packets_with_unknown_ssrc_) { - // Check if packet arrives for a stream that requires decryption - // but does not yet have a FrameDecryptor. - // In that case buffer the packet and replay it when the frame decryptor - // is set. - // TODO(bugs.webrtc.org/10416) : Remove this check once FrameDecryptor - // is created as part of creating receive stream. - const uint32_t ssrc = parsed_packet.Ssrc(); - auto vrs = std::find_if( - video_receive_streams_.begin(), video_receive_streams_.end(), - [&](const VideoReceiveStream* stream) { - return (stream->config().rtp.remote_ssrc == ssrc || - stream->config().rtp.rtx_ssrc == ssrc); - }); - if (vrs != video_receive_streams_.end() && - (*vrs)->config().crypto_options.sframe.require_frame_encryption && - (*vrs)->config().frame_decryptor == nullptr) { - return DELIVERY_UNKNOWN_SSRC; - } - } - parsed_packet.IdentifyExtensions(it->second.extensions); NotifyBweOfReceivedPacket(parsed_packet, media_type); diff --git a/call/video_receive_stream.h b/call/video_receive_stream.h index 305e17a715..affc256eb2 100644 --- a/call/video_receive_stream.h +++ b/call/video_receive_stream.h @@ -260,6 +260,11 @@ class VideoReceiveStream { // Returns current value of base minimum delay in milliseconds. virtual int GetBaseMinimumPlayoutDelayMs() const = 0; + // Allows a FrameDecryptor to be attached to a VideoReceiveStream after + // creation without resetting the decoder state. + virtual void SetFrameDecryptor( + rtc::scoped_refptr frame_decryptor) = 0; + protected: virtual ~VideoReceiveStream() {} }; diff --git a/media/engine/fake_webrtc_call.h b/media/engine/fake_webrtc_call.h index b20b7ed771..9839b2d1f3 100644 --- a/media/engine/fake_webrtc_call.h +++ b/media/engine/fake_webrtc_call.h @@ -226,6 +226,9 @@ class FakeVideoReceiveStream final : public webrtc::VideoReceiveStream { return base_mininum_playout_delay_ms_; } + void SetFrameDecryptor(rtc::scoped_refptr + frame_decryptor) override {} + private: // webrtc::VideoReceiveStream implementation. void Start() override; diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index ca357be7e2..52ffcca97b 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -2538,12 +2538,7 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::RecreateWebRtcVideoStream() { if (webrtc::field_trial::IsEnabled( "WebRTC-Video-BufferPacketsWithUnknownSsrc")) { - // TODO(bugs.webrtc.org/10416) : Remove this check and backfill - // when the stream is created (i.e remote check for frame_decryptor) - // once FrameDecryptor is created as part of creating receive stream. - if (config_.frame_decryptor) { - channel_->BackfillBufferedPackets(stream_params_.ssrcs); - } + channel_->BackfillBufferedPackets(stream_params_.ssrcs); } } @@ -2602,9 +2597,9 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::SetFrameDecryptor( config_.frame_decryptor = frame_decryptor; if (stream_) { RTC_LOG(LS_INFO) - << "RecreateWebRtcStream (recv) because of SetFrameDecryptor, " + << "Setting FrameDecryptor (recv) because of SetFrameDecryptor, " << "remote_ssrc=" << config_.rtp.remote_ssrc; - RecreateWebRtcVideoStream(); + stream_->SetFrameDecryptor(frame_decryptor); } } diff --git a/video/buffered_frame_decryptor.cc b/video/buffered_frame_decryptor.cc index 71414af142..9f8e9df22f 100644 --- a/video/buffered_frame_decryptor.cc +++ b/video/buffered_frame_decryptor.cc @@ -20,21 +20,25 @@ namespace webrtc { BufferedFrameDecryptor::BufferedFrameDecryptor( OnDecryptedFrameCallback* decrypted_frame_callback, - OnDecryptionStatusChangeCallback* decryption_status_change_callback, - rtc::scoped_refptr frame_decryptor) + OnDecryptionStatusChangeCallback* decryption_status_change_callback) : generic_descriptor_auth_experiment_( field_trial::IsEnabled("WebRTC-GenericDescriptorAuth")), - frame_decryptor_(std::move(frame_decryptor)), decrypted_frame_callback_(decrypted_frame_callback), decryption_status_change_callback_(decryption_status_change_callback) {} BufferedFrameDecryptor::~BufferedFrameDecryptor() {} +void BufferedFrameDecryptor::SetFrameDecryptor( + rtc::scoped_refptr frame_decryptor) { + frame_decryptor_ = std::move(frame_decryptor); +} + void BufferedFrameDecryptor::ManageEncryptedFrame( std::unique_ptr encrypted_frame) { switch (DecryptFrame(encrypted_frame.get())) { case FrameDecision::kStash: if (stashed_frames_.size() >= kMaxStashedFrames) { + RTC_LOG(LS_WARNING) << "Encrypted frame stash full poping oldest item."; stashed_frames_.pop_front(); } stashed_frames_.push_back(std::move(encrypted_frame)); @@ -52,9 +56,9 @@ BufferedFrameDecryptor::FrameDecision BufferedFrameDecryptor::DecryptFrame( video_coding::RtpFrameObject* frame) { // Optionally attempt to decrypt the raw video frame if it was provided. if (frame_decryptor_ == nullptr) { - RTC_LOG(LS_WARNING) << "Frame decryption required but not attached to this " - "stream. Dropping frame."; - return FrameDecision::kDrop; + RTC_LOG(LS_INFO) << "Frame decryption required but not attached to this " + "stream. Stashing frame."; + return FrameDecision::kStash; } // When using encryption we expect the frame to have the generic descriptor. absl::optional descriptor = diff --git a/video/buffered_frame_decryptor.h b/video/buffered_frame_decryptor.h index dd82fb10fa..06992bbfb5 100644 --- a/video/buffered_frame_decryptor.h +++ b/video/buffered_frame_decryptor.h @@ -58,12 +58,18 @@ class BufferedFrameDecryptor final { // Constructs a new BufferedFrameDecryptor that can hold explicit BufferedFrameDecryptor( OnDecryptedFrameCallback* decrypted_frame_callback, - OnDecryptionStatusChangeCallback* decryption_status_change_callback, - rtc::scoped_refptr frame_decryptor); + OnDecryptionStatusChangeCallback* decryption_status_change_callback); ~BufferedFrameDecryptor(); // This object cannot be copied. BufferedFrameDecryptor(const BufferedFrameDecryptor&) = delete; BufferedFrameDecryptor& operator=(const BufferedFrameDecryptor&) = delete; + + // Sets a new frame decryptor as the decryptor for the buffered frame + // decryptor. This allows the decryptor to be switched out without resetting + // the video stream. + void SetFrameDecryptor( + rtc::scoped_refptr frame_decryptor); + // Determines whether the frame should be stashed, dropped or handed off to // the OnDecryptedFrameCallback. void ManageEncryptedFrame( @@ -86,7 +92,7 @@ class BufferedFrameDecryptor final { const bool generic_descriptor_auth_experiment_; bool first_frame_decrypted_ = false; int last_status_ = -1; - const rtc::scoped_refptr frame_decryptor_; + rtc::scoped_refptr frame_decryptor_; OnDecryptedFrameCallback* const decrypted_frame_callback_; OnDecryptionStatusChangeCallback* const decryption_status_change_callback_; std::deque> stashed_frames_; diff --git a/video/buffered_frame_decryptor_unittest.cc b/video/buffered_frame_decryptor_unittest.cc index 29ed089d23..7926f2421e 100644 --- a/video/buffered_frame_decryptor_unittest.cc +++ b/video/buffered_frame_decryptor_unittest.cc @@ -107,8 +107,9 @@ class BufferedFrameDecryptorTest decryption_status_change_count_ = 0; seq_num_ = 0; mock_frame_decryptor_ = new rtc::RefCountedObject(); - buffered_frame_decryptor_ = absl::make_unique( - this, this, mock_frame_decryptor_.get()); + buffered_frame_decryptor_ = + absl::make_unique(this, this); + buffered_frame_decryptor_->SetFrameDecryptor(mock_frame_decryptor_.get()); } static const size_t kMaxStashedFrames; @@ -220,4 +221,26 @@ TEST_F(BufferedFrameDecryptorTest, MaximumNumberOfFramesStored) { EXPECT_EQ(decryption_status_change_count_, static_cast(2)); } +// Verifies if a BufferedFrameDecryptor is attached but has no FrameDecryptor +// attached it will still store frames up to the frame max. +TEST_F(BufferedFrameDecryptorTest, FramesStoredIfDecryptorNull) { + buffered_frame_decryptor_->SetFrameDecryptor(nullptr); + for (size_t i = 0; i < (2 * kMaxStashedFrames); ++i) { + buffered_frame_decryptor_->ManageEncryptedFrame(CreateRtpFrameObject(true)); + } + + EXPECT_CALL(*mock_frame_decryptor_, Decrypt) + .Times(kMaxStashedFrames + 1) + .WillRepeatedly(Return(0)); + EXPECT_CALL(*mock_frame_decryptor_, GetMaxPlaintextByteSize) + .WillRepeatedly(Return(0)); + + // Attach the frame decryptor at a later point after frames have arrived. + buffered_frame_decryptor_->SetFrameDecryptor(mock_frame_decryptor_.get()); + + // Next frame should trigger kMaxStashedFrame decryptions. + buffered_frame_decryptor_->ManageEncryptedFrame(CreateRtpFrameObject(true)); + EXPECT_EQ(decrypted_frame_call_count_, kMaxStashedFrames + 1); +} + } // namespace webrtc diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc index a24b68d132..0a63c8761e 100644 --- a/video/rtp_video_stream_receiver.cc +++ b/video/rtp_video_stream_receiver.cc @@ -175,11 +175,14 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( clock_, kPacketBufferStartSize, packet_buffer_max_size, this); reference_finder_ = absl::make_unique(this); + // Only construct the encrypted receiver if frame encryption is enabled. - if (frame_decryptor != nullptr || - config_.crypto_options.sframe.require_frame_encryption) { + if (config_.crypto_options.sframe.require_frame_encryption) { buffered_frame_decryptor_ = - absl::make_unique(this, this, frame_decryptor); + absl::make_unique(this, this); + if (frame_decryptor != nullptr) { + buffered_frame_decryptor_->SetFrameDecryptor(std::move(frame_decryptor)); + } } } @@ -452,6 +455,16 @@ void RtpVideoStreamReceiver::OnDecryptionStatusChange(int status) { frames_decryptable_.store(status == 0); } +void RtpVideoStreamReceiver::SetFrameDecryptor( + rtc::scoped_refptr frame_decryptor) { + RTC_DCHECK_RUN_ON(&network_tc_); + if (buffered_frame_decryptor_ == nullptr) { + buffered_frame_decryptor_ = + absl::make_unique(this, this); + } + buffered_frame_decryptor_->SetFrameDecryptor(std::move(frame_decryptor)); +} + void RtpVideoStreamReceiver::UpdateRtt(int64_t max_rtt_ms) { if (nack_module_) nack_module_->UpdateRtt(max_rtt_ms); diff --git a/video/rtp_video_stream_receiver.h b/video/rtp_video_stream_receiver.h index 3391cf351e..1bc5d8a8b3 100644 --- a/video/rtp_video_stream_receiver.h +++ b/video/rtp_video_stream_receiver.h @@ -155,6 +155,11 @@ class RtpVideoStreamReceiver : public LossNotificationSender, // Implements OnDecryptionStatusChangeCallback. void OnDecryptionStatusChange(int status) override; + // Optionally set a frame decryptor after a stream has started. This will not + // reset the decoder state. + void SetFrameDecryptor( + rtc::scoped_refptr frame_decryptor); + // Called by VideoReceiveStream when stats are updated. void UpdateRtt(int64_t max_rtt_ms); diff --git a/video/video_receive_stream.cc b/video/video_receive_stream.cc index 0515015436..a03b24f3c5 100644 --- a/video/video_receive_stream.cc +++ b/video/video_receive_stream.cc @@ -479,6 +479,11 @@ void VideoReceiveStream::OnFrame(const VideoFrame& video_frame) { stats_proxy_.OnRenderedFrame(video_frame); } +void VideoReceiveStream::SetFrameDecryptor( + rtc::scoped_refptr frame_decryptor) { + rtp_video_stream_receiver_.SetFrameDecryptor(std::move(frame_decryptor)); +} + void VideoReceiveStream::SendNack( const std::vector& sequence_numbers) { rtp_video_stream_receiver_.RequestPacketRetransmit(sequence_numbers); diff --git a/video/video_receive_stream.h b/video/video_receive_stream.h index 0397fede48..75f27b3ddd 100644 --- a/video/video_receive_stream.h +++ b/video/video_receive_stream.h @@ -94,6 +94,9 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream, bool SetBaseMinimumPlayoutDelayMs(int delay_ms) override; int GetBaseMinimumPlayoutDelayMs() const override; + void SetFrameDecryptor( + rtc::scoped_refptr frame_decryptor) override; + // Implements rtc::VideoSinkInterface. void OnFrame(const VideoFrame& video_frame) override;