From c3b14ee2681c1020de9708eff0f7fcaca792ec11 Mon Sep 17 00:00:00 2001
From: Ajay Panicker <apanicke@google.com>
Date: Mon, 2 Apr 2018 18:21:04 -0700
Subject: [PATCH] Persist VFS ID's between directory changes.
Some carkits don't honor the fact that VFS ID's only are consistent to
the folder they are currently browsing. Now a UID will remain valid
until the connection ends.
Bug: 68812037
Test: Run net_test_avrcp
Change-Id: I67898277327b54eaeca2121b9ff173b22bd4ba71
---
system/packet/avrcp/change_path.cc | 24 +++++++++++
system/packet/avrcp/change_path.h | 24 +++++++++++
system/packet/avrcp/get_folder_items.cc | 40 +++++++++++++++++++
system/packet/avrcp/get_folder_items.h | 28 +++++++++++++
.../packet/tests/avrcp/avrcp_test_packets.h | 22 +++++++++-
.../tests/avrcp/change_path_packet_test.cc | 9 +++++
.../avrcp/get_folder_items_packet_test.cc | 20 ++++++++++
system/profile/avrcp/device.cc | 4 --
system/profile/avrcp/media_id_map.h | 4 ++
.../profile/avrcp/tests/avrcp_device_test.cc | 36 ++++++++++++-----
10 files changed, 196 insertions(+), 15 deletions(-)
diff --git a/system/packet/avrcp/change_path.cc b/system/packet/avrcp/change_path.cc
index 94bd237e8bd..3c69903bf42 100644
--- a/system/packet/avrcp/change_path.cc
+++ b/system/packet/avrcp/change_path.cc
@@ -84,5 +84,29 @@ std::string ChangePathRequest::ToString() const {
return ss.str();
}
+std::unique_ptr<ChangePathRequestBuilder> ChangePathRequestBuilder::MakeBuilder(
+ uint16_t uid_counter, Direction direction, uint64_t folder_uid) {
+ std::unique_ptr<ChangePathRequestBuilder> builder(
+ new ChangePathRequestBuilder(uid_counter, direction, folder_uid));
+
+ return builder;
+}
+
+size_t ChangePathRequestBuilder::size() const {
+ return ChangePathRequest::kMinSize();
+}
+
+bool ChangePathRequestBuilder::Serialize(
+ const std::shared_ptr<::bluetooth::Packet>& pkt) {
+ ReserveSpace(pkt, size());
+
+ BrowsePacketBuilder::PushHeader(pkt, size() - BrowsePacket::kMinSize());
+
+ AddPayloadOctets2(pkt, base::ByteSwap(uid_counter_));
+ AddPayloadOctets1(pkt, static_cast<uint8_t>(direction_));
+ AddPayloadOctets8(pkt, base::ByteSwap(folder_uid_));
+ return true;
+}
+
} // namespace avrcp
} // namespace bluetooth
\ No newline at end of file
diff --git a/system/packet/avrcp/change_path.h b/system/packet/avrcp/change_path.h
index 12fae7ed9d2..a033aa8271d 100644
--- a/system/packet/avrcp/change_path.h
+++ b/system/packet/avrcp/change_path.h
@@ -69,5 +69,29 @@ class ChangePathRequest : public BrowsePacket {
using BrowsePacket::BrowsePacket;
};
+class ChangePathRequestBuilder : public BrowsePacketBuilder {
+ public:
+ virtual ~ChangePathRequestBuilder() = default;
+
+ static std::unique_ptr<ChangePathRequestBuilder> MakeBuilder(
+ uint16_t uid_counter, Direction direction, uint64_t folder_uid);
+
+ virtual size_t size() const override;
+ virtual bool Serialize(
+ const std::shared_ptr<::bluetooth::Packet>& pkt) override;
+
+ private:
+ ChangePathRequestBuilder(uint16_t uid_counter, Direction direction,
+ uint64_t folder_uid)
+ : BrowsePacketBuilder(BrowsePdu::CHANGE_PATH),
+ uid_counter_(uid_counter),
+ direction_(direction),
+ folder_uid_(folder_uid){};
+
+ uint16_t uid_counter_;
+ Direction direction_;
+ uint64_t folder_uid_;
+};
+
} // namespace avrcp
} // namespace bluetooth
\ No newline at end of file
diff --git a/system/packet/avrcp/get_folder_items.cc b/system/packet/avrcp/get_folder_items.cc
index 319960bbd1c..c95bb4a9c2a 100644
--- a/system/packet/avrcp/get_folder_items.cc
+++ b/system/packet/avrcp/get_folder_items.cc
@@ -301,5 +301,45 @@ std::string GetFolderItemsRequest::ToString() const {
return ss.str();
}
+std::unique_ptr<GetFolderItemsRequestBuilder>
+GetFolderItemsRequestBuilder::MakeBuilder(
+ Scope scope, uint32_t start_item, uint32_t end_item,
+ const std::set<Attribute>& requested_attrs) {
+ std::unique_ptr<GetFolderItemsRequestBuilder> builder(
+ new GetFolderItemsRequestBuilder(scope, start_item, end_item,
+ requested_attrs));
+
+ return builder;
+}
+
+size_t GetFolderItemsRequestBuilder::size() const {
+ size_t len = GetFolderItemsRequest::kMinSize();
+ len += requested_attrs_.size() * sizeof(Attribute);
+ return len;
+}
+
+bool GetFolderItemsRequestBuilder::Serialize(
+ const std::shared_ptr<::bluetooth::Packet>& pkt) {
+ ReserveSpace(pkt, size());
+
+ BrowsePacketBuilder::PushHeader(pkt, size() - BrowsePacket::kMinSize());
+
+ AddPayloadOctets1(pkt, static_cast<uint8_t>(scope_));
+ AddPayloadOctets4(pkt, base::ByteSwap(start_item_));
+ AddPayloadOctets4(pkt, base::ByteSwap(end_item_));
+
+ if (requested_attrs_.size() == 0) {
+ // 0xFF is the value to signify that there are no attributes requested.
+ AddPayloadOctets1(pkt, 0xFF);
+ return true;
+ }
+
+ AddPayloadOctets1(pkt, requested_attrs_.size());
+ for (const auto& attr : requested_attrs_) {
+ AddPayloadOctets4(pkt, base::ByteSwap(static_cast<uint32_t>(attr)));
+ }
+ return true;
+}
+
} // namespace avrcp
} // namespace bluetooth
\ No newline at end of file
diff --git a/system/packet/avrcp/get_folder_items.h b/system/packet/avrcp/get_folder_items.h
index 454a3cee56d..f5291afd9ae 100644
--- a/system/packet/avrcp/get_folder_items.h
+++ b/system/packet/avrcp/get_folder_items.h
@@ -97,5 +97,33 @@ class GetFolderItemsRequest : public BrowsePacket {
using BrowsePacket::BrowsePacket;
};
+class GetFolderItemsRequestBuilder : public BrowsePacketBuilder {
+ public:
+ virtual ~GetFolderItemsRequestBuilder() = default;
+
+ static std::unique_ptr<GetFolderItemsRequestBuilder> MakeBuilder(
+ Scope scope, uint32_t start_item, uint32_t end_item,
+ const std::set<Attribute>& requested_attrs);
+
+ virtual size_t size() const override;
+ virtual bool Serialize(
+ const std::shared_ptr<::bluetooth::Packet>& pkt) override;
+
+ protected:
+ GetFolderItemsRequestBuilder(Scope scope, uint32_t start_item,
+ uint32_t end_item,
+ const std::set<Attribute>& requested_attrs)
+ : BrowsePacketBuilder(BrowsePdu::GET_FOLDER_ITEMS),
+ scope_(scope),
+ start_item_(start_item),
+ end_item_(end_item),
+ requested_attrs_(requested_attrs){};
+
+ Scope scope_;
+ uint32_t start_item_;
+ uint32_t end_item_;
+ std::set<Attribute> requested_attrs_;
+};
+
} // namespace avrcp
} // namespace bluetooth
\ No newline at end of file
diff --git a/system/packet/tests/avrcp/avrcp_test_packets.h b/system/packet/tests/avrcp/avrcp_test_packets.h
index 9f179c3ec02..cd07de375c1 100644
--- a/system/packet/tests/avrcp/avrcp_test_packets.h
+++ b/system/packet/tests/avrcp/avrcp_test_packets.h
@@ -147,11 +147,31 @@ std::vector<uint8_t> reject_player_app_settings_response = {
// scope = 0x00 (Media Player List)
// start_item = 0x00
// end_item = 0x03
-// attributes_requested: none
+// attributes_requested: all
std::vector<uint8_t> get_folder_items_request = {0x71, 0x00, 0x0a, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x03, 0x00};
+// AVRCP Browse Get Folder Items Request packet for media players with
+// the following data:
+// scope = 0x01 (VFS)
+// start_item = 0x00
+// end_item = 0x09
+// attributes_requested: none
+std::vector<uint8_t> get_folder_items_request_no_attrs = {
+ 0x71, 0x00, 0x0a, 0x01, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x09, 0xff};
+
+// AVRCP Browse Get Folder Items Request packet for media players with
+// the following data:
+// scope = 0x01 (VFS)
+// start_item = 0x00
+// end_item = 0x09
+// attributes_requested: Title
+std::vector<uint8_t> get_folder_items_request_title = {
+ 0x71, 0x00, 0x0e, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x09, 0x01, 0x00, 0x00, 0x00, 0x1};
+
// AVRCP Browse Get Folder Items Request packet for vfs with
// the following data:
// scope = 0x01 (VFS)
diff --git a/system/packet/tests/avrcp/change_path_packet_test.cc b/system/packet/tests/avrcp/change_path_packet_test.cc
index 1638c282452..b14d57768ee 100644
--- a/system/packet/tests/avrcp/change_path_packet_test.cc
+++ b/system/packet/tests/avrcp/change_path_packet_test.cc
@@ -72,5 +72,14 @@ TEST(ChangePathRequestTest, invalidTest) {
ASSERT_FALSE(test_packet->IsValid());
}
+TEST(ChangePathRequestBuilder, builderTest) {
+ auto builder = ChangePathRequestBuilder::MakeBuilder(0, Direction::DOWN, 2);
+ ASSERT_EQ(builder->size(), change_path_request.size());
+
+ auto test_packet = TestChangePathReqPacket::Make();
+ builder->Serialize(test_packet);
+ ASSERT_EQ(test_packet->GetData(), change_path_request);
+}
+
} // namespace avrcp
} // namespace bluetooth
\ No newline at end of file
diff --git a/system/packet/tests/avrcp/get_folder_items_packet_test.cc b/system/packet/tests/avrcp/get_folder_items_packet_test.cc
index cf97a611008..ec2d7334572 100644
--- a/system/packet/tests/avrcp/get_folder_items_packet_test.cc
+++ b/system/packet/tests/avrcp/get_folder_items_packet_test.cc
@@ -222,5 +222,25 @@ TEST(GetFolderItemsRequestTest, getterTest) {
ASSERT_EQ(test_packet->GetAttributesRequested(), attribute_list);
}
+TEST(GetFolderItemsRequestBuilderTest, builderZeroAttrsTest) {
+ auto builder =
+ GetFolderItemsRequestBuilder::MakeBuilder(Scope::VFS, 0, 9, {});
+ ASSERT_EQ(builder->size(), get_folder_items_request_no_attrs.size());
+
+ auto test_packet = TestGetFolderItemsReqPacket::Make();
+ builder->Serialize(test_packet);
+ ASSERT_EQ(test_packet->GetData(), get_folder_items_request_no_attrs);
+}
+
+TEST(GetFolderItemsRequestBuilderTest, builderTest) {
+ auto builder = GetFolderItemsRequestBuilder::MakeBuilder(Scope::VFS, 0, 9,
+ {Attribute::TITLE});
+ ASSERT_EQ(builder->size(), get_folder_items_request_title.size());
+
+ auto test_packet = TestGetFolderItemsReqPacket::Make();
+ builder->Serialize(test_packet);
+ ASSERT_EQ(test_packet->GetData(), get_folder_items_request_title);
+}
+
} // namespace avrcp
} // namespace bluetooth
\ No newline at end of file
diff --git a/system/profile/avrcp/device.cc b/system/profile/avrcp/device.cc
index 34e8d212e23..734dc241246 100644
--- a/system/profile/avrcp/device.cc
+++ b/system/profile/avrcp/device.cc
@@ -633,9 +633,6 @@ void Device::HandleChangePath(uint8_t label,
<< "\"";
}
- // All of the VFS ID's are no longer valid
- vfs_ids_.clear();
-
media_interface_->GetFolderItems(
curr_browsed_player_id_, CurrentFolder(),
base::Bind(&Device::ChangePathResponse, base::Unretained(this), label,
@@ -838,7 +835,6 @@ void Device::GetVFSListResponse(uint8_t label,
// TODO (apanicke): Add test that checks if vfs_ids_ is the correct size after
// an operation.
- vfs_ids_.clear();
for (const auto& item : items) {
if (item.type == ListItem::FOLDER) {
vfs_ids_.insert(item.folder.media_id);
diff --git a/system/profile/avrcp/media_id_map.h b/system/profile/avrcp/media_id_map.h
index df5a545181e..f282f20d35a 100644
--- a/system/profile/avrcp/media_id_map.h
+++ b/system/profile/avrcp/media_id_map.h
@@ -44,6 +44,10 @@ class MediaIdMap {
}
uint64_t insert(std::string media_id) {
+ if (media_id_to_uid_.find(media_id) != media_id_to_uid_.end()) {
+ return media_id_to_uid_[media_id];
+ }
+
uint64_t uid = media_id_to_uid_.size() + 1;
media_id_to_uid_.insert(std::pair<std::string, uint64_t>(media_id, uid));
uid_to_media_id_.insert(std::pair<uint64_t, std::string>(uid, media_id));
diff --git a/system/profile/avrcp/tests/avrcp_device_test.cc b/system/profile/avrcp/tests/avrcp_device_test.cc
index bcdcac2878f..e9dde53b077 100644
--- a/system/profile/avrcp/tests/avrcp_device_test.cc
+++ b/system/profile/avrcp/tests/avrcp_device_test.cc
@@ -498,7 +498,7 @@ TEST_F(AvrcpDeviceTest, changePathTest) {
.Times(1)
.WillOnce(InvokeCb<2>(list2));
- // Populate the VFS ID map since we don't persist UIDs
+ // Populate the VFS ID map
auto folder_items_response = GetFolderItemsResponseBuilder::MakeVFSBuilder(
Status::NO_ERROR, 0x0000, 0xFFFF);
folder_items_response->AddFolder(FolderItem(1, 0, true, "Test Folder0"));
@@ -506,7 +506,11 @@ TEST_F(AvrcpDeviceTest, changePathTest) {
EXPECT_CALL(response_cb,
Call(1, true, matchPacket(std::move(folder_items_response))))
.Times(1);
- auto request = TestBrowsePacket::Make(get_folder_items_request_vfs);
+
+ auto folder_request_builder =
+ GetFolderItemsRequestBuilder::MakeBuilder(Scope::VFS, 0, 3, {});
+ auto request = TestBrowsePacket::Make();
+ folder_request_builder->Serialize(request);
SendBrowseMessage(1, request);
// Change path down into Test Folder1
@@ -514,19 +518,25 @@ TEST_F(AvrcpDeviceTest, changePathTest) {
ChangePathResponseBuilder::MakeBuilder(Status::NO_ERROR, list1.size());
EXPECT_CALL(response_cb,
Call(2, true, matchPacket(std::move(change_path_response))));
- request = TestBrowsePacket::Make(change_path_request);
+ auto path_request_builder =
+ ChangePathRequestBuilder::MakeBuilder(0, Direction::DOWN, 2);
+ request = TestBrowsePacket::Make();
+ path_request_builder->Serialize(request);
SendBrowseMessage(2, request);
- // Populate the VFS ID map since we don't persist UIDs
+ // Populate the new VFS ID
folder_items_response = GetFolderItemsResponseBuilder::MakeVFSBuilder(
Status::NO_ERROR, 0x0000, 0xFFFF);
- folder_items_response->AddFolder(FolderItem(1, 0, true, "Test Folder2"));
- folder_items_response->AddFolder(FolderItem(2, 0, true, "Test Folder3"));
- folder_items_response->AddFolder(FolderItem(3, 0, true, "Test Folder4"));
+ folder_items_response->AddFolder(FolderItem(3, 0, true, "Test Folder2"));
+ folder_items_response->AddFolder(FolderItem(4, 0, true, "Test Folder3"));
+ folder_items_response->AddFolder(FolderItem(5, 0, true, "Test Folder4"));
EXPECT_CALL(response_cb,
Call(3, true, matchPacket(std::move(folder_items_response))))
.Times(1);
- request = TestBrowsePacket::Make(get_folder_items_request_vfs);
+ folder_request_builder =
+ GetFolderItemsRequestBuilder::MakeBuilder(Scope::VFS, 0, 3, {});
+ request = TestBrowsePacket::Make();
+ folder_request_builder->Serialize(request);
SendBrowseMessage(3, request);
// Change path down into Test Folder3
@@ -534,7 +544,10 @@ TEST_F(AvrcpDeviceTest, changePathTest) {
ChangePathResponseBuilder::MakeBuilder(Status::NO_ERROR, list2.size());
EXPECT_CALL(response_cb,
Call(4, true, matchPacket(std::move(change_path_response))));
- request = TestBrowsePacket::Make(change_path_request);
+ path_request_builder =
+ ChangePathRequestBuilder::MakeBuilder(0, Direction::DOWN, 4);
+ request = TestBrowsePacket::Make();
+ path_request_builder->Serialize(request);
SendBrowseMessage(4, request);
// Change path up back into Test Folder1
@@ -542,7 +555,10 @@ TEST_F(AvrcpDeviceTest, changePathTest) {
ChangePathResponseBuilder::MakeBuilder(Status::NO_ERROR, list1.size());
EXPECT_CALL(response_cb,
Call(5, true, matchPacket(std::move(change_path_response))));
- request = TestBrowsePacket::Make(change_path_up_request);
+ path_request_builder =
+ ChangePathRequestBuilder::MakeBuilder(0, Direction::UP, 0);
+ request = TestBrowsePacket::Make();
+ path_request_builder->Serialize(request);
SendBrowseMessage(5, request);
}
--
GitLab