[PATCH BlueZ v2] audio/avrcp: Determine Absolute Volume support from feature category 2

[PATCH BlueZ v2] audio/avrcp: Determine Absolute Volume support from feature category 2

[Marijn Suijten]

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
---

Changes since v1:
- Use block comment intead of single-line comment.

 profiles/audio/avrcp.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index d3c9cb795..e530eeab4 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -4175,13 +4175,17 @@ static void target_init(struct avrcp *session)
 				(1 << AVRCP_EVENT_TRACK_REACHED_END) |
 				(1 << AVRCP_EVENT_SETTINGS_CHANGED);
 
+	/* Remote device supports receiving volume notifications */
+	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.33.1

RE: [BlueZ,v2] audio/avrcp: Determine Absolute Volume support from feature category 2

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2364 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=566149

---Test result---

Test Summary:
CheckPatch                    FAIL      0.71 seconds
GitLint                       FAIL      0.33 seconds
Prep - Setup ELL              PASS      52.40 seconds
Build - Prep                  PASS      0.23 seconds
Build - Configure             PASS      9.09 seconds
Build - Make                  PASS      218.92 seconds
Make Check                    PASS      9.54 seconds
Make Distcheck                PASS      260.01 seconds
Build w/ext ELL - Configure   PASS      9.32 seconds
Build w/ext ELL - Make        PASS      203.44 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script with rule in .checkpatch.conf
Output:
[BlueZ,v2] audio/avrcp: Determine Absolute Volume support from feature category 2
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#71: 
[1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r28/bta/av/bta_av_main.cc#761

/github/workspace/src/12569213.patch total: 0 errors, 1 warnings, 19 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/12569213.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - FAIL
Desc: Run gitlint with rule in .gitlint
Output:
[BlueZ,v2] audio/avrcp: Determine Absolute Volume support from feature category 2
1: T1 Title exceeds max length (81>80): "[BlueZ,v2] audio/avrcp: Determine Absolute Volume support from feature category 2"
18: B1 Line exceeds max length (103>80): "[1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r28/bta/av/bta_av_main.cc#761"




---
Regards,
Linux Bluetooth

Re: [PATCH BlueZ v2] audio/avrcp: Determine Absolute Volume support from feature category 2

[Luiz Augusto von Dentz]

Hi Marijn,

On Tue, Oct 19, 2021 at 2:17 AM Marijn Suijten
<marijn.suijten@somainline.org> 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

But I don't think this is valid, if EVENT_VOLUME_CHANGED was
introduced in 1.4 it should have been a reserved value in 1.3 so it
cannot be used. So Android should really report 1.4 on its controller
record, have you tried submitting a patch to Android to change it to
report the version as 1.4? Anyway I'd only adopt such a change if we
could identify the remote end is Android otherwise we risk IOP issues
by reporting a value that is considered reserved with other stacks.

> ---
>
> Changes since v1:
> - Use block comment intead of single-line comment.
>
>  profiles/audio/avrcp.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index d3c9cb795..e530eeab4 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -4175,13 +4175,17 @@ static void target_init(struct avrcp *session)
>                                 (1 << AVRCP_EVENT_TRACK_REACHED_END) |
>                                 (1 << AVRCP_EVENT_SETTINGS_CHANGED);
>
> +       /* Remote device supports receiving volume notifications */
> +       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.33.1
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ v2] audio/avrcp: Determine Absolute Volume support from feature category 2

[Marijn Suijten]

Hi Luiz,

On 2021-10-19 10:38:55, Luiz Augusto von Dentz wrote:
> Hi Marijn,
> 
> On Tue, Oct 19, 2021 at 2:17 AM Marijn Suijten
> <marijn.suijten@somainline.org> 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
> 
> But I don't think this is valid, if EVENT_VOLUME_CHANGED was
> introduced in 1.4 it should have been a reserved value in 1.3 so it
> cannot be used. So Android should really report 1.4 on its controller
> record, have you tried submitting a patch to Android to change it to
> report the version as 1.4? Anyway I'd only adopt such a change if we
> could identify the remote end is Android otherwise we risk IOP issues
> by reporting a value that is considered reserved with other stacks.

I think that's a fair concern, and it took some time to validate this on
the phones I have lying around here.  On a Sony Xperia 1 II running
stock software, both the target and controller version follow what is
set in the developer menu, and range from v1.4 to v1.6.  On the
contrary, a similar phone running a fresh AOSP build on Android 12 still
suffers from the target using version 1.3.  Only the controller version
follows what is set in developer settings.

