Camera: patching treble camera HAL

Bug fixes like deadlock resolution, wrong enum usage etc.

Bug: 30985004
Test: run Camera2 API CTS tests on Angler
Change-Id: I661fa9197f66344ddecca8f68d343c891806eca1

camera/device/3.2/default/CameraDevice.cpp

@@ -212,7 +212,7 @@ Return<void> CameraDevice::open(const sp<ICameraDeviceCallback>& 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();
         }
 

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<buffer_handle_t>& allBufs,
+        hidl_vec<buffer_handle_t*>& allBufPtrs,
         hidl_vec<int>& 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<buffer_handle_t> allBufs;
     allBufs.resize(numBufs);
+    allBufPtrs.resize(numBufs);
     allFences.resize(numBufs);
+    std::vector<int32_t> 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<buffer_handle_t>& allBufs, size_t numBufs,
+void CameraDeviceSession::cleanupInflightFences(
         hidl_vec<int>& 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<void> CameraDeviceSession::constructDefaultRequestSettings(
 Return<void> 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<camera3_stream_t*> streams;
@@ -394,18 +419,62 @@ Return<void> 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<Status> CameraDeviceSession::processCaptureRequest(const CaptureRequest&
         return Status::INTERNAL_ERROR;
     }
 
-    hidl_vec<buffer_handle_t> allBufs;
+    hidl_vec<buffer_handle_t*> allBufPtrs;
     hidl_vec<int> 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<camera3_stream_buffer_t> 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<Status> CameraDeviceSession::flush()  {
 Return<void> 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<Camera3Stream*>(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<Camera3Stream*>(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<Camera3Stream*>(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<Camera3Stream*>(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<Camera3Stream*>(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<Camera3Stream*>(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<Camera3Stream*>(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<Camera3Stream*>(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<CameraDeviceSession*>(static_cast<const CameraDeviceSession*>(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);

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 <unordered_map>
 #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<ICameraDeviceCallback>& mCallback;
+    const sp<ICameraDeviceCallback> mCallback;
     // Stream ID -> Camera3Stream cache
     std::map<int, Camera3Stream> mStreamMap;
+
+    mutable Mutex mInflightLock; // protecting mInflightBuffers and mCirculatingBuffers
     // (streamID, frameNumber) -> inflight buffer cache
-    std::map<std::pair<int, uint32_t>, StreamBufferCache>  mInflightBuffers;
+    std::map<std::pair<int, uint32_t>, 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<buffer_handle_t, buffer_handle_t,
+            BufferHasher, BufferComparator> CirculatingBuffers;
+    // Stream ID -> circulating buffers map
+    std::map<int, CirculatingBuffers> 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<buffer_handle_t>& allBufs,
+            hidl_vec<buffer_handle_t*>& allBufPtrs,
             hidl_vec<int>& allFences);
 
-    static void cleanupInflightBufferFences(
-            hidl_vec<buffer_handle_t>& allBufs, size_t numBufs,
+    static void cleanupInflightFences(
             hidl_vec<int>& allFences, size_t numFences);
 
     /**

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<Camera3Stream*>(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<Camera3Stream*>(
+                        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;

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);
 

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<void> CameraProvider::getCameraIdList(getCameraIdList_cb _hidl_cb)  {
         }
     }
     hidl_vec<hidl_string> hidlDeviceNameList(deviceNameList);
-    _hidl_cb (Status::OK, hidlDeviceNameList);
+    _hidl_cb(Status::OK, hidlDeviceNameList);
     return Void();
 }
 
@@ -321,12 +321,14 @@ Return<void> CameraProvider::isSetTorchModeSupported(isSetTorchModeSupported_cb
     return Void();
 }
 
-Return<void> CameraProvider::getCameraDeviceInterface_V1_x(const hidl_string& /*cameraDeviceName*/, getCameraDeviceInterface_V1_x_cb /*_hidl_cb*/)  {
+Return<void> 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<void> CameraProvider::getCameraDeviceInterface_V3_x(const hidl_string& cameraDeviceName, getCameraDeviceInterface_V3_x_cb _hidl_cb)  {
+Return<void> 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<void> 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<void> 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();

camera/provider/2.4/default/CameraProvider.h

@@ -61,8 +61,12 @@ struct CameraProvider : public ICameraProvider, public camera_module_callbacks_t
     Return<void> getVendorTags(getVendorTags_cb _hidl_cb) override;
     Return<void> getCameraIdList(getCameraIdList_cb _hidl_cb) override;
     Return<void> isSetTorchModeSupported(isSetTorchModeSupported_cb _hidl_cb) override;
-    Return<void> getCameraDeviceInterface_V1_x(const hidl_string& cameraDeviceName, getCameraDeviceInterface_V1_x_cb _hidl_cb) override;
-    Return<void> getCameraDeviceInterface_V3_x(const hidl_string& cameraDeviceName, getCameraDeviceInterface_V3_x_cb _hidl_cb) override;
+    Return<void> getCameraDeviceInterface_V1_x(
+            const hidl_string& cameraDeviceName,
+            getCameraDeviceInterface_V1_x_cb _hidl_cb) override;
+    Return<void> getCameraDeviceInterface_V3_x(
+            const hidl_string& cameraDeviceName,
+            getCameraDeviceInterface_V3_x_cb _hidl_cb) override;
 
 private:
     Mutex mCbLock;

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;