Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 632f70b7 authored by Ajay Panicker's avatar Ajay Panicker
Browse files

AVRCP: Change notification variable when the interim response is sent 

This avoids the possibility of having a changed notification be sent
before an interim response is sent when the AVRCP service has to wait
for a response from the media layer for information regarding the
notification.

Also fix the PlayPosition test for AVRCP as it was testing for play
state correctness.

Bug: 109588762
Test: Run host test net_test_avrcp
Change-Id: Icb7bb1068191ecf2a2e390854084b6b9b47537fa
(cherry picked from commit 4e8aa66b)
parent a6086950

system/profile/avrcp/device.cc

+29 −12
Original line number Diff line number Diff line
@@ -205,21 +205,18 @@ void Device::HandleNotification( 


  switch (pkt->GetEventRegistered()) {

    case Event::TRACK_CHANGED: {

      track_changed_ = Notification(true, label);

      media_interface_->GetNowPlayingList(

          base::Bind(&Device::TrackChangedNotificationResponse,

                     weak_ptr_factory_.GetWeakPtr(), label, true));

    } break;



    case Event::PLAYBACK_STATUS_CHANGED: {

      play_status_changed_ = Notification(true, label);

      media_interface_->GetPlayStatus(

          base::Bind(&Device::PlaybackStatusNotificationResponse,

                     weak_ptr_factory_.GetWeakPtr(), label, true));

    } break;



    case Event::PLAYBACK_POS_CHANGED: {

      play_pos_changed_ = Notification(true, label);

      play_pos_interval_ = pkt->GetInterval();

      media_interface_->GetPlayStatus(

          base::Bind(&Device::PlaybackPosNotificationResponse,
@@ -227,13 +224,15 @@ void Device::HandleNotification( 
    } break;



    case Event::NOW_PLAYING_CONTENT_CHANGED: {

      now_playing_changed_ = Notification(true, label);

      media_interface_->GetNowPlayingList(base::Bind(

          &Device::HandleNowPlayingNotificationResponse,

          weak_ptr_factory_.GetWeakPtr(), now_playing_changed_.second, true));

      media_interface_->GetNowPlayingList(

          base::Bind(&Device::HandleNowPlayingNotificationResponse,

                     weak_ptr_factory_.GetWeakPtr(), label, true));

    } break;



    case Event::AVAILABLE_PLAYERS_CHANGED: {

      // TODO (apanicke): If we make a separate handler function for this, make

      // sure to register the notification in the interim response.



      // Respond immediately since this notification doesn't require any info

      avail_players_changed_ = Notification(true, label);

      auto response =
@@ -243,13 +242,15 @@ void Device::HandleNotification( 
    } break;



    case Event::ADDRESSED_PLAYER_CHANGED: {

      addr_player_changed_ = Notification(true, label);

      media_interface_->GetMediaPlayerList(

          base::Bind(&Device::AddressedPlayerNotificationResponse,

                     weak_ptr_factory_.GetWeakPtr(), label, true));

    } break;



    case Event::UIDS_CHANGED: {

      // TODO (apanicke): If we make a separate handler function for this, make

      // sure to register the notification in the interim response.



      // Respond immediately since this notification doesn't require any info

      uids_changed_ = Notification(true, label);

      auto response =
@@ -362,7 +363,9 @@ void Device::TrackChangedNotificationResponse(uint8_t label, bool interim, 
  DEVICE_VLOG(1) << __func__;

  uint64_t uid = 0;



  if (!track_changed_.first) {

  if (interim) {

    track_changed_ = Notification(true, label);

  } else if (!track_changed_.first) {

    DEVICE_VLOG(0) << __func__ << ": Device not registered for update";

    return;

  }
@@ -402,7 +405,9 @@ void Device::PlaybackStatusNotificationResponse(uint8_t label, bool interim, 
  DEVICE_VLOG(1) << __func__;

  if (status.state == PlayState::PAUSED) play_pos_update_cb_.Cancel();



  if (!play_status_changed_.first) {

  if (interim) {

    play_status_changed_ = Notification(true, label);

  } else if (!play_status_changed_.first) {

    DEVICE_VLOG(0) << __func__ << ": Device not registered for update";

    return;

  }
@@ -433,7 +438,9 @@ void Device::PlaybackPosNotificationResponse(uint8_t label, bool interim, 
                                             PlayStatus status) {

  DEVICE_VLOG(4) << __func__;



  if (!play_pos_changed_.first) {

  if (interim) {

    play_pos_changed_ = Notification(true, label);

  } else if (!play_pos_changed_.first) {

    DEVICE_VLOG(3) << __func__ << ": Device not registered for update";

    return;

  }
@@ -476,6 +483,14 @@ void Device::AddressedPlayerNotificationResponse( 
    std::vector<MediaPlayerInfo> /* unused */) {

  DEVICE_VLOG(1) << __func__

                 << ": curr_player_id=" << (unsigned int)curr_player;



  if (interim) {

    addr_player_changed_ = Notification(true, label);

  } else if (!addr_player_changed_.first) {

    DEVICE_VLOG(3) << __func__ << ": Device not registered for update";

    return;

  }



  // If there is no set browsed player, use the current addressed player as the

  // default NOTE: Using any browsing commands before the browsed player is set

  // is a violation of the AVRCP Spec but there are some carkits that try too
@@ -1200,7 +1215,9 @@ void Device::HandleNowPlayingUpdate() { 
void Device::HandleNowPlayingNotificationResponse(

    uint8_t label, bool interim, std::string curr_song_id,

    std::vector<SongInfo> song_list) {

  if (!now_playing_changed_.first) {

  if (interim) {

    now_playing_changed_ = Notification(true, label);

  } else if (!now_playing_changed_.first) {

    LOG(WARNING) << "Device is not registered for now playing updates";

    return;

  }

system/profile/avrcp/tests/avrcp_device_test.cc

+293 −7
Original line number Diff line number Diff line
@@ -41,10 +41,11 @@ using TestAvrcpPacket = TestPacketType<Packet>; 
using TestBrowsePacket = TestPacketType<BrowsePacket>;



using ::testing::_;

using ::testing::MockFunction;

using ::testing::Mock;

using ::testing::MockFunction;

using ::testing::NiceMock;

using ::testing::Return;

using ::testing::SaveArg;



bool get_pts_avrcp_test(void) { return false; }
@@ -189,7 +190,9 @@ TEST_F(AvrcpDeviceTest, playPositionTest) { 


  test_device->RegisterInterfaces(&interface, &a2dp_interface, nullptr);



  PlayStatus status1 = {0x1234, 0x5678, PlayState::PLAYING};

  // TODO (apanicke): Add an underlying message loop so we can test the playing

  // state.

  PlayStatus status1 = {0x1234, 0x5678, PlayState::PAUSED};

  PlayStatus status2 = {0x5678, 0x9ABC, PlayState::STOPPED};



  EXPECT_CALL(interface, GetPlayStatus(_))
@@ -201,28 +204,311 @@ TEST_F(AvrcpDeviceTest, playPositionTest) { 
  EXPECT_CALL(a2dp_interface, active_peer())

      .WillRepeatedly(Return(test_device->GetAddress()));



  // Test the interim response for play status changed

  // Test the interim response for play position changed

  auto interim_response =

      RegisterNotificationResponseBuilder::MakePlaybackStatusBuilder(

          true, PlayState::PLAYING);

      RegisterNotificationResponseBuilder::MakePlaybackPositionBuilder(true,

                                                                       0x1234);

  EXPECT_CALL(response_cb,

              Call(1, false, matchPacket(std::move(interim_response))))

      .Times(1);



  auto request = RegisterNotificationRequestBuilder::MakeBuilder(

      Event::PLAYBACK_STATUS_CHANGED, 0);

      Event::PLAYBACK_POS_CHANGED, 0);

  auto pkt = TestAvrcpPacket::Make();

  request->Serialize(pkt);

  SendMessage(1, pkt);



  // Test the changed response for play status changed

  // Test the changed response for play position changed

  auto changed_response =

      RegisterNotificationResponseBuilder::MakePlaybackPositionBuilder(false,

                                                                       0x5678);

  EXPECT_CALL(response_cb,

              Call(1, false, matchPacket(std::move(changed_response))))

      .Times(1);

  test_device->HandlePlayPosUpdate();

}



TEST_F(AvrcpDeviceTest, trackChangedBeforeInterimTest) {

  MockMediaInterface interface;

  NiceMock<MockA2dpInterface> a2dp_interface;



  test_device->RegisterInterfaces(&interface, &a2dp_interface, nullptr);



  // Pretend the device is active

  EXPECT_CALL(a2dp_interface, active_peer())

      .WillRepeatedly(Return(test_device->GetAddress()));



  SongInfo info = {"test_id",

                   {// The attribute map

                    AttributeEntry(Attribute::TITLE, "Test Song"),

                    AttributeEntry(Attribute::ARTIST_NAME, "Test Artist"),

                    AttributeEntry(Attribute::ALBUM_NAME, "Test Album"),

                    AttributeEntry(Attribute::TRACK_NUMBER, "1"),

                    AttributeEntry(Attribute::TOTAL_NUMBER_OF_TRACKS, "2"),

                    AttributeEntry(Attribute::GENRE, "Test Genre"),

                    AttributeEntry(Attribute::PLAYING_TIME, "1000")}};

  std::vector<SongInfo> list = {info};



  MediaInterface::NowPlayingCallback interim_cb;

  MediaInterface::NowPlayingCallback changed_cb;



  EXPECT_CALL(interface, GetNowPlayingList(_))

      .Times(2)

      .WillOnce(SaveArg<0>(&interim_cb))

      .WillOnce(SaveArg<0>(&changed_cb));



  // Test that the changed response doesn't get sent before the interim

  ::testing::InSequence s;

  auto interim_response =

      RegisterNotificationResponseBuilder::MakeTrackChangedBuilder(true, 0x01);

  EXPECT_CALL(response_cb,

              Call(1, false, matchPacket(std::move(interim_response))))

      .Times(1);

  auto changed_response =

      RegisterNotificationResponseBuilder::MakeTrackChangedBuilder(false, 0x01);

  EXPECT_CALL(response_cb,

              Call(1, false, matchPacket(std::move(changed_response))))

      .Times(1);



  // Register for the update, sets interim_cb

  auto request =

      RegisterNotificationRequestBuilder::MakeBuilder(Event::TRACK_CHANGED, 0);

  auto pkt = TestAvrcpPacket::Make();

  request->Serialize(pkt);

  SendMessage(1, pkt);



  // Try to send track changed update, should fail and do nothing

  test_device->HandleTrackUpdate();



  // Send the interim response

  interim_cb.Run("test_id", list);



  // Try to send track changed update, should succeed

  test_device->HandleTrackUpdate();

  changed_cb.Run("test_id", list);

}



TEST_F(AvrcpDeviceTest, playStatusChangedBeforeInterimTest) {

  MockMediaInterface interface;

  NiceMock<MockA2dpInterface> a2dp_interface;



  // Pretend the device is active

  EXPECT_CALL(a2dp_interface, active_peer())

      .WillRepeatedly(Return(test_device->GetAddress()));



  test_device->RegisterInterfaces(&interface, &a2dp_interface, nullptr);



  MediaInterface::PlayStatusCallback interim_cb;

  MediaInterface::PlayStatusCallback changed_cb;



  EXPECT_CALL(interface, GetPlayStatus(_))

      .Times(2)

      .WillOnce(SaveArg<0>(&interim_cb))

      .WillOnce(SaveArg<0>(&changed_cb));



  // Test that the changed response doesn't get sent before the interim

  ::testing::InSequence s;

  auto interim_response =

      RegisterNotificationResponseBuilder::MakePlaybackStatusBuilder(

          true, PlayState::PLAYING);

  EXPECT_CALL(response_cb,

              Call(1, false, matchPacket(std::move(interim_response))))

      .Times(1);

  auto changed_response =

      RegisterNotificationResponseBuilder::MakePlaybackStatusBuilder(

          false, PlayState::STOPPED);

  EXPECT_CALL(response_cb,

              Call(1, false, matchPacket(std::move(changed_response))))

      .Times(1);



  // Send the registration packet

  auto request = RegisterNotificationRequestBuilder::MakeBuilder(

      Event::PLAYBACK_STATUS_CHANGED, 0);

  auto pkt = TestAvrcpPacket::Make();

  request->Serialize(pkt);

  SendMessage(1, pkt);



  // Send a play status update, should be ignored since the interim response

  // hasn't been sent yet.

  test_device->HandlePlayStatusUpdate();



  // Send the interim response.

  PlayStatus status1 = {0x1234, 0x5678, PlayState::PLAYING};

  interim_cb.Run(status1);



  // Send the changed response, should succeed this time

  test_device->HandlePlayStatusUpdate();

  PlayStatus status2 = {0x1234, 0x5678, PlayState::STOPPED};

  changed_cb.Run(status2);

}



TEST_F(AvrcpDeviceTest, playPositionChangedBeforeInterimTest) {

  MockMediaInterface interface;

  NiceMock<MockA2dpInterface> a2dp_interface;



  // Pretend the device is active

  EXPECT_CALL(a2dp_interface, active_peer())

      .WillRepeatedly(Return(test_device->GetAddress()));



  test_device->RegisterInterfaces(&interface, &a2dp_interface, nullptr);



  MediaInterface::PlayStatusCallback interim_cb;

  MediaInterface::PlayStatusCallback changed_cb;



  EXPECT_CALL(interface, GetPlayStatus(_))

      .Times(2)

      .WillOnce(SaveArg<0>(&interim_cb))

      .WillOnce(SaveArg<0>(&changed_cb));



  // Test that the changed response doesn't get sent before the interim

  ::testing::InSequence s;

  auto interim_response =

      RegisterNotificationResponseBuilder::MakePlaybackPositionBuilder(true,

                                                                       0x1234);

  EXPECT_CALL(response_cb,

              Call(1, false, matchPacket(std::move(interim_response))))

      .Times(1);

  auto changed_response =

      RegisterNotificationResponseBuilder::MakePlaybackPositionBuilder(false,

                                                                       0x5678);

  EXPECT_CALL(response_cb,

              Call(1, false, matchPacket(std::move(changed_response))))

      .Times(1);



  // Send the registration packet

  auto request = RegisterNotificationRequestBuilder::MakeBuilder(

      Event::PLAYBACK_POS_CHANGED, 0);

  auto pkt = TestAvrcpPacket::Make();

  request->Serialize(pkt);

  SendMessage(1, pkt);



  // Send a play position update, should be ignored since the notification

  // isn't registered since no interim response has been sent.

  test_device->HandlePlayPosUpdate();



  // Run the interim callback for GetPlayStatus which should be pointing to the

  // GetPlayStatus call made by the update.

  PlayStatus status1 = {0x1234, 0x5678, PlayState::PAUSED};

  interim_cb.Run(status1);



  // Send a play position update, this one should succeed.

  test_device->HandlePlayPosUpdate();

  PlayStatus status2 = {0x5678, 0x9ABC, PlayState::STOPPED};

  changed_cb.Run(status2);

}



TEST_F(AvrcpDeviceTest, nowPlayingChangedBeforeInterim) {

  MockMediaInterface interface;

  NiceMock<MockA2dpInterface> a2dp_interface;



  test_device->RegisterInterfaces(&interface, &a2dp_interface, nullptr);



  SongInfo info = {"test_id",

                   {// The attribute map

                    AttributeEntry(Attribute::TITLE, "Test Song"),

                    AttributeEntry(Attribute::ARTIST_NAME, "Test Artist"),

                    AttributeEntry(Attribute::ALBUM_NAME, "Test Album"),

                    AttributeEntry(Attribute::TRACK_NUMBER, "1"),

                    AttributeEntry(Attribute::TOTAL_NUMBER_OF_TRACKS, "2"),

                    AttributeEntry(Attribute::GENRE, "Test Genre"),

                    AttributeEntry(Attribute::PLAYING_TIME, "1000")}};

  std::vector<SongInfo> list = {info};



  MediaInterface::NowPlayingCallback interim_cb;

  MediaInterface::NowPlayingCallback changed_cb;



  EXPECT_CALL(interface, GetNowPlayingList(_))

      .Times(2)

      .WillOnce(SaveArg<0>(&interim_cb))

      .WillOnce(SaveArg<0>(&changed_cb));



  // Test that the changed response doesn't get sent before the interim

  ::testing::InSequence s;

  auto interim_response =

      RegisterNotificationResponseBuilder::MakeNowPlayingBuilder(true);

  EXPECT_CALL(response_cb,

              Call(1, false, matchPacket(std::move(interim_response))))

      .Times(1);

  auto changed_response =

      RegisterNotificationResponseBuilder::MakeNowPlayingBuilder(false);

  EXPECT_CALL(response_cb,

              Call(1, false, matchPacket(std::move(changed_response))))

      .Times(1);



  // Send the registration packet

  auto request = RegisterNotificationRequestBuilder::MakeBuilder(

      Event::NOW_PLAYING_CONTENT_CHANGED, 0);

  auto pkt = TestAvrcpPacket::Make();

  request->Serialize(pkt);

  SendMessage(1, pkt);



  // Send now playing changed, should fail since the interim response hasn't

  // been sent

  test_device->HandleNowPlayingUpdate();



  // Send the data needed for the interim response

  interim_cb.Run("test_id", list);



  // Send now playing changed, should succeed

  test_device->HandleNowPlayingUpdate();

  changed_cb.Run("test_id", list);

}



TEST_F(AvrcpDeviceTest, addressPlayerChangedBeforeInterim) {

  MockMediaInterface interface;

  NiceMock<MockA2dpInterface> a2dp_interface;



  test_device->RegisterInterfaces(&interface, &a2dp_interface, nullptr);



  MediaInterface::MediaListCallback interim_cb;

  MediaInterface::MediaListCallback changed_cb;



  EXPECT_CALL(interface, GetMediaPlayerList(_))

      .Times(2)

      .WillOnce(SaveArg<0>(&interim_cb))

      .WillOnce(SaveArg<0>(&changed_cb));



  // Test that the changed response doesn't get sent before the interim

  ::testing::InSequence s;

  auto interim_response =

      RegisterNotificationResponseBuilder::MakeAddressedPlayerBuilder(true, 0,

                                                                      0);

  EXPECT_CALL(response_cb,

              Call(1, false, matchPacket(std::move(interim_response))))

      .Times(1);

  auto changed_response =

      RegisterNotificationResponseBuilder::MakeAddressedPlayerBuilder(false, 0,

                                                                      0);

  EXPECT_CALL(response_cb,

              Call(1, false, matchPacket(std::move(changed_response))))

      .Times(1);

  // TODO (apanicke): Remove this expectation once b/110957802 is fixed and

  // we don't try to reject notifications that aren't registered.

  auto rejected_response = RejectBuilder::MakeBuilder(

      CommandPdu::REGISTER_NOTIFICATION, Status::ADDRESSED_PLAYER_CHANGED);

  EXPECT_CALL(response_cb,

              Call(_, false, matchPacket(std::move(rejected_response))))

      .Times(4);



  // Send the registration packet

  auto request = RegisterNotificationRequestBuilder::MakeBuilder(

      Event::ADDRESSED_PLAYER_CHANGED, 0);

  auto pkt = TestAvrcpPacket::Make();

  request->Serialize(pkt);

  SendMessage(1, pkt);



  // Send addressed player update, should fail since the interim response

  // hasn't been sent

  test_device->HandleAddressedPlayerUpdate();



  // Send the data needed for the interim response

  MediaPlayerInfo info = {0, "Test Player", true};

  std::vector<MediaPlayerInfo> list = {info};

  interim_cb.Run(0, list);



  // Send addressed player update, should succeed

  test_device->HandleAddressedPlayerUpdate();

  changed_cb.Run(0, list);

}



TEST_F(AvrcpDeviceTest, nowPlayingTest) {