[webkit-reviews] review requested: [Bug 185787] [GTK][WPE] Start implementing MediaStream API : [Attachment 341758] [GTK][WPE] Start implementing 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 341758: [GTK][WPE] Start implementing MediaStream API

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




--- Comment #1 from Thibault Saunier <tsaunier at gnome.org> ---
Created attachment 341758

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

[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:1
7
> > + * 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:1
35
> > +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:1
42
> > +	 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:1
72
> > +		 // 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 :-)