Simplify EVS HAL and move to "agressive opens"
This adapts the API implementation to allow a duplicate "open" operation
to automatically close any previous connections to the device. This
works around a binder level issue that can cause destructors triggered
by remote clients to be delivered out of order to the server.
This was originally change ag/1969959 on master, but has been
recreated on oc-dev (cherry-picking was broken at the time).
The original master change will be abandoned in favor of this getting
merged down from oc-dev.
Test: Run Vts test (added in following change)
Change-Id: I7b417998e59a4d592fbb91811c4101f39097c5dd
diff --git a/automotive/evs/1.0/default/EvsEnumerator.cpp b/automotive/evs/1.0/default/EvsEnumerator.cpp
index e54f699..731e21b 100644
--- a/automotive/evs/1.0/default/EvsEnumerator.cpp
+++ b/automotive/evs/1.0/default/EvsEnumerator.cpp
@@ -28,33 +28,36 @@
namespace implementation {
-// TODO(b/31632518): Need to get notification when our client dies so we can close the camera.
-// As it stands, if the client dies suddenly, the camera will be stuck "open".
-// NOTE: Display should already be safe by virtue of holding only a weak pointer.
+// NOTE: All members values are static so that all clients operate on the same state
+// That is to say, this is effectively a singleton despite the fact that HIDL
+// constructs a new instance for each client.
+std::list<EvsEnumerator::CameraRecord> EvsEnumerator::sCameraList;
+wp<EvsDisplay> EvsEnumerator::sActiveDisplay;
EvsEnumerator::EvsEnumerator() {
ALOGD("EvsEnumerator created");
// Add sample camera data to our list of cameras
- // NOTE: The id strings trigger special initialization inside the EvsCamera constructor
- mCameraList.emplace_back( new EvsCamera(EvsCamera::kCameraName_Backup), false );
- mCameraList.emplace_back( new EvsCamera("LaneView"), false );
- mCameraList.emplace_back( new EvsCamera(EvsCamera::kCameraName_RightTurn), false );
+ // In a real driver, this would be expected to can the available hardware
+ sCameraList.emplace_back(EvsCamera::kCameraName_Backup);
+ sCameraList.emplace_back("LaneView");
+ sCameraList.emplace_back("right turn");
}
+
// Methods from ::android::hardware::automotive::evs::V1_0::IEvsEnumerator follow.
Return<void> EvsEnumerator::getCameraList(getCameraList_cb _hidl_cb) {
ALOGD("getCameraList");
- const unsigned numCameras = mCameraList.size();
+ const unsigned numCameras = sCameraList.size();
// Build up a packed array of CameraDesc for return
// NOTE: Only has to live until the callback returns
std::vector<CameraDesc> descriptions;
descriptions.reserve(numCameras);
- for (const auto& cam : mCameraList) {
- descriptions.push_back( cam.pCamera->getDesc() );
+ for (const auto& cam : sCameraList) {
+ descriptions.push_back( cam.desc );
}
// Encapsulate our camera descriptions in the HIDL vec type
@@ -68,97 +71,137 @@
return Void();
}
+
Return<sp<IEvsCamera>> EvsEnumerator::openCamera(const hidl_string& cameraId) {
ALOGD("openCamera");
// Find the named camera
CameraRecord *pRecord = nullptr;
- for (auto &&cam : mCameraList) {
- if (cam.pCamera->getDesc().cameraId == cameraId) {
+ for (auto &&cam : sCameraList) {
+ if (cam.desc.cameraId == cameraId) {
// Found a match!
pRecord = &cam;
break;
}
}
+ // Is this a recognized camera id?
if (!pRecord) {
ALOGE("Requested camera %s not found", cameraId.c_str());
return nullptr;
- } else if (pRecord->inUse) {
- ALOGE("Cannot open camera %s which is already in use", cameraId.c_str());
- return nullptr;
- } else {
- pRecord->inUse = true;
- return(pRecord->pCamera);
}
+
+ // Has this camera already been instantiated by another caller?
+ sp<EvsCamera> pActiveCamera = pRecord->activeInstance.promote();
+ if (pActiveCamera != nullptr) {
+ ALOGW("Killing previous camera because of new caller");
+ closeCamera(pActiveCamera);
+ }
+
+ // Construct a camera instance for the caller
+ pActiveCamera = new EvsCamera(cameraId);
+ pRecord->activeInstance = pActiveCamera;
+ if (pActiveCamera == nullptr) {
+ ALOGE("Failed to allocate new EvsCamera object for %s\n", cameraId.c_str());
+ }
+
+ return pActiveCamera;
}
-Return<void> EvsEnumerator::closeCamera(const ::android::sp<IEvsCamera>& camera) {
+
+Return<void> EvsEnumerator::closeCamera(const ::android::sp<IEvsCamera>& pCamera) {
ALOGD("closeCamera");
- if (camera == nullptr) {
- ALOGE("Ignoring call to closeCamera with null camera pointer");
- } else {
- // Find this camera in our list
- auto it = std::find_if(mCameraList.begin(),
- mCameraList.end(),
- [camera](const CameraRecord& rec) {
- return (rec.pCamera == camera);
- });
- if (it == mCameraList.end()) {
- ALOGE("Ignoring close on unrecognized camera");
- } else {
- // Make sure the camera has stopped streaming
- camera->stopVideoStream();
+ if (pCamera == nullptr) {
+ ALOGE("Ignoring call to closeCamera with null camera ptr");
+ return Void();
+ }
- it->inUse = false;
+ // Get the camera id so we can find it in our list
+ std::string cameraId;
+ pCamera->getCameraInfo([&cameraId](CameraDesc desc) {
+// TODO(b/36532780) Should we able to just use a simple assignment?
+// cameraId = desc.cameraId;
+ cameraId.assign(desc.cameraId.c_str());
+ }
+ );
+
+ // Find the named camera
+ CameraRecord *pRecord = nullptr;
+ for (auto &&cam : sCameraList) {
+ if (cam.desc.cameraId == cameraId) {
+ // Found a match!
+ pRecord = &cam;
+ break;
+ }
+ }
+
+ // Is the display being destroyed actually the one we think is active?
+ if (!pRecord) {
+ ALOGE("Asked to close a camera who's name isn't recognized");
+ } else {
+ sp<EvsCamera> pActiveCamera = pRecord->activeInstance.promote();
+
+ if (pActiveCamera == nullptr) {
+ ALOGE("Somehow a camera is being destroyed when the enumerator didn't know one existed");
+ } else if (pActiveCamera != pCamera) {
+ // This can happen if the camera was aggressively reopened, orphaning this previous instance
+ ALOGW("Ignoring close of previously orphaned camera - why did a client steal?");
+ } else {
+ // Drop the active camera
+ pActiveCamera->forceShutdown();
+ pRecord->activeInstance = nullptr;
}
}
return Void();
}
+
Return<sp<IEvsDisplay>> EvsEnumerator::openDisplay() {
ALOGD("openDisplay");
- // If we already have a display active, then this request must be denied
- sp<IEvsDisplay> pActiveDisplay = mActiveDisplay.promote();
+ // If we already have a display active, then we need to shut it down so we can
+ // give exclusive access to the new caller.
+ sp<EvsDisplay> pActiveDisplay = sActiveDisplay.promote();
if (pActiveDisplay != nullptr) {
- ALOGW("Rejecting openDisplay request the display is already in use.");
- return nullptr;
- } else {
- // Create a new display interface and return it
- pActiveDisplay = new EvsDisplay();
- mActiveDisplay = pActiveDisplay;
- ALOGD("Returning new EvsDisplay object %p", pActiveDisplay.get());
- return pActiveDisplay;
+ ALOGW("Killing previous display because of new caller");
+ closeDisplay(pActiveDisplay);
}
+
+ // Create a new display interface and return it
+ pActiveDisplay = new EvsDisplay();
+ sActiveDisplay = pActiveDisplay;
+
+ ALOGD("Returning new EvsDisplay object %p", pActiveDisplay.get());
+ return pActiveDisplay;
}
-Return<void> EvsEnumerator::closeDisplay(const ::android::sp<IEvsDisplay>& display) {
+
+Return<void> EvsEnumerator::closeDisplay(const ::android::sp<IEvsDisplay>& pDisplay) {
ALOGD("closeDisplay");
// Do we still have a display object we think should be active?
- sp<IEvsDisplay> pActiveDisplay = mActiveDisplay.promote();
-
+ sp<EvsDisplay> pActiveDisplay = sActiveDisplay.promote();
if (pActiveDisplay == nullptr) {
- ALOGE("Ignoring closeDisplay when there is no active display.");
- } else if (display != pActiveDisplay) {
- ALOGE("Ignoring closeDisplay on a display we didn't issue");
- ALOGI("Got %p while active display is %p.", display.get(), pActiveDisplay.get());
+ ALOGE("Somehow a display is being destroyed when the enumerator didn't know one existed");
+ } else if (sActiveDisplay != pDisplay) {
+ ALOGW("Ignoring close of previously orphaned display - why did a client steal?");
} else {
// Drop the active display
- mActiveDisplay = nullptr;
+ pActiveDisplay->forceShutdown();
+ sActiveDisplay = nullptr;
}
return Void();
}
+
Return<DisplayState> EvsEnumerator::getDisplayState() {
ALOGD("getDisplayState");
// Do we still have a display object we think should be active?
- sp<IEvsDisplay> pActiveDisplay = mActiveDisplay.promote();
+ sp<IEvsDisplay> pActiveDisplay = sActiveDisplay.promote();
if (pActiveDisplay != nullptr) {
return pActiveDisplay->getDisplayState();
} else {