From ec96060c3b4b4d793c6599204ae4f639d065a57d Mon Sep 17 00:00:00 2001 From: Eino-Ville Talvala Date: Tue, 15 Oct 2019 11:46:16 -0700 Subject: [PATCH 1/2] CameraService: Don't block on camera HAL start in all cases When initially starting up, use tryGetService instead of getService to avoid blocking camera service start in cases where the camera HAL is stuck. Test: atest cameraservice_test && atest CameraManagerTest, with and without lazy camera HAL Change-Id: I2ad5c542e77e748902cfb49f90a55620b29ad4cd --- .../libcameraservice/common/CameraProviderManager.cpp | 2 +- .../libcameraservice/common/CameraProviderManager.h | 8 ++++++++ .../libcameraservice/tests/CameraProviderManagerTest.cpp | 5 +++++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/services/camera/libcameraservice/common/CameraProviderManager.cpp b/services/camera/libcameraservice/common/CameraProviderManager.cpp index 974026cbc9..77b8536edf 100644 --- a/services/camera/libcameraservice/common/CameraProviderManager.cpp +++ b/services/camera/libcameraservice/common/CameraProviderManager.cpp @@ -1154,7 +1154,7 @@ status_t CameraProviderManager::addProviderLocked(const std::string& newProvider } sp interface; - interface = mServiceProxy->getService(newProvider); + interface = mServiceProxy->tryGetService(newProvider); if (interface == nullptr) { ALOGE("%s: Camera provider HAL '%s' is not actually available", __FUNCTION__, diff --git a/services/camera/libcameraservice/common/CameraProviderManager.h b/services/camera/libcameraservice/common/CameraProviderManager.h index f4cf6674f0..13a8513de5 100644 --- a/services/camera/libcameraservice/common/CameraProviderManager.h +++ b/services/camera/libcameraservice/common/CameraProviderManager.h @@ -96,6 +96,10 @@ public: const std::string &serviceName, const sp ¬ification) = 0; + // Will not wait for service to start if it's not already running + virtual sp tryGetService( + const std::string &serviceName) = 0; + // Will block for service if it exists but isn't running virtual sp getService( const std::string &serviceName) = 0; virtual hardware::hidl_vec listServices() = 0; @@ -112,6 +116,10 @@ public: return hardware::camera::provider::V2_4::ICameraProvider::registerForNotifications( serviceName, notification); } + virtual sp tryGetService( + const std::string &serviceName) override { + return hardware::camera::provider::V2_4::ICameraProvider::tryGetService(serviceName); + } virtual sp getService( const std::string &serviceName) override { return hardware::camera::provider::V2_4::ICameraProvider::getService(serviceName); diff --git a/services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp b/services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp index 78d737d1b2..ca275f4fe4 100644 --- a/services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp +++ b/services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp @@ -199,6 +199,11 @@ struct TestInteractionProxy : public CameraProviderManager::ServiceInteractionPr return true; } + virtual sp tryGetService( + const std::string &serviceName) override { + return getService(serviceName); + } + virtual sp getService( const std::string &serviceName) override { mLastRequestedServiceNames.push_back(serviceName); From 998d1fe22142a3fd729df87af59476f97b926e02 Mon Sep 17 00:00:00 2001 From: Eino-Ville Talvala Date: Wed, 23 Oct 2019 10:34:53 -0700 Subject: [PATCH 2/2] CameraService: Add unit test for stuck HAL at startup New test verifies that getService is not called during camera service startup, to make sure it can't get stuck if the camera HAL isn't initializing properly. Test: New test passes with new tryGetService code; test fails with older camera service code. Change-Id: I67b3fe392e8cc6961c33abf25e961a8e2df7d7b5 --- .../tests/CameraProviderManagerTest.cpp | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp b/services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp index ca275f4fe4..084dc62adb 100644 --- a/services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp +++ b/services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp @@ -183,6 +183,7 @@ struct TestInteractionProxy : public CameraProviderManager::ServiceInteractionPr sp mTestCameraProvider; TestInteractionProxy() {} + void setProvider(sp provider) { mTestCameraProvider = provider; } @@ -201,16 +202,29 @@ struct TestInteractionProxy : public CameraProviderManager::ServiceInteractionPr virtual sp tryGetService( const std::string &serviceName) override { + // If no provider has been given, act like the HAL isn't available and return null. + if (mTestCameraProvider == nullptr) return nullptr; return getService(serviceName); } virtual sp getService( const std::string &serviceName) override { + // If no provider has been given, fail; in reality, getService would + // block for HALs that don't start correctly, so we should never use + // getService when we don't have a valid HAL running + if (mTestCameraProvider == nullptr) { + ADD_FAILURE() << "getService called with no valid provider; would block indefinitely"; + // Real getService would block, but that's bad in unit tests. So + // just record an error and return nullptr + return nullptr; + } mLastRequestedServiceNames.push_back(serviceName); return mTestCameraProvider; } virtual hardware::hidl_vec listServices() override { + // Always provide a list even if there's no actual provider yet, to + // simulate stuck HAL situations as well hardware::hidl_vec ret = {"test/0"}; return ret; } @@ -443,3 +457,52 @@ TEST(CameraProviderManagerTest, NotifyStateChangeTest) { << "Unable to change device state"; } + +// Test that CameraProviderManager doesn't get stuck when the camera HAL isn't really working +TEST(CameraProviderManagerTest, BadHalStartupTest) { + + std::vector deviceNames; + deviceNames.push_back("device@3.2/test/0"); + deviceNames.push_back("device@1.0/test/0"); + deviceNames.push_back("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); + + // Not setting up provider in the service proxy yet, to test cases where a + // HAL isn't starting right + res = providerManager->initialize(statusListener, &serviceProxy); + ASSERT_EQ(res, OK) << "Unable to initialize provider manager"; + + // Now set up provider and trigger a registration + serviceProxy.setProvider(provider); + int numProviders = static_cast(serviceProxy.listServices().size()); + + hardware::hidl_string testProviderFqInterfaceName = + "android.hardware.camera.provider@2.4::ICameraProvider"; + hardware::hidl_string testProviderInstanceName = "test/0"; + serviceProxy.mManagerNotificationInterface->onRegistration( + testProviderFqInterfaceName, + testProviderInstanceName, false); + + // Check that new provider is called once for all the init methods + EXPECT_EQ(provider->mCalledCounter[TestICameraProvider::SET_CALLBACK], numProviders) << + "Only one call to setCallback per provider expected during register"; + EXPECT_EQ(provider->mCalledCounter[TestICameraProvider::GET_VENDOR_TAGS], numProviders) << + "Only one call to getVendorTags per provider expected during register"; + EXPECT_EQ(provider->mCalledCounter[TestICameraProvider::IS_SET_TORCH_MODE_SUPPORTED], + numProviders) << + "Only one call to isSetTorchModeSupported per provider expected during init"; + EXPECT_EQ(provider->mCalledCounter[TestICameraProvider::GET_CAMERA_ID_LIST], numProviders) << + "Only one call to getCameraIdList per provider expected during init"; + EXPECT_EQ(provider->mCalledCounter[TestICameraProvider::NOTIFY_DEVICE_STATE], numProviders) << + "Only one call to notifyDeviceState per provider expected during init"; + + ASSERT_EQ(serviceProxy.mLastRequestedServiceNames.back(), testProviderInstanceName) << + "Incorrect instance requested from service manager"; +}