diff --git a/camera/device/3.2/default/CameraDevice.cpp b/camera/device/3.2/default/CameraDevice.cpp index 51a29ec7b6..18e0e7bb87 100644 --- a/camera/device/3.2/default/CameraDevice.cpp +++ b/camera/device/3.2/default/CameraDevice.cpp @@ -212,7 +212,7 @@ Return CameraDevice::open(const sp& callback, open_ if (res != OK) { ALOGE("%s: cannot open camera %s!", __FUNCTION__, mCameraId.c_str()); mLock.unlock(); - _hidl_cb(Status::INTERNAL_ERROR, nullptr); + _hidl_cb(getHidlStatus(res), nullptr); return Void(); } diff --git a/camera/device/3.2/default/CameraDeviceSession.cpp b/camera/device/3.2/default/CameraDeviceSession.cpp index 7e43cd7aa7..de61d83164 100644 --- a/camera/device/3.2/default/CameraDeviceSession.cpp +++ b/camera/device/3.2/default/CameraDeviceSession.cpp @@ -94,7 +94,12 @@ public: if (handle == nullptr || handle->numFds == 0) { fd = -1; } else if (handle->numFds == 1) { +//TODO(b/34110242): make this hidl transport agnostic +#ifdef BINDERIZED fd = dup(handle->data[0]); +#else + fd = handle->data[0]; +#endif if (fd < 0) { ALOGE("failed to dup fence fd %d", handle->data[0]); return false; @@ -110,9 +115,13 @@ public: void closeFence(int fd) { +#ifdef BINDERIZED if (fd >= 0) { close(fd); } +#else + (void) fd; +#endif } private: @@ -226,7 +235,6 @@ CameraDeviceSession::CameraDeviceSession( camera3_callback_ops({&sProcessCaptureResult, &sNotify}), mDevice(device), mCallback(callback) { - // For now, we init sHandleImporter but do not cleanup (keep it alive until // HAL process ends) sHandleImporter.initialize(); @@ -293,31 +301,45 @@ void CameraDeviceSession::dumpState(const native_handle_t* fd) { Status CameraDeviceSession::importRequest( const CaptureRequest& request, - hidl_vec& allBufs, + hidl_vec& allBufPtrs, hidl_vec& allFences) { bool hasInputBuf = (request.inputBuffer.streamId != -1 && - request.inputBuffer.buffer.getNativeHandle() == nullptr); + request.inputBuffer.buffer.getNativeHandle() != nullptr); size_t numOutputBufs = request.outputBuffers.size(); size_t numBufs = numOutputBufs + (hasInputBuf ? 1 : 0); // Validate all I/O buffers + hidl_vec allBufs; allBufs.resize(numBufs); + allBufPtrs.resize(numBufs); allFences.resize(numBufs); + std::vector streamIds(numBufs); + for (size_t i = 0; i < numOutputBufs; i++) { allBufs[i] = request.outputBuffers[i].buffer.getNativeHandle(); + allBufPtrs[i] = &allBufs[i]; + streamIds[i] = request.outputBuffers[i].streamId; } if (hasInputBuf) { allBufs[numOutputBufs] = request.inputBuffer.buffer.getNativeHandle(); + allBufPtrs[numOutputBufs] = &allBufs[numOutputBufs]; + streamIds[numOutputBufs] = request.inputBuffer.streamId; } + for (size_t i = 0; i < numBufs; i++) { buffer_handle_t buf = allBufs[i]; - sHandleImporter.importBuffer(buf); - if (buf == nullptr) { - ALOGE("%s: output buffer %zu is invalid!", __FUNCTION__, i); - cleanupInflightBufferFences(allBufs, i, allFences, 0); - return Status::INTERNAL_ERROR; - } else { - allBufs[i] = buf; + CirculatingBuffers& cbs = mCirculatingBuffers[streamIds[i]]; + if (cbs.count(buf) == 0) { + // Register a newly seen buffer + buffer_handle_t importedBuf = buf; + sHandleImporter.importBuffer(importedBuf); + if (importedBuf == nullptr) { + ALOGE("%s: output buffer %zu is invalid!", __FUNCTION__, i); + return Status::INTERNAL_ERROR; + } else { + cbs[buf] = importedBuf; + } } + allBufPtrs[i] = &cbs[buf]; } // All buffers are imported. Now validate output buffer acquire fences @@ -325,7 +347,7 @@ Status CameraDeviceSession::importRequest( if (!sHandleImporter.importFence( request.outputBuffers[i].acquireFence, allFences[i])) { ALOGE("%s: output buffer %zu acquire fence is invalid", __FUNCTION__, i); - cleanupInflightBufferFences(allBufs, numBufs, allFences, i); + cleanupInflightFences(allFences, i); return Status::INTERNAL_ERROR; } } @@ -335,19 +357,15 @@ Status CameraDeviceSession::importRequest( if (!sHandleImporter.importFence( request.inputBuffer.acquireFence, allFences[numOutputBufs])) { ALOGE("%s: input buffer acquire fence is invalid", __FUNCTION__); - cleanupInflightBufferFences(allBufs, numBufs, allFences, numOutputBufs); + cleanupInflightFences(allFences, numOutputBufs); return Status::INTERNAL_ERROR; } } return Status::OK; } -void CameraDeviceSession::cleanupInflightBufferFences( - hidl_vec& allBufs, size_t numBufs, +void CameraDeviceSession::cleanupInflightFences( hidl_vec& allFences, size_t numFences) { - for (size_t j = 0; j < numBufs; j++) { - sHandleImporter.freeBuffer(allBufs[j]); - } for (size_t j = 0; j < numFences; j++) { sHandleImporter.closeFence(allFences[j]); } @@ -378,13 +396,20 @@ Return CameraDeviceSession::constructDefaultRequestSettings( Return CameraDeviceSession::configureStreams( const StreamConfiguration& requestedConfiguration, configureStreams_cb _hidl_cb) { Status status = initStatus(); + HalStreamConfiguration outStreams; + + // hold the inflight lock for entire configureStreams scope since there must not be any + // inflight request/results during stream configuration. + Mutex::Autolock _l(mInflightLock); if (!mInflightBuffers.empty()) { ALOGE("%s: trying to configureStreams while there are still %zu inflight buffers!", __FUNCTION__, mInflightBuffers.size()); - status = Status::INTERNAL_ERROR; + _hidl_cb(Status::INTERNAL_ERROR, outStreams); + return Void(); } - HalStreamConfiguration outStreams; + + if (status == Status::OK) { camera3_stream_configuration_t stream_list; hidl_vec streams; @@ -394,18 +419,62 @@ Return CameraDeviceSession::configureStreams( streams.resize(stream_list.num_streams); stream_list.streams = streams.data(); - mStreamMap.clear(); for (uint32_t i = 0; i < stream_list.num_streams; i++) { - Camera3Stream stream; - convertFromHidl(requestedConfiguration.streams[i], &stream); - mStreamMap[stream.mId] = stream; - streams[i] = &mStreamMap[stream.mId]; + int id = requestedConfiguration.streams[i].id; + + if (mStreamMap.count(id) == 0) { + Camera3Stream stream; + convertFromHidl(requestedConfiguration.streams[i], &stream); + mStreamMap[id] = stream; + mCirculatingBuffers.emplace(stream.mId, CirculatingBuffers{}); + } else { + // width/height/format must not change, but usage/rotation might need to change + if (mStreamMap[id].stream_type != + (int) requestedConfiguration.streams[i].streamType || + mStreamMap[id].width != requestedConfiguration.streams[i].width || + mStreamMap[id].height != requestedConfiguration.streams[i].height || + mStreamMap[id].format != (int) requestedConfiguration.streams[i].format || + mStreamMap[id].data_space != (android_dataspace_t) + requestedConfiguration.streams[i].dataSpace) { + ALOGE("%s: stream %d configuration changed!", __FUNCTION__, id); + _hidl_cb(Status::INTERNAL_ERROR, outStreams); + return Void(); + } + mStreamMap[id].rotation = (int) requestedConfiguration.streams[i].rotation; + mStreamMap[id].usage = (uint32_t) requestedConfiguration.streams[i].usage; + } + streams[i] = &mStreamMap[id]; } ATRACE_BEGIN("camera3->configure_streams"); status_t ret = mDevice->ops->configure_streams(mDevice, &stream_list); ATRACE_END(); + // delete unused streams, note we do this after adding new streams to ensure new stream + // will not have the same address as deleted stream, and HAL has a chance to reference + // the to be deleted stream in configure_streams call + for(auto it = mStreamMap.begin(); it != mStreamMap.end();) { + int id = it->first; + bool found = false; + for (const auto& stream : requestedConfiguration.streams) { + if (id == stream.id) { + found = true; + break; + } + } + if (!found) { + // Unmap all buffers of deleted stream + for (auto& pair : mCirculatingBuffers.at(id)) { + sHandleImporter.freeBuffer(pair.second); + } + mCirculatingBuffers[id].clear(); + mCirculatingBuffers.erase(id); + it = mStreamMap.erase(it); + } else { + ++it; + } + } + if (ret == -EINVAL) { status = Status::ILLEGAL_ARGUMENT; } else if (ret != OK) { @@ -434,57 +503,55 @@ Return CameraDeviceSession::processCaptureRequest(const CaptureRequest& return Status::INTERNAL_ERROR; } - hidl_vec allBufs; + hidl_vec allBufPtrs; hidl_vec allFences; bool hasInputBuf = (request.inputBuffer.streamId != -1 && - request.inputBuffer.buffer.getNativeHandle() == nullptr); + request.inputBuffer.buffer.getNativeHandle() != nullptr); size_t numOutputBufs = request.outputBuffers.size(); size_t numBufs = numOutputBufs + (hasInputBuf ? 1 : 0); - status = importRequest(request, allBufs, allFences); + status = importRequest(request, allBufPtrs, allFences); if (status != Status::OK) { return status; } - if (hasInputBuf) { - auto key = std::make_pair(request.inputBuffer.streamId, request.frameNumber); - auto& bufCache = mInflightBuffers[key] = StreamBufferCache{}; - // The last parameter (&bufCache) must be in heap, or we will - // send a pointer pointing to stack memory to HAL and later HAL will break - // when trying to accessing it after this call returned. - convertFromHidl( - allBufs[numOutputBufs], request.inputBuffer.status, - &mStreamMap[request.inputBuffer.streamId], allFences[numOutputBufs], - &bufCache); - halRequest.input_buffer = &(bufCache.mStreamBuffer); - } else { - halRequest.input_buffer = nullptr; - } - - halRequest.num_output_buffers = numOutputBufs; hidl_vec outHalBufs; outHalBufs.resize(numOutputBufs); - for (size_t i = 0; i < numOutputBufs; i++) { - auto key = std::make_pair(request.outputBuffers[i].streamId, request.frameNumber); - auto& bufCache = mInflightBuffers[key] = StreamBufferCache{}; - // The last parameter (&bufCache) must be in heap, or we will - // send a pointer pointing to stack memory to HAL and later HAL will break - // when trying to accessing it after this call returned. - convertFromHidl( - allBufs[i], request.outputBuffers[i].status, - &mStreamMap[request.outputBuffers[i].streamId], allFences[i], - &bufCache); - outHalBufs[i] = bufCache.mStreamBuffer; + { + Mutex::Autolock _l(mInflightLock); + if (hasInputBuf) { + auto key = std::make_pair(request.inputBuffer.streamId, request.frameNumber); + auto& bufCache = mInflightBuffers[key] = camera3_stream_buffer_t{}; + convertFromHidl( + allBufPtrs[numOutputBufs], request.inputBuffer.status, + &mStreamMap[request.inputBuffer.streamId], allFences[numOutputBufs], + &bufCache); + halRequest.input_buffer = &bufCache; + } else { + halRequest.input_buffer = nullptr; + } + + halRequest.num_output_buffers = numOutputBufs; + for (size_t i = 0; i < numOutputBufs; i++) { + auto key = std::make_pair(request.outputBuffers[i].streamId, request.frameNumber); + auto& bufCache = mInflightBuffers[key] = camera3_stream_buffer_t{}; + convertFromHidl( + allBufPtrs[i], request.outputBuffers[i].status, + &mStreamMap[request.outputBuffers[i].streamId], allFences[i], + &bufCache); + outHalBufs[i] = bufCache; + } + halRequest.output_buffers = outHalBufs.data(); } - halRequest.output_buffers = outHalBufs.data(); ATRACE_ASYNC_BEGIN("frame capture", request.frameNumber); ATRACE_BEGIN("camera3->process_capture_request"); status_t ret = mDevice->ops->process_capture_request(mDevice, &halRequest); ATRACE_END(); if (ret != OK) { + Mutex::Autolock _l(mInflightLock); ALOGE("%s: HAL process_capture_request call failed!", __FUNCTION__); - cleanupInflightBufferFences(allBufs, numBufs, allFences, numBufs); + cleanupInflightFences(allFences, numBufs); if (hasInputBuf) { auto key = std::make_pair(request.inputBuffer.streamId, request.frameNumber); mInflightBuffers.erase(key); @@ -514,9 +581,26 @@ Return CameraDeviceSession::flush() { Return CameraDeviceSession::close() { Mutex::Autolock _l(mStateLock); if (!mClosed) { + { + Mutex::Autolock _l(mInflightLock); + if (!mInflightBuffers.empty()) { + ALOGE("%s: trying to close while there are still %zu inflight buffers!", + __FUNCTION__, mInflightBuffers.size()); + } + } + ATRACE_BEGIN("camera3->close"); mDevice->common.close(&mDevice->common); ATRACE_END(); + + // free all imported buffers + for(auto& pair : mCirculatingBuffers) { + CirculatingBuffers& buffers = pair.second; + for (auto& p2 : buffers) { + sHandleImporter.freeBuffer(p2.second); + } + } + mClosed = true; } return Void(); @@ -536,25 +620,28 @@ void CameraDeviceSession::sProcessCaptureResult( size_t numOutputBufs = hal_result->num_output_buffers; size_t numBufs = numOutputBufs + (hasInputBuf ? 1 : 0); Status status = Status::OK; - if (hasInputBuf) { - int streamId = static_cast(hal_result->input_buffer->stream)->mId; - // validate if buffer is inflight - auto key = std::make_pair(streamId, frameNumber); - if (d->mInflightBuffers.count(key) != 1) { - ALOGE("%s: input buffer for stream %d frame %d is not inflight!", - __FUNCTION__, streamId, frameNumber); - return; + { + Mutex::Autolock _l(d->mInflightLock); + if (hasInputBuf) { + int streamId = static_cast(hal_result->input_buffer->stream)->mId; + // validate if buffer is inflight + auto key = std::make_pair(streamId, frameNumber); + if (d->mInflightBuffers.count(key) != 1) { + ALOGE("%s: input buffer for stream %d frame %d is not inflight!", + __FUNCTION__, streamId, frameNumber); + return; + } } - } - for (size_t i = 0; i < numOutputBufs; i++) { - int streamId = static_cast(hal_result->output_buffers[i].stream)->mId; - // validate if buffer is inflight - auto key = std::make_pair(streamId, frameNumber); - if (d->mInflightBuffers.count(key) != 1) { - ALOGE("%s: output buffer for stream %d frame %d is not inflight!", - __FUNCTION__, streamId, frameNumber); - return; + for (size_t i = 0; i < numOutputBufs; i++) { + int streamId = static_cast(hal_result->output_buffers[i].stream)->mId; + // validate if buffer is inflight + auto key = std::make_pair(streamId, frameNumber); + if (d->mInflightBuffers.count(key) != 1) { + ALOGE("%s: output buffer for stream %d frame %d is not inflight!", + __FUNCTION__, streamId, frameNumber); + return; + } } } // We don't need to validate/import fences here since we will be passing them to camera service @@ -576,6 +663,8 @@ void CameraDeviceSession::sProcessCaptureResult( releaseFences[numOutputBufs] = native_handle_create(/*numFds*/1, /*numInts*/0); releaseFences[numOutputBufs]->data[0] = hal_result->input_buffer->release_fence; result.inputBuffer.releaseFence = releaseFences[numOutputBufs]; + } else { + releaseFences[numOutputBufs] = nullptr; } } else { result.inputBuffer.streamId = -1; @@ -592,33 +681,42 @@ void CameraDeviceSession::sProcessCaptureResult( releaseFences[i] = native_handle_create(/*numFds*/1, /*numInts*/0); releaseFences[i]->data[0] = hal_result->output_buffers[i].release_fence; result.outputBuffers[i].releaseFence = releaseFences[i]; + } else { + releaseFences[i] = nullptr; + } + } + + // Free inflight record/fences. + // Do this before call back to camera service because camera service might jump to + // configure_streams right after the processCaptureResult call so we need to finish + // updating inflight queues first + { + Mutex::Autolock _l(d->mInflightLock); + if (hasInputBuf) { + int streamId = static_cast(hal_result->input_buffer->stream)->mId; + auto key = std::make_pair(streamId, frameNumber); + sHandleImporter.closeFence(d->mInflightBuffers[key].acquire_fence); + d->mInflightBuffers.erase(key); + } + + for (size_t i = 0; i < numOutputBufs; i++) { + int streamId = static_cast(hal_result->output_buffers[i].stream)->mId; + auto key = std::make_pair(streamId, frameNumber); + sHandleImporter.closeFence(d->mInflightBuffers[key].acquire_fence); + d->mInflightBuffers.erase(key); + } + + if (d->mInflightBuffers.empty()) { + ALOGV("%s: inflight buffer queue is now empty!", __FUNCTION__); } } d->mCallback->processCaptureResult(result); - // Free cached buffer/fences. - if (hasInputBuf) { - int streamId = static_cast(hal_result->input_buffer->stream)->mId; - auto key = std::make_pair(streamId, frameNumber); - sHandleImporter.closeFence(d->mInflightBuffers[key].mStreamBuffer.acquire_fence); - sHandleImporter.freeBuffer(d->mInflightBuffers[key].mBuffer); - d->mInflightBuffers.erase(key); - } - - for (size_t i = 0; i < numOutputBufs; i++) { - int streamId = static_cast(hal_result->output_buffers[i].stream)->mId; - auto key = std::make_pair(streamId, frameNumber); - sHandleImporter.closeFence(d->mInflightBuffers[key].mStreamBuffer.acquire_fence); - sHandleImporter.freeBuffer(d->mInflightBuffers[key].mBuffer); - d->mInflightBuffers.erase(key); - } - for (size_t i = 0; i < releaseFences.size(); i++) { // We don't close the FD here as HAL needs to signal it later. native_handle_delete(releaseFences[i]); } - } void CameraDeviceSession::sNotify( @@ -628,7 +726,8 @@ void CameraDeviceSession::sNotify( const_cast(static_cast(cb)); NotifyMsg hidlMsg; convertToHidl(msg, &hidlMsg); - if (hidlMsg.type == (MsgType) CAMERA3_MSG_ERROR) { + if (hidlMsg.type == (MsgType) CAMERA3_MSG_ERROR && + hidlMsg.msg.error.errorStreamId != -1) { if (d->mStreamMap.count(hidlMsg.msg.error.errorStreamId) != 1) { ALOGE("%s: unknown stream ID %d reports an error!", __FUNCTION__, hidlMsg.msg.error.errorStreamId); diff --git a/camera/device/3.2/default/CameraDeviceSession.h b/camera/device/3.2/default/CameraDeviceSession.h index 3e01b1a368..498617ee0f 100644 --- a/camera/device/3.2/default/CameraDeviceSession.h +++ b/camera/device/3.2/default/CameraDeviceSession.h @@ -17,6 +17,7 @@ #ifndef ANDROID_HARDWARE_CAMERA_DEVICE_V3_2_CAMERADEVICE3SESSION_H #define ANDROID_HARDWARE_CAMERA_DEVICE_V3_2_CAMERADEVICE3SESSION_H +#include #include "hardware/camera_common.h" #include "hardware/camera3.h" #include "utils/Mutex.h" @@ -91,11 +92,54 @@ private: bool mDisconnected = false; camera3_device_t* mDevice; - const sp& mCallback; + const sp mCallback; // Stream ID -> Camera3Stream cache std::map mStreamMap; + + mutable Mutex mInflightLock; // protecting mInflightBuffers and mCirculatingBuffers // (streamID, frameNumber) -> inflight buffer cache - std::map, StreamBufferCache> mInflightBuffers; + std::map, camera3_stream_buffer_t> mInflightBuffers; + + struct BufferHasher { + size_t operator()(const buffer_handle_t& buf) const { + if (buf == nullptr) + return 0; + + size_t result = 1; + result = 31 * result + buf->numFds; + result = 31 * result + buf->numInts; + int length = buf->numFds + buf->numInts; + for (int i = 0; i < length; i++) { + result = 31 * result + buf->data[i]; + } + return result; + } + }; + + struct BufferComparator { + bool operator()(const buffer_handle_t& buf1, const buffer_handle_t& buf2) const { + if (buf1->numFds == buf2->numFds && buf1->numInts == buf2->numInts) { + int length = buf1->numFds + buf1->numInts; + for (int i = 0; i < length; i++) { + if (buf1->data[i] != buf2->data[i]) { + return false; + } + } + return true; + } + return false; + } + }; + + // buffers currently ciculating between HAL and camera service + // key: buffer_handle_t sent via HIDL interface + // value: imported buffer_handle_t + // Buffer will be imported during process_capture_request and will be freed + // when the its stream is deleted or camera device session is closed + typedef std::unordered_map CirculatingBuffers; + // Stream ID -> circulating buffers map + std::map mCirculatingBuffers; bool mInitFail; bool initialize(); @@ -103,13 +147,12 @@ private: Status initStatus() const; // Validate and import request's input buffer and acquire fence - static Status importRequest( + Status importRequest( const CaptureRequest& request, - hidl_vec& allBufs, + hidl_vec& allBufPtrs, hidl_vec& allFences); - static void cleanupInflightBufferFences( - hidl_vec& allBufs, size_t numBufs, + static void cleanupInflightFences( hidl_vec& allFences, size_t numFences); /** diff --git a/camera/device/3.2/default/convert.cpp b/camera/device/3.2/default/convert.cpp index ae83e7dfe2..35676df3af 100644 --- a/camera/device/3.2/default/convert.cpp +++ b/camera/device/3.2/default/convert.cpp @@ -50,6 +50,9 @@ bool convertFromHidl(const CameraMetadata &src, const camera_metadata_t** dst) { // Note: existing data in dst will be gone. Caller still owns the memory of src void convertToHidl(const camera_metadata_t *src, CameraMetadata* dst) { + if (src == nullptr) { + return; + } size_t size = get_camera_metadata_size(src); dst->setToExternal((uint8_t *) src, size); return; @@ -97,27 +100,29 @@ void convertToHidl(const camera3_stream_configuration_t& src, HalStreamConfigura } void convertFromHidl( - buffer_handle_t bufIn, BufferStatus status, camera3_stream_t* stream, int acquireFence, - StreamBufferCache* dst) { - dst->mBuffer = bufIn; - dst->mStreamBuffer.stream = stream; - dst->mStreamBuffer.buffer = &dst->mBuffer; - dst->mStreamBuffer.status = (int) status; - dst->mStreamBuffer.acquire_fence = acquireFence; - dst->mStreamBuffer.release_fence = -1; // meant for HAL to fill in + buffer_handle_t* bufPtr, BufferStatus status, camera3_stream_t* stream, int acquireFence, + camera3_stream_buffer_t* dst) { + dst->stream = stream; + dst->buffer = bufPtr; + dst->status = (int) status; + dst->acquire_fence = acquireFence; + dst->release_fence = -1; // meant for HAL to fill in } void convertToHidl(const camera3_notify_msg* src, NotifyMsg* dst) { dst->type = (MsgType) src->type; switch (src->type) { case CAMERA3_MSG_ERROR: - dst->msg.error.frameNumber = src->message.error.frame_number; - // The camera3_stream_t* must be the same as what wrapper HAL passed to conventional - // HAL, or the ID lookup will return garbage. Caller should validate the ID here is - // indeed one of active stream IDs - dst->msg.error.errorStreamId = - static_cast(src->message.error.error_stream)->mId; - dst->msg.error.errorCode = (ErrorCode) src->message.error.error_code; + { + // The camera3_stream_t* must be the same as what wrapper HAL passed to conventional + // HAL, or the ID lookup will return garbage. Caller should validate the ID here is + // indeed one of active stream IDs + Camera3Stream* stream = static_cast( + src->message.error.error_stream); + dst->msg.error.frameNumber = src->message.error.frame_number; + dst->msg.error.errorStreamId = (stream != nullptr) ? stream->mId : -1; + dst->msg.error.errorCode = (ErrorCode) src->message.error.error_code; + } break; case CAMERA3_MSG_SHUTTER: dst->msg.shutter.frameNumber = src->message.shutter.frame_number; diff --git a/camera/device/3.2/default/include/convert.h b/camera/device/3.2/default/include/convert.h index f5ea56459f..96891f091d 100644 --- a/camera/device/3.2/default/include/convert.h +++ b/camera/device/3.2/default/include/convert.h @@ -32,16 +32,6 @@ namespace device { namespace V3_2 { namespace implementation { -// Cacheing the buffer/fence from camera service so HAL can reference the pointer after the -// processCaptureRequest call has returned. -// Remove the cache when: -// 1. HAL API call failed, or -// 2. HAL returns the buffer and the callback to camera service has returned -struct StreamBufferCache { - buffer_handle_t mBuffer; - camera3_stream_buffer_t mStreamBuffer; -}; - // The camera3_stream_t sent to conventional HAL. Added mId fields to enable stream ID lookup // fromt a downcasted camera3_stream struct Camera3Stream : public camera3_stream { @@ -55,14 +45,9 @@ void convertToHidl(const camera_metadata_t* src, CameraMetadata* dst); void convertFromHidl(const Stream &src, Camera3Stream* dst); void convertToHidl(const Camera3Stream* src, HalStream* dst); -// dst->mStreamBuffer.buffer will be pointing to dst->mBuffer. -// Most likely dst will be passed to HAL and HAL will try to access mStreamBuffer.buffer -// after the API call returns. In that case caller must not use a local variable -// within the scope of the API call to hold dst, because then dst->mStreamBuffer.buffer will be -// invalid after the API call returns. void convertFromHidl( - buffer_handle_t, BufferStatus, camera3_stream_t*, int acquireFence, // inputs - StreamBufferCache* dst); + buffer_handle_t*, BufferStatus, camera3_stream_t*, int acquireFence, // inputs + camera3_stream_buffer_t* dst); void convertToHidl(const camera3_stream_configuration_t& src, HalStreamConfiguration* dst); diff --git a/camera/provider/2.4/default/CameraProvider.cpp b/camera/provider/2.4/default/CameraProvider.cpp index d9ac26168e..9617d8d0d4 100644 --- a/camera/provider/2.4/default/CameraProvider.cpp +++ b/camera/provider/2.4/default/CameraProvider.cpp @@ -101,7 +101,7 @@ void CameraProvider::sTorchModeStatusChange( std::string cameraIdStr(camera_id); TorchModeStatus status = (TorchModeStatus) new_status; for (auto const& deviceNamePair : cp->mCameraDeviceNames) { - if (cameraIdStr.compare(getLegacyCameraId(deviceNamePair.first)) == 0) { + if (cameraIdStr.compare(deviceNamePair.first) == 0) { cp->mCallbacks->torchModeStatusChange( deviceNamePair.second, status); } @@ -142,7 +142,7 @@ int CameraProvider::getCameraDeviceVersion(const hidl_string& deviceName) { return -1; } if (sm[1].compare(kHAL3_2) == 0) { - // maybe switched to 3.4 or define the hidl version enumlater + // maybe switched to 3.4 or define the hidl version enum later return CAMERA_DEVICE_API_VERSION_3_2; } else if (sm[1].compare(kHAL1_0) == 0) { return CAMERA_DEVICE_API_VERSION_1_0; @@ -229,8 +229,8 @@ bool CameraProvider::initialize() { // Setup vendor tags here so HAL can setup vendor keys in camera characteristics VendorTagDescriptor::clearGlobalVendorTagDescriptor(); - setUpVendorTags(); - return false; + bool setupSucceed = setUpVendorTags(); + return !setupSucceed; // return flag here is mInitFailed } bool CameraProvider::setUpVendorTags() { @@ -311,7 +311,7 @@ Return CameraProvider::getCameraIdList(getCameraIdList_cb _hidl_cb) { } } hidl_vec hidlDeviceNameList(deviceNameList); - _hidl_cb (Status::OK, hidlDeviceNameList); + _hidl_cb(Status::OK, hidlDeviceNameList); return Void(); } @@ -321,12 +321,14 @@ Return CameraProvider::isSetTorchModeSupported(isSetTorchModeSupported_cb return Void(); } -Return CameraProvider::getCameraDeviceInterface_V1_x(const hidl_string& /*cameraDeviceName*/, getCameraDeviceInterface_V1_x_cb /*_hidl_cb*/) { +Return CameraProvider::getCameraDeviceInterface_V1_x( + const hidl_string& /*cameraDeviceName*/, getCameraDeviceInterface_V1_x_cb /*_hidl_cb*/) { // TODO implement after device 1.0 is implemented return Void(); } -Return CameraProvider::getCameraDeviceInterface_V3_x(const hidl_string& cameraDeviceName, getCameraDeviceInterface_V3_x_cb _hidl_cb) { +Return CameraProvider::getCameraDeviceInterface_V3_x( + const hidl_string& cameraDeviceName, getCameraDeviceInterface_V3_x_cb _hidl_cb) { std::smatch sm; bool match = matchDeviceName(cameraDeviceName, sm); if (!match) { @@ -335,14 +337,18 @@ Return CameraProvider::getCameraDeviceInterface_V3_x(const hidl_string& ca } std::string cameraId = sm[2]; + std::string deviceVersion = sm[1]; std::string deviceName(cameraDeviceName.c_str()); ssize_t index = mCameraDeviceNames.indexOf(std::make_pair(cameraId, deviceName)); if (index == NAME_NOT_FOUND) { // Either an illegal name or a device version mismatch Status status = Status::OK; ssize_t idx = mCameraIds.indexOf(cameraId); if (idx == NAME_NOT_FOUND) { + ALOGE("%s: cannot find camera %s!", __FUNCTION__, cameraId.c_str()); status = Status::ILLEGAL_ARGUMENT; } else { // invalid version + ALOGE("%s: camera device %s does not support version %s!", + __FUNCTION__, cameraId.c_str(), deviceVersion.c_str()); status = Status::OPERATION_NOT_SUPPORTED; } _hidl_cb(status, nullptr); @@ -364,13 +370,13 @@ Return CameraProvider::getCameraDeviceInterface_V3_x(const hidl_string& ca mModule, cameraId, mCameraDeviceNames); if (device == nullptr) { - ALOGE("%s: cannot allocate camera device!", __FUNCTION__); + ALOGE("%s: cannot allocate camera device for id %s", __FUNCTION__, cameraId.c_str()); _hidl_cb(Status::INTERNAL_ERROR, nullptr); return Void(); } if (device->isInitFailed()) { - ALOGE("%s: camera device init failed!", __FUNCTION__); + ALOGE("%s: camera device %s init failed!", __FUNCTION__, cameraId.c_str()); device = nullptr; _hidl_cb(Status::INTERNAL_ERROR, nullptr); return Void(); diff --git a/camera/provider/2.4/default/CameraProvider.h b/camera/provider/2.4/default/CameraProvider.h index 79899d96cc..8497ff3dd8 100644 --- a/camera/provider/2.4/default/CameraProvider.h +++ b/camera/provider/2.4/default/CameraProvider.h @@ -61,8 +61,12 @@ struct CameraProvider : public ICameraProvider, public camera_module_callbacks_t Return getVendorTags(getVendorTags_cb _hidl_cb) override; Return getCameraIdList(getCameraIdList_cb _hidl_cb) override; Return isSetTorchModeSupported(isSetTorchModeSupported_cb _hidl_cb) override; - Return getCameraDeviceInterface_V1_x(const hidl_string& cameraDeviceName, getCameraDeviceInterface_V1_x_cb _hidl_cb) override; - Return getCameraDeviceInterface_V3_x(const hidl_string& cameraDeviceName, getCameraDeviceInterface_V3_x_cb _hidl_cb) override; + Return getCameraDeviceInterface_V1_x( + const hidl_string& cameraDeviceName, + getCameraDeviceInterface_V1_x_cb _hidl_cb) override; + Return getCameraDeviceInterface_V3_x( + const hidl_string& cameraDeviceName, + getCameraDeviceInterface_V3_x_cb _hidl_cb) override; private: Mutex mCbLock; diff --git a/camera/provider/2.4/vts/functional/camera_hidl_hal_test.cpp b/camera/provider/2.4/vts/functional/camera_hidl_hal_test.cpp index b629dcc23f..0eb291c125 100644 --- a/camera/provider/2.4/vts/functional/camera_hidl_hal_test.cpp +++ b/camera/provider/2.4/vts/functional/camera_hidl_hal_test.cpp @@ -78,8 +78,8 @@ class CameraHidlEnvironment : public ::testing::Environment { public: // get the test environment singleton static CameraHidlEnvironment* Instance() { - static CameraHidlEnvironment* instance = new CameraHidlEnvironment; - return instance; + static CameraHidlEnvironment* instance = new CameraHidlEnvironment; + return instance; } virtual void SetUp() override;