[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 #341758|                            |review?, commit-queue?
              Flags|                            |

--- 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:17
> > + * 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:135
> > +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:142
> > +    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:172
> > +            // 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 :-)

-- 
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/20180601/8dcb1a90/attachment.html>