From f14dfed72a707cba6fbf75d25abf3c0e3423b33f Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Mon, 18 Sep 2023 14:20:34 +0200 Subject: [PATCH] Move codecs() to MediaContentDescription allowing for a lot of de-templating BUG=webrtc:15214 Change-Id: Ibe1a5f5d704564566f24c496822a4308ba23c4dd Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/319160 Commit-Queue: Philipp Hancke Reviewed-by: Florent Castelli Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#40774} --- pc/sdp_offer_answer.cc | 15 +--- pc/session_description.h | 88 ++++++++++---------- pc/webrtc_sdp.cc | 171 +++++++++++++++------------------------ 3 files changed, 108 insertions(+), 166 deletions(-) diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index ed2dfca849..fe7093d604 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -438,18 +438,9 @@ RTCError ValidateBundledPayloadTypes( continue; } const auto type = media_description->type(); - if (type == cricket::MEDIA_TYPE_AUDIO) { - RTC_DCHECK(media_description->as_audio()); - for (const auto& c : media_description->as_audio()->codecs()) { - auto error = FindDuplicateCodecParameters( - c.ToCodecParameters(), payload_to_codec_parameters); - if (!error.ok()) { - return error; - } - } - } else if (type == cricket::MEDIA_TYPE_VIDEO) { - RTC_DCHECK(media_description->as_video()); - for (const auto& c : media_description->as_video()->codecs()) { + if (type == cricket::MEDIA_TYPE_AUDIO || + type == cricket::MEDIA_TYPE_VIDEO) { + for (const auto& c : media_description->codecs()) { auto error = FindDuplicateCodecParameters( c.ToCodecParameters(), payload_to_codec_parameters); if (!error.ok()) { diff --git a/pc/session_description.h b/pc/session_description.h index 31992be083..52e891e433 100644 --- a/pc/session_description.h +++ b/pc/session_description.h @@ -83,8 +83,6 @@ class MediaContentDescription { return nullptr; } - virtual bool has_codecs() const = 0; - // Copy operator that returns an unique_ptr. // Not a virtual function. // If a type-specific variant of Clone() is desired, override it, or @@ -234,54 +232,14 @@ class MediaContentDescription { receive_rids_ = rids; } - protected: - bool rtcp_mux_ = false; - bool rtcp_reduced_size_ = false; - bool remote_estimate_ = false; - int bandwidth_ = kAutoBandwidth; - std::string bandwidth_type_ = kApplicationSpecificBandwidth; - std::string protocol_; - std::vector cryptos_; - std::vector rtp_header_extensions_; - bool rtp_header_extensions_set_ = false; - StreamParamsVec send_streams_; - bool conference_mode_ = false; - webrtc::RtpTransceiverDirection direction_ = - webrtc::RtpTransceiverDirection::kSendRecv; - rtc::SocketAddress connection_address_; - ExtmapAllowMixed extmap_allow_mixed_enum_ = kMedia; - - SimulcastDescription simulcast_; - std::vector receive_rids_; - - private: - // Copy function that returns a raw pointer. Caller will assert ownership. - // Should only be called by the Clone() function. Must be implemented - // by each final subclass. - virtual MediaContentDescription* CloneInternal() const = 0; -}; - -template -class MediaContentDescriptionImpl : public MediaContentDescription { - public: - void set_protocol(absl::string_view protocol) override { - RTC_DCHECK(IsRtpProtocol(protocol)); - protocol_ = std::string(protocol); - } - // Codecs should be in preference order (most preferred codec first). const std::vector& codecs() const { return codecs_; } void set_codecs(const std::vector& codecs) { codecs_ = codecs; } - bool has_codecs() const override { return !codecs_.empty(); } + virtual bool has_codecs() const { return !codecs_.empty(); } bool HasCodec(int id) { - bool found = false; - for (auto it = codecs_.begin(); it != codecs_.end(); ++it) { - if (it->id == id) { - found = true; - break; - } - } - return found; + return absl::c_find_if(codecs_, [id](const cricket::Codec codec) { + return codec.id == id; + }) != codecs_.end(); } void AddCodec(const Codec& codec) { codecs_.push_back(codec); } void AddOrReplaceCodec(const Codec& codec) { @@ -299,10 +257,48 @@ class MediaContentDescriptionImpl : public MediaContentDescription { } } + protected: + // TODO(bugs.webrtc.org/15214): move all RTP related things to a subclass that + // the SCTP content description does not inherit from. + std::string protocol_; + private: + bool rtcp_mux_ = false; + bool rtcp_reduced_size_ = false; + bool remote_estimate_ = false; + int bandwidth_ = kAutoBandwidth; + std::string bandwidth_type_ = kApplicationSpecificBandwidth; + + std::vector cryptos_; + std::vector rtp_header_extensions_; + bool rtp_header_extensions_set_ = false; + StreamParamsVec send_streams_; + bool conference_mode_ = false; + webrtc::RtpTransceiverDirection direction_ = + webrtc::RtpTransceiverDirection::kSendRecv; + rtc::SocketAddress connection_address_; + ExtmapAllowMixed extmap_allow_mixed_enum_ = kMedia; + + SimulcastDescription simulcast_; + std::vector receive_rids_; + + // Copy function that returns a raw pointer. Caller will assert ownership. + // Should only be called by the Clone() function. Must be implemented + // by each final subclass. + virtual MediaContentDescription* CloneInternal() const = 0; + std::vector codecs_; }; +template +class MediaContentDescriptionImpl : public MediaContentDescription { + public: + void set_protocol(absl::string_view protocol) override { + RTC_DCHECK(IsRtpProtocol(protocol)); + protocol_ = std::string(protocol); + } +}; + class AudioContentDescription : public MediaContentDescriptionImpl { public: AudioContentDescription() {} diff --git a/pc/webrtc_sdp.cc b/pc/webrtc_sdp.cc index 9ed8ca6eaf..80fef12702 100644 --- a/pc/webrtc_sdp.cc +++ b/pc/webrtc_sdp.cc @@ -1438,17 +1438,11 @@ void BuildMediaLine(const cricket::MediaType media_type, // fmt is a list of payload type numbers that MAY be used in the session. std::string type; std::string fmt; - if (media_type == cricket::MEDIA_TYPE_VIDEO) { - type = kMediaTypeVideo; - const VideoContentDescription* video_desc = media_desc->as_video(); - for (const cricket::VideoCodec& codec : video_desc->codecs()) { - fmt.append(" "); - fmt.append(rtc::ToString(codec.id)); - } - } else if (media_type == cricket::MEDIA_TYPE_AUDIO) { - type = kMediaTypeAudio; - const AudioContentDescription* audio_desc = media_desc->as_audio(); - for (const cricket::AudioCodec& codec : audio_desc->codecs()) { + if (media_type == cricket::MEDIA_TYPE_AUDIO || + media_type == cricket::MEDIA_TYPE_VIDEO) { + type = media_type == cricket::MEDIA_TYPE_AUDIO ? kMediaTypeAudio + : kMediaTypeVideo; + for (const cricket::VideoCodec& codec : media_desc->codecs()) { fmt.append(" "); fmt.append(rtc::ToString(codec.id)); } @@ -1867,8 +1861,7 @@ bool WriteFmtpParameters(const cricket::CodecParameterMap& parameters, return !empty; } -template -void AddFmtpLine(const T& codec, std::string* message) { +void AddFmtpLine(const cricket::Codec& codec, std::string* message) { rtc::StringBuilder os; WriteFmtpHeader(codec.id, &os); os << kSdpDelimiterSpace; @@ -1879,8 +1872,7 @@ void AddFmtpLine(const T& codec, std::string* message) { return; } -template -void AddPacketizationLine(const T& codec, std::string* message) { +void AddPacketizationLine(const cricket::Codec& codec, std::string* message) { if (!codec.packetization) { return; } @@ -1890,8 +1882,7 @@ void AddPacketizationLine(const T& codec, std::string* message) { AddLine(os.str(), message); } -template -void AddRtcpFbLines(const T& codec, std::string* message) { +void AddRtcpFbLines(const cricket::Codec& codec, std::string* message) { for (const cricket::FeedbackParam& param : codec.feedback_params.params()) { rtc::StringBuilder os; WriteRtcpFbHeader(codec.id, &os); @@ -1932,7 +1923,7 @@ void BuildRtpmap(const MediaContentDescription* media_desc, RTC_DCHECK(media_desc != NULL); rtc::StringBuilder os; if (media_type == cricket::MEDIA_TYPE_VIDEO) { - for (const cricket::VideoCodec& codec : media_desc->as_video()->codecs()) { + for (const cricket::Codec& codec : media_desc->codecs()) { // RFC 4566 // a=rtpmap: / // [/] @@ -1950,7 +1941,7 @@ void BuildRtpmap(const MediaContentDescription* media_desc, std::vector ptimes; std::vector maxptimes; int max_minptime = 0; - for (const cricket::AudioCodec& codec : media_desc->as_audio()->codecs()) { + for (const cricket::Codec& codec : media_desc->codecs()) { RTC_DCHECK(!codec.name.empty()); // RFC 4566 // a=rtpmap: / @@ -2857,21 +2848,6 @@ bool ParseMediaDescription( return true; } -bool VerifyCodec(const cricket::Codec& codec) { - // Codec has not been populated correctly unless the name has been set. This - // can happen if an SDP has an fmtp or rtcp-fb with a payload type but doesn't - // have a corresponding "rtpmap" line. - return !codec.name.empty(); -} - -bool VerifyAudioCodecs(const AudioContentDescription* audio_desc) { - return absl::c_all_of(audio_desc->codecs(), &VerifyCodec); -} - -bool VerifyVideoCodecs(const VideoContentDescription* video_desc) { - return absl::c_all_of(video_desc->codecs(), &VerifyCodec); -} - void AddParameters(const cricket::CodecParameterMap& parameters, cricket::Codec* codec) { for (const auto& entry : parameters) { @@ -2896,11 +2872,11 @@ void AddFeedbackParameters(const cricket::FeedbackParams& feedback_params, // Gets the current codec setting associated with `payload_type`. If there // is no Codec associated with that payload type it returns an empty codec // with that payload type. -template -T GetCodecWithPayloadType(cricket::MediaType type, - const std::vector& codecs, - int payload_type) { - const T* codec = FindCodecById(codecs, payload_type); +cricket::Codec GetCodecWithPayloadType( + cricket::MediaType type, + const std::vector& codecs, + int payload_type) { + const cricket::Codec* codec = FindCodecById(codecs, payload_type); if (codec) return *codec; // Return empty codec with `payload_type`. @@ -2912,12 +2888,11 @@ T GetCodecWithPayloadType(cricket::MediaType type, } // Updates or creates a new codec entry in the media description. -template -void AddOrReplaceCodec(MediaContentDescription* content_desc, const U& codec) { - T* desc = static_cast(content_desc); - std::vector codecs = desc->codecs(); +void AddOrReplaceCodec(MediaContentDescription* content_desc, + const cricket::Codec& codec) { + std::vector codecs = content_desc->codecs(); bool found = false; - for (U& existing_codec : codecs) { + for (cricket::Codec& existing_codec : codecs) { if (codec.id == existing_codec.id) { // Overwrite existing codec with the new codec. existing_codec = codec; @@ -2926,38 +2901,34 @@ void AddOrReplaceCodec(MediaContentDescription* content_desc, const U& codec) { } } if (!found) { - desc->AddCodec(codec); + content_desc->AddCodec(codec); return; } - desc->set_codecs(codecs); + content_desc->set_codecs(codecs); } // Adds or updates existing codec corresponding to `payload_type` according // to `parameters`. -template void UpdateCodec(MediaContentDescription* content_desc, int payload_type, const cricket::CodecParameterMap& parameters) { // Codec might already have been populated (from rtpmap). - U new_codec = GetCodecWithPayloadType(content_desc->type(), - static_cast(content_desc)->codecs(), - payload_type); + cricket::Codec new_codec = GetCodecWithPayloadType( + content_desc->type(), content_desc->codecs(), payload_type); AddParameters(parameters, &new_codec); - AddOrReplaceCodec(content_desc, new_codec); + AddOrReplaceCodec(content_desc, new_codec); } // Adds or updates existing codec corresponding to `payload_type` according // to `feedback_param`. -template void UpdateCodec(MediaContentDescription* content_desc, int payload_type, const cricket::FeedbackParam& feedback_param) { // Codec might already have been populated (from rtpmap). - U new_codec = GetCodecWithPayloadType(content_desc->type(), - static_cast(content_desc)->codecs(), - payload_type); + cricket::Codec new_codec = GetCodecWithPayloadType( + content_desc->type(), content_desc->codecs(), payload_type); AddFeedbackParameter(feedback_param, &new_codec); - AddOrReplaceCodec(content_desc, new_codec); + AddOrReplaceCodec(content_desc, new_codec); } // Adds or updates existing video codec corresponding to `payload_type` @@ -2974,15 +2945,15 @@ void UpdateVideoCodecPacketization(VideoContentDescription* video_desc, cricket::VideoCodec codec = GetCodecWithPayloadType( video_desc->type(), video_desc->codecs(), payload_type); codec.packetization = std::string(packetization); - AddOrReplaceCodec(video_desc, - codec); + AddOrReplaceCodec(video_desc, codec); } -template -absl::optional PopWildcardCodec(std::vector* codecs) { +absl::optional PopWildcardCodec( + std::vector* codecs) { + RTC_DCHECK(codecs); for (auto iter = codecs->begin(); iter != codecs->end(); ++iter) { if (iter->id == kWildcardPayloadType) { - T wildcard_codec = *iter; + cricket::Codec wildcard_codec = *iter; codecs->erase(iter); return wildcard_codec; } @@ -2990,10 +2961,10 @@ absl::optional PopWildcardCodec(std::vector* codecs) { return absl::nullopt; } -template -void UpdateFromWildcardCodecs(cricket::MediaContentDescriptionImpl* desc) { +void UpdateFromWildcardCodecs(cricket::MediaContentDescription* desc) { + RTC_DCHECK(desc); auto codecs = desc->codecs(); - absl::optional wildcard_codec = PopWildcardCodec(&codecs); + absl::optional wildcard_codec = PopWildcardCodec(&codecs); if (!wildcard_codec) { return; } @@ -3006,6 +2977,7 @@ void UpdateFromWildcardCodecs(cricket::MediaContentDescriptionImpl* desc) { void AddAudioAttribute(const std::string& name, absl::string_view value, AudioContentDescription* audio_desc) { + RTC_DCHECK(audio_desc); if (value.empty()) { return; } @@ -3415,27 +3387,20 @@ bool ParseContent(absl::string_view message, media_desc->AddStream(track); } - if (media_type == cricket::MEDIA_TYPE_AUDIO) { - AudioContentDescription* audio_desc = media_desc->as_audio(); - UpdateFromWildcardCodecs(audio_desc); - - // Verify audio codec ensures that no audio codec has been populated with - // only fmtp. - if (!VerifyAudioCodecs(audio_desc)) { - return ParseFailed("Failed to parse audio codecs correctly.", error); - } - AddAudioAttribute(kCodecParamMaxPTime, maxptime_as_string, audio_desc); - AddAudioAttribute(kCodecParamPTime, ptime_as_string, audio_desc); + UpdateFromWildcardCodecs(media_desc); + // Codec has not been populated correctly unless the name has been set. This + // can happen if an SDP has an fmtp or rtcp-fb with a payload type but doesn't + // have a corresponding "rtpmap" line. This should lead to a parse error. + if (!absl::c_all_of(media_desc->codecs(), [](const cricket::Codec codec) { + return !codec.name.empty(); + })) { + return ParseFailed("Failed to parse codecs correctly.", error); } - - if (media_type == cricket::MEDIA_TYPE_VIDEO) { - VideoContentDescription* video_desc = media_desc->as_video(); - UpdateFromWildcardCodecs(video_desc); - // Verify video codec ensures that no video codec has been populated with - // only rtcp-fb. - if (!VerifyVideoCodecs(video_desc)) { - return ParseFailed("Failed to parse video codecs correctly.", error); - } + if (media_type == cricket::MEDIA_TYPE_AUDIO) { + AddAudioAttribute(kCodecParamMaxPTime, maxptime_as_string, + media_desc->as_audio()); + AddAudioAttribute(kCodecParamPTime, ptime_as_string, + media_desc->as_audio()); } // RFC 5245 @@ -3601,14 +3566,13 @@ void UpdateCodec(int payload_type, AudioContentDescription* audio_desc) { // Codec may already be populated with (only) optional parameters // (from an fmtp). - cricket::AudioCodec codec = GetCodecWithPayloadType( + cricket::Codec codec = GetCodecWithPayloadType( audio_desc->type(), audio_desc->codecs(), payload_type); codec.name = std::string(name); codec.clockrate = clockrate; codec.bitrate = bitrate; codec.channels = channels; - AddOrReplaceCodec(audio_desc, - codec); + AddOrReplaceCodec(audio_desc, codec); } // Updates or creates a new codec entry in the video description according to @@ -3618,11 +3582,10 @@ void UpdateCodec(int payload_type, VideoContentDescription* video_desc) { // Codec may already be populated with (only) optional parameters // (from an fmtp). - cricket::VideoCodec codec = GetCodecWithPayloadType( + cricket::Codec codec = GetCodecWithPayloadType( video_desc->type(), video_desc->codecs(), payload_type); codec.name = std::string(name); - AddOrReplaceCodec(video_desc, - codec); + AddOrReplaceCodec(video_desc, codec); } bool ParseRtpmapAttribute(absl::string_view line, @@ -3671,8 +3634,7 @@ bool ParseRtpmapAttribute(absl::string_view line, } if (media_type == cricket::MEDIA_TYPE_VIDEO) { - VideoContentDescription* video_desc = media_desc->as_video(); - for (const cricket::VideoCodec& existing_codec : video_desc->codecs()) { + for (const cricket::VideoCodec& existing_codec : media_desc->codecs()) { if (!existing_codec.name.empty() && payload_type == existing_codec.id && (!absl::EqualsIgnoreCase(encoding_name, existing_codec.name) || clock_rate != existing_codec.clockrate)) { @@ -3686,7 +3648,7 @@ bool ParseRtpmapAttribute(absl::string_view line, return ParseFailed(line, description.Release(), error); } } - UpdateCodec(payload_type, encoding_name, video_desc); + UpdateCodec(payload_type, encoding_name, media_desc->as_video()); } else if (media_type == cricket::MEDIA_TYPE_AUDIO) { // RFC 4566 // For audio streams, indicates the number @@ -3703,8 +3665,7 @@ bool ParseRtpmapAttribute(absl::string_view line, return ParseFailed(line, "At most 24 channels are supported.", error); } - AudioContentDescription* audio_desc = media_desc->as_audio(); - for (const cricket::AudioCodec& existing_codec : audio_desc->codecs()) { + for (const cricket::AudioCodec& existing_codec : media_desc->codecs()) { // TODO(crbug.com/1338902) re-add checks for clockrate and number of // channels. if (!existing_codec.name.empty() && payload_type == existing_codec.id && @@ -3720,7 +3681,7 @@ bool ParseRtpmapAttribute(absl::string_view line, } } UpdateCodec(payload_type, encoding_name, clock_rate, 0, channels, - audio_desc); + media_desc->as_audio()); } return true; } @@ -3791,12 +3752,9 @@ bool ParseFmtpAttributes(absl::string_view line, codec_params[name] = value; } - if (media_type == cricket::MEDIA_TYPE_AUDIO) { - UpdateCodec( - media_desc, payload_type, codec_params); - } else if (media_type == cricket::MEDIA_TYPE_VIDEO) { - UpdateCodec( - media_desc, payload_type, codec_params); + if (media_type == cricket::MEDIA_TYPE_AUDIO || + media_type == cricket::MEDIA_TYPE_VIDEO) { + UpdateCodec(media_desc, payload_type, codec_params); } return true; } @@ -3862,12 +3820,9 @@ bool ParseRtcpFbAttribute(absl::string_view line, } const cricket::FeedbackParam feedback_param(id, param); - if (media_type == cricket::MEDIA_TYPE_AUDIO) { - UpdateCodec( - media_desc, payload_type, feedback_param); - } else if (media_type == cricket::MEDIA_TYPE_VIDEO) { - UpdateCodec( - media_desc, payload_type, feedback_param); + if (media_type == cricket::MEDIA_TYPE_AUDIO || + media_type == cricket::MEDIA_TYPE_VIDEO) { + UpdateCodec(media_desc, payload_type, feedback_param); } return true; }