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

Commit 44685cb3 authored by Steven Thomas's avatar Steven Thomas
Browse files

Merge damage region when dropping an app buffer 

When we decide to drop a buffer from a buffer queue, make sure to merge
the damage region from the dropped buffer into the current damage
region. Otherwise we'll pass the wrong damage region to hardware
composer and get broken rendering.

Bug: 136158117
Test: Wrote a new test in Transaction_test.cpp to repro the problem and
confirm the fix works.

Change-Id: Icdc61e1be3297450f2869c496dad1453fb6dca6d
parent 74d87f73

services/surfaceflinger/BufferLayerConsumer.cpp

+9 −0
Original line number Diff line number Diff line
@@ -369,6 +369,15 @@ const Region& BufferLayerConsumer::getSurfaceDamage() const { 
    return mCurrentSurfaceDamage;

}



void BufferLayerConsumer::mergeSurfaceDamage(const Region& damage) {

    if (damage.bounds() == Rect::INVALID_RECT ||

        mCurrentSurfaceDamage.bounds() == Rect::INVALID_RECT) {

        mCurrentSurfaceDamage = Region::INVALID_REGION;

    } else {

        mCurrentSurfaceDamage |= damage;

    }

}



int BufferLayerConsumer::getCurrentApi() const {

    Mutex::Autolock lock(mMutex);

    return mCurrentApi;

services/surfaceflinger/BufferLayerConsumer.h

+3 −0
Original line number Diff line number Diff line
@@ -140,6 +140,9 @@ public: 
    // must be called from SF main thread

    const Region& getSurfaceDamage() const;



    // Merge the given damage region into the current damage region value.

    void mergeSurfaceDamage(const Region& damage);



    // getCurrentApi retrieves the API which queues the current buffer.

    int getCurrentApi() const;

services/surfaceflinger/BufferQueueLayer.cpp

+2 −0
Original line number Diff line number Diff line
@@ -316,6 +316,7 @@ status_t BufferQueueLayer::updateTexImage(bool& recomputeVisibleRegions, nsecs_t 
        // and return early

        if (queuedBuffer) {

            Mutex::Autolock lock(mQueueItemLock);

            mConsumer->mergeSurfaceDamage(mQueueItems[0].mSurfaceDamage);

            mFlinger->mTimeStats->removeTimeRecord(layerID, mQueueItems[0].mFrameNumber);

            mQueueItems.removeAt(0);

            mQueuedFrames--;
@@ -351,6 +352,7 @@ status_t BufferQueueLayer::updateTexImage(bool& recomputeVisibleRegions, nsecs_t 
        // Remove any stale buffers that have been dropped during

        // updateTexImage

        while (mQueueItems[0].mFrameNumber != currentFrameNumber) {

            mConsumer->mergeSurfaceDamage(mQueueItems[0].mSurfaceDamage);

            mFlinger->mTimeStats->removeTimeRecord(layerID, mQueueItems[0].mFrameNumber);

            mQueueItems.removeAt(0);

            mQueuedFrames--;

services/surfaceflinger/tests/Transaction_test.cpp

+94 −0
Original line number Diff line number Diff line
@@ -28,6 +28,7 @@ 


#include <binder/ProcessState.h>

#include <gui/BufferItemConsumer.h>

#include <gui/IProducerListener.h>

#include <gui/ISurfaceComposer.h>

#include <gui/LayerState.h>

#include <gui/Surface.h>
@@ -6059,4 +6060,97 @@ TEST_F(RelativeZTest, LayerRemovedOffscreenRelativeParent) { 
    }

}



// This test ensures that when we drop an app buffer in SurfaceFlinger, we merge

// the dropped buffer's damage region into the next buffer's damage region. If

// we don't do this, we'll report an incorrect damage region to hardware

// composer, resulting in broken rendering. This test checks the BufferQueue

// case.

//

// Unfortunately, we don't currently have a way to inspect the damage region

// SurfaceFlinger sends to hardware composer from a test, so this test requires

// the dev to manually watch the device's screen during the test to spot broken

// rendering. Because the results can't be automatically verified, this test is

// marked disabled.

TEST_F(LayerTransactionTest, DISABLED_BufferQueueLayerMergeDamageRegionWhenDroppingBuffers) {

    const int width = mDisplayWidth;

    const int height = mDisplayHeight;

    sp<SurfaceControl> layer;

    ASSERT_NO_FATAL_FAILURE(layer = createLayer("test", width, height));

    const auto producer = layer->getIGraphicBufferProducer();

    const sp<IProducerListener> dummyListener(new DummyProducerListener);

    IGraphicBufferProducer::QueueBufferOutput queueBufferOutput;

    ASSERT_EQ(OK,

              producer->connect(dummyListener, NATIVE_WINDOW_API_CPU, true, &queueBufferOutput));



    std::map<int, sp<GraphicBuffer>> slotMap;

    auto slotToBuffer = [&](int slot, sp<GraphicBuffer>* buf) {

        ASSERT_NE(nullptr, buf);

        const auto iter = slotMap.find(slot);

        ASSERT_NE(slotMap.end(), iter);

        *buf = iter->second;

    };



    auto dequeue = [&](int* outSlot) {

        ASSERT_NE(nullptr, outSlot);

        *outSlot = -1;

        int slot;

        sp<Fence> fence;

        uint64_t age;

        FrameEventHistoryDelta timestamps;

        const status_t dequeueResult =

                producer->dequeueBuffer(&slot, &fence, width, height, PIXEL_FORMAT_RGBA_8888,

                                        GRALLOC_USAGE_SW_READ_OFTEN | GRALLOC_USAGE_SW_WRITE_OFTEN,

                                        &age, &timestamps);

        if (dequeueResult == IGraphicBufferProducer::BUFFER_NEEDS_REALLOCATION) {

            sp<GraphicBuffer> newBuf;

            ASSERT_EQ(OK, producer->requestBuffer(slot, &newBuf));

            ASSERT_NE(nullptr, newBuf.get());

            slotMap[slot] = newBuf;

        } else {

            ASSERT_EQ(OK, dequeueResult);

        }

        *outSlot = slot;

    };



    auto queue = [&](int slot, const Region& damage, nsecs_t displayTime) {

        IGraphicBufferProducer::QueueBufferInput input(

                /*timestamp=*/displayTime, /*isAutoTimestamp=*/false, HAL_DATASPACE_UNKNOWN,

                /*crop=*/Rect::EMPTY_RECT, NATIVE_WINDOW_SCALING_MODE_SCALE_TO_WINDOW,

                /*transform=*/0, Fence::NO_FENCE);

        input.setSurfaceDamage(damage);

        IGraphicBufferProducer::QueueBufferOutput output;

        ASSERT_EQ(OK, producer->queueBuffer(slot, input, &output));

    };



    auto fillAndPostBuffers = [&](const Color& color) {

        int slot1;

        ASSERT_NO_FATAL_FAILURE(dequeue(&slot1));

        int slot2;

        ASSERT_NO_FATAL_FAILURE(dequeue(&slot2));



        sp<GraphicBuffer> buf1;

        ASSERT_NO_FATAL_FAILURE(slotToBuffer(slot1, &buf1));

        sp<GraphicBuffer> buf2;

        ASSERT_NO_FATAL_FAILURE(slotToBuffer(slot2, &buf2));

        fillGraphicBufferColor(buf1, Rect(width, height), color);

        fillGraphicBufferColor(buf2, Rect(width, height), color);



        const auto displayTime = systemTime() + milliseconds_to_nanoseconds(100);

        ASSERT_NO_FATAL_FAILURE(queue(slot1, Region::INVALID_REGION, displayTime));

        ASSERT_NO_FATAL_FAILURE(

                queue(slot2, Region(Rect(width / 3, height / 3, 2 * width / 3, 2 * height / 3)),

                      displayTime));

    };



    const auto startTime = systemTime();

    const std::array<Color, 3> colors = {Color::RED, Color::GREEN, Color::BLUE};

    int colorIndex = 0;

    while (nanoseconds_to_seconds(systemTime() - startTime) < 10) {

        ASSERT_NO_FATAL_FAILURE(fillAndPostBuffers(colors[colorIndex++ % colors.size()]));

        std::this_thread::sleep_for(1s);

    }



    ASSERT_EQ(OK, producer->disconnect(NATIVE_WINDOW_API_CPU));

}



} // namespace android