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

Commit da470061 authored by Patrick Williams's avatar Patrick Williams
Browse files

Add TransactionHandle to cleanup Transaction resources 

TransactionHandle introduces a reliable mechanism for cleaning up resources even if the corresponding Transaction has been parceled and/or sent to another process. Transaction callbacks are removed from TransactionCompletedListener when the last reference to a TransactionHandle is dropped. If the transaction is applied, the TransactionHandles registered for its callbacks are passed to SurfaceFlinger and then back to the client process, keeping them alive until after the callbacks are called. If the transaction is dropped without getting applied, uncalled callbacks and corresponding SurfaceControl references are removed from TransactionCompletedListener.

Bug: 261757446
Flag: com.android.graphics.libgui.flags.unapplied_transaction_cleanup
Test: atest libgui_test --test-filter='TransactionCallbackTest*'
Change-Id: I5099ec819e31f4b83e7069ff3fb117d8d9708d1e
parent be3f3316

libs/gui/ITransactionCompletedListener.cpp

+9 −11
Original line number Diff line number Diff line
@@ -182,6 +182,10 @@ status_t TransactionStats::writeToParcel(Parcel* output) const { 
    if (err != NO_ERROR) {

        return err;

    }

    err = output->writeStrongBinderVector(transactionHandles);

    if (err != NO_ERROR) {

        return err;

    }

    err = output->writeInt64(latchTime);

    if (err != NO_ERROR) {

        return err;
@@ -206,6 +210,10 @@ status_t TransactionStats::readFromParcel(const Parcel* input) { 
    if (err != NO_ERROR) {

        return err;

    }

    err = input->readStrongBinderVector(&transactionHandles);

    if (err != NO_ERROR) {

        return err;

    }

    err = input->readInt64(&latchTime);

    if (err != NO_ERROR) {

        return err;
@@ -253,16 +261,6 @@ status_t ListenerStats::readFromParcel(const Parcel* input) { 
    return NO_ERROR;

}



ListenerStats ListenerStats::createEmpty(

        const sp<IBinder>& listener,

        const std::unordered_set<CallbackId, CallbackIdHash>& callbackIds) {

    ListenerStats listenerStats;

    listenerStats.listener = listener;

    listenerStats.transactionStats.emplace_back(callbackIds);



    return listenerStats;

}



class BpTransactionCompletedListener : public SafeBpInterface<ITransactionCompletedListener> {

public:

    explicit BpTransactionCompletedListener(const sp<IBinder>& impl)
@@ -332,7 +330,7 @@ ListenerCallbacks ListenerCallbacks::filter(CallbackId::Type type) const { 
            filteredCallbackIds.push_back(callbackId);

        }

    }

    return ListenerCallbacks(transactionCompletedListener, filteredCallbackIds);

    return ListenerCallbacks(transactionCompletedListener, filteredCallbackIds, transactionHandles);

}



status_t CallbackId::writeToParcel(Parcel* output) const {

libs/gui/LayerState.cpp

+5 −1
Original line number Diff line number Diff line
@@ -180,6 +180,7 @@ status_t layer_state_t::write(Parcel& output) const 
    for (auto listener : listeners) {

        SAFE_PARCEL(output.writeStrongBinder, listener.transactionCompletedListener);

        SAFE_PARCEL(output.writeParcelableVector, listener.callbackIds);

        SAFE_PARCEL(output.writeStrongBinderVector, listener.transactionHandles);

    }

    SAFE_PARCEL(output.writeFloat, shadowRadius);

    SAFE_PARCEL(output.writeParcelable, borderSettings);
@@ -328,9 +329,12 @@ status_t layer_state_t::read(const Parcel& input) 
    for (int i = 0; i < numListeners; i++) {

        sp<IBinder> listener;

        std::vector<CallbackId> callbackIds;

        std::vector<sp<IBinder>> transactionHandles;

        SAFE_PARCEL(input.readNullableStrongBinder, &listener);

        SAFE_PARCEL(input.readParcelableVector, &callbackIds);

        listeners.emplace_back(listener, callbackIds);

        SAFE_PARCEL(input.readStrongBinderVector, &transactionHandles);

        listeners.emplace_back(std::move(listener), std::move(callbackIds),

                               std::move(transactionHandles));

    }

    SAFE_PARCEL(input.readFloat, &shadowRadius);

    SAFE_PARCEL(input.readParcelable, &borderSettings);

libs/gui/SurfaceComposerClient.cpp

+84 −39
Original line number Diff line number Diff line
@@ -419,25 +419,28 @@ CallbackId TransactionCompletedListener::addCallbackFunction( 
        const TransactionCompletedCallback& callbackFunction,

        const std::unordered_set<sp<SurfaceControl>, SurfaceComposerClient::SCHash>&

                surfaceControls,

        CallbackId::Type callbackType) {

        CallbackId::Type callbackType, const wp<IBinder>& transactionHandle) {

    std::lock_guard<std::mutex> lock(mMutex);

    startListeningLocked();



    CallbackId callbackId(getNextIdLocked(), callbackType);

    mCallbacks[callbackId].callbackFunction = callbackFunction;

    auto& callbackSurfaceControls = mCallbacks[callbackId].surfaceControls;



    CallbackTranslation& callbackTranslation = mCallbacks[callbackId];

    callbackTranslation.callbackFunction = callbackFunction;

    callbackTranslation.transactionHandle = transactionHandle;

    for (const auto& surfaceControl : surfaceControls) {

        callbackSurfaceControls[surfaceControl->getHandle()] = surfaceControl;

        callbackTranslation.surfaceControls.emplace(surfaceControl->getHandle(), surfaceControl);

    }



    return callbackId;

}



void TransactionCompletedListener::setReleaseBufferCallback(const ReleaseCallbackId& callbackId,

                                                            ReleaseBufferCallback listener) {

                                                            ReleaseBufferCallback listener,

                                                            const wp<IBinder>& transactionHandle) {

    std::scoped_lock<std::mutex> lock(mMutex);

    mReleaseBufferCallbacks[callbackId] = listener;

    ReleaseCallbackTranslation& callbackTranslation = mReleaseBufferCallbacks[callbackId];

    callbackTranslation.callbackFunction = std::move(listener);

    callbackTranslation.transactionHandle = transactionHandle;

}



void TransactionCompletedListener::addSurfaceStatsListener(void* context, void* cookie,
@@ -502,7 +505,7 @@ void TransactionCompletedListener::onTransactionCompleted(ListenerStats listener 
            if (callbackId.type != CallbackId::Type::ON_COMMIT) {

                continue;

            }

            auto& [callbackFunction, callbackSurfaceControls] = callbacksMap[callbackId];

            auto& [callbackFunction, _, callbackSurfaceControls] = callbacksMap[callbackId];

            if (!callbackFunction) {

                continue;

            }
@@ -532,7 +535,7 @@ void TransactionCompletedListener::onTransactionCompleted(ListenerStats listener 
            if (callbackId.type != CallbackId::Type::ON_COMPLETE) {

                continue;

            }

            auto& [callbackFunction, callbackSurfaceControls] = callbacksMap[callbackId];

            auto& [callbackFunction, _, callbackSurfaceControls] = callbacksMap[callbackId];

            if (!callbackFunction) {

                ALOGE("cannot call null callback function, skipping");

                continue;
@@ -662,7 +665,7 @@ ReleaseBufferCallback TransactionCompletedListener::popReleaseBufferCallbackLock 
    if (itr == mReleaseBufferCallbacks.end()) {

        return nullptr;

    }

    callback = itr->second;

    callback = std::move(itr->second.callbackFunction);

    mReleaseBufferCallbacks.erase(itr);

    return callback;

}
@@ -714,6 +717,26 @@ void TransactionCompletedListener::onTrustedPresentationChanged(int id, 
    tpc(context, presentedWithinThresholds);

}



void TransactionCompletedListener::onTransactionDestroyed(const wp<IBinder>& transactionHandle) {

    std::lock_guard lock{mMutex};



    for (auto it = mCallbacks.begin(); it != mCallbacks.end();) {

        if (it->second.transactionHandle == transactionHandle) {

            it = mCallbacks.erase(it);

        } else {

            it++;

        }

    }



    for (auto it = mReleaseBufferCallbacks.begin(); it != mReleaseBufferCallbacks.end();) {

        if (it->second.transactionHandle == transactionHandle) {

            it = mReleaseBufferCallbacks.erase(it);

        } else {

            it++;

        }

    }

}



// ---------------------------------------------------------------------------



void removeDeadBufferCallback(void* /*context*/, uint64_t graphicBufferId);
@@ -829,12 +852,12 @@ SurfaceComposerClient::Transaction::Transaction() { 
}



SurfaceComposerClient::Transaction::Transaction(const Transaction& other)

      : mState(other.mState),

      : mTransactionCompletedListener(TransactionCompletedListener::getInstance()),

        mState(other.mState),

        mListenerCallbacks(other.mListenerCallbacks),

        mTransactionHandle(other.mTransactionHandle),

        mMayContainBuffer(other.mMayContainBuffer),

        mApplyToken(other.mApplyToken) {

    mListenerCallbacks = other.mListenerCallbacks;

    mTransactionCompletedListener = TransactionCompletedListener::getInstance();

}

        mApplyToken(other.mApplyToken) {}



void SurfaceComposerClient::Transaction::sanitize(int pid, int uid) {

    // TODO(b/356936695) move to TransactionState.
@@ -874,6 +897,7 @@ status_t SurfaceComposerClient::Transaction::readFromParcel(const Parcel* parcel 
    for (size_t i = 0; i < count; i++) {

        sp<ITransactionCompletedListener> listener =

                interface_cast<ITransactionCompletedListener>(parcel->readStrongBinder());

        CallbackInfo& callbackInfo = listenerCallbacks[listener];

        size_t numCallbackIds = parcel->readUint32();

        if (numCallbackIds > parcel->dataSize()) {

            return BAD_VALUE;
@@ -881,7 +905,7 @@ status_t SurfaceComposerClient::Transaction::readFromParcel(const Parcel* parcel 
        for (size_t j = 0; j < numCallbackIds; j++) {

            CallbackId id;

            parcel->readParcelable(&id);

            listenerCallbacks[listener].callbackIds.insert(id);

            callbackInfo.callbackIds.insert(id);

        }

        size_t numSurfaces = parcel->readUint32();

        if (numSurfaces > parcel->dataSize()) {
@@ -890,13 +914,22 @@ status_t SurfaceComposerClient::Transaction::readFromParcel(const Parcel* parcel 
        for (size_t j = 0; j < numSurfaces; j++) {

            sp<SurfaceControl> surface;

            SAFE_PARCEL(SurfaceControl::readFromParcel, *parcel, &surface);

            listenerCallbacks[listener].surfaceControls.insert(surface);

            callbackInfo.surfaceControls.insert(surface);

        }

        size_t numHandles = parcel->readUint32();

        if (numHandles > parcel->dataSize()) {

            return BAD_VALUE;

        }

        for (size_t j = 0; j < numHandles; j++) {

            sp<IBinder> transactionHandle;

            SAFE_PARCEL(parcel->readStrongBinder, &transactionHandle);

            callbackInfo.transactionHandles.insert(std::move(transactionHandle));

        }

    }



    // Parsing was successful. Update the object.

    mState = std::move(state);

    mListenerCallbacks = listenerCallbacks;

    mListenerCallbacks = std::move(listenerCallbacks);

    mApplyToken = applyToken;

    return NO_ERROR;

}
@@ -930,6 +963,10 @@ status_t SurfaceComposerClient::Transaction::writeToParcel(Parcel* parcel) const 
        for (auto surfaceControl : callbackInfo.surfaceControls) {

            SAFE_PARCEL(surfaceControl->writeToParcel, *parcel);

        }

        parcel->writeUint32(static_cast<uint32_t>(callbackInfo.transactionHandles.size()));

        for (auto& transactionHandle : callbackInfo.transactionHandles) {

            SAFE_PARCEL(parcel->writeStrongBinder, transactionHandle);

        }

    }



    return NO_ERROR;
@@ -957,13 +994,14 @@ void SurfaceComposerClient::Transaction::releaseBufferIfOverwriting(const layer_ 


SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::merge(Transaction&& other) {

    for (const auto& [listener, callbackInfo] : other.mListenerCallbacks) {

        auto& [callbackIds, surfaceControls] = callbackInfo;

        mListenerCallbacks[listener].callbackIds.insert(std::make_move_iterator(

                                                                callbackIds.begin()),

                                                        std::make_move_iterator(callbackIds.end()));

        auto& [callbackIds, surfaceControls, transactionHandles] = callbackInfo;



        mListenerCallbacks[listener].surfaceControls.insert(surfaceControls.begin(),

                                                            surfaceControls.end());

        auto& mergedCallbackInfo = mListenerCallbacks[listener];

        mergedCallbackInfo.callbackIds.insert(callbackIds.begin(), callbackIds.end());

        mergedCallbackInfo.surfaceControls.insert(surfaceControls.begin(), surfaceControls.end());

        mergedCallbackInfo.transactionHandles

                .insert(std::make_move_iterator(transactionHandles.begin()),

                        std::make_move_iterator(transactionHandles.end()));



        auto& currentProcessCallbackInfo =

                mListenerCallbacks[TransactionCompletedListener::getIInstance()];
@@ -1000,6 +1038,7 @@ void SurfaceComposerClient::Transaction::clear() { 
    mState.clear();

    mState.mId = generateId();

    mListenerCallbacks.clear();

    mTransactionHandle.clear();

    mMayContainBuffer = false;

    mApplyToken = nullptr;

    mLogCallPoints = false;
@@ -1137,15 +1176,14 @@ status_t SurfaceComposerClient::Transaction::apply(bool synchronous, bool oneWay 
    listenerCallbacks.mHasListenerCallbacks = !mListenerCallbacks.empty();

    // For every listener with registered callbacks

    for (const auto& [listener, callbackInfo] : mListenerCallbacks) {

        auto& [callbackIds, surfaceControls] = callbackInfo;

        auto& [callbackIds, surfaceControls, transactionHandles] = callbackInfo;

        if (callbackIds.empty()) {

            continue;

        }



        if (surfaceControls.empty()) {

            listenerCallbacks.mFlattenedListenerCallbacks.emplace_back(IInterface::asBinder(

                                                                               listener),

                                                                       std::move(callbackIds));

            listenerCallbacks.mFlattenedListenerCallbacks

                    .emplace_back(IInterface::asBinder(listener), callbackIds, transactionHandles);

        } else {

            // If the listener has any SurfaceControls set on this Transaction update the surface

            // state
@@ -1156,8 +1194,11 @@ status_t SurfaceComposerClient::Transaction::apply(bool synchronous, bool oneWay 
                    continue;

                }

                std::vector<CallbackId> callbacks(callbackIds.begin(), callbackIds.end());

                std::vector<sp<IBinder>> handles(transactionHandles.begin(),

                                                 transactionHandles.end());

                s->what |= layer_state_t::eHasListenerCallbacksChanged;

                s->listeners.emplace_back(IInterface::asBinder(listener), callbacks);

                s->listeners.emplace_back(IInterface::asBinder(listener), std::move(callbacks),

                                          std::move(handles));

            }

        }

    }
@@ -1694,14 +1735,8 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setBuffe 
    s->bufferData = std::move(bufferData);

    registerSurfaceControlForCallback(sc);



    // With the current infrastructure, a release callback will not be invoked if there's no

    // transaction callback in the case when a buffer is latched and not released early. This is

    // because the legacy implementation didn't have a release callback and sent releases in the

    // transaction callback. Because of this, we need to make sure to have a transaction callback

    // set up when a buffer is sent in a transaction to ensure the caller gets the release

    // callback, regardless if they set up a transaction callback.

    //

    // TODO (b/230380821): Remove when release callbacks are separated from transaction callbacks

    // Ensure there's a transaction completed callback registered so that buffers can be released

    // via ITransactionCompletedListener::onTransactionCompleted.

    addTransactionCompletedCallback([](void*, nsecs_t, const sp<Fence>&,

                                       const std::vector<SurfaceControlStats>&) {},

                                    nullptr);
@@ -1744,7 +1779,7 @@ void SurfaceComposerClient::Transaction::setReleaseBufferCallback(BufferData* bu 
    bufferData->releaseBufferListener =

            static_cast<sp<ITransactionCompletedListener>>(mTransactionCompletedListener);

    mTransactionCompletedListener->setReleaseBufferCallback(bufferData->generateReleaseCallbackId(),

                                                            callback);

                                                            callback, getTransactionHandle());

}



SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::setDataspace(
@@ -1917,6 +1952,11 @@ SurfaceComposerClient::Transaction::setFrameRateSelectionPriority(const sp<Surfa 
    return *this;

}



SurfaceComposerClient::TransactionHandle::~TransactionHandle() {

    TransactionCompletedListener::getInstance()->onTransactionDestroyed(

            wp<TransactionHandle>::fromExisting(this));

}



SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::addTransactionCallback(

        TransactionCompletedCallbackTakesContext callback, void* callbackContext,

        CallbackId::Type callbackType) {
@@ -1927,9 +1967,14 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::addTrans 


    CallbackId callbackId =

            mTransactionCompletedListener->addCallbackFunction(callbackWithContext, surfaceControls,

                                                               callbackType);

                                                               callbackType,

                                                               getTransactionHandle());



    mListenerCallbacks[mTransactionCompletedListener].callbackIds.emplace(callbackId);

    if (libgui_flags::unapplied_transaction_cleanup()) {

        mListenerCallbacks[mTransactionCompletedListener].transactionHandles.emplace(

                getTransactionHandle());

    }

    return *this;

}

libs/gui/TransactionState.cpp

+6 −2
Original line number Diff line number Diff line
@@ -30,9 +30,10 @@ void TransactionListenerCallbacks::clear() { 
status_t TransactionListenerCallbacks::writeToParcel(Parcel* parcel) const {

    SAFE_PARCEL(parcel->writeBool, mHasListenerCallbacks);

    SAFE_PARCEL(parcel->writeUint32, static_cast<uint32_t>(mFlattenedListenerCallbacks.size()));

    for (const auto& [listener, callbackIds] : mFlattenedListenerCallbacks) {

    for (const auto& [listener, callbackIds, transactionHandles] : mFlattenedListenerCallbacks) {

        SAFE_PARCEL(parcel->writeStrongBinder, listener);

        SAFE_PARCEL(parcel->writeParcelableVector, callbackIds);

        SAFE_PARCEL(parcel->writeStrongBinderVector, transactionHandles);

    }



    return NO_ERROR;
@@ -49,7 +50,10 @@ status_t TransactionListenerCallbacks::readFromParcel(const Parcel* parcel) { 
        SAFE_PARCEL(parcel->readStrongBinder, &tmpBinder);

        std::vector<CallbackId> callbackIds;

        SAFE_PARCEL(parcel->readParcelableVector, &callbackIds);

        mFlattenedListenerCallbacks.emplace_back(tmpBinder, callbackIds);

        std::vector<sp<IBinder>> transactionHandles;

        SAFE_PARCEL(parcel->readStrongBinderVector, &transactionHandles);

        mFlattenedListenerCallbacks.emplace_back(std::move(tmpBinder), std::move(callbackIds),

                                                 std::move(transactionHandles));

    }



    return NO_ERROR;

libs/gui/include/gui/ITransactionCompletedListener.h

+21 −11
Original line number Diff line number Diff line
@@ -22,6 +22,7 @@ 
#include <binder/SafeInterface.h>



#include <gui/FrameTimestamps.h>

#include <gui/SpHash.h>

#include <ui/Fence.h>

#include <utils/Timers.h>
@@ -32,6 +33,8 @@ 


namespace android {



using gui::SpHash;



class ITransactionCompletedListener;

class ListenerCallbacks;
@@ -139,14 +142,19 @@ public: 
    status_t readFromParcel(const Parcel* input) override;



    TransactionStats() = default;

    TransactionStats(const std::vector<CallbackId>& ids) : callbackIds(ids) {}

    TransactionStats(const std::unordered_set<CallbackId, CallbackIdHash>& ids)

          : callbackIds(ids.begin(), ids.end()) {}

    TransactionStats(const std::vector<CallbackId>& ids,

                     const std::vector<sp<IBinder>>& transactionHandles)

          : callbackIds(ids), transactionHandles(transactionHandles) {}

    TransactionStats(const std::unordered_set<CallbackId, CallbackIdHash>& ids,

                     const std::unordered_set<sp<IBinder>, SpHash<IBinder>>& transactionHandles)

          : callbackIds(ids.begin(), ids.end()),

            transactionHandles(transactionHandles.begin(), transactionHandles.end()) {}

    TransactionStats(const std::vector<CallbackId>& ids, nsecs_t latch, const sp<Fence>& present,

                     const std::vector<SurfaceStats>& surfaces)

          : callbackIds(ids), latchTime(latch), presentFence(present), surfaceStats(surfaces) {}



    std::vector<CallbackId> callbackIds;

    std::vector<sp<IBinder>> transactionHandles;

    nsecs_t latchTime = -1;

    sp<Fence> presentFence = nullptr;

    std::vector<SurfaceStats> surfaceStats;
@@ -157,10 +165,6 @@ public: 
    status_t writeToParcel(Parcel* output) const override;

    status_t readFromParcel(const Parcel* input) override;



    static ListenerStats createEmpty(

            const sp<IBinder>& listener,

            const std::unordered_set<CallbackId, CallbackIdHash>& callbackIds);



    sp<IBinder> listener;

    std::vector<TransactionStats> transactionStats;

};
@@ -191,12 +195,17 @@ public: 
class ListenerCallbacks {

public:

    ListenerCallbacks(const sp<IBinder>& listener,

                      const std::unordered_set<CallbackId, CallbackIdHash>& callbacks)

                      const std::unordered_set<CallbackId, CallbackIdHash>& callbacks,

                      const std::unordered_set<sp<IBinder>, SpHash<IBinder>>& transactionHandles)

          : transactionCompletedListener(listener),

            callbackIds(callbacks.begin(), callbacks.end()) {}

            callbackIds(callbacks.begin(), callbacks.end()),

            transactionHandles(transactionHandles.begin(), transactionHandles.end()) {}



    ListenerCallbacks(const sp<IBinder>& listener, const std::vector<CallbackId>& ids)

          : transactionCompletedListener(listener), callbackIds(ids) {}

    ListenerCallbacks(sp<IBinder> listener, std::vector<CallbackId> ids,

                      std::vector<sp<IBinder>> transactionHandles)

          : transactionCompletedListener(std::move(listener)),

            callbackIds(std::move(ids)),

            transactionHandles(std::move(transactionHandles)) {}



    bool operator==(const ListenerCallbacks& rhs) const {

        if (transactionCompletedListener != rhs.transactionCompletedListener) {
@@ -213,6 +222,7 @@ public: 


    sp<IBinder> transactionCompletedListener;

    std::vector<CallbackId> callbackIds;

    std::vector<sp<IBinder>> transactionHandles;

};



struct IListenerHash {