Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <CABBYNZKcr74zYzPMcLo5+-49Fv02Kxoyf11bNPSuSGWThz-eqQ@mail.gmail.com>
 <9d386692-4c1c-b262-bca2-cec3568dc579@somainline.org> <CABBYNZLXRK9TN_TKdj5T7oP3D_HaeQiBsaCaRtE7nAK9hYuh-w@mail.gmail.com>
 <f3e18adc-b1ad-2ab5-164e-43a1ae20f708@somainline.org> <CAO271mkpy5W0KB60X5G1mwp92bB+K2Ru3ODP8K2APCkjfkX70w@mail.gmail.com>
 <391e3587-bb19-05be-cc36-08a8c91916de@somainline.org>
In-Reply-To: <391e3587-bb19-05be-cc36-08a8c91916de@somainline.org>
From:   Sonny Sasaka <sonnysasaka@chromium.org>
Date:   Mon, 9 Aug 2021 10:46:50 -0700
Message-ID: <CAO271mk0N4yyA74kzB14y8nbFKfT914JNA7Hvq8QMCfs-VR2zw@mail.gmail.com>
Subject: Re: [PATCH BlueZ] Queue SetAbsoluteVolume if there is an in-progress one.
To:     Marijn Suijten <marijn.suijten@somainline.org>
Cc:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Marijn,

Thank you for following up. Chrome OS has temporarily adopted my patch
to resolve the issue without changing the audio client. We will pick
up your patch at the next uprev.


On Sun, Aug 8, 2021 at 3:15 AM Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> 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?