With GStreamer 1.2 at least. We now need to send downstream stream-start, caps and segment events along with the first buffers. I started a patch... Perhaps it would be good to rework the whole playback pipeline: - make webkitwebsrc a basesrc sub-class - one webkitwebsrc per WebAudio bus channel - handle interleaving and wavenc in the pipeline, not in the src element like it's done currently. Food for thought :)
(In reply to comment #0) > With GStreamer 1.2 at least. We now need to send downstream stream-start, caps and segment events along with the first buffers. I started a patch... > > Perhaps it would be good to rework the whole playback pipeline: Nice :) Any chance to make it so that it can be integrated with mediastream pipeline (ongoing work - central pipeline unit) ? > - make webkitwebsrc a basesrc sub-class > - one webkitwebsrc per WebAudio bus channel > - handle interleaving and wavenc in the pipeline, not in the src element like it's done currently. > > Food for thought :) Thanks
(In reply to comment #1) > (In reply to comment #0) > > With GStreamer 1.2 at least. We now need to send downstream stream-start, caps and segment events along with the first buffers. I started a patch... > > > > Perhaps it would be good to rework the whole playback pipeline: > > Nice :) > > Any chance to make it so that it can be integrated with mediastream pipeline (ongoing work - central pipeline unit) ? > I thought you were working on mediastream support? :)
(In reply to comment #2) > (In reply to comment #1) > > (In reply to comment #0) > > > With GStreamer 1.2 at least. We now need to send downstream stream-start, caps and segment events along with the first buffers. I started a patch... > > > > > > Perhaps it would be good to rework the whole playback pipeline: > > > > Nice :) > > > > Any chance to make it so that it can be integrated with mediastream pipeline (ongoing work - central pipeline unit) ? > > > > I thought you were working on mediastream support? :) Yes, we are. But currently there is no integration between webaudio/mediastream pipelines.
Created attachment 215393 [details] wip patch
I reported this in GStreamer's bugzilla: https://bugzilla.gnome.org/show_bug.cgi?id=711699 OTOH the warnings emitted because our element doesn't send the events are not fatal but It'd be good to fix anyway.
(In reply to comment #5) > I reported this in GStreamer's bugzilla: https://bugzilla.gnome.org/show_bug.cgi?id=711699 > That was fixed and shipped in gst 1.2.1. > OTOH the warnings emitted because our element doesn't send the events are not fatal but It'd be good to fix anyway. I'll update the patch.
Comment on attachment 215393 [details] wip patch View in context: https://bugs.webkit.org/attachment.cgi?id=215393&action=review > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:402 > + gchar* queueName = gst_element_get_name(queue); Maybe you can use GOwnPtr here? > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:403 > + gchar* stream_id = g_strdup_printf("webaudio/%s", queueName); Ditto.
Created attachment 216549 [details] patch
(In reply to comment #7) > (From update of attachment 215393 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=215393&action=review > > > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:402 > > + gchar* queueName = gst_element_get_name(queue); > > Maybe you can use GOwnPtr here? > > > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:403 > > + gchar* stream_id = g_strdup_printf("webaudio/%s", queueName); > > Ditto. That was a WiP patch, not really worth a review, but yes it had a comment about using smart pointers :)
Comment on attachment 216549 [details] patch Attachment 216549 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/22369306
Created attachment 216550 [details] patch
Comment on attachment 216550 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=216550&action=review Looks good overall IMHO > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:213 > + gst_segment_init(priv->segment, GST_FORMAT_TIME); You can directly store the segment in the WebKitWebAudioSourcePrivate. No need for another allocation
Created attachment 217279 [details] patch
Comment on attachment 217279 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=217279&action=review > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:408 > + GRefPtr<GstElement> queue = gst_pad_get_parent_element(pad); From http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstPad.html, it appears this method returns a fresh reference, so it seems this should be adoptGRef. > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:409 > + GRefPtr<GstPad> sinkPad = gst_element_get_static_pad(queue.get(), "sink"); Ditto. > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:412 > + GstEvent* event = gst_event_new_stream_start(streamId.get()); Maybe call this streamStartEvent, just for clarity? > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:422 > + GRefPtr<GstCaps> caps = adoptGRef(gst_audio_info_to_caps(&info)); capsWithChannelPosition?
Created attachment 218584 [details] patch
Comment on attachment 218584 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=218584&action=review > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:266 > > + gst_segment_free(&priv->segment); > + It's probably a good idea to switch to a smart pointer here.
Comment on attachment 218584 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=218584&action=review >> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:266 >> + > > It's probably a good idea to switch to a smart pointer here. Also this will crash because you free some random memory that you didn't allocate ;) Just don't do anything here, the segment does not have any extra allocations
OMG uncomfortable moment here. :)
Committed r160216: <http://trac.webkit.org/changeset/160216>