On Mon, 08.06.09 15:03, Zheng, Huan (huan.zheng at intel.com) wrote:
> Hi, Lennart
Heya!
> The attachment is the patch for volume ramping. Please review.
Here's finally the review. Sorry for the delay!
>
> Impact to PA:
> This patch will enable PA with volume ramping feature. People use
> IPhone may have this kind of experience that when a telephone call
> comes in when user is listening to music, the music is gradually
> volume down to mute; and after the call, the music is gradually
> volume up to previous level. This patch will give PA the ability to
> do the similar thing, and I have tested with this patch, the effect
> is as good as IPhone.
> Two API is exported for this feature:
> void pa_sink_input_set_volume_with_ramping(pa_sink_input *i, const pa_cvolume *volume, pa_bool_t save, pa_bool_t absolute, pa_usec_t t);
> void pa_sink_input_set_mute_with_ramping(pa_sink_input *i, pa_bool_t mute, pa_bool_t save, pa_usec_t t);
This looks pretty good.
> What's included in this patch:
> 1, Fixed various bugs in envelope.c
> 2, Added logic in sink-input.c for volume ramping
>
> Calculation formula is in the comments for the code, so I don't explain in this mail, please check the code. Thanks!
Next time, could you please include the patch inline? That makes it
easy for me to reply to it with a review.
The first part looks pretty OK, however there are a few issues:
> +void pa_envelope_restart(pa_envelope* e) {
> + pa_assert(e);
> +
> + pa_envelope_rewind(e, e->x);
> +}
Hmm, this is supposed to be called by the main thread, right? e->x is
however maintained by IO thread side of things.
This is not particularly well documented in the envelope code I
guess. However, the whole idea of it is that rewind()/apply() can be
run from the IO thread (i.e. as the *reader*) while not being
influenced by modifications of the envelope settings from the main
thread (i.e. as the *writer*), all without locking.
To implement this properly there needs to be a clear regime which
variables may be accessed frm which side threads. And e->x is
controlled exclusively by the IO thread side.
A possible simple fix is to signal the rewind via an atomic int flags
field. i.e. from the main trhead side you simply set this flag, and
from the IO thread side the first thing you do in _apply() is to check
if it is set and reset it and actually do the rewind.
> +pa_bool_t pa_envelope_is_finished(pa_envelope* e) {
> + pa_assert(e);
> +
> + int v;
> + pa_bool_t finished;
> +
> + envelope_begin_read(e, &v);
> + finished = (e->x >= e->points[v].x[e->points[v].n_points-1]);
> + envelope_commit_read(e, v);
> +
> + return finished;
> +}
This too is not really correct in the thread-safety logic: this
function is to be called from the main thead (i.e. the *writer*),
right? However you call the lock functions for *reader* side.
> + /* Set Ramping info */
> + i->ramp_info.is_ramping = FALSE;
> + i->ramp_info.envelope = NULL;
> + i->ramp_info.item = NULL;
Here's a problem: you fill in the envelope object not right-away but
while the IO thread is already running. To do that safely you'd either
have use pa_atomic_ptr_t, or simply allocate it right from the beginning.
> + else if (i->ramp_info.is_ramping) {
> + pa_cvolume v;
> + pa_cvolume_reset(&v, i->sink->sample_spec.channels);
> + /* Return back 1/linear(i->sink->soft_volume) */
> + pa_sw_cvolume_divide(volume, &v,
> &i->sink->soft_volume);
Hmm, this doesn't look right. Volume calculations are generally not
reversible. I am also not sure I get what this is good for.
> +static void sink_input_volume_ramping(pa_sink_input* i, pa_memchunk* chunk){
> + pa_assert(i);
> + pa_assert(chunk);
> + pa_assert(chunk->memblock);
> + pa_assert(i->ramp_info.is_ramping);
> +
> + /* Volume is adjusted with ramping effect here */
> + pa_envelope_apply(i->ramp_info.envelope, chunk);
> +
> + if (pa_envelope_is_finished(i->ramp_info.envelope)) {
> + sink_input_reset_ramping(i);
> + }
Hmm, this is too simple I fear.
To deal with very long hardware buffers PA is able to rewrite those
buffers under specific circumstances. (i.e. if you have 2s of buffers
and want instant volume chnages then PA will rerender the whole buffer
witht the new volume applied) That means that you need to keep the
envelope around not just until you finished to apply it, but
until you can be sure you won't need it anymore.
For this to work, you'd have to leave the envelope around after it
finished until at least pa_sink_get_max_rewind(si->sink) additional
bytes were processed (converted to the sink inputs sample spec of
course). Only then you can now for sure that it won't be tried to
rewind your envelope anymore.
As a side note: you need to make sure that pa_sink_input_rewind()
properly rewinds the envelope which I think is missing right now.
> + if (i->ramp_info.item)
> + pa_envelope_remove(i->ramp_info.envelope,
> i->ramp_info.item);
Hmm, the idea of the enveloping system is that you can have multiple
envelopes active at the same time. I'd thus suggest we'd have two
items here, that in there name make clear what they are used for:
i->ramp_info.mute_item and i->ramp_info.volume_item.
> + v = i->sink->soft_volume;
> + pa_cvolume_remap(&v, &i->sink->channel_map, &i->channel_map);
> +
> + pa_log_debug("Sink's soft volume is %d= %f ", pa_cvolume_avg(&v), pa_sw_volume_to_linear(pa_cvolume_avg(&v)));
> + pa_log_debug("Sink input's soft volume is %d= %f ", pa_cvolume_avg(&i->soft_volume), pa_sw_volume_to_linear(pa_cvolume_avg(&i->soft_volume)));
> +
> + /* Calculation formula are target_abs_vol := i->soft_volume * i->sink->soft_volume
> + * target_apply_vol := lrint(pa_sw_volume_to_linear(target_abs_vol) * 0x10000)
> + * pre_apply_vol := ( previous_virtual_volume / target_virtual_volume ) * target_apply_vol
> + *
> + * Will do volume adjustment inside pa_sink_input_peek and return out 1/i->sink->soft_volume
> + * Reason of doing this is trying to avoid calculation overflow inside apply_envelop,
> + * E.g. Say hardware_volume is not considered, and in FLAT_VOLUME_MODE, set volume from 1 to 0.01 will cause i->soft_volume = 1, i->sink->soft_volume = 0.01
> + * Thus, the value of pre_apply_vol will become 100, target_abs_vol become 1, applify 100 will definitely cause overflow, so we want to apply i->sink->soft_volume in advance here.
> + *
> + * This could avoid calculation overflow at limited level, if hardware_volume is set to 0.01, then, applying i->sink->soft_volume here will not help.
> + */
> + target_abs_vol = pa_sw_volume_multiply(pa_cvolume_avg(&i->soft_volume), pa_cvolume_avg(&v));
> + target_apply_vol = (int32_t) lrint(pa_sw_volume_to_linear(target_abs_vol) * 0x10000);
> + pre_apply_vol = (int32_t)
> ((pa_sw_volume_to_linear(pre_virtual_volume) /
> pa_sw_volume_to_linear(target_virtual_volume)) * target_apply_vol);
I am sorry, I don't follow. I don't see why the sink volumes should
be needed here. Could you try to explain that?
> +static void sink_input_set_ramping_info_for_mute(pa_sink_input* i, pa_bool_t mute, pa_usec_t t) {
> +
> + int32_t cur_vol;
> + pa_cvolume v;
> + pa_assert(i);
> +
> + if (!i->ramp_info.envelope)
> + i->ramp_info.envelope = pa_envelope_new(&i->sink->sample_spec);
> + if (i->ramp_info.item)
> + pa_envelope_remove(i->ramp_info.envelope, i->ramp_info.item);
> +
> + i->ramp_info.item = pa_envelope_add(i->ramp_info.envelope, & i->ramp_info.using_def);
There's some benefit in calling envelop_replace() instead of
remove/add, because the former will make sure there is no gap in what
the IO side sees of this.
> +
> +static void sink_input_reset_ramping(pa_sink_input* i){
> + pa_assert(i);
> +
> + i->ramp_info.is_ramping = FALSE;
> +
> + if (i->ramp_info.envelope && i->ramp_info.item) {
> + pa_envelope_remove(i->ramp_info.envelope, i->ramp_info.item);
> + i->ramp_info.item = NULL;
> + }
> +
> + if (i->ramp_info.envelope)
> + pa_envelope_restart(i->ramp_info.envelope);
> +}
As mentioned, _restart() as it is implemented in your patch should not
be run from the main thread.
> +
> +void pa_sink_input_set_volume_with_ramping(pa_sink_input *i, const pa_cvolume *volume, pa_bool_t save, pa_bool_t absolute, pa_usec_t t){
> + pa_cvolume v;
> + pa_volume_t previous_virtual_volume, target_virtual_volume;
> + pa_sink_input_assert_ref(i);
> +
> + pa_assert(PA_SINK_INPUT_IS_LINKED(i->state));
> + pa_assert(volume);
> + pa_assert(pa_cvolume_valid(volume));
> + pa_assert(pa_cvolume_compatible(volume, &i->sample_spec));
> +
> + if ((i->sink->flags & PA_SINK_FLAT_VOLUME) && !absolute) {
> + v = i->sink->reference_volume;
> + pa_cvolume_remap(&v, &i->sink->channel_map, &i->channel_map);
> + volume = pa_sw_cvolume_multiply(&v, &v, volume);
> + }
> +
> + if (pa_cvolume_equal(volume, &i->virtual_volume))
> + return;
> +
> + previous_virtual_volume = pa_cvolume_avg(&i->virtual_volume);
> + target_virtual_volume = pa_cvolume_avg(volume);
> + pa_log_debug("SetVolumeWithRamping: Virtual Volume From %u=%f to %u=%f\n", previous_virtual_volume, pa_sw_volume_to_linear(previous_virtual_volume),
> + target_virtual_volume, pa_sw_volume_to_linear(target_virtual_volume));
> +
> + i->virtual_volume = *volume;
> + i->save_volume = save;
> +
> + if (i->sink->flags & PA_SINK_FLAT_VOLUME) {
> + pa_cvolume new_volume;
> +
> + /* We are in flat volume mode, so let's update all sink input
> + * volumes and update the flat volume of the sink */
> +
> + pa_sink_update_flat_volume(i->sink, &new_volume);
> + pa_sink_set_volume(i->sink, &new_volume, FALSE, TRUE, FALSE);
> +
> + } else {
> +
> + /* OK, we are in normal volume mode. The volume only affects
> + * ourselves */
> + pa_sink_input_set_relative_volume(i, volume);
> +
> + /* Hooks have the ability to play games with i->soft_volume */
> + pa_hook_fire(&i->core->hooks[PA_CORE_HOOK_SINK_INPUT_SET_VOLUME], i);
> +
> + /* Copy the new soft_volume to the thread_info struct */
> + pa_assert_se(pa_asyncmsgq_send(i->sink->asyncmsgq, PA_MSGOBJECT(i), PA_SINK_INPUT_MESSAGE_SET_SOFT_VOLUME, NULL, 0, NULL) == 0);
> + }
> +
> + sink_input_reset_ramping(i);
> + if (target_virtual_volume > 0)
> + sink_input_set_ramping_info(i, previous_virtual_volume, target_virtual_volume, t);
> +
> + /* The virtual volume changed, let's tell people so */
> + pa_subscription_post(i->core, PA_SUBSCRIPTION_EVENT_SINK_INPUT|PA_SUBSCRIPTION_EVENT_CHANGE, i->index);
> +}
> +
Hmm, this duplicates a lot of code. Would be good if we could do this
a bitt differenlty, so that the common parts can be shared between
set_volume() with and without ramping. Maybe simnply make the old
function a special case of the new one with t=0?
> +void pa_sink_input_set_mute_with_ramping(pa_sink_input *i, pa_bool_t mute, pa_bool_t save, pa_usec_t t){
> +
> + pa_assert(i);
> + pa_sink_input_assert_ref(i);
> + pa_assert(PA_SINK_INPUT_IS_LINKED(i->state));
> +
> + if (!i->muted == !mute)
> + return;
> +
> + sink_input_reset_ramping(i);
> + sink_input_set_ramping_info_for_mute(i, mute, t);
> +
> + i->muted = mute;
> + i->save_muted = save;
> +
> + pa_assert_se(pa_asyncmsgq_send(i->sink->asyncmsgq, PA_MSGOBJECT(i), PA_SINK_INPUT_MESSAGE_SET_SOFT_MUTE, NULL, 0, NULL) == 0);
> + pa_subscription_post(i->core, PA_SUBSCRIPTION_EVENT_SINK_INPUT|PA_SUBSCRIPTION_EVENT_CHANGE, i->index);
> +}
Hmm, the big big problem I see here is that we need to atomically
replace the envelope by the new volume/mute setting. Which is
something I haven't fully wrapped my head around yet.
Also, I am a bit concernced that you code does away with balance and
stuff while the envelope is applied, doesn't it? I'd prefer if the
envelope stuff would be entirely independant of the other volume
control stuff. i.e. when you change the volume/balance/fade while you
are applying e.g. a mute envelope this should have an immediate
effect.
Originally when I planned this, I wanted to apply the softvol
unconditionally, and on top of that the envelope. If I understood
correctly your patch applies either, but not both.
Now, I see that my original plan is flawed a bit since for increasing
ramps we would end up attenuating first and then amplify again, which
really sucks I guess. -- Hmm, unless we'd integrate the softvol
application and the ramp application into one. i.e. by extending
pa_envelope_apply() to take a pa_cvolume parameter that is multiplied
into the envelope factors but is per-channel in contrast to the
envelope factors which are just one for all channels.
So here's what I'd suggest for the hooking up of the envelope stuff
with sink input:
If an envelope is active, we apply the softvol plus the envelope
inside a single call to pa_envelope_apply(), where the envelope factors,
and the softvol volume adjustment happen in one step.
Then, if an envelope is fully finnished, -- and only then - we'd
replace the softvol by a new softvol. So, in a bit of proto code,
ignoring all the threading issue and details:
when the softvol should be changed with ramping:
def.n_points = 2;
def.points_x[0] = 0;
def.points_y.f[0] = 1.0;
def.points_x[1]= 500ms;
def.points_y.f[1] = new_soft_volume / old_soft_volume;
volume_item = pa_envelope_add(e, &def); /* or replace if there already is one */
s->future_soft_volume = new_soft_volume;
and on the thread side when applying the ramping:
if (!ramping_is_active) {
pa_memchunk_volume(chunk, s->soft_volume);
} else {
pa_envelope_apply(e, chunk, s->soft_volume);
if (pa_envelope_finished(volume_item) && rewind_time_passed()) {
/* OK, the envelope is fully completed, so let's get rid
of it and instead apply the new soft volume. */
pa_envelope_deactivate_item(volume_item);
soft_volume = s->future_soft_volume;
}
}
Or something like that if you follow.
Also, please keep in mind that the use case of the enveloping logic is
not just user-influenced volume changes, but also for temporary
automatic ones. e.g. if a module wanted to attenuate music while a
flash clip playes it should do so by simply installing another
envelope in the music stream, and when the clip is done, it would just
replace that envelope again.
I think it is very important to keep in mind that installing envelopes
from other stuff than just user volume/mute changes needs to be
possible.
So much for now. I hope this is not too confusing. If you have any
questions, just ask!
Many thanks for your work so far!
Lennart
--
Lennart Poettering Red Hat, Inc.
lennart [at] poettering [dot] net
http://0pointer.net/lennart/ GnuPG 0x1A015CC4
