Bug 186932

Summary: [WPE][GTK] Implement PeerConnection API on top of libwebrtc
Product: WebKit Reporter: Thibault Saunier <tsaunier>
Component: WebRTCAssignee: Thibault Saunier <tsaunier>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, commit-queue, ews-watchlist, pnormand, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 187302, 187303    
Bug Blocks: 187064, 187643    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews106 for mac-sierra-wk2
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews102 for mac-sierra
none
Patch
none
Archive of layout-test-results from ews206 for win-future
none
Patch
none
Patch none

Description Thibault Saunier 2018-06-22 10:54:15 PDT
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.
Comment 1 Thibault Saunier 2018-07-09 12:16:05 PDT
Created attachment 344601 [details]
Patch
Comment 2 EWS Watchlist 2018-07-09 13:19:53 PDT
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.
Comment 3 EWS Watchlist 2018-07-09 13:19:55 PDT
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
Comment 4 EWS Watchlist 2018-07-09 14:04:32 PDT
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
Comment 5 EWS Watchlist 2018-07-09 14:04:34 PDT
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
Comment 6 Thibault Saunier 2018-07-09 14:06:04 PDT
Created attachment 344615 [details]
Patch
Comment 7 EWS Watchlist 2018-07-09 16:21:06 PDT
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
Comment 8 EWS Watchlist 2018-07-09 16:21:08 PDT
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
Comment 9 Philippe Normand 2018-07-10 01:38:20 PDT
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?
Comment 10 Thibault Saunier 2018-07-10 07:58:14 PDT
Created attachment 344697 [details]
Patch

They are strings,
Comment 11 Thibault Saunier 2018-07-10 07:58:53 PDT
(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,
Comment 12 EWS Watchlist 2018-07-10 19:09:20 PDT
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
Comment 13 EWS Watchlist 2018-07-10 19:09:32 PDT
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
Comment 14 Thibault Saunier 2018-07-11 12:31:45 PDT
Created attachment 344776 [details]
Patch
Comment 15 Philippe Normand 2018-07-19 01:45:30 PDT
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
Comment 16 Thibault Saunier 2018-07-23 07:50:10 PDT
Created attachment 345571 [details]
Patch
Comment 17 Thibault Saunier 2018-07-23 07:52:50 PDT
(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.
Comment 18 WebKit Commit Bot 2018-07-24 01:24:42 PDT
Comment on attachment 345571 [details]
Patch

Clearing flags on attachment: 345571

Committed r234138: <https://trac.webkit.org/changeset/234138>
Comment 19 WebKit Commit Bot 2018-07-24 01:24:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Radar WebKit Bug Importer 2018-07-24 01:26:16 PDT
<rdar://problem/42532854>