2013-01-11 20:25:23

by Joao Paulo Rechi Vita

[permalink] [raw]
Subject: [PATCH BlueZ 00/11] AVRCP Absolute Volume Control support

This series implements support for the Absolute Volume Control feature for the
CT role of AVRCP version 1.5.

João Paulo Rechi Vita (11):
transport: Initialize the "Volume" property with 50%
transport: Store the value set through the 'Volume' property
avrcp: Add EVENT_VOLUME_CHANGED to CT capabilities
transport: Get volume from transport
transport: Keep a list o all existent transports
transport: Get volume passing only audio_device
avrcp: Handle RegisterNotification for volume
avrcp: Notify remote of volume changes
transport: Update volume passing only audio_device
avrcp: Handle SetAbsoluteVolume command
audio: Bump AVRCP CT version to 1.5

profiles/audio/avrcp.c | 76 +++++++++++++++++++++++++++++++++++++++++-----
profiles/audio/transport.c | 55 ++++++++++++++++++++++++++++++++-
profiles/audio/transport.h | 6 ++++
3 files changed, 128 insertions(+), 9 deletions(-)

--
1.7.11.7



2013-01-15 18:12:13

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ v2 01/11] transport: Initialize the "Volume" property

Hi Joao,

On Mon, Jan 14, 2013 at 6:10 PM, Jo?o Paulo Rechi Vita
<[email protected]> wrote:
> ---
> profiles/audio/transport.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
> index a4370a5..8defc6f 100644
> --- a/profiles/audio/transport.c
> +++ b/profiles/audio/transport.c
> @@ -787,7 +787,6 @@ struct media_transport *media_transport_create(struct media_endpoint *endpoint,
> struct a2dp_transport *a2dp;
>
> a2dp = g_new0(struct a2dp_transport, 1);
> - a2dp->volume = -1;
>
> transport->resume = resume_a2dp;
> transport->suspend = suspend_a2dp;
> @@ -795,14 +794,17 @@ struct media_transport *media_transport_create(struct media_endpoint *endpoint,
> transport->data = a2dp;
> transport->destroy = destroy_a2dp;
>
> - if (strcasecmp(uuid, A2DP_SOURCE_UUID) == 0)
> + if (strcasecmp(uuid, A2DP_SOURCE_UUID) == 0) {
> + a2dp->volume = -1;
> transport->sink_watch = sink_add_state_cb(
> sink_state_changed,
> transport);
> - else
> + } else {
> + a2dp->volume = 127;
> transport->source_watch = source_add_state_cb(
> source_state_changed,
> transport);
> + }
> } else
> goto fail;
>
> --
> 1.7.11.7
>

This set is now upstream, thanks.

--
Luiz Augusto von Dentz

2013-01-15 10:43:14

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ 01/11] transport: Initialize the "Volume" property with 50%

Hi Oleksandr,

