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

Commit 3d316f37 authored by Shuzhen Wang's avatar Shuzhen Wang
Browse files

Camera: Fix missing physical camera availability callback 

The missing physical camera availability callback is due to a race
condition between provider enumeration and physical camera availability
callback. If the HAL triggers a physical camera availability callback,
it could be dropped because the logical camera's availability callback
isn't triggered until the provider enumeration finishes.

Fix the issue by deferring physical camera availability callbacks
until the logical camera availability callback is called.

Test: Camera CTS, vendor test on foldable phone, cameraservice_test
Bug: 255472536
Change-Id: Ia5a535b1530b3666e616cc3619fcb759ff5d8560
parent 25c68079

services/camera/libcameraservice/CameraService.cpp

+9 −2
Original line number Diff line number Diff line
@@ -203,6 +203,7 @@ status_t CameraService::enumerateProviders() { 
    status_t res;



    std::vector<std::string> deviceIds;

    std::unordered_map<std::string, std::set<std::string>> unavailPhysicalIds;

    {

        Mutex::Autolock l(mServiceLock);
@@ -233,7 +234,7 @@ status_t CameraService::enumerateProviders() { 
            ALOGE("Failed to enumerate flash units: %s (%d)", strerror(-res), res);

        }



        deviceIds = mCameraProviderManager->getCameraDeviceIds();

        deviceIds = mCameraProviderManager->getCameraDeviceIds(&unavailPhysicalIds);

    }
@@ -242,6 +243,12 @@ status_t CameraService::enumerateProviders() { 
        if (getCameraState(id8) == nullptr) {

            onDeviceStatusChanged(id8, CameraDeviceStatus::PRESENT);

        }

        if (unavailPhysicalIds.count(cameraId) > 0) {

            for (const auto& physicalId : unavailPhysicalIds[cameraId]) {

                String8 physicalId8 = String8(physicalId.c_str());

                onDeviceStatusChanged(id8, physicalId8, CameraDeviceStatus::NOT_PRESENT);

            }

        }

    }



    // Derive primary rear/front cameras, and filter their charactierstics.
@@ -495,7 +502,7 @@ void CameraService::onDeviceStatusChanged(const String8& id, 


    if (state == nullptr) {

        ALOGE("%s: Physical camera id %s status change on a non-present ID %s",

                __FUNCTION__, id.string(), physicalId.string());

                __FUNCTION__, physicalId.string(), id.string());

        return;

    }

services/camera/libcameraservice/common/CameraProviderManager.cpp

+18 −39
Original line number Diff line number Diff line
@@ -197,12 +197,17 @@ std::pair<int, int> CameraProviderManager::getCameraCount() const { 
    return std::make_pair(systemCameraCount, publicCameraCount);

}



std::vector<std::string> CameraProviderManager::getCameraDeviceIds() const {

std::vector<std::string> CameraProviderManager::getCameraDeviceIds(std::unordered_map<

            std::string, std::set<std::string>>* unavailablePhysicalIds) const {

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

    std::vector<std::string> deviceIds;

    for (auto& provider : mProviders) {

        for (auto& id : provider->mUniqueCameraIds) {

            deviceIds.push_back(id);

            if (unavailablePhysicalIds != nullptr &&

                    provider->mUnavailablePhysicalCameras.count(id) > 0) {

                (*unavailablePhysicalIds)[id] = provider->mUnavailablePhysicalCameras.at(id);

            }

        }

    }

    return deviceIds;
@@ -839,9 +844,6 @@ status_t CameraProviderManager::dump(int fd, const Vector<String16>& args) { 


void CameraProviderManager::ProviderInfo::initializeProviderInfoCommon(

        const std::vector<std::string> &devices) {



    sp<StatusListener> listener = mManager->getStatusListener();



    for (auto& device : devices) {

        std::string id;

        status_t res = addDevice(device, CameraDeviceStatus::PRESENT, &id);
@@ -856,38 +858,22 @@ void CameraProviderManager::ProviderInfo::initializeProviderInfoCommon( 
            mProviderName.c_str(), mDevices.size());



    // Process cached status callbacks

    std::unique_ptr<std::vector<CameraStatusInfoT>> cachedStatus =

            std::make_unique<std::vector<CameraStatusInfoT>>();

    {

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



        for (auto& statusInfo : mCachedStatus) {

            std::string id, physicalId;

            status_t res = OK;

            if (statusInfo.isPhysicalCameraStatus) {

                res = physicalCameraDeviceStatusChangeLocked(&id, &physicalId,

                physicalCameraDeviceStatusChangeLocked(&id, &physicalId,

                    statusInfo.cameraId, statusInfo.physicalCameraId, statusInfo.status);

            } else {

                res = cameraDeviceStatusChangeLocked(&id, statusInfo.cameraId, statusInfo.status);

            }

            if (res == OK) {

                cachedStatus->emplace_back(statusInfo.isPhysicalCameraStatus,

                        id.c_str(), physicalId.c_str(), statusInfo.status);

                cameraDeviceStatusChangeLocked(&id, statusInfo.cameraId, statusInfo.status);

            }

        }

        mCachedStatus.clear();



        mInitialized = true;

    }



    // The cached status change callbacks cannot be fired directly from this

    // function, due to same-thread deadlock trying to acquire mInterfaceMutex

    // twice.

    if (listener != nullptr) {

        mInitialStatusCallbackFuture = std::async(std::launch::async,

                &CameraProviderManager::ProviderInfo::notifyInitialStatusChange, this,

                listener, std::move(cachedStatus));

    }

}



CameraProviderManager::ProviderInfo::DeviceInfo* CameraProviderManager::findDeviceInfoLocked(
@@ -1957,6 +1943,7 @@ void CameraProviderManager::ProviderInfo::removeDevice(std::string id) { 
    for (auto it = mDevices.begin(); it != mDevices.end(); it++) {

        if ((*it)->mId == id) {

            mUniqueCameraIds.erase(id);

            mUnavailablePhysicalCameras.erase(id);

            if ((*it)->isAPI1Compatible()) {

                mUniqueAPI1CompatibleCameraIds.erase(std::remove(

                    mUniqueAPI1CompatibleCameraIds.begin(),
@@ -2224,6 +2211,15 @@ status_t CameraProviderManager::ProviderInfo::physicalCameraDeviceStatusChangeLo 
        return BAD_VALUE;

    }



    if (mUnavailablePhysicalCameras.count(cameraId) == 0) {

        mUnavailablePhysicalCameras.emplace(cameraId, std::set<std::string>{});

    }

    if (newStatus != CameraDeviceStatus::PRESENT) {

        mUnavailablePhysicalCameras[cameraId].insert(physicalCameraDeviceName);

    } else {

        mUnavailablePhysicalCameras[cameraId].erase(physicalCameraDeviceName);

    }



    *id = cameraId;

    *physicalId = physicalCameraDeviceName.c_str();

    return OK;
@@ -2282,20 +2278,6 @@ void CameraProviderManager::ProviderInfo::notifyDeviceInfoStateChangeLocked( 
    }

}



void CameraProviderManager::ProviderInfo::notifyInitialStatusChange(

        sp<StatusListener> listener,

        std::unique_ptr<std::vector<CameraStatusInfoT>> cachedStatus) {

    for (auto& statusInfo : *cachedStatus) {

        if (statusInfo.isPhysicalCameraStatus) {

            listener->onDeviceStatusChanged(String8(statusInfo.cameraId.c_str()),

                    String8(statusInfo.physicalCameraId.c_str()), statusInfo.status);

        } else {

            listener->onDeviceStatusChanged(

                    String8(statusInfo.cameraId.c_str()), statusInfo.status);

        }

    }

}



CameraProviderManager::ProviderInfo::DeviceInfo3::DeviceInfo3(const std::string& name,

        const metadata_vendor_id_t tagId, const std::string &id,

        uint16_t minorVersion,
@@ -2645,9 +2627,6 @@ status_t CameraProviderManager::ProviderInfo::parseDeviceName(const std::string& 
}



CameraProviderManager::ProviderInfo::~ProviderInfo() {

    if (mInitialStatusCallbackFuture.valid()) {

        mInitialStatusCallbackFuture.wait();

    }

    // Destruction of ProviderInfo is only supposed to happen when the respective

    // CameraProvider interface dies, so do not unregister callbacks.

}

services/camera/libcameraservice/common/CameraProviderManager.h

+9 −7
Original line number Diff line number Diff line
@@ -23,7 +23,6 @@ 
#include <set>

#include <string>

#include <mutex>

#include <future>



#include <camera/camera2/ConcurrentCamera.h>

#include <camera/CameraParameters2.h>
@@ -220,7 +219,14 @@ public: 
     */

    std::pair<int, int> getCameraCount() const;



    std::vector<std::string> getCameraDeviceIds() const;

    /**

     * Upon the function return, if unavailablePhysicalIds is not nullptr, it

     * will contain all of the unavailable physical camera Ids represented in

     * the form of:

     * {[logicalCamera, {physicalCamera1, physicalCamera2, ...}], ...}.

     */

    std::vector<std::string> getCameraDeviceIds(std::unordered_map<

            std::string, std::set<std::string>>* unavailablePhysicalIds = nullptr) const;



    /**

     * Retrieve the number of API1 compatible cameras; these are internal and
@@ -607,6 +613,7 @@ private: 
        };

        std::vector<std::unique_ptr<DeviceInfo>> mDevices;

        std::unordered_set<std::string> mUniqueCameraIds;

        std::unordered_map<std::string, std::set<std::string>> mUnavailablePhysicalCameras;

        int mUniqueDeviceCount;

        std::vector<std::string> mUniqueAPI1CompatibleCameraIds;

        // The initial public camera IDs published by the camera provider.
@@ -715,8 +722,6 @@ private: 
        std::vector<CameraStatusInfoT> mCachedStatus;

        // End of scope for mInitLock



        std::future<void> mInitialStatusCallbackFuture;



        std::unique_ptr<ProviderInfo::DeviceInfo>

        virtual initializeDeviceInfo(

                const std::string &name, const metadata_vendor_id_t tagId,
@@ -724,9 +729,6 @@ private: 


        virtual status_t reCacheConcurrentStreamingCameraIdsLocked() = 0;



        void notifyInitialStatusChange(sp<StatusListener> listener,

                std::unique_ptr<std::vector<CameraStatusInfoT>> cachedStatus);



        std::vector<std::unordered_set<std::string>> mConcurrentCameraIdCombinations;



        // Parse provider instance name for type and id

services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp

+84 −3
Original line number Diff line number Diff line
@@ -102,23 +102,57 @@ struct TestICameraProvider : virtual public provider::V2_5::ICameraProvider { 
    sp<device::V3_2::ICameraDevice> mDeviceInterface;

    hardware::hidl_vec<common::V1_0::VendorTagSection> mVendorTagSections;



    // Whether to call a physical camera unavailable callback upon setCallback

    bool mHasPhysicalCameraUnavailableCallback;

    hardware::hidl_string mLogicalCameraId;

    hardware::hidl_string mUnavailablePhysicalCameraId;



    TestICameraProvider(const std::vector<hardware::hidl_string> &devices,

            const hardware::hidl_vec<common::V1_0::VendorTagSection> &vendorSection) :

        mDeviceNames(devices),

        mDeviceInterface(new TestDeviceInterface(devices)),

        mVendorTagSections (vendorSection) {}

        mVendorTagSections (vendorSection),

        mHasPhysicalCameraUnavailableCallback(false) {}



    TestICameraProvider(const std::vector<hardware::hidl_string> &devices,

            const hardware::hidl_vec<common::V1_0::VendorTagSection> &vendorSection,

            android::hardware::hidl_vec<uint8_t> chars) :

        mDeviceNames(devices),

        mDeviceInterface(new TestDeviceInterface(devices, chars)),

        mVendorTagSections (vendorSection) {}

        mVendorTagSections (vendorSection),

        mHasPhysicalCameraUnavailableCallback(false) {}



    TestICameraProvider(const std::vector<hardware::hidl_string> &devices,

            const hardware::hidl_vec<common::V1_0::VendorTagSection> &vendorSection,

            android::hardware::hidl_vec<uint8_t> chars,

            const hardware::hidl_string& logicalCameraId,

            const hardware::hidl_string& unavailablePhysicalCameraId) :

        mDeviceNames(devices),

        mDeviceInterface(new TestDeviceInterface(devices, chars)),

        mVendorTagSections (vendorSection),

        mHasPhysicalCameraUnavailableCallback(true),

        mLogicalCameraId(logicalCameraId),

        mUnavailablePhysicalCameraId(unavailablePhysicalCameraId) {}



    virtual hardware::Return<Status> setCallback(

            const sp<provider::V2_4::ICameraProviderCallback>& callbacks) override {

        mCalledCounter[SET_CALLBACK]++;

        mCallbacks = callbacks;

        if (mHasPhysicalCameraUnavailableCallback) {

            auto cast26 = provider::V2_6::ICameraProviderCallback::castFrom(callbacks);

            if (!cast26.isOk()) {

                ADD_FAILURE() << "Failed to cast ICameraProviderCallback to V2_6";

            } else {

                sp<provider::V2_6::ICameraProviderCallback> callback26 = cast26;

                if (callback26 == nullptr) {

                    ADD_FAILURE() << "V2_6::ICameraProviderCallback is null after conversion";

                } else {

                    callback26->physicalCameraDeviceStatusChange(mLogicalCameraId,

                            mUnavailablePhysicalCameraId,

                            android::hardware::camera::common::V1_0::CameraDeviceStatus::NOT_PRESENT);

                }

            }

        }

        return hardware::Return<Status>(Status::OK);

    }
@@ -266,12 +300,16 @@ struct TestInteractionProxy : public CameraProviderManager::HidlServiceInteracti 
};



struct TestStatusListener : public CameraProviderManager::StatusListener {

    int mPhysicalCameraStatusChangeCount = 0;



    ~TestStatusListener() {}



    void onDeviceStatusChanged(const String8 &,

            CameraDeviceStatus) override {}

    void onDeviceStatusChanged(const String8 &, const String8 &,

            CameraDeviceStatus) override {}

            CameraDeviceStatus) override {

        mPhysicalCameraStatusChangeCount++;

    }

    void onTorchStatusChanged(const String8 &,

            TorchModeStatus) override {}

    void onTorchStatusChanged(const String8 &,
@@ -634,3 +672,46 @@ TEST(CameraProviderManagerTest, BinderDeathRegistrationRaceTest) { 
    ASSERT_EQ(deviceCount, deviceNames.size()) <<

            "Unexpected amount of camera devices";

}



// Test that CameraProviderManager does not trigger

// onDeviceStatusChanged(NOT_PRESENT) for physical camera before initialize()

// returns.

TEST(CameraProviderManagerTest, PhysicalCameraAvailabilityCallbackRaceTest) {

    std::vector<hardware::hidl_string> deviceNames;

    deviceNames.push_back("device@3.2/test/0");

    hardware::hidl_vec<common::V1_0::VendorTagSection> vendorSection;



    sp<CameraProviderManager> providerManager = new CameraProviderManager();

    sp<TestStatusListener> statusListener = new TestStatusListener();

    TestInteractionProxy serviceProxy;



    android::hardware::hidl_vec<uint8_t> chars;

    CameraMetadata meta;

    int32_t charKeys[] = { ANDROID_REQUEST_AVAILABLE_CAPABILITIES };

    meta.update(ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS, charKeys,

            sizeof(charKeys) / sizeof(charKeys[0]));

    uint8_t capabilities[] = { ANDROID_REQUEST_AVAILABLE_CAPABILITIES_LOGICAL_MULTI_CAMERA };

    meta.update(ANDROID_REQUEST_AVAILABLE_CAPABILITIES, capabilities,

            sizeof(capabilities)/sizeof(capabilities[0]));

    uint8_t physicalCameraIds[] = { '2', '\0', '3', '\0' };

    meta.update(ANDROID_LOGICAL_MULTI_CAMERA_PHYSICAL_IDS, physicalCameraIds,

            sizeof(physicalCameraIds)/sizeof(physicalCameraIds[0]));

    camera_metadata_t* metaBuffer = const_cast<camera_metadata_t*>(meta.getAndLock());

    chars.setToExternal(reinterpret_cast<uint8_t*>(metaBuffer),

            get_camera_metadata_size(metaBuffer));



    sp<TestICameraProvider> provider = new TestICameraProvider(deviceNames,

            vendorSection, chars, "device@3.2/test/0", "2");

    serviceProxy.setProvider(provider);



    status_t res = providerManager->initialize(statusListener, &serviceProxy);

    ASSERT_EQ(res, OK) << "Unable to initialize provider manager";



    ASSERT_EQ(statusListener->mPhysicalCameraStatusChangeCount, 0)

            << "Unexpected physical camera status change callback upon provider init.";



    std::unordered_map<std::string, std::set<std::string>> unavailablePhysicalIds;

    auto cameraIds = providerManager->getCameraDeviceIds(&unavailablePhysicalIds);

    ASSERT_TRUE(unavailablePhysicalIds.count("0") > 0 && unavailablePhysicalIds["0"].count("2") > 0)

        << "Unavailable physical camera Ids not set properly.";

}