Add RTX codecs for codecs only supported by external encoder.

webrtc/media/engine/webrtcvideoengine2.cc

Index: webrtc/media/engine/webrtcvideoengine2.cc
diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc
index 005cb68f9b2477a13d15d688e544000e8ef5bdd3..82e3ea001687eeaf959e499f370575aecd8eba25 100644
--- a/webrtc/media/engine/webrtcvideoengine2.cc
+++ b/webrtc/media/engine/webrtcvideoengine2.cc
@@ -622,6 +622,12 @@ std::vector<VideoCodec> WebRtcVideoEngine2::GetSupportedCodecs() const {
 
     // External video encoders are given payloads 120-127. This also means that
     // we only support up to 8 external payload types.
+    // TODO(deadbeef): mediasession.cc already has code to dynamically
+    // determine a payload type. We should be able to just leave the payload
+    // type empty and let mediasession determine it. However, currently RTX
+    // codecs are associated to codecs by payload type, meaning we DO need
+    // to allocate unique payload types here. So to make this change we would
+    // need to make RTX codecs associated by name instead.

[pthatcher1, 2016/06/23 21:26:30]

Can we at least leave the RTX codecs with an unset payload type since nothing
refers to them?  That would make it more clear what's going on (a
kPayloadTypeUnset or rtc::optional).

And should we just make all calls to CreateRtxCodec in this class always have
such an unset payload type?  Are they not used in those other areas either?

[Taylor Brandstetter, 2016/06/23 23:05:49]

I agree about using rtc::Optional. But I'd rather just do all the refactoring at
once, rather than in stages (stage 1 being RTX codecs, stage 2 being everything
else). And I'd rather do it in a separate CL from this one.

And to answer your question "Are they not used in those other areas either?":
They are definitely used.

     const int kExternalVideoPayloadTypeBase = 120;
     size_t payload_type = kExternalVideoPayloadTypeBase + i;
     RTC_DCHECK(payload_type < 128);
@@ -631,6 +637,11 @@ std::vector<VideoCodec> WebRtcVideoEngine2::GetSupportedCodecs() const {
 
     AddDefaultFeedbackParams(&codec);
     supported_codecs.push_back(codec);
+    // Even if we add multiple RTX codecs with the same payload type (96),
+    // their payload types should be automatically re-assigned when creating
+    // the offer.
+    supported_codecs.push_back(
+        VideoCodec::CreateRtxCodec(kDynamicPayloadTypeMin, payload_type));

[pbos-webrtc, 2016/06/26 21:55:58]

Can we apply this for all "real" video codecs outside this function (regardless
of internal/external)? All VideoCodec::CODEC_VIDEO should have a RTX codec
created, right?

[Taylor Brandstetter, 2016/06/27 17:46:44]

You mean, just create all of the RTX codecs here, instead of in
DefaultVideoCodecList()? I'd like to, but again I'm worried about something
breaking if I muck with existing payload types, so I planned to do it in a
separate CL. In that case, maybe in this CL I should do "if H.264, use H.264 rtx
payload type, if VP8 use VP8 RTX payload type" etc.?

   }
   LOG(LS_INFO) << "Supported codecs (incl. external codecs): "
                << CodecVectorToString(supported_codecs);