| Summary: | Filter SDP from ICE candidates in case of local ICE candidate filtering | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||
| Component: | Media | Assignee: | youenn fablet <youennf> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | commit-queue, eric.carlson, jonlee, sam, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
youenn fablet
2017-06-08 15:46:40 PDT
Created attachment 312358 [details]
Patch
Comment on attachment 312358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312358&action=review > LayoutTests/webrtc/datachannel/filter-ice-candidate.html:47 > + assert_equals(pc.localDescription.sdp.indexOf("a:=candidate"), -1); shouldn't this be a=candidate ? Comment on attachment 312358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312358&action=review > Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:47 > +String filterICECandidate(String&&); Nit: This can be static. >> LayoutTests/webrtc/datachannel/filter-ice-candidate.html:47 >> + assert_equals(pc.localDescription.sdp.indexOf("a:=candidate"), -1); > > shouldn't this be a=candidate ? I agree. Comment on attachment 312358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312358&action=review >> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:47 >> +String filterICECandidate(String&&); > > Nit: This can be static. I think you can revert this... > Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:306 > +String filterICECandidate(String&& sdp) ... and this. > LayoutTests/webrtc/datachannel/filter-ice-candidate.html:34 > + }); I'm confused by these tests. Above there are potentially two resolve() calls. Is that right? Why is filtering set to true in this block, and not set in the next promise_test(), for parallelism? Comment on attachment 312358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312358&action=review >> LayoutTests/webrtc/datachannel/filter-ice-candidate.html:34 >> + }); > > I'm confused by these tests. Above there are potentially two resolve() calls. Is that right? Why is filtering set to true in this block, and not set in the next promise_test(), for parallelism? It seems that the test isn't reaching some of these asserts. I am able to change the assertions to clearly wrong conditions, and the test still passes. Comment on attachment 312358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312358&action=review >>> LayoutTests/webrtc/datachannel/filter-ice-candidate.html:34 >>> + }); >> >> I'm confused by these tests. Above there are potentially two resolve() calls. Is that right? Why is filtering set to true in this block, and not set in the next promise_test(), for parallelism? > > It seems that the test isn't reaching some of these asserts. I am able to change the assertions to clearly wrong conditions, and the test still passes. The second resolve is a noop and should be removed. We could change the place where we set the filtering to true. That might be indeed clearer but will not change any behavior here. Can you explain which assertions you changed? Comment on attachment 312358 [details]
Patch
I l clean the tests to make them more readable. Putting patch in r? in the meantime.
Comment on attachment 312358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312358&action=review > Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:355 > + Vector<String> lines; > + sdp.split('\n', false, lines); > + > + StringBuilder builder; > + for (auto& line : lines) { > + if (!line.contains("a=candidate")) > + builder.append(line); > + else if (!line.contains(" host ")) > + builder.append(filterICECandidate(WTFMove(line))); > + else > + continue; > + builder.append('\n'); > + } > + return builder.toString(); I'm not sure how hot this is, but a lot of unnecessary String allocations are taking place. A split that took a functor, something like spd.split('\n', false, [] (StringView line) { }) would allow many fewer allocations. (See splitOnSpaces in SubresourceIntegrity.cpp for non-generalized version of this). Created attachment 312613 [details]
Patch
> I'm not sure how hot this is, but a lot of unnecessary String allocations
> are taking place. A split that took a functor, something like
> spd.split('\n', false, [] (StringView line) { }) would allow many fewer
> allocations. (See splitOnSpaces in SubresourceIntegrity.cpp for
> non-generalized version of this).
It is not very hot, it will happen once for a webrtc page typically.
But this is a good idea anyway and some other places might benefit from it as well.
Patch adds a split helper routine here.
In the same patch, I cleaned the tests as discussed above.
I added an additional assert after candidates have been gathered.
Comment on attachment 312613 [details] Patch Clearing flags on attachment: 312613 Committed r218168: <http://trac.webkit.org/changeset/218168> All reviewed patches have been landed. Closing bug. |