CodecOwner::SetEncoders: Return error code when given bad arguments
Instead of FATAL on a bad codec specification, log and return an error
code. This is a band-aid until callers are taught to only give it good
specifications.
BUG=webrtc:5033, chromium:526478
Review URL: https://codereview.webrtc.org/1364193002
Cr-Commit-Position: refs/heads/master@{#10066}
diff --git a/webrtc/modules/audio_coding/main/acm2/codec_manager.cc b/webrtc/modules/audio_coding/main/acm2/codec_manager.cc
index 39905ad..862feaa 100644
--- a/webrtc/modules/audio_coding/main/acm2/codec_manager.cc
+++ b/webrtc/modules/audio_coding/main/acm2/codec_manager.cc
@@ -281,9 +281,10 @@
// VAD/DTX not supported.
dtx_enabled_ = false;
}
- codec_owner_.SetEncoders(
- send_codec, dtx_enabled_ ? CngPayloadType(send_codec.plfreq) : -1,
- vad_mode_, red_enabled_ ? RedPayloadType(send_codec.plfreq) : -1);
+ if (!codec_owner_.SetEncoders(
+ send_codec, dtx_enabled_ ? CngPayloadType(send_codec.plfreq) : -1,
+ vad_mode_, red_enabled_ ? RedPayloadType(send_codec.plfreq) : -1))
+ return -1;
RTC_DCHECK(codec_owner_.Encoder());
codec_fec_enabled_ = codec_fec_enabled_ &&
@@ -297,9 +298,10 @@
if (send_codec_inst_.plfreq != send_codec.plfreq ||
send_codec_inst_.pacsize != send_codec.pacsize ||
send_codec_inst_.channels != send_codec.channels) {
- codec_owner_.SetEncoders(
- send_codec, dtx_enabled_ ? CngPayloadType(send_codec.plfreq) : -1,
- vad_mode_, red_enabled_ ? RedPayloadType(send_codec.plfreq) : -1);
+ if (!codec_owner_.SetEncoders(
+ send_codec, dtx_enabled_ ? CngPayloadType(send_codec.plfreq) : -1,
+ vad_mode_, red_enabled_ ? RedPayloadType(send_codec.plfreq) : -1))
+ return -1;
RTC_DCHECK(codec_owner_.Encoder());
}
send_codec_inst_.plfreq = send_codec.plfreq;
diff --git a/webrtc/modules/audio_coding/main/acm2/codec_owner.cc b/webrtc/modules/audio_coding/main/acm2/codec_owner.cc
index 1fb7026..4dde085 100644
--- a/webrtc/modules/audio_coding/main/acm2/codec_owner.cc
+++ b/webrtc/modules/audio_coding/main/acm2/codec_owner.cc
@@ -11,6 +11,7 @@
#include "webrtc/modules/audio_coding/main/acm2/codec_owner.h"
#include "webrtc/base/checks.h"
+#include "webrtc/base/logging.h"
#include "webrtc/engine_configurations.h"
#include "webrtc/modules/audio_coding/codecs/cng/include/audio_encoder_cng.h"
#include "webrtc/modules/audio_coding/codecs/g711/include/audio_encoder_pcm.h"
@@ -117,6 +118,8 @@
#endif
}
+// Returns a new speech encoder, or null on error.
+// TODO(kwiberg): Don't handle errors here (bug 5033)
rtc::scoped_ptr<AudioEncoder> CreateSpeechEncoder(
const CodecInst& speech_inst,
LockedIsacBandwidthInfo* bwinfo) {
@@ -135,7 +138,8 @@
} else if (IsG722(speech_inst)) {
return rtc_make_scoped_ptr(new AudioEncoderG722(speech_inst));
} else {
- FATAL() << "Could not create encoder of type " << speech_inst.plname;
+ LOG_F(LS_ERROR) << "Could not create encoder of type "
+ << speech_inst.plname;
return rtc::scoped_ptr<AudioEncoder>();
}
}
@@ -189,13 +193,16 @@
}
} // namespace
-void CodecOwner::SetEncoders(const CodecInst& speech_inst,
+bool CodecOwner::SetEncoders(const CodecInst& speech_inst,
int cng_payload_type,
ACMVADMode vad_mode,
int red_payload_type) {
speech_encoder_ = CreateSpeechEncoder(speech_inst, &isac_bandwidth_info_);
+ if (!speech_encoder_)
+ return false;
external_speech_encoder_ = nullptr;
ChangeCngAndRed(cng_payload_type, vad_mode, red_payload_type);
+ return true;
}
void CodecOwner::SetEncoders(AudioEncoder* external_speech_encoder,
diff --git a/webrtc/modules/audio_coding/main/acm2/codec_owner.h b/webrtc/modules/audio_coding/main/acm2/codec_owner.h
index ae7e452..3835c28 100644
--- a/webrtc/modules/audio_coding/main/acm2/codec_owner.h
+++ b/webrtc/modules/audio_coding/main/acm2/codec_owner.h
@@ -29,10 +29,12 @@
CodecOwner();
~CodecOwner();
- void SetEncoders(const CodecInst& speech_inst,
+ // Start using the specified encoder. Returns false on error.
+ // TODO(kwiberg): Don't handle errors here (bug 5033)
+ bool SetEncoders(const CodecInst& speech_inst,
int cng_payload_type,
ACMVADMode vad_mode,
- int red_payload_type);
+ int red_payload_type) WARN_UNUSED_RESULT;
void SetEncoders(AudioEncoder* external_speech_encoder,
int cng_payload_type,
diff --git a/webrtc/modules/audio_coding/main/acm2/codec_owner_unittest.cc b/webrtc/modules/audio_coding/main/acm2/codec_owner_unittest.cc
index 9c17073..6c23261 100644
--- a/webrtc/modules/audio_coding/main/acm2/codec_owner_unittest.cc
+++ b/webrtc/modules/audio_coding/main/acm2/codec_owner_unittest.cc
@@ -8,6 +8,8 @@
* be found in the AUTHORS file in the root of the source tree.
*/
+#include <cstring>
+
#include "testing/gtest/include/gtest/gtest.h"
#include "webrtc/base/arraysize.h"
#include "webrtc/base/safe_conversions.h"
@@ -34,7 +36,8 @@
CodecOwnerTest() : timestamp_(0) {}
void CreateCodec() {
- codec_owner_.SetEncoders(kDefaultCodecInst, kCngPt, VADNormal, -1);
+ ASSERT_TRUE(
+ codec_owner_.SetEncoders(kDefaultCodecInst, kCngPt, VADNormal, -1));
}
void EncodeAndVerify(size_t expected_out_length,
@@ -167,7 +170,7 @@
// Change to internal encoder.
CodecInst codec_inst = kDefaultCodecInst;
codec_inst.pacsize = kPacketSizeSamples;
- codec_owner_.SetEncoders(codec_inst, -1, VADNormal, -1);
+ ASSERT_TRUE(codec_owner_.SetEncoders(codec_inst, -1, VADNormal, -1));
// Don't expect any more calls to the external encoder.
info = codec_owner_.Encoder()->Encode(1, audio, arraysize(audio),
arraysize(encoded), encoded);
@@ -196,5 +199,12 @@
TestCngAndRedResetSpeechEncoder(false, false);
}
+TEST_F(CodecOwnerTest, SetEncodersError) {
+ CodecInst codec_inst = kDefaultCodecInst;
+ static const char bad_name[] = "Robert'); DROP TABLE Students;";
+ std::memcpy(codec_inst.plname, bad_name, sizeof bad_name);
+ EXPECT_FALSE(codec_owner_.SetEncoders(codec_inst, -1, VADNormal, -1));
+}
+
} // namespace acm2
} // namespace webrtc
