From 4ee35438095692be67245afb5100384b558bac71 Mon Sep 17 00:00:00 2001 From: Yin-Chia Yeh Date: Wed, 10 Oct 2018 13:52:31 -0700 Subject: [PATCH] Camera: synchronize mOutputStreams access mOutputStreams access used to be protected by mLock, but that has changed due to: - Treble interface switched from passing stream pointer to stream index - The buffer management API runs in HAL callback thread Test: CTS Bug: 109829698 Change-Id: I3561b197f46f07d2a15bb4f52b096f36c73a0407 --- .../device3/Camera3Device.cpp | 185 ++++++++++-------- .../libcameraservice/device3/Camera3Device.h | 18 +- 2 files changed, 115 insertions(+), 88 deletions(-) diff --git a/services/camera/libcameraservice/device3/Camera3Device.cpp b/services/camera/libcameraservice/device3/Camera3Device.cpp index 1d40d13c83..04d3639283 100644 --- a/services/camera/libcameraservice/device3/Camera3Device.cpp +++ b/services/camera/libcameraservice/device3/Camera3Device.cpp @@ -986,14 +986,13 @@ hardware::Return Camera3Device::requestStreamBuffers( const auto& bufReq = bufReqs[i]; auto& bufRet = bufRets[i]; int32_t streamId = bufReq.streamId; - ssize_t idx = mOutputStreams.indexOfKey(streamId); - if (idx == NAME_NOT_FOUND) { + sp outputStream = mOutputStreams.get(streamId); + if (outputStream == nullptr) { ALOGE("%s: Output stream id %d not found!", __FUNCTION__, streamId); hardware::hidl_vec emptyBufRets; _hidl_cb(BufferRequestStatus::FAILED_ILLEGAL_ARGUMENTS, emptyBufRets); return hardware::Void(); } - sp outputStream = mOutputStreams.valueAt(idx); bufRet.streamId = streamId; uint32_t numBuffersRequested = bufReq.numBuffersRequested; @@ -1126,15 +1125,12 @@ hardware::Return Camera3Device::returnStreamBuffers( continue; } - // Need to lock mLock here if we were to allow HAL to return buffer during - // stream configuration. This is not currently possible because we only - // do stream configuration when there is no inflight buffers in HAL. - ssize_t idx = mOutputStreams.indexOfKey(buf.streamId); - if (idx == NAME_NOT_FOUND) { + sp stream = mOutputStreams.get(buf.streamId); + if (stream == nullptr) { ALOGE("%s: Output stream id %d not found!", __FUNCTION__, buf.streamId); continue; } - streamBuffer.stream = mOutputStreams.valueAt(idx)->asHalStream(); + streamBuffer.stream = stream->asHalStream(); returnOutputBuffers(&streamBuffer, /*size*/1, /*timestamp*/ 0); } return hardware::Void(); @@ -1288,13 +1284,13 @@ void Camera3Device::processOneCaptureResultLocked( auto& bDst = outputBuffers[i]; const StreamBuffer &bSrc = result.outputBuffers[i]; - ssize_t idx = mOutputStreams.indexOfKey(bSrc.streamId); - if (idx == NAME_NOT_FOUND) { + sp stream = mOutputStreams.get(bSrc.streamId); + if (stream == nullptr) { ALOGE("%s: Frame %d: Buffer %zu: Invalid output stream id %d", __FUNCTION__, result.frameNumber, i, bSrc.streamId); return; } - bDst.stream = mOutputStreams.valueAt(idx)->asHalStream(); + bDst.stream = stream->asHalStream(); buffer_handle_t *buffer; if (mUseHalBufManager) { @@ -1395,13 +1391,13 @@ void Camera3Device::notify( m.type = CAMERA3_MSG_ERROR; m.message.error.frame_number = msg.msg.error.frameNumber; if (msg.msg.error.errorStreamId >= 0) { - ssize_t idx = mOutputStreams.indexOfKey(msg.msg.error.errorStreamId); - if (idx == NAME_NOT_FOUND) { - ALOGE("%s: Frame %d: Invalid error stream id %d", - __FUNCTION__, m.message.error.frame_number, msg.msg.error.errorStreamId); + sp stream = mOutputStreams.get(msg.msg.error.errorStreamId); + if (stream == nullptr) { + ALOGE("%s: Frame %d: Invalid error stream id %d", __FUNCTION__, + m.message.error.frame_number, msg.msg.error.errorStreamId); return; } - m.message.error.error_stream = mOutputStreams.valueAt(idx)->asHalStream(); + m.message.error.error_stream = stream->asHalStream(); } else { m.message.error.error_stream = nullptr; } @@ -1582,6 +1578,47 @@ status_t Camera3Device::createInputStream( return OK; } +status_t Camera3Device::StreamSet::add( + int streamId, sp stream) { + if (stream == nullptr) { + ALOGE("%s: cannot add null stream", __FUNCTION__); + return BAD_VALUE; + } + std::lock_guard lock(mLock); + return mData.add(streamId, stream); +} + +ssize_t Camera3Device::StreamSet::remove(int streamId) { + std::lock_guard lock(mLock); + return mData.removeItem(streamId); +} + +sp +Camera3Device::StreamSet::get(int streamId) { + std::lock_guard lock(mLock); + ssize_t idx = mData.indexOfKey(streamId); + if (idx == NAME_NOT_FOUND) { + return nullptr; + } + return mData.editValueAt(idx); +} + +sp +Camera3Device::StreamSet::operator[] (size_t index) { + std::lock_guard lock(mLock); + return mData.editValueAt(index); +} + +size_t Camera3Device::StreamSet::size() const { + std::lock_guard lock(mLock); + return mData.size(); +} + +void Camera3Device::StreamSet::clear() { + std::lock_guard lock(mLock); + return mData.clear(); +} + status_t Camera3Device::createStream(sp consumer, uint32_t width, uint32_t height, int format, android_dataspace dataSpace, camera3_stream_rotation_t rotation, int *id, @@ -1765,20 +1802,20 @@ status_t Camera3Device::getStreamInfo(int id, StreamInfo *streamInfo) { return INVALID_OPERATION; } - ssize_t idx = mOutputStreams.indexOfKey(id); - if (idx == NAME_NOT_FOUND) { + sp stream = mOutputStreams.get(id); + if (stream == nullptr) { CLOGE("Stream %d is unknown", id); - return idx; + return BAD_VALUE; } - streamInfo->width = mOutputStreams[idx]->getWidth(); - streamInfo->height = mOutputStreams[idx]->getHeight(); - streamInfo->format = mOutputStreams[idx]->getFormat(); - streamInfo->dataSpace = mOutputStreams[idx]->getDataSpace(); - streamInfo->formatOverridden = mOutputStreams[idx]->isFormatOverridden(); - streamInfo->originalFormat = mOutputStreams[idx]->getOriginalFormat(); - streamInfo->dataSpaceOverridden = mOutputStreams[idx]->isDataSpaceOverridden(); - streamInfo->originalDataSpace = mOutputStreams[idx]->getOriginalDataSpace(); + streamInfo->width = stream->getWidth(); + streamInfo->height = stream->getHeight(); + streamInfo->format = stream->getFormat(); + streamInfo->dataSpace = stream->getDataSpace(); + streamInfo->formatOverridden = stream->isFormatOverridden(); + streamInfo->originalFormat = stream->getOriginalFormat(); + streamInfo->dataSpaceOverridden = stream->isDataSpaceOverridden(); + streamInfo->originalDataSpace = stream->getOriginalDataSpace(); return OK; } @@ -1805,14 +1842,12 @@ status_t Camera3Device::setStreamTransform(int id, return INVALID_OPERATION; } - ssize_t idx = mOutputStreams.indexOfKey(id); - if (idx == NAME_NOT_FOUND) { - CLOGE("Stream %d does not exist", - id); + sp stream = mOutputStreams.get(id); + if (stream == nullptr) { + CLOGE("Stream %d does not exist", id); return BAD_VALUE; } - - return mOutputStreams.editValueAt(idx)->setTransform(transform); + return stream->setTransform(transform); } status_t Camera3Device::deleteStream(int id) { @@ -1837,21 +1872,21 @@ status_t Camera3Device::deleteStream(int id) { } sp deletedStream; - ssize_t outputStreamIdx = mOutputStreams.indexOfKey(id); + sp stream = mOutputStreams.get(id); if (mInputStream != NULL && id == mInputStream->getId()) { deletedStream = mInputStream; mInputStream.clear(); } else { - if (outputStreamIdx == NAME_NOT_FOUND) { + if (stream == nullptr) { CLOGE("Stream %d does not exist", id); return BAD_VALUE; } } // Delete output stream or the output part of a bi-directional stream. - if (outputStreamIdx != NAME_NOT_FOUND) { - deletedStream = mOutputStreams.editValueAt(outputStreamIdx); - mOutputStreams.removeItem(id); + if (stream != nullptr) { + deletedStream = stream; + mOutputStreams.remove(id); } // Free up the stream endpoint so that it can be used by some other stream @@ -2270,15 +2305,12 @@ status_t Camera3Device::prepare(int maxCount, int streamId) { Mutex::Autolock il(mInterfaceLock); Mutex::Autolock l(mLock); - sp stream; - ssize_t outputStreamIdx = mOutputStreams.indexOfKey(streamId); - if (outputStreamIdx == NAME_NOT_FOUND) { + sp stream = mOutputStreams.get(streamId); + if (stream == nullptr) { CLOGE("Stream %d does not exist", streamId); return BAD_VALUE; } - stream = mOutputStreams.editValueAt(outputStreamIdx); - if (stream->isUnpreparable() || stream->hasOutstandingBuffers() ) { CLOGE("Stream %d has already been a request target", streamId); return BAD_VALUE; @@ -2298,15 +2330,12 @@ status_t Camera3Device::tearDown(int streamId) { Mutex::Autolock il(mInterfaceLock); Mutex::Autolock l(mLock); - sp stream; - ssize_t outputStreamIdx = mOutputStreams.indexOfKey(streamId); - if (outputStreamIdx == NAME_NOT_FOUND) { + sp stream = mOutputStreams.get(streamId); + if (stream == nullptr) { CLOGE("Stream %d does not exist", streamId); return BAD_VALUE; } - stream = mOutputStreams.editValueAt(outputStreamIdx); - if (stream->hasOutstandingBuffers() || mRequestThread->isStreamPending(stream)) { CLOGE("Stream %d is a target of a in-progress request", streamId); return BAD_VALUE; @@ -2322,14 +2351,11 @@ status_t Camera3Device::addBufferListenerForStream(int streamId, Mutex::Autolock il(mInterfaceLock); Mutex::Autolock l(mLock); - sp stream; - ssize_t outputStreamIdx = mOutputStreams.indexOfKey(streamId); - if (outputStreamIdx == NAME_NOT_FOUND) { + sp stream = mOutputStreams.get(streamId); + if (stream == nullptr) { CLOGE("Stream %d does not exist", streamId); return BAD_VALUE; } - - stream = mOutputStreams.editValueAt(outputStreamIdx); stream->addBufferListener(listener); return OK; @@ -2388,12 +2414,11 @@ status_t Camera3Device::setConsumerSurfaces(int streamId, return BAD_VALUE; } - ssize_t idx = mOutputStreams.indexOfKey(streamId); - if (idx == NAME_NOT_FOUND) { + sp stream = mOutputStreams.get(streamId); + if (stream == nullptr) { CLOGE("Stream %d is unknown", streamId); - return idx; + return BAD_VALUE; } - sp stream = mOutputStreams[idx]; status_t res = stream->setConsumers(consumers); if (res != OK) { CLOGE("Stream %d set consumer failed (error %d %s) ", streamId, res, strerror(-res)); @@ -2438,10 +2463,10 @@ status_t Camera3Device::updateStream(int streamId, const std::vector Mutex::Autolock il(mInterfaceLock); Mutex::Autolock l(mLock); - ssize_t idx = mOutputStreams.indexOfKey(streamId); - if (idx == NAME_NOT_FOUND) { + sp stream = mOutputStreams.get(streamId); + if (stream == nullptr) { CLOGE("Stream %d is unknown", streamId); - return idx; + return BAD_VALUE; } for (const auto &it : removedSurfaceIds) { @@ -2451,7 +2476,6 @@ status_t Camera3Device::updateStream(int streamId, const std::vector } } - sp stream = mOutputStreams[idx]; status_t res = stream->updateStream(newSurfaces, outputInfo, removedSurfaceIds, outputMap); if (res != OK) { CLOGE("Stream %d failed to update stream (error %d %s) ", @@ -2470,13 +2494,11 @@ status_t Camera3Device::dropStreamBuffers(bool dropping, int streamId) { Mutex::Autolock il(mInterfaceLock); Mutex::Autolock l(mLock); - int idx = mOutputStreams.indexOfKey(streamId); - if (idx == NAME_NOT_FOUND) { + sp stream = mOutputStreams.get(streamId); + if (stream == nullptr) { ALOGE("%s: Stream %d is not found.", __FUNCTION__, streamId); return BAD_VALUE; } - - sp stream = mOutputStreams.editValueAt(idx); return stream->dropBuffers(dropping); } @@ -2530,15 +2552,12 @@ sp Camera3Device::createCaptureRequest( } for (size_t i = 0; i < streams.count; i++) { - int idx = mOutputStreams.indexOfKey(streams.data.i32[i]); - if (idx == NAME_NOT_FOUND) { + sp stream = mOutputStreams.get(streams.data.i32[i]); + if (stream == nullptr) { CLOGE("Request references unknown stream %d", - streams.data.u8[i]); + streams.data.i32[i]); return NULL; } - sp stream = - mOutputStreams.editValueAt(idx); - // It is illegal to include a deferred consumer output stream into a request auto iter = surfaceMap.find(streams.data.i32[i]); if (iter != surfaceMap.end()) { @@ -2599,7 +2618,7 @@ void Camera3Device::cancelStreamsConfigurationLocked() { } for (size_t i = 0; i < mOutputStreams.size(); i++) { - sp outputStream = mOutputStreams.editValueAt(i); + sp outputStream = mOutputStreams[i]; if (outputStream->isConfiguring()) { res = outputStream->cancelConfiguration(); if (res != OK) { @@ -2734,7 +2753,7 @@ status_t Camera3Device::configureStreamsLocked(int operatingMode, } camera3_stream_t *outputStream; - outputStream = mOutputStreams.editValueAt(i)->startConfiguration(); + outputStream = mOutputStreams[i]->startConfiguration(); if (outputStream == NULL) { CLOGE("Can't start output stream configuration"); cancelStreamsConfigurationLocked(); @@ -2792,8 +2811,7 @@ status_t Camera3Device::configureStreamsLocked(int operatingMode, } for (size_t i = 0; i < mOutputStreams.size(); i++) { - sp outputStream = - mOutputStreams.editValueAt(i); + sp outputStream = mOutputStreams[i]; if (outputStream->isConfiguring() && !outputStream->isConsumerConfigurationDeferred()) { res = outputStream->finishConfiguration(); if (res != OK) { @@ -2900,15 +2918,12 @@ status_t Camera3Device::tryRemoveDummyStreamLocked() { // Ok, have a dummy stream and there's at least one other output stream, // so remove the dummy - sp deletedStream; - ssize_t outputStreamIdx = mOutputStreams.indexOfKey(mDummyStreamId); - if (outputStreamIdx == NAME_NOT_FOUND) { + sp deletedStream = mOutputStreams.get(mDummyStreamId); + if (deletedStream == nullptr) { SET_ERR_L("Dummy stream %d does not appear to exist", mDummyStreamId); return INVALID_OPERATION; } - - deletedStream = mOutputStreams.editValueAt(outputStreamIdx); - mOutputStreams.removeItemsAt(outputStreamIdx); + mOutputStreams.remove(mDummyStreamId); // Free up the stream endpoint so that it can be used by some other stream res = deletedStream->disconnect(); @@ -3175,12 +3190,12 @@ void Camera3Device::flushInflightRequests() { frameNumber, streamId, strerror(-res), res); } } else { - ssize_t idx = mOutputStreams.indexOfKey(streamId); - if (idx == NAME_NOT_FOUND) { + sp stream = mOutputStreams.get(streamId); + if (stream == nullptr) { ALOGE("%s: Output stream id %d not found!", __FUNCTION__, streamId); continue; } - streamBuffer.stream = mOutputStreams.valueAt(idx)->asHalStream(); + streamBuffer.stream = stream->asHalStream(); returnOutputBuffers(&streamBuffer, /*size*/1, /*timestamp*/ 0); } } diff --git a/services/camera/libcameraservice/device3/Camera3Device.h b/services/camera/libcameraservice/device3/Camera3Device.h index 7656ef33cd..4ed6d9dd6b 100644 --- a/services/camera/libcameraservice/device3/Camera3Device.h +++ b/services/camera/libcameraservice/device3/Camera3Device.h @@ -406,9 +406,21 @@ class Camera3Device : // Tracking cause of fatal errors when in STATUS_ERROR String8 mErrorCause; - // Mapping of stream IDs to stream instances - typedef KeyedVector > - StreamSet; + // Synchronized mapping of stream IDs to stream instances + class StreamSet { + public: + status_t add(int streamId, sp); + ssize_t remove(int streamId); + sp get(int streamId); + // get by (underlying) vector index + sp operator[] (size_t index); + size_t size() const; + void clear(); + + private: + mutable std::mutex mLock; + KeyedVector> mData; + }; StreamSet mOutputStreams; sp mInputStream;