Closed Bug 1057337 Opened 6 years ago Closed 6 years ago

[Bluetooth] Convert A2DP/AVRCP callbacks to backend-neutral interface

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(5 files, 12 obsolete files)

11.99 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
18.37 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
7.89 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
4.77 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
16.05 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
Both, A2DP and AVRCP, are handled by the same manager class. So they should best be converted by the same patch set.
Blocks: 1048913
Depends on: 1053804
Attachment #8478289 - Flags: review?(shuang)
Attachment #8478289 - Flags: feedback?(btian)
Attachment #8478291 - Flags: review?(shuang)
Attachment #8478291 - Flags: feedback?(btian)
Attachment #8478292 - Flags: review?(shuang)
Attachment #8478292 - Flags: feedback?(btian)
Attachment #8478294 - Flags: review?(shuang)
Attachment #8478294 - Flags: feedback?(btian)
That's the final piece of the conversion. \o/ After that, some cleanups will still be required, and then I'll start with the daemon protocol.
Comment on attachment 8478289 [details] [diff] [review]
[01] Bug 1057337: Add Bluetooth A2DP and AVRCP notifications

Review of attachment 8478289 [details] [diff] [review]:
-----------------------------------------------------------------

f=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
@@ +912,5 @@
> +  memcpy(aOut.mValues, aIn.attr_values, aIn.num_attr);
> +
> +  return NS_OK;
> +}
> +#endif

Add |// ANDROID_VERSION >= 18| after #endif.

@@ +2870,5 @@
> +                                         BluetoothA2dpAudioState,
> +                                         const nsAString&>
> +    AudioStateNotification;
> +
> +  // Bluedroid Handsfree callbacks

A2DP callbacks

@@ +3071,5 @@
> +
> +  typedef BluetoothNotificationRunnable1<AvrcpNotificationHandlerWrapper,
> +                                         void,
> +                                         BluetoothAvrcpPlayerSettings,
> +                                         const BluetoothAvrcpPlayerSettings& >

nit: extra space.

@@ +3094,5 @@
> +
> +  typedef BluetoothNotificationRunnable2<AvrcpNotificationHandlerWrapper,
> +                                         void,
> +                                         uint8_t, uint8_t>
> +    VolumeChangeNotification;

Ditto.

@@ +3102,5 @@
> +                                         int, int>
> +    PassthroughCmdNotification;
> +#endif
> +
> +  // Bluedroid Handsfree callbacks

AVRCP callbacks

::: dom/bluetooth/bluedroid/BluetoothInterface.h
@@ +380,5 @@
> +                            BluetoothAvrcpRemoteFeature aFeatures)
> +  { }
> +
> +  virtual void
> +  VolumeChangeNotification(uint8_t aVolume, uint8_t aCType)

Change'd'
Attachment #8478289 - Flags: feedback?(btian) → feedback+
Comment on attachment 8478291 [details] [diff] [review]
[02] Bug 1057337: Implement Bluetooth A2DP notifications

Review of attachment 8478291 [details] [diff] [review]:
-----------------------------------------------------------------

f=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp
@@ +270,5 @@
> +    [A2DP_CONNECTION_STATE_CONNECTED] = NS_LITERAL_STRING("connected"),
> +    [A2DP_CONNECTION_STATE_DISCONNECTING] = NS_LITERAL_STRING("disconnecting")
> +  };
> +  if (aState >= MOZ_ARRAY_LENGTH(sString)) {
> +    BT_WARNING("Unknown sink state");

Print out |aState| here.

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.h
@@ +9,5 @@
>  
>  #include "BluetoothCommon.h"
>  #include "BluetoothProfileController.h"
>  #include "BluetoothProfileManagerBase.h"
> +#include "BluetoothInterface.h"

nit: alphabetical order.
Attachment #8478291 - Flags: feedback?(btian) → feedback+
Comment on attachment 8478292 [details] [diff] [review]
[03] Bug 1057337: Use Bluetooth A2DP and AVRCP notifications

Review of attachment 8478292 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8478292 - Flags: feedback?(btian) → feedback+
Comment on attachment 8478293 [details] [diff] [review]
[04] Bug 1057337: Integrate helper runnables into A2DP and AVRCP notifications

Review of attachment 8478293 [details] [diff] [review]:
-----------------------------------------------------------------

f=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp
@@ +1466,5 @@
>    uint8_t aNumAttrs, const BluetoothAvrcpMediaAttribute* aAttrs)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  nsAutoArrayPtr<BluetoothAvrcpElementAttribute> attrs(

This section should be also inside |#if ANDROID_VERSION >= 17| otherwise it's useless.

@@ +1474,5 @@
> +    attrs[i].mId = aAttrs[i];
> +    ConvertAttributeString(attrs[i].mId, attrs[i].mValue);
> +  }
> +
> +#if ANDROID_VERSION >= 17

