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
