RESOLVED WONTFIX Bug 110150
[GStreamer] MediaStreamCenter implementation
https://bugs.webkit.org/show_bug.cgi?id=110150
Summary [GStreamer] MediaStreamCenter implementation
Jonathon Jongsma (jonner)
Reported 2013-02-18 13:25:57 PST
[gtk] Implement core mediastream functionality
Attachments
Implement core mediastream functionality for gstreamer (55.12 KB, patch)
2013-02-18 13:27 PST, Jonathon Jongsma (jonner)
no flags
Patch (58.26 KB, patch)
2013-03-13 08:30 PDT, Jonathon Jongsma (jonner)
pnormand: review-
Patch (29.83 KB, patch)
2013-10-21 05:48 PDT, Philippe Normand
no flags
MediaStreamCenter and minimal CentralPipelineUnit (29.62 KB, patch)
2013-10-25 09:45 PDT, Philippe Normand
no flags
MediaStreamCenter and minimal CentralPipelineUnit (28.05 KB, patch)
2013-10-30 08:58 PDT, Philippe Normand
no flags
MediaStreamCenter and minimal CentralPipelineUnit (27.84 KB, patch)
2013-10-30 11:03 PDT, Philippe Normand
gtk-ews: commit-queue-
MediaStreamCenter and minimal CentralPipelineUnit (27.79 KB, patch)
2013-10-31 02:08 PDT, Philippe Normand
no flags
MediaStreamCenter and minimal CentralPipelineUnit (27.79 KB, patch)
2013-11-05 07:11 PST, Philippe Normand
no flags
Patch (26.44 KB, patch)
2014-05-03 11:52 PDT, Seongjun Kim
no flags
Jonathon Jongsma (jonner)
Comment 1 2013-02-18 13:27:21 PST
Created attachment 188936 [details] Implement core mediastream functionality for gstreamer
Philippe Normand
Comment 2 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.
Tommy Widenflycht
Comment 3 2013-02-19 00:13:45 PST
Jonathon Jongsma (jonner)
Comment 4 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.
Build Bot
Comment 5 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
Jonathon Jongsma (jonner)
Comment 6 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.
Philippe Normand
Comment 7 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?
Philippe Normand
Comment 8 2013-02-28 00:14:55 PST
Comment on attachment 188936 [details] Implement core mediastream functionality for gstreamer r- as per above comments
Jonathon Jongsma (jonner)
Comment 9 2013-03-13 08:30:26 PDT
Dongheun Kang
Comment 10 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.
Philippe Normand
Comment 11 2013-04-12 12:47:50 PDT
I need to review this, sorry for the delay
Philippe Normand
Comment 12 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
Philippe Normand
Comment 13 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
Philippe Normand
Comment 14 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 :)
Nick Diego Yamane (diegoyam)
Comment 15 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?
Philippe Normand
Comment 16 2013-09-06 06:58:40 PDT
Jonathon, do you plan to continue work on this?
Jonathon Jongsma (jonner)
Comment 17 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.
Philippe Normand
Comment 18 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.
Philippe Normand
Comment 19 2013-10-21 05:48:20 PDT
Eric Carlson
Comment 20 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.
Philippe Normand
Comment 21 2013-10-25 09:45:32 PDT
Created attachment 215186 [details] MediaStreamCenter and minimal CentralPipelineUnit
Martin Robinson
Comment 22 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?
Philippe Normand
Comment 23 2013-10-30 08:58:16 PDT
Created attachment 215511 [details] MediaStreamCenter and minimal CentralPipelineUnit
Philippe Normand
Comment 24 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 :)
Philippe Normand
Comment 25 2013-10-30 11:03:32 PDT
Created attachment 215527 [details] MediaStreamCenter and minimal CentralPipelineUnit
kov's GTK+ EWS bot
Comment 26 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
Philippe Normand
Comment 27 2013-10-31 02:08:51 PDT
Created attachment 215634 [details] MediaStreamCenter and minimal CentralPipelineUnit
Eric Carlson
Comment 28 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.
Martin Robinson
Comment 29 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.
Philippe Normand
Comment 30 2013-11-05 07:11:47 PST
Created attachment 216036 [details] MediaStreamCenter and minimal CentralPipelineUnit
Philippe Normand
Comment 31 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.
Seongjun Kim
Comment 32 2014-05-03 11:52:24 PDT
Philippe Normand
Comment 33 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 :)
Philippe Normand
Comment 34 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.
Note You need to log in before you can comment on or make changes to this bug.