From 52426edef163fe8c7f82f92a3dde8b8807046c53 Mon Sep 17 00:00:00 2001 From: Benjamin Wright Date: Fri, 1 Mar 2019 11:01:59 -0800 Subject: [PATCH] Modify BufferedFrameDecryptor to perform fine grained key requests. The current Key Frame request system doesn't take into account failed decryptions and this can lead to WebRTC spamming new key frame requests when the issue is actually in the decryptor layer. To prevent this if frame decryption is required for the PeerConnection key frame requests will not be sent at 200ms intervals but will wait until the stream is decryptable before utilizing this logic. Bug: webrtc:10330 Change-Id: I188a21dfd142dec6175d9def95f39a2bc517017c Reviewed-on: https://webrtc-review.googlesource.com/c/123414 Commit-Queue: Benjamin Wright Reviewed-by: Stefan Holmer Reviewed-by: Philip Eliasson Cr-Commit-Position: refs/heads/master@{#26931} --- video/buffered_frame_decryptor.cc | 18 ++++++++++++++---- video/buffered_frame_decryptor.h | 16 ++++++++++++++++ video/buffered_frame_decryptor_unittest.cc | 18 +++++++++++++++++- video/rtp_video_stream_receiver.cc | 13 +++++++++++-- video/rtp_video_stream_receiver.h | 14 +++++++++++++- video/video_receive_stream.cc | 4 +++- 6 files changed, 74 insertions(+), 9 deletions(-) diff --git a/video/buffered_frame_decryptor.cc b/video/buffered_frame_decryptor.cc index 99dc74254b..71414af142 100644 --- a/video/buffered_frame_decryptor.cc +++ b/video/buffered_frame_decryptor.cc @@ -20,11 +20,13 @@ namespace webrtc { BufferedFrameDecryptor::BufferedFrameDecryptor( OnDecryptedFrameCallback* decrypted_frame_callback, + OnDecryptionStatusChangeCallback* decryption_status_change_callback, rtc::scoped_refptr frame_decryptor) : generic_descriptor_auth_experiment_( field_trial::IsEnabled("WebRTC-GenericDescriptorAuth")), frame_decryptor_(std::move(frame_decryptor)), - decrypted_frame_callback_(decrypted_frame_callback) {} + decrypted_frame_callback_(decrypted_frame_callback), + decryption_status_change_callback_(decryption_status_change_callback) {} BufferedFrameDecryptor::~BufferedFrameDecryptor() {} @@ -78,9 +80,17 @@ BufferedFrameDecryptor::FrameDecision BufferedFrameDecryptor::DecryptFrame( // Attempt to decrypt the video frame. size_t bytes_written = 0; - if (frame_decryptor_->Decrypt( - cricket::MEDIA_TYPE_VIDEO, /*csrcs=*/{}, additional_data, *frame, - inline_decrypted_bitstream, &bytes_written) != 0) { + const int status = frame_decryptor_->Decrypt( + cricket::MEDIA_TYPE_VIDEO, /*csrcs=*/{}, additional_data, *frame, + inline_decrypted_bitstream, &bytes_written); + + // Optionally call the callback if there was a change in status + if (status != last_status_) { + last_status_ = status; + decryption_status_change_callback_->OnDecryptionStatusChange(status); + } + + if (status != 0) { // Only stash frames if we have never decrypted a frame before. return first_frame_decrypted_ ? FrameDecision::kDrop : FrameDecision::kStash; diff --git a/video/buffered_frame_decryptor.h b/video/buffered_frame_decryptor.h index 82732a2ea8..dd82fb10fa 100644 --- a/video/buffered_frame_decryptor.h +++ b/video/buffered_frame_decryptor.h @@ -32,6 +32,19 @@ class OnDecryptedFrameCallback { std::unique_ptr frame) = 0; }; +// This callback is called each time there is a status change in the decryption +// stream. For example going from a none state to a first decryption or going +// frome a decryptable state to a non decryptable state. +class OnDecryptionStatusChangeCallback { + public: + virtual ~OnDecryptionStatusChangeCallback() = default; + // Called each time the decryption stream status changes. This call is + // blocking so the caller must relinquish the callback quickly. This status + // must match what is specified in the FrameDecryptorInterface file. Notably + // 0 must indicate success and any positive integer is a failure. + virtual void OnDecryptionStatusChange(int status) = 0; +}; + // The BufferedFrameDecryptor is responsible for deciding when to pass // decrypted received frames onto the OnDecryptedFrameCallback. Frames can be // delayed when frame encryption is enabled but the key hasn't arrived yet. In @@ -45,6 +58,7 @@ 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); ~BufferedFrameDecryptor(); // This object cannot be copied. @@ -71,8 +85,10 @@ class BufferedFrameDecryptor final { const bool generic_descriptor_auth_experiment_; bool first_frame_decrypted_ = false; + int last_status_ = -1; const 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 9230b47c88..0719caf142 100644 --- a/video/buffered_frame_decryptor_unittest.cc +++ b/video/buffered_frame_decryptor_unittest.cc @@ -60,6 +60,7 @@ class FakePacketBuffer : public video_coding::PacketBuffer { class BufferedFrameDecryptorTest : public ::testing::Test, public OnDecryptedFrameCallback, + public OnDecryptionStatusChangeCallback, public video_coding::OnAssembledFrameCallback { public: // Implements the OnDecryptedFrameCallbackInterface @@ -68,6 +69,10 @@ class BufferedFrameDecryptorTest decrypted_frame_call_count_++; } + void OnDecryptionStatusChange(int status) { + ++decryption_status_change_count_; + } + // Implements the OnAssembledFrameCallback interface. void OnAssembledFrame( std::unique_ptr frame) override {} @@ -98,10 +103,11 @@ class BufferedFrameDecryptorTest void SetUp() override { fake_packet_data_ = std::vector(100); decrypted_frame_call_count_ = 0; + decryption_status_change_count_ = 0; seq_num_ = 0; mock_frame_decryptor_ = new rtc::RefCountedObject(); buffered_frame_decryptor_ = absl::make_unique( - this, mock_frame_decryptor_.get()); + this, this, mock_frame_decryptor_.get()); } static const size_t kMaxStashedFrames; @@ -111,6 +117,7 @@ class BufferedFrameDecryptorTest rtc::scoped_refptr mock_frame_decryptor_; std::unique_ptr buffered_frame_decryptor_; size_t decrypted_frame_call_count_; + size_t decryption_status_change_count_ = 0; uint16_t seq_num_; }; @@ -124,6 +131,7 @@ TEST_F(BufferedFrameDecryptorTest, CallbackCalledOnSuccessfulDecryption) { .WillOnce(Return(0)); buffered_frame_decryptor_->ManageEncryptedFrame(CreateRtpFrameObject(true)); EXPECT_EQ(decrypted_frame_call_count_, static_cast(1)); + EXPECT_EQ(decryption_status_change_count_, static_cast(1)); } // An initial fail to decrypt should not trigger the callback. @@ -134,6 +142,7 @@ TEST_F(BufferedFrameDecryptorTest, CallbackNotCalledOnFailedDecryption) { .WillOnce(Return(0)); buffered_frame_decryptor_->ManageEncryptedFrame(CreateRtpFrameObject(true)); EXPECT_EQ(decrypted_frame_call_count_, static_cast(0)); + EXPECT_EQ(decryption_status_change_count_, static_cast(1)); } // Initial failures should be stored and retried after the first successful @@ -151,9 +160,11 @@ TEST_F(BufferedFrameDecryptorTest, DelayedCallbackOnBufferedFrames) { // The first decrypt will fail stashing the first frame. buffered_frame_decryptor_->ManageEncryptedFrame(CreateRtpFrameObject(true)); EXPECT_EQ(decrypted_frame_call_count_, static_cast(0)); + EXPECT_EQ(decryption_status_change_count_, static_cast(1)); // The second call will succeed playing back both frames. buffered_frame_decryptor_->ManageEncryptedFrame(CreateRtpFrameObject(false)); EXPECT_EQ(decrypted_frame_call_count_, static_cast(2)); + EXPECT_EQ(decryption_status_change_count_, static_cast(2)); } // Subsequent failure to decrypts after the first successful decryption should @@ -172,13 +183,16 @@ TEST_F(BufferedFrameDecryptorTest, FTDDiscardedAfterFirstSuccess) { // The first decrypt will fail stashing the first frame. buffered_frame_decryptor_->ManageEncryptedFrame(CreateRtpFrameObject(true)); EXPECT_EQ(decrypted_frame_call_count_, static_cast(0)); + EXPECT_EQ(decryption_status_change_count_, static_cast(1)); // The second call will succeed playing back both frames. buffered_frame_decryptor_->ManageEncryptedFrame(CreateRtpFrameObject(false)); EXPECT_EQ(decrypted_frame_call_count_, static_cast(2)); + EXPECT_EQ(decryption_status_change_count_, static_cast(2)); // A new failure call will not result in an additional decrypted frame // callback. buffered_frame_decryptor_->ManageEncryptedFrame(CreateRtpFrameObject(true)); EXPECT_EQ(decrypted_frame_call_count_, static_cast(2)); + EXPECT_EQ(decryption_status_change_count_, static_cast(3)); } // Validate that the maximum number of stashed frames cannot be exceeded even if @@ -195,12 +209,14 @@ TEST_F(BufferedFrameDecryptorTest, MaximumNumberOfFramesStored) { buffered_frame_decryptor_->ManageEncryptedFrame(CreateRtpFrameObject(true)); } EXPECT_EQ(decrypted_frame_call_count_, static_cast(0)); + EXPECT_EQ(decryption_status_change_count_, static_cast(1)); EXPECT_CALL(*mock_frame_decryptor_, Decrypt) .Times(kMaxStashedFrames + 1) .WillRepeatedly(Return(0)); buffered_frame_decryptor_->ManageEncryptedFrame(CreateRtpFrameObject(true)); EXPECT_EQ(decrypted_frame_call_count_, kMaxStashedFrames + 1); + EXPECT_EQ(decryption_status_change_count_, static_cast(2)); } } // namespace webrtc diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc index f40df3ecf6..5032f0e980 100644 --- a/video/rtp_video_stream_receiver.cc +++ b/video/rtp_video_stream_receiver.cc @@ -111,7 +111,8 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( packet_router)), complete_frame_callback_(complete_frame_callback), keyframe_request_sender_(keyframe_request_sender), - has_received_frame_(false) { + has_received_frame_(false), + frames_decryptable_(false) { constexpr bool remb_candidate = true; packet_router_->AddReceiveRtpModule(rtp_rtcp_.get(), remb_candidate); @@ -174,7 +175,7 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( if (frame_decryptor != nullptr || config_.crypto_options.sframe.require_frame_encryption) { buffered_frame_decryptor_ = - absl::make_unique(this, frame_decryptor); + absl::make_unique(this, this, frame_decryptor); } } @@ -398,6 +399,10 @@ void RtpVideoStreamReceiver::RequestPacketRetransmit( rtp_rtcp_->SendNack(sequence_numbers); } +bool RtpVideoStreamReceiver::IsDecryptable() const { + return frames_decryptable_.load(); +} + int32_t RtpVideoStreamReceiver::ResendPackets(const uint16_t* sequence_numbers, uint16_t length) { return rtp_rtcp_->SendNACK(sequence_numbers, length); @@ -449,6 +454,10 @@ void RtpVideoStreamReceiver::OnDecryptedFrame( reference_finder_->ManageFrame(std::move(frame)); } +void RtpVideoStreamReceiver::OnDecryptionStatusChange(int status) { + frames_decryptable_.store(status == 0); +} + 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 16ea612de4..9e50dd3b7b 100644 --- a/video/rtp_video_stream_receiver.h +++ b/video/rtp_video_stream_receiver.h @@ -11,6 +11,7 @@ #ifndef VIDEO_RTP_VIDEO_STREAM_RECEIVER_H_ #define VIDEO_RTP_VIDEO_STREAM_RECEIVER_H_ +#include #include #include #include @@ -62,7 +63,8 @@ class RtpVideoStreamReceiver : public LossNotificationSender, public VCMPacketRequestCallback, public video_coding::OnAssembledFrameCallback, public video_coding::OnCompleteFrameCallback, - public OnDecryptedFrameCallback { + public OnDecryptedFrameCallback, + public OnDecryptionStatusChangeCallback { public: RtpVideoStreamReceiver( Transport* transport, @@ -126,6 +128,12 @@ class RtpVideoStreamReceiver : public LossNotificationSender, bool IsUlpfecEnabled() const; bool IsRetransmissionsEnabled() const; + + // Returns true if a decryptor is attached and frames can be decrypted. + // Updated by OnDecryptionStatusChangeCallback. Note this refers to Frame + // Decryption not SRTP. + bool IsDecryptable() const; + // Don't use, still experimental. void RequestPacketRetransmit(const std::vector& sequence_numbers); @@ -145,6 +153,9 @@ class RtpVideoStreamReceiver : public LossNotificationSender, void OnDecryptedFrame( std::unique_ptr frame) override; + // Implements OnDecryptionStatusChangeCallback. + void OnDecryptionStatusChange(int status) override; + // Called by VideoReceiveStream when stats are updated. void UpdateRtt(int64_t max_rtt_ms); @@ -230,6 +241,7 @@ class RtpVideoStreamReceiver : public LossNotificationSender, // rtp_reference_finder if they are decryptable. std::unique_ptr buffered_frame_decryptor_ RTC_PT_GUARDED_BY(network_tc_); + std::atomic frames_decryptable_; absl::optional last_color_space_; }; diff --git a/video/video_receive_stream.cc b/video/video_receive_stream.cc index 868ae05e8c..9d84fd3147 100644 --- a/video/video_receive_stream.cc +++ b/video/video_receive_stream.cc @@ -620,7 +620,9 @@ bool VideoReceiveStream::Decode() { last_keyframe_packet_ms && now_ms - *last_keyframe_packet_ms < kMaxWaitForKeyFrameMs; - if (stream_is_active && !receiving_keyframe) { + if (stream_is_active && !receiving_keyframe && + (!config_.crypto_options.sframe.require_frame_encryption || + rtp_video_stream_receiver_.IsDecryptable())) { RTC_LOG(LS_WARNING) << "No decodable frame in " << wait_ms << " ms, requesting keyframe."; RequestKeyFrame();