On Tue, Jan 15, 2013 at 10:02 AM, <[email protected]> wrote:
> Hi Joao,
>>
>>Hello Mikel and Luiz,
>>
>>On Mon, Jan 14, 2013 at 12:34 PM, Luiz Augusto von Dentz
>><[email protected]> wrote:
>>> Hi Mikel,
>>>
>>> On Mon, Jan 14, 2013 at 4:55 PM, Mikel Astiz <[email protected]>
>>wrote:
>>>> Hi all,
>>>>
>>>>> Hi Joao,
>>>>>
>>>>> On Mon, Jan 14, 2013 at 4:05 PM, Joao Paulo Rechi Vita
>>>>> <[email protected]> wrote:
>>>>> > On Sun, Jan 13, 2013 at 12:22 PM, Luiz Augusto von Dentz
>>>>> > <[email protected]> wrote:
>>>>> >> Hi Joao,
>>>>> >>
>>>>> >> On Fri, Jan 11, 2013 at 10:25 PM, Jo?o Paulo Rechi Vita
>>>>> >> <[email protected]> wrote:
>>>>> >>> ---
>>>>> >>> profiles/audio/transport.c | 2 +-
>>>>> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>> >>>
>>>>> >>> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
>>>>> >>> index a4370a5..6ffa98a 100644
>>>>> >>> --- a/profiles/audio/transport.c
>>>>> >>> +++ b/profiles/audio/transport.c
>>>>> >>> @@ -787,7 +787,7 @@ struct media_transport
>>>>> *media_transport_create(struct media_endpoint *endpoint,
>>>>> >>> struct a2dp_transport *a2dp;
>>>>> >>>
>>>>> >>> a2dp = g_new0(struct a2dp_transport, 1);
>>>>> >>> - a2dp->volume = -1;
>>>>> >>> + a2dp->volume = 63;
>>>>> >>>
>>>>> >>> transport->resume = resume_a2dp;
>>>>> >>> transport->suspend = suspend_a2dp;
>>>>> >>> --
>>>>> >>> 1.7.11.7
>>>>> >>
>>>>> >> Does the spec say anything regarding this? Actually it seems this
>>>>> >> value must be set by PA if it does support volume notification, which
>>>>> >> means a new version of PA, then it should set the value when the card
>>>>> >> is initialized, otherwise if the endpoint doesn't set a value it
>>>>> >> should remain -1/not available. If volume is not set by the endpoint
>>>>> >> we should either return and error upon register notification or
>>>>> >> return maximum volume always and refuse to SetAbsoluteVolume, my
>>>>> >> guess is that the latter is better for IOP reasons since the remote
>>>>> >> device may register to volume while the endpoint is setting up the
>>>>> >> transport so the volume may be set latter.
>>>>> >>
>>>>> >>
>>>>> >
>>>>> > I agree the right value will come from PA. The problem is that the
>>>>> > org.bluez.MediaTransport.Volume property doesn't exist when volume is
>>>>> > < 0 or > 127 and PA will not be able to inform the correct volume
>>>>> > value. Should we simply remove volume_exists(), or do you have any
>>>>> > other suggestion?
>>>>>
>>>>> I guess we could use the role of transport, if it is a sink/controller
>>transport it
>>>>> should be initialized to max volume otherwise it should still be
>>initialized with
>>>>> -1 so we can continue to omit in case of source/target role.
>>>>
>>>> A side question: AFAIK this feature has been added in AVRCP 1.4. How
>>would the endpoint (PulseAudio) know if it's supported or not by a certain
>>device?
>>>
>>> For the controller/sink role there is no point of PulseAudio knowing
>>> about this, it basically need to set the volume and that about it, if
>>> the remote device is interested it register to be notified otherwise
>>> we don't do anything with the value, plain and simple.
>>>
>>
>>I think PA will also want to register for notifications of changes on
>>that property, so it can reflects the current volume on the remote
>>through the master volume of the bluetooth card. The volume can be
>>changed on the remote side and it will inform bluez through the
>>SetAbsoluteVolume command.
>
> In our current AVRCP implementations, we only set the volume on the
> target to 100%. In this case the target will encode the audio
> with full quality. So the volume on the target is not coupled to the
> volume on the system.

The volume of the stream has nothing to do with the volume we are
talking about here, this is about output volume of the
sink/controller. In fact it is recommended that the stream volume be
constant at 100% to minimize precision errors of the codec.

>>
>>>> My proposal would be that the property is not present if the feature is
>>not supported (or also if AVRCP is not connected). Therefore, I'd propose
>>exposing a special value in D-Bus (i.e. -1) as soon as the feature becomes
>>available (AVRCP 1.4 connected) so the endpoint can set the initial value.
>>>
>>> That is a bad idea, the notification mechanism of AVRCP is not
>>> something I would like to see exposed over D-Bus, it is way too broken
>>> since you have to registration again every time the value changes.
>>>
>>
>>I personally thinks it makes sense to not support the property if
>>AVRCP < 1.4 (this was on my initial plans also), or if the remote
>>doesn't implement AVRCP (A2DP only). We just need to find an elegant
>>way to make this information available for the transport. Otherwise
>>AVRCP will be connected every time A2DP is also connected, right? Or
>>am I missing some corner case here?
>
> Question is do you really want to control the volume of your CT
> from the Target? I can speak from the OEM perspective and this is
> not what we want.

Not sure what you really mean here by not what we want, this is
specification and by not conforming with it you may get into IOP
problems or even not be able to qualify the stack. Note that similar
functionality exist also in HFP with SpeakerGain and MicrophoneGain,
so whatever you have against the phone controlling the output volume
is now past the point that you can say no to it.

--
Luiz Augusto von Dentz

2013-01-15 08:02:46

by Oleksandr.Domin

[permalink] [raw]
Subject: RE: [PATCH BlueZ 01/11] transport: Initialize the "Volume" property with 50%

SGkgSm9hbywNCj4NCj5IZWxsbyBNaWtlbCBhbmQgTHVpeiwNCj4NCj5PbiBNb24sIEphbiAxNCwg
MjAxMyBhdCAxMjozNCBQTSwgTHVpeiBBdWd1c3RvIHZvbiBEZW50eg0KPjxsdWl6LmRlbnR6QGdt
YWlsLmNvbT4gd3JvdGU6DQo+PiBIaSBNaWtlbCwNCj4+DQo+PiBPbiBNb24sIEphbiAxNCwgMjAx
MyBhdCA0OjU1IFBNLCBNaWtlbCBBc3RpeiA8TWlrZWwuQXN0aXpAYm13LWNhcml0LmRlPg0KPndy
b3RlOg0KPj4+IEhpIGFsbCwNCj4+Pg0KPj4+PiBIaSBKb2FvLA0KPj4+Pg0KPj4+PiBPbiBNb24s
IEphbiAxNCwgMjAxMyBhdCA0OjA1IFBNLCBKb2FvIFBhdWxvIFJlY2hpIFZpdGENCj4+Pj4gPGpw
cnZpdGFAb3BlbmJvc3NhLm9yZz4gd3JvdGU6DQo+Pj4+ID4gT24gU3VuLCBKYW4gMTMsIDIwMTMg
YXQgMTI6MjIgUE0sIEx1aXogQXVndXN0byB2b24gRGVudHoNCj4+Pj4gPiA8bHVpei5kZW50ekBn
bWFpbC5jb20+IHdyb3RlOg0KPj4+PiA+PiBIaSBKb2FvLA0KPj4+PiA+Pg0KPj4+PiA+PiBPbiBG
cmksIEphbiAxMSwgMjAxMyBhdCAxMDoyNSBQTSwgSm/Do28gUGF1bG8gUmVjaGkgVml0YQ0KPj4+
PiA+PiA8anBydml0YUBvcGVuYm9zc2Eub3JnPiB3cm90ZToNCj4+Pj4gPj4+IC0tLQ0KPj4+PiA+
Pj4gIHByb2ZpbGVzL2F1ZGlvL3RyYW5zcG9ydC5jIHwgMiArLQ0KPj4+PiA+Pj4gIDEgZmlsZSBj
aGFuZ2VkLCAxIGluc2VydGlvbigrKSwgMSBkZWxldGlvbigtKQ0KPj4+PiA+Pj4NCj4+Pj4gPj4+
IGRpZmYgLS1naXQgYS9wcm9maWxlcy9hdWRpby90cmFuc3BvcnQuYyBiL3Byb2ZpbGVzL2F1ZGlv
L3RyYW5zcG9ydC5jDQo+Pj4+ID4+PiBpbmRleCBhNDM3MGE1Li42ZmZhOThhIDEwMDY0NA0KPj4+
PiA+Pj4gLS0tIGEvcHJvZmlsZXMvYXVkaW8vdHJhbnNwb3J0LmMNCj4+Pj4gPj4+ICsrKyBiL3By
b2ZpbGVzL2F1ZGlvL3RyYW5zcG9ydC5jDQo+Pj4+ID4+PiBAQCAtNzg3LDcgKzc4Nyw3IEBAIHN0
cnVjdCBtZWRpYV90cmFuc3BvcnQNCj4+Pj4gKm1lZGlhX3RyYW5zcG9ydF9jcmVhdGUoc3RydWN0
IG1lZGlhX2VuZHBvaW50ICplbmRwb2ludCwNCj4+Pj4gPj4+ICAgICAgICAgICAgICAgICBzdHJ1
Y3QgYTJkcF90cmFuc3BvcnQgKmEyZHA7DQo+Pj4+ID4+Pg0KPj4+PiA+Pj4gICAgICAgICAgICAg
ICAgIGEyZHAgPSBnX25ldzAoc3RydWN0IGEyZHBfdHJhbnNwb3J0LCAxKTsNCj4+Pj4gPj4+IC0g
ICAgICAgICAgICAgICBhMmRwLT52b2x1bWUgPSAtMTsNCj4+Pj4gPj4+ICsgICAgICAgICAgICAg
ICBhMmRwLT52b2x1bWUgPSA2MzsNCj4+Pj4gPj4+DQo+Pj4+ID4+PiAgICAgICAgICAgICAgICAg
dHJhbnNwb3J0LT5yZXN1bWUgPSByZXN1bWVfYTJkcDsNCj4+Pj4gPj4+ICAgICAgICAgICAgICAg
ICB0cmFuc3BvcnQtPnN1c3BlbmQgPSBzdXNwZW5kX2EyZHA7DQo+Pj4+ID4+PiAtLQ0KPj4+PiA+
Pj4gMS43LjExLjcNCj4+Pj4gPj4NCj4+Pj4gPj4gRG9lcyB0aGUgc3BlYyBzYXkgYW55dGhpbmcg
cmVnYXJkaW5nIHRoaXM/IEFjdHVhbGx5IGl0IHNlZW1zIHRoaXMNCj4+Pj4gPj4gdmFsdWUgbXVz
dCBiZSBzZXQgYnkgUEEgaWYgaXQgZG9lcyBzdXBwb3J0IHZvbHVtZSBub3RpZmljYXRpb24sIHdo
aWNoDQo+Pj4+ID4+IG1lYW5zIGEgbmV3IHZlcnNpb24gb2YgUEEsIHRoZW4gaXQgc2hvdWxkIHNl
dCB0aGUgdmFsdWUgd2hlbiB0aGUgY2FyZA0KPj4+PiA+PiBpcyBpbml0aWFsaXplZCwgb3RoZXJ3
aXNlIGlmIHRoZSBlbmRwb2ludCBkb2Vzbid0IHNldCBhIHZhbHVlIGl0DQo+Pj4+ID4+IHNob3Vs
ZCByZW1haW4gLTEvbm90IGF2YWlsYWJsZS4gSWYgdm9sdW1lIGlzIG5vdCBzZXQgYnkgdGhlIGVu
ZHBvaW50DQo+Pj4+ID4+IHdlIHNob3VsZCBlaXRoZXIgcmV0dXJuIGFuZCBlcnJvciB1cG9uIHJl
Z2lzdGVyIG5vdGlmaWNhdGlvbiBvcg0KPj4+PiA+PiByZXR1cm4gbWF4aW11bSB2b2x1bWUgYWx3
YXlzIGFuZCByZWZ1c2UgdG8gU2V0QWJzb2x1dGVWb2x1bWUsIG15DQo+Pj4+ID4+IGd1ZXNzIGlz
IHRoYXQgdGhlIGxhdHRlciBpcyBiZXR0ZXIgZm9yIElPUCByZWFzb25zIHNpbmNlIHRoZSByZW1v
dGUNCj4+Pj4gPj4gZGV2aWNlIG1heSByZWdpc3RlciB0byB2b2x1bWUgd2hpbGUgdGhlIGVuZHBv
aW50IGlzIHNldHRpbmcgdXAgdGhlDQo+Pj4+ID4+IHRyYW5zcG9ydCBzbyB0aGUgdm9sdW1lIG1h
eSBiZSBzZXQgbGF0dGVyLg0KPj4+PiA+Pg0KPj4+PiA+Pg0KPj4+PiA+DQo+Pj4+ID4gSSBhZ3Jl
ZSB0aGUgcmlnaHQgdmFsdWUgd2lsbCBjb21lIGZyb20gUEEuIFRoZSBwcm9ibGVtIGlzIHRoYXQg
dGhlDQo+Pj4+ID4gb3JnLmJsdWV6Lk1lZGlhVHJhbnNwb3J0LlZvbHVtZSBwcm9wZXJ0eSBkb2Vz
bid0IGV4aXN0IHdoZW4gdm9sdW1lIGlzDQo+Pj4+ID4gPCAwIG9yID4gMTI3IGFuZCBQQSB3aWxs
IG5vdCBiZSBhYmxlIHRvIGluZm9ybSB0aGUgY29ycmVjdCB2b2x1bWUNCj4+Pj4gPiB2YWx1ZS4g
U2hvdWxkIHdlIHNpbXBseSByZW1vdmUgdm9sdW1lX2V4aXN0cygpLCBvciBkbyB5b3UgaGF2ZSBh
bnkNCj4+Pj4gPiBvdGhlciBzdWdnZXN0aW9uPw0KPj4+Pg0KPj4+PiBJIGd1ZXNzIHdlIGNvdWxk
IHVzZSB0aGUgcm9sZSBvZiB0cmFuc3BvcnQsIGlmIGl0IGlzIGEgc2luay9jb250cm9sbGVyDQo+
dHJhbnNwb3J0IGl0DQo+Pj4+IHNob3VsZCBiZSBpbml0aWFsaXplZCB0byBtYXggdm9sdW1lIG90
aGVyd2lzZSBpdCBzaG91bGQgc3RpbGwgYmUNCj5pbml0aWFsaXplZCB3aXRoDQo+Pj4+IC0xIHNv
IHdlIGNhbiBjb250aW51ZSB0byBvbWl0IGluIGNhc2Ugb2Ygc291cmNlL3RhcmdldCByb2xlLg0K
Pj4+DQo+Pj4gQSBzaWRlIHF1ZXN0aW9uOiBBRkFJSyB0aGlzIGZlYXR1cmUgaGFzIGJlZW4gYWRk
ZWQgaW4gQVZSQ1AgMS40LiBIb3cNCj53b3VsZCB0aGUgZW5kcG9pbnQgKFB1bHNlQXVkaW8pIGtu
b3cgaWYgaXQncyBzdXBwb3J0ZWQgb3Igbm90IGJ5IGEgY2VydGFpbg0KPmRldmljZT8NCj4+DQo+
PiBGb3IgdGhlIGNvbnRyb2xsZXIvc2luayByb2xlIHRoZXJlIGlzIG5vIHBvaW50IG9mIFB1bHNl
QXVkaW8ga25vd2luZw0KPj4gYWJvdXQgdGhpcywgaXQgYmFzaWNhbGx5IG5lZWQgdG8gc2V0IHRo
ZSB2b2x1bWUgYW5kIHRoYXQgYWJvdXQgaXQsIGlmDQo+PiB0aGUgcmVtb3RlIGRldmljZSBpcyBp
bnRlcmVzdGVkIGl0IHJlZ2lzdGVyIHRvIGJlIG5vdGlmaWVkIG90aGVyd2lzZQ0KPj4gd2UgZG9u
J3QgZG8gYW55dGhpbmcgd2l0aCB0aGUgdmFsdWUsIHBsYWluIGFuZCBzaW1wbGUuDQo+Pg0KPg0K
PkkgdGhpbmsgUEEgd2lsbCBhbHNvIHdhbnQgdG8gcmVnaXN0ZXIgZm9yIG5vdGlmaWNhdGlvbnMg
b2YgY2hhbmdlcyBvbg0KPnRoYXQgcHJvcGVydHksIHNvIGl0IGNhbiByZWZsZWN0cyB0aGUgY3Vy
cmVudCB2b2x1bWUgb24gdGhlIHJlbW90ZQ0KPnRocm91Z2ggdGhlIG1hc3RlciB2b2x1bWUgb2Yg
dGhlIGJsdWV0b290aCBjYXJkLiBUaGUgdm9sdW1lIGNhbiBiZQ0KPmNoYW5nZWQgb24gdGhlIHJl
bW90ZSBzaWRlIGFuZCBpdCB3aWxsIGluZm9ybSBibHVleiB0aHJvdWdoIHRoZQ0KPlNldEFic29s
dXRlVm9sdW1lIGNvbW1hbmQuDQoNCkluIG91ciBjdXJyZW50IEFWUkNQIGltcGxlbWVudGF0aW9u
cywgd2Ugb25seSBzZXQgdGhlIHZvbHVtZSBvbiB0aGUgDQp0YXJnZXQgdG8gMTAwJS4gSW4gdGhp
cyBjYXNlIHRoZSB0YXJnZXQgd2lsbCBlbmNvZGUgdGhlIGF1ZGlvDQp3aXRoIGZ1bGwgcXVhbGl0
eS4gU28gdGhlIHZvbHVtZSBvbiB0aGUgdGFyZ2V0IGlzIG5vdCBjb3VwbGVkIHRvIHRoZQ0Kdm9s
dW1lIG9uIHRoZSBzeXN0ZW0uDQoNCj4NCj4+PiBNeSBwcm9wb3NhbCB3b3VsZCBiZSB0aGF0IHRo
ZSBwcm9wZXJ0eSBpcyBub3QgcHJlc2VudCBpZiB0aGUgZmVhdHVyZSBpcw0KPm5vdCBzdXBwb3J0
ZWQgKG9yIGFsc28gaWYgQVZSQ1AgaXMgbm90IGNvbm5lY3RlZCkuIFRoZXJlZm9yZSwgSSdkIHBy
b3Bvc2UNCj5leHBvc2luZyBhIHNwZWNpYWwgdmFsdWUgaW4gRC1CdXMgKGkuZS4gLTEpIGFzIHNv
b24gYXMgdGhlIGZlYXR1cmUgYmVjb21lcw0KPmF2YWlsYWJsZSAoQVZSQ1AgMS40IGNvbm5lY3Rl
ZCkgc28gdGhlIGVuZHBvaW50IGNhbiBzZXQgdGhlIGluaXRpYWwgdmFsdWUuDQo+Pg0KPj4gVGhh
dCBpcyBhIGJhZCBpZGVhLCB0aGUgbm90aWZpY2F0aW9uIG1lY2hhbmlzbSBvZiBBVlJDUCBpcyBu
b3QNCj4+IHNvbWV0aGluZyBJIHdvdWxkIGxpa2UgdG8gc2VlIGV4cG9zZWQgb3ZlciBELUJ1cywg
aXQgaXMgd2F5IHRvbyBicm9rZW4NCj4+IHNpbmNlIHlvdSBoYXZlIHRvIHJlZ2lzdHJhdGlvbiBh
Z2FpbiBldmVyeSB0aW1lIHRoZSB2YWx1ZSBjaGFuZ2VzLg0KPj4NCj4NCj5JIHBlcnNvbmFsbHkg
dGhpbmtzIGl0IG1ha2VzIHNlbnNlIHRvIG5vdCBzdXBwb3J0IHRoZSBwcm9wZXJ0eSBpZg0KPkFW
UkNQIDwgMS40ICh0aGlzIHdhcyBvbiBteSBpbml0aWFsIHBsYW5zIGFsc28pLCBvciBpZiB0aGUg
cmVtb3RlDQo+ZG9lc24ndCBpbXBsZW1lbnQgQVZSQ1AgKEEyRFAgb25seSkuIFdlIGp1c3QgbmVl
ZCB0byBmaW5kIGFuIGVsZWdhbnQNCj53YXkgdG8gbWFrZSB0aGlzIGluZm9ybWF0aW9uIGF2YWls
YWJsZSBmb3IgdGhlIHRyYW5zcG9ydC4gT3RoZXJ3aXNlDQo+QVZSQ1Agd2lsbCBiZSBjb25uZWN0
ZWQgZXZlcnkgdGltZSBBMkRQIGlzIGFsc28gY29ubmVjdGVkLCByaWdodD8gT3INCj5hbSBJIG1p
c3Npbmcgc29tZSBjb3JuZXIgY2FzZSBoZXJlPw0KDQpRdWVzdGlvbiBpcyBkbyB5b3UgcmVhbGx5
IHdhbnQgdG8gY29udHJvbCB0aGUgdm9sdW1lIG9mIHlvdXIgQ1QNCmZyb20gdGhlIFRhcmdldD8g
SSBjYW4gc3BlYWsgZnJvbSB0aGUgT0VNIHBlcnNwZWN0aXZlIGFuZCB0aGlzIGlzDQpub3Qgd2hh
dCB3ZSB3YW50Lg0KDQpSZWdhcmRzLA0KT2xla3NhbmRyDQoNCi0tLQ0KQk1XIEdyb3VwDQpPbGVr
c2FuZHLCoERvbWluDQpBcHBDZW50ZXIsIEVudGVydGFpbm1lbnQgYW5kIE1vYmlsZSBFbmRnZXLD
pHRlDQpTb2Z0d2FyZSBBcmNoaXRlY3QgR0VOSVZJDQpNYXgtRGlhbWFuZC1TdHIuIDI1DQo4MDkz
N8KgTcO8bmNoZW4NCg0KVGVsOsKgKzQ5IDg5IDM4MiAtIDYwNjMyDQpNb2JpbGU6wqArNDkgMTc2
IDYwMTYwNjMyDQpNYWlsOsKgb2xla3NhbmRyLmRvbWluQGJtdy5kZQ0KV2ViOsKgaHR0cDovL3d3
dy5ibXdncm91cC5jb20NCi0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tDQpCYXllcmlzY2hlIE1vdG9y
ZW4gV2Vya2UgQWt0aWVuZ2VzZWxsc2NoYWZ0DQpCb2FyZCBvZiBNYW5hZ2VtZW50OiBOb3JiZXJ0
IFJlaXRob2ZlciwgQ2hhaXJtYW4sDQpGcmFuay1QZXRlciBBcm5kdCwgTWlsYWdyb3MgQ2Fpw7Fh
IENhcnJlaXJvLUFuZHJlZSwNCkhlcmJlcnQgRGllc3MsIEtsYXVzIERyYWVnZXIsIEZyaWVkcmlj
aCBFaWNoaW5lciwNCkhhcmFsZCBLcsO8Z2VyLCBJYW4gUm9iZXJ0c29uLg0KQ2hhaXJtYW4gb2Yg
U3VwZXJ2aXNvcnkgQm9hcmQ6IEpvYWNoaW0gTWlsYmVyZw0KUmVnaXN0ZXJlZCBpbiBHZXJtYW55
OiBNw7xuY2hlbiBIUkIgNDIyNDMNCi0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tDQoNCg0KDQo=

2013-01-14 16:10:29

by Joao Paulo Rechi Vita

[permalink] [raw]
Subject: [PATCH BlueZ v2 01/11] transport: Initialize the "Volume" property

---
profiles/audio/transport.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
index a4370a5..8defc6f 100644
--- a/profiles/audio/transport.c
+++ b/profiles/audio/transport.c
@@ -787,7 +787,6 @@ struct media_transport *media_transport_create(struct media_endpoint *endpoint,
struct a2dp_transport *a2dp;

a2dp = g_new0(struct a2dp_transport, 1);
- a2dp->volume = -1;

transport->resume = resume_a2dp;
transport->suspend = suspend_a2dp;
@@ -795,14 +794,17 @@ struct media_transport *media_transport_create(struct media_endpoint *endpoint,
transport->data = a2dp;
transport->destroy = destroy_a2dp;

- if (strcasecmp(uuid, A2DP_SOURCE_UUID) == 0)
+ if (strcasecmp(uuid, A2DP_SOURCE_UUID) == 0) {
+ a2dp->volume = -1;
transport->sink_watch = sink_add_state_cb(
sink_state_changed,
transport);
- else
+ } else {
+ a2dp->volume = 127;
transport->source_watch = source_add_state_cb(
source_state_changed,
transport);
+ }
} else
goto fail;

--
1.7.11.7


2013-01-14 15:43:40

by Joao Paulo Rechi Vita

[permalink] [raw]
Subject: Re: [PATCH BlueZ 01/11] transport: Initialize the "Volume" property with 50%

Hello Mikel and Luiz,

On Mon, Jan 14, 2013 at 12:34 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Mikel,
>
> On Mon, Jan 14, 2013 at 4:55 PM, Mikel Astiz <[email protected]> wrote:
>> Hi all,
>>
>>> Hi Joao,
>>>
>>> On Mon, Jan 14, 2013 at 4:05 PM, Joao Paulo Rechi Vita
>>> <[email protected]> wrote:
>>> > On Sun, Jan 13, 2013 at 12:22 PM, Luiz Augusto von Dentz
>>> > <[email protected]> wrote:
>>> >> Hi Joao,
>>> >>
>>> >> On Fri, Jan 11, 2013 at 10:25 PM, João Paulo Rechi Vita
>>> >> <[email protected]> wrote:
>>> >>> ---
>>> >>> profiles/audio/transport.c | 2 +-
>>> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> >>>
>>> >>> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
>>> >>> index a4370a5..6ffa98a 100644
>>> >>> --- a/profiles/audio/transport.c
>>> >>> +++ b/profiles/audio/transport.c
>>> >>> @@ -787,7 +787,7 @@ struct media_transport
>>> *media_transport_create(struct media_endpoint *endpoint,
>>> >>> struct a2dp_transport *a2dp;
>>> >>>
>>> >>> a2dp = g_new0(struct a2dp_transport, 1);
>>> >>> - a2dp->volume = -1;
>>> >>> + a2dp->volume = 63;
>>> >>>
>>> >>> transport->resume = resume_a2dp;
>>> >>> transport->suspend = suspend_a2dp;
>>> >>> --
>>> >>> 1.7.11.7
>>> >>
>>> >> Does the spec say anything regarding this? Actually it seems this
>>> >> value must be set by PA if it does support volume notification, which
>>> >> means a new version of PA, then it should set the value when the card
>>> >> is initialized, otherwise if the endpoint doesn't set a value it
>>> >> should remain -1/not available. If volume is not set by the endpoint
>>> >> we should either return and error upon register notification or
>>> >> return maximum volume always and refuse to SetAbsoluteVolume, my
>>> >> guess is that the latter is better for IOP reasons since the remote
>>> >> device may register to volume while the endpoint is setting up the
>>> >> transport so the volume may be set latter.
>>> >>
>>> >>
>>> >
>>> > I agree the right value will come from PA. The problem is that the
>>> > org.bluez.MediaTransport.Volume property doesn't exist when volume is
>>> > < 0 or > 127 and PA will not be able to inform the correct volume
>>> > value. Should we simply remove volume_exists(), or do you have any
>>> > other suggestion?
>>>
>>> I guess we could use the role of transport, if it is a sink/controller transport it
>>> should be initialized to max volume otherwise it should still be initialized with
>>> -1 so we can continue to omit in case of source/target role.
>>
>> A side question: AFAIK this feature has been added in AVRCP 1.4. How would the endpoint (PulseAudio) know if it's supported or not by a certain device?
>
> For the controller/sink role there is no point of PulseAudio knowing
> about this, it basically need to set the volume and that about it, if
> the remote device is interested it register to be notified otherwise
> we don't do anything with the value, plain and simple.
>

I think PA will also want to register for notifications of changes on
that property, so it can reflects the current volume on the remote
through the master volume of the bluetooth card. The volume can be
changed on the remote side and it will inform bluez through the
SetAbsoluteVolume command.

>> My proposal would be that the property is not present if the feature is not supported (or also if AVRCP is not connected). Therefore, I'd propose exposing a special value in D-Bus (i.e. -1) as soon as the feature becomes available (AVRCP 1.4 connected) so the endpoint can set the initial value.
>
> That is a bad idea, the notification mechanism of AVRCP is not
> something I would like to see exposed over D-Bus, it is way too broken
> since you have to registration again every time the value changes.
>

I personally thinks it makes sense to not support the property if
AVRCP < 1.4 (this was on my initial plans also), or if the remote
doesn't implement AVRCP (A2DP only). We just need to find an elegant
way to make this information available for the transport. Otherwise
AVRCP will be connected every time A2DP is also connected, right? Or
am I missing some corner case here?


--
João Paulo Rechi Vita
Openbossa Labs - INdT

2013-01-14 15:37:00

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ 01/11] transport: Initialize the "Volume" property with 50%

Hi Joao,

On Mon, Jan 14, 2013 at 5:32 PM, Joao Paulo Rechi Vita
<[email protected]> wrote:
> On Mon, Jan 14, 2013 at 11:21 AM, Von Dentz, Luiz
> <[email protected]> wrote:
>> Hi Joao,
>>
>> On Mon, Jan 14, 2013 at 4:05 PM, Joao Paulo Rechi Vita
>> <[email protected]> wrote:
>>> On Sun, Jan 13, 2013 at 12:22 PM, Luiz Augusto von Dentz
>>> <[email protected]> wrote:
>>>> Hi Joao,
>>>>
>>>> On Fri, Jan 11, 2013 at 10:25 PM, Jo?o Paulo Rechi Vita
>>>> <[email protected]> wrote:
>>>>> ---
>>>>> profiles/audio/transport.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
>>>>> index a4370a5..6ffa98a 100644
>>>>> --- a/profiles/audio/transport.c
>>>>> +++ b/profiles/audio/transport.c
>>>>> @@ -787,7 +787,7 @@ struct media_transport *media_transport_create(struct media_endpoint *endpoint,
>>>>> struct a2dp_transport *a2dp;
>>>>>
>>>>> a2dp = g_new0(struct a2dp_transport, 1);
>>>>> - a2dp->volume = -1;
>>>>> + a2dp->volume = 63;
>>>>>
>>>>> transport->resume = resume_a2dp;
>>>>> transport->suspend = suspend_a2dp;
>>>>> --
>>>>> 1.7.11.7
>>>>
>>>> Does the spec say anything regarding this? Actually it seems this
>>>> value must be set by PA if it does support volume notification, which
>>>> means a new version of PA, then it should set the value when the card
>>>> is initialized, otherwise if the endpoint doesn't set a value it
>>>> should remain -1/not available. If volume is not set by the endpoint
>>>> we should either return and error upon register notification or return
>>>> maximum volume always and refuse to SetAbsoluteVolume, my guess is
>>>> that the latter is better for IOP reasons since the remote device may
>>>> register to volume while the endpoint is setting up the transport so
>>>> the volume may be set latter.
>>>>
>>>>
>>>
>>> I agree the right value will come from PA. The problem is that the
>>> org.bluez.MediaTransport.Volume property doesn't exist when volume is
>>> < 0 or > 127 and PA will not be able to inform the correct volume
>>> value. Should we simply remove volume_exists(), or do you have any
>>> other suggestion?
>>
>> I guess we could use the role of transport, if it is a sink/controller
>> transport it should be initialized to max volume otherwise it should
>> still be initialized with -1 so we can continue to omit in case of
>> source/target role.
>
> I agree we omit it by now, but when we add support for AVC on the
> Source/TG role we can use it to control the volume on the remote,
> sending a SetAbsoluteVolume command. I'll send an updated version in a
> few moments.

For Source/TG it is different because as soon as we are able to
register the volume notification we can update the volume and then it
became available, in fact it should be already working like this.


--
Luiz Augusto von Dentz

2013-01-14 15:34:39

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ 01/11] transport: Initialize the "Volume" property with 50%

Hi Mikel,

On Mon, Jan 14, 2013 at 4:55 PM, Mikel Astiz <[email protected]> wrote:
> Hi all,
>
>> Hi Joao,
>>
>> On Mon, Jan 14, 2013 at 4:05 PM, Joao Paulo Rechi Vita
>> <[email protected]> wrote:
>> > On Sun, Jan 13, 2013 at 12:22 PM, Luiz Augusto von Dentz
>> > <[email protected]> wrote:
>> >> Hi Joao,
>> >>
>> >> On Fri, Jan 11, 2013 at 10:25 PM, Jo?o Paulo Rechi Vita
>> >> <[email protected]> wrote:
>> >>> ---
>> >>> profiles/audio/transport.c | 2 +-
>> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> >>>
>> >>> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
>> >>> index a4370a5..6ffa98a 100644
>> >>> --- a/profiles/audio/transport.c
>> >>> +++ b/profiles/audio/transport.c
>> >>> @@ -787,7 +787,7 @@ struct media_transport
>> *media_transport_create(struct media_endpoint *endpoint,
>> >>> struct a2dp_transport *a2dp;
>> >>>
>> >>> a2dp = g_new0(struct a2dp_transport, 1);
>> >>> - a2dp->volume = -1;
>> >>> + a2dp->volume = 63;
>> >>>
>> >>> transport->resume = resume_a2dp;
>> >>> transport->suspend = suspend_a2dp;
>> >>> --
>> >>> 1.7.11.7
>> >>
>> >> Does the spec say anything regarding this? Actually it seems this
>> >> value must be set by PA if it does support volume notification, which
>> >> means a new version of PA, then it should set the value when the card
>> >> is initialized, otherwise if the endpoint doesn't set a value it
>> >> should remain -1/not available. If volume is not set by the endpoint
>> >> we should either return and error upon register notification or
>> >> return maximum volume always and refuse to SetAbsoluteVolume, my
>> >> guess is that the latter is better for IOP reasons since the remote
>> >> device may register to volume while the endpoint is setting up the
>> >> transport so the volume may be set latter.
>> >>
>> >>
>> >
>> > I agree the right value will come from PA. The problem is that the
>> > org.bluez.MediaTransport.Volume property doesn't exist when volume is
>> > < 0 or > 127 and PA will not be able to inform the correct volume
>> > value. Should we simply remove volume_exists(), or do you have any
>> > other suggestion?
>>
>> I guess we could use the role of transport, if it is a sink/controller transport it
>> should be initialized to max volume otherwise it should still be initialized with
>> -1 so we can continue to omit in case of source/target role.
>
> A side question: AFAIK this feature has been added in AVRCP 1.4. How would the endpoint (PulseAudio) know if it's supported or not by a certain device?

For the controller/sink role there is no point of PulseAudio knowing
about this, it basically need to set the volume and that about it, if
the remote device is interested it register to be notified otherwise
we don't do anything with the value, plain and simple.

> My proposal would be that the property is not present if the feature is not supported (or also if AVRCP is not connected). Therefore, I'd propose exposing a special value in D-Bus (i.e. -1) as soon as the feature becomes available (AVRCP 1.4 connected) so the endpoint can set the initial value.

That is a bad idea, the notification mechanism of AVRCP is not
something I would like to see exposed over D-Bus, it is way too broken
since you have to registration again every time the value changes.

Btw, everytime someone else jump in this conversation please read at
least the Absolute Volume Control part of the spec, quite often people
think the volume notification are from the source to the sink when in
fact it is the opposite.


--
Luiz Augusto von Dentz

2013-01-14 15:32:58

by Joao Paulo Rechi Vita

[permalink] [raw]
Subject: Re: [PATCH BlueZ 01/11] transport: Initialize the "Volume" property with 50%

On Mon, Jan 14, 2013 at 11:21 AM, Von Dentz, Luiz
<[email protected]> wrote:
> Hi Joao,
>
> On Mon, Jan 14, 2013 at 4:05 PM, Joao Paulo Rechi Vita
> <[email protected]> wrote:
>> On Sun, Jan 13, 2013 at 12:22 PM, Luiz Augusto von Dentz
>> <[email protected]> wrote:
>>> Hi Joao,
>>>
>>> On Fri, Jan 11, 2013 at 10:25 PM, João Paulo Rechi Vita
>>> <[email protected]> wrote:
>>>> ---
>>>> profiles/audio/transport.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
>>>> index a4370a5..6ffa98a 100644
>>>> --- a/profiles/audio/transport.c
>>>> +++ b/profiles/audio/transport.c
>>>> @@ -787,7 +787,7 @@ struct media_transport *media_transport_create(struct media_endpoint *endpoint,
>>>> struct a2dp_transport *a2dp;
>>>>
>>>> a2dp = g_new0(struct a2dp_transport, 1);
>>>> - a2dp->volume = -1;
>>>> + a2dp->volume = 63;
>>>>
>>>> transport->resume = resume_a2dp;
>>>> transport->suspend = suspend_a2dp;
>>>> --
>>>> 1.7.11.7
>>>
>>> Does the spec say anything regarding this? Actually it seems this
>>> value must be set by PA if it does support volume notification, which
>>> means a new version of PA, then it should set the value when the card
>>> is initialized, otherwise if the endpoint doesn't set a value it
>>> should remain -1/not available. If volume is not set by the endpoint
>>> we should either return and error upon register notification or return
>>> maximum volume always and refuse to SetAbsoluteVolume, my guess is
>>> that the latter is better for IOP reasons since the remote device may
>>> register to volume while the endpoint is setting up the transport so
>>> the volume may be set latter.
>>>
>>>
>>
>> I agree the right value will come from PA. The problem is that the
>> org.bluez.MediaTransport.Volume property doesn't exist when volume is
>> < 0 or > 127 and PA will not be able to inform the correct volume
>> value. Should we simply remove volume_exists(), or do you have any
>> other suggestion?
>
> I guess we could use the role of transport, if it is a sink/controller
> transport it should be initialized to max volume otherwise it should
> still be initialized with -1 so we can continue to omit in case of
> source/target role.

I agree we omit it by now, but when we add support for AVC on the
Source/TG role we can use it to control the volume on the remote,
sending a SetAbsoluteVolume command. I'll send an updated version in a
few moments.

--
João Paulo Rechi Vita
Openbossa Labs - INdT

2013-01-14 14:55:19

by Mikel Astiz

[permalink] [raw]
Subject: AW: [PATCH BlueZ 01/11] transport: Initialize the "Volume" property with 50%

Hi all,

> Hi Joao,
>
> On Mon, Jan 14, 2013 at 4:05 PM, Joao Paulo Rechi Vita
> <[email protected]> wrote:
> > On Sun, Jan 13, 2013 at 12:22 PM, Luiz Augusto von Dentz
> > <[email protected]> wrote:
> >> Hi Joao,
> >>
> >> On Fri, Jan 11, 2013 at 10:25 PM, Jo?o Paulo Rechi Vita
> >> <[email protected]> wrote:
> >>> ---
> >>> profiles/audio/transport.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
> >>> index a4370a5..6ffa98a 100644
> >>> --- a/profiles/audio/transport.c
> >>> +++ b/profiles/audio/transport.c
> >>> @@ -787,7 +787,7 @@ struct media_transport
> *media_transport_create(struct media_endpoint *endpoint,
> >>> struct a2dp_transport *a2dp;
> >>>
> >>> a2dp = g_new0(struct a2dp_transport, 1);
> >>> - a2dp->volume = -1;
> >>> + a2dp->volume = 63;
> >>>
> >>> transport->resume = resume_a2dp;
> >>> transport->suspend = suspend_a2dp;
> >>> --
> >>> 1.7.11.7
> >>
> >> Does the spec say anything regarding this? Actually it seems this
> >> value must be set by PA if it does support volume notification, which
> >> means a new version of PA, then it should set the value when the card
> >> is initialized, otherwise if the endpoint doesn't set a value it
> >> should remain -1/not available. If volume is not set by the endpoint
> >> we should either return and error upon register notification or
> >> return maximum volume always and refuse to SetAbsoluteVolume, my
> >> guess is that the latter is better for IOP reasons since the remote
> >> device may register to volume while the endpoint is setting up the
> >> transport so the volume may be set latter.
> >>
> >>
> >
> > I agree the right value will come from PA. The problem is that the
> > org.bluez.MediaTransport.Volume property doesn't exist when volume is
> > < 0 or > 127 and PA will not be able to inform the correct volume
> > value. Should we simply remove volume_exists(), or do you have any
> > other suggestion?
>
> I guess we could use the role of transport, if it is a sink/controller transport it
> should be initialized to max volume otherwise it should still be initialized with
> -1 so we can continue to omit in case of source/target role.

A side question: AFAIK this feature has been added in AVRCP 1.4. How would the endpoint (PulseAudio) know if it's supported or not by a certain device?

My proposal would be that the property is not present if the feature is not supported (or also if AVRCP is not connected). Therefore, I'd propose exposing a special value in D-Bus (i.e. -1) as soon as the feature becomes available (AVRCP 1.4 connected) so the endpoint can set the initial value.

Cheers,
Mikel


2013-01-14 14:21:58

by Von Dentz, Luiz

[permalink] [raw]
Subject: Re: [PATCH BlueZ 01/11] transport: Initialize the "Volume" property with 50%

Hi Joao,

On Mon, Jan 14, 2013 at 4:05 PM, Joao Paulo Rechi Vita
<[email protected]> wrote:
> On Sun, Jan 13, 2013 at 12:22 PM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> Hi Joao,
>>
>> On Fri, Jan 11, 2013 at 10:25 PM, Jo?o Paulo Rechi Vita
>> <[email protected]> wrote:
>>> ---
>>> profiles/audio/transport.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
>>> index a4370a5..6ffa98a 100644
>>> --- a/profiles/audio/transport.c
>>> +++ b/profiles/audio/transport.c
>>> @@ -787,7 +787,7 @@ struct media_transport *media_transport_create(struct media_endpoint *endpoint,
>>> struct a2dp_transport *a2dp;
>>>
>>> a2dp = g_new0(struct a2dp_transport, 1);
>>> - a2dp->volume = -1;
>>> + a2dp->volume = 63;
>>>
>>> transport->resume = resume_a2dp;
>>> transport->suspend = suspend_a2dp;
>>> --
>>> 1.7.11.7
>>
>> Does the spec say anything regarding this? Actually it seems this
>> value must be set by PA if it does support volume notification, which
>> means a new version of PA, then it should set the value when the card
>> is initialized, otherwise if the endpoint doesn't set a value it
>> should remain -1/not available. If volume is not set by the endpoint
>> we should either return and error upon register notification or return
>> maximum volume always and refuse to SetAbsoluteVolume, my guess is
>> that the latter is better for IOP reasons since the remote device may
>> register to volume while the endpoint is setting up the transport so
>> the volume may be set latter.
>>
>>
>
> I agree the right value will come from PA. The problem is that the
> org.bluez.MediaTransport.Volume property doesn't exist when volume is
> < 0 or > 127 and PA will not be able to inform the correct volume
> value. Should we simply remove volume_exists(), or do you have any
> other suggestion?

I guess we could use the role of transport, if it is a sink/controller
transport it should be initialized to max volume otherwise it should
still be initialized with -1 so we can continue to omit in case of
source/target role.

2013-01-14 14:05:51

by Joao Paulo Rechi Vita

[permalink] [raw]
Subject: Re: [PATCH BlueZ 01/11] transport: Initialize the "Volume" property with 50%

On Sun, Jan 13, 2013 at 12:22 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Joao,
>
> On Fri, Jan 11, 2013 at 10:25 PM, João Paulo Rechi Vita
> <[email protected]> wrote:
>> ---
>> profiles/audio/transport.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
>> index a4370a5..6ffa98a 100644
>> --- a/profiles/audio/transport.c
>> +++ b/profiles/audio/transport.c
>> @@ -787,7 +787,7 @@ struct media_transport *media_transport_create(struct media_endpoint *endpoint,
>> struct a2dp_transport *a2dp;
>>
>> a2dp = g_new0(struct a2dp_transport, 1);
>> - a2dp->volume = -1;
>> + a2dp->volume = 63;
>>
>> transport->resume = resume_a2dp;
>> transport->suspend = suspend_a2dp;
>> --
>> 1.7.11.7
>
> Does the spec say anything regarding this? Actually it seems this
> value must be set by PA if it does support volume notification, which
> means a new version of PA, then it should set the value when the card
> is initialized, otherwise if the endpoint doesn't set a value it
> should remain -1/not available. If volume is not set by the endpoint
> we should either return and error upon register notification or return
> maximum volume always and refuse to SetAbsoluteVolume, my guess is
> that the latter is better for IOP reasons since the remote device may
> register to volume while the endpoint is setting up the transport so
> the volume may be set latter.
>
>

I agree the right value will come from PA. The problem is that the
org.bluez.MediaTransport.Volume property doesn't exist when volume is
< 0 or > 127 and PA will not be able to inform the correct volume
value. Should we simply remove volume_exists(), or do you have any
other suggestion?

--
João Paulo Rechi Vita
Openbossa Labs - INdT

2013-01-13 15:22:31

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH BlueZ 01/11] transport: Initialize the "Volume" property with 50%

Hi Joao,

On Fri, Jan 11, 2013 at 10:25 PM, Jo?o Paulo Rechi Vita
<[email protected]> wrote:
> ---
> profiles/audio/transport.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
> index a4370a5..6ffa98a 100644
> --- a/profiles/audio/transport.c
> +++ b/profiles/audio/transport.c
> @@ -787,7 +787,7 @@ struct media_transport *media_transport_create(struct media_endpoint *endpoint,
> struct a2dp_transport *a2dp;
>
> a2dp = g_new0(struct a2dp_transport, 1);
> - a2dp->volume = -1;
> + a2dp->volume = 63;
>
> transport->resume = resume_a2dp;
> transport->suspend = suspend_a2dp;
> --
> 1.7.11.7

Does the spec say anything regarding this? Actually it seems this
value must be set by PA if it does support volume notification, which
means a new version of PA, then it should set the value when the card
is initialized, otherwise if the endpoint doesn't set a value it
should remain -1/not available. If volume is not set by the endpoint
we should either return and error upon register notification or return
maximum volume always and refuse to SetAbsoluteVolume, my guess is
that the latter is better for IOP reasons since the remote device may
register to volume while the endpoint is setting up the transport so
the volume may be set latter.


--
Luiz Augusto von Dentz

2013-01-11 20:25:34

by Joao Paulo Rechi Vita

[permalink] [raw]
Subject: [PATCH BlueZ 11/11] audio: Bump AVRCP CT version to 1.5

---
profiles/audio/avrcp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 8bd5fb6..19b6721 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -231,7 +231,7 @@ static sdp_record_t *avrcp_ct_record(void)
sdp_record_t *record;
sdp_data_t *psm, *version, *features;
uint16_t lp = AVCTP_CONTROL_PSM;
- uint16_t avrcp_ver = 0x0103, avctp_ver = 0x0103;
+ uint16_t avrcp_ver = 0x0105, avctp_ver = 0x0103;
uint16_t feat = ( AVRCP_FEATURE_CATEGORY_1 |
AVRCP_FEATURE_CATEGORY_2 |
AVRCP_FEATURE_CATEGORY_3 |
--
1.7.11.7


2013-01-11 20:25:33

by Joao Paulo Rechi Vita

[permalink] [raw]
Subject: [PATCH BlueZ 10/11] avrcp: Handle SetAbsoluteVolume command

---
profiles/audio/avrcp.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index d715177..8bd5fb6 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -1392,6 +1392,32 @@ err:
return AVC_CTYPE_REJECTED;
}

+static uint8_t avrcp_handle_set_absolute_volume(struct avrcp *session,
+ struct avrcp_header *pdu,
+ uint8_t transaction)
+{
+ struct avrcp_player *player = session->player;
+ uint16_t len = ntohs(pdu->params_len);
+
+ if (len != 1)
+ goto err;
+
+ if (pdu->params[0] > 127)
+ goto err;
+
+ if (!player)
+ goto err;
+
+ media_transport_update_device_volume(session->dev, pdu->params[0]);
+
+ return AVC_CTYPE_ACCEPTED;
+
+err:
+ pdu->params_len = htons(1);
+ pdu->params[0] = AVRCP_STATUS_INVALID_PARAM;
+ return AVC_CTYPE_REJECTED;
+}
+
static const struct control_pdu_handler tg_control_handlers[] = {
{ AVRCP_GET_CAPABILITIES, AVC_CTYPE_STATUS,
avrcp_handle_get_capabilities },
@@ -1429,6 +1455,8 @@ static const struct control_pdu_handler ct_control_handlers[] = {
avrcp_handle_get_capabilities },
{ AVRCP_REGISTER_NOTIFICATION, AVC_CTYPE_NOTIFY,
avrcp_handle_register_notification },
+ { AVRCP_SET_ABSOLUTE_VOLUME, AVC_CTYPE_CONTROL,
+ avrcp_handle_set_absolute_volume },
{ },
};

--
1.7.11.7


2013-01-11 20:25:27

by Joao Paulo Rechi Vita

[permalink] [raw]
Subject: [PATCH BlueZ 04/11] transport: Get volume from transport

---
profiles/audio/transport.c | 6 ++++++
profiles/audio/transport.h | 1 +
2 files changed, 7 insertions(+)

diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
index f3caa1b..4a81d85 100644
--- a/profiles/audio/transport.c
+++ b/profiles/audio/transport.c
@@ -849,6 +849,12 @@ struct audio_device *media_transport_get_dev(struct media_transport *transport)
return transport->device;
}

+uint16_t media_transport_get_volume(struct media_transport *transport)
+{
+ struct a2dp_transport *a2dp = transport->data;
+ return a2dp->volume;
+}
+
void media_transport_update_volume(struct media_transport *transport,
uint8_t volume)
{
diff --git a/profiles/audio/transport.h b/profiles/audio/transport.h
index a6b71e5..0fe8973 100644
--- a/profiles/audio/transport.h
+++ b/profiles/audio/transport.h
@@ -32,6 +32,7 @@ struct media_transport *media_transport_create(struct media_endpoint *endpoint,
void media_transport_destroy(struct media_transport *transport);
const char *media_transport_get_path(struct media_transport *transport);
struct audio_device *media_transport_get_dev(struct media_transport *transport);
+uint16_t media_transport_get_volume(struct media_transport *transport);
void media_transport_update_delay(struct media_transport *transport,
uint16_t delay);
void media_transport_update_volume(struct media_transport *transport,
--
1.7.11.7


2013-01-11 20:25:29

by Joao Paulo Rechi Vita

[permalink] [raw]
Subject: [PATCH BlueZ 06/11] transport: Get volume passing only audio_device

---
profiles/audio/transport.c | 20 ++++++++++++++++++++
profiles/audio/transport.h | 2 ++
2 files changed, 22 insertions(+)

diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
index c3e26f4..9419af0 100644
--- a/profiles/audio/transport.c
+++ b/profiles/audio/transport.c
@@ -876,3 +876,23 @@ void media_transport_update_volume(struct media_transport *transport,
transport->path,
MEDIA_TRANSPORT_INTERFACE, "Volume");
}
+
+uint8_t media_transport_get_device_volume(struct audio_device *dev)
+{
+ GSList *l;
+
+ if (dev == NULL)
+ return 128;
+
+ for (l = transports; l; l = l->next) {
+ struct media_transport *transport = l->data;
+ if (transport->device != dev)
+ continue;
+
+ /* Volume is A2DP only */
+ if (media_endpoint_get_sep(transport->endpoint))
+ return media_transport_get_volume(transport);
+ }
+
+ return 128;
+}
diff --git a/profiles/audio/transport.h b/profiles/audio/transport.h
index 0fe8973..78c6fa7 100644
--- a/profiles/audio/transport.h
+++ b/profiles/audio/transport.h
@@ -39,3 +39,5 @@ void media_transport_update_volume(struct media_transport *transport,
uint8_t volume);
void transport_get_properties(struct media_transport *transport,
DBusMessageIter *iter);
+
+uint8_t media_transport_get_device_volume(struct audio_device *dev);
--
1.7.11.7


2013-01-11 20:25:32

by Joao Paulo Rechi Vita

[permalink] [raw]
Subject: [PATCH BlueZ 09/11] transport: Update volume passing only audio_device

---
profiles/audio/transport.c | 19 +++++++++++++++++++
profiles/audio/transport.h | 2 ++
2 files changed, 21 insertions(+)

diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
index 9419af0..c344a92 100644
--- a/profiles/audio/transport.c
+++ b/profiles/audio/transport.c
@@ -896,3 +896,22 @@ uint8_t media_transport_get_device_volume(struct audio_device *dev)

return 128;
}
+
+void media_transport_update_device_volume(struct audio_device *dev,
+ uint8_t volume)
+{
+ GSList *l;
+
+ if (dev == NULL)
+ return;
+
+ for (l = transports; l; l = l->next) {
+ struct media_transport *transport = l->data;
+ if (transport->device != dev)
+ continue;
+
+ /* Volume is A2DP only */
+ if (media_endpoint_get_sep(transport->endpoint))
+ media_transport_update_volume(transport, volume);
+ }
+}
diff --git a/profiles/audio/transport.h b/profiles/audio/transport.h
index cbbd0b6..b8cbf1f 100644
--- a/profiles/audio/transport.h
+++ b/profiles/audio/transport.h
@@ -42,3 +42,5 @@ void transport_get_properties(struct media_transport *transport,
DBusMessageIter *iter);

uint8_t media_transport_get_device_volume(struct audio_device *dev);
+void media_transport_update_device_volume(struct audio_device *dev,
+ uint8_t volume);
--
1.7.11.7


2013-01-11 20:25:24

by Joao Paulo Rechi Vita

[permalink] [raw]
Subject: [PATCH BlueZ 01/11] transport: Initialize the "Volume" property with 50%

---
profiles/audio/transport.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
index a4370a5..6ffa98a 100644
--- a/profiles/audio/transport.c
+++ b/profiles/audio/transport.c
@@ -787,7 +787,7 @@ struct media_transport *media_transport_create(struct media_endpoint *endpoint,
struct a2dp_transport *a2dp;

a2dp = g_new0(struct a2dp_transport, 1);
- a2dp->volume = -1;
+ a2dp->volume = 63;

transport->resume = resume_a2dp;
transport->suspend = suspend_a2dp;
--
1.7.11.7


2013-01-11 20:25:30

by Joao Paulo Rechi Vita

[permalink] [raw]
Subject: [PATCH BlueZ 07/11] avrcp: Handle RegisterNotification for volume

This commit answers a NOTIFY command to register for nofications of
EVENT_VOLUME_CHANGED with an INTERIM response containing the current
absolute volume value.
---
profiles/audio/avrcp.c | 15 +++++++++++++++
profiles/audio/transport.h | 1 +
2 files changed, 16 insertions(+)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index bb8ab24..b1016df 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -62,6 +62,7 @@
#include "avdtp.h"
#include "sink.h"
#include "player.h"
+#include "transport.h"

/* Company IDs for vendor dependent commands */
#define IEEEID_BTSIG 0x001958
@@ -1247,6 +1248,7 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
uint8_t transaction)
{
struct avrcp_player *player = session->player;
+ struct audio_device *dev = session->dev;
uint16_t len = ntohs(pdu->params_len);
uint64_t uid;
GList *settings;
@@ -1293,6 +1295,17 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
}

break;
+ case AVRCP_EVENT_VOLUME_CHANGED:
+ if (session->version < 0x0104)
+ goto err;
+
+ pdu->params[1] = media_transport_get_device_volume(dev);
+ if (pdu->params[1] > 127)
+ goto err;
+
+ len = 2;
+
+ break;
default:
/* All other events are not supported yet */
goto err;
@@ -1414,6 +1427,8 @@ static const struct control_pdu_handler tg_control_handlers[] = {
static const struct control_pdu_handler ct_control_handlers[] = {
{ AVRCP_GET_CAPABILITIES, AVC_CTYPE_STATUS,
avrcp_handle_get_capabilities },
+ { AVRCP_REGISTER_NOTIFICATION, AVC_CTYPE_NOTIFY,
+ avrcp_handle_register_notification },
{ },
};

diff --git a/profiles/audio/transport.h b/profiles/audio/transport.h
index 78c6fa7..cbbd0b6 100644
--- a/profiles/audio/transport.h
+++ b/profiles/audio/transport.h
@@ -23,6 +23,7 @@
*/

struct media_transport;
+struct media_endpoint;

struct media_transport *media_transport_create(struct media_endpoint *endpoint,
struct audio_device *device,
--
1.7.11.7


2013-01-11 20:25:26

by Joao Paulo Rechi Vita

[permalink] [raw]
Subject: [PATCH BlueZ 03/11] avrcp: Add EVENT_VOLUME_CHANGED to CT capabilities

---
profiles/audio/avrcp.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index ef324b0..bb8ab24 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -2164,6 +2164,9 @@ static void session_ct_init(struct avrcp *session)

session->control_handlers = ct_control_handlers;

+ if (session->version >= 0x0104)
+ session->supported_events = (1 << AVRCP_EVENT_VOLUME_CHANGED);
+
DBG("%p version 0x%04x", session, session->version);

session->control_id = avctp_register_pdu_handler(session->conn,
--
1.7.11.7


2013-01-11 20:25:28

by Joao Paulo Rechi Vita

[permalink] [raw]
Subject: [PATCH BlueZ 05/11] transport: Keep a list o all existent transports

---
profiles/audio/transport.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
index 4a81d85..c3e26f4 100644
--- a/profiles/audio/transport.c
+++ b/profiles/audio/transport.c
@@ -107,6 +107,8 @@ struct media_transport {
void *data;
};

+static GSList *transports = NULL;
+
static const char *state2str(transport_state_t state)
{
switch (state) {
@@ -703,6 +705,8 @@ static void media_transport_free(void *data)
{
struct media_transport *transport = data;

+ transports = g_slist_remove(transports, transport);
+
if (transport->owner)
media_transport_remove_owner(transport);

@@ -816,6 +820,8 @@ struct media_transport *media_transport_create(struct media_endpoint *endpoint,
goto fail;
}

+ transports = g_slist_append(transports, transport);
+
return transport;

fail:
--
1.7.11.7


2013-01-11 20:25:31

by Joao Paulo Rechi Vita

[permalink] [raw]
Subject: [PATCH BlueZ 08/11] avrcp: Notify remote of volume changes

---
profiles/audio/avrcp.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index b1016df..d715177 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -2540,7 +2540,7 @@ int avrcp_set_volume(struct audio_device *dev, uint8_t volume)
{
struct avrcp_server *server;
struct avrcp *session;
- uint8_t buf[AVRCP_HEADER_LENGTH + 1];
+ uint8_t buf[AVRCP_HEADER_LENGTH + 2];
struct avrcp_header *pdu = (void *) buf;

server = find_server(servers, device_get_adapter(dev->btd_dev));
@@ -2555,13 +2555,27 @@ int avrcp_set_volume(struct audio_device *dev, uint8_t volume)

set_company_id(pdu->company_id, IEEEID_BTSIG);

- pdu->pdu_id = AVRCP_SET_ABSOLUTE_VOLUME;
- pdu->params[0] = volume;
- pdu->params_len = htons(1);
-
DBG("volume=%u", volume);

- return avctp_send_vendordep_req(session->conn, AVC_CTYPE_CONTROL,
- AVC_SUBUNIT_PANEL, buf, sizeof(buf),
+ if (session->target) {
+ pdu->pdu_id = AVRCP_SET_ABSOLUTE_VOLUME;
+ pdu->params[0] = volume;
+ pdu->params_len = htons(1);
+
+ return avctp_send_vendordep_req(session->conn,
+ AVC_CTYPE_CONTROL, AVC_SUBUNIT_PANEL,
+ buf, sizeof(buf),
avrcp_handle_set_volume, session);
+ } else {
+ uint8_t id = AVRCP_EVENT_VOLUME_CHANGED;
+ pdu->pdu_id = AVRCP_REGISTER_NOTIFICATION;
+ pdu->params[0] = AVRCP_EVENT_VOLUME_CHANGED;
+ pdu->params[1] = volume;
+ pdu->params_len = htons(2);
+
+ return avctp_send_vendordep(session->conn,
+ session->transaction_events[id],
+ AVC_CTYPE_CHANGED, AVC_SUBUNIT_PANEL,
+ buf, sizeof(buf));
+ }
}
--
1.7.11.7


2013-01-11 20:25:25

by Joao Paulo Rechi Vita

[permalink] [raw]
Subject: [PATCH BlueZ 02/11] transport: Store the value set through the 'Volume' property

---
profiles/audio/transport.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
index 6ffa98a..f3caa1b 100644
--- a/profiles/audio/transport.c
+++ b/profiles/audio/transport.c
@@ -658,6 +658,8 @@ static void set_volume(const GDBusPropertyTable *property,
if (a2dp->volume != volume)
avrcp_set_volume(transport->device, volume);

+ a2dp->volume = volume;
+
g_dbus_pending_property_success(id);
}

--
1.7.11.7