audiosink: expose more audioringbuffer vmethods to child sinks

Submitted by Philippe Renon

Link to original bug (#788361)

Description

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

Version: 1.12.3

Philippe Renon said:

This is change is required by https://bugzilla.gnome.org/show_bug.cgi?id=788362

Philippe Renon submitted a patch:

Patch 360684, "audiosink: expose more audioringbuffer vmethods to child sinks":
0001-audiosink-expose-more-audioringbuffer-vmethods-to-ch.patch

Philippe Renon submitted a patch:

Patch 360810, "audiosink: expose more audioringbuffer vmethods to child sinks":
0001-audiosink-expose-more-audioringbuffer-vmethods-to-ch.patch

Philippe Renon said:

Last attached patch exposes clear_all in addition to pause, resume and stop.

Philippe Renon submitted a patch:

Patch 360812, "directsoundsink: don't reset DirectSoundBuffer when pausing":
0004-directsoundsink-don-t-reset-DirectSoundBuffer-when-p.patch

Philippe Renon submitted a patch:

Patch 360814, "audiosink: expose more audioringbuffer vmethods to child sinks":
0001-audiosink-expose-more-audioringbuffer-vmethods-to-ch.patch

Philippe Renon submitted a patch:

Patch 360815, "audiosink: expose more audioringbuffer vmethods to child sinks":
0001-audiosink-expose-more-audioringbuffer-vmethods-to-ch.patch

Philippe Renon submitted a patch:

Patch 360816, "audiosink: expose more audioringbuffer vmethods to child sinks":
0001-audiosink-expose-more-audioringbuffer-vmethods-to-ch.patch

Philippe Renon submitted a patch:

Patch 361026, "audiosink: chain up to the ringbuffer clear_all parent implementation":
0002-audiosink-chain-up-to-the-ringbuffer-clear_all-paren.patch

Philippe Renon said:

Found a simpler fix for the directsoundsink issue that does not require to expose clear_all. Will submit a new patch soon.

Philippe Renon submitted a patch:

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.

Patch 361287, "audiosink: expose more audioringbuffer vmethods to child sinks":
0001-audiosink-expose-more-audioringbuffer-vmethods-to-ch.patch

Axel Mårtensson submitted a patch:

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

Patch 372822, "Fix resuming after paused":
0002-audiosink-fix-resuming-after-pause.patch

Philippe Renon said:

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.

Axel Mårtensson said:

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

Philippe Renon said:

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

mentioned in merge request !213 (merged)

added Base Plugins labels

added Stale label

Hello, this issue has been inactive open for more than 60 days. Should it be moved to the monorepo or closed ?