Bug 110150

Created attachment 188936 [details] Implement core mediastream functionality for gstreamer

I haven't done a review yet but ChangeLogs are missing ;) Apart from the port-level API is there anything else still pending to upload to bugzilla? I think I need to read more about MEDIASTREAM before reviewing this patch.

FYI http://dev.w3.org/2011/webrtc/editor/getusermedia.html is the specification.

Yeah, I realize the ChangeLogs are missing. Operator error when using webkit-patch. Sorry about that. I was meaning to mention this when I uploaded this patch. I have one further patch that adds the implementation of the StreamMediaPlayerGStreamer. I was debating combining that patch with this one but wasn't sure what would be easier to review. thoughts? When that patch is applied, you should be able to view your local webcam in a <video> element, etc. After these two, the remote streaming (PeerConnection) stuff will come next, but I want to land this infrastructure first.

Comment on attachment 188936 [details] Implement core mediastream functionality for gstreamer Attachment 188936 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16638081 New failing tests: svg/as-image/img-preserveAspectRatio-support-2.html

By the way, the second patch (and also a temporary third patch that simply accepts all user media requests for now to make testing easier) is included in my branch here if you'd like to test: http://cgit.collabora.com/git/user/jonathon/webkit/log/?h=mediastream-core You can use this simple page for a quick test if you'd like: http://people.collabora.com/~jonathon/webrtc-test/localmediastream.html I should note that although it does basically work with gst 1.0, a lot of video frames get dropped for some reason, whereas it works just fine with gst 0.10. I'd appreciate any help you can give debugging this issue.

