[pulseaudio-discuss] [RFC] Per-client flat-volumes control

[Arun Raghavan]

On 11 March 2015 at 19:32, Tanu Kaskinen <tanu.kaskinen at linux.intel.com> wrote:
> Hi Arun,
>
> As you requested, I'm reviving the per-stream flat volumes discussion.

Thanks for helping revive this!

> My preference would still be to add a separate "stream-local" volume
> control pointer to pa_sink_input_info, but assuming that you haven't
> changed your mind, that idea doesn't seem feasible to get through.
>
> You seemed positive about extending the pa_stream API by adding
> pa_stream_get_volume() and pa_stream_set_volume(), and some mechanism to
> choose whether those functions should deal with the stream local volume
> or the possibly-flat volume.
>
> You suggested using a stream flag for controlling the volume mode, but
> using a flag would prevent the application from switching the mode after
> creating the stream, so maybe pa_stream_set_volume_mode() would be
> better?

My reasoning with using a flag rather than a new function was that a
new function increases the API surface, whereas a flag is merely an
opt-in parameter on an existing API -- most developers won't have to
look at or use this.

The only thing the API buys us is the ability to switch behaviour at
runtime, but I can't see any reason that apps might want to do this
(so it's extra API surface for flexibility that, afaict, we don't
need).

> Another solution would be to have separate functions for dealing with
> the stream-local volume and the possibly-flat volume. For example:
>
>     pa_stream_get_volume() (possibly-flat)
>     pa_stream_set_volume() (possibly-flat)
>     pa_stream_get_local_volume() (stream-local)
>     pa_stream_set_local_volume() (stream-local)

I prefer a single API for the reasons I mentioned above.

> Then there's the question of whether to use pa_cvolume or something
> else. My opinion is that we shouldn't use pa_cvolume - I'm trying to
> move away from that representation with the volume control work. If we
> don't use pa_cvolume, then the alternatives are to use pa_bvolume or
> separate attributes for volume and balance (and mute?).
>
> (A note about pa_bvolume: while the current plan is to introduce
> pa_bvolume in the volume control work, I'm not 100% sure that that plan
> is going to hold - I have some suspicion that pa_bvolume might not be
> needed after all. Instead, it might be better to have separate
> attributes for volume, balance and mute in volume controls.)

My concern here is that we don't know how the overall API will look,
and other volume API we have uses pa_cvolume. Not using it in this one
case just makes it inconsistent. I guess we should figure out how to
deal with the transition away from pa_cvolume, if that's what we want,
but I'd like to avoid an intermediate state where we have one part of
the API that follows one convention, and another that follows
something different (unless we can break it up into something simple
like "for all pa_volume_control* API, pa_cvolume is not used", and
even that I'd like to eventually make consistent).

> Another open question is that should pa_stream_get_volume() be an async
> operation, or should the pa_stream object automatically track the stream
> and device volume? I'd prefer the getter function(s) to not be
> asynchronous, that is, pa_stream would automatically subscribe to stream
> and device changes and cache the volume locally in the application
> process. If that's what we should do, the next question is that should
> *every* application subscribe to the changes even if the application
> doesn't actually care about the volume? If not, then the subscription
> needs to be explicitly enabled somehow, for example with
> pa_stream_set_track_volume().

At the risk of sounding like a broken record (or a stuck ringbuffer),
I'd like to avoid new API if possible.

> -- Tanu
>
> Here's my earlier mail to which I never got an answer, in case you want
> to review it before replying:
>
> On Fri, 2014-08-15 at 20:06 +0300, Tanu Kaskinen wrote:
>> On Wed, 2014-08-13 at 16:58 +0530, Arun Raghavan wrote:
>> > On 11 August 2014 20:31, Tanu Kaskinen <tanu.kaskinen at linux.intel.com> wrote:
>> > > I don't really agree on the API niceness argument, because in my
>> > > proposal there are just two uint32_t fields added (indexes of the
>> > > always-relative volume control objects), one to pa_sink_input_info and
>> > > one to pa_source_output_info. I don't see much difference to adding a
>> > > flag.
>> >
>> > It's not just a matter of number of fields, but also the number of
>> > ideas the developer using our API has to walk through to use it
>> > successfully. Most developers don't need to deal with this problem, so
>> > we not add complexity for them.
>>
>> I suppose the complexity that you mean is the need to make a choice
>> between using the volume_control field or the relative_volume_control
>> field. The same choice has to be made with the flag: should the flag be
>> used or not. When you say "don't need to deal with this problem", I
>> suppose "not dealing" means being ignorant about the possibility of
>> using relative volume. It may indeed be easier to be ignorant of the
>> existence of the flag than being ignorant about the
>> relative_volume_control field, but as soon as the developer reads
>> through the list of available stream flags, s/he will be hit by the same
>> complexity either way.
>>
>> But I don't strictly need the relative_volume_control fields in the
>> client API, so I may as well leave those out, if a satisfactory solution
>> is found for the UI problems in your original proposal. The fields may
>> be added later if needed.
>>
>> > >> One option is to have the relative volume not be exposed as API, but
>> > >> be the volume that is controlled if this stream flag is set (i.e.
>> > >> pa_sink_input_set_volume() would modify the relative volume if the
>> > >> stream flag is set).
>> > >
>> > > So the idea is that the browser sees relative volume, but pavucontrol
>> > > sees flat volume. This would otherwise work, but since the browser and
>> > > pavucontrol use the same API, there's no way for the server to know
>> > > which behaviour is desired for a given set or get volume operation,
>> > > except by making the flag client-specific, and that seems ugly to me.
>> > > What if you want to implement pavucontrol in the browser and the
>> > > in-browser-pavucontrol and the stream share the same client connection?
>> > > In that case the server has absolutely no way to distinguish which
>> > > behaviour is wanted for a given get/set volume operation.
>> >
>> > I possibly don't understand this - the browser sets the flag on the
>> > stream that it wants to apply this behaviour to, so the client
>> > connection it sets up is unaffected by it.
>>
>> Did I interpret you wrong then, you did not mean to make a difference
>> between how pavucontrol and the web browser see the volume of the web
>> browser's stream? What does this proposal (I mean the quoted part above:
>> "One option is to have...") solve, then - how is the end result any
>> different from what you proposed first?

The end result isn't really different -- I think I proposed this as a
way to keep the server-side logic simple and push the volume
calculation to the client side.

>> > > One solution would be to add pa_stream_set_volume() and
>> > > pa_stream_get_volume() functions instead of the flag, which would be
>> > > available only to the stream owner. Those functions could then have a
>> > > boolean argument for deciding between relative/absolute. But then this
>> > > is certainly not any simpler than the two added integers that I proposed
>> > > for the introspect API...
>> >
>> > I like the idea of a set/get volume API, but not of exposing the
>> > relative vs. absolute thing (for the same reason of added API
>> > complexity). I would rather have the flag set on the stream and have
>> > the API pick the right behaviour based on that.
>>
>> Do you mean that the flag would be a client-side-only thing? A
>> client-side flag + pa_stream_get/set_volume() should work without the UI
>> problems of the original proposal.

I'm fine with either. In the future we might want to set the flag on a
stream from the server side (you might have a blacklist module that
forces badly behaving applications to use relative volume). This could
still be handled by passing that not-flat-volume flag back in the
protocol.

-- Arun