| Message ID | 20200711115031.123793-1-marijns95@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [BlueZ] audio/avrcp: Always update transport volume regardless of player | expand |
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.
While we are preparing for reviewing the patches, we found the following
issue/warning.
Test Result:
checkpatch Failed
Outputs:
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#43:
[2]: https://specifications.freedesktop.org/mpris-spec/2.2/Player_Interface.html#Property:Volume
WARNING:LONG_LINE_COMMENT: line over 80 characters
#59: FILE: profiles/audio/avrcp.c:3630:
+ /* Always update the transport volume, which is separate from the player */
WARNING:LONG_LINE_COMMENT: line over 80 characters
#71: FILE: profiles/audio/avrcp.c:4382:
+ /* Always update the transport volume, which is separate from the player */
- total: 0 errors, 3 warnings, 53 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.
Your patch has style problems, please review.
NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPLIT_STRING SSCANF_TO_KSTRTO
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
---
Regards,
Linux Bluetooth
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. While we are preparing for reviewing the patches, we found the following issue/warning. Test Result: checkgitlint Failed Outputs: 39: B1 Line exceeds max length (96>80): "[2]: https://specifications.freedesktop.org/mpris-spec/2.2/Player_Interface.html#Property:Volume" --- Regards, Linux Bluetooth
Hi Marijn, On Sat, Jul 11, 2020 at 4:50 AM Marijn Suijten <marijns95@gmail.com> wrote: > > `Volume` is a special property that not only exists on players but also > on the transport (see org.bluez.MediaTransport1). A player is not > attached when the controller does not support FEATURE_CATEGORY_1, which > is common on headphones without media browsing capabilities. > > On such audio devices (headphones, in-ears and the like) Absolute Volume > is not available unless an external player is registered > (org.bluez.Media1.RegisterPlayer) and the device sends a volume event > back after that to set a2dp->volume in transport.c to a valid value > (causing volume_exists to finally return true). > > This [1] mail thread denoting the same issue has a solution to at least > request capabilities from the controller, but the proposed player object > is not created on category 2 devices. Any notifications received on > AVRCP_EVENT_VOLUME_CHANGED (avrcp_volume_changed) that is subsequently > registered, or handling the result of avrcp_set_volume in > avrcp_handle_set_volume will be ignored unless said player is present. > > This issue is not addressed by adding a fake player but instead dealing > with the fact that volume is "special" and available on the transport > regardless of the existence of a player. This is confirmed in > avrcp_get_capabilities_resp as well which requires a player to register > any event except AVRCP_EVENT_VOLUME_CHANGED. > > The applied solution moves media_transport_update_device_volume out of > the player and into avrcp_volume_changed/avrcp_handle_set_volume where > it is unconditionally called. These functions are the only users of > avrcp_player->set_volume. > > Note that the volume member of media_player is never used which seems a > result of updating from org.bluez.MediaPlayer1 to > org.mpris.MediaPlayer2.Player in > 15e421737ccc4696ed567edcc24d178aedb47854, where the volume property [2] > is left out. This is actually on purpose since the volume notification indicates the volume at the device side we don't want to change the player volume as well as it would most likely result in duplicating the volume change both at sink and source, note that the likes of handling the transport like pulseaudio would already notify the device volume level change without actually applying any change to the volume locally. > [1]: https://marc.info/?l=linux-bluetooth&m=145337574806153 > [2]: https://specifications.freedesktop.org/mpris-spec/2.2/Player_Interface.html#Property:Volume > > --- > Hi, > > This is is a resend from an earlier mail that didn't comply with the > contributor guidelines. Seeing that the topic of AVRCP volume is brought > up recently it is about time to repair it and hereby send it again. > > I still have an incomplete patch lying around that synchronizes Volume > on org.mpris.MediaPlayer2.Player back when this patch was written 6 > months ago. It'll require some time to get back in to it and finalize > it, let me know if that's desired. > > - Marijn Suijten > > profiles/audio/avrcp.c | 12 ++++++++---- > profiles/audio/media.c | 16 ---------------- > 2 files changed, 8 insertions(+), 20 deletions(-) > > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c > index e2428250e..8370c8a44 100644 > --- a/profiles/audio/avrcp.c > +++ b/profiles/audio/avrcp.c > @@ -3625,12 +3625,13 @@ static void avrcp_volume_changed(struct avrcp *session, > struct avrcp_player *player = target_get_player(session); > uint8_t volume; > > - if (!player) > - return; > - > volume = pdu->params[1] & 0x7F; > > - player->cb->set_volume(volume, session->dev, player->user_data); > + /* Always update the transport volume, which is separate from the player */ > + media_transport_update_device_volume(session->dev, volume); > + > + if (player) > + player->cb->set_volume(volume, session->dev, player->user_data); > } > > static void avrcp_status_changed(struct avrcp *session, > @@ -4378,6 +4379,9 @@ static gboolean avrcp_handle_set_volume(struct avctp *conn, uint8_t code, > > volume = pdu->params[0] & 0x7F; > > + /* Always update the transport volume, which is separate from the player */ > + media_transport_update_device_volume(session->dev, volume); > + > if (player != NULL) > player->cb->set_volume(volume, session->dev, player->user_data); > > diff --git a/profiles/audio/media.c b/profiles/audio/media.c > index 993ecb3b3..a0173fdd4 100644 > --- a/profiles/audio/media.c > +++ b/profiles/audio/media.c > @@ -1202,27 +1202,11 @@ static uint32_t get_duration(void *user_data) > static void set_volume(uint8_t volume, struct btd_device *dev, void *user_data) > { > struct media_player *mp = user_data; > - GSList *l; > > if (mp->volume == volume) > return; > > mp->volume = volume; > - > - for (l = mp->adapter->endpoints; l; l = l->next) { > - struct media_endpoint *endpoint = l->data; > - struct media_transport *transport; > - > - /* Volume is A2DP only */ > - if (endpoint->sep == NULL) > - continue; > - > - transport = find_device_transport(endpoint, dev); > - if (transport == NULL) > - continue; > - > - media_transport_update_volume(transport, volume); > - } > } > > static bool media_player_send(struct media_player *mp, const char *name) > -- > 2.27.0 >
Hi Marijn, On Mon, Jul 13, 2020 at 5:05 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Marijn, > > On Sat, Jul 11, 2020 at 4:50 AM Marijn Suijten <marijns95@gmail.com> wrote: > > > > `Volume` is a special property that not only exists on players but also > > on the transport (see org.bluez.MediaTransport1). A player is not > > attached when the controller does not support FEATURE_CATEGORY_1, which > > is common on headphones without media browsing capabilities. > > > > On such audio devices (headphones, in-ears and the like) Absolute Volume > > is not available unless an external player is registered > > (org.bluez.Media1.RegisterPlayer) and the device sends a volume event > > back after that to set a2dp->volume in transport.c to a valid value > > (causing volume_exists to finally return true). > > > > This [1] mail thread denoting the same issue has a solution to at least > > request capabilities from the controller, but the proposed player object > > is not created on category 2 devices. Any notifications received on > > AVRCP_EVENT_VOLUME_CHANGED (avrcp_volume_changed) that is subsequently > > registered, or handling the result of avrcp_set_volume in > > avrcp_handle_set_volume will be ignored unless said player is present. > > > > This issue is not addressed by adding a fake player but instead dealing > > with the fact that volume is "special" and available on the transport > > regardless of the existence of a player. This is confirmed in > > avrcp_get_capabilities_resp as well which requires a player to register > > any event except AVRCP_EVENT_VOLUME_CHANGED. > > > > The applied solution moves media_transport_update_device_volume out of > > the player and into avrcp_volume_changed/avrcp_handle_set_volume where > > it is unconditionally called. These functions are the only users of > > avrcp_player->set_volume. > > > > Note that the volume member of media_player is never used which seems a > > result of updating from org.bluez.MediaPlayer1 to > > org.mpris.MediaPlayer2.Player in > > 15e421737ccc4696ed567edcc24d178aedb47854, where the volume property [2] > > is left out. > > This is actually on purpose since the volume notification indicates > the volume at the device side we don't want to change the player > volume as well as it would most likely result in duplicating the > volume change both at sink and source, note that the likes of handling > the transport like pulseaudio would already notify the device volume > level change without actually applying any change to the volume > locally. > > > [1]: https://marc.info/?l=linux-bluetooth&m=145337574806153 > > [2]: https://specifications.freedesktop.org/mpris-spec/2.2/Player_Interface.html#Property:Volume > > > > --- > > Hi, > > > > This is is a resend from an earlier mail that didn't comply with the > > contributor guidelines. Seeing that the topic of AVRCP volume is brought > > up recently it is about time to repair it and hereby send it again. > > > > I still have an incomplete patch lying around that synchronizes Volume > > on org.mpris.MediaPlayer2.Player back when this patch was written 6 > > months ago. It'll require some time to get back in to it and finalize > > it, let me know if that's desired. > > > > - Marijn Suijten > > > > profiles/audio/avrcp.c | 12 ++++++++---- > > profiles/audio/media.c | 16 ---------------- > > 2 files changed, 8 insertions(+), 20 deletions(-) > > > > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c > > index e2428250e..8370c8a44 100644 > > --- a/profiles/audio/avrcp.c > > +++ b/profiles/audio/avrcp.c > > @@ -3625,12 +3625,13 @@ static void avrcp_volume_changed(struct avrcp *session, > > struct avrcp_player *player = target_get_player(session); > > uint8_t volume; > > > > - if (!player) > > - return; > > - > > volume = pdu->params[1] & 0x7F; > > > > - player->cb->set_volume(volume, session->dev, player->user_data); > > + /* Always update the transport volume, which is separate from the player */ > > + media_transport_update_device_volume(session->dev, volume); > > + > > + if (player) > > + player->cb->set_volume(volume, session->dev, player->user_data); > > } > > > > static void avrcp_status_changed(struct avrcp *session, > > @@ -4378,6 +4379,9 @@ static gboolean avrcp_handle_set_volume(struct avctp *conn, uint8_t code, > > > > volume = pdu->params[0] & 0x7F; > > > > + /* Always update the transport volume, which is separate from the player */ > > + media_transport_update_device_volume(session->dev, volume); > > + > > if (player != NULL) > > player->cb->set_volume(volume, session->dev, player->user_data); > > > > diff --git a/profiles/audio/media.c b/profiles/audio/media.c > > index 993ecb3b3..a0173fdd4 100644 > > --- a/profiles/audio/media.c > > +++ b/profiles/audio/media.c > > @@ -1202,27 +1202,11 @@ static uint32_t get_duration(void *user_data) > > static void set_volume(uint8_t volume, struct btd_device *dev, void *user_data) > > { > > struct media_player *mp = user_data; > > - GSList *l; > > > > if (mp->volume == volume) > > return; > > > > mp->volume = volume; > > - > > - for (l = mp->adapter->endpoints; l; l = l->next) { > > - struct media_endpoint *endpoint = l->data; > > - struct media_transport *transport; > > - > > - /* Volume is A2DP only */ > > - if (endpoint->sep == NULL) > > - continue; > > - > > - transport = find_device_transport(endpoint, dev); > > - if (transport == NULL) > > - continue; > > - > > - media_transport_update_volume(transport, volume); > > - } > > } > > > > static bool media_player_send(struct media_player *mp, const char *name) > > -- > > 2.27.0 > > Applied, Thanks. Note that I did fix some coding style problems.
Hi Luiz, On Tue, 14 Jul 2020 at 02:06, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Marijn, > > On Mon, Jul 13, 2020 at 5:05 PM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Marijn, > > > > On Sat, Jul 11, 2020 at 4:50 AM Marijn Suijten <marijns95@gmail.com> wrote: > > > > > > `Volume` is a special property that not only exists on players but also > > > on the transport (see org.bluez.MediaTransport1). A player is not > > > attached when the controller does not support FEATURE_CATEGORY_1, which > > > is common on headphones without media browsing capabilities. > > > > > > On such audio devices (headphones, in-ears and the like) Absolute Volume > > > is not available unless an external player is registered > > > (org.bluez.Media1.RegisterPlayer) and the device sends a volume event > > > back after that to set a2dp->volume in transport.c to a valid value > > > (causing volume_exists to finally return true). > > > > > > This [1] mail thread denoting the same issue has a solution to at least > > > request capabilities from the controller, but the proposed player object > > > is not created on category 2 devices. Any notifications received on > > > AVRCP_EVENT_VOLUME_CHANGED (avrcp_volume_changed) that is subsequently > > > registered, or handling the result of avrcp_set_volume in > > > avrcp_handle_set_volume will be ignored unless said player is present. > > > > > > This issue is not addressed by adding a fake player but instead dealing > > > with the fact that volume is "special" and available on the transport > > > regardless of the existence of a player. This is confirmed in > > > avrcp_get_capabilities_resp as well which requires a player to register > > > any event except AVRCP_EVENT_VOLUME_CHANGED. > > > > > > The applied solution moves media_transport_update_device_volume out of > > > the player and into avrcp_volume_changed/avrcp_handle_set_volume where > > > it is unconditionally called. These functions are the only users of > > > avrcp_player->set_volume. > > > > > > Note that the volume member of media_player is never used which seems a > > > result of updating from org.bluez.MediaPlayer1 to > > > org.mpris.MediaPlayer2.Player in > > > 15e421737ccc4696ed567edcc24d178aedb47854, where the volume property [2] > > > is left out. > > > > This is actually on purpose since the volume notification indicates > > the volume at the device side we don't want to change the player > > volume as well as it would most likely result in duplicating the > > volume change both at sink and source, note that the likes of handling > > the transport like pulseaudio would already notify the device volume > > level change without actually applying any change to the volume > > locally. I might have wrongfully assumed that a player registers a MediaPlayer2 with BlueZ to stay in sync with the playback endpoint, including Volume. In which case the application should be aware that attenuation is applied at the sink (hence not feasible). The other way around mpris-proxy "re-exports" the transport volume over the MediaPlayer2 interface. As for pulseaudio it currently doesn't do anything with the transport volume and always applies attenuation based on a local gain. My attempt to change that [1] is what ended up in submitting this patch to make it possible. > > > [1]: https://marc.info/?l=linux-bluetooth&m=145337574806153 > > > [2]: https://specifications.freedesktop.org/mpris-spec/2.2/Player_Interface.html#Property:Volume > > > > > > --- > > > Hi, > > > > > > This is is a resend from an earlier mail that didn't comply with the > > > contributor guidelines. Seeing that the topic of AVRCP volume is brought > > > up recently it is about time to repair it and hereby send it again. > > > > > > I still have an incomplete patch lying around that synchronizes Volume > > > on org.mpris.MediaPlayer2.Player back when this patch was written 6 > > > months ago. It'll require some time to get back in to it and finalize > > > it, let me know if that's desired. > > > > > > - Marijn Suijten > > > > > > profiles/audio/avrcp.c | 12 ++++++++---- > > > profiles/audio/media.c | 16 ---------------- > > > 2 files changed, 8 insertions(+), 20 deletions(-) > > > > > > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c > > > index e2428250e..8370c8a44 100644 > > > --- a/profiles/audio/avrcp.c > > > +++ b/profiles/audio/avrcp.c > > > @@ -3625,12 +3625,13 @@ static void avrcp_volume_changed(struct avrcp *session, > > > struct avrcp_player *player = target_get_player(session); > > > uint8_t volume; > > > > > > - if (!player) > > > - return; > > > - > > > volume = pdu->params[1] & 0x7F; > > > > > > - player->cb->set_volume(volume, session->dev, player->user_data); > > > + /* Always update the transport volume, which is separate from the player */ > > > + media_transport_update_device_volume(session->dev, volume); > > > + > > > + if (player) > > > + player->cb->set_volume(volume, session->dev, player->user_data); > > > } > > > > > > static void avrcp_status_changed(struct avrcp *session, > > > @@ -4378,6 +4379,9 @@ static gboolean avrcp_handle_set_volume(struct avctp *conn, uint8_t code, > > > > > > volume = pdu->params[0] & 0x7F; > > > > > > + /* Always update the transport volume, which is separate from the player */ > > > + media_transport_update_device_volume(session->dev, volume); > > > + > > > if (player != NULL) > > > player->cb->set_volume(volume, session->dev, player->user_data); > > > > > > diff --git a/profiles/audio/media.c b/profiles/audio/media.c > > > index 993ecb3b3..a0173fdd4 100644 > > > --- a/profiles/audio/media.c > > > +++ b/profiles/audio/media.c > > > @@ -1202,27 +1202,11 @@ static uint32_t get_duration(void *user_data) > > > static void set_volume(uint8_t volume, struct btd_device *dev, void *user_data) > > > { > > > struct media_player *mp = user_data; > > > - GSList *l; > > > > > > if (mp->volume == volume) > > > return; > > > > > > mp->volume = volume; > > > - > > > - for (l = mp->adapter->endpoints; l; l = l->next) { > > > - struct media_endpoint *endpoint = l->data; > > > - struct media_transport *transport; > > > - > > > - /* Volume is A2DP only */ > > > - if (endpoint->sep == NULL) > > > - continue; > > > - > > > - transport = find_device_transport(endpoint, dev); > > > - if (transport == NULL) > > > - continue; > > > - > > > - media_transport_update_volume(transport, volume); > > > - } > > > } > > > > > > static bool media_player_send(struct media_player *mp, const char *name) > > > -- > > > 2.27.0 > > > > > Applied, Thanks. Note that I did fix some coding style problems. Thanks, I wasn't sure about submitting a v2 to merely trim the comments or remove them altogether. Note that the link to the mail thread referenced with `[1]` inadvertently disappeared too. > -- > Luiz Augusto von Dentz - Marijn [1]: https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/merge_requests/239
diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c index e2428250e..8370c8a44 100644 --- a/profiles/audio/avrcp.c +++ b/profiles/audio/avrcp.c @@ -3625,12 +3625,13 @@ static void avrcp_volume_changed(struct avrcp *session, struct avrcp_player *player = target_get_player(session); uint8_t volume; - if (!player) - return; - volume = pdu->params[1] & 0x7F; - player->cb->set_volume(volume, session->dev, player->user_data); + /* Always update the transport volume, which is separate from the player */ + media_transport_update_device_volume(session->dev, volume); + + if (player) + player->cb->set_volume(volume, session->dev, player->user_data); } static void avrcp_status_changed(struct avrcp *session, @@ -4378,6 +4379,9 @@ static gboolean avrcp_handle_set_volume(struct avctp *conn, uint8_t code, volume = pdu->params[0] & 0x7F; + /* Always update the transport volume, which is separate from the player */ + media_transport_update_device_volume(session->dev, volume); + if (player != NULL) player->cb->set_volume(volume, session->dev, player->user_data); diff --git a/profiles/audio/media.c b/profiles/audio/media.c index 993ecb3b3..a0173fdd4 100644 --- a/profiles/audio/media.c +++ b/profiles/audio/media.c @@ -1202,27 +1202,11 @@ static uint32_t get_duration(void *user_data) static void set_volume(uint8_t volume, struct btd_device *dev, void *user_data) { struct media_player *mp = user_data; - GSList *l; if (mp->volume == volume) return; mp->volume = volume; - - for (l = mp->adapter->endpoints; l; l = l->next) { - struct media_endpoint *endpoint = l->data; - struct media_transport *transport; - - /* Volume is A2DP only */ - if (endpoint->sep == NULL) - continue; - - transport = find_device_transport(endpoint, dev); - if (transport == NULL) - continue; - - media_transport_update_volume(transport, volume); - } } static bool media_player_send(struct media_player *mp, const char *name)