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

Commit 51b882c2 authored by Greg Kaiser's avatar Greg Kaiser
Browse files

Revert "Camera: Add lastCompletedFrameNumber in CaptureResultExtras" 

Revert "Camera: Fix race for onCaptureBufferLost callback"

Revert submission 11415576-bufferErrorLossFix

Reason for revert: Breaks Camera on at least the wembley device
Reverted Changes:
I12b716acc:Camera: Fix race for onCaptureBufferLost callback
I43f0f5ea1:Camera: Add lastCompletedFrameNumber in CaptureRes...

Bug: 158622719
Change-Id: I98440c52f61d571e1cb6692667fb067020746795
Test: Locally tried this revert on rvc-dev on a wembley, and now the Camera works
parent 730a7916

camera/CaptureResult.cpp

+0 −6
Original line number Diff line number Diff line
@@ -49,9 +49,6 @@ status_t CaptureResultExtras::readFromParcel(const android::Parcel *parcel) { 
        }

        errorPhysicalCameraId = cameraId;

    }

    parcel->readInt64(&lastCompletedRegularFrameNumber);

    parcel->readInt64(&lastCompletedReprocessFrameNumber);

    parcel->readInt64(&lastCompletedZslFrameNumber);



    return OK;

}
@@ -79,9 +76,6 @@ status_t CaptureResultExtras::writeToParcel(android::Parcel *parcel) const { 
    } else {

        parcel->writeBool(false);

    }

    parcel->writeInt64(lastCompletedRegularFrameNumber);

    parcel->writeInt64(lastCompletedReprocessFrameNumber);

    parcel->writeInt64(lastCompletedZslFrameNumber);



    return OK;

}

camera/include/camera/CaptureResult.h