The condition should be >= 18 or > 17. Note there's an inconsistency between >= in BluetoothInterface.cpp and > in BluetoothA2dpManager.cpp

@@ +1475,5 @@
> +    ConvertAttributeString(attrs[i].mId, attrs[i].mValue);
> +  }
> +
> +#if ANDROID_VERSION >= 17
> +  sBtAvrcpInterface->GetElementAttrRsp(aNumAttrs, attrs, nullptr);

Ensure |sBtAvrcpInterface| is not nullptr before using it.

@@ +1485,5 @@
>    BluetoothAvrcpEvent aEvent, uint32_t aParam)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  BluetoothA2dpManager* a2dp = BluetoothA2dpManager::Get();

This section should be also inside |#if ANDROID_VERSION >= 17|.

@@ +1490,5 @@
> +  if (!a2dp) {
> +    return;
> +  }
> +
> +#if ANDROID_VERSION >= 17

Ditto.
Attachment #8478293 - Flags: feedback?(btian) → feedback+
Comment on attachment 8478294 [details] [diff] [review]
[05] Bug 1057337: Cleanup BluetoothA2DPManager

Review of attachment 8478294 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8478294 - Flags: feedback?(btian) → feedback+
> ::: dom/bluetooth/bluedroid/BluetoothInterface.h
> @@ +380,5 @@
> > +                            BluetoothAvrcpRemoteFeature aFeatures)
> > +  { }
> > +
> > +  virtual void
> > +  VolumeChangeNotification(uint8_t aVolume, uint8_t aCType)
> 
> Change'd'

The Bluedroid callback is actually called 'change' without 'd'. :) Other callbacks had 'd'. I'm not sure if there's a clear guideline.
Changes since v1:

  - added comments to #endif directives
  - removed space from template typedef
Attachment #8478289 - Attachment is obsolete: true
Attachment #8478289 - Flags: review?(shuang)
Attachment #8479060 - Flags: review?(shuang)
Changes since v1:

  - fixed order of #include statements
  - print unknown A2DP connection state in error message
Attachment #8478291 - Attachment is obsolete: true
Attachment #8478291 - Flags: review?(shuang)
Attachment #8479061 - Flags: review?(shuang)
> ::: dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp
> @@ +1466,5 @@
> >    uint8_t aNumAttrs, const BluetoothAvrcpMediaAttribute* aAttrs)
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> >  
> > +  nsAutoArrayPtr<BluetoothAvrcpElementAttribute> attrs(
> 
> This section should be also inside |#if ANDROID_VERSION >= 17| otherwise
> it's useless.

These ANDROID_VERSION ifdefs will completely go away in the next major patch set, and interface method will return an error if the call is not supported. In the current patch I simply tried to minimize the amount of code that is protected by ANDROID_VERSION.
> 
> @@ +1474,5 @@
> > +    attrs[i].mId = aAttrs[i];
> > +    ConvertAttributeString(attrs[i].mId, attrs[i].mValue);
> > +  }
> > +
> > +#if ANDROID_VERSION >= 17
> 
> The condition should be >= 18 or > 17. Note there's an inconsistency between
> >= in BluetoothInterface.cpp and > in BluetoothA2dpManager.cpp

