<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:pnormand@igalia.com" title="Philippe Normand <pnormand@igalia.com>"> <span class="fn">Philippe Normand</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - WebRTC: [GTK] Add MediaEndpointOwr - an OpenWebRTC WebRTC backend"
href="https://bugs.webkit.org/show_bug.cgi?id=163327">bug 163327</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #292061 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - WebRTC: [GTK] Add MediaEndpointOwr - an OpenWebRTC WebRTC backend"
href="https://bugs.webkit.org/show_bug.cgi?id=163327#c3">Comment # 3</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - WebRTC: [GTK] Add MediaEndpointOwr - an OpenWebRTC WebRTC backend"
href="https://bugs.webkit.org/show_bug.cgi?id=163327">bug 163327</a>
from <span class="vcard"><a class="email" href="mailto:pnormand@igalia.com" title="Philippe Normand <pnormand@igalia.com>"> <span class="fn">Philippe Normand</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=292061&action=diff" name="attach_292061" title="Proposed patch">attachment 292061</a> <a href="attachment.cgi?id=292061&action=edit" title="Proposed patch">[details]</a></span>
Proposed patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=292061&action=review">https://bugs.webkit.org/attachment.cgi?id=292061&action=review</a>
Nice patch! Looking forward having it in trunk :) Was this tested with a debug build too? Please also check the comments inline!
<span class="quote">> Source/WebCore/ChangeLog:10
> + is till done with MockMediaEndpoint.</span >
typo: till
<span class="quote">> Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:88
> + g_free(m_dtlsPrivateKey);
> + g_free(m_dtlsCertificate);
> +
> + g_regex_unref(m_helperServerRegEx);</span >
Could we use smart pointers for those?
<span class="quote">> Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:113
> + payload->setEncodingName("OPUS");</span >
What if we don't have the Opus gst plugin? I suspect we should build the vector depending on the codecs and (de)payloaders available. Maybe open a new bug about this and link it here.
<span class="quote">> Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:141
> + payload->setEncodingName("H264");</span >
Ditto :)
<span class="quote">> Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:260
> + printf("updateSendConfiguration: BAD missing configuration element for %d\n", i);</span >
Replace with an ASSERT ?
<span class="quote">> Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:288
> + printf("updateSendConfiguration: no payloads\n");</span >
Remove this one or ASSERT ?
<span class="quote">> Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:356
> + // FIXME: An OWR bug prevents this from succeeding</span >
details? Is there a github OWR issue for this?
<span class="quote">> Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:478
> + bool useRtpMux = !isInitiator && mediaDescription->rtcpMux();</span >
useRtcpMux
<span class="quote">> Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:557
> + parseHelperServerUrl(m_helperServerRegEx, webkitUrl, url);</span >
I suppose the regex could be managed here locally instead of having it as class member?
<span class="quote">> Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:559
> + unsigned short port = url.port ? url.port : 3478;</span >
Use a define for the default port
<span class="quote">> Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:658
> + iceCandidate->setPort(port ? port : 9);</span >
ditto?
<span class="quote">> Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:669
> + iceCandidate->setRelatedPort(relatedPort ? relatedPort : 9);</span >
:)
<span class="quote">> Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:680
> + g_free(foundation);
> + g_free(address);
> + g_free(relatedAddress);</span >
Smart pointers could be used for these, I think.
<span class="quote">> Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.h:37
> +#include <owr/owr_media_session.h>
> +#include <owr/owr_transport_agent.h></span >
Maybe use forward declarations of the structs needed and move includes to the .cpp file, if possible :)</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>