Bug 186932 – [WPE][GTK] Implement PeerConnection API on top of libwebrtc

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[Thibault Saunier]

The getUserMedia API supporte for WebkitGTK/WPE landed recently as part of Bug #185787 and we are now working on implementing the WebRTC/PeerConnection part.

[Thibault Saunier]

Created attachment 344601 [details]
Patch

[EWS Watchlist]

Comment on attachment 344601 [details]
Patch

Attachment 344601 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8484754

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 344606 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 344601 [details]
Patch

Attachment 344601 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8484850

New failing tests:
webrtc/libwebrtc/descriptionGetters.html

[EWS Watchlist]

Created attachment 344614 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Thibault Saunier]

Created attachment 344615 [details]
Patch

[EWS Watchlist]

Comment on attachment 344615 [details]
Patch

Attachment 344615 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8487017

New failing tests:
media/media-fullscreen-return-to-inline.html

[EWS Watchlist]

Created attachment 344636 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Philippe Normand]

Comment on attachment 344615 [details]
Patch

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

> Source/WebCore/platform/GStreamer.cmake:4
> +        "${WEBCORE_DIR}/platform/graphics/gstreamer"
>          "${WEBCORE_DIR}/platform/graphics/gstreamer"

No need for duplicates :)

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.cpp:52
> +            (gpointer)comps[i], compsize, 0, compsize, webrtcbuffer, nullptr);

C cast

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.cpp:57
> +    auto sample = adoptGRef(gst_sample_new(buffer.get(), caps.get(), nullptr, nullptr));

Instead of using a memory for each plane, would it be simpler to use a single memory and keep track of offsets/stride with a video meta?

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.cpp:108
> +        GST_INFO("RealtimeOutgoingVideoSourceGStreamer::videoSampleAvailable unable to allocate buffer for conversion to YUV");

Should this be a warning or error instead?

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.h:39
> +    GStreamerVideoFrameLibWebRTC(GstSample *sample, GstVideoInfo info)

Misplaced *

> Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.h:49
> +    int width() const override { return GST_VIDEO_INFO_WIDTH (&m_info); }
> +    int height() const override { return GST_VIDEO_INFO_HEIGHT (&m_info); }

No space before ( please :)

> Source/WebCore/platform/mediastream/gstreamer/RealtimeIncomingAudioSourceLibWebRTC.cpp:63
> +        LibWebRTCAudioFormat::sampleSize,
> +        LibWebRTCAudioFormat::sampleSize);

depth == width here really?

> Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingAudioSourceLibWebRTC.cpp:57
> +        LibWebRTCAudioFormat::sampleSize,
> +        LibWebRTCAudioFormat::sampleSize);

Same question, also this appears to be the same code, maybe factor out in a new function?

> Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingAudioSourceLibWebRTC.cpp:75
> +        GST_ERROR_OBJECT(this, "FIXME - Audio format renegotiation is not possible yet!");

GST_FIXME_OBJECT? :)

> Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingAudioSourceLibWebRTC.cpp:132
> +        goto cleanupMaps;

To avoid the goto you could change the test to if (gst_audio_....())

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:43
> +// Required because of unified builds
> +#ifdef GST_CAT_DEFAULT
> +#undef GST_CAT_DEFAULT
> +#endif

why?

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:172
> +        GST_LOG_OBJECT(pipeline(), "%ld Decoding: %" GST_PTR_FORMAT, renderTimeMs, buffer);

G_GINT64_FORMAT

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:47
> +// Required for unified builds
> +#ifdef GST_CAT_DEFAULT
> +#undef GST_CAT_DEFAULT
> +#endif

:?

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:220
> +        gst_structure_get(gst_caps_get_structure(caps, 0),
> +            "width", G_TYPE_INT, &frame._encodedWidth,
> +            "height", G_TYPE_INT, &frame._encodedHeight,
> +            nullptr);

What about the pixel aspect ratio? Is it available somehow in libwebrtc?

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:243
> +    GRefPtr<GstElement> CreateEncoder(GstElement** encoder)

Hum would it be possible to use some smart pointer instead of raw pointer here?

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:280
> +    void AddCodecIfSupported(std::vector<webrtc::SdpVideoFormat>* supportedFormats)

ditto

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:503
> +    if (format.name == cricket::kVp8CodecName)
> +        return std::make_unique<VP8Encoder>(format);
> +
> +    if (format.name == cricket::kH264CodecName)
> +        return std::make_unique<H264Encoder>(format);

MAybe use a switch?