Thanks for noting. I had *fun* with these version checks several times now. :)
Changes since v1:

  - fixed checks for ANDROID_VERSION
Attachment #8478293 - Attachment is obsolete: true
Attachment #8478293 - Flags: review?(shuang)
Attachment #8479070 - Flags: review?(shuang)
Changes since v2:

  - attached correct file
Attachment #8479070 - Attachment is obsolete: true
Attachment #8479070 - Flags: review?(shuang)
Attachment #8479071 - Flags: review?(shuang)
Changes since v1:

  - rebased onto [04]
Attachment #8478294 - Attachment is obsolete: true
Attachment #8478294 - Flags: review?(shuang)
Attachment #8479073 - Flags: review?(shuang)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #12)
> The Bluedroid callback is actually called 'change' without 'd'. :) Other
> callbacks had 'd'. I'm not sure if there's a clear guideline.

I see. Please keep no 'd' to conform with bluedroid. I asked 'd' because original A2DP manager code has it.
../../../gecko/dom/bluetooth/bluedroid/BluetoothInterface.cpp: In static member function 'static void mozilla::dom::bluetooth::BluetoothAvrcpCallback::RemoteFeature(bt_bdaddr_t*, btrc_remote_features_t)':
../../../gecko/dom/bluetooth/bluedroid/BluetoothInterface.cpp:3193:25: error: no matching function for call to 'mozilla::dom::bluetooth::BluetoothNotificationRunnable2<mozilla::dom::bluetooth::BluetoothAvrcpCallback::AvrcpNotificationHandlerWrapper, void, nsString, unsigned int, const nsAString_internal&>::Dispatch(void (mozilla::dom::bluetooth::BluetoothAvrcpNotificationHandler::*)(const nsAString_internal&, mozilla::dom::bluetooth::BluetoothAvrcpRemoteFeature), bt_bdaddr_t*&, btrc_remote_features_t&)'
../../../gecko/dom/bluetooth/bluedroid/BluetoothInterface.cpp:3193:25: note: candidate is:
../../../gecko/dom/bluetooth/bluedroid/BluetoothInterface.cpp:1336:3: note: template<class T1, class T2> static void mozilla::dom::bluetooth::BluetoothNotificationRunnable2::Dispatch(Res (ObjectType::*)(Arg1, Arg2), const T1&, const T2&) [with T1 = T1; T2 = T2; ObjectWrapper = mozilla::dom::bluetooth::BluetoothAvrcpCallback::AvrcpNotificationHandlerWrapper; Res = void; Tin1 = nsString; Tin2 = unsigned int; Arg1 = const nsAString_internal&; Arg2 = unsigned int]
../../../gecko/dom/bluetooth/bluedroid/BluetoothInterface.cpp:1336:3: note:   template argument deduction/substitution failed:
../../../gecko/dom/bluetooth/bluedroid/BluetoothInterface.cpp:3193:25: note:   cannot convert '&mozilla::dom::bluetooth::BluetoothAvrcpNotificationHandler::RemoteFeatureNotification' (type 'void (mozilla::dom::bluetooth::BluetoothAvrcpNotificationHandler::*)(const nsAString_internal&, mozilla::dom::bluetooth::BluetoothAvrcpRemoteFeature)') to type 'void (mozilla::dom::bluetooth::BluetoothAvrcpNotificationHandler::*)(const nsAString_internal&, unsigned int)'
Comment on attachment 8479060 [details] [diff] [review]
[01] Bug 1057337: Add Bluetooth A2DP and AVRCP notifications (v2)

