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: <20210422192253.553048-1-marijns95@gmail.com>
In-Reply-To: <20210422192253.553048-1-marijns95@gmail.com>
From:   Marijn Suijten <marijns95@gmail.com>
Date:   Thu, 3 Jun 2021 15:57:19 +0200
Message-ID: <CANX-K3vis3qqDHwPMSH132y5ermf6Gcr2d0Yd_T4Ksd=j6dLUQ@mail.gmail.com>
Subject: Re: [PATCH BlueZ] audio/avrcp: Determine Absolute Volume support from
 feature category 2
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     Marek Czerski <ma.czerski@gmail.com>,
        linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, 22 Apr 2021 at 21:23, Marijn Suijten <marijns95@gmail.com> wrote:
>
> The AVRCP spec (1.6.2) does not mention anything about a version
> requirement for Absolute Volume, despite this feature only existing
> since spec version 1.4.  Android reports a version of 1.3 [1] for its
> "AVRCP remote" (CT) service and mentions in the comment above it itself
> relies on feature bits rather than the exposed version.  As it stands
> BlueZ requires at least version 1.4 making it unable to communicate
> absolute volume levels with even the most recent Android phones running
> Fluoride (have not checked the version on Gabeldorsche).
>
> The spec states that supporting SetAbsoluteVolume and
> EVENT_VOLUME_CHANGED are mandatory when feature level 2 is declared,
> excluded otherwise.  This feature bit is set on Android and, when used
> by this patch, allows for successfully communicating volume back and
> forth despite the version theoretically being too low.
>
> [1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r28/bta/av/bta_av_main.cc#761
> ---
>
> Hi Luiz, Marek,
>
> It's been quite a while since our last mail contact.  As mentioned
> Android simply reports a too low version for its CT despite setting
> category 2 for absolute volume support.  Using this feature instead of
> the version solves being unable to synchronize volume, is that okay with
> you?
>
> - Marijn
>
>  profiles/audio/avrcp.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index 05dd791de..bacd1aeb4 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -4136,13 +4136,16 @@ static void target_init(struct avrcp *session)
>                                 (1 << AVRCP_EVENT_TRACK_REACHED_END) |
>                                 (1 << AVRCP_EVENT_SETTINGS_CHANGED);
>
> +       if (target->features & AVRCP_FEATURE_CATEGORY_2)
> +               session->supported_events |=
> +                               (1 << AVRCP_EVENT_VOLUME_CHANGED);
> +
>         if (target->version < 0x0104)
>                 return;
>
>         session->supported_events |=
>                                 (1 << AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED) |
> -                               (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED) |
> -                               (1 << AVRCP_EVENT_VOLUME_CHANGED);
> +                               (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED);
>
>         /* Only check capabilities if controller is not supported */
>         if (session->controller == NULL)
> --
> 2.31.1
>

Hi Luiz,

Has this patch by chance not reached your inbox?  Would like to
discuss the approach and get this issue fixed.

Any suggestions on solving the test bot error?  I don't think
splitting too-long links over multiple lines is standard practice just
to appease it?

Thanks for looking into it.

- Marijn