[Thibault Saunier]

Created attachment 344697 [details]
Patch

They are strings,

[Thibault Saunier]

(In reply to Philippe Normand from comment #9)
> Comment on attachment 344615 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344615&action=review
> 
> > Source/WebCore/platform/GStreamer.cmake:4
> > +        "${WEBCORE_DIR}/platform/graphics/gstreamer"
> >          "${WEBCORE_DIR}/platform/graphics/gstreamer"
> 
> No need for duplicates :)

Oops, fixed/

> > Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.cpp:52
> > +            (gpointer)comps[i], compsize, 0, compsize, webrtcbuffer, nullptr);
> 
> C cast

Fixed, I did have extra care on that this time, but not enough yet!

> > Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.cpp:57
> > +    auto sample = adoptGRef(gst_sample_new(buffer.get(), caps.get(), nullptr, nullptr));
> 
> Instead of using a memory for each plane, would it be simpler to use a
> single memory and keep track of offsets/stride with a video meta?

The memory not be contigous so using one GstMemory per plane is the right way to represent that in GStreamer I think.

> > Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.cpp:108
> > +        GST_INFO("RealtimeOutgoingVideoSourceGStreamer::videoSampleAvailable unable to allocate buffer for conversion to YUV");
> 
> Should this be a warning or error instead?

Indeed, bumped to WARNING.

> > Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.h:39
> > +    GStreamerVideoFrameLibWebRTC(GstSample *sample, GstVideoInfo info)
> 
> Misplaced *

Fixed, it is weird how the -style script sometimes find that kind of mistakes and sometime not.

> > Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoFrameLibWebRTC.h:49
> > +    int width() const override { return GST_VIDEO_INFO_WIDTH (&m_info); }
> > +    int height() const override { return GST_VIDEO_INFO_HEIGHT (&m_info); }
> 
> No space before ( please :) 

Fixed.
 
> > Source/WebCore/platform/mediastream/gstreamer/RealtimeIncomingAudioSourceLibWebRTC.cpp:63
> > +        LibWebRTCAudioFormat::sampleSize,
> > +        LibWebRTCAudioFormat::sampleSize);
> 
> depth == width here really?

Yeah, it is the case for S16LE/BE which is what libwebrtc uses.

> > Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingAudioSourceLibWebRTC.cpp:57
> > +        LibWebRTCAudioFormat::sampleSize,
> > +        LibWebRTCAudioFormat::sampleSize);
> 
> Same question, also this appears to be the same code, maybe factor out in a
> new function?

Well, it is already using 2 helpers and is 1 single function call, not sure it is worth having
another function for that.

> > Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingAudioSourceLibWebRTC.cpp:75
> > +        GST_ERROR_OBJECT(this, "FIXME - Audio format renegotiation is not possible yet!");
> 
> GST_FIXME_OBJECT? :)
> 
> > Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingAudioSourceLibWebRTC.cpp:132
> > +        goto cleanupMaps;
> 
> To avoid the goto you could change the test to if (gst_audio_....())

Indeed, done.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:43
> > +// Required because of unified builds
> > +#ifdef GST_CAT_DEFAULT
> > +#undef GST_CAT_DEFAULT
> > +#endif
> 
> why?

"because of unified builds" ie. we have a generated cpp file which `#include` several
files where GST_CAT_DEFAULT is defined... and build fails as it gets redefined.


> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:172
> > +        GST_LOG_OBJECT(pipeline(), "%ld Decoding: %" GST_PTR_FORMAT, renderTimeMs, buffer);
> 
> G_GINT64_FORMAT
> 
> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:47
> > +// Required for unified builds
> > +#ifdef GST_CAT_DEFAULT
> > +#undef GST_CAT_DEFAULT
> > +#endif
> 
> :?

Ditto.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:220
> > +        gst_structure_get(gst_caps_get_structure(caps, 0),
> > +            "width", G_TYPE_INT, &frame._encodedWidth,
> > +            "height", G_TYPE_INT, &frame._encodedHeight,
> > +            nullptr);
> 
> What about the pixel aspect ratio? Is it available somehow in libwebrtc?

Nop, it is not present and square pixels seem to be required, which makes sense to
me in that context.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:243
> > +    GRefPtr<GstElement> CreateEncoder(GstElement** encoder)
> 
> Hum would it be possible to use some smart pointer instead of raw pointer
> here?

We could but it really brings nothing to me here tbh. The ref is owned by the encodebin already.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:280
> > +    void AddCodecIfSupported(std::vector<webrtc::SdpVideoFormat>* supportedFormats)
> 
> ditto

