Bug 110150 - [GStreamer] MediaStreamCenter implementation
Summary: [GStreamer] MediaStreamCenter implementation
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords:
Depends on:
Blocks: 79203 124288
  Show dependency treegraph
 
Reported: 2013-02-18 13:25 PST by Jonathon Jongsma (jonner)
Modified: 2015-03-09 02:46 PDT (History)
25 users (show)

See Also:


Attachments
Implement core mediastream functionality for gstreamer (55.12 KB, patch)
2013-02-18 13:27 PST, Jonathon Jongsma (jonner)
no flags Details | Formatted Diff | Diff
Patch (58.26 KB, patch)
2013-03-13 08:30 PDT, Jonathon Jongsma (jonner)
pnormand: review-
Details | Formatted Diff | Diff
Patch (29.83 KB, patch)
2013-10-21 05:48 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
MediaStreamCenter and minimal CentralPipelineUnit (29.62 KB, patch)
2013-10-25 09:45 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
MediaStreamCenter and minimal CentralPipelineUnit (28.05 KB, patch)
2013-10-30 08:58 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
MediaStreamCenter and minimal CentralPipelineUnit (27.84 KB, patch)
2013-10-30 11:03 PDT, Philippe Normand
gtk-ews: commit-queue-
Details | Formatted Diff | Diff
MediaStreamCenter and minimal CentralPipelineUnit (27.79 KB, patch)
2013-10-31 02:08 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
MediaStreamCenter and minimal CentralPipelineUnit (27.79 KB, patch)
2013-11-05 07:11 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (26.44 KB, patch)
2014-05-03 11:52 PDT, Seongjun Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathon Jongsma (jonner) 2013-02-18 13:25:57 PST
[gtk] Implement core mediastream functionality
Comment 1 Jonathon Jongsma (jonner) 2013-02-18 13:27:21 PST
Created attachment 188936 [details]
Implement core mediastream functionality for gstreamer
Comment 2 Philippe Normand 2013-02-18 23:25:23 PST
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.
Comment 3 Tommy Widenflycht 2013-02-19 00:13:45 PST
FYI http://dev.w3.org/2011/webrtc/editor/getusermedia.html is the specification.
Comment 4 Jonathon Jongsma (jonner) 2013-02-19 07:04:28 PST
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 5 Build Bot 2013-02-19 16:40:53 PST
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
Comment 6 Jonathon Jongsma (jonner) 2013-02-21 09:25:01 PST
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 7 Philippe Normand 2013-02-22 03:44:55 PST
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 8 Philippe Normand 2013-02-28 00:14:55 PST
Comment on attachment 188936 [details]
Implement core mediastream functionality for gstreamer

r- as per above comments
Comment 9 Jonathon Jongsma (jonner) 2013-03-13 08:30:26 PDT
Created attachment 192927 [details]
Patch
Comment 10 Dongheun Kang 2013-03-18 17:09:06 PDT
maybe 

567 #if GST_API_VERSION_1

to

567 #ifdef GST_API_VERSION_1

if not define the macro, webkit occurs that undef error.
Comment 11 Philippe Normand 2013-04-12 12:47:50 PDT
I need to review this, sorry for the delay
Comment 12 Philippe Normand 2013-07-18 01:14:14 PDT
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 13 Philippe Normand 2013-07-18 01:15:32 PDT
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 14 Philippe Normand 2013-08-08 07:53:13 PDT
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 :)
Comment 15 Nick Diego Yamane (diegoyam) 2013-08-14 11:18:09 PDT
> 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?
Comment 16 Philippe Normand 2013-09-06 06:58:40 PDT
Jonathon, do you plan to continue work on this?
Comment 17 Jonathon Jongsma (jonner) 2013-09-06 09:18:20 PDT
(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.
Comment 18 Philippe Normand 2013-10-01 02:49:40 PDT
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.
Comment 19 Philippe Normand 2013-10-21 05:48:20 PDT
Created attachment 214731 [details]
Patch
Comment 20 Eric Carlson 2013-10-22 10:54:23 PDT
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.
Comment 21 Philippe Normand 2013-10-25 09:45:32 PDT
Created attachment 215186 [details]
MediaStreamCenter and minimal CentralPipelineUnit
Comment 22 Martin Robinson 2013-10-29 10:00:41 PDT
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?
Comment 23 Philippe Normand 2013-10-30 08:58:16 PDT
Created attachment 215511 [details]
MediaStreamCenter and minimal CentralPipelineUnit
Comment 24 Philippe Normand 2013-10-30 09:00:21 PDT
(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 :)
Comment 25 Philippe Normand 2013-10-30 11:03:32 PDT
Created attachment 215527 [details]
MediaStreamCenter and minimal CentralPipelineUnit
Comment 26 kov's GTK+ EWS bot 2013-10-31 00:36:40 PDT
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
Comment 27 Philippe Normand 2013-10-31 02:08:51 PDT
Created attachment 215634 [details]
MediaStreamCenter and minimal CentralPipelineUnit
Comment 28 Eric Carlson 2013-10-31 08:31:10 PDT
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 29 Martin Robinson 2013-10-31 08:46:09 PDT
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.
Comment 30 Philippe Normand 2013-11-05 07:11:47 PST
Created attachment 216036 [details]
MediaStreamCenter and minimal CentralPipelineUnit
Comment 31 Philippe Normand 2013-11-05 07:15:06 PST
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.
Comment 32 Seongjun Kim 2014-05-03 11:52:24 PDT
Created attachment 230757 [details]
Patch
Comment 33 Philippe Normand 2014-10-27 10:35:58 PDT
Comment on attachment 216036 [details]
MediaStreamCenter and minimal CentralPipelineUnit

Hopefully with OpenWebRTC there won't be any need for such a CentralPipelineUnit thing :)
Comment 34 Philippe Normand 2015-03-09 02:46:17 PDT
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.