Reland "Refactor FrameDecryptorInterface::Decrypt to use new API."
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 <steveanton@webrtc.org>
> > Reviewed-by: Fredrik Solenberg <solenberg@webrtc.org>
> > Reviewed-by: Rasmus Brandt <brandtr@webrtc.org>
> > Reviewed-by: Stefan Holmer <stefan@webrtc.org>
> > Commit-Queue: Benjamin Wright <benwright@webrtc.org>
> > 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 <hbos@webrtc.org>
> Commit-Queue: Henrik Boström <hbos@webrtc.org>
> 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 <benwright@webrtc.org>
Commit-Queue: Benjamin Wright <benwright@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27529}diff --git a/api/crypto/frame_decryptor_interface.h b/api/crypto/frame_decryptor_interface.h
index 69f1375..ec900ab 100644
--- a/api/crypto/frame_decryptor_interface.h
+++ b/api/crypto/frame_decryptor_interface.h
@@ -34,8 +34,9 @@
// 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 @@
// 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<uint32_t>& csrcs,
- rtc::ArrayView<const uint8_t> additional_data,
- rtc::ArrayView<const uint8_t> encrypted_frame,
- rtc::ArrayView<uint8_t> 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<uint32_t>& csrcs,
rtc::ArrayView<const uint8_t> additional_data,
rtc::ArrayView<const uint8_t> encrypted_frame,
- rtc::ArrayView<uint8_t> 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<uint8_t> 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 b77017f..4af42a6 100644
--- a/api/test/fake_frame_decryptor.cc
+++ b/api/test/fake_frame_decryptor.cc
@@ -18,14 +18,14 @@
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<uint32_t>& csrcs,
- rtc::ArrayView<const uint8_t> additional_data,
- rtc::ArrayView<const uint8_t> encrypted_frame,
- rtc::ArrayView<uint8_t> frame,
- size_t* bytes_written) {
+FakeFrameDecryptor::Result FakeFrameDecryptor::Decrypt(
+ cricket::MediaType media_type,
+ const std::vector<uint32_t>& csrcs,
+ rtc::ArrayView<const uint8_t> additional_data,
+ rtc::ArrayView<const uint8_t> encrypted_frame,
+ rtc::ArrayView<uint8_t> frame) {
if (fail_decryption_) {
- return static_cast<int>(FakeDecryptStatus::FORCED_FAILURE);
+ return Result(Status::kFailedToDecrypt, 0);
}
RTC_CHECK_EQ(frame.size() + 1, encrypted_frame.size());
@@ -34,11 +34,10 @@
}
if (encrypted_frame[frame.size()] != expected_postfix_byte_) {
- return static_cast<int>(FakeDecryptStatus::INVALID_POSTFIX);
+ return Result(Status::kFailedToDecrypt, 0);
}
- *bytes_written = frame.size();
- return static_cast<int>(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 cb370b9..05813db 100644
--- a/api/test/fake_frame_decryptor.h
+++ b/api/test/fake_frame_decryptor.h
@@ -35,12 +35,11 @@
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<uint32_t>& csrcs,
- rtc::ArrayView<const uint8_t> additional_data,
- rtc::ArrayView<const uint8_t> encrypted_frame,
- rtc::ArrayView<uint8_t> frame,
- size_t* bytes_written) override;
+ Result Decrypt(cricket::MediaType media_type,
+ const std::vector<uint32_t>& csrcs,
+ rtc::ArrayView<const uint8_t> additional_data,
+ rtc::ArrayView<const uint8_t> encrypted_frame,
+ rtc::ArrayView<uint8_t> 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 feac9b3..77aa4f9 100644
--- a/api/test/mock_frame_decryptor.h
+++ b/api/test/mock_frame_decryptor.h
@@ -23,13 +23,12 @@
MockFrameDecryptor();
~MockFrameDecryptor() override;
- MOCK_METHOD6(Decrypt,
- int(cricket::MediaType,
- const std::vector<uint32_t>&,
- rtc::ArrayView<const uint8_t>,
- rtc::ArrayView<const uint8_t>,
- rtc::ArrayView<uint8_t>,
- size_t*));
+ MOCK_METHOD5(Decrypt,
+ Result(cricket::MediaType,
+ const std::vector<uint32_t>&,
+ rtc::ArrayView<const uint8_t>,
+ rtc::ArrayView<const uint8_t>,
+ rtc::ArrayView<uint8_t>));
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 088aa57..4f00d9f 100644
--- a/audio/channel_receive.cc
+++ b/audio/channel_receive.cc
@@ -103,8 +103,8 @@
void StopPlayout() override;
// Codecs
- absl::optional<std::pair<int, SdpAudioFormat>>
- GetReceiveCodec() const override;
+ absl::optional<std::pair<int, SdpAudioFormat>> GetReceiveCodec()
+ const override;
void ReceivedRTCPPacket(const uint8_t* data, size_t length) override;
@@ -627,28 +627,26 @@
// 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<uint32_t> csrcs(header.arrOfCSRCs,
- header.arrOfCSRCs + header.numCSRCs);
- int decrypt_status = frame_decryptor_->Decrypt(
- cricket::MEDIA_TYPE_AUDIO, csrcs,
- /*additional_data=*/nullptr,
- rtc::ArrayView<const uint8_t>(payload, payload_data_length),
- decrypted_audio_payload, &bytes_written);
+ const std::vector<uint32_t> 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<const uint8_t>(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 2d7ec25..41eddea 100644
--- a/video/buffered_frame_decryptor.cc
+++ b/video/buffered_frame_decryptor.cc
@@ -83,25 +83,25 @@
}
// 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 06992bb..49ab9a7 100644
--- a/video/buffered_frame_decryptor.h
+++ b/video/buffered_frame_decryptor.h
@@ -42,7 +42,8 @@
// 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 @@
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<FrameDecryptorInterface> 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 7926f24..a056b4a 100644
--- a/video/buffered_frame_decryptor_unittest.cc
+++ b/video/buffered_frame_decryptor_unittest.cc
@@ -55,6 +55,16 @@
std::map<uint16_t, VCMPacket> 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 @@
decrypted_frame_call_count_++;
}
- void OnDecryptionStatusChange(int status) {
+ void OnDecryptionStatusChange(FrameDecryptorInterface::Status status) {
++decryption_status_change_count_;
}
@@ -127,7 +137,9 @@
// 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 @@
// 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, 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, 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 @@
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 @@
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<size_t>(2));
@@ -231,7 +245,7 @@
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 87b735c..2381bef 100644
--- a/video/rtp_video_stream_receiver.cc
+++ b/video/rtp_video_stream_receiver.cc
@@ -451,8 +451,11 @@
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 7caeedc..14f5f4d 100644
--- a/video/rtp_video_stream_receiver.h
+++ b/video/rtp_video_stream_receiver.h
@@ -153,7 +153,8 @@
std::unique_ptr<video_coding::RtpFrameObject> 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.