Comment on attachment 188936 [details] Implement core mediastream functionality for gstreamer View in context: https://bugs.webkit.org/attachment.cgi?id=188936&action=review Ok I did a first pass, I probably missed some things but we can iterate on patches. > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:49 > + GOwnPtr<GError> error; > + gboolean gstInitialized = gst_init_check(0, 0, &error.outPtr()); > + g_assert(gstInitialized); Use initializeGStreamer() please > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:52 > + GRefPtr<GstBus> bus = adoptGRef(gst_pipeline_get_bus(GST_PIPELINE(m_pipeline.get()))); Use webkitGstPipelineGetBus() perhaps? > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:65 > +CentralPipelineUnit::~CentralPipelineUnit() I think the bus signal watch should be removed here. > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:79 > +bool CentralPipelineUnit::connectAndGetSourceElement(const String& sourceId, GstElement** sourceElement, GstPad** sourcePad) Perhaps smart pointers can be used for the output parameters? > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:83 > + CentralPipelineUnit::PipelineMap::iterator sourceIt = m_pipelineMap.find(sourceId); nit: sourceIterator > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:92 > + CentralPipelineUnit::SourceFactoryMap::iterator sourceFactoryIt = m_sourceFactoryMap.find(sourceId); Ditto :) > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:116 > + GstElement* tee = gst_element_factory_make("tee", NULL); s/NULL/0 > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:131 > + GstPad* sinkpad = gst_element_get_static_pad(tee, "sink"); I think sinkPad is leaked here. Use a smart pointer please. > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:137 > +bool CentralPipelineUnit::connectToSource(const String& sourceId, GstElement* sink, GstPad* sinkpad) I can see in the next patch this method is always called with sinkpad=NULL. Maybe that argument can be removed and handled internally? In the current form of this method the sinkpad also seems to be leaked. > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:164 > + GRefPtr<GstElement> queue = gst_element_factory_make("queue", NULL); NULL should be used for sentinels only, please use 0 here :) > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:175 > + } else { > + if (sinkParent.get() != m_pipeline.get()) { nit: } else if { ? > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:193 > +#ifndef GST_API_VERSION_1 > + const gchar* padTemplate = "src%d"; > +#else > + const gchar* padTemplate = "src_%u"; > +#endif One way to avoid this would be to use gst_element_link_pads_full(tee.get(), 0, queue.get(), "sink", GST_PAD_LINK_CHECK_DEFAULT); That function will request a pad to tee itself, AFAIK. > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:210 > +bool CentralPipelineUnit::disconnectFromSource(const String& sourceId, GstElement* sink, GstPad* sinkpad) Same as for connectToSource, this method seems to be always called without sinkpad. > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:241 > + GRefPtr<GstPad> lQueueSrcPad = adoptGRef(gst_pad_get_peer(sinkpad)); > + GRefPtr<GstElement> lQueue = adoptGRef(gst_pad_get_parent_element(lQueueSrcPad.get())); > + GRefPtr<GstPad> lQueueSinkPad = adoptGRef(gst_element_get_static_pad(lQueue.get(), "sink")); > + GRefPtr<GstPad> lTeeSrcPad = adoptGRef(gst_pad_get_peer(lQueueSinkPad.get())); > + GRefPtr<GstElement> lTee = adoptGRef(gst_pad_get_parent_element(lTeeSrcPad.get())); > + GRefPtr<GstPad> lTeeSinkPad = adoptGRef(gst_element_get_static_pad(lTee.get(), "sink")); > + GRefPtr<GstPad> sourceSrcPad = adoptGRef(gst_pad_get_peer(lTeeSinkPad.get())); > + GRefPtr<GstElement> sourceElement = adoptGRef(gst_pad_get_parent_element(sourceSrcPad.get())); So all you need is lTee and sourceElement it seems, perhaps those can be stored somewhere when they're created so all this pipeline cliff-hanging could be avoided? I'm fine with this otherwise. > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:245 > + guint numSourceTeePads = disconnectSinkFromTee(lTee.get(), sink, sinkpad); > + if (!numSourceTeePads) nit: no need for a variable here. > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:258 > + gst_element_set_state(sink, GST_STATE_NULL); There's no pad blocking needed here? Just curious :) > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:282 > + // Get right hand side elements Comments should be sentences ending with a dot :). > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:514 > + // FIXME: do block properly async Unless this can wait it should be fixed in this patch. > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:543 > + // queue might actually be a fakesink > + gchar* qName = gst_element_get_name(queue.get()); Hum, not sure checking the name of the element is the best idea ;) Can the element type be checked instead? Somehow? > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:620 > + bool done = false; > + while (!done) { > +#ifdef GST_API_VERSION_1 > + GValue item = G_VALUE_INIT; > +#else Ok third time I see this code in the patch so far, maybe it can be refactored in a function taking a callback argument? > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:686 > + if (err) > + g_error_free(err); > + if (debug) > + g_free(debug); Please use GOwnPtrs. > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.h:25 > +#include <gst/gst.h> Can this be replaced by forward declarations? > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.h:40 > +class CentralPipelineUnit { > +public: > + CentralPipelineUnit(); > + virtual ~CentralPipelineUnit(); I think it'd make sense to have this class non-copyable and implement a create function, if possible. > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:60 > + GOwnPtr<GError> error; > + if (!gst_init_check(0, 0, &error.outPtr())) { > + client->didCompleteQuery(audioSources, videoSources); > + return; > + } Please use initializeGStreamer() > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:311 > +static gchar* createUniqueName(const gchar* prefix) Perhaps use PassOwnPtr<char> here? > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:318 > +static GstElement* createAudioSourceBin(GstElement* source) Return a GRefPtr? > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:354 > +static GstElement* createVideoSourceBin(GstElement *source) Ditto > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.h:32 > +#include <gst/gst.h> > +#include <wtf/HashMap.h> > +#include <wtf/gobject/GOwnPtr.h> > +#include <wtf/gobject/GRefPtr.h> > +#include <wtf/text/WTFString.h> Probably some of these includes can be replaced with forward declarations. > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.h:71 > + GstElementFactory *storedElementFactory(const String& key); Nit: * on the wrong side ;) > Source/WebCore/platform/mediastream/gstreamer/SourceFactory.h:25 > +#include <gst/gst.h> > +#include <wtf/text/WTFString.h> Ditto about forward declarations > Source/WebCore/platform/mediastream/gstreamer/SourceFactory.h:29 > +class SourceFactory { Hum, perhaps this can be made non-copyable as well?

Comment on attachment 188936 [details] Implement core mediastream functionality for gstreamer r- as per above comments

Created attachment 192927 [details] Patch

maybe 567 #if GST_API_VERSION_1 to 567 #ifdef GST_API_VERSION_1 if not define the macro, webkit occurs that undef error.

I need to review this, sorry for the delay

Comment on attachment 192927 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192927&action=review Looks good, just a couple of remarks :) Ah also we need,... a ChangeLog :) > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:69 > + gst_bus_remove_signal_watch(bus.get()); No need to clear the sync handler here? > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:58 > + GOwnPtr<GError> error; Unused variable it seems

Comment on attachment 192927 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192927&action=review > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:548 > +#if GST_API_VERSION_1 #ifdef > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:567 > +#if GST_API_VERSION_1 ditto > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:603 > +#if GST_API_VERSION_1 ditto

