From 341434e4da2c193b8842917d73afed6eea3a4332 Mon Sep 17 00:00:00 2001 From: Olga Sharonova Date: Wed, 3 Jun 2020 13:16:39 +0000 Subject: [PATCH] Revert "Added mid to error messages reported during SDP apply." This reverts commit d2890e8833796f13c4a1243769be966bebdfcaa7. Reason for revert: speculative: WebRtcBrowserTest.NegotiateUnsupportedVideoCodec broken on all FYI bots, example: https://ci.chromium.org/p/chromium/builders/webrtc.fyi/WebRTC%20Chromium%20FYI%20Linux%20Tester/6659 Original change's description: > Added mid to error messages reported during SDP apply. > > Bug: webrtc:10139 > Change-Id: I7462b632e00a2da7b189b63022d30f594700b68a > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176400 > Reviewed-by: Tommi > Commit-Queue: Yura Yaroshevich > Cr-Commit-Position: refs/heads/master@{#31421} TBR=tommi@webrtc.org,yura.yaroshevich@gmail.com Change-Id: I18972815df10e2bd7b914ad82df9596009c2fecc No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:10139 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176418 Reviewed-by: Olga Sharonova Commit-Queue: Olga Sharonova Cr-Commit-Position: refs/heads/master@{#31425} --- pc/channel.cc | 106 +++++++-------------------- pc/jsep_transport_controller.cc | 42 +++++------ pc/peer_connection_media_unittest.cc | 9 +-- 3 files changed, 48 insertions(+), 109 deletions(-) diff --git a/pc/channel.cc b/pc/channel.cc index 4b05bb946c..8a3a800210 100644 --- a/pc/channel.cc +++ b/pc/channel.cc @@ -604,8 +604,7 @@ bool BaseChannel::UpdateLocalStreams_w(const std::vector& streams, if (!media_channel()->RemoveSendStream(old_stream.first_ssrc())) { rtc::StringBuilder desc; desc << "Failed to remove send stream with ssrc " - << old_stream.first_ssrc() << " from m-section with mid='" - << content_name() << "'."; + << old_stream.first_ssrc() << "."; SafeSetError(desc.str(), error_desc); ret = false; } @@ -631,8 +630,7 @@ bool BaseChannel::UpdateLocalStreams_w(const std::vector& streams, if (new_stream.has_ssrcs() && new_stream.has_rids()) { rtc::StringBuilder desc; desc << "Failed to add send stream: " << new_stream.first_ssrc() - << " into m-section with mid='" << content_name() - << "'. Stream has both SSRCs and RIDs."; + << ". Stream has both SSRCs and RIDs."; SafeSetError(desc.str(), error_desc); ret = false; continue; @@ -651,8 +649,7 @@ bool BaseChannel::UpdateLocalStreams_w(const std::vector& streams, << " into " << ToString(); } else { rtc::StringBuilder desc; - desc << "Failed to add send stream ssrc: " << new_stream.first_ssrc() - << " into m-section with mid='" << content_name() << "'"; + desc << "Failed to add send stream ssrc: " << new_stream.first_ssrc(); SafeSetError(desc.str(), error_desc); ret = false; } @@ -682,8 +679,7 @@ bool BaseChannel::UpdateRemoteStreams_w( } else { rtc::StringBuilder desc; desc << "Failed to remove remote stream with ssrc " - << old_stream.first_ssrc() << " from m-section with mid='" - << content_name() << "'."; + << old_stream.first_ssrc() << "."; SafeSetError(desc.str(), error_desc); ret = false; } @@ -866,11 +862,8 @@ bool VoiceChannel::SetLocalContent_w(const MediaContentDescription* content, audio, rtp_header_extensions, webrtc::RtpTransceiverDirectionHasRecv(audio->direction()), &recv_params); if (!media_channel()->SetRecvParameters(recv_params)) { - SafeSetError( - "Failed to set local audio description recv parameters for m-section " - "with mid='" + - content_name() + "'.", - error_desc); + SafeSetError("Failed to set local audio description recv parameters.", + error_desc); return false; } @@ -892,11 +885,7 @@ bool VoiceChannel::SetLocalContent_w(const MediaContentDescription* content, // description too (without a remote description, we won't be able // to send them anyway). if (!UpdateLocalStreams_w(audio->streams(), type, error_desc)) { - SafeSetError( - "Failed to set local audio description streams for m-section with " - "mid='" + - content_name() + "'.", - error_desc); + SafeSetError("Failed to set local audio description streams.", error_desc); return false; } @@ -931,11 +920,8 @@ bool VoiceChannel::SetRemoteContent_w(const MediaContentDescription* content, bool parameters_applied = media_channel()->SetSendParameters(send_params); if (!parameters_applied) { - SafeSetError( - "Failed to set remote audio description send parameters for m-section " - "with mid='" + - content_name() + "'.", - error_desc); + SafeSetError("Failed to set remote audio description send parameters.", + error_desc); return false; } last_send_params_ = send_params; @@ -956,11 +942,7 @@ bool VoiceChannel::SetRemoteContent_w(const MediaContentDescription* content, // description too (without a local description, we won't be able to // recv them anyway). if (!UpdateRemoteStreams_w(audio->streams(), type, error_desc)) { - SafeSetError( - "Failed to set remote audio description streams for m-section with " - "mid='" + - content_name() + "'.", - error_desc); + SafeSetError("Failed to set remote audio description streams.", error_desc); return false; } @@ -1048,9 +1030,7 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content, needs_send_params_update = true; } else if (recv_codec->packetization != send_codec.packetization) { SafeSetError( - "Failed to set local answer due to invalid codec packetization " - "specified in m-section with mid='" + - content_name() + "'.", + "Failed to set local answer due to invalid codec packetization.", error_desc); return false; } @@ -1059,11 +1039,8 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content, } if (!media_channel()->SetRecvParameters(recv_params)) { - SafeSetError( - "Failed to set local video description recv parameters for m-section " - "with mid='" + - content_name() + "'.", - error_desc); + SafeSetError("Failed to set local video description recv parameters.", + error_desc); return false; } @@ -1082,9 +1059,7 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content, if (needs_send_params_update) { if (!media_channel()->SetSendParameters(send_params)) { - SafeSetError("Failed to set send parameters for m-section with mid='" + - content_name() + "'.", - error_desc); + SafeSetError("Failed to set send parameters.", error_desc); return false; } last_send_params_ = send_params; @@ -1095,11 +1070,7 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content, // description too (without a remote description, we won't be able // to send them anyway). if (!UpdateLocalStreams_w(video->streams(), type, error_desc)) { - SafeSetError( - "Failed to set local video description streams for m-section with " - "mid='" + - content_name() + "'.", - error_desc); + SafeSetError("Failed to set local video description streams.", error_desc); return false; } @@ -1147,9 +1118,7 @@ bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content, needs_recv_params_update = true; } else if (send_codec->packetization != recv_codec.packetization) { SafeSetError( - "Failed to set remote answer due to invalid codec packetization " - "specifid in m-section with mid='" + - content_name() + "'.", + "Failed to set remote answer due to invalid codec packetization.", error_desc); return false; } @@ -1158,20 +1127,15 @@ bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content, } if (!media_channel()->SetSendParameters(send_params)) { - SafeSetError( - "Failed to set remote video description send parameters for m-section " - "with mid='" + - content_name() + "'.", - error_desc); + SafeSetError("Failed to set remote video description send parameters.", + error_desc); return false; } last_send_params_ = send_params; if (needs_recv_params_update) { if (!media_channel()->SetRecvParameters(recv_params)) { - SafeSetError("Failed to set recv parameters for m-section with mid='" + - content_name() + "'.", - error_desc); + SafeSetError("Failed to set recv parameters.", error_desc); return false; } last_recv_params_ = recv_params; @@ -1193,11 +1157,7 @@ bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content, // description too (without a local description, we won't be able to // recv them anyway). if (!UpdateRemoteStreams_w(video->streams(), type, error_desc)) { - SafeSetError( - "Failed to set remote video description streams for m-section with " - "mid='" + - content_name() + "'.", - error_desc); + SafeSetError("Failed to set remote video description streams.", error_desc); return false; } set_remote_content_direction(content->direction()); @@ -1288,11 +1248,8 @@ bool RtpDataChannel::SetLocalContent_w(const MediaContentDescription* content, data, rtp_header_extensions, webrtc::RtpTransceiverDirectionHasRecv(data->direction()), &recv_params); if (!media_channel()->SetRecvParameters(recv_params)) { - SafeSetError( - "Failed to set remote data description recv parameters for m-section " - "with mid='" + - content_name() + "'.", - error_desc); + SafeSetError("Failed to set remote data description recv parameters.", + error_desc); return false; } for (const DataCodec& codec : data->codecs()) { @@ -1311,11 +1268,7 @@ bool RtpDataChannel::SetLocalContent_w(const MediaContentDescription* content, // description too (without a remote description, we won't be able // to send them anyway). if (!UpdateLocalStreams_w(data->streams(), type, error_desc)) { - SafeSetError( - "Failed to set local data description streams for m-section with " - "mid='" + - content_name() + "'.", - error_desc); + SafeSetError("Failed to set local data description streams.", error_desc); return false; } @@ -1357,11 +1310,8 @@ bool RtpDataChannel::SetRemoteContent_w(const MediaContentDescription* content, data, rtp_header_extensions, webrtc::RtpTransceiverDirectionHasRecv(data->direction()), &send_params); if (!media_channel()->SetSendParameters(send_params)) { - SafeSetError( - "Failed to set remote data description send parameters for m-section " - "with mid='" + - content_name() + "'.", - error_desc); + SafeSetError("Failed to set remote data description send parameters.", + error_desc); return false; } last_send_params_ = send_params; @@ -1371,11 +1321,7 @@ bool RtpDataChannel::SetRemoteContent_w(const MediaContentDescription* content, // description too (without a local description, we won't be able to // recv them anyway). if (!UpdateRemoteStreams_w(data->streams(), type, error_desc)) { - SafeSetError( - "Failed to set remote data description streams for m-section with " - "mid='" + - content_name() + "'.", - error_desc); + SafeSetError("Failed to set remote data description streams.", error_desc); return false; } diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index c62631df7a..a7e1b876fe 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -654,8 +654,7 @@ RTCError JsepTransportController::ApplyDescription_n( if (IsBundled(content_info.name) && content_info.name != *bundled_mid()) { if (!HandleBundledContent(content_info)) { return RTCError(RTCErrorType::INVALID_PARAMETER, - "Failed to process the bundled m= section with mid='" + - content_info.name + "'."); + "Failed to process the bundled m= section."); } continue; } @@ -707,10 +706,9 @@ RTCError JsepTransportController::ApplyDescription_n( } if (!error.ok()) { - LOG_AND_RETURN_ERROR( - RTCErrorType::INVALID_PARAMETER, - "Failed to apply the description for m= section with mid='" + - content_info.name + "': " + error.message()); + LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, + "Failed to apply the description for " + + content_info.name + ": " + error.message()); } } if (type == SdpType::kAnswer) { @@ -729,11 +727,11 @@ RTCError JsepTransportController::ValidateAndMaybeUpdateBundleGroup( // The BUNDLE group containing a MID that no m= section has is invalid. if (new_bundle_group) { - for (const std::string& content_name : new_bundle_group->content_names()) { + for (const auto& content_name : new_bundle_group->content_names()) { if (!description->GetContentByName(content_name)) { return RTCError(RTCErrorType::INVALID_PARAMETER, - "The BUNDLE group contains MID='" + content_name + - "' matching no m= section."); + "The BUNDLE group contains MID:" + content_name + + " matching no m= section."); } } } @@ -745,21 +743,18 @@ RTCError JsepTransportController::ValidateAndMaybeUpdateBundleGroup( if (new_bundle_group) { // The BUNDLE group in answer should be a subset of offered group. - for (const std::string& content_name : - new_bundle_group->content_names()) { + for (const auto& content_name : new_bundle_group->content_names()) { if (!offered_bundle_group || !offered_bundle_group->HasContentName(content_name)) { return RTCError(RTCErrorType::INVALID_PARAMETER, - "The BUNDLE group in answer contains a MID='" + - content_name + - "' that was " - "not in the offered group."); + "The BUNDLE group in answer contains a MID that was " + "not in the offered group."); } } } if (bundle_group_) { - for (const std::string& content_name : bundle_group_->content_names()) { + for (const auto& content_name : bundle_group_->content_names()) { // An answer that removes m= sections from pre-negotiated BUNDLE group // without rejecting it, is invalid. if (!new_bundle_group || @@ -767,9 +762,8 @@ RTCError JsepTransportController::ValidateAndMaybeUpdateBundleGroup( auto* content_info = description->GetContentByName(content_name); if (!content_info || !content_info->rejected) { return RTCError(RTCErrorType::INVALID_PARAMETER, - "Answer cannot remove m= section with mid='" + - content_name + - "' from already-established BUNDLE group."); + "Answer cannot remove m= section " + content_name + + " from already-established BUNDLE group."); } } } @@ -804,9 +798,9 @@ RTCError JsepTransportController::ValidateAndMaybeUpdateBundleGroup( for (const auto& content_name : bundle_group_->content_names()) { auto other_content = description->GetContentByName(content_name); if (!other_content->rejected) { - return RTCError(RTCErrorType::INVALID_PARAMETER, - "The m= section with mid='" + content_name + - "' should be rejected."); + return RTCError( + RTCErrorType::INVALID_PARAMETER, + "The m= section:" + content_name + " should be rejected."); } } } @@ -821,8 +815,8 @@ RTCError JsepTransportController::ValidateContent( content_info.type == cricket::MediaProtocolType::kRtp && !content_info.media_description()->rtcp_mux()) { return RTCError(RTCErrorType::INVALID_PARAMETER, - "The m= section with mid='" + content_info.name + - "' is invalid. RTCP-MUX is not " + "The m= section:" + content_info.name + + " is invalid. RTCP-MUX is not " "enabled when it is required."); } return RTCError::OK(); diff --git a/pc/peer_connection_media_unittest.cc b/pc/peer_connection_media_unittest.cc index 9f276bdf90..c9ffd776d9 100644 --- a/pc/peer_connection_media_unittest.cc +++ b/pc/peer_connection_media_unittest.cc @@ -1118,11 +1118,10 @@ TEST_P(PeerConnectionMediaTest, MediaEngineErrorPropagatedToClients) { std::string error; ASSERT_FALSE(caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal(), &error)); - EXPECT_EQ(std::string("Failed to set remote answer sdp: Failed to set remote " - "video description " - "send parameters for m-section with mid='") + - (IsUnifiedPlan() ? "1" : "video") + "'.", - error); + EXPECT_EQ( + "Failed to set remote answer sdp: Failed to set remote video description " + "send parameters.", + error); } // Tests that if the underlying video encoder fails once then subsequent