Bug 185787

Created attachment 341758 [details] [GTK][WPE] Start implementing MediaStream API This patch implements the MediaStream API for the GTK+ and WPE ports, introducing all the required classes. Parts of this patche have already been propose on https://bugs.webkit.org/show_bug.cgi?id=185761 but as discussed there the patch proposal splitting was changed so that the patch introducing new API to handle enumerateDevices is a patch on its own. To make sure no previous review is lost in the process, this patch already adresses previous reviews, and I am replying here to Philip's comment from https://bugs.webkit.org/show_bug.cgi?id=185761: (In reply to Philippe Normand from comment #5) | from #185761 > Comment on attachment 340841 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=340841&action=review > > I did a first pass on the GStreamer bits. > > > Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioCaptureSource.cpp:17 > > + * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR > > Hum I don't think we want the Apple version of the header. For every new > file please make sure to use the standard BSD header. Fixed on all added files. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioCaptureSource.cpp:135 > > +void GStreamerAudioCaptureSource::stopProducingData() > > This should undo what startProducingData() does I think? Namely disconnect > the new-sample signal. Fixed. Same fixed applied to GStreamerVideoCaptureSource::stopProducingData. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioCaptureSource.cpp:142 > > + if (!m_capabilities) { > > This could become an early return. Done. Same thing in GStreamerVideoCaptureSource.cpp. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioCapturer.cpp:49 > > + // FIXME Handle errors. > > Please open a bug and mention it for every FIXME :) For this one I added an assert, it should not fail otherwise the install is wrong and playbin expects those elements anyway. I guess we should go over the FIXME right before merging and do that yes. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioStreamDescription.h:58 > > + m_platformDescription = { PlatformDescription::GStreamerAudioStreamDescription, (AudioStreamBasicDescription*) &m_info }; > > Please avoid C-style casts everywhere in C++ code. Use static_cast or > reinterpret_cast. OK, doing that where I noticed it, but ooc, why? :-) > > Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioStreamDescription.h:83 > > + double sampleRate() const final { return GST_AUDIO_INFO_RATE (&m_info); } > > No space before ( ... I wonder why check-webkit-style doesn't complain about > that :/ Fixed. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:141 > > + ASSERT_NOT_REACHED(); > > This is enabled only for debug builds I think. You'd need an early return > for release builds. Done. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:172 > > + // Only accept raw video for now. > > + if (!gst_structure_has_name(str, "video/x-raw")) > > Will be nice to support compressed formats too :) Indeed, not so simple to do but it would be neat :-)

