CameraProviderManager: Handle transaction errors, HAL process death
- Register and handle camera provider deaths
- Check for transport errors on all provider calls
- Clean up provider callback locking
Test: No camera CTS regressions
Bug: 35096594
Bug: 35441122
Change-Id: I08117f38f5201368a28093debdbcae67a68a4e7
diff --git a/services/camera/libcameraservice/CameraService.cpp b/services/camera/libcameraservice/CameraService.cpp
index 7133709..aec7794 100644
--- a/services/camera/libcameraservice/CameraService.cpp
+++ b/services/camera/libcameraservice/CameraService.cpp
@@ -423,20 +423,18 @@
// Remove cached shim parameters
state->setShimParams(CameraParameters());
- // Remove the client from the list of active clients
+ // Remove the client from the list of active clients, if there is one
clientToDisconnect = removeClientLocked(id);
+ }
+ // Disconnect client
+ if (clientToDisconnect.get() != nullptr) {
+ ALOGI("%s: Client for camera ID %s evicted due to device status change from HAL",
+ __FUNCTION__, id.string());
// Notify the client of disconnection
clientToDisconnect->notifyError(
hardware::camera2::ICameraDeviceCallbacks::ERROR_CAMERA_DISCONNECTED,
CaptureResultExtras{});
- }
-
- ALOGI("%s: Client for camera ID %s evicted due to device status change from HAL",
- __FUNCTION__, id.string());
-
- // Disconnect client
- if (clientToDisconnect.get() != nullptr) {
// Ensure not in binder RPC so client disconnect PID checks work correctly
LOG_ALWAYS_FATAL_IF(getCallingPid() != getpid(),
"onDeviceStatusChanged must be called from the camera service process!");
diff --git a/services/camera/libcameraservice/common/CameraProviderManager.cpp b/services/camera/libcameraservice/common/CameraProviderManager.cpp
index f6ad7d7..f8b2908 100644
--- a/services/camera/libcameraservice/common/CameraProviderManager.cpp
+++ b/services/camera/libcameraservice/common/CameraProviderManager.cpp
@@ -21,6 +21,7 @@
#include "CameraProviderManager.h"
#include <chrono>
+#include <inttypes.h>
#include <hidl/ServiceManagement.h>
namespace android {
@@ -65,7 +66,7 @@
}
// See if there's a passthrough HAL, but let's not complain if there's not
- addProvider(kLegacyProviderName, /*expected*/ false);
+ addProviderLocked(kLegacyProviderName, /*expected*/ false);
return OK;
}
@@ -194,14 +195,19 @@
for (auto& provider : mProviders) {
hardware::hidl_vec<VendorTagSection> vts;
Status status;
- provider->mInterface->getVendorTags(
+ hardware::Return<void> ret;
+ ret = provider->mInterface->getVendorTags(
[&](auto s, const auto& vendorTagSecs) {
status = s;
if (s == Status::OK) {
vts = vendorTagSecs;
}
});
-
+ if (!ret.isOk()) {
+ ALOGE("%s: Transaction error getting vendor tags from provider '%s': %s",
+ __FUNCTION__, provider->mProviderName.c_str(), ret.description().c_str());
+ return DEAD_OBJECT;
+ }
if (status != Status::OK) {
return mapToStatusT(status);
}
@@ -239,13 +245,19 @@
auto *deviceInfo3 = static_cast<ProviderInfo::DeviceInfo3*>(deviceInfo);
Status status;
- deviceInfo3->mInterface->open(callback, [&status, &session]
+ hardware::Return<void> ret;
+ ret = deviceInfo3->mInterface->open(callback, [&status, &session]
(Status s, const sp<device::V3_2::ICameraDeviceSession>& cameraSession) {
status = s;
if (status == Status::OK) {
*session = cameraSession;
}
});
+ if (!ret.isOk()) {
+ ALOGE("%s: Transaction error opening a session for camera device %s: %s",
+ __FUNCTION__, id.c_str(), ret.description().c_str());
+ return DEAD_OBJECT;
+ }
return mapToStatusT(status);
}
@@ -262,7 +274,12 @@
auto *deviceInfo1 = static_cast<ProviderInfo::DeviceInfo1*>(deviceInfo);
- Status status = deviceInfo1->mInterface->open(callback);
+ hardware::Return<Status> status = deviceInfo1->mInterface->open(callback);
+ if (!status.isOk()) {
+ ALOGE("%s: Transaction error opening a session for camera device %s: %s",
+ __FUNCTION__, id.c_str(), status.description().c_str());
+ return DEAD_OBJECT;
+ }
if (status == Status::OK) {
*session = deviceInfo1->mInterface;
}
@@ -276,7 +293,7 @@
bool /*preexisting*/) {
std::lock_guard<std::mutex> lock(mInterfaceMutex);
- addProvider(name);
+ addProviderLocked(name);
return hardware::Return<void>();
}
@@ -304,7 +321,7 @@
}
-status_t CameraProviderManager::addProvider(const std::string& newProvider, bool expected) {
+status_t CameraProviderManager::addProviderLocked(const std::string& newProvider, bool expected) {
for (const auto& providerInfo : mProviders) {
if (providerInfo->mProviderName == newProvider) {
ALOGW("%s: Camera provider HAL with name '%s' already registered", __FUNCTION__,
@@ -312,13 +329,14 @@
return ALREADY_EXISTS;
}
}
- sp<provider::V2_4::ICameraProvider> interface =
- mServiceProxy->getService(newProvider);
+
+ sp<provider::V2_4::ICameraProvider> interface;
+ interface = mServiceProxy->getService(newProvider);
if (interface == nullptr) {
- ALOGW("%s: Camera provider HAL '%s' is not actually available", __FUNCTION__,
- newProvider.c_str());
if (expected) {
+ ALOGE("%s: Camera provider HAL '%s' is not actually available", __FUNCTION__,
+ newProvider.c_str());
return BAD_VALUE;
} else {
return OK;
@@ -338,15 +356,39 @@
}
status_t CameraProviderManager::removeProvider(const std::string& provider) {
+ std::unique_lock<std::mutex> lock(mInterfaceMutex);
+ std::vector<String8> removedDeviceIds;
+ status_t res = NAME_NOT_FOUND;
for (auto it = mProviders.begin(); it != mProviders.end(); it++) {
if ((*it)->mProviderName == provider) {
+ removedDeviceIds.reserve((*it)->mDevices.size());
+ for (auto& deviceInfo : (*it)->mDevices) {
+ removedDeviceIds.push_back(String8(deviceInfo->mId.c_str()));
+ }
mProviders.erase(it);
- return OK;
+ res = OK;
+ break;
}
}
- ALOGW("%s: Camera provider HAL with name '%s' is not registered", __FUNCTION__,
- provider.c_str());
- return NAME_NOT_FOUND;
+ if (res != OK) {
+ ALOGW("%s: Camera provider HAL with name '%s' is not registered", __FUNCTION__,
+ provider.c_str());
+ } else {
+ // Inform camera service of loss of presence for all the devices from this provider,
+ // without lock held for reentrancy
+ sp<StatusListener> listener = getStatusListener();
+ if (listener != nullptr) {
+ lock.unlock();
+ for (auto& id : removedDeviceIds) {
+ listener->onDeviceStatusChanged(id, CameraDeviceStatus::NOT_PRESENT);
+ }
+ }
+ }
+ return res;
+}
+
+sp<CameraProviderManager::StatusListener> CameraProviderManager::getStatusListener() const {
+ return mListener.promote();
}
/**** Methods for ProviderInfo ****/
@@ -370,17 +412,31 @@
}
ALOGI("Connecting to new camera provider: %s, isRemote? %d",
mProviderName.c_str(), mInterface->isRemote());
- Status status = mInterface->setCallback(this);
+ hardware::Return<Status> status = mInterface->setCallback(this);
+ if (!status.isOk()) {
+ ALOGE("%s: Transaction error setting up callbacks with camera provider '%s': %s",
+ __FUNCTION__, mProviderName.c_str(), status.description().c_str());
+ return DEAD_OBJECT;
+ }
if (status != Status::OK) {
ALOGE("%s: Unable to register callbacks with camera provider '%s'",
__FUNCTION__, mProviderName.c_str());
return mapToStatusT(status);
}
- // TODO: Register for hw binder death notifications as well
+
+ hardware::Return<bool> linked = mInterface->linkToDeath(this, /*cookie*/ mId);
+ if (!linked.isOk()) {
+ ALOGE("%s: Transaction error in linking to camera provider '%s' death: %s",
+ __FUNCTION__, mProviderName.c_str(), linked.description().c_str());
+ return DEAD_OBJECT;
+ } else if (!linked) {
+ ALOGW("%s: Unable to link to provider '%s' death notifications",
+ __FUNCTION__, mProviderName.c_str());
+ }
// Get initial list of camera devices, if any
std::vector<std::string> devices;
- mInterface->getCameraIdList([&status, &devices](
+ hardware::Return<void> ret = mInterface->getCameraIdList([&status, &devices](
Status idStatus,
const hardware::hidl_vec<hardware::hidl_string>& cameraDeviceNames) {
status = idStatus;
@@ -389,18 +445,29 @@
devices.push_back(cameraDeviceNames[i]);
}
} });
-
+ if (!ret.isOk()) {
+ ALOGE("%s: Transaction error in getting camera ID list from provider '%s': %s",
+ __FUNCTION__, mProviderName.c_str(), linked.description().c_str());
+ return DEAD_OBJECT;
+ }
if (status != Status::OK) {
ALOGE("%s: Unable to query for camera devices from provider '%s'",
__FUNCTION__, mProviderName.c_str());
return mapToStatusT(status);
}
+ sp<StatusListener> listener = mManager->getStatusListener();
for (auto& device : devices) {
- status_t res = addDevice(device);
+ std::string id;
+ status_t res = addDevice(device,
+ hardware::camera::common::V1_0::CameraDeviceStatus::PRESENT, &id);
if (res != OK) {
ALOGE("%s: Unable to enumerate camera device '%s': %s (%d)",
__FUNCTION__, device.c_str(), strerror(-res), res);
+ continue;
+ }
+ if (listener != nullptr) {
+ listener->onDeviceStatusChanged(String8(id.c_str()), CameraDeviceStatus::PRESENT);
}
}
@@ -462,8 +529,9 @@
}
status_t CameraProviderManager::ProviderInfo::dump(int fd, const Vector<String16>&) const {
- dprintf(fd, "== Camera Provider HAL %s (v2.4) static info: %zu devices: ==\n",
- mProviderName.c_str(), mDevices.size());
+ dprintf(fd, "== Camera Provider HAL %s (v2.4, %s) static info: %zu devices: ==\n",
+ mProviderName.c_str(), mInterface->isRemote() ? "remote" : "passthrough",
+ mDevices.size());
for (auto& device : mDevices) {
dprintf(fd, "== Camera HAL device %s (v%d.%d) static information: ==\n", device->mName.c_str(),
@@ -512,7 +580,7 @@
sp<StatusListener> listener;
std::string id;
{
- std::lock_guard<std::mutex> lock(mManager->mStatusListenerMutex);
+ std::lock_guard<std::mutex> lock(mLock);
bool known = false;
for (auto& deviceInfo : mDevices) {
if (deviceInfo->mName == cameraDeviceName) {
@@ -534,7 +602,7 @@
}
addDevice(cameraDeviceName, newStatus, &id);
}
- listener = mManager->mListener.promote();
+ listener = mManager->getStatusListener();
}
// Call without lock held to allow reentrancy into provider manager
if (listener != nullptr) {
@@ -565,7 +633,7 @@
mProviderName.c_str(), cameraDeviceName.c_str(), newStatus);
return hardware::Void();
}
- listener = mManager->mListener.promote();
+ listener = mManager->getStatusListener();
}
// Call without lock held to allow reentrancy into provider manager
if (listener != nullptr) {
@@ -574,6 +642,16 @@
return hardware::Void();
}
+void CameraProviderManager::ProviderInfo::serviceDied(uint64_t cookie,
+ const wp<hidl::base::V1_0::IBase>& who) {
+ (void) who;
+ ALOGI("Camera provider '%s' has died; removing it", mProviderName.c_str());
+ if (cookie != mId) {
+ ALOGW("%s: Unexpected serviceDied cookie %" PRIu64 ", expected %" PRIu32,
+ __FUNCTION__, cookie, mId);
+ }
+ mManager->removeProvider(mProviderName);
+}
template<class DeviceInfoT>
std::unique_ptr<CameraProviderManager::ProviderInfo::DeviceInfo>
@@ -615,11 +693,17 @@
<device::V1_0::ICameraDevice>(const std::string &name) const {
Status status;
sp<device::V1_0::ICameraDevice> cameraInterface;
- mInterface->getCameraDeviceInterface_V1_x(name, [&status, &cameraInterface](
+ hardware::Return<void> ret;
+ ret = mInterface->getCameraDeviceInterface_V1_x(name, [&status, &cameraInterface](
Status s, sp<device::V1_0::ICameraDevice> interface) {
status = s;
cameraInterface = interface;
});
+ if (!ret.isOk()) {
+ ALOGE("%s: Transaction error trying to obtain interface for camera device %s: %s",
+ __FUNCTION__, name.c_str(), ret.description().c_str());
+ return nullptr;
+ }
if (status != Status::OK) {
ALOGE("%s: Unable to obtain interface for camera device %s: %s", __FUNCTION__,
name.c_str(), statusToString(status));
@@ -634,11 +718,17 @@
<device::V3_2::ICameraDevice>(const std::string &name) const {
Status status;
sp<device::V3_2::ICameraDevice> cameraInterface;
- mInterface->getCameraDeviceInterface_V3_x(name, [&status, &cameraInterface](
+ hardware::Return<void> ret;
+ ret = mInterface->getCameraDeviceInterface_V3_x(name, [&status, &cameraInterface](
Status s, sp<device::V3_2::ICameraDevice> interface) {
status = s;
cameraInterface = interface;
});
+ if (!ret.isOk()) {
+ ALOGE("%s: Transaction error trying to obtain interface for camera device %s: %s",
+ __FUNCTION__, name.c_str(), ret.description().c_str());
+ return nullptr;
+ }
if (status != Status::OK) {
ALOGE("%s: Unable to obtain interface for camera device %s: %s", __FUNCTION__,
name.c_str(), statusToString(status));
@@ -665,25 +755,37 @@
mInterface(interface) {
// Get default parameters and initialize flash unit availability
// Requires powering on the camera device
- Status status = mInterface->open(nullptr);
- if (status != Status::OK) {
- ALOGE("%s: Unable to open camera device %s to check for a flash unit: %s (%d)", __FUNCTION__,
- mId.c_str(), CameraProviderManager::statusToString(status), status);
+ hardware::Return<Status> status = mInterface->open(nullptr);
+ if (!status.isOk()) {
+ ALOGE("%s: Transaction error opening camera device %s to check for a flash unit: %s",
+ __FUNCTION__, mId.c_str(), status.description().c_str());
return;
}
- mInterface->getParameters([this](const hardware::hidl_string& parms) {
+ if (status != Status::OK) {
+ ALOGE("%s: Unable to open camera device %s to check for a flash unit: %s", __FUNCTION__,
+ mId.c_str(), CameraProviderManager::statusToString(status));
+ return;
+ }
+ hardware::Return<void> ret;
+ ret = mInterface->getParameters([this](const hardware::hidl_string& parms) {
mDefaultParameters.unflatten(String8(parms.c_str()));
});
-
+ if (!ret.isOk()) {
+ ALOGE("%s: Transaction error reading camera device %s params to check for a flash unit: %s",
+ __FUNCTION__, mId.c_str(), status.description().c_str());
+ return;
+ }
const char *flashMode =
mDefaultParameters.get(CameraParameters::KEY_SUPPORTED_FLASH_MODES);
if (flashMode && strstr(flashMode, CameraParameters::FLASH_MODE_TORCH)) {
mHasFlashUnit = true;
- } else {
- mHasFlashUnit = false;
}
- mInterface->close();
+ ret = mInterface->close();
+ if (!ret.isOk()) {
+ ALOGE("%s: Transaction error closing camera device %s after check for a flash unit: %s",
+ __FUNCTION__, mId.c_str(), status.description().c_str());
+ }
}
CameraProviderManager::ProviderInfo::DeviceInfo1::~DeviceInfo1() {}
@@ -698,10 +800,16 @@
Status status;
device::V1_0::CameraInfo cInfo;
- mInterface->getCameraInfo([&status, &cInfo](Status s, device::V1_0::CameraInfo camInfo) {
+ hardware::Return<void> ret;
+ ret = mInterface->getCameraInfo([&status, &cInfo](Status s, device::V1_0::CameraInfo camInfo) {
status = s;
cInfo = camInfo;
});
+ if (!ret.isOk()) {
+ ALOGE("%s: Transaction error reading camera info from device %s: %s",
+ __FUNCTION__, mId.c_str(), ret.description().c_str());
+ return DEAD_OBJECT;
+ }
if (status != Status::OK) {
return mapToStatusT(status);
}
@@ -716,7 +824,8 @@
info->facing = hardware::CAMERA_FACING_FRONT;
break;
default:
- ALOGW("%s: Unknown camera facing: %d", __FUNCTION__, cInfo.facing);
+ ALOGW("%s: Device %s: Unknown camera facing: %d",
+ __FUNCTION__, mId.c_str(), cInfo.facing);
info->facing = hardware::CAMERA_FACING_BACK;
}
info->orientation = cInfo.orientation;
@@ -733,7 +842,8 @@
mInterface(interface) {
// Get camera characteristics and initialize flash unit availability
Status status;
- mInterface->getCameraCharacteristics([&status, this](Status s,
+ hardware::Return<void> ret;
+ ret = mInterface->getCameraCharacteristics([&status, this](Status s,
device::V3_2::CameraMetadata metadata) {
status = s;
if (s == Status::OK) {
@@ -742,6 +852,12 @@
mCameraCharacteristics = buffer;
}
});
+ if (!ret.isOk()) {
+ ALOGE("%s: Transaction error getting camera characteristics for device %s"
+ " to check for a flash unit: %s", __FUNCTION__, mId.c_str(),
+ ret.description().c_str());
+ return;
+ }
if (status != Status::OK) {
ALOGE("%s: Unable to get camera characteristics for device %s: %s (%d)",
__FUNCTION__, mId.c_str(), CameraProviderManager::statusToString(status), status);
diff --git a/services/camera/libcameraservice/common/CameraProviderManager.h b/services/camera/libcameraservice/common/CameraProviderManager.h
index 5ae16cd..b1da831 100644
--- a/services/camera/libcameraservice/common/CameraProviderManager.h
+++ b/services/camera/libcameraservice/common/CameraProviderManager.h
@@ -226,7 +226,10 @@
static HardwareServiceInteractionProxy sHardwareServiceInteractionProxy;
- struct ProviderInfo : virtual public hardware::camera::provider::V2_4::ICameraProviderCallback {
+ struct ProviderInfo :
+ virtual public hardware::camera::provider::V2_4::ICameraProviderCallback,
+ virtual public hardware::hidl_death_recipient
+ {
const std::string mProviderName;
const sp<hardware::camera::provider::V2_4::ICameraProvider> mInterface;
@@ -254,6 +257,9 @@
const hardware::hidl_string& cameraDeviceName,
hardware::camera::common::V1_0::TorchModeStatus newStatus) override;
+ // hidl_death_recipient interface - this locks the parent mInterfaceMutex
+ virtual void serviceDied(uint64_t cookie, const wp<hidl::base::V1_0::IBase>& who) override;
+
// Basic device information, common to all camera devices
struct DeviceInfo {
const std::string mName; // Full instance name
@@ -327,6 +333,8 @@
std::string mType;
uint32_t mId;
+ std::mutex mLock;
+
CameraProviderManager *mManager;
// Templated method to instantiate the right kind of DeviceInfo and call the
@@ -357,8 +365,10 @@
hardware::hidl_version minVersion = hardware::hidl_version{0,0},
hardware::hidl_version maxVersion = hardware::hidl_version{1000,0}) const;
- status_t addProvider(const std::string& newProvider, bool expected = true);
+ status_t addProviderLocked(const std::string& newProvider, bool expected = true);
+
status_t removeProvider(const std::string& provider);
+ sp<StatusListener> getStatusListener() const;
bool isValidDeviceLocked(const std::string &id, uint16_t majorVersion) const;