Change bufferhub's bufferId to int

After discussion we decided that our new system should still use int as
type of buffer id. Now the Id generator will generate int ids >= 0
instead of uint32_t ids > 0.

Remove redundant log in destructor of BufferNode, now only log when
freeId failed.

Update BufferHubIdGenerator_test.cpp to fit in current API. Add code to
cleanup generated ids in TestGenerateUniqueIncrementalID.

Test: BufferHubServer_test
Change-Id: I2018bfd009a3c311a99b9114762d0f4fbf1e3fe2
Fix: 118844348

7 files changed, 41 insertions, 47 deletions

diff --git a/services/bufferhub/BufferHubIdGenerator.cpp b/services/bufferhub/BufferHubIdGenerator.cpp
index 6444a033e1..2c12f0e26d 100644
--- a/services/bufferhub/BufferHubIdGenerator.cpp
+++ b/services/bufferhub/BufferHubIdGenerator.cpp
@@ -15,6 +15,7 @@
  */
 
 #include <bufferhub/BufferHubIdGenerator.h>
+#include <log/log.h>
 
 namespace android {
 namespace frameworks {
@@ -22,20 +23,18 @@ namespace bufferhub {
 namespace V1_0 {
 namespace implementation {
 
-constexpr uint32_t BufferHubIdGenerator::kInvalidId;
-
 BufferHubIdGenerator& BufferHubIdGenerator::getInstance() {
     static BufferHubIdGenerator generator;
 
     return generator;
 }
 
-uint32_t BufferHubIdGenerator::getId() {
+int BufferHubIdGenerator::getId() {
     std::lock_guard<std::mutex> lock(mIdsInUseMutex);
 
     do {
-        if (++mLastId >= std::numeric_limits<uint32_t>::max()) {
-            mLastId = kInvalidId + 1;
+        if (++mLastId >= std::numeric_limits<int>::max()) {
+            mLastId = 0;
         }
     } while (mIdsInUse.find(mLastId) != mIdsInUse.end());
 
@@ -43,15 +42,14 @@ uint32_t BufferHubIdGenerator::getId() {
     return mLastId;
 }
 
-bool BufferHubIdGenerator::freeId(uint32_t id) {
+void BufferHubIdGenerator::freeId(int id) {
     std::lock_guard<std::mutex> lock(mIdsInUseMutex);
     auto iter = mIdsInUse.find(id);
     if (iter != mIdsInUse.end()) {
         mIdsInUse.erase(iter);
-        return true;
+    } else {
+        ALOGW("%s: Cannot free nonexistent id #%d", __FUNCTION__, id);
     }
-
-    return false;
 }
 
 } // namespace implementation
diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp
index 6389521e6b..ad49cd62ed 100644
--- a/services/bufferhub/BufferHubService.cpp
+++ b/services/bufferhub/BufferHubService.cpp
@@ -343,15 +343,15 @@ void BufferHubService::onClientClosed(const BufferClient* client) {
 
 // Implementation of this function should be consistent with the definition of bufferInfo handle in
 // ui/BufferHubDefs.h.
-hidl_handle BufferHubService::buildBufferInfo(uint32_t bufferId, uint32_t clientBitMask,
+hidl_handle BufferHubService::buildBufferInfo(int bufferId, uint32_t clientBitMask,
                                               uint32_t userMetadataSize, const int metadataFd) {
     native_handle_t* infoHandle = native_handle_create(BufferHubDefs::kBufferInfoNumFds,
                                                        BufferHubDefs::kBufferInfoNumInts);
 
     infoHandle->data[0] = dup(metadataFd);
+    infoHandle->data[1] = bufferId;
     // Use memcpy to convert to int without missing digit.
     // TOOD(b/121345852): use bit_cast to unpack bufferInfo when C++20 becomes available.
-    memcpy(&infoHandle->data[1], &bufferId, sizeof(bufferId));
     memcpy(&infoHandle->data[2], &clientBitMask, sizeof(clientBitMask));
     memcpy(&infoHandle->data[3], &userMetadataSize, sizeof(userMetadataSize));
 
diff --git a/services/bufferhub/BufferNode.cpp b/services/bufferhub/BufferNode.cpp
index da19a6fb1d..51063903b5 100644
--- a/services/bufferhub/BufferNode.cpp
+++ b/services/bufferhub/BufferNode.cpp
@@ -30,7 +30,7 @@ void BufferNode::InitializeMetadata() {
 
 // Allocates a new BufferNode.
 BufferNode::BufferNode(uint32_t width, uint32_t height, uint32_t layer_count, uint32_t format,
-                       uint64_t usage, size_t user_metadata_size, uint32_t id)
+                       uint64_t usage, size_t user_metadata_size, int id)
       : mId(id) {
     uint32_t out_stride = 0;
     // graphicBufferId is not used in GraphicBufferAllocator::allocate
@@ -73,12 +73,8 @@ BufferNode::~BufferNode() {
     }
 
     // Free the id, if valid
-    if (id() != BufferHubIdGenerator::kInvalidId) {
-        if (BufferHubIdGenerator::getInstance().freeId(id())) {
-            ALOGI("%s: id #%u is freed.", __FUNCTION__, id());
-        } else {
-            ALOGE("%s: Cannot free nonexistent id #%u", __FUNCTION__, id());
-        }
+    if (mId >= 0) {
+        BufferHubIdGenerator::getInstance().freeId(mId);
     }
 }
 
diff --git a/services/bufferhub/include/bufferhub/BufferHubIdGenerator.h b/services/bufferhub/include/bufferhub/BufferHubIdGenerator.h
index b51fcda3cd..ef7c077feb 100644
--- a/services/bufferhub/include/bufferhub/BufferHubIdGenerator.h
+++ b/services/bufferhub/include/bufferhub/BufferHubIdGenerator.h
@@ -28,30 +28,28 @@ namespace bufferhub {
 namespace V1_0 {
 namespace implementation {
 
-// A thread-safe incremental uint32_t id generator.
+// A thread-safe, non-negative, incremental, int id generator.
 class BufferHubIdGenerator {
 public:
-    // 0 is considered invalid
-    static constexpr uint32_t kInvalidId = 0U;
-
     // Get the singleton instance of this class
     static BufferHubIdGenerator& getInstance();
 
-    // Gets next available id. If next id is greater than std::numeric_limits<uint32_t>::max() (2 ^
-    // 32 - 1), it will try to get an id start from 1 again.
-    uint32_t getId();
+    // Gets next available id. If next id is greater than std::numeric_limits<int32_t>::max(), it
+    // will try to get an id start from 0 again.
+    int getId();
 
-    // Free a specific id. Return true on freed, false on not found.
-    bool freeId(uint32_t id);
+    // Free a specific id.
+    void freeId(int id);
 
 private:
     BufferHubIdGenerator() = default;
     ~BufferHubIdGenerator() = default;
 
+    // Start from -1 so all valid ids will be >= 0
+    int mLastId = -1;
+
     std::mutex mIdsInUseMutex;
-    // Start from kInvalidID to avoid generating it.
-    uint32_t mLastId = kInvalidId;
-    std::set<uint32_t> mIdsInUse GUARDED_BY(mIdsInUseMutex);
+    std::set<int> mIdsInUse GUARDED_BY(mIdsInUseMutex);
 };
 
 } // namespace implementation
diff --git a/services/bufferhub/include/bufferhub/BufferHubService.h b/services/bufferhub/include/bufferhub/BufferHubService.h
index 04ba5b76f9..23e664e036 100644
--- a/services/bufferhub/include/bufferhub/BufferHubService.h
+++ b/services/bufferhub/include/bufferhub/BufferHubService.h
@@ -58,8 +58,8 @@ public:
 
 private:
     // Helper function to build BufferTraits.bufferInfo handle
-    hidl_handle buildBufferInfo(uint32_t bufferId, uint32_t clientBitMask,
-                                uint32_t userMetadataSize, const int metadataFd);
+    hidl_handle buildBufferInfo(int bufferId, uint32_t clientBitMask, uint32_t userMetadataSize,
+                                const int metadataFd);
 
     // Helper function to remove all the token belongs to a specific client.
     void removeTokenByClient(const BufferClient* client);
diff --git a/services/bufferhub/include/bufferhub/BufferNode.h b/services/bufferhub/include/bufferhub/BufferNode.h
index cf56c33ec0..b7a195b0a1 100644
--- a/services/bufferhub/include/bufferhub/BufferNode.h
+++ b/services/bufferhub/include/bufferhub/BufferNode.h
@@ -16,15 +16,14 @@ class BufferNode {
 public:
     // Allocates a new BufferNode.
     BufferNode(uint32_t width, uint32_t height, uint32_t layer_count, uint32_t format,
-               uint64_t usage, size_t user_metadata_size,
-               uint32_t id = BufferHubIdGenerator::kInvalidId);
+               uint64_t usage, size_t user_metadata_size, int id = -1);
 
     ~BufferNode();
 
     // Returns whether the object holds a valid metadata.
     bool IsValid() const { return metadata_.IsValid(); }
 
-    uint32_t id() const { return mId; }
+    int id() const { return mId; }
 
     size_t user_metadata_size() const { return metadata_.user_metadata_size(); }
 
@@ -64,10 +63,10 @@ private:
     // Metadata in shared memory.
     BufferHubMetadata metadata_;
 
-    // A system-unique id generated by bufferhub from 1 to std::numeric_limits<uint32_t>::max().
-    // BufferNodes not created by bufferhub will have id = 0, meaning "not specified".
-    // TODO(b/118891412): remove default id = 0 and update comments after pdx is no longer in use
-    const uint32_t mId = 0;
+    // A system-unique id generated by bufferhub from 0 to std::numeric_limits<int>::max().
+    // BufferNodes not created by bufferhub will have id < 0, meaning "not specified".
+    // TODO(b/118891412): remove default id = -1 and update comments after pdx is no longer in use
+    const int mId = -1;
 
     // The following variables are atomic variables in metadata_ that are visible
     // to Bn object and Bp objects. Please find more info in
diff --git a/services/bufferhub/tests/BufferHubIdGenerator_test.cpp b/services/bufferhub/tests/BufferHubIdGenerator_test.cpp
index fe010137fd..fb6de0d18e 100644
--- a/services/bufferhub/tests/BufferHubIdGenerator_test.cpp
+++ b/services/bufferhub/tests/BufferHubIdGenerator_test.cpp
@@ -15,25 +15,28 @@ protected:
 };
 
 TEST_F(BufferHubIdGeneratorTest, TestGenerateAndFreeID) {
-    uint32_t id = mIdGenerator->getId();
-    EXPECT_NE(id, BufferHubIdGenerator::kInvalidId);
+    int id = mIdGenerator->getId();
+    EXPECT_GE(id, 0);
 
-    EXPECT_TRUE(mIdGenerator->freeId(id));
-    EXPECT_FALSE(mIdGenerator->freeId(id));
+    mIdGenerator->freeId(id);
 }
 
 TEST_F(BufferHubIdGeneratorTest, TestGenerateUniqueIncrementalID) {
     // 10 IDs should not overflow the UniqueIdGenerator to cause a roll back to start, so the
     // resulting IDs should still keep incresing.
-    const size_t kTestSize = 10U;
-    uint32_t ids[kTestSize];
-    for (size_t i = 0UL; i < kTestSize; ++i) {
+    const int kTestSize = 10;
+    int ids[kTestSize];
+    for (int i = 0; i < kTestSize; ++i) {
         ids[i] = mIdGenerator->getId();
-        EXPECT_NE(ids[i], BufferHubIdGenerator::kInvalidId);
+        EXPECT_GE(ids[i], 0);
         if (i >= 1) {
             EXPECT_GT(ids[i], ids[i - 1]);
         }
     }
+
+    for (int i = 0; i < kTestSize; ++i) {
+        mIdGenerator->freeId(ids[i]);
+    }
 }
 
 } // namespace