From 2d87908fe8825245dcc2e1dbde027331f1e2371d Mon Sep 17 00:00:00 2001 From: Ajay Panicker Date: Tue, 26 Jun 2018 14:52:56 -0700 Subject: [PATCH] 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 --- profile/avrcp/device.cc | 41 +++-- profile/avrcp/tests/avrcp_device_test.cc | 298 ++++++++++++++++++++++++++++++- 2 files changed, 321 insertions(+), 18 deletions(-) diff --git a/profile/avrcp/device.cc b/profile/avrcp/device.cc index d2a985f98..1b1a177c2 100644 --- a/profile/avrcp/device.cc +++ b/profile/avrcp/device.cc @@ -186,21 +186,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, @@ -208,13 +205,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 = @@ -224,13 +223,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 = @@ -343,7 +344,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; } @@ -383,7 +386,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; } @@ -414,7 +419,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; } @@ -457,6 +464,14 @@ void Device::AddressedPlayerNotificationResponse( std::vector /* 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 @@ -1158,7 +1173,9 @@ void Device::HandleNowPlayingUpdate() { void Device::HandleNowPlayingNotificationResponse( uint8_t label, bool interim, std::string curr_song_id, std::vector 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; } diff --git a/profile/avrcp/tests/avrcp_device_test.cc b/profile/avrcp/tests/avrcp_device_test.cc index d50644c2a..e2edfed09 100644 --- a/profile/avrcp/tests/avrcp_device_test.cc +++ b/profile/avrcp/tests/avrcp_device_test.cc @@ -45,6 +45,7 @@ using ::testing::MockFunction; using ::testing::Mock; 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 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 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 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 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 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 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 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 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) { -- 2.11.0