From 2af5dcbe9ec322cb5f5288ed1c31681a2febdc4f Mon Sep 17 00:00:00 2001 From: Benjamin Wright Date: Tue, 9 Apr 2019 20:08:41 +0000 Subject: [PATCH] Reland "Refactor FrameDecryptorInterface::Decrypt to use new API." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 7dd83e2bf73a7f1746c5ee976939bf52e19fa8be. Reason for revert: This wasn't the cause of the break. Original change's description: > Revert "Refactor FrameDecryptorInterface::Decrypt to use new API." > > This reverts commit 642aa81f7d5cc55d5b99e2abc51327eed9d40195. > > Reason for revert: Speculative revert. The chromium roll is failing: > https://ci.chromium.org/p/chromium/builders/try/linux-rel/64388 > But I can't figure out exactly what is failing, this looks suspecious. > > Original change's description: > > Refactor FrameDecryptorInterface::Decrypt to use new API. > > > > This change refactors the FrameDecryptorInterface to use the new API. The new > > API surface simply moves bytes_written to the return type and implements a > > simple Status type. > > > > Bug: webrtc:10512 > > Change-Id: I622c5d344d58e618853c94c2f691cf7c8fb73a36 > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/131460 > > Reviewed-by: Steve Anton > > Reviewed-by: Fredrik Solenberg > > Reviewed-by: Rasmus Brandt > > Reviewed-by: Stefan Holmer > > Commit-Queue: Benjamin Wright > > Cr-Commit-Position: refs/heads/master@{#27497} > > TBR=brandtr@webrtc.org,steveanton@webrtc.org,solenberg@webrtc.org,ossu@webrtc.org,stefan@webrtc.org,benwright@webrtc.org > > Change-Id: Ia9ec70263762c34671af13f0d519e636eb8473cd > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: webrtc:10512 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/132013 > Reviewed-by: Henrik Boström > Commit-Queue: Henrik Boström > Cr-Commit-Position: refs/heads/master@{#27510} TBR=brandtr@webrtc.org,steveanton@webrtc.org,solenberg@webrtc.org,hbos@webrtc.org,ossu@webrtc.org,stefan@webrtc.org,benwright@webrtc.org Change-Id: I8e4b7965cf1d1a1554c3b46e6245f5ad0d2dcbb4 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:10512 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/131982 Reviewed-by: Benjamin Wright Commit-Queue: Benjamin Wright Cr-Commit-Position: refs/heads/master@{#27529} --- api/crypto/frame_decryptor_interface.h | 33 +++++------------- api/test/fake_frame_decryptor.cc | 19 +++++----- api/test/fake_frame_decryptor.h | 11 +++--- api/test/mock_frame_decryptor.h | 13 ++++--- audio/channel_receive.cc | 34 +++++++++--------- video/buffered_frame_decryptor.cc | 22 ++++++------ video/buffered_frame_decryptor.h | 6 ++-- video/buffered_frame_decryptor_unittest.cc | 40 +++++++++++++++------- video/rtp_video_stream_receiver.cc | 7 ++-- video/rtp_video_stream_receiver.h | 3 +- 10 files changed, 93 insertions(+), 95 deletions(-) diff --git a/api/crypto/frame_decryptor_interface.h b/api/crypto/frame_decryptor_interface.h index 69f137504c..ec900ab80a 100644 --- a/api/crypto/frame_decryptor_interface.h +++ b/api/crypto/frame_decryptor_interface.h @@ -34,8 +34,9 @@ class FrameDecryptorInterface : public rtc::RefCountInterface { // returned when attempting to decrypt a frame. kRecoverable indicates that // there was an error with the given frame and so it should not be passed to // the decoder, however it hints that the receive stream is still decryptable - // which is important for determining when to send key frame requests. - enum class Status { kOk, kRecoverable, kFailedToDecrypt }; + // which is important for determining when to send key frame requests + // kUnknown should never be returned by the implementor. + enum class Status { kOk, kRecoverable, kFailedToDecrypt, kUnknown }; struct Result { Result(Status status, size_t bytes_written) @@ -54,33 +55,15 @@ class FrameDecryptorInterface : public rtc::RefCountInterface { // that the frames are in order if SRTP is enabled. The stream is not provided // here and it is up to the implementor to transport this information to the // receiver if they care about it. You must set bytes_written to how many - // bytes you wrote to in the frame buffer. 0 must be returned if successful - // all other numbers can be selected by the implementer to represent error - // codes. - // TODO(bugs.webrtc.org/10512) - Remove this after implementation rewrite. - virtual int Decrypt(cricket::MediaType media_type, - const std::vector& csrcs, - rtc::ArrayView additional_data, - rtc::ArrayView encrypted_frame, - rtc::ArrayView frame, - size_t* bytes_written) { - bytes_written = 0; - return 1; - } - - // TODO(bugs.webrtc.org/10512) - Remove the other decrypt function and turn - // this to a pure virtual. + // bytes you wrote to in the frame buffer. kOk must be returned if successful, + // kRecoverable should be returned if the failure was due to something other + // than a decryption failure. kFailedToDecrypt should be returned in all other + // cases. virtual Result Decrypt(cricket::MediaType media_type, const std::vector& csrcs, rtc::ArrayView additional_data, rtc::ArrayView encrypted_frame, - rtc::ArrayView frame) { - size_t bytes_written = 0; - const int status = Decrypt(media_type, csrcs, additional_data, - encrypted_frame, frame, &bytes_written); - return Result(status == 0 ? Status::kOk : Status::kFailedToDecrypt, - bytes_written); - } + rtc::ArrayView frame) = 0; // Returns the total required length in bytes for the output of the // decryption. This can be larger than the actual number of bytes you need but diff --git a/api/test/fake_frame_decryptor.cc b/api/test/fake_frame_decryptor.cc index b77017fdb4..4af42a6b82 100644 --- a/api/test/fake_frame_decryptor.cc +++ b/api/test/fake_frame_decryptor.cc @@ -18,14 +18,14 @@ FakeFrameDecryptor::FakeFrameDecryptor(uint8_t fake_key, uint8_t expected_postfix_byte) : fake_key_(fake_key), expected_postfix_byte_(expected_postfix_byte) {} -int FakeFrameDecryptor::Decrypt(cricket::MediaType media_type, - const std::vector& csrcs, - rtc::ArrayView additional_data, - rtc::ArrayView encrypted_frame, - rtc::ArrayView frame, - size_t* bytes_written) { +FakeFrameDecryptor::Result FakeFrameDecryptor::Decrypt( + cricket::MediaType media_type, + const std::vector& csrcs, + rtc::ArrayView additional_data, + rtc::ArrayView encrypted_frame, + rtc::ArrayView frame) { if (fail_decryption_) { - return static_cast(FakeDecryptStatus::FORCED_FAILURE); + return Result(Status::kFailedToDecrypt, 0); } RTC_CHECK_EQ(frame.size() + 1, encrypted_frame.size()); @@ -34,11 +34,10 @@ int FakeFrameDecryptor::Decrypt(cricket::MediaType media_type, } if (encrypted_frame[frame.size()] != expected_postfix_byte_) { - return static_cast(FakeDecryptStatus::INVALID_POSTFIX); + return Result(Status::kFailedToDecrypt, 0); } - *bytes_written = frame.size(); - return static_cast(FakeDecryptStatus::OK); + return Result(Status::kOk, frame.size()); } size_t FakeFrameDecryptor::GetMaxPlaintextByteSize( diff --git a/api/test/fake_frame_decryptor.h b/api/test/fake_frame_decryptor.h index cb370b905a..05813dbbd0 100644 --- a/api/test/fake_frame_decryptor.h +++ b/api/test/fake_frame_decryptor.h @@ -35,12 +35,11 @@ class FakeFrameDecryptor final uint8_t expected_postfix_byte = 255); // Fake decryption that just xors the payload with the 1 byte key and checks // the postfix byte. This will always fail if fail_decryption_ is set to true. - int Decrypt(cricket::MediaType media_type, - const std::vector& csrcs, - rtc::ArrayView additional_data, - rtc::ArrayView encrypted_frame, - rtc::ArrayView frame, - size_t* bytes_written) override; + Result Decrypt(cricket::MediaType media_type, + const std::vector& csrcs, + rtc::ArrayView additional_data, + rtc::ArrayView encrypted_frame, + rtc::ArrayView frame) override; // Always returns 1 less than the size of the encrypted frame. size_t GetMaxPlaintextByteSize(cricket::MediaType media_type, size_t encrypted_frame_size) override; diff --git a/api/test/mock_frame_decryptor.h b/api/test/mock_frame_decryptor.h index feac9b3809..77aa4f9147 100644 --- a/api/test/mock_frame_decryptor.h +++ b/api/test/mock_frame_decryptor.h @@ -23,13 +23,12 @@ class MockFrameDecryptor : public FrameDecryptorInterface { MockFrameDecryptor(); ~MockFrameDecryptor() override; - MOCK_METHOD6(Decrypt, - int(cricket::MediaType, - const std::vector&, - rtc::ArrayView, - rtc::ArrayView, - rtc::ArrayView, - size_t*)); + MOCK_METHOD5(Decrypt, + Result(cricket::MediaType, + const std::vector&, + rtc::ArrayView, + rtc::ArrayView, + rtc::ArrayView)); MOCK_METHOD2(GetMaxPlaintextByteSize, size_t(cricket::MediaType, size_t encrypted_frame_size)); diff --git a/audio/channel_receive.cc b/audio/channel_receive.cc index 088aa57a07..4f00d9fdf2 100644 --- a/audio/channel_receive.cc +++ b/audio/channel_receive.cc @@ -103,8 +103,8 @@ class ChannelReceive : public ChannelReceiveInterface, void StopPlayout() override; // Codecs - absl::optional> - GetReceiveCodec() const override; + absl::optional> GetReceiveCodec() + const override; void ReceivedRTCPPacket(const uint8_t* data, size_t length) override; @@ -627,28 +627,26 @@ bool ChannelReceive::ReceivePacket(const uint8_t* packet, // Keep this buffer around for the lifetime of the OnReceivedPayloadData call. rtc::Buffer decrypted_audio_payload; if (frame_decryptor_ != nullptr) { - size_t max_plaintext_size = frame_decryptor_->GetMaxPlaintextByteSize( + const size_t max_plaintext_size = frame_decryptor_->GetMaxPlaintextByteSize( cricket::MEDIA_TYPE_AUDIO, payload_length); decrypted_audio_payload.SetSize(max_plaintext_size); - size_t bytes_written = 0; - std::vector csrcs(header.arrOfCSRCs, - header.arrOfCSRCs + header.numCSRCs); - int decrypt_status = frame_decryptor_->Decrypt( - cricket::MEDIA_TYPE_AUDIO, csrcs, - /*additional_data=*/nullptr, - rtc::ArrayView(payload, payload_data_length), - decrypted_audio_payload, &bytes_written); + const std::vector csrcs(header.arrOfCSRCs, + header.arrOfCSRCs + header.numCSRCs); + const FrameDecryptorInterface::Result decrypt_result = + frame_decryptor_->Decrypt( + cricket::MEDIA_TYPE_AUDIO, csrcs, + /*additional_data=*/nullptr, + rtc::ArrayView(payload, payload_data_length), + decrypted_audio_payload); - // In this case just interpret the failure as a silent frame. - if (decrypt_status != 0) { - bytes_written = 0; + if (decrypt_result.IsOk()) { + decrypted_audio_payload.SetSize(decrypt_result.bytes_written); + } else { + // Interpret failures as a silent frame. + decrypted_audio_payload.SetSize(0); } - // Resize the decrypted audio payload to the number of bytes actually - // written. - decrypted_audio_payload.SetSize(bytes_written); - // Update the final payload. payload = decrypted_audio_payload.data(); payload_data_length = decrypted_audio_payload.size(); } else if (crypto_options_.sframe.require_frame_encryption) { diff --git a/video/buffered_frame_decryptor.cc b/video/buffered_frame_decryptor.cc index 2d7ec25098..41eddea17e 100644 --- a/video/buffered_frame_decryptor.cc +++ b/video/buffered_frame_decryptor.cc @@ -83,25 +83,25 @@ BufferedFrameDecryptor::FrameDecision BufferedFrameDecryptor::DecryptFrame( } // Attempt to decrypt the video frame. - size_t bytes_written = 0; - const int status = frame_decryptor_->Decrypt( - cricket::MEDIA_TYPE_VIDEO, /*csrcs=*/{}, additional_data, *frame, - inline_decrypted_bitstream, &bytes_written); - + const FrameDecryptorInterface::Result decrypt_result = + frame_decryptor_->Decrypt(cricket::MEDIA_TYPE_VIDEO, /*csrcs=*/{}, + additional_data, *frame, + inline_decrypted_bitstream); // 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 (decrypt_result.status != last_status_) { + last_status_ = decrypt_result.status; + decryption_status_change_callback_->OnDecryptionStatusChange( + decrypt_result.status); } - if (status != 0) { + if (!decrypt_result.IsOk()) { // Only stash frames if we have never decrypted a frame before. return first_frame_decrypted_ ? FrameDecision::kDrop : FrameDecision::kStash; } - RTC_CHECK_LE(bytes_written, max_plaintext_byte_size); + RTC_CHECK_LE(decrypt_result.bytes_written, max_plaintext_byte_size); // Update the frame to contain just the written bytes. - frame->set_size(bytes_written); + frame->set_size(decrypt_result.bytes_written); // Indicate that all future fail to decrypt frames should be dropped. if (!first_frame_decrypted_) { diff --git a/video/buffered_frame_decryptor.h b/video/buffered_frame_decryptor.h index 06992bbfb5..49ab9a7bd9 100644 --- a/video/buffered_frame_decryptor.h +++ b/video/buffered_frame_decryptor.h @@ -42,7 +42,8 @@ class OnDecryptionStatusChangeCallback { // 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; + virtual void OnDecryptionStatusChange( + FrameDecryptorInterface::Status status) = 0; }; // The BufferedFrameDecryptor is responsible for deciding when to pass @@ -91,7 +92,8 @@ class BufferedFrameDecryptor final { const bool generic_descriptor_auth_experiment_; bool first_frame_decrypted_ = false; - int last_status_ = -1; + FrameDecryptorInterface::Status last_status_ = + FrameDecryptorInterface::Status::kUnknown; rtc::scoped_refptr frame_decryptor_; OnDecryptedFrameCallback* const decrypted_frame_callback_; OnDecryptionStatusChangeCallback* const decryption_status_change_callback_; diff --git a/video/buffered_frame_decryptor_unittest.cc b/video/buffered_frame_decryptor_unittest.cc index 7926f2421e..a056b4a720 100644 --- a/video/buffered_frame_decryptor_unittest.cc +++ b/video/buffered_frame_decryptor_unittest.cc @@ -55,6 +55,16 @@ class FakePacketBuffer : public video_coding::PacketBuffer { std::map packets_; }; +FrameDecryptorInterface::Result DecryptSuccess() { + return FrameDecryptorInterface::Result(FrameDecryptorInterface::Status::kOk, + 0); +} + +FrameDecryptorInterface::Result DecryptFail() { + return FrameDecryptorInterface::Result( + FrameDecryptorInterface::Status::kFailedToDecrypt, 0); +} + } // namespace class BufferedFrameDecryptorTest @@ -69,7 +79,7 @@ class BufferedFrameDecryptorTest decrypted_frame_call_count_++; } - void OnDecryptionStatusChange(int status) { + void OnDecryptionStatusChange(FrameDecryptorInterface::Status status) { ++decryption_status_change_count_; } @@ -127,7 +137,9 @@ const size_t BufferedFrameDecryptorTest::kMaxStashedFrames = 24; // Callback should always be triggered on a successful decryption. TEST_F(BufferedFrameDecryptorTest, CallbackCalledOnSuccessfulDecryption) { - EXPECT_CALL(*mock_frame_decryptor_, Decrypt).Times(1).WillOnce(Return(0)); + EXPECT_CALL(*mock_frame_decryptor_, Decrypt) + .Times(1) + .WillOnce(Return(DecryptSuccess())); EXPECT_CALL(*mock_frame_decryptor_, GetMaxPlaintextByteSize) .Times(1) .WillOnce(Return(0)); @@ -138,7 +150,9 @@ TEST_F(BufferedFrameDecryptorTest, CallbackCalledOnSuccessfulDecryption) { // An initial fail to decrypt should not trigger the callback. TEST_F(BufferedFrameDecryptorTest, CallbackNotCalledOnFailedDecryption) { - EXPECT_CALL(*mock_frame_decryptor_, Decrypt).Times(1).WillOnce(Return(1)); + EXPECT_CALL(*mock_frame_decryptor_, Decrypt) + .Times(1) + .WillOnce(Return(DecryptFail())); EXPECT_CALL(*mock_frame_decryptor_, GetMaxPlaintextByteSize) .Times(1) .WillOnce(Return(0)); @@ -152,9 +166,9 @@ TEST_F(BufferedFrameDecryptorTest, CallbackNotCalledOnFailedDecryption) { TEST_F(BufferedFrameDecryptorTest, DelayedCallbackOnBufferedFrames) { EXPECT_CALL(*mock_frame_decryptor_, Decrypt) .Times(3) - .WillOnce(Return(1)) - .WillOnce(Return(0)) - .WillOnce(Return(0)); + .WillOnce(Return(DecryptFail())) + .WillOnce(Return(DecryptSuccess())) + .WillOnce(Return(DecryptSuccess())); EXPECT_CALL(*mock_frame_decryptor_, GetMaxPlaintextByteSize) .Times(3) .WillRepeatedly(Return(0)); @@ -174,10 +188,10 @@ TEST_F(BufferedFrameDecryptorTest, DelayedCallbackOnBufferedFrames) { TEST_F(BufferedFrameDecryptorTest, FTDDiscardedAfterFirstSuccess) { EXPECT_CALL(*mock_frame_decryptor_, Decrypt) .Times(4) - .WillOnce(Return(1)) - .WillOnce(Return(0)) - .WillOnce(Return(0)) - .WillOnce(Return(1)); + .WillOnce(Return(DecryptFail())) + .WillOnce(Return(DecryptSuccess())) + .WillOnce(Return(DecryptSuccess())) + .WillOnce(Return(DecryptFail())); EXPECT_CALL(*mock_frame_decryptor_, GetMaxPlaintextByteSize) .Times(4) .WillRepeatedly(Return(0)); @@ -203,7 +217,7 @@ TEST_F(BufferedFrameDecryptorTest, MaximumNumberOfFramesStored) { const size_t failed_to_decrypt_count = kMaxStashedFrames * 2; EXPECT_CALL(*mock_frame_decryptor_, Decrypt) .Times(failed_to_decrypt_count) - .WillRepeatedly(Return(1)); + .WillRepeatedly(Return(DecryptFail())); EXPECT_CALL(*mock_frame_decryptor_, GetMaxPlaintextByteSize) .WillRepeatedly(Return(0)); @@ -215,7 +229,7 @@ TEST_F(BufferedFrameDecryptorTest, MaximumNumberOfFramesStored) { EXPECT_CALL(*mock_frame_decryptor_, Decrypt) .Times(kMaxStashedFrames + 1) - .WillRepeatedly(Return(0)); + .WillRepeatedly(Return(DecryptSuccess())); buffered_frame_decryptor_->ManageEncryptedFrame(CreateRtpFrameObject(true)); EXPECT_EQ(decrypted_frame_call_count_, kMaxStashedFrames + 1); EXPECT_EQ(decryption_status_change_count_, static_cast(2)); @@ -231,7 +245,7 @@ TEST_F(BufferedFrameDecryptorTest, FramesStoredIfDecryptorNull) { EXPECT_CALL(*mock_frame_decryptor_, Decrypt) .Times(kMaxStashedFrames + 1) - .WillRepeatedly(Return(0)); + .WillRepeatedly(Return(DecryptSuccess())); EXPECT_CALL(*mock_frame_decryptor_, GetMaxPlaintextByteSize) .WillRepeatedly(Return(0)); diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc index 87b735c624..2381bef81d 100644 --- a/video/rtp_video_stream_receiver.cc +++ b/video/rtp_video_stream_receiver.cc @@ -451,8 +451,11 @@ void RtpVideoStreamReceiver::OnDecryptedFrame( reference_finder_->ManageFrame(std::move(frame)); } -void RtpVideoStreamReceiver::OnDecryptionStatusChange(int status) { - frames_decryptable_.store(status == 0); +void RtpVideoStreamReceiver::OnDecryptionStatusChange( + FrameDecryptorInterface::Status status) { + frames_decryptable_.store( + (status == FrameDecryptorInterface::Status::kOk) || + (status == FrameDecryptorInterface::Status::kRecoverable)); } void RtpVideoStreamReceiver::SetFrameDecryptor( diff --git a/video/rtp_video_stream_receiver.h b/video/rtp_video_stream_receiver.h index 7caeedca16..14f5f4d428 100644 --- a/video/rtp_video_stream_receiver.h +++ b/video/rtp_video_stream_receiver.h @@ -153,7 +153,8 @@ class RtpVideoStreamReceiver : public LossNotificationSender, std::unique_ptr frame) override; // Implements OnDecryptionStatusChangeCallback. - void OnDecryptionStatusChange(int status) override; + void OnDecryptionStatusChange( + FrameDecryptorInterface::Status status) override; // Optionally set a frame decryptor after a stream has started. This will not // reset the decoder state.