[webkit-reviews] review requested: [Bug 185787] [GTK][WPE] Start implementing MediaStream API : [Attachment 342085] [GTK][WPE]: Implement MediaStream API

[bugzilla-daemon at webkit.org]

Thibault Saunier <tsaunier at gnome.org> has asked  for review:
Bug 185787: [GTK][WPE] Start implementing MediaStream API
https://bugs.webkit.org/show_bug.cgi?id=185787

Attachment 342085: [GTK][WPE]: Implement MediaStream API

https://bugs.webkit.org/attachment.cgi?id=342085&action=review




--- Comment #4 from Thibault Saunier <tsaunier at gnome.org> ---
Created attachment 342085

  --> https://bugs.webkit.org/attachment.cgi?id=342085&action=review

[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:16
4
> > +	 GstFlowCombiner* flow_combiner;
> 
> Variables should use camelCase whenever possible

Fixed a few I noticed.

> >
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:31
6
> > +	 /**
> > +	  * 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:31
9
> > +		 "Let playbin3 we are a live source.",
> 
> +know
> 
> >
Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:1
91
> > +		 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.