Comment on attachment 192927 [details] Patch r- as per above comments, would you have time to prepare a new iteration? I think we're getting close :)

> Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp > + audiocaps = adoptGRef(gst_caps_new_simple("audio/x-raw-int", "channels", G_TYPE_INT, 1, NULL)); Any specific reason to hardcode channels here?

Jonathon, do you plan to continue work on this?

(In reply to comment #16) > Jonathon, do you plan to continue work on this? I probably won't have time to continue this, but probably somebody else from Collabora will pick it up eventually. cc-ing Danilo.

Since no one else showed interest for this I'm picking it up... The patch needs some more work to cope with the latest mediastream changes done at WebCore level. I'll also remove gst 0.10 support from this patch.

Created attachment 214731 [details] Patch

Comment on attachment 214731 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214731&action=review > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterGStreamer.cpp:56 > + static bool registered = false; > + if (!registered) { > + registered = true; > + MediaStreamCenter::setSharedStreamCenter(&center); > + } This shouldn't be necessary, setSharedStreamCenter() is only needed if you want to use something other than the center returned by MediaStreamCenter::platformCenter. > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterGStreamer.cpp:86 > + // TODO: verify constraints according to registered We usually use FIXME instead of TODO. Even better if it includes a bug number ;-). > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterGStreamer.cpp:97 > + // TODO: verify constraints according to registered Ditto. > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:125 > + // TODO: implement Ditto.

Created attachment 215186 [details] MediaStreamCenter and minimal CentralPipelineUnit

Comment on attachment 215186 [details] MediaStreamCenter and minimal CentralPipelineUnit View in context: https://bugs.webkit.org/attachment.cgi?id=215186&action=review Looking pretty good, but I have a few general comments. > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:30 > +#include "SourceFactory.h" > + > +#include <wtf/MainThread.h> Extra newline. > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:58 > + GRefPtr<GstBus> bus = webkitGstPipelineGetBus(GST_PIPELINE(m_pipeline.get())); > + if (bus) { > + gst_bus_add_signal_watch(bus.get()); Hrm. Is it an error if there's no bus? Maybe this should be an ASSERT? > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:69 > + if (bus) > + gst_bus_remove_signal_watch(bus.get()); Ditto? > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.cpp:89 > + GOwnPtr<GError> error; > + GOwnPtr<gchar> debug; > + > + switch (GST_MESSAGE_TYPE(message)) { > + case GST_MESSAGE_ERROR: { > + gst_message_parse_error(message, &error.outPtr(), &debug.outPtr()); You should probably define error and debug in the innermost scope. > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.h:32 > +typedef struct _GstElement GstElement; > +typedef struct _GstMessage GstMessage; > +typedef struct _GstPad GstPad; Unrelated: I wonder if it makes sense to centralize the gst typedefs somewhere. > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.h:47 > + gboolean handleMessage(GstMessage*); I think it's fine to have this return a bool. > Source/WebCore/platform/mediastream/gstreamer/CentralPipelineUnit.h:57 > + Source() : m_sourceElement(0), m_sourcePad(0), m_sourceFactory(0), m_removeWhenNotUsed(true) { } > + Source(GRefPtr<GstElement> sourceElement, GRefPtr<GstPad> sourcePad, SourceFactory* sourceFactory, bool removeWhenNotUsed) : m_sourceElement(sourceElement), m_sourcePad(sourcePad), m_sourceFactory(sourceFactory), m_removeWhenNotUsed(removeWhenNotUsed) { } > + I think it might be more readable to use WebKit style here. GRefPtrs do not need to be initialized to zero. The default constructor assigns null. > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterGStreamer.cpp:64 > +void MediaStreamCenterGStreamer::validateRequestConstraints(PassRefPtr<MediaStreamCreationClient> prpClient, PassRefPtr<MediaConstraints> audioConstraints, PassRefPtr<MediaConstraints> videoConstraints) videoConstraints in unused here. > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterGStreamer.cpp:105 > +bool MediaStreamCenterGStreamer::getMediaStreamTrackSources(PassRefPtr<MediaStreamTrackSourcesRequestClient> client) client is unused here. > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:42 > + GRefPtr<GstElement> deviceSrc; > + GRefPtr<GstElement> autoSrc = gst_element_factory_make(elementName, 0); > + if (!autoSrc) > + return deviceSrc; deviceSrc is null here, so just return null. > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:46 > + GstChildProxy* childProxy = GST_CHILD_PROXY(autoSrc.get()); > + if (!childProxy) > + return deviceSrc; Ditto. > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:50 > + return deviceSrc; Ditto. > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:52 > + if (gst_child_proxy_get_children_count(childProxy)) Move the declaration of deviceSrc above this line. I recommend the names deviceSource and audioSource. It's the right thing to use "source" because the name of the method is findDeviceSource. > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:67 > + String strFactoryName(factoryName.get()); You probably want to be converting from UTF8 and be using StringBuilder. > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:73 > + g_object_get(source, "device-name", &deviceName.outPtr(), NULL); Can NULL be nullptr here? > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:105 > + else > + ASSERT_NOT_REACHED(); There is no else. :) Probably better to do: switch (type) { case Audio: case Video: } so that the compiler can give an error if a new type is added. > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:118 > + LOG_MEDIA_MESSAGE("Registering source factory for source with id=%s", id.ascii().data()); utf8() instead of ascii? > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:126 > + return 0; nullptr? > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:137 > + return 0; Can this be nullptr? > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.h:29 > + Extra newline here. > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.h:82 > + // SourceFactory What's the purpose of the comment here? > Source/WebCore/platform/mediastream/gstreamer/SourceFactory.h:43 > +class SourceFactory { > + WTF_MAKE_NONCOPYABLE(SourceFactory); > +public: > + SourceFactory() { } > + virtual GRefPtr<GstElement> createSource(const String& sourceId, GRefPtr<GstPad>& srcPad) = 0; > + > +protected: > + virtual ~SourceFactory() { } > +}; > + Are there going to be multiple implementations of this?

Created attachment 215511 [details] MediaStreamCenter and minimal CentralPipelineUnit

(In reply to comment #22) > > > Source/WebCore/platform/mediastream/gstreamer/SourceFactory.h:43 > > +class SourceFactory { > > + WTF_MAKE_NONCOPYABLE(SourceFactory); > > +public: > > + SourceFactory() { } > > + virtual GRefPtr<GstElement> createSource(const String& sourceId, GRefPtr<GstPad>& srcPad) = 0; > > + > > +protected: > > + virtual ~SourceFactory() { } > > +}; > > + > > Are there going to be multiple implementations of this? It's a remnant from a previous branch that I think is indeed used only in one place. So this made me think of a grand refactoring in which the SourceFactory disappeared :)

Created attachment 215527 [details] MediaStreamCenter and minimal CentralPipelineUnit

Comment on attachment 215527 [details] MediaStreamCenter and minimal CentralPipelineUnit Attachment 215527 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/17028380

Created attachment 215634 [details] MediaStreamCenter and minimal CentralPipelineUnit

Comment on attachment 215634 [details] MediaStreamCenter and minimal CentralPipelineUnit View in context: https://bugs.webkit.org/attachment.cgi?id=215634&action=review > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterGStreamer.cpp:91 > + audioSource->reset(); Do you really want to reset the source here? What if it is already in use by another stream? > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterGStreamer.cpp:92 > + audioSource->setReadyState(MediaStreamSource::Live); Shouldn't this be done by the source when it actually begins to provide data? > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterGStreamer.cpp:103 > + videoSource->reset(); > + videoSource->setReadyState(MediaStreamSource::Live); Ditto x 2.

Comment on attachment 215634 [details] MediaStreamCenter and minimal CentralPipelineUnit View in context: https://bugs.webkit.org/attachment.cgi?id=215634&action=review > Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:108 > + default: > + ASSERT_NOT_REACHED(); No can omit the default case actually and that will get us the compilation error if another one is ever added.

Created attachment 216036 [details] MediaStreamCenter and minimal CentralPipelineUnit

Sorry I forgot to address Eric's comments. I think the reset() call is indeed not needed but the setReadyState() call would be ok because the gst elements are added in a live pipeline. Will update the patch.

Created attachment 230757 [details] Patch

Comment on attachment 216036 [details] MediaStreamCenter and minimal CentralPipelineUnit Hopefully with OpenWebRTC there won't be any need for such a CentralPipelineUnit thing :)

Sorry I made a mistake re-titling this bug, it doesn't make sense to hijack this bug with the OpenWebRTC approach, the comments posted here wouldn't make sense anymore. Instead I'll just close this bug, if anyone wants to provide a pure-GStreamer-based MediaStreamCenter in the future they can reopen this bug.