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

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Adam Bergkvist]

MediaEndpointOwr is an implementation of the MediaEndpoint interface that uses the GStreamer-based OpenWebRTC framework. The plan is to add a manual test.

[Adam Bergkvist]

Created attachment 292061 [details]
Proposed patch

[Adam Bergkvist]

Comment on attachment 292061 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=292061&action=review

> ManualTests/webrtc-one-tab-p2p.html:310
> +<p>Click start to request user media. The same stream is sent in both directions so a successful bidirectional media setup shows the same output in all four video elements. Open console to view signaling details. Some browsers only allow access to user media via a secure origin (e.g. localhost).</p>

Will wrap this line in a follow-up patch.

[Philippe Normand]

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!

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

typo: till

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

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

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

Ditto :)

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

Replace with an ASSERT ?

> Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:288
> +            printf("updateSendConfiguration: no payloads\n");

Remove this one or ASSERT ?

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

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

useRtcpMux

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

> Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:559
> +                unsigned short port = url.port ? url.port : 3478;

Use a define for the default port

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

ditto?

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

:)

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

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

[Adam Bergkvist]

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

[Adam Bergkvist]

Created attachment 292236 [details]
Updated patch

[Philippe Normand]

Comment on attachment 292236 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=292236&action=review

> Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:352
> +    owr_media_session_set_send_source(OWR_MEDIA_SESSION(transceiver->session()), owrSource.mediaSource());

So this won't work then? Maybe replace with notImplemented() ?

[Philippe Normand]

(In reply to comment #4)
> > > 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).

typedef struct _Foo Foo;

We do this in some of the player headers for Gst structs.
The issue usually is with C enums that can't be forward-declared.

[Adam Bergkvist]

Created attachment 292334 [details]
Updated patch

[Adam Bergkvist]

(In reply to comment #6)
> Comment on attachment 292236 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292236&action=review
> 
> > Source/WebCore/platform/mediastream/openwebrtc/MediaEndpointOwr.cpp:352
> > +    owr_media_session_set_send_source(OWR_MEDIA_SESSION(transceiver->session()), owrSource.mediaSource());
> 
> So this won't work then? Maybe replace with notImplemented() ?

Fixed.

(In reply to comment #7)
> (In reply to comment #4)
> > > > 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).
> 
> typedef struct _Foo Foo;
> 
> We do this in some of the player headers for Gst structs.
> The issue usually is with C enums that can't be forward-declared.

Fixed. We need owr_session.h in the header for the OwrIceState enum.

[Adam Bergkvist]

Comment on attachment 292334 [details]
Updated patch

Thanks for reviewing Philippe :)

[WebKit Commit Bot]

Comment on attachment 292334 [details]
Updated patch

Clearing flags on attachment: 292334

Committed r207665: <http://trac.webkit.org/changeset/207665>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.