On Wed, 2015-03-18 at 17:08 +0530, Arun Raghavan wrote: > 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). Yes, the flexibility quite possibly won't be needed. If you think that the intrusiveness of a new function is that much greater than a new flag that it justifies the risk of adding a limited API that will possibly (but not likely) need to be extended later on, then let's agree to disagree (as I explained earlier, I don't see much difference in the intrusiveness). > > 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. What overall API do you mean? The volume control API? Yes, that's still pending final approval. I think the initial patches will be ready relatively soon, and once the volume control API is agreed upon, then I think would be the time for deciding the details of the new pa_stream_get/set_volume() API. > 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. Do I interpret correctly, that out of the three options: 1) asynchronous pa_stream_get_volume(), with the problems of more complex API and inconsistency with other pa_stream_* query functions, which are all synchronous (with the exception of pa_stream_update_timing_info(), but there's a good reason why that's asynchronous) 2) synchronous pa_stream_get_volume() with redundant event monitoring in clients that do streaming but don't care about the volume 3) synchronous pa_stream_get_volume() with explicit opt-in monitoring with pa_stream_set_track_volume() you prefer 1 or 2? > > 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. If I understand your blacklisting idea correctly, such blacklisting wouldn't be based on the flag that the application sets, because blacklisting an application would mean that the application is not trustworthy, and therefore any flag that it sets or doesn't set can't be trusted either. The stream flag for blacklisting would be a different flag, controlled by the server. -- Tanu
