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}

api/crypto/frame_decryptor_interface.h

@@ -34,9 +34,8 @@ 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
+  // 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 };
-  enum class Status { kOk, kRecoverable, kFailedToDecrypt, kUnknown };
 
   struct Result {
     Result(Status status, size_t bytes_written)
@@ -55,15 +54,33 @@ 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. kOk must be returned if successful,
+  // bytes you wrote to in the frame buffer. 0 must be returned if successful
-  // kRecoverable should be returned if the failure was due to something other
+  // all other numbers can be selected by the implementer to represent error
-  // than a decryption failure. kFailedToDecrypt should be returned in all other
+  // codes.
-  // cases.
+  // 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.
   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) = 0;
+                         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);
+  }
 
   // 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

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) {}
 
-FakeFrameDecryptor::Result FakeFrameDecryptor::Decrypt(
+int FakeFrameDecryptor::Decrypt(cricket::MediaType media_type,
-    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) {
+                                rtc::ArrayView<uint8_t> frame,
+                                size_t* bytes_written) {
   if (fail_decryption_) {
-    return Result(Status::kFailedToDecrypt, 0);
+    return static_cast<int>(FakeDecryptStatus::FORCED_FAILURE);
   }
 
   RTC_CHECK_EQ(frame.size() + 1, encrypted_frame.size());
@@ -34,10 +34,11 @@ FakeFrameDecryptor::Result FakeFrameDecryptor::Decrypt(
   }
 
   if (encrypted_frame[frame.size()] != expected_postfix_byte_) {
-    return Result(Status::kFailedToDecrypt, 0);
+    return static_cast<int>(FakeDecryptStatus::INVALID_POSTFIX);
   }
 
-  return Result(Status::kOk, frame.size());
+  *bytes_written = frame.size();
+  return static_cast<int>(FakeDecryptStatus::OK);
 }
 
 size_t FakeFrameDecryptor::GetMaxPlaintextByteSize(

api/test/fake_frame_decryptor.h

@@ -35,11 +35,12 @@ 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.
-  Result Decrypt(cricket::MediaType media_type,
+  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) override;
+              rtc::ArrayView<uint8_t> frame,
+              size_t* bytes_written) 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;

api/test/mock_frame_decryptor.h

@@ -23,12 +23,13 @@ class MockFrameDecryptor : public FrameDecryptorInterface {
   MockFrameDecryptor();
   ~MockFrameDecryptor() override;
 
-  MOCK_METHOD5(Decrypt,
+  MOCK_METHOD6(Decrypt,
-               Result(cricket::MediaType,
+               int(cricket::MediaType,
                    const std::vector<uint32_t>&,
                    rtc::ArrayView<const uint8_t>,
                    rtc::ArrayView<const uint8_t>,
-                      rtc::ArrayView<uint8_t>));
+                   rtc::ArrayView<uint8_t>,
+                   size_t*));
 
   MOCK_METHOD2(GetMaxPlaintextByteSize,
                size_t(cricket::MediaType, size_t encrypted_frame_size));

audio/channel_receive.cc

@@ -103,8 +103,8 @@ class ChannelReceive : public ChannelReceiveInterface,
   void StopPlayout() override;
 
   // Codecs
-  absl::optional<std::pair<int, SdpAudioFormat>> GetReceiveCodec()
+  absl::optional<std::pair<int, SdpAudioFormat>>
-      const override;
+      GetReceiveCodec() const override;
 
   void ReceivedRTCPPacket(const uint8_t* data, size_t length) override;
 
@@ -627,26 +627,28 @@ 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) {
-    const size_t max_plaintext_size = frame_decryptor_->GetMaxPlaintextByteSize(
+    size_t max_plaintext_size = frame_decryptor_->GetMaxPlaintextByteSize(
         cricket::MEDIA_TYPE_AUDIO, payload_length);
     decrypted_audio_payload.SetSize(max_plaintext_size);
 
-    const std::vector<uint32_t> csrcs(header.arrOfCSRCs,
+    size_t bytes_written = 0;
+    std::vector<uint32_t> csrcs(header.arrOfCSRCs,
                                 header.arrOfCSRCs + header.numCSRCs);
-    const FrameDecryptorInterface::Result decrypt_result =
+    int decrypt_status = frame_decryptor_->Decrypt(
-        frame_decryptor_->Decrypt(
         cricket::MEDIA_TYPE_AUDIO, csrcs,
         /*additional_data=*/nullptr,
         rtc::ArrayView<const uint8_t>(payload, payload_data_length),
-            decrypted_audio_payload);
+        decrypted_audio_payload, &bytes_written);
 
-    if (decrypt_result.IsOk()) {
+    // In this case just interpret the failure as a silent frame.
-      decrypted_audio_payload.SetSize(decrypt_result.bytes_written);
+    if (decrypt_status != 0) {
-    } else {
+      bytes_written = 0;
-      // 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) {

video/buffered_frame_decryptor.cc

@@ -83,25 +83,25 @@ BufferedFrameDecryptor::FrameDecision BufferedFrameDecryptor::DecryptFrame(
   }
 
   // Attempt to decrypt the video frame.
-  const FrameDecryptorInterface::Result decrypt_result =
+  size_t bytes_written = 0;
-      frame_decryptor_->Decrypt(cricket::MEDIA_TYPE_VIDEO, /*csrcs=*/{},
+  const int status = frame_decryptor_->Decrypt(
-                                additional_data, *frame,
+      cricket::MEDIA_TYPE_VIDEO, /*csrcs=*/{}, additional_data, *frame,
-                                inline_decrypted_bitstream);
+      inline_decrypted_bitstream, &bytes_written);
+
   // Optionally call the callback if there was a change in status
-  if (decrypt_result.status != last_status_) {
+  if (status != last_status_) {
-    last_status_ = decrypt_result.status;
+    last_status_ = status;
-    decryption_status_change_callback_->OnDecryptionStatusChange(
+    decryption_status_change_callback_->OnDecryptionStatusChange(status);
-        decrypt_result.status);
   }
 
-  if (!decrypt_result.IsOk()) {
+  if (status != 0) {
     // Only stash frames if we have never decrypted a frame before.
     return first_frame_decrypted_ ? FrameDecision::kDrop
                                   : FrameDecision::kStash;
   }
-  RTC_CHECK_LE(decrypt_result.bytes_written, max_plaintext_byte_size);
+  RTC_CHECK_LE(bytes_written, max_plaintext_byte_size);
   // Update the frame to contain just the written bytes.
-  frame->set_size(decrypt_result.bytes_written);
+  frame->set_size(bytes_written);
 
   // Indicate that all future fail to decrypt frames should be dropped.
   if (!first_frame_decrypted_) {

video/buffered_frame_decryptor.h

@@ -42,8 +42,7 @@ 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(
+  virtual void OnDecryptionStatusChange(int status) = 0;
-      FrameDecryptorInterface::Status status) = 0;
 };
 
 // The BufferedFrameDecryptor is responsible for deciding when to pass
@@ -92,8 +91,7 @@ class BufferedFrameDecryptor final {
 
   const bool generic_descriptor_auth_experiment_;
   bool first_frame_decrypted_ = false;
-  FrameDecryptorInterface::Status last_status_ =
+  int last_status_ = -1;
-      FrameDecryptorInterface::Status::kUnknown;
   rtc::scoped_refptr<FrameDecryptorInterface> frame_decryptor_;
   OnDecryptedFrameCallback* const decrypted_frame_callback_;
   OnDecryptionStatusChangeCallback* const decryption_status_change_callback_;

video/buffered_frame_decryptor_unittest.cc

@@ -55,16 +55,6 @@ class FakePacketBuffer : public video_coding::PacketBuffer {
   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
@@ -79,7 +69,7 @@ class BufferedFrameDecryptorTest
     decrypted_frame_call_count_++;
   }
 
-  void OnDecryptionStatusChange(FrameDecryptorInterface::Status status) {
+  void OnDecryptionStatusChange(int status) {
     ++decryption_status_change_count_;
   }
 
@@ -137,9 +127,7 @@ 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)
+  EXPECT_CALL(*mock_frame_decryptor_, Decrypt).Times(1).WillOnce(Return(0));
-      .Times(1)
-      .WillOnce(Return(DecryptSuccess()));
   EXPECT_CALL(*mock_frame_decryptor_, GetMaxPlaintextByteSize)
       .Times(1)
       .WillOnce(Return(0));
@@ -150,9 +138,7 @@ TEST_F(BufferedFrameDecryptorTest, CallbackCalledOnSuccessfulDecryption) {
 
 // An initial fail to decrypt should not trigger the callback.
 TEST_F(BufferedFrameDecryptorTest, CallbackNotCalledOnFailedDecryption) {
-  EXPECT_CALL(*mock_frame_decryptor_, Decrypt)
+  EXPECT_CALL(*mock_frame_decryptor_, Decrypt).Times(1).WillOnce(Return(1));
-      .Times(1)
-      .WillOnce(Return(DecryptFail()));
   EXPECT_CALL(*mock_frame_decryptor_, GetMaxPlaintextByteSize)
       .Times(1)
       .WillOnce(Return(0));
@@ -166,9 +152,9 @@ TEST_F(BufferedFrameDecryptorTest, CallbackNotCalledOnFailedDecryption) {
 TEST_F(BufferedFrameDecryptorTest, DelayedCallbackOnBufferedFrames) {
   EXPECT_CALL(*mock_frame_decryptor_, Decrypt)
       .Times(3)
-      .WillOnce(Return(DecryptFail()))
+      .WillOnce(Return(1))
-      .WillOnce(Return(DecryptSuccess()))
+      .WillOnce(Return(0))
-      .WillOnce(Return(DecryptSuccess()));
+      .WillOnce(Return(0));
   EXPECT_CALL(*mock_frame_decryptor_, GetMaxPlaintextByteSize)
       .Times(3)
       .WillRepeatedly(Return(0));
@@ -188,10 +174,10 @@ TEST_F(BufferedFrameDecryptorTest, DelayedCallbackOnBufferedFrames) {
 TEST_F(BufferedFrameDecryptorTest, FTDDiscardedAfterFirstSuccess) {
   EXPECT_CALL(*mock_frame_decryptor_, Decrypt)
       .Times(4)
-      .WillOnce(Return(DecryptFail()))
+      .WillOnce(Return(1))
-      .WillOnce(Return(DecryptSuccess()))
+      .WillOnce(Return(0))
-      .WillOnce(Return(DecryptSuccess()))
+      .WillOnce(Return(0))
-      .WillOnce(Return(DecryptFail()));
+      .WillOnce(Return(1));
   EXPECT_CALL(*mock_frame_decryptor_, GetMaxPlaintextByteSize)
       .Times(4)
       .WillRepeatedly(Return(0));
@@ -217,7 +203,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(DecryptFail()));
+      .WillRepeatedly(Return(1));
   EXPECT_CALL(*mock_frame_decryptor_, GetMaxPlaintextByteSize)
       .WillRepeatedly(Return(0));
 
@@ -229,7 +215,7 @@ TEST_F(BufferedFrameDecryptorTest, MaximumNumberOfFramesStored) {
 
   EXPECT_CALL(*mock_frame_decryptor_, Decrypt)
       .Times(kMaxStashedFrames + 1)
-      .WillRepeatedly(Return(DecryptSuccess()));
+      .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<size_t>(2));
@@ -245,7 +231,7 @@ TEST_F(BufferedFrameDecryptorTest, FramesStoredIfDecryptorNull) {
 
   EXPECT_CALL(*mock_frame_decryptor_, Decrypt)
       .Times(kMaxStashedFrames + 1)
-      .WillRepeatedly(Return(DecryptSuccess()));
+      .WillRepeatedly(Return(0));
   EXPECT_CALL(*mock_frame_decryptor_, GetMaxPlaintextByteSize)
       .WillRepeatedly(Return(0));
 

video/rtp_video_stream_receiver.cc

@@ -451,11 +451,8 @@ void RtpVideoStreamReceiver::OnDecryptedFrame(
   reference_finder_->ManageFrame(std::move(frame));
 }
 
-void RtpVideoStreamReceiver::OnDecryptionStatusChange(
+void RtpVideoStreamReceiver::OnDecryptionStatusChange(int status) {
-    FrameDecryptorInterface::Status status) {
+  frames_decryptable_.store(status == 0);
-  frames_decryptable_.store(
-      (status == FrameDecryptorInterface::Status::kOk) ||
-      (status == FrameDecryptorInterface::Status::kRecoverable));
 }
 
 void RtpVideoStreamReceiver::SetFrameDecryptor(

video/rtp_video_stream_receiver.h

@@ -153,8 +153,7 @@ class RtpVideoStreamReceiver : public LossNotificationSender,
       std::unique_ptr<video_coding::RtpFrameObject> frame) override;
 
   // Implements OnDecryptionStatusChangeCallback.
-  void OnDecryptionStatusChange(
+  void OnDecryptionStatusChange(int status) override;
-      FrameDecryptorInterface::Status status) override;
 
   // Optionally set a frame decryptor after a stream has started. This will not
   // reset the decoder state.