Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1321543002:
Enable generation of local host candidate when RTCConfiguration is null. (Closed)
|
Created:
5 years, 3 months ago by guoweis_left_chromium Modified:
5 years, 3 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable generation of local host candidate when RTCConfiguration is null.
For demo page which doesn't use mic/camera to retain connectivity without stun/turn, it needs localhost candidate. Localhost candidate will only be generated if RTCConfiguration is not specified in the JS RTCPeerConnection ctor.
Note that enable_localhost_candidate is only honored when the adapter enumeration is disabled inside webrtc.
hangouts use iceServers:[] which will not cause local host generated when multiple_routes disabled.
This change depens on ongoing CL https://codereview.chromium.org/1315413004/
BUG=webrtc:4517
Committed: https://crrev.com/e4916a9c873c13e33af1d630dc29fd0205deae27
Cr-Commit-Position: refs/heads/master@{#349315}
Patch Set 1 #
Total comments: 1
Patch Set 2 : #
Total comments: 1
Patch Set 3 : #Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Total comments: 1
Patch Set 6 : #Messages
Total messages: 22 (6 generated)
guoweis@chromium.org changed reviewers: + juberti@chromium.org, pthatcher@chromium.org
PTAL.
juberti@webrtc.org changed reviewers: + juberti@webrtc.org
https://codereview.chromium.org/1321543002/diff/1/content/renderer/media/rtc_... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/1321543002/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_peer_connection_handler.cc:195: webrtc_config->enable_localhost_ice_candidate = true; I think we might also want this when iceServers is null.
iceServers:null is not a valid syntax for either FF or chrome. On Wed, Aug 26, 2015 at 3:49 PM, <juberti@webrtc.org> wrote: > > > https://codereview.chromium.org/1321543002/diff/1/content/renderer/media/rtc_... > File content/renderer/media/rtc_peer_connection_handler.cc (right): > > > https://codereview.chromium.org/1321543002/diff/1/content/renderer/media/rtc_... > content/renderer/media/rtc_peer_connection_handler.cc:195: > webrtc_config->enable_localhost_ice_candidate = true; > I think we might also want this when iceServers is null. > > https://codereview.chromium.org/1321543002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/08/26 22:51:07, guoweis_chromium wrote: > iceServers:null is not a valid syntax for either FF or chrome. > > On Wed, Aug 26, 2015 at 3:49 PM, <mailto:juberti@webrtc.org> wrote: > > > > > > > > https://codereview.chromium.org/1321543002/diff/1/content/renderer/media/rtc_... > > File content/renderer/media/rtc_peer_connection_handler.cc (right): > > > > > > > https://codereview.chromium.org/1321543002/diff/1/content/renderer/media/rtc_... > > content/renderer/media/rtc_peer_connection_handler.cc:195: > > webrtc_config->enable_localhost_ice_candidate = true; > > I think we might also want this when iceServers is null. > > > > https://codereview.chromium.org/1321543002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. iceServers:undefined must be, though? I can imagine demo pages with RTCConfig defined but no iceservers.
On 2015/08/26 23:11:53, juberti1 wrote: > On 2015/08/26 22:51:07, guoweis_chromium wrote: > > iceServers:null is not a valid syntax for either FF or chrome. > > > > On Wed, Aug 26, 2015 at 3:49 PM, <mailto:juberti@webrtc.org> wrote: > > > > > > > > > > > > > > https://codereview.chromium.org/1321543002/diff/1/content/renderer/media/rtc_... > > > File content/renderer/media/rtc_peer_connection_handler.cc (right): > > > > > > > > > > > > https://codereview.chromium.org/1321543002/diff/1/content/renderer/media/rtc_... > > > content/renderer/media/rtc_peer_connection_handler.cc:195: > > > webrtc_config->enable_localhost_ice_candidate = true; > > > I think we might also want this when iceServers is null. > > > > > > https://codereview.chromium.org/1321543002/ > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > iceServers:undefined must be, though? > > I can imagine demo pages with RTCConfig defined but no iceservers. iceServers:undefined is valid for FF but not for chrome. I'll change webkit to make it valid then.
On 2015/08/27 04:39:45, guoweis_chromium wrote: > On 2015/08/26 23:11:53, juberti1 wrote: > > On 2015/08/26 22:51:07, guoweis_chromium wrote: > > > iceServers:null is not a valid syntax for either FF or chrome. > > > > > > On Wed, Aug 26, 2015 at 3:49 PM, <mailto:juberti@webrtc.org> wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1321543002/diff/1/content/renderer/media/rtc_... > > > > File content/renderer/media/rtc_peer_connection_handler.cc (right): > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1321543002/diff/1/content/renderer/media/rtc_... > > > > content/renderer/media/rtc_peer_connection_handler.cc:195: > > > > webrtc_config->enable_localhost_ice_candidate = true; > > > > I think we might also want this when iceServers is null. > > > > > > > > https://codereview.chromium.org/1321543002/ > > > > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > > email > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > iceServers:undefined must be, though? > > > > I can imagine demo pages with RTCConfig defined but no iceservers. > > iceServers:undefined is valid for FF but not for chrome. I'll change webkit to > make it valid then. PTAL.
juberti@google.com changed reviewers: + juberti@google.com
https://codereview.chromium.org/1321543002/diff/20001/content/renderer/media/... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/1321543002/diff/20001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:199: webrtc_config->enable_localhost_ice_candidate = What does this do, exactly?
On 2015/08/28 00:24:49, juberti wrote: > https://codereview.chromium.org/1321543002/diff/20001/content/renderer/media/... > File content/renderer/media/rtc_peer_connection_handler.cc (right): > > https://codereview.chromium.org/1321543002/diff/20001/content/renderer/media/... > content/renderer/media/rtc_peer_connection_handler.cc:199: > webrtc_config->enable_localhost_ice_candidate = > What does this do, exactly? It copies the parsing result from WebRTCConfiguration. This change depens on ongoing CL https://codereview.chromium.org/1315413004/
guoweis@chromium.org changed reviewers: + tommi@chromium.org
tommi@chromium.org: PTAL. This is depending on the other ongoing change https://codereview.chromium.org/1315413004/
https://codereview.chromium.org/1321543002/diff/60001/content/renderer/media/... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/1321543002/diff/60001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:188: if (!webrtc_config) Is this even possible? maybe we should just replace this with a DCHECK. https://codereview.chromium.org/1321543002/diff/60001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:195: webrtc_config->enable_localhost_ice_candidate = true; this line, line 201 and the scope starting in 203 seem to be assuming that the value of enable_localhost_ice_candidate will always be false. If that's the case, can you add a DCHECK at the top of this function? If it's not the case, then do we need to set this to false in 203?
PTAL. https://codereview.chromium.org/1321543002/diff/60001/content/renderer/media/... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/1321543002/diff/60001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:188: if (!webrtc_config) On 2015/09/16 18:13:40, tommi wrote: > Is this even possible? maybe we should just replace this with a DCHECK. Done. https://codereview.chromium.org/1321543002/diff/60001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:195: webrtc_config->enable_localhost_ice_candidate = true; On 2015/09/16 18:13:40, tommi wrote: > this line, line 201 and the scope starting in 203 seem to be assuming that the > value of enable_localhost_ice_candidate will always be false. If that's the > case, can you add a DCHECK at the top of this function? If it's not the case, > then do we need to set this to false in 203? Yes, it was initialized to be false in the ctor of webrtc::PeerConnectionInterface::RTCConfiguration. But adding explicit setting to false is better.
lgtm https://codereview.chromium.org/1321543002/diff/80001/content/renderer/media/... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/1321543002/diff/80001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:190: webrtc_config->enable_localhost_ice_candidate = false; It would be OK with me to do this instead: DCHECK_EQ(webrtc_config->enable_localhost_ice_candidate, false); Oh, actually, now that we're dereferencing webrtc_config anyway, the DCHECK above doesn't mean anything (the DCHECK(webrtc_config) one), so you can remove it. In Chromium it's preferred to not have those sort of checks just before using the pointer regardless.
The CQ bit was checked by guoweis@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org Link to the patchset: https://codereview.chromium.org/1321543002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1321543002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1321543002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e4916a9c873c13e33af1d630dc29fd0205deae27 Cr-Commit-Position: refs/heads/master@{#349315}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1352753002/ by thakis@chromium.org. The reason for reverting is: Looks like this might've broken browser_tests on 32-bit linux: http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29... WebRtcBrowserTest.RunsAudioVideoWebRTCCallInTwoTabs (run #1): ASSERTION FAILED: !m_storage ../../third_party/WebKit/public/platform/WebPrivatePtr.h(243) : ~WebPrivatePtr 1 0xf5b3b647 blink::failedAssertion(char const*, int, char const*, char const*) 2 0xf2d690c1 3 0xf2d68dcb 4 0xf2d4dbed 5 0xf2d4d66c content::RTCPeerConnectionHandler::initialize(blink::WebRTCConfiguration const&, blink::WebMediaConstraints const&) 6 0xe2ce3d09 7 0xe2ce3846 8 0xe32e2ed9 9 0xe32e21fb 10 0xf005197a 11 0xf0099a00 12 0x5270c15c 13 0x5272d9b6 14 0x52740518 15 0x527402c4 16 0x52740190 17 0x5272da9e 18 0x5272a59f 19 0xf02f1760 20 0xf02f0b64 21 0xeffefe92 v8::Script::Run(v8::Local<v8::Context>) 22 0xe3d0a79c blink::V8ScriptRunner::runCompiledScript(v8::Isolate*, v8::Local<v8::Script>, blink::ExecutionContext*) 23 0xe3c4f4a6 blink::ScriptController::executeScriptAndReturnValue(v8::Local<v8::Context>, blink::ScriptSourceCode const&, blink::AccessControlStatus, double*) 24 0xe3c5214d blink::ScriptController::evaluateScriptInMainWorld(blink::ScriptSourceCode const&, blink::AccessControlStatus, blink::ScriptController::ExecuteScriptPolicy, double*) 25 0xe3c524e9 blink::ScriptController::executeScriptInMainWorldAndReturnValue(blink::ScriptSourceCode const&) 26 0xec860f96 27 0xf29f2b67 content::RenderFrameImpl::OnJavaScriptExecuteRequestForTests(std::basic_string<unsigned short, base::string16_char_traits, std::allocator<unsigned short> > const&, int, bool, bool) 28 0xf2a2b98b 29 0xf2a2b829 30 0xf2a181be 31 0xf29ef105 content::RenderFrameImpl::OnMessageReceived(IPC::Message const&) Received signal 11 SEGV_MAPERR 0000fbadbeef #0 0x0000eb12f924 base::debug::StackTrace::StackTrace() #1 0x0000eb12f44b base::debug::(anonymous namespace)::StackDumpSignalHandler() #2 0x0000f76e5420 ([vdso]+0x41f) #3 0x0000f5b3b64c blink::failedAssertion() #4 0x0000f2d690c1 blink::WebPrivatePtr<>::~WebPrivatePtr() #5 0x0000f2d68dcb blink::WebRTCICEServerArray::~WebRTCICEServerArray() #6 0x0000f2d4dbed content::(anonymous namespace)::GetNativeRtcConfiguration() #7 0x0000f2d4d66c content::RTCPeerConnectionHandler::initialize() #8 0x0000e2ce3d09 blink::RTCPeerConnection::RTCPeerConnection() #9 0x0000e2ce3846 blink::RTCPeerConnection::create() #10 0x0000e32e2ed9 blink::RTCPeerConnectionV8Internal::constructor() #11 0x0000e32e21fb blink::V8RTCPeerConnection::constructorCallback() #12 0x0000f005197a v8::internal::FunctionCallbackArguments::Call() #13 0x0000f0099a00 v8::internal::Builtin_HandleApiCallConstruct() #14 0x00005270c15c <unknown> #15 0x00005272d9b6 <unknown> #16 0x000052740518 <unknown> #17 0x0000527402c4 <unknown> #18 0x000052740190 <unknown> #19 0x00005272da9e <unknown> #20 0x00005272a59f <unknown> #21 0x0000f02f1760 v8::internal::Invoke() #22 0x0000f02f0b64 v8::internal::Execution::Call() #23 0x0000effefe92 v8::Script::Run() #24 0x0000e3d0a79c blink::V8ScriptRunner::runCompiledScript() #25 0x0000e3c4f4a6 blink::ScriptController::executeScriptAndReturnValue() #26 0x0000e3c5214d blink::ScriptController::evaluateScriptInMainWorld() #27 0x0000e3c524e9 blink::ScriptController::executeScriptInMainWorldAndReturnValue() #28 0x0000ec860f96 blink::WebLocalFrameImpl::executeScriptAndReturnValue() #29 0x0000f29f2b67 content::RenderFrameImpl::OnJavaScriptExecuteRequestForTests() #30 0x0000f2a2b98b _ZN4base20DispatchToMethodImplIN7content15RenderFrameImplEMS2_FvRKSbItNS_20string16_char_traitsESaItEEibbEJS5_ibbEJLj0ELj1ELj2ELj3EEEEvPT_T0_RKNS_5TupleIJDpT1_EEENS_13IndexSequenceIJXspT2_EEEE #31 0x0000f2a2b829 _ZN4base16DispatchToMethodIN7content15RenderFrameImplEMS2_FvRKSbItNS_20string16_char_traitsESaItEEibbEJS5_ibbEEEvPT_T0_RKNS_5TupleIJDpT1_EEE #32 0x0000f2a181be FrameMsg_JavaScriptExecuteRequestForTests::Dispatch<>() #33 0x0000f29ef105 content::RenderFrameImpl::OnMessageReceived() #34 0x0000f279d277 content::MessageRouter::RouteMessage() #35 0x0000f279d1d8 content::MessageRouter::OnMessageReceived() #36 0x0000f21dd5bc content::ChildThreadImpl::OnMessageReceived() #37 0x0000e8da2c32 IPC::ChannelProxy::Context::OnDispatchMessage() #38 0x0000e8da9073 base::internal::RunnableAdapter<>::Run(). |
