Hi Sonny,
On 6/8/21 8:37 PM, Sonny Sasaka wrote:
Hi Marijn,
Thanks for your inputs. Having a separate SetAbsoluteVolume API that
blocks (until the peer acknowledges the change) is certainly more
featureful than this patch's approach, since the media player can
decide what to do with pending SetAbsoluteVolume calls and have the
flexibility to handle the situation. However, there is also a value in
the shorter term approach that this patch without any changes in the
media player part and has been tested to solve the janky slider issue
in Chrome OS. I believe that this would also solve PulseAudio's
similar janky slider issue for the short term.
If Marijn and Luiz are okay with the shorter term approach first, I
will continue updating this patch according to Luiz's comments, but
otherwise I will abandon this patch in favor of the separate
SetAbsoluteVolume API that Marijn will provide.
With no acknowledgement from Luiz yet I've gone ahead and added an
experimental `uint16 SetVolume(uint16)` call to "MediaTransport1" to
start testing this behaviour. You can find the commits here:
https://github.com/MarijnS95/bluez/commits/master
This has only been tested with dbus-send, the PA changes still have to
be written. Given the original review on Sonny's patches we might want
to replace the allocation with `pending` members on the session instead,
so that we can possibly do some debouncing if multiple .Set calls
arrive. Seems like we need three members then:
volume: current known volume between BlueZ and the peer.
pending_volume: an active AVRCP SetAbsoluteVolume call is in progress
with this value. Though this could also be a non-null
DBusMessage *volume_reply; as we don't need the
requested volume anymore.
next_volume: a client already queued a new value through .Set, while
SetAbsoluteVolume (pending_volume != -1) is still in
flight.
While putting this together I noticed that manually calling `.Set
"MediaTransport1" "Volume" XX` doesn't notify other applications. What
happens is that `a2dp->volume` is set to the actual volume (in
set_volume), and no "PropertiesChanged" is emitted unless we're in
"notify" mode ("we are the headphones"). Then, as soon as the peer
replies (avrcp_handle_set_volume, media_transport_update_device_volume,
media_transport_update_volume) we see that `a2dp->volume == volume` and
never emit "PropertiesChanged" either, leaving all other clients unaware
of the change.
This seems simply addressed by not setting `a2dp->volume` set_volume()
and instead rely on that to happen in avrcp_handle_set_volume, just like
I'm doing in the handler for this new SetVolume function.
That might already solve your problem in CrOS, by simply waiting for a
property change notification before sending the next volume change. We
however won't be able to distinguish it from a button-press on the
headphones by the user, but I'm not entirely sure anymore if that's
still needed.
Marijn
PS: Since these messages seem to come through, Luiz have you received my
patch to address AVRCP Absolute Volume missing when connected to Android
phones?