Review of attachment 8479060 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
@@ +3180,5 @@
> +  RemoteFeature(bt_bdaddr_t* aBdAddr, btrc_remote_features_t aFeatures)
> +  {
> +    RemoteFeatureNotification::Dispatch(
> +      &BluetoothAvrcpNotificationHandler::RemoteFeatureNotification,
> +      aBdAddr, aFeatures);

Does the data type match? I saw build break on flame-kk.
Attachment #8479060 - Flags: review?(shuang)
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #22)
> Comment on attachment 8479060 [details] [diff] [review]
> [01] Bug 1057337: Add Bluetooth A2DP and AVRCP notifications (v2)
> 
> Review of attachment 8479060 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
> @@ +3180,5 @@
> > +  RemoteFeature(bt_bdaddr_t* aBdAddr, btrc_remote_features_t aFeatures)
> > +  {
> > +    RemoteFeatureNotification::Dispatch(
> > +      &BluetoothAvrcpNotificationHandler::RemoteFeatureNotification,
> > +      aBdAddr, aFeatures);
> 
> Does the data type match? I saw build break on flame-kk.

Sorry about this. I'll fix it later today. It built for flame-kk at some point, but probably got broken by rebasing or whatever.
Changes since v2:

  - fix second argument's type of |RemoteNotificationNotification|
Attachment #8479060 - Attachment is obsolete: true
Attachment #8479872 - Flags: review?(shuang)
Changes since v2:

  - fix second argument's type of |RemoteNotificationNotification|
Attachment #8479061 - Attachment is obsolete: true
Attachment #8479061 - Flags: review?(shuang)
Attachment #8479873 - Flags: review?(shuang)
Changes since v2:

  - cleanup BluetoothUtils.{cpp,h} as well
Attachment #8479073 - Attachment is obsolete: true
Attachment #8479073 - Flags: review?(shuang)
Attachment #8479875 - Flags: review?(shuang)
Hi Shawn,

that was a problem with one method's arguments. Flame-kk built for me with the fixed patches. Sorry about this.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #27)
> Hi Shawn,
> 
> that was a problem with one method's arguments. Flame-kk built for me with
> the fixed patches. Sorry about this.

No problem :)
Comment on attachment 8479872 [details] [diff] [review]
[01] Bug 1057337: Add Bluetooth A2DP and AVRCP notifications (v3)

Review of attachment 8479872 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
@@ +714,5 @@
>    aOut = sVolumeType[aIn];
>    return NS_OK;
>  }
>  
>  #if ANDROID_VERSION >= 18

Perhaps we can move Convert(const bt_remote_version_t& aIn, BluetoothRemoteInfo& aOut) to this ANDROID_VERSION >= 18. So we don't need two 'ANDROID_VERSION >= 18' for 'Convert' functions.
Attachment #8479872 - Flags: review?(shuang)
Comment on attachment 8479873 [details] [diff] [review]
[02] Bug 1057337: Implement Bluetooth A2DP notifications (v3)

Review of attachment 8479873 [details] [diff] [review]:
-----------------------------------------------------------------

hg commit message seems to be duplicated.
Attachment #8479873 - Flags: review?(shuang) → review+
Changes since v3:

  - merged 'ANDROID_VERSION >= 18' sections
  - removed A2DP conversion functions from ANDROID_VERSION protection to build on Flatish
Attachment #8479872 - Attachment is obsolete: true
Attachment #8480523 - Flags: review?(shuang)
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #30)
> Comment on attachment 8479873 [details] [diff] [review]
> [02] Bug 1057337: Implement Bluetooth A2DP notifications (v3)
> 
> Review of attachment 8479873 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> hg commit message seems to be duplicated.

I checked, but it's different from the others. :)
Changes since v1:

  - Use BluetoothAvrcpMediaAttribute in |ConvertAttributeString|

|ConvertAttributeString| still used Bluedroid constants, which are different from the Gecko constants. I didn't notice until I actually tried to remove all Bluedroid headers from Gecko.
Attachment #8478292 - Attachment is obsolete: true
Attachment #8480637 - Flags: review?(shuang)
Attachment #8479071 - Attachment is obsolete: true
Changes since v3:

  - rebased onto [03](v2)
Attachment #8479875 - Attachment is obsolete: true
Attachment #8480642 - Flags: review+
You need to log in before you can comment on or make changes to this bug.