This leaves me curious what other Android vendors do here.  I doubt they
all go in and bump this by hand just to make absolute volume work
without breaking certification.

On a similar note, various Bluetooth headphones in use here (such as the
WH-1000XM3's which must surely be certified) have worked fine with an
AOSP build only signaling version 1.3 with the FEATURE_CATEGORY_2 bit
set.  Either they're detecting Android, or they're just not caring about
the version and only about the feature bits.  I wouldn't be surprised if
the extra validation performed by BlueZ here is more than most firmware.

As far as I'm aware AVRCP 1.3 doesn't define FEATURE_CATEGORY_2 either
(but I don't have that spec available to check) so IOP would only find
this if it combines v1.3 with that feature bit and then tries to check
CAP_EVENTS_SUPPORTED.  But if it does that, it should also find that
we're not even checking for the controller supporting FEATURE_CATEGORY_2
in the first place, nor are disallowing the controller to send
SetAbsoluteVolume.  That's something we should add for sure, even if we
don't go ahead with decreasing the minimum version for category-2
features below 1.4.
I can send a preliminary patch enforcing this if you want.

I've previously submitted changes to Android and they almost always get
lost in the noise - and even if they get through most Android phones
won't see that update anyway.  It seems there already is some move in
this direction [1] but that's on AOSP master and doesn't even reach
Android 12 (yet), besides requiringg a strange "enablenewavrcp"
property.

Finally, on the subject of incorrect behaviour and IOP, I found
179ccb936 ("avrcp: Set volume if volume changed event is registered")
which also seems counter-intuitive besides going completely against the
spec.  It doesn't seem to have gone in through the mailing lists nor
discusses the affected device and any potential misbehaviour as a
result.  If you're concerned with this patch, is that something you'd
like to keep as well?

- Marijn

[1]: https://android-review.googlesource.com/c/platform/system/bt/+/1827796

> > ---
> >
> > Changes since v1:
> > - Use block comment instead of single-line comment.
> >
> >  profiles/audio/avrcp.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > index d3c9cb795..e530eeab4 100644
> > --- a/profiles/audio/avrcp.c
> > +++ b/profiles/audio/avrcp.c
> > @@ -4175,13 +4175,17 @@ static void target_init(struct avrcp *session)
> >                                 (1 << AVRCP_EVENT_TRACK_REACHED_END) |
> >                                 (1 << AVRCP_EVENT_SETTINGS_CHANGED);
> >
> > +       /* Remote device supports receiving volume notifications */
> > +       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.33.1
> >
> 
> 
> -- 
> Luiz Augusto von Dentz

Re: [PATCH BlueZ v2] audio/avrcp: Determine Absolute Volume support from feature category 2

[Luiz Augusto von Dentz]

Hi Marijn,

On Mon, Oct 25, 2021 at 6:42 AM Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> Hi Luiz,
>
> On 2021-10-19 10:38:55, Luiz Augusto von Dentz wrote:
> > Hi Marijn,
> >
> > On Tue, Oct 19, 2021 at 2:17 AM Marijn Suijten
> > <marijn.suijten@somainline.org> 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
> >
> > But I don't think this is valid, if EVENT_VOLUME_CHANGED was
> > introduced in 1.4 it should have been a reserved value in 1.3 so it
> > cannot be used. So Android should really report 1.4 on its controller
> > record, have you tried submitting a patch to Android to change it to
> > report the version as 1.4? Anyway I'd only adopt such a change if we
> > could identify the remote end is Android otherwise we risk IOP issues
> > by reporting a value that is considered reserved with other stacks.
>
> I think that's a fair concern, and it took some time to validate this on
> the phones I have lying around here.  On a Sony Xperia 1 II running
> stock software, both the target and controller version follow what is
> set in the developer menu, and range from v1.4 to v1.6.  On the
> contrary, a similar phone running a fresh AOSP build on Android 12 still
> suffers from the target using version 1.3.  Only the controller version
> follows what is set in developer settings.
>
> This leaves me curious what other Android vendors do here.  I doubt they
> all go in and bump this by hand just to make absolute volume work
> without breaking certification.
>
> On a similar note, various Bluetooth headphones in use here (such as the
> WH-1000XM3's which must surely be certified) have worked fine with an
> AOSP build only signaling version 1.3 with the FEATURE_CATEGORY_2 bit
> set.  Either they're detecting Android, or they're just not caring about
> the version and only about the feature bits.  I wouldn't be surprised if
> the extra validation performed by BlueZ here is more than most firmware.
>
> As far as I'm aware AVRCP 1.3 doesn't define FEATURE_CATEGORY_2 either
> (but I don't have that spec available to check) so IOP would only find
> this if it combines v1.3 with that feature bit and then tries to check
> CAP_EVENTS_SUPPORTED.  But if it does that, it should also find that
> we're not even checking for the controller supporting FEATURE_CATEGORY_2
> in the first place, nor are disallowing the controller to send
> SetAbsoluteVolume.  That's something we should add for sure, even if we
> don't go ahead with decreasing the minimum version for category-2
> features below 1.4.
> I can send a preliminary patch enforcing this if you want.

So you are saying FEATURE_CATEGORY_2 is not defined in AVRCP 1.3
either? If the is the case we should probably make it clear on the
code with a code comment that we will be going to verify it only
because of Android using it with AVRCP 1.3, but I wonder if there is
anything in the records that you give us the information that it is
indeed Android and we should be fine doing such check since AOSP has
been doing this for a while.

> I've previously submitted changes to Android and they almost always get
> lost in the noise - and even if they get through most Android phones
> won't see that update anyway.  It seems there already is some move in
> this direction [1] but that's on AOSP master and doesn't even reach
> Android 12 (yet), besides requiringg a strange "enablenewavrcp"
> property.
>
> Finally, on the subject of incorrect behaviour and IOP, I found
> 179ccb936 ("avrcp: Set volume if volume changed event is registered")
> which also seems counter-intuitive besides going completely against the
> spec.  It doesn't seem to have gone in through the mailing lists nor
> discusses the affected device and any potential misbehaviour as a
> result.  If you're concerned with this patch, is that something you'd
> like to keep as well?
>
> - Marijn
>
> [1]: https://android-review.googlesource.com/c/platform/system/bt/+/1827796
>
> > > ---
> > >
> > > Changes since v1:
> > > - Use block comment instead of single-line comment.
> > >
> > >  profiles/audio/avrcp.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > > index d3c9cb795..e530eeab4 100644
> > > --- a/profiles/audio/avrcp.c
> > > +++ b/profiles/audio/avrcp.c
> > > @@ -4175,13 +4175,17 @@ static void target_init(struct avrcp *session)
> > >                                 (1 << AVRCP_EVENT_TRACK_REACHED_END) |
> > >                                 (1 << AVRCP_EVENT_SETTINGS_CHANGED);
> > >
> > > +       /* Remote device supports receiving volume notifications */
> > > +       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.33.1
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ v2] audio/avrcp: Determine Absolute Volume support from feature category 2

[Marijn Suijten]

Hi Luiz,

On 2021-10-25 10:48:34, Luiz Augusto von Dentz wrote:
> Hi Marijn,
> 
> On Mon, Oct 25, 2021 at 6:42 AM Marijn Suijten
> <marijn.suijten@somainline.org> wrote:
> >
> > Hi Luiz,
> > [..]
> > As far as I'm aware AVRCP 1.3 doesn't define FEATURE_CATEGORY_2 either
> > (but I don't have that spec available to check) so IOP would only find
> > this if it combines v1.3 with that feature bit and then tries to check
> > CAP_EVENTS_SUPPORTED.  But if it does that, it should also find that
> > we're not even checking for the controller supporting FEATURE_CATEGORY_2
> > in the first place, nor are disallowing the controller to send
> > SetAbsoluteVolume.  That's something we should add for sure, even if we
> > don't go ahead with decreasing the minimum version for category-2
> > features below 1.4.
> > I can send a preliminary patch enforcing this if you want.
> 
> So you are saying FEATURE_CATEGORY_2 is not defined in AVRCP 1.3
> either? If the is the case we should probably make it clear on the
> code with a code comment that we will be going to verify it only
> because of Android using it with AVRCP 1.3, but I wonder if there is
> anything in the records that you give us the information that it is
> indeed Android and we should be fine doing such check since AOSP has
> been doing this for a while.

I'm not sure since I don't have access to the 1.3 spec and haven't found
it online in a quick search.  This however makes the most sense since
feature category 2 seems to _only_ concern itself with volume-related
functionality, which are merely SetAbsoluteVolume and
EVENT_VOLUME_CHANGED and introduced only since 1.4.

I wonder if there's anything specific besides the class indicating a
phone factor and the appearance of an avrcp controller with v1.3 but
this feature bit set.

I'll send a followup in two stages: one that introduces a
FEATURE_CATEGORY_2 check around all volume handling, and another that
bumps the requirement for the peer-controller down to v1.3 with clear
comments about AOSP - unless you have better ideas to detect it :)

> > [..]
> >
> > Finally, on the subject of incorrect behaviour and IOP, I found
> > 179ccb936 ("avrcp: Set volume if volume changed event is registered")
> > which also seems counter-intuitive besides going completely against the
> > spec.  It doesn't seem to have gone in through the mailing lists nor
> > discusses the affected device and any potential misbehaviour as a
> > result.  If you're concerned with this patch, is that something you'd
> > like to keep as well?

Anything on this commit?  I'd like to improve the FEATURE_CATEGORY_2
checks and this is quite alarming and conflicting with that.

- Marijn

Re: [PATCH BlueZ v2] audio/avrcp: Determine Absolute Volume support from feature category 2

[Luiz Augusto von Dentz]

Hi Marijn,

On Mon, Oct 25, 2021 at 11:37 AM Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> Hi Luiz,
>
> On 2021-10-25 10:48:34, Luiz Augusto von Dentz wrote:
> > Hi Marijn,
> >
> > On Mon, Oct 25, 2021 at 6:42 AM Marijn Suijten
> > <marijn.suijten@somainline.org> wrote:
> > >
> > > Hi Luiz,
> > > [..]
> > > As far as I'm aware AVRCP 1.3 doesn't define FEATURE_CATEGORY_2 either
> > > (but I don't have that spec available to check) so IOP would only find
> > > this if it combines v1.3 with that feature bit and then tries to check
> > > CAP_EVENTS_SUPPORTED.  But if it does that, it should also find that
> > > we're not even checking for the controller supporting FEATURE_CATEGORY_2
> > > in the first place, nor are disallowing the controller to send
> > > SetAbsoluteVolume.  That's something we should add for sure, even if we
> > > don't go ahead with decreasing the minimum version for category-2
> > > features below 1.4.
> > > I can send a preliminary patch enforcing this if you want.
> >
> > So you are saying FEATURE_CATEGORY_2 is not defined in AVRCP 1.3
> > either? If the is the case we should probably make it clear on the
> > code with a code comment that we will be going to verify it only
> > because of Android using it with AVRCP 1.3, but I wonder if there is
> > anything in the records that you give us the information that it is
> > indeed Android and we should be fine doing such check since AOSP has
> > been doing this for a while.
>
> I'm not sure since I don't have access to the 1.3 spec and haven't found
> it online in a quick search.  This however makes the most sense since
> feature category 2 seems to _only_ concern itself with volume-related
> functionality, which are merely SetAbsoluteVolume and
> EVENT_VOLUME_CHANGED and introduced only since 1.4.
>
> I wonder if there's anything specific besides the class indicating a
> phone factor and the appearance of an avrcp controller with v1.3 but
> this feature bit set.
>
> I'll send a followup in two stages: one that introduces a
> FEATURE_CATEGORY_2 check around all volume handling, and another that
> bumps the requirement for the peer-controller down to v1.3 with clear
> comments about AOSP - unless you have better ideas to detect it :)
>
> > > [..]
> > >
> > > Finally, on the subject of incorrect behaviour and IOP, I found
> > > 179ccb936 ("avrcp: Set volume if volume changed event is registered")
> > > which also seems counter-intuitive besides going completely against the
> > > spec.  It doesn't seem to have gone in through the mailing lists nor
> > > discusses the affected device and any potential misbehaviour as a
> > > result.  If you're concerned with this patch, is that something you'd
> > > like to keep as well?
>
> Anything on this commit?  I'd like to improve the FEATURE_CATEGORY_2
> checks and this is quite alarming and conflicting with that.

So you want to change that check to check for FEATURE_CATEGORY_2
instead of checking if AVRCP_EVENT_VOLUME_CHANGED has been registered?
Note that the reason why this was done like that is because there is
no record to check the version so I assume there are no features to
check either, I wonder how these devices are even qualified like that.
Anyway we probably need more code comments when we are doing something
like that, and perhaps we could have some entry on main.conf, under
AVRCPO, to make these checks less strict so the system can allow
things like 1.3 with FEATURE_CATEGORY_2 and target without record.


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ v2] audio/avrcp: Determine Absolute Volume support from feature category 2

[Marijn Suijten]

Hi Luiz,

On 2021-10-25 13:32:50, Luiz Augusto von Dentz wrote:
> Hi Marijn,
> 
> On Mon, Oct 25, 2021 at 11:37 AM Marijn Suijten
> <marijn.suijten@somainline.org> wrote:
> >
> > Hi Luiz,
> >
> > On 2021-10-25 10:48:34, Luiz Augusto von Dentz wrote:
> > > Hi Marijn,
> > >
> > > On Mon, Oct 25, 2021 at 6:42 AM Marijn Suijten
> > > <marijn.suijten@somainline.org> wrote:
> > > >
> > > > Hi Luiz,
> > > > [..]
> > > > As far as I'm aware AVRCP 1.3 doesn't define FEATURE_CATEGORY_2 either
> > > > (but I don't have that spec available to check) so IOP would only find
> > > > this if it combines v1.3 with that feature bit and then tries to check
> > > > CAP_EVENTS_SUPPORTED.  But if it does that, it should also find that
> > > > we're not even checking for the controller supporting FEATURE_CATEGORY_2
> > > > in the first place, nor are disallowing the controller to send
> > > > SetAbsoluteVolume.  That's something we should add for sure, even if we
> > > > don't go ahead with decreasing the minimum version for category-2
> > > > features below 1.4.
> > > > I can send a preliminary patch enforcing this if you want.
> > >
> > > So you are saying FEATURE_CATEGORY_2 is not defined in AVRCP 1.3
> > > either? If the is the case we should probably make it clear on the
> > > code with a code comment that we will be going to verify it only
> > > because of Android using it with AVRCP 1.3, but I wonder if there is
> > > anything in the records that you give us the information that it is
> > > indeed Android and we should be fine doing such check since AOSP has
> > > been doing this for a while.
> >
> > I'm not sure since I don't have access to the 1.3 spec and haven't found
> > it online in a quick search.  This however makes the most sense since
> > feature category 2 seems to _only_ concern itself with volume-related
> > functionality, which are merely SetAbsoluteVolume and
> > EVENT_VOLUME_CHANGED and introduced only since 1.4.
> >
> > I wonder if there's anything specific besides the class indicating a
> > phone factor and the appearance of an avrcp controller with v1.3 but
> > this feature bit set.
> >
> > I'll send a followup in two stages: one that introduces a
> > FEATURE_CATEGORY_2 check around all volume handling, and another that
> > bumps the requirement for the peer-controller down to v1.3 with clear
> > comments about AOSP - unless you have better ideas to detect it :)
> >
> > > > [..]
> > > >
> > > > Finally, on the subject of incorrect behaviour and IOP, I found
> > > > 179ccb936 ("avrcp: Set volume if volume changed event is registered")
> > > > which also seems counter-intuitive besides going completely against the
> > > > spec.  It doesn't seem to have gone in through the mailing lists nor
> > > > discusses the affected device and any potential misbehaviour as a
> > > > result.  If you're concerned with this patch, is that something you'd
> > > > like to keep as well?
> >
> > Anything on this commit?  I'd like to improve the FEATURE_CATEGORY_2
> > checks and this is quite alarming and conflicting with that.
> 
> So you want to change that check to check for FEATURE_CATEGORY_2
> instead of checking if AVRCP_EVENT_VOLUME_CHANGED has been registered?
> Note that the reason why this was done like that is because there is
> no record to check the version so I assume there are no features to
> check either, I wonder how these devices are even qualified like that.

Yeah I'd check for the TG version (done before that patch) and
FEATURE_CATEGORY_2 here, but the patch clearly mentions that there's no
target to begin with.  This seems counter-intuitive and more like a
fluke (maybe the SDP cache got of date, or the device dynamically
"un"-publishes this record) and without knowing what device this was
happening on it's very hard to validate if/whether this is still valid.

> Anyway we probably need more code comments when we are doing something
> like that, and perhaps we could have some entry on main.conf, under
> AVRCPO, to make these checks less strict so the system can allow
> things like 1.3 with FEATURE_CATEGORY_2 and target without record.

I totally agree that all these edge-cases should use extra comments to
detail what's going on, and I like the idea of hiding them behind config
flags.  That way we can pass validation without worries and at the same
time give distros and individuals the ability to be less strict, though
we'll have to think about the default value for for example this AOSP
workaround.

Based on your suggestion I'll also move absolute volume control without
target SDP record behind a similar flag, and add Yu Liu to the CC here
to see if we can get some more clarification. Yu, can you detail what
device this was happening on, and if this is still the case?  Thanks!

- Marijn