Bug 123015 - [GStreamer] webkitwebaudiosrc element needs to emit stream-start, caps and segment events
Summary: [GStreamer] webkitwebaudiosrc element needs to emit stream-start, caps and se...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-18 03:43 PDT by Philippe Normand
Modified: 2013-12-06 03:17 PST (History)
12 users (show)

See Also:


Attachments
wip patch (3.36 KB, patch)
2013-10-29 09:38 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
patch (6.06 KB, patch)
2013-11-11 00:48 PST, Philippe Normand
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
patch (6.07 KB, patch)
2013-11-11 00:56 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
patch (6.36 KB, patch)
2013-11-19 01:36 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
patch (6.30 KB, patch)
2013-12-06 02:45 PST, Philippe Normand
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2013-10-18 03:43:01 PDT
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 :)
Comment 1 Nick Diego Yamane (diegoyam) 2013-10-18 11:33:42 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
Comment 2 Philippe Normand 2013-10-21 02:55:05 PDT
(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? :)
Comment 3 Nick Diego Yamane (diegoyam) 2013-10-21 12:02:28 PDT
(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.
Comment 4 Philippe Normand 2013-10-29 09:38:02 PDT
Created attachment 215393 [details]
wip patch
Comment 5 Philippe Normand 2013-11-08 08:40:45 PST
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.
Comment 6 Philippe Normand 2013-11-11 00:08:34 PST
(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 7 Sergio Correia (qrwteyrutiyoup) 2013-11-11 00:29:38 PST
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.
Comment 8 Philippe Normand 2013-11-11 00:48:40 PST
Created attachment 216549 [details]
patch
Comment 9 Philippe Normand 2013-11-11 00:51:31 PST
(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 10 EFL EWS Bot 2013-11-11 00:54:29 PST
Comment on attachment 216549 [details]
patch

Attachment 216549 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/22369306
Comment 11 Philippe Normand 2013-11-11 00:56:45 PST
Created attachment 216550 [details]
patch
Comment 12 Sebastian Dröge (slomo) 2013-11-18 09:53:10 PST
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
Comment 13 Philippe Normand 2013-11-19 01:36:15 PST
Created attachment 217279 [details]
patch
Comment 14 Martin Robinson 2013-12-06 01:58:52 PST
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?
Comment 15 Philippe Normand 2013-12-06 02:45:35 PST
Created attachment 218584 [details]
patch
Comment 16 Martin Robinson 2013-12-06 03:01:53 PST
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 17 Sebastian Dröge (slomo) 2013-12-06 03:04:14 PST
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
Comment 18 Philippe Normand 2013-12-06 03:09:02 PST
OMG uncomfortable moment here. :)
Comment 19 Philippe Normand 2013-12-06 03:17:36 PST
Committed r160216: <http://trac.webkit.org/changeset/160216>