Add RTX codecs for codecs only supported by external encoder.

webrtc/media/engine/webrtcvideoengine2.cc

 /*
  *  Copyright (c) 2014 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 604 matching lines @@
     if (i != codecs.size() - 1) {
       out << ", ";
     }
     // Don't add internally-supported codecs twice.
     if (CodecIsInternallySupported(codecs[i].name)) {
       continue;
     }
 
     // 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);
     VideoCodec codec(static_cast<int>(payload_type), codecs[i].name,
                      codecs[i].max_width, codecs[i].max_height,
                      codecs[i].max_fps);
 
     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);
   LOG(LS_INFO) << "Codecs supported by the external encoder factory: "
                << out.str();
   return supported_codecs;
 }
 
 WebRtcVideoChannel2::WebRtcVideoChannel2(
     webrtc::Call* call,
@@ skipping 1984 matching lines @@
         rtx_mapping[video_codecs[i].codec.id] !=
             fec_settings.red_payload_type) {
       video_codecs[i].rtx_payload_type = rtx_mapping[video_codecs[i].codec.id];
     }
   }
 
   return video_codecs;
 }
 
 }  // namespace cricket