NetEq: Removing LastError and LastDecoderError

LastDecoderError was only used in tests. LastError was only used in
conjunction with RemovePayloadType, and always to distinguish between
"decoder not found" and "other error". In AcmReceiver, "decoder not
found" was not treated as an error.

With this change, calling NetEq::RemovePayloadType with a payload type
that is not registered is no longer considered to be an error. This
allows to rewrite the code in AcmReceiver, such that it no longer has
to call LastError.

The internal member variables NetEqImpl::error_code_ and
NetEqImpl::decoder_error_code_ are removed, since they were no longer
read.

Bug: none
Change-Id: Ibfe97265954a2870c3caea4a34aac958351d7ff1
Reviewed-on: https://chromium-review.googlesource.com/535533
Commit-Queue: Henrik Lundin <henrik.lundin@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#18588}

webrtc/modules/audio_coding/acm2/acm_receiver.cc

@@ -219,8 +219,7 @@ int32_t AcmReceiver::AddCodec(int acm_codec_id,
     return 0;
   }
 
-  if (neteq_->RemovePayloadType(payload_type) != NetEq::kOK &&
+  if (neteq_->RemovePayloadType(payload_type) != NetEq::kOK) {
-      neteq_->LastError() != NetEq::kDecoderNotFound) {
     LOG(LERROR) << "Cannot remove payload " << static_cast<int>(payload_type);
     return -1;
   }
@@ -249,8 +248,7 @@ bool AcmReceiver::AddCodec(int rtp_payload_type,
     return true;
   }
 
-  if (neteq_->RemovePayloadType(rtp_payload_type) != NetEq::kOK &&
+  if (neteq_->RemovePayloadType(rtp_payload_type) != NetEq::kOK) {
-      neteq_->LastError() != NetEq::kDecoderNotFound) {
     LOG(LERROR) << "AcmReceiver::AddCodec: Could not remove existing decoder"
                    " for payload type "
                 << rtp_payload_type;
@@ -280,9 +278,9 @@ void AcmReceiver::RemoveAllCodecs() {
 
 int AcmReceiver::RemoveCodec(uint8_t payload_type) {
   rtc::CritScope lock(&crit_sect_);
-  if (neteq_->RemovePayloadType(payload_type) != NetEq::kOK &&
+  if (neteq_->RemovePayloadType(payload_type) != NetEq::kOK) {
-      neteq_->LastError() != NetEq::kDecoderNotFound) {
+    LOG(LERROR) << "AcmReceiver::RemoveCodec "
-    LOG(LERROR) << "AcmReceiver::RemoveCodec" << static_cast<int>(payload_type);
+                << static_cast<int>(payload_type);
     return -1;
   }
   if (last_audio_decoder_ && payload_type == last_audio_decoder_->pltype) {

webrtc/modules/audio_coding/neteq/include/neteq.h

@@ -102,32 +102,6 @@ class NetEq {
     kNotImplemented = -2
   };
 
-  enum ErrorCodes {
-    kNoError = 0,
-    kOtherError,
-    kInvalidRtpPayloadType,
-    kUnknownRtpPayloadType,
-    kCodecNotSupported,
-    kDecoderExists,
-    kDecoderNotFound,
-    kInvalidSampleRate,
-    kInvalidPointer,
-    kAccelerateError,
-    kPreemptiveExpandError,
-    kComfortNoiseErrorCode,
-    kDecoderErrorCode,
-    kOtherDecoderError,
-    kInvalidOperation,
-    kDtmfParameterError,
-    kDtmfParsingError,
-    kDtmfInsertError,
-    kStereoNotSupported,
-    kSampleUnderrun,
-    kDecodedTooMuch,
-    kRedundancySplitError,
-    kPacketBufferCorruption
-  };
-
   // Creates a new NetEq object, with parameters set in |config|. The |config|
   // object will only have to be valid for the duration of the call to this
   // method.
@@ -191,7 +165,8 @@ class NetEq {
                                    const SdpAudioFormat& audio_format) = 0;
 
   // Removes |rtp_payload_type| from the codec database. Returns 0 on success,
-  // -1 on failure.
+  // -1 on failure. Removing a payload type that is not registered is ok and
+  // will not result in an error.
   virtual int RemovePayloadType(uint8_t rtp_payload_type) = 0;
 
   // Removes all payload types from the codec database.
@@ -282,15 +257,6 @@ class NetEq {
   // Not implemented.
   virtual int SetTargetSampleRate() = 0;
 
-  // Returns the error code for the last occurred error. If no error has
-  // occurred, 0 is returned.
-  virtual int LastError() const = 0;
-
-  // Returns the error code last returned by a decoder (audio or comfort noise).
-  // When LastError() returns kDecoderErrorCode or kComfortNoiseErrorCode, check
-  // this method to get the decoder's error code.
-  virtual int LastDecoderError() = 0;
-
   // Flushes both the packet buffer and the sync buffer.
   virtual void FlushBuffers() = 0;
 

webrtc/modules/audio_coding/neteq/neteq_impl.cc

@@ -98,8 +98,6 @@ NetEqImpl::NetEqImpl(const NetEq::Config& config,
       reset_decoder_(false),
       ssrc_(0),
       first_packet_(true),
-      error_code_(0),
-      decoder_error_code_(0),
       background_noise_mode_(config.background_noise_mode),
       playout_mode_(config.playout_mode),
       enable_fast_accelerate_(config.enable_fast_accelerate),
@@ -136,10 +134,7 @@ int NetEqImpl::InsertPacket(const RTPHeader& rtp_header,
   rtc::MsanCheckInitialized(payload);
   TRACE_EVENT0("webrtc", "NetEqImpl::InsertPacket");
   rtc::CritScope lock(&crit_sect_);
-  int error =
+  if (InsertPacketInternal(rtp_header, payload, receive_timestamp) != 0) {
-      InsertPacketInternal(rtp_header, payload, receive_timestamp);
-  if (error != 0) {
-    error_code_ = error;
     return kFail;
   }
   return kOK;
@@ -199,9 +194,7 @@ void SetAudioFrameActivityAndType(bool vad_enabled,
 int NetEqImpl::GetAudio(AudioFrame* audio_frame, bool* muted) {
   TRACE_EVENT0("webrtc", "NetEqImpl::GetAudio");
   rtc::CritScope lock(&crit_sect_);
-  int error = GetAudioInternal(audio_frame, muted);
+  if (GetAudioInternal(audio_frame, muted) != 0) {
-  if (error != 0) {
-    error_code_ = error;
     return kFail;
   }
   RTC_DCHECK_EQ(
@@ -235,21 +228,8 @@ int NetEqImpl::RegisterPayloadType(NetEqDecoder codec,
   LOG(LS_VERBOSE) << "RegisterPayloadType "
                   << static_cast<int>(rtp_payload_type) << " "
                   << static_cast<int>(codec);
-  int ret = decoder_database_->RegisterPayload(rtp_payload_type, codec, name);
+  if (decoder_database_->RegisterPayload(rtp_payload_type, codec, name) !=
-  if (ret != DecoderDatabase::kOK) {
+      DecoderDatabase::kOK) {
-    switch (ret) {
-      case DecoderDatabase::kInvalidRtpPayloadType:
-        error_code_ = kInvalidRtpPayloadType;
-        break;
-      case DecoderDatabase::kCodecNotSupported:
-        error_code_ = kCodecNotSupported;
-        break;
-      case DecoderDatabase::kDecoderExists:
-        error_code_ = kDecoderExists;
-        break;
-      default:
-        error_code_ = kOtherError;
-    }
     return kFail;
   }
   return kOK;
@@ -268,28 +248,8 @@ int NetEqImpl::RegisterExternalDecoder(AudioDecoder* decoder,
     assert(false);
     return kFail;
   }
-  int ret = decoder_database_->InsertExternal(rtp_payload_type, codec,
+  if (decoder_database_->InsertExternal(rtp_payload_type, codec, codec_name,
-                                              codec_name, decoder);
+                                        decoder) != DecoderDatabase::kOK) {
-  if (ret != DecoderDatabase::kOK) {
-    switch (ret) {
-      case DecoderDatabase::kInvalidRtpPayloadType:
-        error_code_ = kInvalidRtpPayloadType;
-        break;
-      case DecoderDatabase::kCodecNotSupported:
-        error_code_ = kCodecNotSupported;
-        break;
-      case DecoderDatabase::kDecoderExists:
-        error_code_ = kDecoderExists;
-        break;
-      case DecoderDatabase::kInvalidSampleRate:
-        error_code_ = kInvalidSampleRate;
-        break;
-      case DecoderDatabase::kInvalidPointer:
-        error_code_ = kInvalidPointer;
-        break;
-      default:
-        error_code_ = kOtherError;
-    }
     return kFail;
   }
   return kOK;
@@ -300,40 +260,16 @@ bool NetEqImpl::RegisterPayloadType(int rtp_payload_type,
   LOG(LS_VERBOSE) << "NetEqImpl::RegisterPayloadType: payload type "
                   << rtp_payload_type << ", codec " << audio_format;
   rtc::CritScope lock(&crit_sect_);
-  switch (decoder_database_->RegisterPayload(rtp_payload_type, audio_format)) {
+  return decoder_database_->RegisterPayload(rtp_payload_type, audio_format) ==
-    case DecoderDatabase::kOK:
+         DecoderDatabase::kOK;
-      return true;
-    case DecoderDatabase::kInvalidRtpPayloadType:
-      error_code_ = kInvalidRtpPayloadType;
-      return false;
-    case DecoderDatabase::kCodecNotSupported:
-      error_code_ = kCodecNotSupported;
-      return false;
-    case DecoderDatabase::kDecoderExists:
-      error_code_ = kDecoderExists;
-      return false;
-    case DecoderDatabase::kInvalidSampleRate:
-      error_code_ = kInvalidSampleRate;
-      return false;
-    case DecoderDatabase::kInvalidPointer:
-      error_code_ = kInvalidPointer;
-      return false;
-    default:
-      error_code_ = kOtherError;
-      return false;
-  }
 }
 
 int NetEqImpl::RemovePayloadType(uint8_t rtp_payload_type) {
   rtc::CritScope lock(&crit_sect_);
   int ret = decoder_database_->Remove(rtp_payload_type);
-  if (ret == DecoderDatabase::kOK) {
+  if (ret == DecoderDatabase::kOK || ret == DecoderDatabase::kDecoderNotFound) {
     packet_buffer_->DiscardPacketsWithPayloadType(rtp_payload_type);
     return kOK;
-  } else if (ret == DecoderDatabase::kDecoderNotFound) {
-    error_code_ = kDecoderNotFound;
-  } else {
-    error_code_ = kOtherError;
   }
   return kFail;
 }
@@ -527,16 +463,6 @@ int NetEqImpl::SetTargetSampleRate() {
   return kNotImplemented;
 }
 
-int NetEqImpl::LastError() const {
-  rtc::CritScope lock(&crit_sect_);
-  return error_code_;
-}
-
-int NetEqImpl::LastDecoderError() {
-  rtc::CritScope lock(&crit_sect_);
-  return decoder_error_code_;
-}
-
 void NetEqImpl::FlushBuffers() {
   rtc::CritScope lock(&crit_sect_);
   LOG(LS_VERBOSE) << "FlushBuffers";
@@ -1469,7 +1395,6 @@ int NetEqImpl::Decode(PacketList* packet_list, Operations* operation,
       error_code = decoder->ErrorCode();
     if (error_code != 0) {
       // Got some error code from the decoder.
-      decoder_error_code_ = error_code;
       return_value = kDecoderErrorCode;
       LOG(LS_WARNING) << "Decoder returned error code: " << error_code;
     } else {
@@ -1835,7 +1760,8 @@ int NetEqImpl::DoRfc3389Cng(PacketList* packet_list, bool play_dtmf) {
     dtmf_tone_generator_->Reset();
   }
   if (cn_return == ComfortNoise::kInternalError) {
-    decoder_error_code_ = comfort_noise_->internal_error_code();
+    LOG(LS_WARNING) << "Comfort noise generator returned error code: "
+                    << comfort_noise_->internal_error_code();
     return kComfortNoiseErrorCode;
   } else if (cn_return == ComfortNoise::kUnknownPayloadType) {
     return kUnknownRtpPayloadType;

webrtc/modules/audio_coding/neteq/neteq_impl.h

@@ -68,6 +68,26 @@ class NetEqImpl : public webrtc::NetEq {
     kVadPassive
   };
 
+  enum ErrorCodes {
+    kNoError = 0,
+    kOtherError,
+    kUnknownRtpPayloadType,
+    kDecoderNotFound,
+    kInvalidPointer,
+    kAccelerateError,
+    kPreemptiveExpandError,
+    kComfortNoiseErrorCode,
+    kDecoderErrorCode,
+    kOtherDecoderError,
+    kInvalidOperation,
+    kDtmfParsingError,
+    kDtmfInsertError,
+    kSampleUnderrun,
+    kDecodedTooMuch,
+    kRedundancySplitError,
+    kPacketBufferCorruption
+  };
+
   struct Dependencies {
     // The constructor populates the Dependencies struct with the default
     // implementations of the objects. They can all be replaced by the user
@@ -188,15 +208,6 @@ class NetEqImpl : public webrtc::NetEq {
 
   int SetTargetSampleRate() override;
 
-  // Returns the error code for the last occurred error. If no error has
-  // occurred, 0 is returned.
-  int LastError() const override;
-
-  // Returns the error code last returned by a decoder (audio or comfort noise).
-  // When LastError() returns kDecoderErrorCode or kComfortNoiseErrorCode, check
-  // this method to get the decoder's error code.
-  int LastDecoderError() override;
-
   // Flushes both the packet buffer and the sync buffer.
   void FlushBuffers() override;
 
@@ -408,8 +419,6 @@ class NetEqImpl : public webrtc::NetEq {
   rtc::Optional<uint8_t> current_cng_rtp_payload_type_ GUARDED_BY(crit_sect_);
   uint32_t ssrc_ GUARDED_BY(crit_sect_);
   bool first_packet_ GUARDED_BY(crit_sect_);
-  int error_code_ GUARDED_BY(crit_sect_);  // Store last error code.
-  int decoder_error_code_ GUARDED_BY(crit_sect_);
   const BackgroundNoiseMode background_noise_mode_ GUARDED_BY(crit_sect_);
   NetEqPlayoutMode playout_mode_ GUARDED_BY(crit_sect_);
   bool enable_fast_accelerate_ GUARDED_BY(crit_sect_);

webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc

@@ -281,8 +281,9 @@ TEST_F(NetEqImplTest, RemovePayloadType) {
   uint8_t rtp_payload_type = 0;
   EXPECT_CALL(*mock_decoder_database_, Remove(rtp_payload_type))
       .WillOnce(Return(DecoderDatabase::kDecoderNotFound));
-  // Check that kFail is returned when database returns kDecoderNotFound.
+  // Check that kOK is returned when database returns kDecoderNotFound, because
-  EXPECT_EQ(NetEq::kFail, neteq_->RemovePayloadType(rtp_payload_type));
+  // removing a payload type that was never registered is not an error.
+  EXPECT_EQ(NetEq::kOK, neteq_->RemovePayloadType(rtp_payload_type));
 }
 
 TEST_F(NetEqImplTest, RemoveAllPayloadTypes) {
@@ -653,7 +654,6 @@ TEST_F(NetEqImplTest, FirstPacketUnknown) {
   // this packet will be rejected.
   EXPECT_EQ(NetEq::kFail,
             neteq_->InsertPacket(rtp_header, payload, kReceiveTime));
-  EXPECT_EQ(NetEq::kUnknownRtpPayloadType, neteq_->LastError());
 
   // Pull audio once.
   const size_t kMaxOutputSize = static_cast<size_t>(10 * kSampleRateHz / 1000);
@@ -911,10 +911,8 @@ TEST_F(NetEqImplTest, UnsupportedDecoder) {
   AudioFrame output;
   bool muted;
   // First call to GetAudio will try to decode the "faulty" packet.
-  // Expect kFail return value...
+  // Expect kFail return value.
   EXPECT_EQ(NetEq::kFail, neteq_->GetAudio(&output, &muted));
-  // ... and kOtherDecoderError error code.
-  EXPECT_EQ(NetEq::kOtherDecoderError, neteq_->LastError());
   // Output size and number of channels should be correct.
   const size_t kExpectedOutputSize = 10 * (kSampleRateHz / 1000) * kChannels;
   EXPECT_EQ(kExpectedOutputSize, output.samples_per_channel_ * kChannels);
@@ -1123,8 +1121,6 @@ TEST_F(NetEqImplTest, DecodingError) {
 
   // Pull audio again. Decoder fails.
   EXPECT_EQ(NetEq::kFail, neteq_->GetAudio(&output, &muted));
-  EXPECT_EQ(NetEq::kDecoderErrorCode, neteq_->LastError());
-  EXPECT_EQ(kDecoderErrorCode, neteq_->LastDecoderError());
   EXPECT_EQ(kMaxOutputSize, output.samples_per_channel_);
   EXPECT_EQ(1u, output.num_channels_);
   // We are not expecting anything for output.speech_type_, since an error was
@@ -1235,8 +1231,6 @@ TEST_F(NetEqImplTest, DecodingErrorDuringInternalCng) {
 
   // Pull audio again. Decoder fails.
   EXPECT_EQ(NetEq::kFail, neteq_->GetAudio(&output, &muted));
-  EXPECT_EQ(NetEq::kDecoderErrorCode, neteq_->LastError());
-  EXPECT_EQ(kDecoderErrorCode, neteq_->LastDecoderError());
   EXPECT_EQ(kMaxOutputSize, output.samples_per_channel_);
   EXPECT_EQ(1u, output.num_channels_);
   // We are not expecting anything for output.speech_type_, since an error was

webrtc/modules/audio_coding/neteq/neteq_unittest.cc

@@ -830,7 +830,6 @@ TEST_F(NetEqDecodingTest, UnknownPayloadType) {
   PopulateRtpInfo(0, 0, &rtp_info);
   rtp_info.payloadType = 1;  // Not registered as a decoder.
   EXPECT_EQ(NetEq::kFail, neteq_->InsertPacket(rtp_info, payload, 0));
-  EXPECT_EQ(NetEq::kUnknownRtpPayloadType, neteq_->LastError());
 }
 
 #if defined(WEBRTC_CODEC_ISAC) || defined(WEBRTC_CODEC_ISACFX)
@@ -855,18 +854,7 @@ TEST_F(NetEqDecodingTest, MAYBE_DecoderError) {
   bool muted;
   EXPECT_EQ(NetEq::kFail, neteq_->GetAudio(&out_frame_, &muted));
   ASSERT_FALSE(muted);
-  // Verify that there is a decoder error to check.
-  EXPECT_EQ(NetEq::kDecoderErrorCode, neteq_->LastError());
 
-  enum NetEqDecoderError {
-    ISAC_LENGTH_MISMATCH = 6730,
-    ISAC_RANGE_ERROR_DECODE_FRAME_LENGTH = 6640
-  };
-#if defined(WEBRTC_CODEC_ISAC)
-  EXPECT_EQ(ISAC_LENGTH_MISMATCH, neteq_->LastDecoderError());
-#elif defined(WEBRTC_CODEC_ISACFX)
-  EXPECT_EQ(ISAC_RANGE_ERROR_DECODE_FRAME_LENGTH, neteq_->LastDecoderError());
-#endif
   // Verify that the first 160 samples are set to 0.
   static const int kExpectedOutputLength = 160;  // 10 ms at 16 kHz sample rate.
   const int16_t* const_out_frame_data = out_frame_.data();

webrtc/modules/audio_coding/neteq/tools/neteq_test.cc

@@ -18,21 +18,14 @@ namespace webrtc {
 namespace test {
 
 void DefaultNetEqTestErrorCallback::OnInsertPacketError(
-    int error_code,
     const NetEqInput::PacketData& packet) {
-  if (error_code == NetEq::kUnknownRtpPayloadType) {
+  std::cerr << "InsertPacket returned an error." << std::endl;
-    std::cerr << "RTP Payload type "
-              << static_cast<int>(packet.header.payloadType) << " is unknown."
-              << std::endl;
-  } else {
-    std::cerr << "InsertPacket returned error code " << error_code << std::endl;
-  }
   std::cerr << "Packet data: " << packet.ToString() << std::endl;
   FATAL();
 }
 
-void DefaultNetEqTestErrorCallback::OnGetAudioError(int error_code) {
+void DefaultNetEqTestErrorCallback::OnGetAudioError() {
-  std::cerr << "GetAudio returned error code " << error_code << std::endl;
+  std::cerr << "GetAudio returned an error." << std::endl;
   FATAL();
 }
 
@@ -70,8 +63,7 @@ int64_t NetEqTest::Run() {
           rtc::ArrayView<const uint8_t>(packet_data->payload),
           static_cast<uint32_t>(packet_data->time_ms * sample_rate_hz_ / 1000));
       if (error != NetEq::kOK && callbacks_.error_callback) {
-        callbacks_.error_callback->OnInsertPacketError(neteq_->LastError(),
+        callbacks_.error_callback->OnInsertPacketError(*packet_data);
-                                                       *packet_data);
       }
       if (callbacks_.post_insert_packet) {
         callbacks_.post_insert_packet->AfterInsertPacket(*packet_data,
@@ -91,7 +83,7 @@ int64_t NetEqTest::Run() {
       RTC_CHECK(!muted) << "The code does not handle enable_muted_state";
       if (error != NetEq::kOK) {
         if (callbacks_.error_callback) {
-          callbacks_.error_callback->OnGetAudioError(neteq_->LastError());
+          callbacks_.error_callback->OnGetAudioError();
         }
       } else {
         sample_rate_hz_ = out_frame.sample_rate_hz_;

webrtc/modules/audio_coding/neteq/tools/neteq_test.h

@@ -26,15 +26,13 @@ namespace test {
 class NetEqTestErrorCallback {
  public:
   virtual ~NetEqTestErrorCallback() = default;
-  virtual void OnInsertPacketError(int error_code,
+  virtual void OnInsertPacketError(const NetEqInput::PacketData& packet) {}
-                                   const NetEqInput::PacketData& packet) {}
+  virtual void OnGetAudioError() {}
-  virtual void OnGetAudioError(int error_code) {}
 };
 
 class DefaultNetEqTestErrorCallback : public NetEqTestErrorCallback {
-  void OnInsertPacketError(int error_code,
+  void OnInsertPacketError(const NetEqInput::PacketData& packet) override;
-                           const NetEqInput::PacketData& packet) override;
+  void OnGetAudioError() override;
-  void OnGetAudioError(int error_code) override;
 };
 
 class NetEqPostInsertPacket {