Same answer, if you really want I can work something out.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:503
> > +    if (format.name == cricket::kVp8CodecName)
> > +        return std::make_unique<VP8Encoder>(format);
> > +
> > +    if (format.name == cricket::kH264CodecName)
> > +        return std::make_unique<H264Encoder>(format);
> 
> MAybe use a switch?

They are strings,

[EWS Watchlist]

Comment on attachment 344697 [details]
Patch

Attachment 344697 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8500382

New failing tests:
http/tests/security/local-video-source-from-remote.html

[EWS Watchlist]

Created attachment 344746 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Thibault Saunier]

Created attachment 344776 [details]
Patch

[Philippe Normand]

Comment on attachment 344776 [details]
Patch

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

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:43
> +// Required because of unified builds
> +#ifdef GST_CAT_DEFAULT
> +#undef GST_CAT_DEFAULT
> +#endif

I still wonder why this is needed. It's the first time such hack is needed in the code-base... Can't we simply disable unified build for the concerned files?

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:100
> +        g_object_set(sink, "sync", false, NULL);

Why this? Might be good to add a comment.
Also please be consistent and use nullptr. false and FALSE are different things as well, I think :)

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:103
> +        if (!gst_element_link_many(m_src, decoder, nullptr)) {

nit: Could use gst_element_link() here

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:108
> +        if (!gst_element_link_many(capsfilter, sink, nullptr)) {

ditto

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:114
> +            GST_ERROR_OBJECT(pipeline(), "Could not set state to PLAYBIN.");

typo :)

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:265
> +    std::map<GstClockTime, GstClockTime> m_dts_pts_map;

no snake_case please ;)

> Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:189
> +                auto force_key_unit = gst_video_event_new_downstream_force_key_unit(GST_CLOCK_TIME_NONE,
> +                    GST_CLOCK_TIME_NONE, GST_CLOCK_TIME_NONE, FALSE, 1);
> +
> +                if (!gst_pad_push_event(pad.get(), force_key_unit))

no need for a local variable here, I think

[Thibault Saunier]

Created attachment 345571 [details]
Patch

[Thibault Saunier]

(In reply to Philippe Normand from comment #15)
> Comment on attachment 344776 [details]
> Patch
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344776&action=review
>
> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:43
> > +// Required because of unified builds
> > +#ifdef GST_CAT_DEFAULT
> > +#undef GST_CAT_DEFAULT
> > +#endif
>
> I still wonder why this is needed. It's the first time such hack is needed
> in the code-base... Can't we simply disable unified build for the concerned
> files?

Looks like it is not needed anymore, it depends a lot on how the build is unified,
and I think that way of doing it was the rihgt solution to the problem. New files
went in and the #include order/grouping of the unified build files must have changed
and makes it unneeded, so removed.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:100
> > +        g_object_set(sink, "sync", false, NULL);
>
> Why this? Might be good to add a comment.
> Also please be consistent and use nullptr. false and FALSE are different
> things as well, I think :)

Added:
    // This is an encoder, everything should happen as fast as possible and not
    // be synced on the clock.

And fixed the usage of NULL/TRUE.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:103
> > +        if (!gst_element_link_many(m_src, decoder, nullptr)) {
>
> nit: Could use gst_element_link() here
>
> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:108
> > +        if (!gst_element_link_many(capsfilter, sink, nullptr)) {
>
> ditto

Fixed.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:114
> > +            GST_ERROR_OBJECT(pipeline(), "Could not set state to PLAYBIN.");
>
> typo :)

Fixed.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoDecoderFactory.cpp:265
> > +    std::map<GstClockTime, GstClockTime> m_dts_pts_map;
>
> no snake_case please ;)

Fixed.

> > Source/WebCore/platform/mediastream/libwebrtc/GStreamerVideoEncoderFactory.cpp:189
> > +                auto force_key_unit = gst_video_event_new_downstream_force_key_unit(GST_CLOCK_TIME_NONE,
> > +                    GST_CLOCK_TIME_NONE, GST_CLOCK_TIME_NONE, FALSE, 1);
> > +
> > +                if (!gst_pad_push_event(pad.get(), force_key_unit))
>
> no need for a local variable here, I think

I added it to make it simpler to read. Fixed the snake_case name.

[WebKit Commit Bot]

Comment on attachment 345571 [details]
Patch

Clearing flags on attachment: 345571

Committed r234138: <https://trac.webkit.org/changeset/234138>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42532854>