Bug 163327

Created attachment 292061 [details] Proposed patch

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.

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

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

Created attachment 292236 [details] Updated patch

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

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

Created attachment 292334 [details] Updated patch

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

Comment on attachment 292334 [details] Updated patch Thanks for reviewing Philippe :)

Comment on attachment 292334 [details] Updated patch Clearing flags on attachment: 292334 Committed r207665: <http://trac.webkit.org/changeset/207665>

All reviewed patches have been landed. Closing bug.