From ca70b7ba83ff4c0a9a91b52296ef089ab062e762 Mon Sep 17 00:00:00 2001 From: Fan Xu Date: Wed, 31 Oct 2018 13:20:12 -0700 Subject: Implement IBufferClient hwbinder interface Some boiler plate code for future use. Test: "atest BufferHubBuffer_test" passed. Bug: b/116681016 Change-Id: I12854ac6f553777451584e86a81f2e6064a12696 --- services/bufferhub/BufferClient.cpp | 39 +++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 services/bufferhub/BufferClient.cpp (limited to 'services/bufferhub/BufferClient.cpp') diff --git a/services/bufferhub/BufferClient.cpp b/services/bufferhub/BufferClient.cpp new file mode 100644 index 0000000000..b3662b265b --- /dev/null +++ b/services/bufferhub/BufferClient.cpp @@ -0,0 +1,39 @@ +/* + * Copyright (C) 2018 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include + +namespace android { +namespace frameworks { +namespace bufferhub { +namespace V1_0 { +namespace implementation { + +using hardware::hidl_handle; +using hardware::Void; + +Return BufferClient::duplicate(duplicate_cb _hidl_cb) { + // TODO(b/118614157): implement token generation and registration + _hidl_cb(/*token=*/hidl_handle(), /*status=*/BufferHubStatus::NO_ERROR); + return Void(); +} + +} // namespace implementation +} // namespace V1_0 +} // namespace bufferhub +} // namespace frameworks +} // namespace android \ No newline at end of file -- cgit v1.2.3-59-g8ed1b From 18d90eaf05d8b3a21a68d84ebf23ea2326067aad Mon Sep 17 00:00:00 2001 From: Fan Xu Date: Tue, 6 Nov 2018 15:46:44 -0800 Subject: Implement duplicate buffer Allow BufferClient to generate a opaque token in hidl_handle (i.e. native_handle_t) format that could be used for IBufferHub::import. The service will keep a mapping from token to client. Token comparison is handled in BufferHubService.cpp, as it's opaque to the users. Test: BufferHubBuffer_test (passed) Bug: 118614157 Change-Id: Ie2b0b650dba90fe2ed2f8091dd88acb9768f261f --- libs/ui/tests/BufferHubBuffer_test.cpp | 28 ++++++++++++++++++++ services/bufferhub/Android.bp | 2 ++ services/bufferhub/BufferClient.cpp | 30 ++++++++++++++++++++-- services/bufferhub/BufferHubService.cpp | 22 +++++++++++++++- .../bufferhub/include/bufferhub/BufferClient.h | 10 +++++++- .../bufferhub/include/bufferhub/BufferHubService.h | 17 +++++++++--- 6 files changed, 102 insertions(+), 7 deletions(-) (limited to 'services/bufferhub/BufferClient.cpp') diff --git a/libs/ui/tests/BufferHubBuffer_test.cpp b/libs/ui/tests/BufferHubBuffer_test.cpp index 143335a091..6af8033e76 100644 --- a/libs/ui/tests/BufferHubBuffer_test.cpp +++ b/libs/ui/tests/BufferHubBuffer_test.cpp @@ -144,5 +144,33 @@ TEST_F(BufferHubBufferTest, AllocateBuffer) { EXPECT_TRUE(bufferHub->allocateBuffer(desc, kUserMetadataSize, callback).isOk()); } +TEST_F(BufferHubBufferTest, DuplicateBuffer) { + // TODO(b/116681016): directly test on BufferHubBuffer instead of the service. + sp bufferhub = IBufferHub::getService(); + ASSERT_NE(nullptr, bufferhub.get()); + + // Stride is an output, rfu0 and rfu1 are reserved data slot for future use. + AHardwareBuffer_Desc aDesc = {kWidth, kHeight, kLayerCount, kFormat, + kUsage, /*stride=*/0UL, /*rfu0=*/0UL, /*rfu1=*/0ULL}; + HardwareBufferDescription desc; + memcpy(&desc, &aDesc, sizeof(HardwareBufferDescription)); + + sp client; + IBufferHub::allocateBuffer_cb alloc_cb = [&](const auto& outClient, const auto& status) { + ASSERT_EQ(status, BufferHubStatus::NO_ERROR); + ASSERT_NE(nullptr, outClient.get()); + client = outClient; + }; + ASSERT_TRUE(bufferhub->allocateBuffer(desc, kUserMetadataSize, alloc_cb).isOk()); + + IBufferClient::duplicate_cb dup_cb = [](const auto& token, const auto& status) { + ASSERT_EQ(status, BufferHubStatus::NO_ERROR); + ASSERT_NE(token.getNativeHandle(), nullptr); + EXPECT_EQ(token->numInts, 1); + EXPECT_EQ(token->numFds, 0); + }; + EXPECT_TRUE(client->duplicate(dup_cb).isOk()); +} + } // namespace } // namespace android diff --git a/services/bufferhub/Android.bp b/services/bufferhub/Android.bp index 871561f171..8b4333342d 100644 --- a/services/bufferhub/Android.bp +++ b/services/bufferhub/Android.bp @@ -34,6 +34,7 @@ cc_library_shared { ], shared_libs: [ "android.frameworks.bufferhub@1.0", + "libcutils", "libhidlbase", "libhidltransport", "libhwbinder", @@ -61,6 +62,7 @@ cc_binary { shared_libs: [ "android.frameworks.bufferhub@1.0", "libbufferhubservice", + "libcutils", "libhidltransport", "libhwbinder", "liblog", diff --git a/services/bufferhub/BufferClient.cpp b/services/bufferhub/BufferClient.cpp index b3662b265b..37fd75fe80 100644 --- a/services/bufferhub/BufferClient.cpp +++ b/services/bufferhub/BufferClient.cpp @@ -15,6 +15,7 @@ */ #include +#include #include namespace android { @@ -26,9 +27,34 @@ namespace implementation { using hardware::hidl_handle; using hardware::Void; +BufferClient* BufferClient::create(BufferHubService* service, + const std::shared_ptr& node) { + if (!service) { + ALOGE("%s: service cannot be nullptr.", __FUNCTION__); + return nullptr; + } else if (!node) { + ALOGE("%s: node cannot be nullptr.", __FUNCTION__); + return nullptr; + } + return new BufferClient(service, node); +} + Return BufferClient::duplicate(duplicate_cb _hidl_cb) { - // TODO(b/118614157): implement token generation and registration - _hidl_cb(/*token=*/hidl_handle(), /*status=*/BufferHubStatus::NO_ERROR); + if (!mBufferNode) { + // Should never happen + ALOGE("%s: node is missing.", __FUNCTION__); + _hidl_cb(/*token=*/hidl_handle(), /*status=*/BufferHubStatus::BUFFER_FREED); + return Void(); + } + + sp service = mService.promote(); + if (service == nullptr) { + // Should never happen. Kill the process. + LOG_FATAL("%s: service died.", __FUNCTION__); + } + + const hidl_handle token = service->registerToken(this); + _hidl_cb(/*token=*/token, /*status=*/BufferHubStatus::NO_ERROR); return Void(); } diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index b72b556c6e..3bfd9cbe7e 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -16,6 +16,7 @@ #include #include +#include #include namespace android { @@ -41,7 +42,7 @@ Return BufferHubService::allocateBuffer(const HardwareBufferDescription& d return Void(); } - sp client = new BufferClient(node); + sp client = BufferClient::create(this, node); // Add it to list for bookkeeping and dumpsys. std::lock_guard lock(mClientListMutex); mClientList.push_back(client); @@ -57,6 +58,25 @@ Return BufferHubService::importBuffer(const hidl_handle& /*nativeHandle*/, return Void(); } +hidl_handle BufferHubService::registerToken(const BufferClient* client) { + uint32_t token; + std::lock_guard lock(mTokenMapMutex); + do { + token = mTokenEngine(); + } while (mTokenMap.find(token) != mTokenMap.end()); + + // native_handle_t use int[], so here need one slots to fit in uint32_t + native_handle_t* handle = native_handle_create(/*numFds=*/0, /*numInts=*/1); + handle->data[0] = token; + + // returnToken owns the native_handle_t* thus doing lifecycle management + hidl_handle returnToken; + returnToken.setTo(handle, /*shoudOwn=*/true); + + mTokenMap.emplace(token, client); + return returnToken; +} + } // namespace implementation } // namespace V1_0 } // namespace bufferhub diff --git a/services/bufferhub/include/bufferhub/BufferClient.h b/services/bufferhub/include/bufferhub/BufferClient.h index c6eb791b1f..5456ec33f0 100644 --- a/services/bufferhub/include/bufferhub/BufferClient.h +++ b/services/bufferhub/include/bufferhub/BufferClient.h @@ -17,6 +17,8 @@ #ifndef ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_BUFFER_CLIENT_H #define ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_BUFFER_CLIENT_H +#include + #include #include @@ -26,6 +28,7 @@ namespace bufferhub { namespace V1_0 { namespace implementation { +using hardware::hidl_handle; using hardware::Return; // Forward declaration to avoid circular dependency @@ -35,11 +38,16 @@ class BufferClient : public IBufferClient { public: // Creates a server-side buffer client from an existing BufferNode. Note that // this funciton takes ownership of the shared_ptr. - explicit BufferClient(const std::shared_ptr& node) : mBufferNode(node){}; + // Returns a raw pointer to the BufferClient on success, nullptr on failure. + static BufferClient* create(BufferHubService* service, const std::shared_ptr& node); Return duplicate(duplicate_cb _hidl_cb) override; private: + BufferClient(wp service, const std::shared_ptr& node) + : mService(service), mBufferNode(node){}; + + wp mService; std::shared_ptr mBufferNode; }; diff --git a/services/bufferhub/include/bufferhub/BufferHubService.h b/services/bufferhub/include/bufferhub/BufferHubService.h index 1bbd807069..dbbee8fccf 100644 --- a/services/bufferhub/include/bufferhub/BufferHubService.h +++ b/services/bufferhub/include/bufferhub/BufferHubService.h @@ -18,6 +18,7 @@ #define ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_BUFFER_HUB_SERVICE_H #include +#include #include #include @@ -29,9 +30,9 @@ namespace bufferhub { namespace V1_0 { namespace implementation { -using ::android::hardware::hidl_handle; -using ::android::hardware::Return; -using ::android::hardware::graphics::common::V1_2::HardwareBufferDescription; +using hardware::hidl_handle; +using hardware::Return; +using hardware::graphics::common::V1_2::HardwareBufferDescription; class BufferHubService : public IBufferHub { public: @@ -40,10 +41,20 @@ public: allocateBuffer_cb _hidl_cb) override; Return importBuffer(const hidl_handle& nativeHandle, importBuffer_cb _hidl_cb) override; + // Non-binder functions + // Internal help function for IBufferClient::duplicate. + hidl_handle registerToken(const BufferClient* client); + private: // List of active BufferClient for bookkeeping. std::mutex mClientListMutex; std::vector> mClientList GUARDED_BY(mClientListMutex); + + // TODO(b/118180214): use a more secure implementation + std::mt19937 mTokenEngine; + // The mapping from token to the client creates it. + std::mutex mTokenMapMutex; + std::map mTokenMap GUARDED_BY(mTokenMapMutex); }; } // namespace implementation -- cgit v1.2.3-59-g8ed1b From a7422fe2e0afc5bce205db37460061e0ad7e3fb4 Mon Sep 17 00:00:00 2001 From: Fan Xu Date: Mon, 19 Nov 2018 15:21:32 -0800 Subject: Implement IBufferClient::close on BufferClient The close() function will mark a BufferClient as closed (set mClosed = true), remove it from service's mClientList, and remove all of its token in service's mTokenMap. Add test cases to make sure it works in all situations. Test: BufferHubBuffer_test (passed) Bug: 119623209 Change-Id: Ic1d17ced97b67ef5432c9d341469d8e6105e2717 --- libs/ui/tests/BufferHubBuffer_test.cpp | 93 +++++++++++++++++++++- services/bufferhub/BufferClient.cpp | 32 +++++++- services/bufferhub/BufferHubService.cpp | 24 ++++++ .../bufferhub/include/bufferhub/BufferClient.h | 8 ++ .../bufferhub/include/bufferhub/BufferHubService.h | 5 ++ 5 files changed, 155 insertions(+), 7 deletions(-) (limited to 'services/bufferhub/BufferClient.cpp') diff --git a/libs/ui/tests/BufferHubBuffer_test.cpp b/libs/ui/tests/BufferHubBuffer_test.cpp index 553ee0be8e..5f9fa672ef 100644 --- a/libs/ui/tests/BufferHubBuffer_test.cpp +++ b/libs/ui/tests/BufferHubBuffer_test.cpp @@ -127,7 +127,7 @@ TEST_F(BufferHubBufferTest, DuplicateBufferHubBuffer) { return; } -TEST_F(BufferHubBufferTest, AllocateBuffer) { +TEST_F(BufferHubBufferTest, AllocateAndFreeBuffer) { // TODO(b/116681016): directly test on BufferHubBuffer instead of the service. sp bufferHub = IBufferHub::getService(); ASSERT_NE(nullptr, bufferHub.get()); @@ -138,11 +138,51 @@ TEST_F(BufferHubBufferTest, AllocateBuffer) { HardwareBufferDescription desc; memcpy(&desc, &aDesc, sizeof(HardwareBufferDescription)); - IBufferHub::allocateBuffer_cb callback = [](const auto& client, const auto& status) { - EXPECT_EQ(status, BufferHubStatus::NO_ERROR); - EXPECT_NE(nullptr, client.get()); + sp client; + BufferHubStatus ret; + IBufferHub::allocateBuffer_cb callback = [&](const auto& outClient, const auto& outStatus) { + client = outClient; + ret = outStatus; + }; + EXPECT_TRUE(bufferHub->allocateBuffer(desc, kUserMetadataSize, callback).isOk()); + EXPECT_EQ(ret, BufferHubStatus::NO_ERROR); + ASSERT_NE(nullptr, client.get()); + + EXPECT_EQ(BufferHubStatus::NO_ERROR, client->close()); + EXPECT_EQ(BufferHubStatus::CLIENT_CLOSED, client->close()); +} + +TEST_F(BufferHubBufferTest, DuplicateFreedBuffer) { + // TODO(b/116681016): directly test on BufferHubBuffer instead of the service. + sp bufferHub = IBufferHub::getService(); + ASSERT_NE(nullptr, bufferHub.get()); + + // Stride is an output, rfu0 and rfu1 are reserved data slot for future use. + AHardwareBuffer_Desc aDesc = {kWidth, kHeight, kLayerCount, kFormat, + kUsage, /*stride=*/0UL, /*rfu0=*/0UL, /*rfu1=*/0ULL}; + HardwareBufferDescription desc; + memcpy(&desc, &aDesc, sizeof(HardwareBufferDescription)); + + sp client; + BufferHubStatus ret; + IBufferHub::allocateBuffer_cb callback = [&](const auto& outClient, const auto& outStatus) { + client = outClient; + ret = outStatus; }; EXPECT_TRUE(bufferHub->allocateBuffer(desc, kUserMetadataSize, callback).isOk()); + EXPECT_EQ(ret, BufferHubStatus::NO_ERROR); + ASSERT_NE(nullptr, client.get()); + + EXPECT_EQ(BufferHubStatus::NO_ERROR, client->close()); + + hidl_handle token; + IBufferClient::duplicate_cb dup_cb = [&](const auto& outToken, const auto& status) { + token = outToken; + ret = status; + }; + EXPECT_TRUE(client->duplicate(dup_cb).isOk()); + EXPECT_EQ(ret, BufferHubStatus::CLIENT_CLOSED); + EXPECT_EQ(token.getNativeHandle(), nullptr); } TEST_F(BufferHubBufferTest, DuplicateAndImportBuffer) { @@ -230,5 +270,50 @@ TEST_F(BufferHubBufferTest, ImportInvalidToken) { native_handle_delete(tokenHandle); } +TEST_F(BufferHubBufferTest, ImportFreedBuffer) { + // TODO(b/116681016): directly test on BufferHubBuffer instead of the service. + sp bufferhub = IBufferHub::getService(); + ASSERT_NE(nullptr, bufferhub.get()); + + // Stride is an output, rfu0 and rfu1 are reserved data slot for future use. + AHardwareBuffer_Desc aDesc = {kWidth, kHeight, kLayerCount, kFormat, + kUsage, /*stride=*/0UL, /*rfu0=*/0UL, /*rfu1=*/0ULL}; + HardwareBufferDescription desc; + memcpy(&desc, &aDesc, sizeof(HardwareBufferDescription)); + + sp client; + BufferHubStatus ret; + IBufferHub::allocateBuffer_cb alloc_cb = [&](const auto& outClient, const auto& status) { + client = outClient; + ret = status; + }; + ASSERT_TRUE(bufferhub->allocateBuffer(desc, kUserMetadataSize, alloc_cb).isOk()); + EXPECT_EQ(ret, BufferHubStatus::NO_ERROR); + ASSERT_NE(nullptr, client.get()); + + hidl_handle token; + IBufferClient::duplicate_cb dup_cb = [&](const auto& outToken, const auto& status) { + token = outToken; + ret = status; + }; + ASSERT_TRUE(client->duplicate(dup_cb).isOk()); + EXPECT_EQ(ret, BufferHubStatus::NO_ERROR); + ASSERT_NE(token.getNativeHandle(), nullptr); + EXPECT_EQ(token->numInts, 1); + EXPECT_EQ(token->numFds, 0); + + // Close the client. Now the token should be invalid. + client->close(); + + sp client2; + IBufferHub::importBuffer_cb import_cb = [&](const auto& outClient, const auto& status) { + client2 = outClient; + ret = status; + }; + EXPECT_TRUE(bufferhub->importBuffer(token, import_cb).isOk()); + EXPECT_EQ(ret, BufferHubStatus::INVALID_TOKEN); + EXPECT_EQ(nullptr, client2.get()); +} + } // namespace } // namespace android diff --git a/services/bufferhub/BufferClient.cpp b/services/bufferhub/BufferClient.cpp index 37fd75fe80..745951745a 100644 --- a/services/bufferhub/BufferClient.cpp +++ b/services/bufferhub/BufferClient.cpp @@ -39,7 +39,29 @@ BufferClient* BufferClient::create(BufferHubService* service, return new BufferClient(service, node); } +BufferClient::~BufferClient() { + close(); +} + +Return BufferClient::close() { + std::lock_guard lock(mClosedMutex); + if (mClosed) { + return BufferHubStatus::CLIENT_CLOSED; + } + + getService()->onClientClosed(this); + mBufferNode.reset(); + mClosed = true; + return BufferHubStatus::NO_ERROR; +} + Return BufferClient::duplicate(duplicate_cb _hidl_cb) { + std::lock_guard lock(mClosedMutex); + if (mClosed) { + _hidl_cb(/*token=*/hidl_handle(), /*status=*/BufferHubStatus::CLIENT_CLOSED); + return Void(); + } + if (!mBufferNode) { // Should never happen ALOGE("%s: node is missing.", __FUNCTION__); @@ -47,15 +69,19 @@ Return BufferClient::duplicate(duplicate_cb _hidl_cb) { return Void(); } + const hidl_handle token = getService()->registerToken(this); + _hidl_cb(/*token=*/token, /*status=*/BufferHubStatus::NO_ERROR); + return Void(); +} + +sp BufferClient::getService() { sp service = mService.promote(); if (service == nullptr) { // Should never happen. Kill the process. LOG_FATAL("%s: service died.", __FUNCTION__); } - const hidl_handle token = service->registerToken(this); - _hidl_cb(/*token=*/token, /*status=*/BufferHubStatus::NO_ERROR); - return Void(); + return service; } } // namespace implementation diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp index 6f97f0d8c0..409f2ea8b0 100644 --- a/services/bufferhub/BufferHubService.cpp +++ b/services/bufferhub/BufferHubService.cpp @@ -111,6 +111,30 @@ hidl_handle BufferHubService::registerToken(const wp& client) { return returnToken; } +void BufferHubService::onClientClosed(const BufferClient* client) { + removeTokenByClient(client); + + std::lock_guard lock(mClientListMutex); + auto iter = std::find(mClientList.begin(), mClientList.end(), client); + if (iter != mClientList.end()) { + mClientList.erase(iter); + } +} + +void BufferHubService::removeTokenByClient(const BufferClient* client) { + std::lock_guard lock(mTokenMapMutex); + auto iter = mTokenMap.begin(); + while (iter != mTokenMap.end()) { + if (iter->second == client) { + auto oldIter = iter; + ++iter; + mTokenMap.erase(oldIter); + } else { + ++iter; + } + } +} + } // namespace implementation } // namespace V1_0 } // namespace bufferhub diff --git a/services/bufferhub/include/bufferhub/BufferClient.h b/services/bufferhub/include/bufferhub/BufferClient.h index 769ec86776..7f5d3a6aa1 100644 --- a/services/bufferhub/include/bufferhub/BufferClient.h +++ b/services/bufferhub/include/bufferhub/BufferClient.h @@ -44,14 +44,22 @@ public: // Creates a BufferClient from an existing BufferClient. Will share the same BufferNode. explicit BufferClient(const BufferClient& other) : mService(other.mService), mBufferNode(other.mBufferNode) {} + ~BufferClient(); + Return close() override; Return duplicate(duplicate_cb _hidl_cb) override; private: BufferClient(wp service, const std::shared_ptr& node) : mService(service), mBufferNode(node) {} + sp getService(); + wp mService; + + std::mutex mClosedMutex; + bool mClosed GUARDED_BY(mClosedMutex) = false; + std::shared_ptr mBufferNode; }; diff --git a/services/bufferhub/include/bufferhub/BufferHubService.h b/services/bufferhub/include/bufferhub/BufferHubService.h index 6535659ae7..1c328b33a2 100644 --- a/services/bufferhub/include/bufferhub/BufferHubService.h +++ b/services/bufferhub/include/bufferhub/BufferHubService.h @@ -48,7 +48,12 @@ public: // Internal help function for IBufferClient::duplicate. hidl_handle registerToken(const wp& client); + void onClientClosed(const BufferClient* client); + private: + // Helper function to remove all the token belongs to a specific client. + void removeTokenByClient(const BufferClient* client); + // List of active BufferClient for bookkeeping. std::mutex mClientListMutex; std::vector> mClientList GUARDED_BY(mClientListMutex); -- cgit v1.2.3-59-g8ed1b From cfbe07453ebd7cde74a7ed75568ee2c81d726049 Mon Sep 17 00:00:00 2001 From: Fan Xu Date: Wed, 21 Nov 2018 15:03:32 -0800 Subject: Move MetadataHeader to libui To remove BufferHubMetadata off pdx dependency. Note that the __attribute__(packed) is removed as part of this CL, as it's not really needed and is triggering clang warnings. Test: build passed. Not test needed as no behavior changes. Bug: 118893702 Change-Id: Ifae94a143a2bedef68a653c57f089b95d166e6d7 --- libs/ui/BufferHubBuffer.cpp | 4 +- libs/ui/BufferHubMetadata.cpp | 5 +- libs/ui/include/ui/BufferHubDefs.h | 64 ++++++++++++++++++++++ libs/ui/include/ui/BufferHubMetadata.h | 30 ++-------- libs/ui/tests/Android.bp | 7 ++- libs/ui/tests/BufferHubMetadata_test.cpp | 2 + .../include/private/dvr/buffer_hub_defs.h | 36 ++---------- libs/vr/libdvr/include/dvr/dvr_api.h | 1 + services/bufferhub/BufferClient.cpp | 1 + services/bufferhub/include/bufferhub/BufferNode.h | 1 + services/bufferhub/tests/BufferNode_test.cpp | 4 +- 11 files changed, 91 insertions(+), 64 deletions(-) create mode 100644 libs/ui/include/ui/BufferHubDefs.h (limited to 'services/bufferhub/BufferClient.cpp') diff --git a/libs/ui/BufferHubBuffer.cpp b/libs/ui/BufferHubBuffer.cpp index 3816c1bc4f..36eaed93ad 100644 --- a/libs/ui/BufferHubBuffer.cpp +++ b/libs/ui/BufferHubBuffer.cpp @@ -40,6 +40,7 @@ #include #include +#include using android::base::unique_fd; using android::dvr::BufferTraits; @@ -69,7 +70,6 @@ using dvr::BufferHubDefs::IsClientGained; using dvr::BufferHubDefs::IsClientPosted; using dvr::BufferHubDefs::IsClientReleased; using dvr::BufferHubDefs::kHighBitsMask; -using dvr::BufferHubDefs::kMetadataHeaderSize; } // namespace @@ -161,7 +161,7 @@ int BufferHubBuffer::ImportGraphicBuffer() { } size_t metadataSize = static_cast(bufferTraits.metadata_size()); - if (metadataSize < kMetadataHeaderSize) { + if (metadataSize < BufferHubDefs::kMetadataHeaderSize) { ALOGE("BufferHubBuffer::ImportGraphicBuffer: metadata too small: %zu", metadataSize); return -EINVAL; } diff --git a/libs/ui/BufferHubMetadata.cpp b/libs/ui/BufferHubMetadata.cpp index 18d9a2c963..816707db9d 100644 --- a/libs/ui/BufferHubMetadata.cpp +++ b/libs/ui/BufferHubMetadata.cpp @@ -16,6 +16,7 @@ #include #include +#include #include #include @@ -29,8 +30,8 @@ static const int kAshmemProt = PROT_READ | PROT_WRITE; } // namespace -using dvr::BufferHubDefs::kMetadataHeaderSize; -using dvr::BufferHubDefs::MetadataHeader; +using BufferHubDefs::kMetadataHeaderSize; +using BufferHubDefs::MetadataHeader; /* static */ BufferHubMetadata BufferHubMetadata::Create(size_t userMetadataSize) { diff --git a/libs/ui/include/ui/BufferHubDefs.h b/libs/ui/include/ui/BufferHubDefs.h new file mode 100644 index 0000000000..a1948256f5 --- /dev/null +++ b/libs/ui/include/ui/BufferHubDefs.h @@ -0,0 +1,64 @@ +/* + * Copyright (C) 2018 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef ANDROID_BUFFER_HUB_DEFS_H_ +#define ANDROID_BUFFER_HUB_DEFS_H_ + +#include + +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wpacked" +// TODO(b/118893702): remove dependency once DvrNativeBufferMetadata moved out of libdvr +#include +#pragma clang diagnostic pop + +namespace android { + +namespace BufferHubDefs { + +struct __attribute__((aligned(8))) MetadataHeader { + // Internal data format, which can be updated as long as the size, padding and field alignment + // of the struct is consistent within the same ABI. As this part is subject for future updates, + // it's not stable cross Android version, so don't have it visible from outside of the Android + // platform (include Apps and vendor HAL). + + // Every client takes up one bit from the higher 32 bits and one bit from the lower 32 bits in + // buffer_state. + std::atomic buffer_state; + + // Every client takes up one bit in fence_state. Only the lower 32 bits are valid. The upper 32 + // bits are there for easier manipulation, but the value should be ignored. + std::atomic fence_state; + + // Every client takes up one bit from the higher 32 bits and one bit from the lower 32 bits in + // active_clients_bit_mask. + std::atomic active_clients_bit_mask; + + // The index of the buffer queue where the buffer belongs to. + uint64_t queue_index; + + // Public data format, which should be updated with caution. See more details in dvr_api.h + DvrNativeBufferMetadata metadata; +}; + +static_assert(sizeof(MetadataHeader) == 136, "Unexpected MetadataHeader size"); +static constexpr size_t kMetadataHeaderSize = sizeof(MetadataHeader); + +} // namespace BufferHubDefs + +} // namespace android + +#endif // ANDROID_BUFFER_HUB_DEFS_H_ diff --git a/libs/ui/include/ui/BufferHubMetadata.h b/libs/ui/include/ui/BufferHubMetadata.h index 4261971f55..212189497a 100644 --- a/libs/ui/include/ui/BufferHubMetadata.h +++ b/libs/ui/include/ui/BufferHubMetadata.h @@ -17,26 +17,8 @@ #ifndef ANDROID_BUFFER_HUB_METADATA_H_ #define ANDROID_BUFFER_HUB_METADATA_H_ -// We would eliminate the clang warnings introduced by libdpx. -// TODO(b/112338294): Remove those once BufferHub moved to use Binder -#pragma clang diagnostic push -#pragma clang diagnostic ignored "-Wconversion" -#pragma clang diagnostic ignored "-Wdouble-promotion" -#pragma clang diagnostic ignored "-Wgnu-case-range" -#pragma clang diagnostic ignored "-Wgnu-zero-variadic-macro-arguments" -#pragma clang diagnostic ignored "-Winconsistent-missing-destructor-override" -#pragma clang diagnostic ignored "-Wnested-anon-types" -#pragma clang diagnostic ignored "-Wpacked" -#pragma clang diagnostic ignored "-Wshadow" -#pragma clang diagnostic ignored "-Wsign-conversion" -#pragma clang diagnostic ignored "-Wswitch-enum" -#pragma clang diagnostic ignored "-Wundefined-func-template" -#pragma clang diagnostic ignored "-Wunused-template" -#pragma clang diagnostic ignored "-Wweak-vtables" -#include -#pragma clang diagnostic pop - #include +#include namespace android { @@ -84,23 +66,21 @@ public: bool IsValid() const { return mAshmemFd.get() != -1 && mMetadataHeader != nullptr; } size_t user_metadata_size() const { return mUserMetadataSize; } - size_t metadata_size() const { - return mUserMetadataSize + dvr::BufferHubDefs::kMetadataHeaderSize; - } + size_t metadata_size() const { return mUserMetadataSize + BufferHubDefs::kMetadataHeaderSize; } const unique_fd& ashmem_fd() const { return mAshmemFd; } - dvr::BufferHubDefs::MetadataHeader* metadata_header() { return mMetadataHeader; } + BufferHubDefs::MetadataHeader* metadata_header() { return mMetadataHeader; } private: BufferHubMetadata(size_t userMetadataSize, unique_fd ashmemFd, - dvr::BufferHubDefs::MetadataHeader* metadataHeader); + BufferHubDefs::MetadataHeader* metadataHeader); BufferHubMetadata(const BufferHubMetadata&) = delete; void operator=(const BufferHubMetadata&) = delete; size_t mUserMetadataSize = 0; unique_fd mAshmemFd; - dvr::BufferHubDefs::MetadataHeader* mMetadataHeader = nullptr; + BufferHubDefs::MetadataHeader* mMetadataHeader = nullptr; }; } // namespace android diff --git a/libs/ui/tests/Android.bp b/libs/ui/tests/Android.bp index 18bbb3e876..ca73be79d1 100644 --- a/libs/ui/tests/Android.bp +++ b/libs/ui/tests/Android.bp @@ -73,10 +73,13 @@ cc_test { cc_test { name: "BufferHubMetadata_test", - header_libs: ["libbufferhub_headers", "libdvr_headers"], + header_libs: [ + "libbufferhub_headers", + "libdvr_headers", + "libpdx_headers", + ], shared_libs: [ "libbase", - "libpdx_default_transport", "libui", "libutils", ], diff --git a/libs/ui/tests/BufferHubMetadata_test.cpp b/libs/ui/tests/BufferHubMetadata_test.cpp index 14422bf987..2265336c30 100644 --- a/libs/ui/tests/BufferHubMetadata_test.cpp +++ b/libs/ui/tests/BufferHubMetadata_test.cpp @@ -15,8 +15,10 @@ */ #include +#include #include +// TODO(b/118893702): move this function to ui/BufferHubDefs.h after ag/5483995 is landed using android::dvr::BufferHubDefs::IsBufferReleased; namespace android { diff --git a/libs/vr/libbufferhub/include/private/dvr/buffer_hub_defs.h b/libs/vr/libbufferhub/include/private/dvr/buffer_hub_defs.h index 400def7341..62ef475f16 100644 --- a/libs/vr/libbufferhub/include/private/dvr/buffer_hub_defs.h +++ b/libs/vr/libbufferhub/include/private/dvr/buffer_hub_defs.h @@ -8,8 +8,7 @@ #include #include #include - -#include +#include namespace android { namespace dvr { @@ -120,36 +119,9 @@ static inline uint64_t FindNextAvailableClientStateMask(uint64_t union_bits) { return new_low_bit + (new_low_bit << kMaxNumberOfClients); } -struct __attribute__((packed, aligned(8))) MetadataHeader { - // Internal data format, which can be updated as long as the size, padding and - // field alignment of the struct is consistent within the same ABI. As this - // part is subject for future updates, it's not stable cross Android version, - // so don't have it visible from outside of the Android platform (include Apps - // and vendor HAL). - - // Every client takes up one bit from the higher 32 bits and one bit from the - // lower 32 bits in buffer_state. - std::atomic buffer_state; - - // Every client takes up one bit in fence_state. Only the lower 32 bits are - // valid. The upper 32 bits are there for easier manipulation, but the value - // should be ignored. - std::atomic fence_state; - - // Every client takes up one bit from the higher 32 bits and one bit from the - // lower 32 bits in active_clients_bit_mask. - std::atomic active_clients_bit_mask; - - // The index of the buffer queue where the buffer belongs to. - uint64_t queue_index; - - // Public data format, which should be updated with caution. See more details - // in dvr_api.h - DvrNativeBufferMetadata metadata; -}; - -static_assert(sizeof(MetadataHeader) == 136, "Unexpected MetadataHeader size"); -static constexpr size_t kMetadataHeaderSize = sizeof(MetadataHeader); +using MetadataHeader = android::BufferHubDefs::MetadataHeader; +static constexpr size_t kMetadataHeaderSize = + android::BufferHubDefs::kMetadataHeaderSize; } // namespace BufferHubDefs diff --git a/libs/vr/libdvr/include/dvr/dvr_api.h b/libs/vr/libdvr/include/dvr/dvr_api.h index fef8512266..a204f62fff 100644 --- a/libs/vr/libdvr/include/dvr/dvr_api.h +++ b/libs/vr/libdvr/include/dvr/dvr_api.h @@ -412,6 +412,7 @@ typedef int (*DvrTrackingSensorsStopPtr)(DvrTrackingSensors* sensors); // existing data members. If new fields need to be added, please take extra care // to make sure that new data field is padded properly the size of the struct // stays same. +// TODO(b/118893702): move the definition to libnativewindow or libui struct ALIGNED_DVR_STRUCT(8) DvrNativeBufferMetadata { #ifdef __cplusplus DvrNativeBufferMetadata() diff --git a/services/bufferhub/BufferClient.cpp b/services/bufferhub/BufferClient.cpp index 745951745a..e312011696 100644 --- a/services/bufferhub/BufferClient.cpp +++ b/services/bufferhub/BufferClient.cpp @@ -17,6 +17,7 @@ #include #include #include +#include namespace android { namespace frameworks { diff --git a/services/bufferhub/include/bufferhub/BufferNode.h b/services/bufferhub/include/bufferhub/BufferNode.h index 94ef505d41..02bb5af0e0 100644 --- a/services/bufferhub/include/bufferhub/BufferNode.h +++ b/services/bufferhub/include/bufferhub/BufferNode.h @@ -3,6 +3,7 @@ #include #include +#include #include namespace android { diff --git a/services/bufferhub/tests/BufferNode_test.cpp b/services/bufferhub/tests/BufferNode_test.cpp index df31d78b89..3358c87eb8 100644 --- a/services/bufferhub/tests/BufferNode_test.cpp +++ b/services/bufferhub/tests/BufferNode_test.cpp @@ -1,7 +1,9 @@ -#include #include + +#include #include #include +#include #include namespace android { -- cgit v1.2.3-59-g8ed1b From 7da1c05e1edc39ecd7aa9c48e50048952b1759be Mon Sep 17 00:00:00 2001 From: Fan Xu Date: Wed, 16 Jan 2019 15:31:04 -0800 Subject: Log when BufferClient destroyed without close The destructor will now check mClosed and log at warning level if the client is closed. This Could happen when the client process forget to call close or died. Test: run VtsHalBufferHubV1_0TargetTest, then check "adb logcat | grep ~BufferClient" Fix: 122916649 Change-Id: I717b4ac3896a0672a5a3b81c802093f9da58c903 --- services/bufferhub/BufferClient.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'services/bufferhub/BufferClient.cpp') diff --git a/services/bufferhub/BufferClient.cpp b/services/bufferhub/BufferClient.cpp index e312011696..ec7e535699 100644 --- a/services/bufferhub/BufferClient.cpp +++ b/services/bufferhub/BufferClient.cpp @@ -41,6 +41,14 @@ BufferClient* BufferClient::create(BufferHubService* service, } BufferClient::~BufferClient() { + { + std::lock_guard lock(mClosedMutex); + if (!mClosed) { + ALOGW("%s: client of buffer #%d destroyed without close. Closing it now.", __FUNCTION__, + mBufferNode->id()); + } + } + close(); } -- cgit v1.2.3-59-g8ed1b