Send audio and video codecs to RTPPayloadRegistry

webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h

 /*
  *  Copyright (c) 2013 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.
  */
 
 #ifndef WEBRTC_MODULES_RTP_RTCP_INCLUDE_RTP_PAYLOAD_REGISTRY_H_
 #define WEBRTC_MODULES_RTP_RTCP_INCLUDE_RTP_PAYLOAD_REGISTRY_H_
 
 #include <map>
 #include <memory>
 #include <set>
 
 #include "webrtc/base/criticalsection.h"
 #include "webrtc/base/deprecation.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_receiver_strategy.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_utility.h"
 
 namespace webrtc {
 
+struct CodecInst;
+class VideoCodec;
+
 // This strategy deals with the audio/video-specific aspects
 // of payload handling.
 class RTPPayloadStrategy {
  public:
   virtual ~RTPPayloadStrategy() {}
 
   virtual bool CodecsMustBeUnique() const = 0;
 
   virtual bool PayloadIsCompatible(const RtpUtility::Payload& payload,
                                    uint32_t frequency,
@@ skipping 17 matching lines @@
  protected:
   RTPPayloadStrategy() {}
 };
 
 class RTPPayloadRegistry {
  public:
   // The registry takes ownership of the strategy.
   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;
 
   void SetRtxSsrc(uint32_t ssrc);
 
   bool GetRtxSsrc(uint32_t* ssrc) const;
 
   void SetRtxPayloadType(int payload_type, int associated_payload_type);
 
@@ skipping 49 matching lines @@
   }
 
   int8_t last_received_media_payload_type() const {
     rtc::CritScope cs(&crit_sect_);
     return last_received_media_payload_type_;
   }
 
   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,
       size_t payload_name_length,
       uint32_t frequency,
       size_t channels,
       uint32_t rate);
 
   bool IsRtxInternal(const RTPHeader& header) const;
 
@@ skipping 10 matching lines @@
   std::map<int, int> rtx_payload_type_map_;
   uint32_t ssrc_rtx_;
   // Only warn once per payload type, if an RTX packet is received but
   // no associated payload type found in |rtx_payload_type_map_|.
   std::set<int> payload_types_with_suppressed_warnings_ GUARDED_BY(crit_sect_);
 };
 
 }  // namespace webrtc
 
 #endif  // WEBRTC_MODULES_RTP_RTCP_INCLUDE_RTP_PAYLOAD_REGISTRY_H_