Bug 123015 – [GStreamer] webkitwebaudiosrc element needs to emit stream-start, caps and segment events

• Component: Web Audio
• Status: RESOLVED FIXED

[Philippe Normand]

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 :)

[Nick Diego Yamane (diegoyam)]

(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

[Philippe Normand]

(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? :)

[Nick Diego Yamane (diegoyam)]

(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.

[Philippe Normand]

Created attachment 215393 [details]
wip patch

[Philippe Normand]

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.

[Philippe Normand]

(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.

[Sergio Correia (qrwteyrutiyoup)]

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.

[Philippe Normand]

Created attachment 216549 [details]
patch

[Philippe Normand]

(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 :)

[EFL EWS Bot]

Comment on attachment 216549 [details]
patch

Attachment 216549 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/22369306

[Philippe Normand]

Created attachment 216550 [details]
patch

[Sebastian Dröge (slomo)]

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

[Philippe Normand]

Created attachment 217279 [details]
patch

[Martin Robinson]

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?

[Philippe Normand]

Created attachment 218584 [details]
patch

[Martin Robinson]

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.

[Sebastian Dröge (slomo)]

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

[Philippe Normand]

OMG uncomfortable moment here. :)

[Philippe Normand]

Committed r160216: <http://trac.webkit.org/changeset/160216>