[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

Philippe Normand <pnormand at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #292061|review?                     |review-
              Flags|                            |

--- Comment #3 from Philippe Normand <pnormand at igalia.com> ---
Comment on attachment 292061
  --> https://bugs.webkit.org/attachment.cgi?id=292061
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 :)

-- 
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/e03eb57b/attachment-0001.html>