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}diff --git a/webrtc/modules/audio_coding/acm2/acm_receiver.cc b/webrtc/modules/audio_coding/acm2/acm_receiver.cc
index a2a5eb7..8f21e89 100644
--- a/webrtc/modules/audio_coding/acm2/acm_receiver.cc
+++ b/webrtc/modules/audio_coding/acm2/acm_receiver.cc
@@ -219,8 +219,7 @@
return 0;
}
- if (neteq_->RemovePayloadType(payload_type) != NetEq::kOK &&
- neteq_->LastError() != NetEq::kDecoderNotFound) {
+ if (neteq_->RemovePayloadType(payload_type) != NetEq::kOK) {
LOG(LERROR) << "Cannot remove payload " << static_cast<int>(payload_type);
return -1;
}
@@ -249,8 +248,7 @@
return true;
}
- if (neteq_->RemovePayloadType(rtp_payload_type) != NetEq::kOK &&
- neteq_->LastError() != NetEq::kDecoderNotFound) {
+ if (neteq_->RemovePayloadType(rtp_payload_type) != NetEq::kOK) {
LOG(LERROR) << "AcmReceiver::AddCodec: Could not remove existing decoder"
" for payload type "
<< rtp_payload_type;
@@ -280,9 +278,9 @@
int AcmReceiver::RemoveCodec(uint8_t payload_type) {
rtc::CritScope lock(&crit_sect_);
- if (neteq_->RemovePayloadType(payload_type) != NetEq::kOK &&
- neteq_->LastError() != NetEq::kDecoderNotFound) {
- LOG(LERROR) << "AcmReceiver::RemoveCodec" << static_cast<int>(payload_type);
+ if (neteq_->RemovePayloadType(payload_type) != NetEq::kOK) {
+ LOG(LERROR) << "AcmReceiver::RemoveCodec "
+ << static_cast<int>(payload_type);
return -1;
}
if (last_audio_decoder_ && payload_type == last_audio_decoder_->pltype) {
diff --git a/webrtc/modules/audio_coding/neteq/include/neteq.h b/webrtc/modules/audio_coding/neteq/include/neteq.h
index 45aae11..648c04f 100644
--- a/webrtc/modules/audio_coding/neteq/include/neteq.h
+++ b/webrtc/modules/audio_coding/neteq/include/neteq.h
@@ -102,32 +102,6 @@
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 @@
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 @@
// 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;
diff --git a/webrtc/modules/audio_coding/neteq/neteq_impl.cc b/webrtc/modules/audio_coding/neteq/neteq_impl.cc
index f9ec3bb..fb3cc11 100644
--- a/webrtc/modules/audio_coding/neteq/neteq_impl.cc
+++ b/webrtc/modules/audio_coding/neteq/neteq_impl.cc
@@ -98,8 +98,6 @@
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 @@
rtc::MsanCheckInitialized(payload);
TRACE_EVENT0("webrtc", "NetEqImpl::InsertPacket");
rtc::CritScope lock(&crit_sect_);
- int error =
- InsertPacketInternal(rtp_header, payload, receive_timestamp);
- if (error != 0) {
- error_code_ = error;
+ if (InsertPacketInternal(rtp_header, payload, receive_timestamp) != 0) {
return kFail;
}
return kOK;
@@ -199,9 +194,7 @@
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 (error != 0) {
- error_code_ = error;
+ if (GetAudioInternal(audio_frame, muted) != 0) {
return kFail;
}
RTC_DCHECK_EQ(
@@ -235,21 +228,8 @@
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 (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;
- default:
- error_code_ = kOtherError;
- }
+ if (decoder_database_->RegisterPayload(rtp_payload_type, codec, name) !=
+ DecoderDatabase::kOK) {
return kFail;
}
return kOK;
@@ -268,28 +248,8 @@
assert(false);
return kFail;
}
- int ret = decoder_database_->InsertExternal(rtp_payload_type, codec,
- codec_name, decoder);
- 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;
- }
+ if (decoder_database_->InsertExternal(rtp_payload_type, codec, codec_name,
+ decoder) != DecoderDatabase::kOK) {
return kFail;
}
return kOK;
@@ -300,40 +260,16 @@
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)) {
- case 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;
- }
+ return decoder_database_->RegisterPayload(rtp_payload_type, audio_format) ==
+ DecoderDatabase::kOK;
}
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 @@
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 @@
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 @@
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;
diff --git a/webrtc/modules/audio_coding/neteq/neteq_impl.h b/webrtc/modules/audio_coding/neteq/neteq_impl.h
index 3436765..d1d2039 100644
--- a/webrtc/modules/audio_coding/neteq/neteq_impl.h
+++ b/webrtc/modules/audio_coding/neteq/neteq_impl.h
@@ -68,6 +68,26 @@
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 @@
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 @@
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_);
diff --git a/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc b/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc
index b6c4a77..0676ad6 100644
--- a/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc
+++ b/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc
@@ -281,8 +281,9 @@
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.
- EXPECT_EQ(NetEq::kFail, neteq_->RemovePayloadType(rtp_payload_type));
+ // Check that kOK is returned when database returns kDecoderNotFound, because
+ // 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 @@
// 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 @@
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 @@
// 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 @@
// 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
diff --git a/webrtc/modules/audio_coding/neteq/neteq_unittest.cc b/webrtc/modules/audio_coding/neteq/neteq_unittest.cc
index fae1e23..eed7740 100644
--- a/webrtc/modules/audio_coding/neteq/neteq_unittest.cc
+++ b/webrtc/modules/audio_coding/neteq/neteq_unittest.cc
@@ -830,7 +830,6 @@
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 @@
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();
diff --git a/webrtc/modules/audio_coding/neteq/tools/neteq_test.cc b/webrtc/modules/audio_coding/neteq/tools/neteq_test.cc
index 34a1d50..598ffd7 100644
--- a/webrtc/modules/audio_coding/neteq/tools/neteq_test.cc
+++ b/webrtc/modules/audio_coding/neteq/tools/neteq_test.cc
@@ -18,21 +18,14 @@
namespace test {
void DefaultNetEqTestErrorCallback::OnInsertPacketError(
- int error_code,
const NetEqInput::PacketData& packet) {
- if (error_code == NetEq::kUnknownRtpPayloadType) {
- 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 << "InsertPacket returned an error." << std::endl;
std::cerr << "Packet data: " << packet.ToString() << std::endl;
FATAL();
}
-void DefaultNetEqTestErrorCallback::OnGetAudioError(int error_code) {
- std::cerr << "GetAudio returned error code " << error_code << std::endl;
+void DefaultNetEqTestErrorCallback::OnGetAudioError() {
+ std::cerr << "GetAudio returned an error." << std::endl;
FATAL();
}
@@ -70,8 +63,7 @@
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(),
- *packet_data);
+ callbacks_.error_callback->OnInsertPacketError(*packet_data);
}
if (callbacks_.post_insert_packet) {
callbacks_.post_insert_packet->AfterInsertPacket(*packet_data,
@@ -91,7 +83,7 @@
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_;
diff --git a/webrtc/modules/audio_coding/neteq/tools/neteq_test.h b/webrtc/modules/audio_coding/neteq/tools/neteq_test.h
index e4d3996..18fad9a 100644
--- a/webrtc/modules/audio_coding/neteq/tools/neteq_test.h
+++ b/webrtc/modules/audio_coding/neteq/tools/neteq_test.h
@@ -26,15 +26,13 @@
class NetEqTestErrorCallback {
public:
virtual ~NetEqTestErrorCallback() = default;
- virtual void OnInsertPacketError(int error_code,
- const NetEqInput::PacketData& packet) {}
- virtual void OnGetAudioError(int error_code) {}
+ virtual void OnInsertPacketError(const NetEqInput::PacketData& packet) {}
+ virtual void OnGetAudioError() {}
};
class DefaultNetEqTestErrorCallback : public NetEqTestErrorCallback {
- void OnInsertPacketError(int error_code,
- const NetEqInput::PacketData& packet) override;
- void OnGetAudioError(int error_code) override;
+ void OnInsertPacketError(const NetEqInput::PacketData& packet) override;
+ void OnGetAudioError() override;
};
class NetEqPostInsertPacket {