(In reply to Thibault Saunier from comment #1) > > > > Please avoid C-style casts everywhere in C++ code. Use static_cast or > > reinterpret_cast. > > OK, doing that where I noticed it, but ooc, why? :-) > This is one of the implicit coding style guidelines of the project :) > > > > Will be nice to support compressed formats too :) > > Indeed, not so simple to do but it would be neat :-) But that's a requirement for "light" platforms like the RPi IIRC. When I was working on that it was quite challenging to achieve good perf with the RPi Camera without compressed format support.

Comment on attachment 341758 [details] [GTK][WPE] Start implementing MediaStream API View in context: https://bugs.webkit.org/attachment.cgi?id=341758&action=review > LayoutTests/platform/gtk/TestExpectations:-577 > -fast/mediastream [ Skip ] Can similar test expectation changes be applied to WPE? > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:85 > +void connectSimpleBusMessageCallback(GstElement *pipeline); * misplaced :) Also, a disconnect function would probably be nice to have? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:55 > +#include "gstreamer/GStreamerMediaStreamSource.h" Please add platform/mediastream/gstreamer to the include directories, somewhere in the CMake stuff so there's no need for gstreamer/ prefix here. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1769 > + // gst_object_ref(sourceElement); left over? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:85 > +#include "gstreamer/GStreamerMediaStreamSource.h" No prefix needed. > Source/WebCore/platform/graphics/gstreamer/VideoTrackPrivateGStreamer.cpp:54 > + if (gst_tag_list_get_int(tags, "webkit-media-stream-kind", &kind) && kind == (gint)VideoTrackPrivate::Kind::Main) { C-style cast detected :) > Source/WebCore/platform/graphics/gstreamer/VideoTrackPrivateGStreamer.cpp:56 > + gst_stream_set_stream_flags(stream.get(), (GstStreamFlags)(streamFlags | GST_STREAM_FLAG_SELECT)); Ditto > Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDevice.h:42 > + GstDevice * device() { return m_device.get(); } no space :P > Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:110 > + gst_element_link_many(source.get(), converter.get(), m_capsfilter.get(), m_tee.get(), NULL); nullptr here too please :) > Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:178 > + GRefPtr<GstBus> bus = adoptGRef(gst_pipeline_get_bus(GST_PIPELINE(pipeline()))); > + gst_bus_set_sync_handler(bus.get(), nullptr, nullptr, nullptr); As mentioned before, this could go to a new function in GStreamerCommon. > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:32 > +#include "gstreamer/GStreamerAudioData.h" No WebCore dir prefix in includes, anywhere in this patch please :) > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:37 > +#if GST_CHECK_VERSION(1, 10, 0) This could be merged with the first #if > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:43 > +#define WEBKIT_MEDIA_STREAM_SRC_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST((klass), WEBKIT_TYPE_MEDIA_STREAM_SRC, WebKitMediaStreamSrcClass)) > +#define WEBKIT_IS_MEDIA_STREAM_SRC_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE((klass), WEBKIT_TYPE_MEDIA_STREAM_SRC)) > +#define WEBKIT_MEDIA_STREAM_SRC_GET_CLASS(o) (G_TYPE_INSTANCE_GET_CLASS((o), WEBKIT_TYPE_MEDIA_STREAM_SRC, WebKitMediaStreamSrcClass)) I think our other gst custom elements define those macros in their header file. > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:70 > + (gint)AudioTrackPrivate::Kind::Main, nullptr); C-style cast. It seems the whole patch needs to be checked about this. > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:164 > + GstFlowCombiner* flow_combiner; Variables should use camelCase whenever possible > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:316 > + /** > + * WebKitMediaStreamSrcClass::is-live: > + * > + * Instruct the source to behave like a live source. This includes that it > + * will only push out buffers in the PLAYING state. > + */ No need for this, the element is private. > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:319 > + "Let playbin3 we are a live source.", +know > Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:191 > + tmpMinFramerate = (int)(tmpMinFPSNumerator / tmpMinFPSDenominator); > + tmpMaxFramerate = (int)(tmpMaxFPSNumerator / tmpMaxFPSDenominator); Framerates are integers in libwebrtc? That's a bit surprising :)

Created attachment 342085 [details] [GTK][WPE]: Implement MediaStream API (In reply to Philippe Normand from comment #3) > Comment on attachment 341758 [details] > [GTK][WPE] Start implementing MediaStream API > > View in context: > https://bugs.webkit.org/attachment.cgi?id=341758&action=review > > > LayoutTests/platform/gtk/TestExpectations:-577 > > -fast/mediastream [ Skip ] > > Can similar test expectation changes be applied to WPE? This will be a follow up patch, we are not ready for that yet but I am working on it. > > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:85 > > +void connectSimpleBusMessageCallback(GstElement *pipeline); > > * misplaced :) > Also, a disconnect function would probably be nice to have? Added, also fixed a place where we forgot to disconnect (which would have lead to a leak iirc). > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:55 > > +#include "gstreamer/GStreamerMediaStreamSource.h" > > Please add platform/mediastream/gstreamer to the include directories, > somewhere in the CMake stuff so there's no need for gstreamer/ prefix here. Done. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1769 > > + // gst_object_ref(sourceElement); > > left over? Indeed sorry, FIXED. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:85 > > +#include "gstreamer/GStreamerMediaStreamSource.h" > > No prefix needed. FIXED. > > Source/WebCore/platform/graphics/gstreamer/VideoTrackPrivateGStreamer.cpp:54 > > + if (gst_tag_list_get_int(tags, "webkit-media-stream-kind", &kind) && kind == (gint)VideoTrackPrivate::Kind::Main) { > > C-style cast detected :) Fixed > > Source/WebCore/platform/graphics/gstreamer/VideoTrackPrivateGStreamer.cpp:56 > > + gst_stream_set_stream_flags(stream.get(), (GstStreamFlags)(streamFlags | GST_STREAM_FLAG_SELECT)); > > Ditto Fixed. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDevice.h:42 > > + GstDevice * device() { return m_device.get(); } > > no space :P Fixed. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:110 > > + gst_element_link_many(source.get(), converter.get(), m_capsfilter.get(), m_tee.get(), NULL); > > nullptr here too please :) Fixed, I wonder how the style checker missed it oO > > > Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:178 > > + GRefPtr<GstBus> bus = adoptGRef(gst_pipeline_get_bus(GST_PIPELINE(pipeline()))); > > + gst_bus_set_sync_handler(bus.get(), nullptr, nullptr, nullptr); > > As mentioned before, this could go to a new function in GStreamerCommon. It is unrelated as this is for the sync message handler. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:32 > > +#include "gstreamer/GStreamerAudioData.h" > > No WebCore dir prefix in includes, anywhere in this patch please :) Fixed, looks like you spotted the 3 instances :-) > > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:37 > > +#if GST_CHECK_VERSION(1, 10, 0) > > This could be merged with the first #if Problem was that <gst/gst.h> was not included at that point, changed it this way: ``` ``` > > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:43 > > +#define WEBKIT_MEDIA_STREAM_SRC_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST((klass), WEBKIT_TYPE_MEDIA_STREAM_SRC, WebKitMediaStreamSrcClass)) > > +#define WEBKIT_IS_MEDIA_STREAM_SRC_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE((klass), WEBKIT_TYPE_MEDIA_STREAM_SRC)) > > +#define WEBKIT_MEDIA_STREAM_SRC_GET_CLASS(o) (G_TYPE_INSTANCE_GET_CLASS((o), WEBKIT_TYPE_MEDIA_STREAM_SRC, WebKitMediaStreamSrcClass)) > > I think our other gst custom elements define those macros in their header > file. Removed them, they were not used in the end. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:70 > > + (gint)AudioTrackPrivate::Kind::Main, nullptr); > > C-style cast. It seems the whole patch needs to be checked about this. Rebuilt the modified c++ files with `-Wold-style-cast` and tried to go over mine (but there were **many** issues from random headers). > > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:164 > > + GstFlowCombiner* flow_combiner; > > Variables should use camelCase whenever possible Fixed a few I noticed. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:316 > > + /** > > + * WebKitMediaStreamSrcClass::is-live: > > + * > > + * Instruct the source to behave like a live source. This includes that it > > + * will only push out buffers in the PLAYING state. > > + */ > > No need for this, the element is private. Removed. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:319 > > + "Let playbin3 we are a live source.", > > +know > > > Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:191 > > + tmpMinFramerate = (int)(tmpMinFPSNumerator / tmpMinFPSDenominator); > > + tmpMaxFramerate = (int)(tmpMaxFPSNumerator / tmpMaxFPSDenominator); > > Framerates are integers in libwebrtc? That's a bit surprising :) Crazy thing happen there :P (or we forgot to wake up that day :D). Fixed.

Comment on attachment 342085 [details] [GTK][WPE]: Implement MediaStream API View in context: https://bugs.webkit.org/attachment.cgi?id=342085&action=review Ok I think this is almost ready to land. Is this patch supposed to apply on trunk? The EWS fails. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:55 > +#include "gstreamer/GStreamerMediaStreamSource.h" No prefix please ;) > Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:177 > + GST_INFO_OBJECT((gpointer) pipeline(), "Going to PLAYING!"); This cast is really needed? > Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:186 > + GST_INFO_OBJECT((gpointer) pipeline(), "Tearing down!"); Ditto > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:302 > + GstElementClass* gstelement_klass = (GstElementClass*)klass; Use GST_ELEMENT_CLASS(klass) > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:356 > + (GstPadChainFunction)webkitMediaStreamSrcChain); You'll think I'm obsessed with casts ;) > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:358 > + g_assert(gst_element_add_pad(GST_ELEMENT(self), (GstPad*)ghostpad)); GST_PAD_CAST() :) Also please use ASSERT instead of g_assert

Created attachment 342160 [details] Addressed comments. (In reply to Philippe Normand from comment #5) > Comment on attachment 342085 [details] > [GTK][WPE]: Implement MediaStream API > > View in context: > https://bugs.webkit.org/attachment.cgi?id=342085&action=review > > Ok I think this is almost ready to land. Is this patch supposed to apply on > trunk? The EWS fails. Now that the GstStreamCollection management rework landed, it should apply. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:55 > > +#include "gstreamer/GStreamerMediaStreamSource.h" > > No prefix please ;) Sorry, FIXED. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:177 > > + GST_INFO_OBJECT((gpointer) pipeline(), "Going to PLAYING!"); > > This cast is really needed? No, removed. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:186 > > + GST_INFO_OBJECT((gpointer) pipeline(), "Tearing down!"); > > Ditto Ditto. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:302 > > + GstElementClass* gstelement_klass = (GstElementClass*)klass; > > Use GST_ELEMENT_CLASS(klass) > > > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:356 > > + (GstPadChainFunction)webkitMediaStreamSrcChain); > > You'll think I'm obsessed with casts ;) Fixed. I went over all code manually and tried to fix them all, hopefully I didn't miss any, but that C macro synthax is hard to detect! (no way to grep it seems). > > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:358 > > + g_assert(gst_element_add_pad(GST_ELEMENT(self), (GstPad*)ghostpad)); > > GST_PAD_CAST() :) > Also please use ASSERT instead of g_assert Done. Removed all occurences of g_assrt.

Attachment 342160 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:25: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:29: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:25: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:29: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:35: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:358: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:186: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:187: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:192: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:193: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:206: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 11 in 43 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 342168 [details] Fix styling issues.

I had to revert my change for the `#if GST_CHECK_VERSION(1,14,0)` as it requires `gst.h` to be included and including it before local includes is invalid stylewise.

Comment on attachment 342168 [details] Fix styling issues. View in context: https://bugs.webkit.org/attachment.cgi?id=342168&action=review > Source/WebCore/platform/GStreamer.cmake:51 > + platform/mock/MockRealtimeAudioSource.cpp > + platform/mock/MockRealtimeMediaSource.cpp > + platform/mock/MockRealtimeMediaSourceCenter.cpp > + platform/mock/MockRealtimeVideoSource.cpp Hum, this should move to a different cmake file I think, they're not related with GStreamer.

(In reply to Philippe Normand from comment #10) > Comment on attachment 342168 [details] > Fix styling issues. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=342168&action=review > > > Source/WebCore/platform/GStreamer.cmake:51 > > + platform/mock/MockRealtimeAudioSource.cpp > > + platform/mock/MockRealtimeMediaSource.cpp > > + platform/mock/MockRealtimeMediaSourceCenter.cpp > > + platform/mock/MockRealtimeVideoSource.cpp > > Hum, this should move to a different cmake file I think, they're not related > with GStreamer. Where then? Copied twice for WPE and Gtk? They are being built so that we can build the GStreamer wrappers

Either Source/WebCore/CMakeLists.txt or Source/WebCore/Sources.txt under MEDIA_STREAM guards.

Actually they're already in Source/WebCore/Sources.txt :)

Created attachment 342187 [details] Stop building MockRealtime** from 2 .cmake files.

Comment on attachment 342187 [details] Stop building MockRealtime** from 2 .cmake files. Clearing flags on attachment: 342187 Committed r232589: <https://trac.webkit.org/changeset/232589>

All reviewed patches have been landed. Closing bug.

<rdar://problem/40903578>