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.
Created attachment 344601 [details] Patch
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.
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 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
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
Created attachment 344615 [details] Patch
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
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 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?
Created attachment 344697 [details] Patch They are strings,
(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 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
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
Created attachment 344776 [details] Patch
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
Created attachment 345571 [details] Patch
(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 on attachment 345571 [details] Patch Clearing flags on attachment: 345571 Committed r234138: <https://trac.webkit.org/changeset/234138>
All reviewed patches have been landed. Closing bug.
<rdar://problem/42532854>