'Twas brillig, and Tanu Kaskinen at 07/07/11 20:42 did gyre and gimble:
> On Thu, 2011-07-07 at 10:55 +0100, Colin Guthrie wrote:
>> Some sink flags are really just a product of what callbacks
>> set on the sink. Rather than assert when the flags don't
>> match the callbacks registered, just set them automatically.
>
> For the hw volume/mute flags the automation is a bit broken: the hw
> flags are enabled also for virtual devices that use the volume/mute
> callbacks for something (like adjusting the output stream volume). Many
> virtual devices seem to set the hw volume/mute flags explicitly for some
> reason, so this was broken already before, but for another reason...
Personally I think this is OK. The flags are really misnamed with "HW"
in them, but really they mean "have volume callbacks". This was how they
were used before (the asserts made sure of this generally) so I think
this is acceptable. If you feel strongly about it tho', please do speak
up and I can reconsider.
> Of course it's not a big deal in this case if the flags don't correspond
> to the reality, since the flags are mostly useful for debugging
> purposes, but if the flags are not trustworthy, why have them at all?
Well, it's a fair point, but I think we can mitigate this a little by
not naming the flags with "HW" in the string in pacmd/pactl. This makes
them reflect reality a bit more.
> Just so that the alsa modules can report their status? I'd either make
> the hw flags non-automatic (but still dynamic) or remove the flags
> altogether and use proplists to report the status in the alsa modules.
The latter is pretty much how I did do it originally, but Lennart was
more in favour of just keeping the flags. Personally I'm not really
fussed either way.
The "automatic" setting is more or less for convenience and clarity as I
didn't want to implement the same logic in multiple places and for
things to be matched up manually in order to reflect accurate information.
> Another thing (this is not something that needs to be fixed quickly,
> just FYI): another problem with the flags is that the virtual device
> flags depend on the master device flags. If the virtual device is moved
> to another master device, the flags don't get updated.
Yeah I did notice this. I figured it would be pretty trivial to add a
couple loops to propagate the flags, but didn't do that in my patch to
keep things focused on the task in hand.
>> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
>> index 45761a6..15c07a5 100644
>> --- a/src/pulsecore/sink.c
>> +++ b/src/pulsecore/sink.c
>> @@ -462,8 +462,19 @@ void pa_sink_put(pa_sink* s) {
>> pa_assert(s->thread_info.min_latency <= s->thread_info.max_latency);
>>
>> /* Generally, flags should be initialized via pa_sink_new(). As a
>> - * special exception we allow volume related flags to be set
>> - * between _new() and _put(). */
>> + * special exception we allow some volume related flags to be set
>> + * between _new() and _put(). However some are dictated by what
>> + * callbacks were registered so we set those here.
>> + *
>> + * Note: All of these flags set here can change over the life time
>> + * of the sink. */
>> + s->flags &= ~(PA_SINK_HW_VOLUME_CTRL|PA_SINK_HW_MUTE_CTRL|PA_SINK_SYNC_VOLUME|PA_SINK_FLAT_VOLUME);
>
> This breaks the "force flat volume" feature of module-virtual-sink.
> Would it do any harm to leave PA_SINK_FLAT_VOLUME out of this list?
So it does. I'll work something out (probably leaving it out and
duplicating the flat volume flag setting to the alsa-sink/source code,
but if I can find something that reduces duplication then all the better).
>> + if (s->get_volume || s->set_volume)
>> + s->flags |= PA_SINK_HW_VOLUME_CTRL;
>> + if (s->get_mute || s->set_mute)
>> + s->flags |= PA_SINK_HW_MUTE_CTRL;
>
> I don't think providing get_volume is really enough to claim hw volume
> control support. The same goes for get_mute. I'd check only for
> set_volume and set_mute.
Seems fair enough to me. I'll do that.
>> + if (s->write_volume)
>> + s->flags |= PA_SINK_SYNC_VOLUME;
>>
>> /* XXX: Currently decibel volume is disabled for all sinks that use volume
>> * sharing. When the master sink supports decibel volume, it would be good
>> @@ -472,12 +483,17 @@ void pa_sink_put(pa_sink* s) {
>> * a master sink to another. One solution for this problem would be to
>> * remove user-visible volume altogether from filter sinks when volume
>> * sharing is used, but the current approach was easier to implement... */
>> + /* We always support decibel volumes in software, otherwise we leave it to
>> + * the sink implementor to set this flag as needed.
>> + *
>> + * Note: This flag can also change over the life time of the sink. */
>> if (!(s->flags & PA_SINK_HW_VOLUME_CTRL) && !(s->flags & PA_SINK_SHARE_VOLUME_WITH_MASTER))
>> s->flags |= PA_SINK_DECIBEL_VOLUME;
>>
>> if ((s->flags & PA_SINK_DECIBEL_VOLUME) && s->core->flat_volumes)
>> s->flags |= PA_SINK_FLAT_VOLUME;
>>
>> +
>
> Extra empty line.
Factored out later, but I'll squash it in this commit for cleanliness.
>> diff --git a/src/pulsecore/source.c b/src/pulsecore/source.c
>> index 7024802..121c358 100644
>> --- a/src/pulsecore/source.c
>> +++ b/src/pulsecore/source.c
>> @@ -394,8 +394,19 @@ void pa_source_put(pa_source *s) {
>> pa_assert(s->thread_info.min_latency <= s->thread_info.max_latency);
>>
>> /* Generally, flags should be initialized via pa_source_new(). As a
>> - * special exception we allow volume related flags to be set
>> - * between _new() and _put(). */
>> + * special exception we allow some volume related flags to be set
>> + * between _new() and _put(). However some are dictated by what
>> + * callbacks were registered so we set those here.
>> + *
>> + * Note: All of these flags set here can change over the life time
>> + * of the source. */
>> + s->flags &= ~(PA_SOURCE_HW_VOLUME_CTRL|PA_SOURCE_HW_MUTE_CTRL|PA_SOURCE_SYNC_VOLUME|PA_SOURCE_FLAT_VOLUME);
>
> I noticed that you've implemented the "force flat volume" feature also
> for module-virtual-source. Too bad you broke it now :) (See the comment
> above about module-virtual-sink.)
Hehe, yeah point take. I didn't grep well enough for the flat volume
bits....
>
>> + if (s->get_volume || s->set_volume)
>> + s->flags |= PA_SOURCE_HW_VOLUME_CTRL;
>> + if (s->get_mute || s->set_mute)
>> + s->flags |= PA_SOURCE_HW_MUTE_CTRL;
>
> Same comment as for sinks.
ACK
Cheers for the review :)
Col
--
Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/
Day Job:
Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
Mageia Contributor [http://www.mageia.org/]
PulseAudio Hacker [http://www.pulseaudio.org/]
Trac Hacker [http://trac.edgewall.org/]