+1 −32
Original line number Diff line number Diff line
@@ -76,34 +76,6 @@ struct CaptureResultExtras : public android::Parcelable { 
     */

    String16  errorPhysicalCameraId;



    // The last completed frame numbers shouldn't be checked in onResultReceived() and notifyError()

    // because the output buffers could be arriving after onResultReceived() and

    // notifyError(). Given this constraint, we check it for each

    // onCaptureStarted, and if there is no further onCaptureStarted(),

    // check for onDeviceIdle() to clear out all pending frame numbers.



    /**

     * The latest regular request frameNumber for which all buffers and capture result have been

     * returned or notified as an BUFFER_ERROR/RESULT_ERROR/REQUEST_ERROR. -1 if

     * none has completed.

     */

    int64_t lastCompletedRegularFrameNumber;



    /**

     * The latest reprocess request frameNumber for which all buffers and capture result have been

     * returned or notified as an BUFFER_ERROR/RESULT_ERROR/REQUEST_ERROR. -1 if

     * none has completed.

     */

    int64_t lastCompletedReprocessFrameNumber;



    /**

     * The latest Zsl request frameNumber for which all buffers and capture result have been

     * returned or notified as an BUFFER_ERROR/RESULT_ERROR/REQUEST_ERROR. -1 if

     * none has completed.

     */

    int64_t lastCompletedZslFrameNumber;





    /**

     * Constructor initializes object as invalid by setting requestId to be -1.

     */
@@ -115,10 +87,7 @@ struct CaptureResultExtras : public android::Parcelable { 
          frameNumber(0),

          partialResultCount(0),

          errorStreamId(-1),

          errorPhysicalCameraId(),

          lastCompletedRegularFrameNumber(-1),

          lastCompletedReprocessFrameNumber(-1),

          lastCompletedZslFrameNumber(-1) {

          errorPhysicalCameraId() {

    }



    /**

camera/ndk/impl/ACameraDevice.cpp

+35 −82
Original line number Diff line number Diff line
@@ -1336,19 +1336,19 @@ CameraDevice::checkRepeatingSequenceCompleteLocked( 
void

CameraDevice::checkAndFireSequenceCompleteLocked() {

    int64_t completedFrameNumber = mFrameNumberTracker.getCompletedFrameNumber();

    //std::map<int, int64_t> mSequenceLastFrameNumberMap;

    auto it = mSequenceLastFrameNumberMap.begin();

    while (it != mSequenceLastFrameNumberMap.end()) {

        int sequenceId = it->first;

        int64_t lastFrameNumber = it->second.lastFrameNumber;

        int64_t lastFrameNumber = it->second;

        bool seqCompleted = false;

        bool hasCallback  = true;



        if (mRemote == nullptr) {

            ALOGW("Camera %s closed while checking sequence complete", getId());

            return;

        }

        ALOGV("%s: seq %d's last frame number %" PRId64 ", completed %" PRId64,

                __FUNCTION__, sequenceId, lastFrameNumber, completedFrameNumber);

        if (!it->second.isSequenceCompleted) {



        // Check if there is callback for this sequence

        // This should not happen because we always register callback (with nullptr inside)

        if (mSequenceCallbackMap.count(sequenceId) == 0) {
@@ -1357,14 +1357,16 @@ CameraDevice::checkAndFireSequenceCompleteLocked() { 
        }



        if (lastFrameNumber <= completedFrameNumber) {

                ALOGV("Mark sequenceId %d as sequence completed", sequenceId);

                it->second.isSequenceCompleted = true;

            ALOGV("seq %d reached last frame %" PRId64 ", completed %" PRId64,

                    sequenceId, lastFrameNumber, completedFrameNumber);

            seqCompleted = true;

        }



            if (it->second.isSequenceCompleted && hasCallback) {

        if (seqCompleted && hasCallback) {

            // remove callback holder from callback map

            auto cbIt = mSequenceCallbackMap.find(sequenceId);

            CallbackHolder cbh = cbIt->second;



            mSequenceCallbackMap.erase(cbIt);

            // send seq complete callback

            sp<AMessage> msg = new AMessage(kWhatCaptureSeqEnd, mHandler);

            msg->setPointer(kContextKey, cbh.mContext);
@@ -1380,53 +1382,10 @@ CameraDevice::checkAndFireSequenceCompleteLocked() { 
            cbh.mSession.clear();

            postSessionMsgAndCleanup(msg);

        }

        }



        if (it->second.isSequenceCompleted && it->second.isInflightCompleted) {

            if (mSequenceCallbackMap.find(sequenceId) != mSequenceCallbackMap.end()) {

                mSequenceCallbackMap.erase(sequenceId);

            }

        // No need to track sequence complete if there is no callback registered

        if (seqCompleted || !hasCallback) {

            it = mSequenceLastFrameNumberMap.erase(it);

            ALOGV("%s: Remove holder for sequenceId %d", __FUNCTION__, sequenceId);

        } else {

            ++it;

        }

    }

}



void

CameraDevice::removeCompletedCallbackHolderLocked(int64_t lastCompletedRegularFrameNumber) {

    auto it = mSequenceLastFrameNumberMap.begin();

    while (it != mSequenceLastFrameNumberMap.end()) {

        int sequenceId = it->first;

        int64_t lastFrameNumber = it->second.lastFrameNumber;



        if (mRemote == nullptr) {

            ALOGW("Camera %s closed while checking sequence complete", getId());

            return;

        }



        ALOGV("%s: seq %d's last frame number %" PRId64

                ", completed inflight frame number %" PRId64,

                __FUNCTION__, sequenceId, lastFrameNumber,

                lastCompletedRegularFrameNumber);

        if (lastFrameNumber <= lastCompletedRegularFrameNumber) {

            if (it->second.isSequenceCompleted) {

                // Check if there is callback for this sequence

                // This should not happen because we always register callback (with nullptr inside)

                if (mSequenceCallbackMap.count(sequenceId) == 0) {

                    ALOGW("No callback found for sequenceId %d", sequenceId);

                } else {

                    mSequenceCallbackMap.erase(sequenceId);

                }



                it = mSequenceLastFrameNumberMap.erase(it);

                ALOGV("%s: Remove holder for sequenceId %d", __FUNCTION__, sequenceId);

            } else {

                ALOGV("Mark sequenceId %d as inflight completed", sequenceId);

                it->second.isInflightCompleted = true;

                ++it;

            }

        } else {

            ++it;

        }
@@ -1521,9 +1480,6 @@ CameraDevice::ServiceCallback::onDeviceIdle() { 
        return ret;

    }



    dev->removeCompletedCallbackHolderLocked(

             std::numeric_limits<int64_t>::max()/*lastCompletedRegularFrameNumber*/);



    if (dev->mIdle) {

        // Already in idle state. Possibly other thread did waitUntilIdle

        return ret;
@@ -1566,9 +1522,6 @@ CameraDevice::ServiceCallback::onCaptureStarted( 
        return ret;

    }



    dev->removeCompletedCallbackHolderLocked(

            resultExtras.lastCompletedRegularFrameNumber);



    int sequenceId = resultExtras.requestId;

    int32_t burstId = resultExtras.burstId;

camera/ndk/impl/ACameraDevice.h

+2 −10
Original line number Diff line number Diff line
@@ -267,15 +267,8 @@ class CameraDevice final : public RefBase { 
    static const int REQUEST_ID_NONE = -1;

    int mRepeatingSequenceId = REQUEST_ID_NONE;



    // sequence id -> last frame number holder map

    struct RequestLastFrameNumberHolder {

        int64_t lastFrameNumber;

        bool isSequenceCompleted = false;

        bool isInflightCompleted = false;

        RequestLastFrameNumberHolder(int64_t lastFN) :

                lastFrameNumber(lastFN) {}

    };

    std::map<int, RequestLastFrameNumberHolder> mSequenceLastFrameNumberMap;

    // sequence id -> last frame number map

    std::map<int, int64_t> mSequenceLastFrameNumberMap;



    struct CallbackHolder {

        CallbackHolder(sp<ACameraCaptureSession>          session,
@@ -345,7 +338,6 @@ class CameraDevice final : public RefBase { 


    void checkRepeatingSequenceCompleteLocked(const int sequenceId, const int64_t lastFrameNumber);

    void checkAndFireSequenceCompleteLocked();

    void removeCompletedCallbackHolderLocked(int64_t lastCompletedRegularFrameNumber);



    // Misc variables

    int32_t mShadingMapSize[2];   // const after constructor

services/camera/libcameraservice/device3/Camera3Device.cpp

+13 −19
Original line number Diff line number Diff line
@@ -756,10 +756,9 @@ status_t Camera3Device::dump(int fd, const Vector<String16> &args) { 
        for (size_t i = 0; i < mInFlightMap.size(); i++) {

            InFlightRequest r = mInFlightMap.valueAt(i);

            lines.appendFormat("      Frame %d |  Timestamp: %" PRId64 ", metadata"

                    " arrived: %s, buffers left: %d, buffers returned with STATUS_ERROR: %d, "

                    " buffers notified with error: %d\n", mInFlightMap.keyAt(i),

                    " arrived: %s, buffers left: %d\n", mInFlightMap.keyAt(i),

                    r.shutterTimestamp, r.haveResultMetadata ? "true" : "false",

                    r.numBuffersLeft, r.numErrorBuffersReturned, r.numErrorBuffersNotified);

                    r.numBuffersLeft);

        }

    }

    write(fd, lines.string(), lines.size());
@@ -1044,9 +1043,8 @@ hardware::Return<void> Camera3Device::processCaptureResult_3_4( 
    }

    CaptureOutputStates states {

        mId,

        mInFlightLock, mLastCompletedRegularFrameNumber,

        mLastCompletedReprocessFrameNumber, mLastCompletedZslFrameNumber,

        mInFlightMap, mOutputLock,  mResultQueue, mResultSignal,

        mInFlightLock, mInFlightMap,

        mOutputLock,  mResultQueue, mResultSignal,

        mNextShutterFrameNumber,

        mNextReprocessShutterFrameNumber, mNextZslStillShutterFrameNumber,

        mNextResultFrameNumber,
@@ -1102,9 +1100,8 @@ hardware::Return<void> Camera3Device::processCaptureResult( 


    CaptureOutputStates states {

        mId,

        mInFlightLock, mLastCompletedRegularFrameNumber,

        mLastCompletedReprocessFrameNumber, mLastCompletedZslFrameNumber,

        mInFlightMap, mOutputLock,  mResultQueue, mResultSignal,

        mInFlightLock, mInFlightMap,

        mOutputLock,  mResultQueue, mResultSignal,

        mNextShutterFrameNumber,

        mNextReprocessShutterFrameNumber, mNextZslStillShutterFrameNumber,

        mNextResultFrameNumber,
@@ -1142,9 +1139,8 @@ hardware::Return<void> Camera3Device::notify( 


    CaptureOutputStates states {

        mId,

        mInFlightLock, mLastCompletedRegularFrameNumber,

        mLastCompletedReprocessFrameNumber, mLastCompletedZslFrameNumber,

        mInFlightMap, mOutputLock,  mResultQueue, mResultSignal,

        mInFlightLock, mInFlightMap,

        mOutputLock,  mResultQueue, mResultSignal,

        mNextShutterFrameNumber,

        mNextReprocessShutterFrameNumber, mNextZslStillShutterFrameNumber,

        mNextResultFrameNumber,
@@ -5910,13 +5906,11 @@ status_t Camera3Device::switchToOffline( 
    //       though technically no other thread should be talking to Camera3Device at this point

    Camera3OfflineStates offlineStates(

            mTagMonitor, mVendorTagId, mUseHalBufManager, mNeedFixupMonochromeTags,

            mUsePartialResult, mNumPartialResults, mLastCompletedRegularFrameNumber,

            mLastCompletedReprocessFrameNumber, mLastCompletedZslFrameNumber,

            mNextResultFrameNumber, mNextReprocessResultFrameNumber,

            mNextZslStillResultFrameNumber, mNextShutterFrameNumber,

            mNextReprocessShutterFrameNumber, mNextZslStillShutterFrameNumber,

            mDeviceInfo, mPhysicalDeviceInfoMap, mDistortionMappers,

            mZoomRatioMappers, mRotateAndCropMappers);

            mUsePartialResult, mNumPartialResults, mNextResultFrameNumber,

            mNextReprocessResultFrameNumber, mNextZslStillResultFrameNumber,

            mNextShutterFrameNumber, mNextReprocessShutterFrameNumber,

            mNextZslStillShutterFrameNumber, mDeviceInfo, mPhysicalDeviceInfoMap,

            mDistortionMappers, mZoomRatioMappers, mRotateAndCropMappers);



    *session = new Camera3OfflineSession(mId, inputStream, offlineStreamSet,

            std::move(bufferRecords), offlineReqs, offlineStates, offlineSession);