<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<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#c4">Comment # 4</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:adam.bergkvist@ericsson.com" title="Adam Bergkvist <adam.bergkvist@ericsson.com>"> <span class="fn">Adam Bergkvist</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=163327#c3">comment #3</a>)
<span class="quote">> 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 >
Thanks for reviewing. (Now) it's successfully tested with a debug build. :)
<span class="quote">> > Source/WebCore/ChangeLog:10
> > + is till done with MockMediaEndpoint.
>
> typo: till</span >
Fixed.
<span class="quote">> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:88
> > + g_free(m_dtlsPrivateKey);
> > + g_free(m_dtlsCertificate);
> > +
> > + g_regex_unref(m_helperServerRegEx);
>
> Could we use smart pointers for those?</span >
Fixed first two (using WTFString).
The third can't directly be put into a unique_ptr and I'm not sure it's worth going the length to fix it since it's a temporary solution until WebKit's URL class can handle the ICE helper server urls properly.
<span class="quote">> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:113
> > + payload->setEncodingName("OPUS");
>
> 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 >
True. Added FIXME and bug.
<span class="quote">> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:141
> > + payload->setEncodingName("H264");
>
> Ditto :)</span >
See above.
<span class="quote">> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:260
> > + printf("updateSendConfiguration: BAD missing configuration element for %d\n", i);
>
> Replace with an ASSERT ?</span >
Removed (was debug logging)
<span class="quote">> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:288
> > + printf("updateSendConfiguration: no payloads\n");
>
> Remove this one or ASSERT ?</span >
Removed (was debug logging)
<span class="quote">> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:356
> > + // FIXME: An OWR bug prevents this from succeeding
>
> details? Is there a github OWR issue for this?</span >
Yes. Added a link.
<span class="quote">> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:478
> > + bool useRtpMux = !isInitiator && mediaDescription->rtcpMux();
>
> useRtcpMux</span >
Fixed.
<span class="quote">> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:557
> > + parseHelperServerUrl(m_helperServerRegEx, webkitUrl, url);
>
> I suppose the regex could be managed here locally instead of having it as
> class member?</span >
Having it as class member has the benefit that we only need to compile the regex once instead of every time the parseHelperServerUrl is called. I don't have a strong preference though. Left as is for now.
<span class="quote">> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:559
> > + unsigned short port = url.port ? url.port : 3478;
>
> Use a define for the default port</span >
Fixed.
<span class="quote">> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:658
> > + iceCandidate->setPort(port ? port : 9);
>
> ditto?</span >
Fixed.
<span class="quote">> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:669
> > + iceCandidate->setRelatedPort(relatedPort ? relatedPort : 9);
>
> :)</span >
Fixed.
<span class="quote">> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:680
> > + g_free(foundation);
> > + g_free(address);
> > + g_free(relatedAddress);
>
> Smart pointers could be used for these, I think.</span >
We could put these in WTFString objects, but that would imply copying the data and still freeing the gchar* strings, since it doesn't seem possible to String::adopt() a plain char*. This would also require alternative names for the WTFString versions. I think it would be worth the extra work if the function had multiple exit points (to avoid forgetting a free), but it probably never will since it's just a bunch of set-operations.
<span class="quote">> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.h:37
> > +#include <owr/owr_media_session.h>
> > +#include <owr/owr_transport_agent.h>
>
> Maybe use forward declarations of the structs needed and move includes to
> the .cpp file, if possible :)</span >
It made a try, but it gets a bit messy since all the OWR types are typedefs (of structs).</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>