WebRtcVoiceEngine: Use AudioDecoderFactory to generate recv codecs.

webrtc/media/engine/webrtcvoiceengine.cc

Index: webrtc/media/engine/webrtcvoiceengine.cc
diff --git a/webrtc/media/engine/webrtcvoiceengine.cc b/webrtc/media/engine/webrtcvoiceengine.cc
index b2eda0ee135174722834f87bad595ee1dfb60aab..54c608193b9d600b68771e26e6c2a4119be40139 100644
--- a/webrtc/media/engine/webrtcvoiceengine.cc
+++ b/webrtc/media/engine/webrtcvoiceengine.cc
@@ -33,6 +33,7 @@
 #include "webrtc/media/base/audiosource.h"
 #include "webrtc/media/base/mediaconstants.h"
 #include "webrtc/media/base/streamparams.h"
+#include "webrtc/media/engine/payload_type_mapper.h"
 #include "webrtc/media/engine/webrtcmediaengine.h"
 #include "webrtc/media/engine/webrtcvoe.h"
 #include "webrtc/modules/audio_coding/acm2/rent_a_codec.h"
@@ -249,7 +250,7 @@ class WebRtcVoiceCodecs final {
  public:
   // TODO(solenberg): Do this filtering once off-line, add a simple AudioCodec
   // list and add a test which verifies VoE supports the listed codecs.
-  static std::vector<AudioCodec> SupportedCodecs() {
+  static std::vector<AudioCodec> SupportedSendCodecs() {
     std::vector<AudioCodec> result;
     // Iterate first over our preferred codecs list, so that the results are
     // added in order of preference.
@@ -512,13 +513,20 @@ WebRtcVoiceEngine::WebRtcVoiceEngine(
   RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
   LOG(LS_INFO) << "WebRtcVoiceEngine::WebRtcVoiceEngine";
   RTC_DCHECK(voe_wrapper);
+  RTC_DCHECK(decoder_factory);
 
   signal_thread_checker_.DetachFromThread();
 
   // Load our audio codec list.
-  LOG(LS_INFO) << "Supported codecs in order of preference:";
-  codecs_ = WebRtcVoiceCodecs::SupportedCodecs();
-  for (const AudioCodec& codec : codecs_) {
+  LOG(LS_INFO) << "Supported send codecs in order of preference:";
+  send_codecs_ = WebRtcVoiceCodecs::SupportedSendCodecs();
+  for (const AudioCodec& codec : send_codecs_) {
+    LOG(LS_INFO) << ToString(codec);
+  }
+
+  LOG(LS_INFO) << "Supported recv codecs in order of preference:";
+  recv_codecs_ = CollectRecvCodecs();

[ivoc, 2016/07/05 14:43:50]

So why can't this be part of WebRtcVoiceCodecs like the SupportedSendCodecs
function?

[ossu, 2016/07/05 15:31:56]

WebRtcVoiceCodecs uses hard-coded information to generate the set of codecs to
use. Once we get the encoder side of this reworking in as well,
WebRtcVoiceCodecs should go away and be replaced by a similar method to this
one: CollectSendCodecs().

[ivoc, 2016/07/06 15:24:50]

Sorry, I'm probably missing the big picture here :-)

[ossu, 2016/07/06 16:23:25]

No worries. :)

+  for (const AudioCodec& codec : recv_codecs_) {
     LOG(LS_INFO) << ToString(codec);
   }
 
@@ -906,12 +914,12 @@ int WebRtcVoiceEngine::GetInputLevel() {
 
 const std::vector<AudioCodec>& WebRtcVoiceEngine::send_codecs() const {
   RTC_DCHECK(signal_thread_checker_.CalledOnValidThread());
-  return codecs_;
+  return send_codecs_;
 }
 
 const std::vector<AudioCodec>& WebRtcVoiceEngine::recv_codecs() const {
   RTC_DCHECK(signal_thread_checker_.CalledOnValidThread());
-  return codecs_;
+  return recv_codecs_;
 }
 
 RtpCapabilities WebRtcVoiceEngine::GetCapabilities() const {
@@ -1072,6 +1080,64 @@ webrtc::AudioDeviceModule* WebRtcVoiceEngine::adm() {
   return adm_;
 }
 
+AudioCodecs WebRtcVoiceEngine::CollectRecvCodecs() const {
+  PayloadTypeMapper mapper;
+  AudioCodecs out;
+  std::vector<webrtc::SdpAudioFormat> formats =
+      decoder_factory_->GetSupportedFormats();
+  std::map<int, bool> generate_cn = {{  8000, false },
+                                     { 16000, false },
+                                     { 32000, false }};
+
+  auto map_format = [&mapper, &out] (const webrtc::SdpAudioFormat& format) {

[ivoc, 2016/07/05 14:43:50]

I don't really see the point of this lambda, and I think it can be eliminated
since it does almost the same thing as the loop below.

You can add {kDtmfCodecName, 8000, 1} to |formats| (eliminates the call on
1136), and on line 1120 you can add a check to test if the bool is currently
false, and if so add {kCnCodecName, cn.first, 1} to |out| (eliminates the loop
starting on 1129). 

[ossu, 2016/07/05 15:31:56]

Well, I'd like to use it for all calls to mapper.ToAudioCodec and I believe I
can do so with a change to the loop below. I'd prefer not to change |formats| in
this function. It's basically the input and could (should?) probably be const&.
Is the use of a lambda in itself off-putting to you?

You're right, though, I could add comfort noise codecs directly in the inner
loop. If I were to imagine a reason for not doing so, it would be to keep the
"proper" codecs grouped together. Maybe if we run out of payload types, it's
better if the CN codecs get left out - not that that should ever happen. I'll
give it a think! :)

[ivoc, 2016/07/06 15:24:51]

It's not that I mind lambda's (I kinda like them), but more that the code in the
lambda looks pretty similar to the loop below, so I thought it would be nice to
get rid of some duplication if possible. Would it be possible to get rid of the
loop and put the functionality in map_format instead? But maybe I'm wrong and
it's just a bad idea.

[ossu, 2016/07/06 16:23:25]

Well, the lambda deals with the possibility that the mapping may fail when
creating a mapping for a single format, whereas the loop maps all formats. Once
the infrastructure is in place, I think we could put the transport-cc feedback
parameter on all codecs as with a single rtcp-fb line using a wildcard.

[ivoc, 2016/07/08 12:50:32]

It looks to me like the only difference between the lambda and what happens in
each iteration of the loop is lines 1106-1111 and lines 1114-1122. I think it
would be safe to add those lines to the lambda, and instead of the loop just
call the lambda for each element in the vector. There would be no duplicated
code that way, WDYT?

[ossu, 2016/07/08 14:39:14]

Yeah, I guess you're right. With the check for Opus in it, it's fine if we send
in other types of codecs as well. It just won't apply to CN or DTMF either.

+    rtc::Optional<AudioCodec> opt_codec = mapper.ToAudioCodec(format);
+    if (opt_codec) {
+      out.push_back(*opt_codec);
+    } else {
+      LOG(LS_ERROR) << "Unable to assign payload type to format: " << format;
+    }
+  };
+
+  for (const auto& format : formats) {
+    auto opt_codec = mapper.ToAudioCodec(format);

[ossu, 2016/06/16 16:21:24]

Although we use the mapper here to assign payload types, it doesn't need to get
it completely right. If there are any overlaps with, for example, video payload
types, that will get resolved in mediasession.cc. I've tried to make
PayloadTypeMapper return the same payload types as we'd otherwise use, and not
step on any types we already use somewhere else, so as to not trigger some
unforeseen change in payload types that may (it really shouldn't) break
something somewhere.

+    if (opt_codec) {
+      auto& codec = *opt_codec;
+
+      if (IsCodec(codec, kOpusCodecName)) {
+        // TODO(ossu): Should we set this specifically for just this codec?

[ossu, 2016/07/05 15:31:56]

I've just gotten feedback (no pun intended) that it should be fine to add this
for every codec, so I'll rewrite this bit to do just that.

[ossu, 2016/07/06 13:20:15]

Turns out that, no, I still can't get this to use map_format cleanly, and I'd
also rather avoid functionality changes in this CL. It'll come in as a different
change, later.

+        // If not, this can be replaced with a call to map_format.
+        codec.AddFeedbackParam(
+            FeedbackParam(kRtcpFbParamTransportCc, kParamValueEmpty));
+      }
+      out.push_back(codec);
+
+      // TODO(ossu): We should get more than just a format from the factory, so
+      // we

[ivoc, 2016/07/05 14:43:50]

Looks like a strange linebreak here, please move "we" to the previous or next
line.

[ossu, 2016/07/05 15:31:56]

Yes, something went very wrong here.

+      // can determine if a format should be used with CN or not. For now,
+      // generate a CN entry for each supported clock rate also used by a format
+      // supported by the factory.
+      auto cn_entry = generate_cn.find(format.clockrate_hz);
+      if (cn_entry != generate_cn.end() /* && format.allow_comfort_noise */) {

[ivoc, 2016/07/05 14:43:50]

Remove commented code please.

[ossu, 2016/07/05 15:31:56]

No, I'd rather not. It's directly related to the TODO above. Once we have this
augmented information from the factory (it'll probably be a struct with a flag
and an SdpAudioFormat in it) this is where that check'll be added.

[ivoc, 2016/07/06 15:24:51]

Hmm, somehow I was sure this was in the style guide, but I just tried to look
for it and couldn't find it. I guess you can keep your commented code then. :-)

[ossu, 2016/07/06 16:23:25]

Well, I agree with you that we shouldn't keep commented code laying around. I
feel this is special since it documents the change in the TODO, it's not just
something that we've commented out because we don't want it anymore.

+        cn_entry->second = true;
+      }
+    } else {
+      LOG(LS_ERROR) << "Unable to assign payload type to format: " << format;
+    }
+  }
+
+  // Add CN codecs
+  for (const auto& cn : generate_cn) {
+    if (cn.second) {
+      map_format({kCnCodecName, cn.first, 1});
+    }
+  }
+
+  // Add telephone-event codec
+  map_format({kDtmfCodecName, 8000, 1});
+
+  return out;
+}
+
 class WebRtcVoiceMediaChannel::WebRtcAudioSendStream
     : public AudioSource::Sink {
  public: