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

Commit 9abe1ebc authored by Daniel Lam's avatar Daniel Lam
Browse files

Fixed disconnect bug in SurfaceTexture 

BufferQueue's disconnect could race with updateTexImage
where invalid buffers could be released.  Additionally
fixed similar bug with setBufferCount.  Tests were added
to stress the disconnect mechanism.

Change-Id: I9afa4c64f3e025984e8a9e8d924852a71d044716
parent 0e1080f8

include/gui/BufferQueue.h

+6 −1
Original line number Diff line number Diff line
@@ -42,6 +42,7 @@ public: 
    enum { NUM_BUFFER_SLOTS = 32 };

    enum { NO_CONNECTED_API = 0 };

    enum { INVALID_BUFFER_SLOT = -1 };

    enum { STALE_BUFFER_SLOT = 1 };



    // ConsumerListener is the interface through which the BufferQueue notifies

    // the consumer of events that the consumer may wish to react to.  Because
@@ -297,7 +298,8 @@ private: 
          mTimestamp(0),

          mFrameNumber(0),

          mFence(EGL_NO_SYNC_KHR),

          mAcquireCalled(false) {

          mAcquireCalled(false),

          mNeedsCleanupOnRelease(false) {

            mCrop.makeInvalid();

        }
@@ -376,6 +378,9 @@ private: 


        // Indicates whether this buffer has been seen by a consumer yet

        bool mAcquireCalled;



        // Indicates whether this buffer needs to be cleaned up by consumer

        bool mNeedsCleanupOnRelease;

    };



    // mSlots is the array of buffer slots that must be mirrored on the client

include/gui/SurfaceTexture.h

+0 −1
Original line number Diff line number Diff line
@@ -185,7 +185,6 @@ public: 
    status_t setConsumerUsageBits(uint32_t usage);

    status_t setTransformHint(uint32_t hint);

    virtual status_t setSynchronousMode(bool enabled);

    virtual status_t setBufferCount(int bufferCount);

    virtual status_t connect(int api,

                uint32_t* outWidth, uint32_t* outHeight, uint32_t* outTransform);

libs/gui/BufferQueue.cpp

+58 −41
Original line number Diff line number Diff line
@@ -163,6 +163,9 @@ status_t BufferQueue::setTransformHint(uint32_t hint) { 


status_t BufferQueue::setBufferCount(int bufferCount) {

    ST_LOGV("setBufferCount: count=%d", bufferCount);



    sp<ConsumerListener> listener;

    {

        Mutex::Autolock lock(mMutex);



        if (mAbandoned) {
@@ -205,6 +208,13 @@ status_t BufferQueue::setBufferCount(int bufferCount) { 
        mBufferHasBeenQueued = false;

        mQueue.clear();

        mDequeueCondition.broadcast();

        listener = mConsumerListener;

    } // scope for lock



    if (listener != NULL) {

        listener->onBuffersReleased();

    }



    return OK;

}
@@ -780,6 +790,9 @@ void BufferQueue::dump(String8& result, const char* prefix, 


void BufferQueue::freeBufferLocked(int i) {

    mSlots[i].mGraphicBuffer = 0;

    if (mSlots[i].mBufferState == BufferSlot::ACQUIRED) {

        mSlots[i].mNeedsCleanupOnRelease = true;

    }

    mSlots[i].mBufferState = BufferSlot::FREE;

    mSlots[i].mFrameNumber = 0;

    mSlots[i].mAcquireCalled = false;
@@ -853,15 +866,19 @@ status_t BufferQueue::releaseBuffer(int buf, EGLDisplay display, 
    mSlots[buf].mEglDisplay = display;

    mSlots[buf].mFence = fence;



    // The current buffer becomes FREE if it was still in the queued

    // state. If it has already been given to the client

    // (synchronous mode), then it stays in DEQUEUED state.

    if (mSlots[buf].mBufferState == BufferSlot::QUEUED

            || mSlots[buf].mBufferState == BufferSlot::ACQUIRED) {

    // The buffer can now only be released if its in the acquired state

    if (mSlots[buf].mBufferState == BufferSlot::ACQUIRED) {

        mSlots[buf].mBufferState = BufferSlot::FREE;

    } else if (mSlots[buf].mNeedsCleanupOnRelease) {

        ST_LOGV("releasing a stale buf %d its state was %d", buf, mSlots[buf].mBufferState);

        mSlots[buf].mNeedsCleanupOnRelease = false;

        return STALE_BUFFER_SLOT;

    } else {

        ST_LOGE("attempted to release buf %d but its state was %d", buf, mSlots[buf].mBufferState);

        return -EINVAL;

    }

    mDequeueCondition.broadcast();



    mDequeueCondition.broadcast();

    return OK;

}

libs/gui/SurfaceTexture.cpp

+15 −12
Original line number Diff line number Diff line
@@ -176,6 +176,8 @@ status_t SurfaceTexture::updateTexImage() { 
    ST_LOGV("updateTexImage");

    Mutex::Autolock lock(mMutex);



    status_t err = NO_ERROR;



    if (mAbandoned) {

        ST_LOGE("updateTexImage: SurfaceTexture is abandoned!");

        return NO_INIT;
@@ -269,11 +271,17 @@ status_t SurfaceTexture::updateTexImage() { 
                mCurrentTextureBuf != NULL ? mCurrentTextureBuf->handle : 0,

                buf, item.mGraphicBuffer != NULL ? item.mGraphicBuffer->handle : 0);



        // Release the old buffer

        // release old buffer

        if (mCurrentTexture != BufferQueue::INVALID_BUFFER_SLOT) {

            mBufferQueue->releaseBuffer(mCurrentTexture, dpy,

                    mEGLSlots[mCurrentTexture].mFence);

            status_t status = mBufferQueue->releaseBuffer(mCurrentTexture, dpy, mEGLSlots[mCurrentTexture].mFence);



            mEGLSlots[mCurrentTexture].mFence = EGL_NO_SYNC_KHR;

            if (status == BufferQueue::STALE_BUFFER_SLOT) {

                freeBufferLocked(mCurrentTexture);

            } else if (status != OK) {

                ST_LOGE("updateTexImage: released invalid buffer");

                err = status;

            }

        }



        // Update the SurfaceTexture state.
@@ -289,7 +297,7 @@ status_t SurfaceTexture::updateTexImage() { 
        glBindTexture(mTexTarget, mTexName);

    }



    return OK;

    return err;

}



status_t SurfaceTexture::detachFromContext() {
@@ -630,6 +638,9 @@ bool SurfaceTexture::isSynchronousMode() const { 
void SurfaceTexture::freeBufferLocked(int slotIndex) {

    ST_LOGV("freeBufferLocked: slotIndex=%d", slotIndex);

    mEGLSlots[slotIndex].mGraphicBuffer = 0;

    if (slotIndex == mCurrentTexture) {

        mCurrentTexture = BufferQueue::INVALID_BUFFER_SLOT;

    }

    if (mEGLSlots[slotIndex].mEglImage != EGL_NO_IMAGE_KHR) {

        EGLImageKHR img = mEGLSlots[slotIndex].mEglImage;

        if (img != EGL_NO_IMAGE_KHR) {
@@ -692,12 +703,6 @@ sp<BufferQueue> SurfaceTexture::getBufferQueue() const { 
    return mBufferQueue;

}



// Used for refactoring, should not be in final interface

status_t SurfaceTexture::setBufferCount(int bufferCount) {

    Mutex::Autolock lock(mMutex);

    return mBufferQueue->setBufferCount(bufferCount);

}



// Used for refactoring, should not be in final interface

status_t SurfaceTexture::connect(int api,

                uint32_t* outWidth, uint32_t* outHeight, uint32_t* outTransform) {
@@ -737,8 +742,6 @@ void SurfaceTexture::onBuffersReleased() { 
            freeBufferLocked(i);

        }

    }



    mCurrentTexture = BufferQueue::INVALID_BUFFER_SLOT;

}



void SurfaceTexture::dump(String8& result) const

libs/gui/tests/SurfaceTexture_test.cpp

+146 −1
Original line number Diff line number Diff line
@@ -486,6 +486,55 @@ protected: 
        Condition mCondition;

    };



    // Note that SurfaceTexture will lose the notifications

    // onBuffersReleased and onFrameAvailable as there is currently

    // no way to forward the events.  This DisconnectWaiter will not let the

    // disconnect finish until finishDisconnect() is called.  It will

    // also block until a disconnect is called

    class DisconnectWaiter : public BufferQueue::ConsumerListener {

    public:

        DisconnectWaiter () :

            mWaitForDisconnect(false),

            mPendingFrames(0) {

        }



        void waitForFrame() {

            Mutex::Autolock lock(mMutex);

            while (mPendingFrames == 0) {

                mFrameCondition.wait(mMutex);

            }

            mPendingFrames--;

        }



        virtual void onFrameAvailable() {

            Mutex::Autolock lock(mMutex);

            mPendingFrames++;

            mFrameCondition.signal();

        }



        virtual void onBuffersReleased() {

            Mutex::Autolock lock(mMutex);

            while (!mWaitForDisconnect) {

                mDisconnectCondition.wait(mMutex);

            }

        }



        void finishDisconnect() {

            Mutex::Autolock lock(mMutex);

            mWaitForDisconnect = true;

            mDisconnectCondition.signal();

        }



    private:

        Mutex mMutex;



        bool mWaitForDisconnect;

        Condition mDisconnectCondition;



        int mPendingFrames;

        Condition mFrameCondition;

    };



    sp<SurfaceTexture> mST;

    sp<SurfaceTextureClient> mSTC;

    sp<ANativeWindow> mANW;
@@ -984,6 +1033,102 @@ TEST_F(SurfaceTextureGLTest, TexturingFromCpuFilledRGBABufferPow2) { 
    EXPECT_TRUE(checkPixel( 3, 52,  35, 231,  35,  35));

}



// Tests if SurfaceTexture and BufferQueue are robust enough

// to handle a special case where updateTexImage is called

// in the middle of disconnect.  This ordering is enforced

// by blocking in the disconnect callback.

TEST_F(SurfaceTextureGLTest, DisconnectStressTest) {



    class ProducerThread : public Thread {

    public:

        ProducerThread(const sp<ANativeWindow>& anw):

                mANW(anw) {

        }



        virtual ~ProducerThread() {

        }



        virtual bool threadLoop() {

            ANativeWindowBuffer* anb;



            native_window_api_connect(mANW.get(), NATIVE_WINDOW_API_EGL);



            for (int numFrames =0 ; numFrames < 2; numFrames ++) {



                if (mANW->dequeueBuffer(mANW.get(), &anb) != NO_ERROR) {

                    return false;

                }

                if (anb == NULL) {

                    return false;

                }

                if (mANW->queueBuffer(mANW.get(), anb)

                        != NO_ERROR) {

                    return false;

                }

            }



            native_window_api_disconnect(mANW.get(), NATIVE_WINDOW_API_EGL);



            return false;

        }



    private:

        sp<ANativeWindow> mANW;

    };



    ASSERT_EQ(OK, mST->setSynchronousMode(true));



    sp<DisconnectWaiter> dw(new DisconnectWaiter());

    mST->getBufferQueue()->consumerConnect(dw);





    sp<Thread> pt(new ProducerThread(mANW));

    pt->run();



    // eat a frame so SurfaceTexture will own an at least one slot

    dw->waitForFrame();

    EXPECT_EQ(OK,mST->updateTexImage());



    dw->waitForFrame();

    // Could fail here as SurfaceTexture thinks it still owns the slot

    // but bufferQueue has released all slots

    EXPECT_EQ(OK,mST->updateTexImage());



    dw->finishDisconnect();

}





// This test ensures that the SurfaceTexture clears the mCurrentTexture

// when it is disconnected and reconnected.  Otherwise it will

// attempt to release a buffer that it does not owned

TEST_F(SurfaceTextureGLTest, DisconnectClearsCurrentTexture) {

    ASSERT_EQ(OK, mST->setSynchronousMode(true));



    native_window_api_connect(mANW.get(), NATIVE_WINDOW_API_EGL);



    ANativeWindowBuffer *anb;



    EXPECT_EQ (OK, mANW->dequeueBuffer(mANW.get(), &anb));

    EXPECT_EQ(OK, mANW->queueBuffer(mANW.get(), anb));



    EXPECT_EQ (OK, mANW->dequeueBuffer(mANW.get(), &anb));

    EXPECT_EQ(OK, mANW->queueBuffer(mANW.get(), anb));



    EXPECT_EQ(OK,mST->updateTexImage());

    EXPECT_EQ(OK,mST->updateTexImage());



    native_window_api_disconnect(mANW.get(), NATIVE_WINDOW_API_EGL);

    native_window_api_connect(mANW.get(), NATIVE_WINDOW_API_EGL);



    ASSERT_EQ(OK, mST->setSynchronousMode(true));



    EXPECT_EQ (OK, mANW->dequeueBuffer(mANW.get(), &anb));

    EXPECT_EQ(OK, mANW->queueBuffer(mANW.get(), anb));



    // Will fail here if mCurrentTexture is not cleared properly

    EXPECT_EQ(OK,mST->updateTexImage());

}



TEST_F(SurfaceTextureGLTest, AbandonUnblocksDequeueBuffer) {

    class ProducerThread : public Thread {

    public: