[pulseaudio-discuss] Properties to suppress save/restore of stream volumes

[Tanu Kaskinen]

On Wed, 2011-02-02 at 13:29 +0530, Arun Raghavan wrote:
> Currently Rhythmbox does fading using the GStreamer 'volume' element, so
> this problem doesn't exist with Rhythmbox as it stands now. Some
> background ...
> 
> To play audio, Rhythmbox/Totem/Banshee and others use gconfaudiosink
> using the "music" profile. In most systems, this is just directly mapped
> to pulsesink. The pulsesink default buffer-time value is pretty low and
> thus not power-efficient (~15 wakeups/s just from the alsa-sink thread).
> While trying to make the default values for this larger, I ran across a
> problem with the RB xfade backend. Having a long buffer means that you
> introduce a buffer-time-sized latency. Obviously, a 1+ second delay
> between hitting next and the actual fading is not acceptable.
> 
> Since GStreamer doesn't support rewinding the stream in any simple
> fashion. there doesn't seem to be a direct approach to fixing this.

Would implementing rewind support in gstreamer count as a direct
approach? There's clearly a need for it.

> Instead, I've been writing a new xfade backend that creates a new
> playbin2 pipeline (and thus new pulsesink) for each stream, and then
> directly controlling the volume on the pulsesink element for fades.
> 
> Hope this clarifies the situation.

Yes, thank you for the clear explanation.

> > Even with this patch, how can fade-in work? If Rhythmbox disables volume
> > restoring when creating a new stream and fading it in, how does it know
> > what should be the target volume?
> 
> Basically, we allow the volume to be restored for the first stream, and
> then maintain the current volume till the end. Thus, when we know we're
> fading a stream in from an existing stream, we don't need to restore the
> volume.

I now realized that your patch actually breaks things. If the xfade
pipeline disables volume restoring, then volume control through
stream-restore doesn't work anymore for that particular stream. If the
volume of the entry that is being applied to RB is updated by some
external program, and the external program chooses to apply updates
immediately (which is the usual case), then RB volume is not updated.

I think this particular problem can be solved simply by removing the
should_restore_volume() check from apply_entry(). The check in
sink_input_fixate_hook_callback() should cover the documented
functionality (prevent restoring at stream creation time).

Uh, actually, for sink_input_fixate_hook_callback(), isn't it already
enough that module-stream-restore checks whether the volume is set for
the new stream? You initialize the volume to zero when starting fade-in,
right? Is the restore_volume property completely redundant?

> > About the namespace: the properties are module-stream-restore specific,
> > so I think the namespace should be "module-stream-restore" instead of
> > "stream". I know that pa_sink_input has field save_volume that is
> > managed in the core, so that makes the volume restoration feature sort
> > of part of the core, but I think that's so just because making
> > module-stream-restore fully self-contained wasn't seen worth the
> > required effort. Using the "module-stream-restore" namespace doesn't
> > induce any extra effort, so I think it should be used.
> 
> I've no objection to changing the namespace of the property itself. Do
> you also mean that we should change the define, though?
> PA_PROP_MODULE_STREAM_RESTORE_RESTORE_VOLUME seems unwieldy to me, and
> there's no reason that clients should need to know about
> module-stream-restore.

Volume restoring isn't part of the core, so the clients already know
about module-stream-restore if they set those properties. Also, I think
"restoring volume" is not clearly defined outside the context of
module-stream-restore. Should all other modules, that affect the initial
volume of a new stream, also respect the properties?

I'd actually put the #define to ext-stream-restore.h, and use name
PA_EXT_STREAM_RESTORE_PROP_SAVE_VOLUME.

I have a feeling that this approach for fading can also break in
situations where volumes are decided by some other logic than the
standard module-stream-restore way. It may actually be that the only
robust way to start any stream is to never set the initial volume at the
application side, unless the application knows the Pulseaudio
configuration. I don't have proof for that, though, and having a feature
that breaks in theoretical cases is probably better than not having the
feature at all (cross-fading in this case).

I would personally just request lower latency when using cross-fade in
RB, but then again, I don't use cross-fading in the first place, and I
don't run RB in any power-challenged device, so I may not be the best
judge here. It would be nice to fix gstreamer to support rewinding, or
maybe implement a fading module in Pulseaudio that could be accessed
through pulsesink in gstreamer, but those are probably more work than
you're willing to do. I don't oppose having your patch in Pulseaudio,
with the suggested changes.

-- 
Tanu