[Webkit-unassigned] [Bug 185787] [GTK][WPE] Start implementing MediaStream API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=185787

Thibault Saunier <tsaunier at gnome.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #342085|                            |review?, commit-queue?
              Flags|                            |

--- 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: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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180606/ac316dd9/attachment-0001.html>