939500 - [bluedroid] Avrcp 1.3 prototype

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Support bluedroid avrcp 1.3

Comment 1

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 939500 - [bluedroid] Avrcp 1.3 prototype (WIP)

Comment 2

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 939500 - [bluedroid] Avrcp 1.3 prototype (WIP)

Now this version can correctly reply metadata.

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 939500 - [bluedroid] Avrcp 1.3 prototyp

Comment 4

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 939500 - [bluedroid] Avrcp 1.3 prototype

Comment 5

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8335461 [details] [diff] [review]
Bug 939500 - [bluedroid] Avrcp 1.3 prototype

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

Hi Shawn, please see my comments below.

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp
@@ +91,5 @@
> +  nsresult Run()
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    BluetoothA2dpManager* a2dp = BluetoothA2dpManager::Get();

Please make sure variable 'a2dp' has value. (NS_ENSURE_TRUE(a2dp, NS_OK))

@@ +114,5 @@
> +  nsresult Run()
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    BluetoothA2dpManager* a2dp = BluetoothA2dpManager::Get();

Please make sure variable 'a2dp' has value. (NS_ENSURE_TRUE(a2dp, NS_OK))

@@ +252,5 @@
> +AvrcpGetElementAttrCallback(uint8_t aNumAttr, btrc_media_attr_t* aPlayerAttrs)
> +{
> +  MOZ_ASSERT(!NS_IsMainThread());
> +  NS_DispatchToMainThread(new UpdateElementAttrsTask(aNumAttr, aPlayerAttrs));
> +

nit: extra line. (maybe it'd be more suitable to insert an extra line before NS_DispatchToMainThread)

@@ +259,5 @@
> +/*
> + * Callback for register notification (Play state change/track change/...)
> + * To reply RegisterNotification INTERIM response
> + * See AVRCP 1.3 Spec 25.2
> + * param: Is only valid if event_id is BTRC_EVT_PLAY_POS_CHANGED,

param => aParam. In addidtion, Is 'Is' a typo?

@@ +362,5 @@
>    if (ret != BT_STATUS_SUCCESS) {
>      BT_LOGR("failed to init a2dp module");
>      return false;
>    }
> +  // Until Android 4.3, AVRCP 1.3 API is supported

Question: shouldn't it be "Until Android 4.3, AVRCP 1.3 API is 'not' supported."?

@@ +367,5 @@
> +  // Gecko needs to support Android 4.2 above
> +  // If it cannot load AVRCP 1.3 HAL, ignore failure
> +  if ((sBtAvrcpInterface = (btrc_interface_t *)
> +    btInf->get_profile_interface(
> +      BT_PROFILE_AV_RC_ID)) == nullptr) {

nit: no need to wrap this line.

@@ +372,5 @@
> +    BT_LOGR("Warning: Bluetooth AVRCP HAL failed to load!");
> +  }
> +  ret = sBtAvrcpInterface->init(&sBtAvrcpCallbacks);
> +  if (ret != BT_STATUS_SUCCESS) {
> +    BT_LOGR("Warning: failed to init avrcp module");

We should return false in this if-statement just like what we've done in the same function when sBtA2dpInterface->init() fails.

@@ +451,5 @@
>    return sBluetoothA2dpManager;
>  }
>  
> +/*
> + * This fucntion maps attribute id and returns corresponding values

typo: function

@@ +455,5 @@
> + * This fucntion maps attribute id and returns corresponding values
> + * Attribute id refers to btrc_media_attr_t in bt_rc.h
> + */
> +void
> +BluetoothA2dpManager::ConvertAttributeString(int aAttrId, nsAString& aAttrStr)

Instead of declaring this function as a static member function (and this is even a public function), since this is only used in this file, in order to avoid exposing unnecessary interface, making it static should be fine.

@@ +457,5 @@
> + */
> +void
> +BluetoothA2dpManager::ConvertAttributeString(int aAttrId, nsAString& aAttrStr)
> +{
> +  BluetoothA2dpManager* a2dp = BluetoothA2dpManager::Get();

Please make sure variable 'a2dp' has value. (NS_ENSURE_TRUE_VOID(a2dp) or maybe call aAttrStr.Truncate())

@@ +476,5 @@
> +      aAttrStr.AppendInt(a2dp->GetTotalMediaNumber());
> +      break;
> +    case BTRC_MEDIA_ATTR_GENRE:
> +      // we currently don't support genre from music player
> +      aAttrStr = EmptyString();

aAttrStr.Truncate() should be fine.

@@ +759,5 @@
>    mMediaNumber = aMediaNumber;
>    mTotalMediaCount = aTotalMediaCount;
>    mDuration = aDuration;
> +  // send track changed if current track name is not moved
> +  if (mMediaNumber != aMediaNumber) {

This should be something wrong. mMediaNumber must be equal to aMediaNumber because you've just done mMediaNumber = aMediaNumber at line 759.

::: dom/bluetooth/bluedroid/gonk/BluetoothServiceBluedroid.cpp
@@ +1298,5 @@
>  BluetoothServiceBluedroid::UpdatePlayStatus(
>    uint32_t aDuration, uint32_t aPosition, ControlPlayStatus aPlayStatus)
>  {
> +  // We don't need this function. In bluez, it only calls dbus api
> +  // But it does not update BluetoothA2dpManager member fields

Based on the comment, please add MOZ_ASSERT(false) to avoid entering this function unexpectedly.

Comment 6

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 939500 - [bluedroid] Avrcp 1.3 prototype

Fixed problem addressed.

Comment 7

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 939500 - [bluedroid] Avrcp 1.3 prototype

Comment 8

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8335911 [details] [diff] [review]
Bug 939500 - [bluedroid] Avrcp 1.3 prototype

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

Hi Shawn, please check my comments below. Thanks.

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp
@@ +153,5 @@
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    BluetoothA2dpManager* a2dp = BluetoothA2dpManager::Get();
> +    NS_ENSURE_TRUE(a2dp, NS_OK);
> +    btrc_element_attr_val_t* attrs = new btrc_element_attr_val_t[mNumAttr];

[Eric Temp] Need to check again

@@ +291,5 @@
> +{
> +  MOZ_ASSERT(!NS_IsMainThread());
> +
> +  NS_DispatchToMainThread(new UpdateElementAttrsTask(aNumAttr, aPlayerAttrs));
> +

nit: extra line.

@@ +756,5 @@
>  {
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  NS_ENSURE_TRUE_VOID(sBtAvrcpInterface);
> +  // send track changed if current track name is not moved

what is "current track name is not moved?" :|

@@ +761,5 @@
> +  if (mMediaNumber != aMediaNumber) {
> +    btrc_register_notification_t param;
> +    int currentMediaNum = GetMediaNumber();
> +    for (int i = 0; i < 8; ++i) {
> +      param.track[i] = (currentMediaNum >> (56 - 8 * i));

Please add comments to explain these magic numbers.

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.h
@@ +65,3 @@
>    void GetTitle(nsAString& aTitle);
> +  void GetArtist(nsAString& aArtist);
> +  void UpdateGetElementRsp(int aNumAttr, btrc_element_attr_val_t* aPlayerAttrs);

After checking the content of UpdateGetElementRsp(), I realized that this function could be re-written as a static function, just like what you've done to ConvertAttributeString(). Then we may not need to include those header files in .h.

::: dom/bluetooth/bluedroid/gonk/BluetoothServiceBluedroid.cpp
@@ +1273,5 @@
>                                          int64_t aTotalMediaCount,
>                                          int64_t aDuration,
>                                          BluetoothReplyRunnable* aRunnable)
>  {
> +  BluetoothA2dpManager* a2dp = BluetoothA2dpManager::Get();

You need error handling in case a2dp == nullptr.

@@ +1285,5 @@
>    int64_t aDuration, int64_t aPosition,
>    const nsAString& aPlayStatus,
>    BluetoothReplyRunnable* aRunnable)
>  {
> +  BluetoothA2dpManager* a2dp = BluetoothA2dpManager::Get();

You need error handling in case a2dp == nullptr.

Comment 9

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 939500 - [bluedroid] Avrcp 1.3 prototype

Comment 10

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8336010 [details] [diff] [review]
Bug 939500 - [bluedroid] Avrcp 1.3 prototype

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

Almost there! Please update your patch and answer my questions. Thanks.

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp
@@ +139,5 @@
> +}
> +
> +static void
> +UpdateGetElementRsp(int aNumAttr,
> +                                          btrc_element_attr_val_t* aPlayerAttrs)

nit: no need to wrap.

@@ +771,5 @@
> +  if (mMediaNumber != aMediaNumber) {
> +    btrc_register_notification_t param;
> +    int currentMediaNum = GetMediaNumber();
> +    for (int i = 0; i < 8; ++i) {
> +      param.track[i] = (currentMediaNum >> (56 - 8 * i));

Please add comments to explain these magic numbers.

@@ +798,5 @@
> +  if (mPlayStatus != aPlayStatus) {
> +    btrc_register_notification_t param;
> +    param.play_status = (btrc_play_status_t) aPlayStatus;
> +    sBtAvrcpInterface->register_notification_rsp(BTRC_EVT_PLAY_STATUS_CHANGED,
> +                                                 BTRC_NOTIFICATION_TYPE_CHANGED,

Question: To my knowledge, we should only need to update to CT of BTRC_NOTIFICATION_TYPE_CHANGED when it has registered the notification. So my question is do we have to keep a set of flags to remember if the device has registered or not?

@@ +862,5 @@
> +      break;
> +    }
> +    default:
> +      break;
> +  }

ok, I would like to ask you a favor to refine this function. The logic looks good to me, however we could make it look better.

Here, sBtAvrcpInterface->register_notification_rsp is called in all cases, which obviously we should pull it out. In addition, for those events that we are not dealing with, they should be worth some warnings.

My version:

btrc_register_notification_t param;

switch (aEventId)
{
  case BTRC_EVT_PLAY_STATUS_CHANGED:
    param.play_status = (btrc_play_status_t)GetPlayStatus();
    break;
  case BTRC_EVT_TRACK_CHANGE:
    {
      int currentMediaNum = GetMediaNumber();
      for (int i = 0; i < 8; ++i) {
        param.track[i] = (currentMediaNum >> (56 - 8 * i));
      }
    }
    break;
  case BTRC_EVT_PLAY_POS_CHANGED:
    param.song_pos = GetPosition();
    mPlaybackInterval = aParam;
    break;
  default:
    BT_LOGR("AVRCP 1.3: Event [%d] hasn't been supported yet.", aEventId);
    return;
}

sBtAvrcpInterface->register_notification_rsp((btrc_event_id_t)aEventId,
                                             BTRC_NOTIFICATION_TYPE_INTERIM, &param);

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.h
@@ +12,5 @@
>  #include "BluetoothProfileManagerBase.h"
>  
> +#include <hardware/bluetooth.h>
> +#include <hardware/bt_av.h>
> +#include <hardware/bt_rc.h>

So we could move these to BluetoothA2dpManager.cpp

Comment 11

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Eric Chou [:ericchou] [:echou] from comment #10)
> Comment on attachment 8336010 [details] [diff] [review]
> Bug 939500 - [bluedroid] Avrcp 1.3 prototype
> 
> Review of attachment 8336010 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Almost there! Please update your patch and answer my questions. Thanks.
> 
> @@ +798,5 @@
> > +  if (mPlayStatus != aPlayStatus) {
> > +    btrc_register_notification_t param;
> > +    param.play_status = (btrc_play_status_t) aPlayStatus;
> > +    sBtAvrcpInterface->register_notification_rsp(BTRC_EVT_PLAY_STATUS_CHANGED,
> > +                                                 BTRC_NOTIFICATION_TYPE_CHANGED,
> 
> Question: To my knowledge, we should only need to update to CT of
> BTRC_NOTIFICATION_TYPE_CHANGED when it has registered the notification. So
> my question is do we have to keep a set of flags to remember if the device
> has registered or not?
 
Good point, when we send INTERIM, we suppose to set a flag that represents we need to send notification for change.

Comment 12

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 939500 - [bluedroid] Avrcp 1.3 prototype

Comment 13

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 939500 - [bluedroid] Avrcp 1.3 prototype

Comment 14

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8336616 [details] [diff] [review]
Bug 939500 - [bluedroid] Avrcp 1.3 prototype

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

Hi Shawn, please see my comments below. Thanks

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp
@@ +778,5 @@
> +  // See also AVRCP 1.3 Spec 5.4.2
> +  if (mMediaNumber != aMediaNumber &&
> +      mTrackChangedNotifType == BTRC_NOTIFICATION_TYPE_INTERIM) {
> +    btrc_register_notification_t param;
> +    int currentMediaNum = GetMediaNumber();

We can use mMediaNumber directly instead of defining another variable. Moreover, please note that mMediaNumber is uint32_t.

@@ +790,5 @@
> +    sBtAvrcpInterface->register_notification_rsp(BTRC_EVT_TRACK_CHANGE,
> +                                                 BTRC_NOTIFICATION_TYPE_CHANGED,
> +                                                 &param);
> +    if (mPlayPosChangedNotifType == BTRC_NOTIFICATION_TYPE_INTERIM) {
> +      param.song_pos = GetPosition();

We can use mPosition instead of GetPosition().

@@ +863,5 @@
> +  NS_ENSURE_TRUE_VOID(sBtAvrcpInterface);
> +  btrc_register_notification_t param;
> +  switch (aEventId) {
> +    case BTRC_EVT_PLAY_STATUS_CHANGED:
> +      param.play_status = (btrc_play_status_t)GetPlayStatus();

"param.play_status = mPlayStatus" should be fine.

@@ +867,5 @@
> +      param.play_status = (btrc_play_status_t)GetPlayStatus();
> +      mPlayPosChangedNotifType = BTRC_NOTIFICATION_TYPE_INTERIM;
> +      break;
> +    case BTRC_EVT_TRACK_CHANGE: {
> +      int currentMediaNum = GetMediaNumber();

We can use mMediaNumber directly instead of defining another variable. Moreover, please note that mMediaNumber is uint32_t.

@@ +878,5 @@
> +      break;
> +    }
> +    case BTRC_EVT_PLAY_POS_CHANGED:
> +      mPlayPosChangedNotifType = BTRC_NOTIFICATION_TYPE_INTERIM;
> +      param.song_pos = GetPosition();

"param.song_pos = mPosition" should be fine.

@@ +885,5 @@
> +    default:
> +      break;
> +  }
> +  sBtAvrcpInterface->register_notification_rsp((btrc_event_id_t)aEventId,
> +                                              BTRC_NOTIFICATION_TYPE_INTERIM,

nit: please line up.

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.h
@@ +100,5 @@
> +   * mPlayStatusChangedNotifType, mTrackChangedNotifType,
> +   * mPlayPosChangedNotifType represents current RegisterNotification
> +   * notification type.
> +   */
> +  int mPlayStatusChangedNotifType;

'Notif' and 'Notify' are only 1-char different. Frankly speaking, I personally prefer using 'Notify'. Here and below.

::: dom/bluetooth/bluedroid/gonk/BluetoothServiceBluedroid.cpp
@@ +1274,5 @@
>                                          int64_t aDuration,
>                                          BluetoothReplyRunnable* aRunnable)
>  {
> +  BluetoothA2dpManager* a2dp = BluetoothA2dpManager::Get();
> +  NS_ENSURE_TRUE_VOID(a2dp);

We can't return without dealing with aRunnable.

@@ +1287,5 @@
>    const nsAString& aPlayStatus,
>    BluetoothReplyRunnable* aRunnable)
>  {
> +  BluetoothA2dpManager* a2dp = BluetoothA2dpManager::Get();
> +  NS_ENSURE_TRUE_VOID(a2dp);

We can't return without dealing with aRunnable.

Comment 15

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 939500 - [bluedroid] Avrcp 1.3 prototype

Comment 16

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8336672 [details] [diff] [review]
Bug 939500 - [bluedroid] Avrcp 1.3 prototype

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

r=me with nits addressed. Thanks. :)

::: dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp
@@ +740,5 @@
>  }
>  
> +/*
> + * In bluedroid stack case, there is no interface to know exactly
> + * avrcp connection status. All connection managed by bluedroid stack.

typo: All connection's' 'are' managed

@@ +741,5 @@
>  
> +/*
> + * In bluedroid stack case, there is no interface to know exactly
> + * avrcp connection status. All connection managed by bluedroid stack.
> + *

nit: please remove the extra line

Comment 17

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 939500 - [bluedroid] Avrcp 1.3 prototype

Comment 18

[Eric Chou [:ericchou] [:echou]]

try: https://tbpl.mozilla.org/?tree=Try&rev=c5b35ba6b423

Comment 19

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Eric Chou [:ericchou] [:echou] from comment #18)
> try: https://tbpl.mozilla.org/?tree=Try&rev=c5b35ba6b423

It looks like try to get rid of this function?
+static void
+UpdateGetElementRsp(int aNumAttr, btrc_element_attr_val_t* aPlayerAttrs)
+{
+  MOZ_ASSERT(NS_IsMainThread());
+
+  NS_ENSURE_TRUE_VOID(sBtAvrcpInterface);
+  sBtAvrcpInterface->get_element_attr_rsp(aNumAttr, aPlayerAttrs);
+}

Comment 20

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

The original patch attachment 8336710 [details] [diff] [review] shall be build pass. It seems like UpdateGetElementRsp is not necessary anymore.

Comment 21

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Final:Bug 939500 - [bluedroid] Avrcp 1.3 prototype, r=echou

Fix based on Comment 18, 19.

Comment 22

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

https://tbpl.mozilla.org/?tree=Try&rev=4a9a990bcbcc

Comment 23

[Eric Chou [:ericchou] [:echou]]

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #21)
> Created attachment 8336931 [details] [diff] [review]
> Final:Bug 939500 - [bluedroid] Avrcp 1.3 prototype, r=echou
> 
> Fix based on Comment 18, 19.

Thanks.

Comment 24

[Eric Chou [:ericchou] [:echou]]

https://hg.mozilla.org/integration/b2g-inbound/rev/acab5151bc99

Thanks, Shawn.

Comment 25

[Eric Chou [:ericchou] [:echou]]

Hi Ryan,

This patch has landed on b2g-inbound for about two days but still not in m-c. Just want to make sure that sheriffs didn't miss it. Would you mind checking it?

Thanks.

Comment 26

[Eric Chou [:ericchou] [:echou]]

Oh, wait. It is in Mozilla central now, however there was no commit link pasted on this bug. Ryan, would you mind closing this bug for us?

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Sorry about that, this got merged to m-c yesterday but the bug was never marked as such :(

https://hg.mozilla.org/mozilla-central/rev/acab5151bc99

Comment 28

[Eric Chou [:ericchou] [:echou]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #27)
> Sorry about that, this got merged to m-c yesterday but the bug was never
> marked as such :(
> 
> https://hg.mozilla.org/mozilla-central/rev/acab5151bc99

No problem. Thanks. :)