Send audio and video codecs to RTPPayloadRegistry

webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h

Index: webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h
diff --git a/webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h b/webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h
index fd228e1f168b15c49b1e0e1512e3d91e0621a1bb..8f9d5829f9cc066c0416d991994205af4d38fe24 100644
--- a/webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h
+++ b/webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h
@@ -22,6 +22,9 @@
 
 namespace webrtc {
 
+struct CodecInst;
+class VideoCodec;
+
 // This strategy deals with the audio/video-specific aspects
 // of payload handling.
 class RTPPayloadStrategy {
@@ -59,19 +62,18 @@ class RTPPayloadRegistry {
   explicit RTPPayloadRegistry(RTPPayloadStrategy* rtp_payload_strategy);
   ~RTPPayloadRegistry();
 
-  int32_t RegisterReceivePayload(const char* payload_name,
-                                 int8_t payload_type,
-                                 uint32_t frequency,
-                                 size_t channels,
-                                 uint32_t rate,
+  // TODO(magjed): Split RTPPayloadRegistry into separate Audio and Video class
+  // and remove RTPPayloadStrategy, RTPPayloadVideoStrategy,
+  // RTPPayloadAudioStrategy, and simplify the code. http://crbug/webrtc/6743.
+  int32_t RegisterReceivePayload(const CodecInst& audio_codec,
                                  bool* created_new_payload_type);
+  int32_t RegisterReceivePayload(const VideoCodec& video_codec);

[the sun, 2016/11/23 19:29:35]

While I do agree that this CL is an improvement over the previous state, I don't
really like that media-specific types are seeping down the way it is here. What
audio/video specific knowledge does RTPPayloadRegistry really need to have? Can
we use a generic type in both cases and have that knowledge somewhere else?

[magjed_webrtc, 2016/11/24 09:39:40]

The codec knowledge RTPPayloadRegistry needs to have is:
Audio: payload name, frequency, channels, rate (same as the arguments we have
now).
Video: payload name + profile.
We currently only have payload name for video. So when we register more than one
H264 video codec (typically Constrained Baseline and Constrained High profile),
and later call ReceivePayloadType to look up the payload type and only have
"H264" as information, we will get the wrong payload type back. Audio codecs
have the same ambiguity with payload names and I guess that's why we already
pass in frequency, channels, and rate. I see adding video profile information as
analogous to the audio specific information.
Yeah, maybe. We already have this generic RtpUtility::Payload tagged union.
Maybe we could send in RtpUtility::Payload as argument and have a function 'bool
IsPayloadCompatible(const RtpUtility::Payload&, const RtpUtility::Payload&)'. I
don't really like the tagged union approach though. The only reason we use it in
RTPPayloadRegistry is because we are trying to be audio/video generic. In
reality, RTPPayloadRegistry is either used for audio codecs only (from
webrtc/voice_engine/channel.cc) or for video codecs only (from
webrtc/video/rtp_stream_receiver.cc). That's why I think it would be better to
subclass RTPPayloadRegistry; one for audio and one for video. The common code
would be kept in the RTPPayloadRegistry base class and the audio/video specific
logic would go in the subclasses. webrtc/voice_engine/channel.cc would use the
audio subclass directly and webrtc/video/rtp_stream_receiver.cc would use the
video subclass directly. That way we can get rid of RTPPayloadStrategy and the
unions.

Another approach would be to make RTPPayloadRegistry a template
template<typename T>
RTPPayloadRegistry;
and require the existence of IsPayloadCompatible(const T&, const T&). That might
be problematic, because there is some audio/video specific logic not related to
the payload type, e.g. the DeregisterAudioCodecOrRedTypeRegardlessOfPayloadType
function.

Regardless, I think this CL and the dependent CL
https://codereview.webrtc.org/2524923002/ is an improvement over the existing
code, and it will unblock H264 High profile in the near future. Maybe we can
file an issue and discuss how RTPPayloadRegistry should be designed in the long
run?

 
   int32_t DeRegisterReceivePayload(int8_t payload_type);
 
-  int32_t ReceivePayloadType(const char* payload_name,
-                             uint32_t frequency,
-                             size_t channels,
-                             uint32_t rate,
+  int32_t ReceivePayloadType(const CodecInst& audio_codec,
+                             int8_t* payload_type) const;
+  int32_t ReceivePayloadType(const VideoCodec& video_codec,
                              int8_t* payload_type) const;
 
   bool RtxEnabled() const;
@@ -141,6 +143,19 @@ class RTPPayloadRegistry {
   RTC_DEPRECATED void set_use_rtx_payload_mapping_on_restore(bool val) {}
 
  private:
+  int32_t RegisterReceivePayload(const char* payload_name,
+                                 int8_t payload_type,
+                                 uint32_t frequency,
+                                 size_t channels,
+                                 uint32_t rate,
+                                 bool* created_new_payload_type);
+
+  int32_t ReceivePayloadType(const char* payload_name,
+                             uint32_t frequency,
+                             size_t channels,
+                             uint32_t rate,
+                             int8_t* payload_type) const;
+
   // Prunes the payload type map of the specific payload type, if it exists.
   void DeregisterAudioCodecOrRedTypeRegardlessOfPayloadType(
       const char* payload_name,