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

Commit 069e8389 authored by Fan Xu's avatar Fan Xu
Browse files

Move BufferHubMetadata off pdx::fileHandle 

Use android::base::unique_fd to replace LocalHandle. For BorrowedHandle,
a const reference of the unique_fd is returned.

Updated the test cases to fit in current behavior.

Test: BufferHubMetadata_test, BufferHubBuffer_test, BufferNode_test,
buffer_hub-test (passed)
Fix: 118888072

Change-Id: I34de335ed9a10864ac226cd4d7d261ba0078045d
parent 910c0aa1

libs/ui/BufferHubBuffer.cpp

+7 −3
Original line number Diff line number Diff line
@@ -36,10 +36,12 @@ 
#include <private/dvr/bufferhub_rpc.h>

#pragma clang diagnostic pop



#include <ui/BufferHubBuffer.h>



#include <poll.h>



#include <android-base/unique_fd.h>

#include <ui/BufferHubBuffer.h>



using android::base::unique_fd;

using android::dvr::BufferTraits;

using android::dvr::DetachedBufferRPC;

using android::dvr::NativeHandleWrapper;
@@ -132,7 +134,9 @@ int BufferHubBuffer::ImportGraphicBuffer() { 
    const int bufferId = bufferTraits.id();



    // Import the metadata.

    mMetadata = BufferHubMetadata::Import(bufferTraits.take_metadata_handle());

    LocalHandle metadataHandle = bufferTraits.take_metadata_handle();

    unique_fd metadataFd(metadataHandle.Release());

    mMetadata = BufferHubMetadata::Import(std::move(metadataFd));



    if (!mMetadata.IsValid()) {

        ALOGE("BufferHubBuffer::ImportGraphicBuffer: invalid metadata.");

libs/ui/BufferHubMetadata.cpp

+12 −12
Original line number Diff line number Diff line
@@ -48,47 +48,47 @@ BufferHubMetadata BufferHubMetadata::Create(size_t userMetadataSize) { 
        return {};

    }



    // Hand over the ownership of the fd to a pdx::LocalHandle immediately after the successful

    // return of ashmem_create_region. The ashmemHandle is going to own the fd and to prevent fd

    // Hand over the ownership of the fd to a unique_fd immediately after the successful

    // return of ashmem_create_region. The ashmemFd is going to own the fd and to prevent fd

    // leaks during error handling.

    pdx::LocalHandle ashmemHandle{fd};

    unique_fd ashmemFd{fd};



    if (ashmem_set_prot_region(ashmemHandle.Get(), kAshmemProt) != 0) {

    if (ashmem_set_prot_region(ashmemFd.get(), kAshmemProt) != 0) {

        ALOGE("BufferHubMetadata::Create: failed to set protect region.");

        return {};

    }



    return BufferHubMetadata::Import(std::move(ashmemHandle));

    return BufferHubMetadata::Import(std::move(ashmemFd));

}



/* static */

BufferHubMetadata BufferHubMetadata::Import(pdx::LocalHandle ashmemHandle) {

    if (!ashmem_valid(ashmemHandle.Get())) {

BufferHubMetadata BufferHubMetadata::Import(unique_fd ashmemFd) {

    if (!ashmem_valid(ashmemFd.get())) {

        ALOGE("BufferHubMetadata::Import: invalid ashmem fd.");

        return {};

    }



    size_t metadataSize = static_cast<size_t>(ashmem_get_size_region(ashmemHandle.Get()));

    size_t metadataSize = static_cast<size_t>(ashmem_get_size_region(ashmemFd.get()));

    size_t userMetadataSize = metadataSize - kMetadataHeaderSize;



    // Note that here the buffer state is mapped from shared memory as an atomic object. The

    // std::atomic's constructor will not be called so that the original value stored in the memory

    // region can be preserved.

    auto metadataHeader = static_cast<MetadataHeader*>(mmap(nullptr, metadataSize, kAshmemProt,

                                                            MAP_SHARED, ashmemHandle.Get(),

                                                            MAP_SHARED, ashmemFd.get(),

                                                            /*offset=*/0));

    if (metadataHeader == nullptr) {

        ALOGE("BufferHubMetadata::Import: failed to map region.");

        return {};

    }



    return BufferHubMetadata(userMetadataSize, std::move(ashmemHandle), metadataHeader);

    return BufferHubMetadata(userMetadataSize, std::move(ashmemFd), metadataHeader);

}



BufferHubMetadata::BufferHubMetadata(size_t userMetadataSize, pdx::LocalHandle ashmemHandle,

BufferHubMetadata::BufferHubMetadata(size_t userMetadataSize, unique_fd ashmemFd,

                                     MetadataHeader* metadataHeader)

      : mUserMetadataSize(userMetadataSize),

        mAshmemHandle(std::move(ashmemHandle)),

        mAshmemFd(std::move(ashmemFd)),

        mMetadataHeader(metadataHeader) {}



BufferHubMetadata::~BufferHubMetadata() {

libs/ui/include/ui/BufferHubMetadata.h

+13 −11
Original line number Diff line number Diff line
@@ -33,12 +33,17 @@ 
#pragma clang diagnostic ignored "-Wundefined-func-template"

#pragma clang diagnostic ignored "-Wunused-template"

#pragma clang diagnostic ignored "-Wweak-vtables"

#include <pdx/file_handle.h>

#include <private/dvr/buffer_hub_defs.h>

#pragma clang diagnostic pop



#include <android-base/unique_fd.h>



namespace android {



namespace {

using base::unique_fd;

} // namespace



class BufferHubMetadata {

public:

    // Creates a new BufferHubMetadata backed by an ashmem region.
@@ -50,11 +55,8 @@ public: 


    // Imports an existing BufferHubMetadata from an ashmem FD.

    //

    // TODO(b/112338294): Refactor BufferHub to use Binder as its internal IPC backend instead of

    // UDS.

    //

    // @param ashmemHandle Ashmem file handle representing an ashmem region.

    static BufferHubMetadata Import(pdx::LocalHandle ashmemHandle);

    // @param ashmemFd Ashmem file descriptor representing an ashmem region.

    static BufferHubMetadata Import(unique_fd ashmemFd);



    BufferHubMetadata() = default;
@@ -67,7 +69,7 @@ public: 
            mUserMetadataSize = other.mUserMetadataSize;

            other.mUserMetadataSize = 0;



            mAshmemHandle = std::move(other.mAshmemHandle);

            mAshmemFd = std::move(other.mAshmemFd);



            // The old raw mMetadataHeader pointer must be cleared, otherwise the destructor will

            // automatically mummap() the shared memory.
@@ -79,25 +81,25 @@ public: 


    // Returns true if the metadata is valid, i.e. the metadata has a valid ashmem fd and the ashmem

    // has been mapped into virtual address space.

    bool IsValid() const { return mAshmemHandle.IsValid() && mMetadataHeader != nullptr; }

    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;

    }



    const pdx::LocalHandle& ashmem_handle() const { return mAshmemHandle; }

    const unique_fd& ashmem_fd() const { return mAshmemFd; }

    dvr::BufferHubDefs::MetadataHeader* metadata_header() { return mMetadataHeader; }



private:

    BufferHubMetadata(size_t userMetadataSize, pdx::LocalHandle ashmemHandle,

    BufferHubMetadata(size_t userMetadataSize, unique_fd ashmemFd,

                      dvr::BufferHubDefs::MetadataHeader* metadataHeader);



    BufferHubMetadata(const BufferHubMetadata&) = delete;

    void operator=(const BufferHubMetadata&) = delete;



    size_t mUserMetadataSize = 0;

    pdx::LocalHandle mAshmemHandle;

    unique_fd mAshmemFd;

    dvr::BufferHubDefs::MetadataHeader* mMetadataHeader = nullptr;

};

libs/ui/tests/BufferHubMetadata_test.cpp

+10 −12
Original line number Diff line number Diff line
@@ -43,11 +43,11 @@ TEST_F(BufferHubMetadataTest, Import_Success) { 
  EXPECT_TRUE(m1.IsValid());

  EXPECT_NE(m1.metadata_header(), nullptr);



  pdx::LocalHandle h2 = m1.ashmem_handle().Duplicate();

  EXPECT_TRUE(h2.IsValid());

  unique_fd h2 = unique_fd(dup(m1.ashmem_fd().get()));

  EXPECT_NE(h2.get(), -1);



  BufferHubMetadata m2 = BufferHubMetadata::Import(std::move(h2));

  EXPECT_FALSE(h2.IsValid());

  EXPECT_EQ(h2.get(), -1);

  EXPECT_TRUE(m1.IsValid());

  BufferHubDefs::MetadataHeader* mh1 = m1.metadata_header();

  EXPECT_NE(mh1, nullptr);
@@ -71,31 +71,29 @@ TEST_F(BufferHubMetadataTest, MoveMetadataInvalidatesOldOne) { 
  BufferHubMetadata m1 = BufferHubMetadata::Create(sizeof(int));

  EXPECT_TRUE(m1.IsValid());

  EXPECT_NE(m1.metadata_header(), nullptr);

  EXPECT_TRUE(m1.ashmem_handle().IsValid());

  EXPECT_NE(m1.ashmem_fd().get(), -1);

  EXPECT_EQ(m1.user_metadata_size(), sizeof(int));



  BufferHubMetadata m2 = std::move(m1);



  // After the move, the metadata header (a raw pointer) should be reset in the

  // older buffer.

  // After the move, the metadata header (a raw pointer) should be reset in the older buffer.

  EXPECT_EQ(m1.metadata_header(), nullptr);

  EXPECT_NE(m2.metadata_header(), nullptr);



  EXPECT_FALSE(m1.ashmem_handle().IsValid());

  EXPECT_TRUE(m2.ashmem_handle().IsValid());

  EXPECT_EQ(m1.ashmem_fd().get(), -1);

  EXPECT_NE(m2.ashmem_fd().get(), -1);



  EXPECT_EQ(m1.user_metadata_size(), 0U);

  EXPECT_EQ(m2.user_metadata_size(), sizeof(int));



  BufferHubMetadata m3{std::move(m2)};



  // After the move, the metadata header (a raw pointer) should be reset in the

  // older buffer.

  // After the move, the metadata header (a raw pointer) should be reset in the older buffer.

  EXPECT_EQ(m2.metadata_header(), nullptr);

  EXPECT_NE(m3.metadata_header(), nullptr);



  EXPECT_FALSE(m2.ashmem_handle().IsValid());

  EXPECT_TRUE(m3.ashmem_handle().IsValid());

  EXPECT_EQ(m2.ashmem_fd().get(), -1);

  EXPECT_NE(m3.ashmem_fd().get(), -1);



  EXPECT_EQ(m2.user_metadata_size(), 0U);

  EXPECT_EQ(m3.user_metadata_size(), sizeof(int));

services/vr/bufferhubd/buffer_channel.cpp

+4 −1
Original line number Diff line number Diff line
@@ -83,10 +83,13 @@ Status<BufferTraits<BorrowedHandle>> BufferChannel::OnImport( 
  ATRACE_NAME("BufferChannel::OnImport");

  ALOGD_IF(TRACE, "BufferChannel::OnImport: buffer=%d.", buffer_id());



  BorrowedHandle ashmem_handle =

      BorrowedHandle(buffer_node_->metadata().ashmem_fd().get());



  // TODO(b/112057680) Move away from the GraphicBuffer-based IonBuffer.

  return BufferTraits<BorrowedHandle>{

      /*buffer_handle=*/buffer_node_->buffer_handle(),

      /*metadata_handle=*/buffer_node_->metadata().ashmem_handle().Borrow(),

      /*metadata_handle=*/ashmem_handle,

      /*id=*/buffer_id(),

      /*client_state_mask=*/client_state_mask_,

      /*metadata_size=*/buffer_node_->metadata().metadata_size(),