Summary: | [GStreamer] webkitwebaudiosrc element needs to emit stream-start, caps and segment events | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||||||||||
Component: | Web Audio | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, crogers, eflews.bot, eric.carlson, glenn, gyuyoung.kim, jer.noble, nick.diego, pnormand, sergio, slomo | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Philippe Normand
2013-10-18 03:43:01 PDT
(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> |