Adopt weak pointers for death detection in EVS
Keep only weak pointers on the server side to objects that are owned by
the client. Use the weak pointers to detect if/when the client goes away
unexpectedly. Use destructors to do server side cleanup when the last
client reference dies.
Also removes explicit handle cloning as that is now handled inside
hidl_handle (as of 1/25/2017)
Adds calls to register buffer to comply with gralloc requirements.
Test: locally compile and run evs_test.
Change-Id: I18fc0df3fa643536ab5dbc89d52de9af6a286972
diff --git a/evs/app/EvsStateControl.cpp b/evs/app/EvsStateControl.cpp
index 265db68..8624246 100644
--- a/evs/app/EvsStateControl.cpp
+++ b/evs/app/EvsStateControl.cpp
@@ -77,6 +77,9 @@
bool EvsStateControl::configureForVehicleState() {
ALOGD("configureForVehicleState");
+ static int32_t sDummyGear = int32_t(VehicleGear::GEAR_REVERSE);
+ static int32_t sDummySignal = int32_t(VehicleTurnSignal::NONE);
+
if (mVehicle != nullptr) {
// Query the car state
if (invokeGet(&mGearValue) != StatusCode::OK) {
@@ -84,14 +87,12 @@
return false;
}
if (invokeGet(&mTurnSignalValue) != StatusCode::OK) {
- ALOGE("TURN_SIGNAL_STATE not available from vehicle. Exiting.");
- return false;
+ // Silently treat missing turn signal state as no turn signal active
+ mTurnSignalValue.value.int32Values.setToExternal(&sDummySignal, 1);
}
} else {
// While testing without a vehicle, behave as if we're in reverse for the first 20 seconds
static const int kShowTime = 20; // seconds
- static int32_t sDummyGear = int32_t(VehicleGear::GEAR_REVERSE);
- static int32_t sDummySignal = int32_t(VehicleTurnSignal::NONE);
// See if it's time to turn off the default reverse camera
static std::chrono::steady_clock::time_point start = std::chrono::steady_clock::now();
@@ -202,6 +203,9 @@
// Activate the display
ALOGD("Arming the display");
mDisplay->setDisplayState(DisplayState::VISIBLE_ON_NEXT_FRAME);
+
+ // TODO: Detect and exit if we encounter a stalled stream or unresponsive driver?
+ // Consider using a timer and watching for frame arrival?
}
}
@@ -221,16 +225,12 @@
std::unique_lock<std::mutex> lock(mAccessLock);
mCurrentState = State::OFF;
lock.unlock();
-
- // In case the main thread is waiting for us, announce our change
- mSignal.notify_one();
} else {
// Get the output buffer we'll use to display the imagery
BufferDesc tgtBuffer = {};
mDisplay->getTargetBuffer([&tgtBuffer]
(const BufferDesc& buff) {
tgtBuffer = buff;
- tgtBuffer.memHandle = native_handle_clone(buff.memHandle);
ALOGD("Got output buffer (%p) with id %d cloned as (%p)",
buff.memHandle.getNativeHandle(),
tgtBuffer.bufferId,
@@ -256,15 +256,6 @@
ALOGE("We encountered error %d when returning a buffer to the display!",
(EvsResult)result);
}
-
- // Now release our copy of the handle
- // TODO: If we don't end up needing to pass it back, then close our handle earlier
- // As it stands, the buffer might still be held by this process for some time after
- // it gets returned to the server via returnTargetBufferForDisplay()
- native_handle_close(tgtBuffer.memHandle.getNativeHandle());
-
- // TODO: Sort our whether this call is needed, and if so, are we forced to const_cast?
- //native_handle_delete(tgtBuffer.memHandle.getNativeHandle());
}
// Send the camera buffer back now that we're done with it
diff --git a/evs/app/EvsStateControl.h b/evs/app/EvsStateControl.h
index f4d9fbe..6390fca 100644
--- a/evs/app/EvsStateControl.h
+++ b/evs/app/EvsStateControl.h
@@ -68,7 +68,6 @@
sp<IEvsCamera> mCurrentCamera;
std::mutex mAccessLock;
- std::condition_variable mSignal;
};
diff --git a/evs/manager/Enumerator.cpp b/evs/manager/Enumerator.cpp
index c710c92..b7f6446 100644
--- a/evs/manager/Enumerator.cpp
+++ b/evs/manager/Enumerator.cpp
@@ -85,14 +85,14 @@
clientCamera = hwCamera->makeVirtualCamera();
}
- // Add the hardware camera to our list, which will keep the object alive via ref count
+ // Add the hardware camera to our list, which will keep it alive via ref count
if (clientCamera != nullptr) {
mCameras.push_back(hwCamera);
} else {
ALOGE("Requested camera %s not found or not available", cameraId.c_str());
}
- // Send the virtual camera object (also owned by our hwCamera) back to the client
+ // Send the virtual camera object back to the client by strong pointer which will keep it alive
return clientCamera;
}
@@ -136,19 +136,20 @@
ALOGD("openDisplay");
// If we already have a display active, then this request must be denied
- if (mActiveDisplay != nullptr) {
+ sp<IEvsDisplay> pActiveDisplay = mActiveDisplay.promote();
+ if (pActiveDisplay != nullptr) {
ALOGW("Rejecting openDisplay request because the display is already in use.");
return nullptr;
} else {
// Request exclusive access to the EVS display
ALOGI("Acquiring EVS Display");
-
- mActiveDisplay = mHwEnumerator->openDisplay();
- if (mActiveDisplay.get() == nullptr) {
+ pActiveDisplay = mHwEnumerator->openDisplay();
+ if (pActiveDisplay == nullptr) {
ALOGE("EVS Display unavailable");
}
- return mActiveDisplay;
+ mActiveDisplay = pActiveDisplay;
+ return pActiveDisplay;
}
}
@@ -156,9 +157,13 @@
Return<void> Enumerator::closeDisplay(const ::android::sp<IEvsDisplay>& display) {
ALOGD("closeDisplay");
+ // Do we still have a display object we think should be active?
+ sp<IEvsDisplay> pActiveDisplay = mActiveDisplay.promote();
+
// Drop the active display
- if (display != mActiveDisplay) {
- ALOGW("Ignoring call to closeDisplay with a display object we don't recognize.");
+ if (display.get() != pActiveDisplay.get()) {
+ ALOGW("Ignoring call to closeDisplay with unrecognzied display object.");
+ ALOGI("Got %p while active display is %p.", display.get(), pActiveDisplay.get());
} else {
// Pass this request through to the hardware layer
mHwEnumerator->closeDisplay(display);
@@ -172,24 +177,20 @@
Return<DisplayState> Enumerator::getDisplayState() {
ALOGD("getDisplayState");
- // Pass this request through to the hardware layer
- if (mHwEnumerator == nullptr) {
- // If we haven't even been initialized, this should not be called
- ALOGE("getDisplayState called on incompletely initialized EvsEnumerator");
- return DisplayState::NOT_OPEN;
- } else if (mActiveDisplay == nullptr) {
- // We actually DO hold the underlying hardware display open (in order to reserve it)
- // but the application hasn't opened it, so we know it isn't actually doing anything.
- return DisplayState::NOT_OPEN;
+ // Do we have a display object we think should be active?
+ sp<IEvsDisplay> pActiveDisplay = mActiveDisplay.promote();
+ if (pActiveDisplay != nullptr) {
+ // Pass this request through to the hardware layer
+ return pActiveDisplay->getDisplayState();
} else {
- return mActiveDisplay->getDisplayState();
+ // We don't have a live display right now
+ mActiveDisplay = nullptr;
+ return DisplayState::NOT_OPEN;
}
}
// TODO(b/31632518): Need to get notification when our client dies so we can close the camera.
-// As possible work around would be to give the client a HIDL object to exclusively hold
-// and use it's destructor to perform some work in the server side.
} // namespace implementation
diff --git a/evs/manager/Enumerator.h b/evs/manager/Enumerator.h
index 532a37f..5a5146d 100644
--- a/evs/manager/Enumerator.h
+++ b/evs/manager/Enumerator.h
@@ -50,7 +50,7 @@
private:
sp<IEvsEnumerator> mHwEnumerator; // Hardware enumerator
- sp<IEvsDisplay> mActiveDisplay; // Hardware display
+ wp<IEvsDisplay> mActiveDisplay; // Hardware display
std::list<sp<HalCamera>> mCameras; // Camera proxy objects wrapping hw cameras
};
diff --git a/evs/manager/HalCamera.cpp b/evs/manager/HalCamera.cpp
index 065b879..b98a53c 100644
--- a/evs/manager/HalCamera.cpp
+++ b/evs/manager/HalCamera.cpp
@@ -30,6 +30,9 @@
namespace implementation {
+// TODO: We need to hook up death monitoring to detect stream death so we can attempt a reconnect
+
+
sp<VirtualCamera> HalCamera::makeVirtualCamera() {
// Create the client camera interface object
@@ -47,9 +50,10 @@
return nullptr;
}
- // Add this client to our ownership list
+ // Add this client to our ownership list via weak pointer
mClients.push_back(client);
+ // Return the strong pointer to the client
return client;
}
@@ -57,7 +61,7 @@
void HalCamera::disownVirtualCamera(sp<VirtualCamera> virtualCamera) {
// Ignore calls with null pointers
if (virtualCamera.get() == nullptr) {
- ALOGW("Ignoring call with null pointer");
+ ALOGW("Ignoring disownVirtualCamera call with null pointer");
return;
}
@@ -65,7 +69,11 @@
virtualCamera->stopVideoStream();
// Remove the virtual camera from our client list
+ unsigned clientCount = mClients.size();
mClients.remove(virtualCamera);
+ if (clientCount != mClients.size() + 1) {
+ ALOGE("Couldn't find camera in our client list to remove it");
+ }
virtualCamera->shutdown();
// Recompute the number of buffers required with the target camera removed from the list
@@ -79,7 +87,10 @@
// Walk all our clients and count their currently required frames
unsigned bufferCount = 0;
for (auto&& client : mClients) {
- bufferCount += client->getAllowedBuffers();
+ sp<VirtualCamera> virtCam = client.promote();
+ if (virtCam != nullptr) {
+ bufferCount += virtCam->getAllowedBuffers();
+ }
}
// Add the requested delta
@@ -130,8 +141,11 @@
void HalCamera::clientStreamEnding() {
// Do we still have a running client?
bool stillRunning = false;
- for (auto&& client : mClients) {
- stillRunning |= client->isStreaming();
+ for (auto&& client : mClients) {
+ sp<VirtualCamera> virtCam = client.promote();
+ if (virtCam != nullptr) {
+ stillRunning |= virtCam->isStreaming();
+ }
}
// If not, then stop the hardware stream
@@ -168,8 +182,11 @@
// Run through all our clients and deliver this frame to any who are eligible
unsigned frameDeliveries = 0;
for (auto&& client : mClients) {
- if (client->deliverFrame(buffer)) {
- frameDeliveries++;
+ sp<VirtualCamera> virtCam = client.promote();
+ if (virtCam != nullptr) {
+ if (virtCam->deliverFrame(buffer)) {
+ frameDeliveries++;
+ }
}
}
@@ -188,7 +205,7 @@
if (i == mFrames.size()) {
mFrames.emplace_back(buffer.bufferId);
} else {
- mFrames[i].frameId = buffer.bufferId;
+ mFrames[i].frameId = buffer.bufferId;
}
mFrames[i].refCount = frameDeliveries;
}
diff --git a/evs/manager/HalCamera.h b/evs/manager/HalCamera.h
index c808738..b719de1 100644
--- a/evs/manager/HalCamera.h
+++ b/evs/manager/HalCamera.h
@@ -65,7 +65,7 @@
private:
sp<IEvsCamera> mHwCamera;
- std::list<sp<VirtualCamera>> mClients;
+ std::list<wp<VirtualCamera>> mClients; // Weak pointers -> objects destruct if client dies
enum {
STOPPED,
diff --git a/evs/manager/VirtualCamera.cpp b/evs/manager/VirtualCamera.cpp
index bf5e9d44..b61d772 100644
--- a/evs/manager/VirtualCamera.cpp
+++ b/evs/manager/VirtualCamera.cpp
@@ -30,10 +30,30 @@
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 suddently, the buffer may be stranded.
-// As possible work around would be to give the client a HIDL object to exclusively hold
-// and use it's destructor to perform some work in the server side.
+// If the client dies before closing the camera, this is our chance to clean up...
+VirtualCamera::~VirtualCamera() {
+ // If we have been connected to a camera device we have some cleanup to do...
+ if (mHalCamera != nullptr) {
+ mStreamState = STOPPED;
+
+ if (mFramesHeld.size() > 0) {
+ ALOGW("VirtualCamera destructing with frames in flight.");
+
+ // Return to the underlying hardware camera any buffers the client was holding
+ for (auto&& heldBuffer : mFramesHeld) {
+ // Tell our parent that we're done with this buffer
+ mHalCamera->doneWithFrame(heldBuffer);
+ }
+ mFramesHeld.clear();
+ }
+
+ // Give the underlying hardware camera the heads up that it might be time to stop
+ mHalCamera->clientStreamEnding();
+
+ // Disconnect ourselves from the underlying hardware camera so it can adjust it's resources
+ mHalCamera->disownVirtualCamera(this);
+ }
+}
void VirtualCamera::shutdown() {
@@ -43,6 +63,12 @@
bool VirtualCamera::deliverFrame(const BufferDesc& buffer) {
if (buffer.memHandle == nullptr) {
+ // Warn if we got an unexpected stream termination
+ if (mStreamState != STOPPING) {
+ // TODO: Should we suicide in this case to trigger a restart of the stack?
+ ALOGW("Stream unexpectedly stopped");
+ }
+
// This is the stream end marker, so send it along, then mark the stream as stopped
mStream->deliverFrame(buffer);
mStreamState = STOPPED;
@@ -51,14 +77,16 @@
if (mStreamState == STOPPED) {
// A stopped stream gets no frames
return false;
- } else if (mFramesHeld >= mFramesAllowed) {
+ } else if (mFramesHeld.size() >= mFramesAllowed) {
// Indicate that we declined to send the frame to the client because they're at quota
- ALOGI("Skipping new frame as we hold %d of %d allowed.",
- mFramesHeld, mFramesAllowed);
+ ALOGI("Skipping new frame as we hold %lu of %u allowed.",
+ mFramesHeld.size(), mFramesAllowed);
return false;
} else {
- // Pass this directly through to our client
- mFramesHeld++;
+ // Keep a record of this frame so we can clean up if we have to in case of client death
+ mFramesHeld.push_back(buffer);
+
+ // Pass this buffer through to our client
mStream->deliverFrame(buffer);
return true;
}
@@ -72,6 +100,7 @@
return mHalCamera->getHwCamera()->getId(id_cb);
}
+
Return<EvsResult> VirtualCamera::setMaxFramesInFlight(uint32_t bufferCount) {
// How many buffers are we trying to add (or remove if negative)
int bufferCountChange = bufferCount - mFramesAllowed;
@@ -88,6 +117,7 @@
return EvsResult::OK;
}
+
Return<EvsResult> VirtualCamera::startVideoStream(const ::android::sp<IEvsCameraStream>& stream) {
// We only support a single stream at a time
if (mStreamState != STOPPED) {
@@ -96,7 +126,7 @@
}
// Validate our held frame count is starting out at zero as we expect
- assert(mFramesHeld == 0);
+ assert(mFramesHeld.size() == 0);
// Record the user's callback for use when we have a frame ready
mStream = stream;
@@ -111,29 +141,48 @@
return EvsResult::UNDERLYING_SERVICE_ERROR;
}
+ // TODO: Detect and exit if we encounter a stalled stream or unresponsive driver?
+ // Consider using a timer and watching for frame arrival?
+
return EvsResult::OK;
}
+
Return<void> VirtualCamera::doneWithFrame(const BufferDesc& buffer) {
if (buffer.memHandle == nullptr) {
ALOGE("ignoring doneWithFrame called with invalid handle");
} else {
- // Drop our held buffer count
- mFramesHeld--;
+ // Find this buffer in our "held" list
+ auto it = mFramesHeld.begin();
+ while (it != mFramesHeld.end()) {
+ if (it->bufferId == buffer.bufferId) {
+ // found it!
+ break;
+ }
+ ++it;
+ }
+ if (it == mFramesHeld.end()) {
+ // We should always find the frame in our "held" list
+ ALOGE("Ignoring doneWithFrame called with unrecognized frameID %d", buffer.bufferId);
+ } else {
+ // Take this frame out of our "held" list
+ mFramesHeld.erase(it);
- // Tell our parent that we're done with this buffer
- mHalCamera->doneWithFrame(buffer);
+ // Tell our parent that we're done with this buffer
+ mHalCamera->doneWithFrame(buffer);
+ }
}
return Void();
}
+
Return<void> VirtualCamera::stopVideoStream() {
if (mStreamState == RUNNING) {
// Tell the frame delivery pipeline we don't want any more frames
mStreamState = STOPPING;
- // Deliver an empty frame to close out the frame stream outside the lock
+ // Deliver an empty frame to close out the frame stream
BufferDesc nullBuff = {};
auto result = mStream->deliverFrame(nullBuff);
if (!result.isOk()) {
@@ -153,11 +202,13 @@
return Void();
}
+
Return<int32_t> VirtualCamera::getExtendedInfo(uint32_t opaqueIdentifier) {
// Pass straight through to the hardware device
return mHalCamera->getHwCamera()->getExtendedInfo(opaqueIdentifier);
}
+
Return<EvsResult> VirtualCamera::setExtendedInfo(uint32_t opaqueIdentifier, int32_t opaqueValue) {
// Pass straight through to the hardware device
// TODO: Should we restrict access to this entry point somehow?
diff --git a/evs/manager/VirtualCamera.h b/evs/manager/VirtualCamera.h
index a09dc5f..235445f 100644
--- a/evs/manager/VirtualCamera.h
+++ b/evs/manager/VirtualCamera.h
@@ -22,6 +22,8 @@
#include <ui/GraphicBuffer.h>
#include <thread>
+#include <deque>
+
using namespace ::android::hardware::evs::V1_0;
using ::android::hardware::Return;
@@ -45,6 +47,7 @@
class VirtualCamera : public IEvsCamera {
public:
explicit VirtualCamera(sp<HalCamera> halCamera) : mHalCamera(halCamera) {};
+ virtual ~VirtualCamera();
void shutdown();
sp<HalCamera> getHalCamera() { return mHalCamera; };
@@ -67,7 +70,7 @@
sp<HalCamera> mHalCamera;
sp<IEvsCameraStream> mStream;
- unsigned mFramesHeld = 0;
+ std::deque<BufferDesc> mFramesHeld;
unsigned mFramesAllowed = 1;
enum {
STOPPED,
diff --git a/evs/test/StreamHandler.cpp b/evs/test/StreamHandler.cpp
index 69b251a..92b3d1e 100644
--- a/evs/test/StreamHandler.cpp
+++ b/evs/test/StreamHandler.cpp
@@ -97,7 +97,6 @@
mDisplay->getTargetBuffer([&tgtBuffer]
(const BufferDesc& buff) {
tgtBuffer = buff;
- tgtBuffer.memHandle = native_handle_clone(buff.memHandle);
ALOGD("Got output buffer (%p) with id %d cloned as (%p)",
buff.memHandle.getNativeHandle(),
tgtBuffer.bufferId,
@@ -108,6 +107,11 @@
if (tgtBuffer.memHandle == nullptr) {
ALOGE("Didn't get requested output buffer -- skipping this frame.");
} else {
+ // In order for the handles passed through HIDL and stored in the BufferDesc to
+ // be lockable, we must register them with GraphicBufferMapper
+ registerBufferHelper(tgtBuffer);
+ registerBufferHelper(buffer);
+
// Copy the contents of the of buffer.memHandle into tgtBuffer
copyBufferContents(tgtBuffer, buffer);
@@ -129,13 +133,9 @@
(EvsResult)result);
}
- // Now release our copy of the handle
- // TODO: If we don't end up needing to pass it back, then close our handle earlier
- // As it stands, the buffer might still be held by this process for some time after
- // it gets returned to the server via returnTargetBufferForDisplay()
- // Could/should this be fixed by "exporting" the tgtBuffer before returning it?
- native_handle_close(tgtBuffer.memHandle);
- native_handle_delete(const_cast<native_handle*>(tgtBuffer.memHandle.getNativeHandle()));
+ // Now tell GraphicBufferMapper we won't be using these handles anymore
+ unregisterBufferHelper(tgtBuffer);
+ unregisterBufferHelper(buffer);
}
// Send the camera buffer back now that we're done with it
@@ -203,3 +203,35 @@
return success;
}
+
+
+void StreamHandler::registerBufferHelper(const BufferDesc& buffer)
+{
+ // In order for the handles passed through HIDL and stored in the BufferDesc to
+ // be lockable, we must register them with GraphicBufferMapper.
+ // If the device upon which we're running supports gralloc1, we could just call
+ // registerBuffer directly with the handle. But that call is broken for gralloc0 devices
+ // (which we care about, at least for now). As a result, we have to synthesize a GraphicBuffer
+ // object around the buffer handle in order to make a call to the overloaded alternate
+ // version of the registerBuffer call that does happen to work on gralloc0 devices.
+#if REGISTER_BUFFER_ALWAYS_WORKS
+ android::GraphicBufferMapper::get().registerBuffer(buffer.memHandle);
+#else
+ android::sp<android::GraphicBuffer> pGfxBuff = new android::GraphicBuffer(
+ buffer.width, buffer.height, buffer.format,
+ 1, /* we always use exactly one layer */
+ buffer.usage, buffer.stride,
+ const_cast<native_handle_t*>(buffer.memHandle.getNativeHandle()),
+ false /* GraphicBuffer should not try to free the handle */
+ );
+
+ android::GraphicBufferMapper::get().registerBuffer(pGfxBuff.get());
+#endif
+}
+
+
+void StreamHandler::unregisterBufferHelper(const BufferDesc& buffer)
+{
+ // Now tell GraphicBufferMapper we won't be using these handles anymore
+ android::GraphicBufferMapper::get().unregisterBuffer(buffer.memHandle);
+}
diff --git a/evs/test/StreamHandler.h b/evs/test/StreamHandler.h
index beabb27..8755591 100644
--- a/evs/test/StreamHandler.h
+++ b/evs/test/StreamHandler.h
@@ -46,6 +46,8 @@
// Local implementation details
bool copyBufferContents(const BufferDesc& tgtBuffer, const BufferDesc& srcBuffer);
+ void registerBufferHelper(const BufferDesc& buffer);
+ void unregisterBufferHelper(const BufferDesc& buffer);
android::sp <IEvsCamera> mCamera;
CameraDesc mCameraInfo;