[webkit-reviews] review requested: [Bug 185787] [GTK][WPE] Start implementing MediaStream API : [Attachment 342160] Addressed comments.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jun 7 07:47:42 PDT 2018
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 342160: Addressed comments.
https://bugs.webkit.org/attachment.cgi?id=342160&action=review
--- Comment #6 from Thibault Saunier <tsaunier at gnome.org> ---
Created attachment 342160
--> https://bugs.webkit.org/attachment.cgi?id=342160&action=review
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:30
2
> > + GstElementClass* gstelement_klass = (GstElementClass*)klass;
>
> Use GST_ELEMENT_CLASS(klass)
>
> >
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:35
6
> > + (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:35
8
> > + 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.
More information about the webkit-reviews
mailing list