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

[bugzilla-daemon at webkit.org]

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

--- Comment #3 from Philippe Normand <pnormand at igalia.com> ---
Comment on attachment 341758
  --> https://bugs.webkit.org/attachment.cgi?id=341758
[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 :)

-- 
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/20180604/a5e861a8/attachment-0001.html>