[Webkit-unassigned] [Bug 163327] WebRTC: [GTK] Add MediaEndpointOwr - an OpenWebRTC WebRTC backend

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=163327

--- Comment #4 from Adam Bergkvist <adam.bergkvist at ericsson.com> ---
(In reply to comment #3)
> Comment on attachment 292061 [details]
> Proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292061&action=review
> 
> Nice patch! Looking forward having it in trunk :) Was this tested with a
> debug build too? Please also check the comments inline!

Thanks for reviewing. (Now) it's successfully tested with a debug build. :)

> > Source/WebCore/ChangeLog:10
> > +        is till done with MockMediaEndpoint.
> 
> typo: till

Fixed.

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

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.

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

True. Added FIXME and bug.

> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:141
> > +    payload->setEncodingName("H264");
> 
> Ditto :)

See above.

> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:260
> > +            printf("updateSendConfiguration: BAD missing configuration element for %d\n", i);
> 
> Replace with an ASSERT ?

Removed (was debug logging)

> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:288
> > +            printf("updateSendConfiguration: no payloads\n");
> 
> Remove this one or ASSERT ?

Removed (was debug logging)

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

Yes. Added a link.

> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:478
> > +    bool useRtpMux = !isInitiator && mediaDescription->rtcpMux();
> 
> useRtcpMux

Fixed.

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

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.

> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:559
> > +                unsigned short port = url.port ? url.port : 3478;
> 
> Use a define for the default port

Fixed.

> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:658
> > +    iceCandidate->setPort(port ? port : 9);
> 
> ditto?

Fixed.

> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:669
> > +        iceCandidate->setRelatedPort(relatedPort ? relatedPort : 9);
> 
> :)

Fixed.

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

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. 

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

It made a try, but it gets a bit messy since all the OWR types are typedefs (of structs).

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20161020/16254e2b/attachment.html>