From a104ceb0ceec0f95e199e6d6704f41ec88a51fc5 Mon Sep 17 00:00:00 2001 From: Johannes Kron Date: Fri, 24 Jan 2020 16:04:04 +0000 Subject: Revert "Reland "Reland "Distinguish between send and receive codecs""" This reverts commit 9bac68c0cc4444b852416396f0e0f31ea66a9cfe. Reason for revert: Breaks perf test on iOS. Original change's description: > Reland "Reland "Distinguish between send and receive codecs"" > > This reverts commit 00a30873c415d717af8dcdf21c2df7fd4b6d1ed2. > > Reason for revert: Flaky test in Chromium fixed. > > Original change's description: > > Revert "Reland "Distinguish between send and receive codecs"" > > > > This reverts commit 133bf2bd28596aab5c7684e0ea3da99b1fece77f. > > > > Reason for revert: Breaks Chromium import due to flaky test in Chromium. > > > > Original change's description: > > > Reland "Distinguish between send and receive codecs" > > > > > > This reverts commit e57b266a20334e47f105a0bd777190ec8c6562e8. > > > > > > Reason for revert: Fixed negotiation of send-only clients. > > > > > > Original change's description: > > > > Revert "Distinguish between send and receive codecs" > > > > > > > > This reverts commit c0f25cf762a6946666c812f7a3df3f0a7f98b38d. > > > > > > > > Reason for revert: breaks negotiation with send-only clients > > > > > > > > (webrtc_video_engine.cc:985): SetRecvParameters called with unsupported video codec: VideoCodec[96:H264] > > > > (peer_connection.cc:6043): Failed to set local video description recv parameters. (INVALID_PARAMETER) > > > > (peer_connection.cc:2591): Failed to set local offer sdp: Failed to set local video description recv parameters. > > > > > > > > Original change's description: > > > > > Distinguish between send and receive codecs > > > > > > > > > > Even though send and receive codecs may be the same, they might have > > > > > different support in HW. Distinguish between send and receive codecs > > > > > to be able to keep track of which codecs have HW support. > > > > > > > > > > Bug: chromium:1029737 > > > > > Change-Id: Id119560becadfe0aaf861c892a6485f1c2eb378d > > > > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/165763 > > > > > Commit-Queue: Johannes Kron > > > > > Reviewed-by: Steve Anton > > > > > Cr-Commit-Position: refs/heads/master@{#30284} > > > > > > > > TBR=steveanton@webrtc.org,kron@webrtc.org > > > > > > > > Change-Id: Iacb7059436b2313b52577b65f164ee363c4816aa > > > > No-Presubmit: true > > > > No-Tree-Checks: true > > > > No-Try: true > > > > Bug: chromium:1029737 > > > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/166420 > > > > Reviewed-by: Steve Anton > > > > Commit-Queue: Steve Anton > > > > Cr-Commit-Position: refs/heads/master@{#30292} > > > > > > TBR=steveanton@webrtc.org,kron@webrtc.org > > > > > > > > > Bug: chromium:1029737 > > > Change-Id: I287efcfdcd1c9a3f2c410aeec8fe26a84204d1fd > > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/166604 > > > Reviewed-by: Johannes Kron > > > Reviewed-by: Steve Anton > > > Commit-Queue: Johannes Kron > > > Cr-Commit-Position: refs/heads/master@{#30348} > > > > TBR=steveanton@webrtc.org,kron@webrtc.org > > > > Change-Id: I9f8731309749e07ce7e651e1550ecfabddb1735f > > No-Presubmit: true > > No-Tree-Checks: true > > No-Try: true > > Bug: chromium:1029737 > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/167205 > > Reviewed-by: Johannes Kron > > Commit-Queue: Johannes Kron > > Cr-Commit-Position: refs/heads/master@{#30360} > > TBR=steveanton@webrtc.org,kron@webrtc.org > > Change-Id: I1cc2d83bd884f10685503a9c31288f96c935d6a3 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: chromium:1029737 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/167206 > Reviewed-by: Johannes Kron > Reviewed-by: Steve Anton > Commit-Queue: Johannes Kron > Cr-Commit-Position: refs/heads/master@{#30367} TBR=steveanton@webrtc.org,kron@webrtc.org Change-Id: I0a9b0b58922ce7c558b3d31b64cc12086b2a6a55 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: chromium:1029737 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/167364 Commit-Queue: Johannes Kron Reviewed-by: Johannes Kron Cr-Commit-Position: refs/heads/master@{#30373} --- pc/media_session.cc | 168 +++++++++++----------------------------------------- 1 file changed, 33 insertions(+), 135 deletions(-) (limited to 'pc/media_session.cc') diff --git a/pc/media_session.cc b/pc/media_session.cc index e764101eef..59f140f951 100644 --- a/pc/media_session.cc +++ b/pc/media_session.cc @@ -1330,12 +1330,10 @@ MediaSessionDescriptionFactory::MediaSessionDescriptionFactory( channel_manager->GetSupportedAudioSendCodecs(&audio_send_codecs_); channel_manager->GetSupportedAudioReceiveCodecs(&audio_recv_codecs_); channel_manager->GetSupportedAudioRtpHeaderExtensions(&audio_rtp_extensions_); - channel_manager->GetSupportedVideoSendCodecs(&video_send_codecs_); - channel_manager->GetSupportedVideoReceiveCodecs(&video_recv_codecs_); + channel_manager->GetSupportedVideoCodecs(&video_codecs_); channel_manager->GetSupportedVideoRtpHeaderExtensions(&video_rtp_extensions_); channel_manager->GetSupportedDataCodecs(&rtp_data_codecs_); ComputeAudioCodecsIntersectionAndUnion(); - ComputeVideoCodecsIntersectionAndUnion(); } const AudioCodecs& MediaSessionDescriptionFactory::audio_sendrecv_codecs() @@ -1359,27 +1357,6 @@ void MediaSessionDescriptionFactory::set_audio_codecs( ComputeAudioCodecsIntersectionAndUnion(); } -const VideoCodecs& MediaSessionDescriptionFactory::video_sendrecv_codecs() - const { - return video_sendrecv_codecs_; -} - -const VideoCodecs& MediaSessionDescriptionFactory::video_send_codecs() const { - return video_send_codecs_; -} - -const VideoCodecs& MediaSessionDescriptionFactory::video_recv_codecs() const { - return video_recv_codecs_; -} - -void MediaSessionDescriptionFactory::set_video_codecs( - const VideoCodecs& send_codecs, - const VideoCodecs& recv_codecs) { - video_send_codecs_ = send_codecs; - video_recv_codecs_ = recv_codecs; - ComputeVideoCodecsIntersectionAndUnion(); -} - static void RemoveUnifiedPlanExtensions(RtpHeaderExtensions* extensions) { RTC_DCHECK(extensions); @@ -1760,41 +1737,6 @@ const AudioCodecs& MediaSessionDescriptionFactory::GetAudioCodecsForAnswer( return audio_sendrecv_codecs_; } -const VideoCodecs& MediaSessionDescriptionFactory::GetVideoCodecsForOffer( - const RtpTransceiverDirection& direction) const { - switch (direction) { - // If stream is inactive - generate list as if sendrecv. - case RtpTransceiverDirection::kSendRecv: - case RtpTransceiverDirection::kInactive: - return video_sendrecv_codecs_; - case RtpTransceiverDirection::kSendOnly: - return video_send_codecs_; - case RtpTransceiverDirection::kRecvOnly: - return video_recv_codecs_; - } - RTC_NOTREACHED(); - return video_sendrecv_codecs_; -} - -const VideoCodecs& MediaSessionDescriptionFactory::GetVideoCodecsForAnswer( - const RtpTransceiverDirection& offer, - const RtpTransceiverDirection& answer) const { - switch (answer) { - // For inactive and sendrecv answers, generate lists as if we were to accept - // the offer's direction. See RFC 3264 Section 6.1. - case RtpTransceiverDirection::kSendRecv: - case RtpTransceiverDirection::kInactive: - return GetVideoCodecsForOffer( - webrtc::RtpTransceiverDirectionReversed(offer)); - case RtpTransceiverDirection::kSendOnly: - return video_send_codecs_; - case RtpTransceiverDirection::kRecvOnly: - return video_recv_codecs_; - } - RTC_NOTREACHED(); - return video_sendrecv_codecs_; -} - void MergeCodecsFromDescription( const std::vector& current_active_contents, AudioCodecs* audio_codecs, @@ -1842,7 +1784,7 @@ void MediaSessionDescriptionFactory::GetCodecsForOffer( // Add our codecs that are not in the current description. MergeCodecs(all_audio_codecs_, audio_codecs, &used_pltypes); - MergeCodecs(all_video_codecs_, video_codecs, &used_pltypes); + MergeCodecs(video_codecs_, video_codecs, &used_pltypes); MergeCodecs(rtp_data_codecs_, rtp_data_codecs, &used_pltypes); } @@ -1890,7 +1832,7 @@ void MediaSessionDescriptionFactory::GetCodecsForAnswer( if (!FindMatchingCodec(video->codecs(), filtered_offered_video_codecs, offered_video_codec, nullptr) && - FindMatchingCodec(video->codecs(), all_video_codecs_, + FindMatchingCodec(video->codecs(), video_codecs_, offered_video_codec, nullptr)) { filtered_offered_video_codecs.push_back(offered_video_codec); } @@ -2097,7 +2039,7 @@ bool MediaSessionDescriptionFactory::AddAudioContentForOffer( IsDtlsActive(current_content, current_description) ? cricket::SEC_DISABLED : secure(); - auto audio = std::make_unique(); + std::unique_ptr audio(new AudioContentDescription()); std::vector crypto_suites; GetSupportedAudioSdesCryptoSuiteNames(session_options.crypto_options, &crypto_suites); @@ -2125,8 +2067,6 @@ bool MediaSessionDescriptionFactory::AddAudioContentForOffer( return true; } -// TODO(kron): This function is very similar to AddAudioContentForOffer. -// Refactor to reuse shared code. bool MediaSessionDescriptionFactory::AddVideoContentForOffer( const MediaDescriptionOptions& media_description_options, const MediaSessionOptions& session_options, @@ -2137,10 +2077,14 @@ bool MediaSessionDescriptionFactory::AddVideoContentForOffer( StreamParamsVec* current_streams, SessionDescription* desc, IceCredentialsIterator* ice_credentials) const { - // Filter video_codecs (which includes all codecs, with correctly remapped - // payload types) based on transceiver direction. - const VideoCodecs& supported_video_codecs = - GetVideoCodecsForOffer(media_description_options.direction); + cricket::SecurePolicy sdes_policy = + IsDtlsActive(current_content, current_description) ? cricket::SEC_DISABLED + : secure(); + + std::unique_ptr video(new VideoContentDescription()); + std::vector crypto_suites; + GetSupportedVideoSdesCryptoSuiteNames(session_options.crypto_options, + &crypto_suites); VideoCodecs filtered_codecs; @@ -2148,7 +2092,7 @@ bool MediaSessionDescriptionFactory::AddVideoContentForOffer( // Add the codecs from the current transceiver's codec preferences. // They override any existing codecs from previous negotiations. filtered_codecs = MatchCodecPreference( - media_description_options.codec_preferences, supported_video_codecs); + media_description_options.codec_preferences, video_codecs_); } else { // Add the codecs from current content if it exists and is not rejected nor // recycled. @@ -2166,11 +2110,11 @@ bool MediaSessionDescriptionFactory::AddVideoContentForOffer( } // Add other supported video codecs. VideoCodec found_codec; - for (const VideoCodec& codec : supported_video_codecs) { - if (FindMatchingCodec(supported_video_codecs, video_codecs, - codec, &found_codec) && - !FindMatchingCodec(supported_video_codecs, - filtered_codecs, codec, nullptr)) { + for (const VideoCodec& codec : video_codecs_) { + if (FindMatchingCodec(video_codecs_, video_codecs, codec, + &found_codec) && + !FindMatchingCodec(video_codecs_, filtered_codecs, codec, + nullptr)) { // Use the |found_codec| from |video_codecs| because it has the // correctly mapped payload type. filtered_codecs.push_back(found_codec); @@ -2186,13 +2130,6 @@ bool MediaSessionDescriptionFactory::AddVideoContentForOffer( } } - cricket::SecurePolicy sdes_policy = - IsDtlsActive(current_content, current_description) ? cricket::SEC_DISABLED - : secure(); - auto video = std::make_unique(); - std::vector crypto_suites; - GetSupportedVideoSdesCryptoSuiteNames(session_options.crypto_options, - &crypto_suites); if (!CreateMediaContentOffer(media_description_options, session_options, filtered_codecs, sdes_policy, GetCryptos(current_content), crypto_suites, @@ -2215,7 +2152,6 @@ bool MediaSessionDescriptionFactory::AddVideoContentForOffer( current_description, desc, ice_credentials)) { return false; } - return true; } @@ -2227,7 +2163,8 @@ bool MediaSessionDescriptionFactory::AddSctpDataContentForOffer( StreamParamsVec* current_streams, SessionDescription* desc, IceCredentialsIterator* ice_credentials) const { - auto data = std::make_unique(); + std::unique_ptr data( + new SctpDataContentDescription()); bool secure_transport = (transport_desc_factory_->secure() != SEC_DISABLED); @@ -2273,7 +2210,8 @@ bool MediaSessionDescriptionFactory::AddRtpDataContentForOffer( StreamParamsVec* current_streams, SessionDescription* desc, IceCredentialsIterator* ice_credentials) const { - auto data = std::make_unique(); + std::unique_ptr data( + new RtpDataContentDescription()); bool secure_transport = (transport_desc_factory_->secure() != SEC_DISABLED); cricket::SecurePolicy sdes_policy = @@ -2413,7 +2351,8 @@ bool MediaSessionDescriptionFactory::AddAudioContentForAnswer( bool bundle_enabled = offer_description->HasGroup(GROUP_TYPE_BUNDLE) && session_options.bundle_enabled; - auto audio_answer = std::make_unique(); + std::unique_ptr audio_answer( + new AudioContentDescription()); // Do not require or create SDES cryptos if DTLS is used. cricket::SecurePolicy sdes_policy = audio_transport->secure() ? cricket::SEC_DISABLED : secure(); @@ -2453,8 +2392,6 @@ bool MediaSessionDescriptionFactory::AddAudioContentForAnswer( return true; } -// TODO(kron): This function is very similar to AddAudioContentForAnswer. -// Refactor to reuse shared code. bool MediaSessionDescriptionFactory::AddVideoContentForAnswer( const MediaDescriptionOptions& media_description_options, const MediaSessionOptions& session_options, @@ -2479,20 +2416,11 @@ bool MediaSessionDescriptionFactory::AddVideoContentForAnswer( return false; } - // Pick codecs based on the requested communications direction in the offer - // and the selected direction in the answer. - // Note these will be filtered one final time in CreateMediaContentAnswer. - auto wants_rtd = media_description_options.direction; - auto offer_rtd = offer_video_description->direction(); - auto answer_rtd = NegotiateRtpTransceiverDirection(offer_rtd, wants_rtd); - VideoCodecs supported_video_codecs = - GetVideoCodecsForAnswer(offer_rtd, answer_rtd); - VideoCodecs filtered_codecs; if (!media_description_options.codec_preferences.empty()) { filtered_codecs = MatchCodecPreference( - media_description_options.codec_preferences, supported_video_codecs); + media_description_options.codec_preferences, video_codecs_); } else { // Add the codecs from current content if it exists and is not rejected nor // recycled. @@ -2509,11 +2437,11 @@ bool MediaSessionDescriptionFactory::AddVideoContentForAnswer( } } // Add other supported video codecs. - for (const VideoCodec& codec : supported_video_codecs) { - if (FindMatchingCodec(supported_video_codecs, video_codecs, - codec, nullptr) && - !FindMatchingCodec(supported_video_codecs, - filtered_codecs, codec, nullptr)) { + for (const VideoCodec& codec : video_codecs_) { + if (FindMatchingCodec(video_codecs_, video_codecs, codec, + nullptr) && + !FindMatchingCodec(video_codecs_, filtered_codecs, codec, + nullptr)) { // We should use the local codec with local parameters and the codec id // would be correctly mapped in |NegotiateCodecs|. filtered_codecs.push_back(codec); @@ -2531,7 +2459,9 @@ bool MediaSessionDescriptionFactory::AddVideoContentForAnswer( bool bundle_enabled = offer_description->HasGroup(GROUP_TYPE_BUNDLE) && session_options.bundle_enabled; - auto video_answer = std::make_unique(); + + std::unique_ptr video_answer( + new VideoContentDescription()); // Do not require or create SDES cryptos if DTLS is used. cricket::SecurePolicy sdes_policy = video_transport->secure() ? cricket::SEC_DISABLED : secure(); @@ -2701,38 +2631,6 @@ void MediaSessionDescriptionFactory::ComputeAudioCodecsIntersectionAndUnion() { &audio_sendrecv_codecs_, true); } -void MediaSessionDescriptionFactory::ComputeVideoCodecsIntersectionAndUnion() { - video_sendrecv_codecs_.clear(); - all_video_codecs_.clear(); - // Compute the video codecs union. - for (const VideoCodec& send : video_send_codecs_) { - all_video_codecs_.push_back(send); - if (!FindMatchingCodec(video_send_codecs_, video_recv_codecs_, - send, nullptr)) { - // TODO(kron): This check is violated by the unit test: - // MediaSessionDescriptionFactoryTest.RtxWithoutApt - // Remove either the test or the check. - - // It doesn't make sense to have an RTX codec we support sending but not - // receiving. - // RTC_DCHECK(!IsRtxCodec(send)); - } - } - for (const VideoCodec& recv : video_recv_codecs_) { - if (!FindMatchingCodec(video_recv_codecs_, video_send_codecs_, - recv, nullptr)) { - all_video_codecs_.push_back(recv); - } - } - // Use NegotiateCodecs to merge our codec lists, since the operation is - // essentially the same. Put send_codecs as the offered_codecs, which is the - // order we'd like to follow. The reasoning is that encoding is usually more - // expensive than decoding, and prioritizing a codec in the send list probably - // means it's a codec we can handle efficiently. - NegotiateCodecs(video_recv_codecs_, video_send_codecs_, - &video_sendrecv_codecs_, true); -} - bool IsMediaContent(const ContentInfo* content) { return (content && (content->type == MediaProtocolType::kRtp || content->type == MediaProtocolType::kSctp)); -- cgit v1.2.3