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

Commit b1ebf102 authored by Jim Shargo's avatar Jim Shargo
Browse files

Surface: improve buffer look up by slot id 

Before, we were comparing buffers only by handle, which may not actually
be consistent for the same buffers since the fds can be duped.

Now, we use both the handle, sp, and the buffer id for the check.

Additionally, we are now collecting all the slots being used and
reporting them as an error, as multiple slots per buffer is a bad state.

Bug: 413059222
Flag: EXEMPT small fix
Test: surface tests
Change-Id: I68fbe0ed41b7f4222de5819fad81da56bf410c8b
parent 59d876a7

libs/gui/Surface.cpp

+23 −4
Original line number Diff line number Diff line
@@ -40,14 +40,17 @@ 


#include <android/native_window.h>



#include <android-base/strings.h>

#include <gui/FenceMonitor.h>

#include <gui/TraceUtils.h>

#include <utils/Errors.h>

#include <utils/Log.h>

#include <utils/NativeHandle.h>

#include <utils/Trace.h>



#include <ui/BufferQueueDefs.h>

#include <ui/DynamicDisplayInfo.h>

#include <ui/FatVector.h>

#include <ui/Fence.h>

#include <ui/GraphicBuffer.h>

#include <ui/Region.h>
@@ -1050,17 +1053,33 @@ int Surface::getSlotFromBufferLocked( 
        return BAD_VALUE;

    }



    sp<GraphicBuffer> graphicBuffer = GraphicBuffer::from(buffer);



    FatVector<int> slots; // FatVector (default size 4) to prevent heap allocations most of the time

#if COM_ANDROID_GRAPHICS_LIBGUI_FLAGS(WB_UNLIMITED_SLOTS)

    for (int i = 0; i < (int)mSlots.size(); i++) {

#else

    for (int i = 0; i < NUM_BUFFER_SLOTS; i++) {

#endif

        if (mSlots[i].buffer != nullptr &&

                mSlots[i].buffer->handle == buffer->handle) {

            return i;

        const sp<GraphicBuffer>& slotBuffer = mSlots[i].buffer;

        if (slotBuffer != nullptr &&

            ((slotBuffer == graphicBuffer) || (slotBuffer->handle == graphicBuffer->handle) ||

             (slotBuffer->getId() == graphicBuffer->getId()))) {

            slots.push_back(i);

        }

    }

    ALOGE("%s: unknown buffer: %p", __FUNCTION__, buffer->handle);



    if (slots.size() >= 1) {

        ALOGE_IF(slots.size() != 1,

                 "%s: More than one slot found for buffer handle=%p id=%" PRIu64 " slots=[%s]",

                 __FUNCTION__, buffer->handle, graphicBuffer->getId(),

                 base::Join(slots, ", ").c_str());



        return slots[0];

    }



    ALOGE("%s: unknown buffer: handle=%p id=%" PRIu64, __FUNCTION__, buffer->handle,

          graphicBuffer->getId());

    return BAD_VALUE;

}

libs/gui/tests/Surface_test.cpp

+54 −0
Original line number Diff line number Diff line
@@ -2763,4 +2763,58 @@ TEST_F(SurfaceTest, isBufferOwned) { 
    EXPECT_EQ(OK, surface->isBufferOwned(consumerAttachableBuffer, &isOwned));

    EXPECT_FALSE(isOwned);

}



TEST_F(SurfaceTest, isBufferOwned_SameBufferBufferWithDifferentHandles) {

    //

    // Surface setup:

    //

    auto [consumer, surface] = BufferItemConsumer::create(GRALLOC_USAGE_SW_READ_OFTEN);

    ASSERT_EQ(OK, surface->connect(NATIVE_WINDOW_API_CPU, sp<StubSurfaceListener>::make(), false));



    //

    // Buffer setup:

    //

    sp<GraphicBuffer> originalBuffer =

            sp<GraphicBuffer>::make(10, 10, PIXEL_FORMAT_RGBA_8888, 1, GRALLOC_USAGE_SW_READ_OFTEN);

    sp<GraphicBuffer> copiedBuffer = sp<GraphicBuffer>::make();



    // Copy all the data from one buffer to the other. At time of writing, this will clone and

    // duplicate the native handle, making them different, while the rest of the aspects of the

    // buffers remain the same.

    std::vector<uint8_t> bufferData(originalBuffer->getFlattenedSize());

    std::vector<int> fdCount(originalBuffer->getFdCount());



    void* data = bufferData.data();

    size_t size = bufferData.size();

    int* fds = fdCount.data();

    size_t count = fdCount.size();

    ASSERT_EQ(OK, originalBuffer->flatten(data, size, fds, count));



    void const* inData = bufferData.data();

    size = bufferData.size();

    int const* inFds = fdCount.data();

    count = fdCount.size();

    ASSERT_EQ(OK, copiedBuffer->unflatten(inData, size, inFds, count));



    // Double check our expectations about a flattened/unflattened buffer:

    ASSERT_NE(originalBuffer, copiedBuffer);

    ASSERT_EQ(originalBuffer->getId(), copiedBuffer->getId());



    ASSERT_EQ(originalBuffer->handle->numFds, copiedBuffer->handle->numFds);

    for (int i = 0; i < originalBuffer->handle->numFds; i++) {

        ASSERT_NE(originalBuffer->handle->data[i], copiedBuffer->handle->data[i]);

    }



    //

    // Test:

    //

    EXPECT_EQ(OK, surface->attachBuffer(originalBuffer->getNativeBuffer()));



    bool isOwned;

    ASSERT_EQ(OK, surface->isBufferOwned(originalBuffer, &isOwned));

    EXPECT_TRUE(isOwned);



    ASSERT_EQ(OK, surface->isBufferOwned(copiedBuffer, &isOwned));

    EXPECT_TRUE(isOwned);

}

} // namespace android