From 63f3611018857754d60a176783133371bae5f009 Mon Sep 17 00:00:00 2001 From: Eino-Ville Talvala Date: Thu, 6 Dec 2018 14:57:03 -0800 Subject: [PATCH] Camera service: Support for device state change notifications Add AIDL method for device-wide physical state updates (such as folding/unfolding), and wire it up to the new camera provider HAL call. Also clean up camera provider startup sequence - devices were being enumerated before the new lazy HAL interface pointers were set up, resulting in many repeated calls to getService even for a non-lazy HAL. Also add unit test for CameraProviderManager to verify its section of the callpath, as well as tests to check that the provider HAL and hardware service manager aren't queried more than expected during initialization. Test: atest cameraservice_test Change-Id: I5ec60fd9d93b7a2fe4d1a5854fad720a972fe8ea --- .../aidl/android/hardware/ICameraService.aidl | 24 +++- camera/cameraserver/Android.bp | 1 + services/camera/libcameraservice/Android.bp | 1 + .../camera/libcameraservice/CameraService.cpp | 43 +++++++ .../camera/libcameraservice/CameraService.h | 2 + .../common/CameraProviderManager.cpp | 119 +++++++++++++++--- .../common/CameraProviderManager.h | 27 +++- .../camera/libcameraservice/tests/Android.mk | 1 + .../tests/CameraProviderManagerTest.cpp | 73 ++++++++++- 9 files changed, 266 insertions(+), 25 deletions(-) diff --git a/camera/aidl/android/hardware/ICameraService.aidl b/camera/aidl/android/hardware/ICameraService.aidl index c03831457f..0e969c7662 100644 --- a/camera/aidl/android/hardware/ICameraService.aidl +++ b/camera/aidl/android/hardware/ICameraService.aidl @@ -162,6 +162,28 @@ interface ICameraService * Callers require the android.permission.CAMERA_SEND_SYSTEM_EVENTS permission. */ const int EVENT_NONE = 0; - const int EVENT_USER_SWITCHED = 1; + const int EVENT_USER_SWITCHED = 1; // The argument is the set of new foreground user IDs. oneway void notifySystemEvent(int eventId, in int[] args); + + /** + * Notify the camera service of a device physical status change. May only be called from + * a privileged process. + * + * newState is a bitfield consisting of DEVICE_STATE_* values combined together. Valid state + * combinations are device-specific. At device startup, the camera service will assume the device + * state is NORMAL until otherwise notified. + * + * Callers require the android.permission.CAMERA_SEND_SYSTEM_EVENTS permission. + */ + oneway void notifyDeviceStateChange(long newState); + + // Bitfield constants for notifyDeviceStateChange + // All bits >= 32 are for custom vendor states + // Written as ints since AIDL does not support long constants. + const int DEVICE_STATE_NORMAL = 0; + const int DEVICE_STATE_BACK_COVERED = 1; + const int DEVICE_STATE_FRONT_COVERED = 2; + const int DEVICE_STATE_FOLDED = 4; + const int DEVICE_STATE_LAST_FRAMEWORK_BIT = 0x80000000; // 1 << 31; + } diff --git a/camera/cameraserver/Android.bp b/camera/cameraserver/Android.bp index b88a2c5a70..92b06c2d10 100644 --- a/camera/cameraserver/Android.bp +++ b/camera/cameraserver/Android.bp @@ -27,6 +27,7 @@ cc_binary { "libhidltransport", "android.hardware.camera.common@1.0", "android.hardware.camera.provider@2.4", + "android.hardware.camera.provider@2.5", "android.hardware.camera.device@1.0", "android.hardware.camera.device@3.2", "android.hardware.camera.device@3.4", diff --git a/services/camera/libcameraservice/Android.bp b/services/camera/libcameraservice/Android.bp index a090479183..38c87c721d 100644 --- a/services/camera/libcameraservice/Android.bp +++ b/services/camera/libcameraservice/Android.bp @@ -93,6 +93,7 @@ cc_library_shared { "android.frameworks.cameraservice.device@2.0", "android.hardware.camera.common@1.0", "android.hardware.camera.provider@2.4", + "android.hardware.camera.provider@2.5", "android.hardware.camera.device@1.0", "android.hardware.camera.device@3.2", "android.hardware.camera.device@3.3", diff --git a/services/camera/libcameraservice/CameraService.cpp b/services/camera/libcameraservice/CameraService.cpp index c3113bf6ea..0b34b3b9f2 100644 --- a/services/camera/libcameraservice/CameraService.cpp +++ b/services/camera/libcameraservice/CameraService.cpp @@ -1632,6 +1632,49 @@ Status CameraService::notifySystemEvent(int32_t eventId, return Status::ok(); } +Status CameraService::notifyDeviceStateChange(int64_t newState) { + const int pid = CameraThreadState::getCallingPid(); + const int selfPid = getpid(); + + // Permission checks + if (pid != selfPid) { + // Ensure we're being called by system_server, or similar process with + // permissions to notify the camera service about system events + if (!checkCallingPermission( + String16("android.permission.CAMERA_SEND_SYSTEM_EVENTS"))) { + const int uid = CameraThreadState::getCallingUid(); + ALOGE("Permission Denial: cannot send updates to camera service about device" + " state changes from pid=%d, uid=%d", pid, uid); + return STATUS_ERROR_FMT(ERROR_PERMISSION_DENIED, + "No permission to send updates to camera service about device state" + " changes from pid=%d, uid=%d", pid, uid); + } + } + + ATRACE_CALL(); + + using hardware::camera::provider::V2_5::DeviceState; + hardware::hidl_bitfield newDeviceState{}; + if (newState & ICameraService::DEVICE_STATE_BACK_COVERED) { + newDeviceState |= DeviceState::BACK_COVERED; + } + if (newState & ICameraService::DEVICE_STATE_FRONT_COVERED) { + newDeviceState |= DeviceState::FRONT_COVERED; + } + if (newState & ICameraService::DEVICE_STATE_FOLDED) { + newDeviceState |= DeviceState::FOLDED; + } + // Only map vendor bits directly + uint64_t vendorBits = static_cast(newState) & 0xFFFFFFFF00000000l; + newDeviceState |= vendorBits; + + ALOGV("%s: New device state 0x%" PRIx64, __FUNCTION__, newDeviceState); + Mutex::Autolock l(mServiceLock); + mCameraProviderManager->notifyDeviceStateChange(newDeviceState); + + return Status::ok(); +} + Status CameraService::addListener(const sp& listener, /*out*/ std::vector *cameraStatuses) { diff --git a/services/camera/libcameraservice/CameraService.h b/services/camera/libcameraservice/CameraService.h index a2961986b2..f351e7c8b7 100644 --- a/services/camera/libcameraservice/CameraService.h +++ b/services/camera/libcameraservice/CameraService.h @@ -154,6 +154,8 @@ public: virtual binder::Status notifySystemEvent(int32_t eventId, const std::vector& args); + virtual binder::Status notifyDeviceStateChange(int64_t newState); + // OK = supports api of that version, -EOPNOTSUPP = does not support virtual binder::Status supportsCameraApi( const String16& cameraId, int32_t apiVersion, diff --git a/services/camera/libcameraservice/common/CameraProviderManager.cpp b/services/camera/libcameraservice/common/CameraProviderManager.cpp index 3059b0737d..137ffea66f 100644 --- a/services/camera/libcameraservice/common/CameraProviderManager.cpp +++ b/services/camera/libcameraservice/common/CameraProviderManager.cpp @@ -77,6 +77,8 @@ status_t CameraProviderManager::initialize(wp>( + provider::V2_5::DeviceState::NORMAL); // Registering will trigger notifications for all already-known providers bool success = mServiceProxy->registerForNotifications( @@ -280,6 +282,26 @@ status_t CameraProviderManager::setUpVendorTags() { return OK; } +status_t CameraProviderManager::notifyDeviceStateChange( + hardware::hidl_bitfield newState) { + std::lock_guard lock(mInterfaceMutex); + mDeviceState = newState; + status_t res = OK; + for (auto& provider : mProviders) { + ALOGV("%s: Notifying %s for new state 0x%" PRIx64, + __FUNCTION__, provider->mProviderName.c_str(), newState); + status_t singleRes = provider->notifyDeviceStateChange(mDeviceState); + if (singleRes != OK) { + ALOGE("%s: Unable to notify provider %s about device state change", + __FUNCTION__, + provider->mProviderName.c_str()); + res = singleRes; + // continue to do the rest of the providers instead of returning now + } + } + return res; +} + status_t CameraProviderManager::openSession(const std::string &id, const sp& callback, /*out*/ @@ -365,7 +387,7 @@ void CameraProviderManager::saveRef(DeviceMode usageType, const std::string &cam if (!kEnableLazyHal) { return; } - ALOGI("Saving camera provider %s for camera device %s", provider->descriptor, cameraId.c_str()); + ALOGV("Saving camera provider %s for camera device %s", provider->descriptor, cameraId.c_str()); std::lock_guard lock(mProviderInterfaceMapLock); std::unordered_map> *primaryMap, *alternateMap; if (usageType == DeviceMode::TORCH) { @@ -389,7 +411,7 @@ void CameraProviderManager::removeRef(DeviceMode usageType, const std::string &c if (!kEnableLazyHal) { return; } - ALOGI("Removing camera device %s", cameraId.c_str()); + ALOGV("Removing camera device %s", cameraId.c_str()); std::unordered_map> *providerMap; if (usageType == DeviceMode::TORCH) { providerMap = &mTorchProviderByCameraId; @@ -947,7 +969,7 @@ status_t CameraProviderManager::addProviderLocked(const std::string& newProvider } sp providerInfo = new ProviderInfo(newProvider, this); - status_t res = providerInfo->initialize(interface); + status_t res = providerInfo->initialize(interface, mDeviceState); if (res != OK) { return res; } @@ -1008,7 +1030,8 @@ CameraProviderManager::ProviderInfo::ProviderInfo( } status_t CameraProviderManager::ProviderInfo::initialize( - sp& interface) { + sp& interface, + hardware::hidl_bitfield currentDeviceState) { status_t res = parseProviderName(mProviderName, &mType, &mId); if (res != OK) { ALOGE("%s: Invalid provider name, ignoring", __FUNCTION__); @@ -1016,6 +1039,15 @@ status_t CameraProviderManager::ProviderInfo::initialize( } ALOGI("Connecting to new camera provider: %s, isRemote? %d", mProviderName.c_str(), interface->isRemote()); + + // Determine minor version + auto castResult = provider::V2_5::ICameraProvider::castFrom(interface); + if (castResult.isOk()) { + mMinorVersion = 5; + } else { + mMinorVersion = 4; + } + // cameraDeviceStatusChange callbacks may be called (and causing new devices added) // before setCallback returns hardware::Return status = interface->setCallback(this); @@ -1040,6 +1072,24 @@ status_t CameraProviderManager::ProviderInfo::initialize( __FUNCTION__, mProviderName.c_str()); } + if (!kEnableLazyHal) { + // Save HAL reference indefinitely + mSavedInterface = interface; + } else { + mActiveInterface = interface; + } + + ALOGV("%s: Setting device state for %s: 0x%" PRIx64, + __FUNCTION__, mProviderName.c_str(), mDeviceState); + notifyDeviceStateChange(currentDeviceState); + + res = setUpVendorTags(); + if (res != OK) { + ALOGE("%s: Unable to set up vendor tags from provider '%s'", + __FUNCTION__, mProviderName.c_str()); + return res; + } + // Get initial list of camera devices, if any std::vector devices; hardware::Return ret = interface->getCameraIdList([&status, this, &devices]( @@ -1096,34 +1146,28 @@ status_t CameraProviderManager::ProviderInfo::initialize( } } - res = setUpVendorTags(); - if (res != OK) { - ALOGE("%s: Unable to set up vendor tags from provider '%s'", - __FUNCTION__, mProviderName.c_str()); - return res; - } - ALOGI("Camera provider %s ready with %zu camera devices", mProviderName.c_str(), mDevices.size()); mInitialized = true; - if (!kEnableLazyHal) { - // Save HAL reference indefinitely - mSavedInterface = interface; - } return OK; } const sp CameraProviderManager::ProviderInfo::startProviderInterface() { ATRACE_CALL(); - ALOGI("Request to start camera provider: %s", mProviderName.c_str()); + ALOGV("Request to start camera provider: %s", mProviderName.c_str()); if (mSavedInterface != nullptr) { return mSavedInterface; } + if (!kEnableLazyHal) { + ALOGE("Bad provider state! Should not be here on a non-lazy HAL!"); + return nullptr; + } + auto interface = mActiveInterface.promote(); if (interface == nullptr) { - ALOGI("Could not promote, calling getService(%s)", mProviderName.c_str()); + ALOGI("Camera HAL provider needs restart, calling getService(%s)", mProviderName.c_str()); interface = mManager->mServiceProxy->getService(mProviderName); interface->setCallback(this); hardware::Return linked = interface->linkToDeath(this, /*cookie*/ mId); @@ -1136,9 +1180,22 @@ CameraProviderManager::ProviderInfo::startProviderInterface() { ALOGW("%s: Unable to link to provider '%s' death notifications", __FUNCTION__, mProviderName.c_str()); } + // Send current device state + if (mMinorVersion >= 5) { + auto castResult = provider::V2_5::ICameraProvider::castFrom(interface); + if (castResult.isOk()) { + sp interface_2_5 = castResult; + if (interface_2_5 != nullptr) { + ALOGV("%s: Initial device state for %s: 0x %" PRIx64, + __FUNCTION__, mProviderName.c_str(), mDeviceState); + interface_2_5->notifyDeviceStateChange(mDeviceState); + } + } + } + mActiveInterface = interface; } else { - ALOGI("Camera provider (%s) already in use. Re-using instance.", mProviderName.c_str()); + ALOGV("Camera provider (%s) already in use. Re-using instance.", mProviderName.c_str()); } return interface; } @@ -1223,8 +1280,10 @@ void CameraProviderManager::ProviderInfo::removeDevice(std::string id) { } status_t CameraProviderManager::ProviderInfo::dump(int fd, const Vector&) const { - dprintf(fd, "== Camera Provider HAL %s (v2.4, %s) static info: %zu devices: ==\n", - mProviderName.c_str(), mIsRemote ? "remote" : "passthrough", + dprintf(fd, "== Camera Provider HAL %s (v2.%d, %s) static info: %zu devices: ==\n", + mProviderName.c_str(), + mMinorVersion, + mIsRemote ? "remote" : "passthrough", mDevices.size()); for (auto& device : mDevices) { @@ -1423,6 +1482,26 @@ status_t CameraProviderManager::ProviderInfo::setUpVendorTags() { return OK; } +status_t CameraProviderManager::ProviderInfo::notifyDeviceStateChange( + hardware::hidl_bitfield newDeviceState) { + mDeviceState = newDeviceState; + if (mMinorVersion >= 5) { + // Check if the provider is currently active - not going to start it up for this notification + auto interface = mSavedInterface != nullptr ? mSavedInterface : mActiveInterface.promote(); + if (interface != nullptr) { + // Send current device state + auto castResult = provider::V2_5::ICameraProvider::castFrom(interface); + if (castResult.isOk()) { + sp interface_2_5 = castResult; + if (interface_2_5 != nullptr) { + interface_2_5->notifyDeviceStateChange(mDeviceState); + } + } + } + } + return OK; +} + template std::unique_ptr CameraProviderManager::ProviderInfo::initializeDeviceInfo( diff --git a/services/camera/libcameraservice/common/CameraProviderManager.h b/services/camera/libcameraservice/common/CameraProviderManager.h index fbd7d2e8f8..78007ff74f 100644 --- a/services/camera/libcameraservice/common/CameraProviderManager.h +++ b/services/camera/libcameraservice/common/CameraProviderManager.h @@ -28,9 +28,8 @@ #include #include #include -#include +#include #include -//#include #include #include @@ -205,6 +204,12 @@ public: */ status_t setUpVendorTags(); + /** + * Inform registered providers about a device state change, such as folding or unfolding + */ + status_t notifyDeviceStateChange( + android::hardware::hidl_bitfield newState); + /** * Open an active session to a camera device. * @@ -276,6 +281,9 @@ private: wp mListener; ServiceInteractionProxy* mServiceProxy; + // Current overall Android device physical status + android::hardware::hidl_bitfield mDeviceState; + // mProviderLifecycleLock is locked during onRegistration and removeProvider mutable std::mutex mProviderLifecycleLock; @@ -302,10 +310,14 @@ private: { const std::string mProviderName; const metadata_vendor_id_t mProviderTagid; + int mMinorVersion; sp mVendorTagDescriptor; bool mSetTorchModeSupported; bool mIsRemote; + // Current overall Android device physical status + hardware::hidl_bitfield mDeviceState; + // This pointer is used to keep a reference to the ICameraProvider that was last accessed. wp mActiveInterface; @@ -315,7 +327,9 @@ private: CameraProviderManager *manager); ~ProviderInfo(); - status_t initialize(sp& interface); + status_t initialize(sp& interface, + hardware::hidl_bitfield + currentDeviceState); const sp startProviderInterface(); @@ -344,6 +358,13 @@ private: */ status_t setUpVendorTags(); + /** + * Notify provider about top-level device physical state changes + */ + status_t notifyDeviceStateChange( + hardware::hidl_bitfield + newDeviceState); + // Basic device information, common to all camera devices struct DeviceInfo { const std::string mName; // Full instance name diff --git a/services/camera/libcameraservice/tests/Android.mk b/services/camera/libcameraservice/tests/Android.mk index ad9963afc1..d777ca1cb1 100644 --- a/services/camera/libcameraservice/tests/Android.mk +++ b/services/camera/libcameraservice/tests/Android.mk @@ -29,6 +29,7 @@ LOCAL_SHARED_LIBRARIES := \ libutils \ android.hardware.camera.common@1.0 \ android.hardware.camera.provider@2.4 \ + android.hardware.camera.provider@2.5 \ android.hardware.camera.device@1.0 \ android.hardware.camera.device@3.2 \ android.hardware.camera.device@3.4 diff --git a/services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp b/services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp index 0086c6c5de..f47e5a581e 100644 --- a/services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp +++ b/services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp @@ -33,6 +33,7 @@ using android::hardware::camera::common::V1_0::VendorTagSection; using android::hardware::camera::common::V1_0::CameraMetadataType; using android::hardware::camera::device::V3_2::ICameraDeviceCallback; using android::hardware::camera::device::V3_2::ICameraDeviceSession; +using android::hardware::camera::provider::V2_5::DeviceState; /** * Basic test implementation of a camera ver. 3.2 device interface @@ -87,7 +88,7 @@ struct TestDeviceInterface : public device::V3_2::ICameraDevice { /** * Basic test implementation of a camera provider */ -struct TestICameraProvider : virtual public provider::V2_4::ICameraProvider { +struct TestICameraProvider : virtual public provider::V2_5::ICameraProvider { sp mCallbacks; std::vector mDeviceNames; sp mDeviceInterface; @@ -101,6 +102,7 @@ struct TestICameraProvider : virtual public provider::V2_4::ICameraProvider { virtual hardware::Return setCallback( const sp& callbacks) override { + mCalledCounter[SET_CALLBACK]++; mCallbacks = callbacks; return hardware::Return(Status::OK); } @@ -108,6 +110,7 @@ struct TestICameraProvider : virtual public provider::V2_4::ICameraProvider { using getVendorTags_cb = std::function& sections)>; hardware::Return getVendorTags(getVendorTags_cb _hidl_cb) override { + mCalledCounter[GET_VENDOR_TAGS]++; _hidl_cb(Status::OK, mVendorTagSections); return hardware::Void(); } @@ -117,6 +120,7 @@ struct TestICameraProvider : virtual public provider::V2_4::ICameraProvider { bool support)>; virtual ::hardware::Return isSetTorchModeSupported( isSetTorchModeSupported_cb _hidl_cb) override { + mCalledCounter[IS_SET_TORCH_MODE_SUPPORTED]++; _hidl_cb(Status::OK, false); return hardware::Void(); } @@ -124,6 +128,7 @@ struct TestICameraProvider : virtual public provider::V2_4::ICameraProvider { using getCameraIdList_cb = std::function& cameraDeviceNames)>; virtual hardware::Return getCameraIdList(getCameraIdList_cb _hidl_cb) override { + mCalledCounter[GET_CAMERA_ID_LIST]++; _hidl_cb(Status::OK, mDeviceNames); return hardware::Void(); } @@ -148,6 +153,25 @@ struct TestICameraProvider : virtual public provider::V2_4::ICameraProvider { return hardware::Void(); } + virtual hardware::Return notifyDeviceStateChange( + hardware::hidl_bitfield newState) override { + mCalledCounter[NOTIFY_DEVICE_STATE]++; + mCurrentState = newState; + return hardware::Void(); + } + + enum MethodNames { + SET_CALLBACK, + GET_VENDOR_TAGS, + IS_SET_TORCH_MODE_SUPPORTED, + NOTIFY_DEVICE_STATE, + GET_CAMERA_ID_LIST, + + METHOD_NAME_COUNT + }; + int mCalledCounter[METHOD_NAME_COUNT] {0}; + + hardware::hidl_bitfield mCurrentState = 0xFFFFFFFF; // Unlikely to be a real state }; /** @@ -209,11 +233,26 @@ TEST(CameraProviderManagerTest, InitializeTest) { res = providerManager->initialize(statusListener, &serviceProxy); ASSERT_EQ(res, OK) << "Unable to initialize provider manager"; + // Check that both "legacy" and "external" providers (really the same object) are called + // once for all the init methods + EXPECT_EQ(provider->mCalledCounter[TestICameraProvider::SET_CALLBACK], 2) << + "Only one call to setCallback per provider expected during init"; + EXPECT_EQ(provider->mCalledCounter[TestICameraProvider::GET_VENDOR_TAGS], 2) << + "Only one call to getVendorTags per provider expected during init"; + EXPECT_EQ(provider->mCalledCounter[TestICameraProvider::IS_SET_TORCH_MODE_SUPPORTED], 2) << + "Only one call to isSetTorchModeSupported per provider expected during init"; + EXPECT_EQ(provider->mCalledCounter[TestICameraProvider::GET_CAMERA_ID_LIST], 2) << + "Only one call to getCameraIdList per provider expected during init"; + EXPECT_EQ(provider->mCalledCounter[TestICameraProvider::NOTIFY_DEVICE_STATE], 2) << + "Only one call to notifyDeviceState per provider expected during init"; std::string legacyInstanceName = "legacy/0"; std::string externalInstanceName = "external/0"; bool gotLegacy = false; bool gotExternal = false; + EXPECT_EQ(2u, serviceProxy.mLastRequestedServiceNames.size()) << + "Only two service queries expected to be seen by hardware service manager"; + for (auto& serviceName : serviceProxy.mLastRequestedServiceNames) { if (serviceName == legacyInstanceName) gotLegacy = true; if (serviceName == externalInstanceName) gotExternal = true; @@ -375,3 +414,35 @@ TEST(CameraProviderManagerTest, MultipleVendorTagTest) { metadataCopy.dump(1, 2); secondMetadata.dump(1, 2); } + +TEST(CameraProviderManagerTest, NotifyStateChangeTest) { + std::vector deviceNames { + "device@3.2/test/0", + "device@1.0/test/0", + "device@3.2/test/1"}; + + hardware::hidl_vec vendorSection; + status_t res; + sp providerManager = new CameraProviderManager(); + sp statusListener = new TestStatusListener(); + TestInteractionProxy serviceProxy; + sp provider = new TestICameraProvider(deviceNames, + vendorSection); + serviceProxy.setProvider(provider); + + res = providerManager->initialize(statusListener, &serviceProxy); + ASSERT_EQ(res, OK) << "Unable to initialize provider manager"; + + ASSERT_EQ(provider->mCurrentState, + static_cast>(DeviceState::NORMAL)) + << "Initial device state not set"; + + res = providerManager->notifyDeviceStateChange( + static_cast>(DeviceState::FOLDED)); + + ASSERT_EQ(res, OK) << "Unable to call notifyDeviceStateChange"; + ASSERT_EQ(provider->mCurrentState, + static_cast>(DeviceState::FOLDED)) + << "Unable to change device state"; + +}