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;