On Mon, 2012-10-22 at 12:22 -0200, Fl?vio Ceolin wrote: > Hi Tanu, > > On Fri, Oct 19, 2012 at 3:33 PM, Tanu Kaskinen <tanuk at iki.fi> wrote: > > On Tue, 2012-10-09 at 15:16 -0300, Flavio Ceolin wrote: > >> + "global=<Should we operate globally or only inside the same device?>" > >> + "volume=<linear factor for the volume. 0.0 and less is muted while 1.0 is PA_VOLUME_NORM>"); > > > > I don't like the choice of using linear factor as the parameter type. > > The "native" volume type is pa_volume_t. So, I'd like you to use either > > that type as the parameter type, or if that's too un-user-friendly (a > > linear factor isn't very user-friendly either, btw), then accept any of > > these: a plain integer (pa_volume_t), a number followed by a percentage > > sign or a number followed by "dB". There could be > > pa_modargs_get_value_volume() that does the parsing (it could be useful > > elsewhere too). > > > > So, instead of accept a linear value, i'll give to user three options > dB, % and integer, right ? Right. For bonus points, add pa_parse_volume() in pulsecore/core-util.[ch] and use it from pa_modargs_get_value_volume(). > >> + pa_hashmap_put(u->ducking_state, j, pa_sink_input_get_volume(j, v, FALSE)); > >> + } > >> + > >> + aux = *v; > >> + pa_cvolume_set(&aux, aux.channels, u->volume); > >> + pa_sink_input_set_volume(j, &aux, FALSE, FALSE); > > > > I don't think the volume change should be visible outside, so > > pa_sink_input_set_volume() shouldn't be called. Instead, > > j->volume_factor should be set (it's a sort of "invisible" volume > > modifier). There's no good way to do that, however. The volume factor > > feature is used very little, and there's currently no code that needs to > > update it on the fly - currently it's only set once during stream > > creation and it stays the same for the lifetime of the stream. I think > > pa_sink_input_set_volume_factor() needs to be created, which would also > > update j->soft_volume (and j->thread_info.soft_volume). > > > > Or even better, pa_sink_input.volume_factor could be replaced with a > > list (or whatever container makes most sense) of volume factors. There > > can be multiple modules, each independently applying volume factors. If > > there is only one volume variable, those modules will overwrite each > > other's volumes. Therefore, it's better to give each module its own > > volume factor object. > > If multiple modules can apply volume factors, should exists some kind > of priority between them ? > is there a similar case in pulseaudio that i can take a look ? I don't think there needs to be any priority, since all of the factors can be applied simultaneously. For example, one module can apply -10 dB attenuation and another module can apply -5 dB attenuation. When combined, the result is -15 dB attenuation, which is expected. Actually, maybe there's no need to have a list of factors, because a module can remember what factor it applied, and when it doesn't want to apply it anymore, it can undo it. There might be rounding errors, though, so when volume_factor starts at PA_VOLUME_NORM, after a temporary volume factor has been applied and then undone by a module, volume_factor may end up being something else than PA_VOLUME_NORM. So, I'd go with a list after all... Combining factors can be done simply with pa_sw_cvolume_multiply(). -- Tanu
