Bug 173120 – Filter SDP from ICE candidates in case of local ICE candidate filtering

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[youenn fablet]

Filter SDP from ICE candidates in case of local ICE candidate filtering

[youenn fablet]

Created attachment 312358 [details]
Patch

[Jon Lee]

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 ?

[Jon Lee]

rdar://problem/32653631

[Eric Carlson]

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.

[Jon Lee]

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?

[Jon Lee]

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.

[youenn fablet]

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?

[youenn fablet]

Comment on attachment 312358 [details]
Patch

I l clean the tests to make them more readable. Putting patch in r? in the meantime.

[Sam Weinig]

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).

[youenn fablet]

Created attachment 312613 [details]
Patch

[youenn fablet]

> 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.

[WebKit Commit Bot]

Comment on attachment 312613 [details]
Patch

Clearing flags on attachment: 312613

Committed r218168: <http://trac.webkit.org/changeset/218168>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.