Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1880913002:
Accept all the media profiles required by JSEP. (Closed)
DescriptionAccept all the media profiles required by JSEP.
JSEP section 5.1.3 states that:
Any profile matching the following patterns MUST be accepted:
"RTP/[S]AVP[F]" and "(UDP/TCP)/TLS/RTP/SAVP[F]"
NOTRY=True
BUG=webrtc:5638
Committed: https://crrev.com/b7f425ab68ec58e2a5beaaf5ef79f50f1982c6f9
Cr-Commit-Position: refs/heads/master@{#12338}
Committed: https://crrev.com/cf5b37cc46156fe8a22cbc5d924ce4526be5694c
Cr-Commit-Position: refs/heads/master@{#12637}
Patch Set 1 #Patch Set 2 : Modified the unit tests #
Total comments: 6
Patch Set 3 : Modified the unit tests according to CR comments. #Patch Set 4 : Fix some problems in tests. #
Total comments: 2
Patch Set 5 : Rename a local vairable #
Total comments: 3
Patch Set 6 : Fix the static initializer problem. #
Total comments: 11
Patch Set 7 : Change to plain string matching. #Patch Set 8 : Fix a minor format problem. #Patch Set 9 : No changes. Just rerun the trybots. #Patch Set 10 : No changes. Rerun trybots. #Messages
Total messages: 41 (17 generated)
zhihuang@webrtc.org changed reviewers: + deadbeef@webrtc.org, pthatcher@webrtc.org
I think the parameterized test makes this a lot cleaner, good job figuring that out. https://codereview.webrtc.org/1880913002/diff/20001/webrtc/pc/mediasession_un... File webrtc/pc/mediasession_unittest.cc (right): https://codereview.webrtc.org/1880913002/diff/20001/webrtc/pc/mediasession_un... webrtc/pc/mediasession_unittest.cc:183: static const char* kMediaProtocols[] = {"RTP/AVP", "RTP/SAVP", "RTP/AVPF"}; What about RTP/SAVPF? https://codereview.webrtc.org/1880913002/diff/20001/webrtc/pc/mediasession_un... webrtc/pc/mediasession_unittest.cc:2441: TEST_P(MediaProtocolTest, TestVideoAcceptance) { I'd rename this to "TestAudioVideoAcceptance". And I think you can remove the audio-only test; if a profile string is accepted for both audio and video, we can probably assume it will be accepted for audio only. https://codereview.webrtc.org/1880913002/diff/20001/webrtc/pc/mediasession_un... webrtc/pc/mediasession_unittest.cc:2444: std::unique_ptr<SessionDescription> offer(f1_.CreateOffer(opts, NULL)); nit: replace "NULL" with C++11 "nullptr" https://codereview.webrtc.org/1880913002/diff/20001/webrtc/pc/mediasession_un... webrtc/pc/mediasession_unittest.cc:2455: EXPECT_FALSE(vc->rejected); One extra useful check would be that the protocol string in the answer is the same as the one in the offer. In other words, if the offer is "RTP/SAVP", the answer is too, and it isn't "UDP/TLS/RTP/SAVPF". https://codereview.webrtc.org/1880913002/diff/20001/webrtc/pc/mediasession_un... webrtc/pc/mediasession_unittest.cc:2458: TEST_P(MediaProtocolTest, TestDataAcceptance) { I'd suggest leaving a TODO to remove this test when we remove the code for RTP data channels. Or just remove it now, since we don't officially support RTP data channels any more anyway.
@Peter: Taylor and I talked about the Tests. We think the Parameterized Tests will make the code cleaner and we can still easily find out which test break. What do you think? https://codereview.webrtc.org/1880913002/diff/20001/webrtc/pc/mediasession_un... File webrtc/pc/mediasession_unittest.cc (right): https://codereview.webrtc.org/1880913002/diff/20001/webrtc/pc/mediasession_un... webrtc/pc/mediasession_unittest.cc:183: static const char* kMediaProtocols[] = {"RTP/AVP", "RTP/SAVP", "RTP/AVPF"}; On 2016/04/12 21:50:05, Taylor Brandstetter wrote: > What about RTP/SAVPF? I thought the RTP/SAVPF is the default protocol use which had been covered already. But I agree that it will be better to test all the valid protocols. I will add it.
lgtm https://codereview.webrtc.org/1880913002/diff/60001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1880913002/diff/60001/webrtc/pc/mediasession.cc... webrtc/pc/mediasession.cc:1115: static bool IsProtocolFound(const std::vector<std::string> protocol_vec, protocol_vec => protocols https://codereview.webrtc.org/1880913002/diff/60001/webrtc/pc/mediasession_un... File webrtc/pc/mediasession_unittest.cc (right): https://codereview.webrtc.org/1880913002/diff/60001/webrtc/pc/mediasession_un... webrtc/pc/mediasession_unittest.cc:2458: ::testing::ValuesIn(kMediaProtocolsDtls)); Wow. That's slick.
lgtm
The CQ bit was checked by zhihuang@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1880913002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880913002/60001
Description was changed from ========== Fix the profile m= related bug. Accept more valid media protocols. Test the protocols with value parameterized tests. BUG=webrtc:5638 ========== to ========== Accept all the media profiles required by JSEP. JSEP section 5.1.3 states that: Any profile matching the following patterns MUST be accepted: "RTP/[S]AVP[F]" and "(UDP/TCP)/TLS/RTP/SAVP[F]" BUG=webrtc:5638 ==========
Description was changed from ========== Accept all the media profiles required by JSEP. JSEP section 5.1.3 states that: Any profile matching the following patterns MUST be accepted: "RTP/[S]AVP[F]" and "(UDP/TCP)/TLS/RTP/SAVP[F]" BUG=webrtc:5638 ========== to ========== Accept all the media profiles required by JSEP. JSEP section 5.1.3 states that: Any profile matching the following patterns MUST be accepted: "RTP/[S]AVP[F]" and "(UDP/TCP)/TLS/RTP/SAVP[F]" NOTRY=True BUG=webrtc:5638 ==========
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1880913002/#ps80001 (title: "Rename a local vairable")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1880913002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880913002/80001
Message was sent while issue was closed.
Description was changed from ========== Accept all the media profiles required by JSEP. JSEP section 5.1.3 states that: Any profile matching the following patterns MUST be accepted: "RTP/[S]AVP[F]" and "(UDP/TCP)/TLS/RTP/SAVP[F]" NOTRY=True BUG=webrtc:5638 ========== to ========== Accept all the media profiles required by JSEP. JSEP section 5.1.3 states that: Any profile matching the following patterns MUST be accepted: "RTP/[S]AVP[F]" and "(UDP/TCP)/TLS/RTP/SAVP[F]" NOTRY=True BUG=webrtc:5638 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Accept all the media profiles required by JSEP. JSEP section 5.1.3 states that: Any profile matching the following patterns MUST be accepted: "RTP/[S]AVP[F]" and "(UDP/TCP)/TLS/RTP/SAVP[F]" NOTRY=True BUG=webrtc:5638 ========== to ========== Accept all the media profiles required by JSEP. JSEP section 5.1.3 states that: Any profile matching the following patterns MUST be accepted: "RTP/[S]AVP[F]" and "(UDP/TCP)/TLS/RTP/SAVP[F]" NOTRY=True BUG=webrtc:5638 Committed: https://crrev.com/b7f425ab68ec58e2a5beaaf5ef79f50f1982c6f9 Cr-Commit-Position: refs/heads/master@{#12338} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b7f425ab68ec58e2a5beaaf5ef79f50f1982c6f9 Cr-Commit-Position: refs/heads/master@{#12338}
Message was sent while issue was closed.
avi@chromium.org changed reviewers: + avi@chromium.org
Message was sent while issue was closed.
This broke the build!! This introduced a zillion static initializers: # mediasession.cc .L.str.1 # mediasession.cc .L.str.3 # mediasession.cc .L.str.6 # mediasession.cc .L.str.7 # mediasession.cc .L.str.8 # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x108 # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x11e # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x134 # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x223 # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x239 # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x24f # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x265 # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x28e # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x2ae # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x2ce # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x2ee # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x30e # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x32e # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x34e # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x36e # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x38e # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x395 # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x3ab # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x3b2 # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x3c8 # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x3cf # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x3e5 # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x3ec # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x402 # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x409 # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x41f # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x426 # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x43c # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x443 # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x459 # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0x460 # mediasession.cc _GLOBAL__sub_I_mediasession.cc+0xf2 # mediasession.cc extensions::api::activity_log_private::DeleteActivities::Params::~Params() # mediasession.cc cricket::kMediaProtocolAvpf # mediasession.cc cricket::kMediaProtocolSavpf # mediasession.cc cricket::kMediaProtocolDtlsSavpf # mediasession.cc cricket::kMediaProtocols # mediasession.cc cricket::kMediaProtocols+0x10 # mediasession.cc cricket::kMediaProtocols+0x8 # mediasession.cc cricket::kMediaProtocolsDtls # mediasession.cc cricket::kMediaProtocolsDtls+0x10 # mediasession.cc cricket::kMediaProtocolsDtls+0x8 # mediasession.cc std::basic_string<char, std::char_traits<char>, std::allocator<char> >::_Rep::_M_destroy(std::allocator<char> const&)@plt # mediasession.cc std::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(char const*, std::allocator<char> const&)@plt # mediasession.cc std::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)@plt # mediasession.cc operator new[](unsigned long) # mediasession.cc __cxa_atexit@plt [registers a dtor to run at exit] # mediasession.cc __init_array_end+0x30f8 # mediasession.cc __init_array_end+0x4b0 # mediasession.cc __init_array_end+0x4e0 Please DO NOT introduce any static initializers into Chromium.
Message was sent while issue was closed.
not lgtm https://codereview.webrtc.org/1880913002/diff/80001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1880913002/diff/80001/webrtc/pc/mediasession.cc... webrtc/pc/mediasession.cc:62: "RTP/AVPF", "RTP/AVP"}; This is a static initializer. https://codereview.webrtc.org/1880913002/diff/80001/webrtc/pc/mediasession.cc... webrtc/pc/mediasession.cc:65: "TCP/TLS/RTP/SAVP"}; This is a static initializer. https://codereview.webrtc.org/1880913002/diff/80001/webrtc/pc/mediasession.cc... webrtc/pc/mediasession.cc:1124: // Data channels can have a protocol of SCTP or SCTP/DTLS. You can totally put those variable declarations into this function as: const std::vector<std::string> kMediaProtocols = ... const std::vector<std::string> kMediaProtocolsDtls = ... and they'll be initialized safely when this function is first called.
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.webrtc.org/1882923002/ by zhihuang@webrtc.org. The reason for reverting is: Broke the Chromium build by introducing static initializers..
Message was sent while issue was closed.
Description was changed from ========== Accept all the media profiles required by JSEP. JSEP section 5.1.3 states that: Any profile matching the following patterns MUST be accepted: "RTP/[S]AVP[F]" and "(UDP/TCP)/TLS/RTP/SAVP[F]" NOTRY=True BUG=webrtc:5638 Committed: https://crrev.com/b7f425ab68ec58e2a5beaaf5ef79f50f1982c6f9 Cr-Commit-Position: refs/heads/master@{#12338} ========== to ========== Accept all the media profiles required by JSEP. JSEP section 5.1.3 states that: Any profile matching the following patterns MUST be accepted: "RTP/[S]AVP[F]" and "(UDP/TCP)/TLS/RTP/SAVP[F]" NOTRY=True BUG=webrtc:5638 Committed: https://crrev.com/b7f425ab68ec58e2a5beaaf5ef79f50f1982c6f9 Cr-Commit-Position: refs/heads/master@{#12338} ==========
Avi: Do you know why none of the bots complained about the static initializers? Both the WebRTC bots and the WebRTC FYI bots have been green. Which build failed exactly?
On 2016/04/13 17:52:18, Taylor Brandstetter wrote: > Avi: Do you know why none of the bots complained about the static initializers? > Both the WebRTC bots and the WebRTC FYI bots have been green. Which build failed > exactly? https://build.chromium.org/p/chromium/builders/Mac https://build.chromium.org/p/chromium/builders/Linux%20x64 The main builders on the waterfall that run the sizes test. I don't know why no trybots found this, sorry.
zhihuang@webrtc.org changed reviewers: - avi@chromium.org
tommi@webrtc.org changed reviewers: + tommi@webrtc.org
drive-by suggestions since this CL will have to be relanded. https://codereview.webrtc.org/1880913002/diff/100001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1880913002/diff/100001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1118: static bool IsProtocolFound(const std::vector<std::string> protocols, Maybe this is by accident - but please don't pass by value. This creates a copy of the vector every time the function is called. https://codereview.webrtc.org/1880913002/diff/100001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1129: "RTP/AVPF", "RTP/AVP"}; I don't think this needs to be std::vector. It contains static data which can be searched without having to copy it into strings every time. Also, as is, the variable name should not be kMediaProtocols. kFooNaming is used for compile-time constants, not runtime, so here using media_protocols would be appropriate. Even though this is const, we're still creating this array every time the function is called. I'd also not want to change this to be a static vector<> in case that's being considered, for inefficiency reasons (and that statics are not thread safe on all platforms). static char*[] on the other hand, is fine. Instead, I'd propose something along these lines: --------- // Since not all applications serialize and deserialize the media protocol, // we will have to accept |protocol| to be empty. if (protocol.empty()) return true; if (type == MEDIA_TYPE_DATA && (secure_transport ? protocol == kMediaProtocolDtlsSctp : protocol == kMediaProtocolSctp))) { return true; } (see other comment for concern around type == MEDIA_TYPE_DATA at this point) static const char* kMediaProtocols[] = {...}; static const char* kMediaProtocolsDtls[] = {...}; (the following line may be my misunderstanding, so defer to peter) return secure_transport ? IsProtocolFound(kMediaProtocolsDtls, arraysize(kMediaProtocolsDtls), protocol) : IsProtocolFound(kMediaProtocols, arraysize(kMediaProtocols), protocol); --------- I suspect that the check for MEDIA_TYPE_DATA ought to be something like this: if (type == MEDIA_TYPE_DATA) { return secure_transport ? protocol == kMediaProtocolDtlsSctp : protocol == kMediaProtocolSctp); } https://codereview.webrtc.org/1880913002/diff/100001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1136: ((protocol == kMediaProtocolSctp && !secure_transport)|| check secure_transport first here and on the next line. https://codereview.webrtc.org/1880913002/diff/100001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1141: // Since not all applications serialize and deserialize the media protocol, If type is MEDIA_TYPE_DATA when we get here, should we be returning true if e.g. protocol is "RTP/SAVPF"? Does that make sense, or should the above check, handle MEDIA_TYPE_DATA completely? https://codereview.webrtc.org/1880913002/diff/100001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1143: return protocol.empty() || IsProtocolFound(kMediaProtocols, protocol) || the |protocol.empty()| check should be done at the top of the function. As is, it's done after a lot of unnecessary things have been done, including several heap allocations. https://codereview.webrtc.org/1880913002/diff/100001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1144: (IsProtocolFound(kMediaProtocolsDtls, protocol) && secure_transport); |secure_transport| should be checked before doing the array lookup. Also, if secure_transport is true, should we be looking up in kMediaProtocols? https://codereview.webrtc.org/1880913002/diff/100001/webrtc/pc/mediasession_u... File webrtc/pc/mediasession_unittest.cc (right): https://codereview.webrtc.org/1880913002/diff/100001/webrtc/pc/mediasession_u... webrtc/pc/mediasession_unittest.cc:2401: rtc::scoped_ptr<rtc::SSLIdentity>(new rtc::FakeSSLIdentity("id1")))); I'm curious - do you need to create a scoped_ptr here explicitly or would this work: static_cast<SSLIdentity*>(new rtc::FakeSSLIdentity("id1")) (no biggie though)
https://codereview.webrtc.org/1880913002/diff/100001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1880913002/diff/100001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1141: // Since not all applications serialize and deserialize the media protocol, On 2016/04/14 12:59:10, tommi-webrtc wrote: > If type is MEDIA_TYPE_DATA when we get here, should we be returning true if e.g. > protocol is "RTP/SAVPF"? Does that make sense, or should the above check, > handle MEDIA_TYPE_DATA completely? This is for RTP data channels, which some downstream applications are still using (even though not officially supported). https://codereview.webrtc.org/1880913002/diff/100001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1144: (IsProtocolFound(kMediaProtocolsDtls, protocol) && secure_transport); On 2016/04/14 12:59:10, tommi-webrtc wrote: > |secure_transport| should be checked before doing the array lookup. > > Also, if secure_transport is true, should we be looking up in kMediaProtocols? Yes. JSEP says we should accept these protocols as long as there's a fingerprint attribute.
https://codereview.webrtc.org/1880913002/diff/100001/webrtc/pc/mediasession.cc File webrtc/pc/mediasession.cc (right): https://codereview.webrtc.org/1880913002/diff/100001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1118: static bool IsProtocolFound(const std::vector<std::string> protocols, On 2016/04/14 12:59:09, tommi-webrtc wrote: > Maybe this is by accident - but please don't pass by value. This creates a copy > of the vector every time the function is called. Good catch. https://codereview.webrtc.org/1880913002/diff/100001/webrtc/pc/mediasession.c... webrtc/pc/mediasession.cc:1129: "RTP/AVPF", "RTP/AVP"}; On 2016/04/14 12:59:09, tommi-webrtc wrote: > I don't think this needs to be std::vector. It contains static data which can > be searched without having to copy it into strings every time. > > Also, as is, the variable name should not be kMediaProtocols. kFooNaming is > used for compile-time constants, not runtime, so here using media_protocols > would be appropriate. Even though this is const, we're still creating this > array every time the function is called. > > I'd also not want to change this to be a static vector<> in case that's being > considered, for inefficiency reasons (and that statics are not thread safe on > all platforms). static char*[] on the other hand, is fine. > > Instead, I'd propose something along these lines: > > --------- > // Since not all applications serialize and deserialize the media protocol, > // we will have to accept |protocol| to be empty. > if (protocol.empty()) > return true; > > if (type == MEDIA_TYPE_DATA && > (secure_transport ? > protocol == kMediaProtocolDtlsSctp : protocol == kMediaProtocolSctp))) { > return true; > } > > (see other comment for concern around type == MEDIA_TYPE_DATA at this point) > > static const char* kMediaProtocols[] = {...}; > static const char* kMediaProtocolsDtls[] = {...}; > > (the following line may be my misunderstanding, so defer to peter) > > return secure_transport ? > IsProtocolFound(kMediaProtocolsDtls, arraysize(kMediaProtocolsDtls), > protocol) : > IsProtocolFound(kMediaProtocols, arraysize(kMediaProtocols), protocol); > --------- > > > I suspect that the check for MEDIA_TYPE_DATA ought to be something like this: > > if (type == MEDIA_TYPE_DATA) { > return secure_transport ? > protocol == kMediaProtocolDtlsSctp : protocol == kMediaProtocolSctp); > } Your idea of using static const char* is a good optimization. But the logic/behavior isn't quite right, and I think we could make this even more readable and correct and just as fast like so: bool IsDtlsRtp(const std::string& protocol) { // Most-likely values first. return protocol == "UDP/TLS/RTP/SAVPF" || protocol == "TCP/TLS/RTP/SAVPF" || protocol == "UDP/TLS/RTP/SAVP" || protocol == "TCP/TLS/RTP/SAVP"; } bool IsPlainRtp(const std::string& protocol) { // Most-likely values first. return protocol == "RTP/SAVPF" || protocol == "RTP/AVPF" || protocol == "RTP/SAVP" || protocol == "RTP/AVP"; } bool IsDtlsSctp(const std::string& protocol) { return protocol == "DTLS/SCTP"; } bool IsPlainSctp(const std::string& protocol) { return protocol == "SCTP"; } if (protocol.empty()) { return true; } if (type == MEDIA_TYPE_DATA) { // Check for SCTP, but also for RTP for RTP-based data channels. // TODO(pthatcher): Remove RTP once RTP-based data channels are gone. if (secure_transport) { // Most likely scenarios first. return IsDtlsSctp(protocol) || IsDtlsRtp(protocol) || IsPlainRtp(protocol); } else { return IsPlainSctp(protocol) || IsPlainRtp(protocol); } } // Allow for non-DTLS RTP protocol even when using DTLS because that's what JSEP specifies. if (secure_transport) { // Most likely scenarios first. return IsDtlsRtp(protocol) || IsPlainRtp(protocol); } else { return IsPlainRtp(protocol); }
zhihuang@webrtc.org changed reviewers: - tommi@webrtc.org
There are no changes in Patch 9 and 10 as I just wanted to re-run the trybots. In Patch 8, there is one trybot(linux_msan) failed and I tried to re-run this trybot but I chose the wrong one with the same name. Same story for Patch 9. The weird thing is that the right trybot (linux_msan) ran the code for three times and failed first twice with different reasons.
On 2016/04/16 00:58:36, zhihuang wrote: > There are no changes in Patch 9 and 10 as I just wanted to re-run the trybots. > > In Patch 8, there is one trybot(linux_msan) failed and I tried to re-run this > trybot but I chose the wrong one with the same name. Same story for Patch 9. > > The weird thing is that the right trybot (linux_msan) ran the code for three > times and failed first twice with different reasons. Just FYI, you can re-run the trybots without uploading a new patch. You can even select a specific trybot that failed, if you suspect it's flaky.
lgtm
The CQ bit was checked by zhihuang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/1880913002/#ps180001 (title: "No changes. Rerun trybots.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1880913002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880913002/180001
Message was sent while issue was closed.
Description was changed from ========== Accept all the media profiles required by JSEP. JSEP section 5.1.3 states that: Any profile matching the following patterns MUST be accepted: "RTP/[S]AVP[F]" and "(UDP/TCP)/TLS/RTP/SAVP[F]" NOTRY=True BUG=webrtc:5638 Committed: https://crrev.com/b7f425ab68ec58e2a5beaaf5ef79f50f1982c6f9 Cr-Commit-Position: refs/heads/master@{#12338} ========== to ========== Accept all the media profiles required by JSEP. JSEP section 5.1.3 states that: Any profile matching the following patterns MUST be accepted: "RTP/[S]AVP[F]" and "(UDP/TCP)/TLS/RTP/SAVP[F]" NOTRY=True BUG=webrtc:5638 Committed: https://crrev.com/b7f425ab68ec58e2a5beaaf5ef79f50f1982c6f9 Cr-Commit-Position: refs/heads/master@{#12338} ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Accept all the media profiles required by JSEP. JSEP section 5.1.3 states that: Any profile matching the following patterns MUST be accepted: "RTP/[S]AVP[F]" and "(UDP/TCP)/TLS/RTP/SAVP[F]" NOTRY=True BUG=webrtc:5638 Committed: https://crrev.com/b7f425ab68ec58e2a5beaaf5ef79f50f1982c6f9 Cr-Commit-Position: refs/heads/master@{#12338} ========== to ========== Accept all the media profiles required by JSEP. JSEP section 5.1.3 states that: Any profile matching the following patterns MUST be accepted: "RTP/[S]AVP[F]" and "(UDP/TCP)/TLS/RTP/SAVP[F]" NOTRY=True BUG=webrtc:5638 Committed: https://crrev.com/b7f425ab68ec58e2a5beaaf5ef79f50f1982c6f9 Cr-Commit-Position: refs/heads/master@{#12338} Committed: https://crrev.com/cf5b37cc46156fe8a22cbc5d924ce4526be5694c Cr-Commit-Position: refs/heads/master@{#12637} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/cf5b37cc46156fe8a22cbc5d924ce4526be5694c Cr-Commit-Position: refs/heads/master@{#12637} |
