After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 788361 - audiosink: expose more audioringbuffer vmethods to child sinks
audiosink: expose more audioringbuffer vmethods to child sinks
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
1.12.3
Other All
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-09-29 21:34 UTC by Philippe Renon
Modified: 2018-11-03 12:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
audiosink: expose more audioringbuffer vmethods to child sinks (6.42 KB, patch)
2017-09-29 21:51 UTC, Philippe Renon
none Details | Review
audiosink: expose more audioringbuffer vmethods to child sinks (7.53 KB, patch)
2017-10-03 06:37 UTC, Philippe Renon
none Details | Review
directsoundsink: don't reset DirectSoundBuffer when pausing (5.53 KB, patch)
2017-10-03 07:21 UTC, Philippe Renon
none Details | Review
audiosink: expose more audioringbuffer vmethods to child sinks (7.53 KB, patch)
2017-10-03 07:23 UTC, Philippe Renon
none Details | Review
audiosink: expose more audioringbuffer vmethods to child sinks (7.53 KB, patch)
2017-10-03 07:28 UTC, Philippe Renon
none Details | Review
audiosink: expose more audioringbuffer vmethods to child sinks (7.48 KB, patch)
2017-10-03 07:36 UTC, Philippe Renon
none Details | Review
audiosink: chain up to the ringbuffer clear_all parent implementation (2.44 KB, patch)
2017-10-06 07:34 UTC, Philippe Renon
none Details | Review
audiosink: expose more audioringbuffer vmethods to child sinks (9.21 KB, patch)
2017-10-10 20:12 UTC, Philippe Renon
none Details | Review
Fix resuming after paused (1.78 KB, patch)
2018-06-26 09:35 UTC, Axel Mårtensson
none Details | Review

Description Philippe Renon 2017-09-29 21:34:15 UTC
The existing reset vmethod is deprecated.
The audio sink will fallback to calling reset if pause or stop are not provided and will fallback to calling start if resume is not provided.

Existing audio sinks continue to work as before.
    
This change is needed for sinks that need to distinguish between a pause and a stop (currently both are handled by a reset).
Comment 1 Philippe Renon 2017-09-29 21:38:28 UTC
This is change is required by https://bugzilla.gnome.org/show_bug.cgi?id=788362
Comment 2 Philippe Renon 2017-09-29 21:51:05 UTC
Created attachment 360684 [details] [review]
audiosink: expose more audioringbuffer vmethods to child sinks
Comment 3 Philippe Renon 2017-10-03 06:37:15 UTC
Created attachment 360810 [details] [review]
audiosink: expose more audioringbuffer vmethods to child sinks
Comment 4 Philippe Renon 2017-10-03 06:38:06 UTC
Last attached patch exposes clear_all in addition to pause, resume and stop.
Comment 5 Philippe Renon 2017-10-03 07:21:21 UTC
Created attachment 360812 [details] [review]
directsoundsink: don't reset DirectSoundBuffer when pausing
Comment 6 Philippe Renon 2017-10-03 07:23:09 UTC
Created attachment 360814 [details] [review]
audiosink: expose more audioringbuffer vmethods to child sinks
Comment 7 Philippe Renon 2017-10-03 07:28:42 UTC
Created attachment 360815 [details] [review]
audiosink: expose more audioringbuffer vmethods to child sinks
Comment 8 Philippe Renon 2017-10-03 07:36:33 UTC
Created attachment 360816 [details] [review]
audiosink: expose more audioringbuffer vmethods to child sinks
Comment 9 Philippe Renon 2017-10-06 07:34:43 UTC
Created attachment 361026 [details] [review]
audiosink: chain up to the ringbuffer clear_all parent implementation
Comment 10 Philippe Renon 2017-10-10 16:35:51 UTC
Found a simpler fix for the directsoundsink issue that does not require to expose clear_all. Will submit a new patch soon.
Comment 11 Philippe Renon 2017-10-10 20:12:30 UTC
Created attachment 361287 [details] [review]
audiosink: expose more audioringbuffer vmethods to child sinks

Final patch for now. Has minor cleanups compared to previous one.

I kept the new clean_all vmethod. Does not hurt.

To summarize this patch: it exposes a bunch of missing ringbuffer vmethods to child classes. Care has been taken to not affect existing audio sinks and to preserve ABI.
Comment 12 Axel Mårtensson 2018-06-26 09:35:55 UTC
Created attachment 372822 [details] [review]
Fix resuming after paused

Hi Philippe,

I took use of your patch to expose the vmethods to child sinks, in my case to alsasink. The vmethods I wanted to expose was the following; pause, resume and stop.  

Pause worked as expected, however when using resume, I noticed that the audioringbuffer never receives a start signal and is therefore stuck in a waiting state. To solve this I changed in gst_audio_sink_ring_buffer_resume() to always call gst_audio_sink_ring_buffer_start() and thereby a start signal is sent to the audioringbuffer.


Best regards,
Axel
Comment 13 Philippe Renon 2018-06-26 10:28:18 UTC
Hi Axel,

Not sure your change is correct.
Looks like now, resume will call the resume vmethod (if it exists) and start vmethod (always).
This will probably break other audio sinks.

You probably want to call start() from the alsasink resume (if that is what alsasink requires/needs).

Best regards,
Philippe.
Comment 14 Axel Mårtensson 2018-06-26 11:46:01 UTC
From my understanding when paused is triggered, the audioringbuffer is signaled to paused (see gst_audio_ring_buffer_pause_unlocked()). To resume the audioringbuffer gst_audio_sink_ring_buffer_start() have to be called to signal the buffer.

The alsasink can not signal the audioringbuffer to start(), due to that it is a child class and have no information about the audioringbuffer. That is why audiosink signals the audioringbuffer to resume/start.  

Best regards, 
Axel
Comment 15 Philippe Renon 2018-06-26 13:41:36 UTC
It's been a while that I worked on that.

What about not implementing resume() in alsasink?
Base class will then default to calling start().
Comment 16 GStreamer system administrator 2018-11-03 12:00:19 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/387.