Camera: pass StreamSurfaceId instead of Surface

Also fix buffer error callback on shared surfaces.

Test: CTS
Bug: 68020997
Change-Id: I71d6a1373ff09dcb21d39f78dd56727cbde9a3ad

camera/camera2/CaptureRequest.cpp

@@ -44,6 +44,8 @@ status_t CaptureRequest::readFromParcel(const android::Parcel* parcel) {
 
     mMetadata.clear();
     mSurfaceList.clear();
+    mStreamIdxList.clear();
+    mSurfaceIdxList.clear();
 
     status_t err = OK;
 
@@ -53,6 +55,13 @@ status_t CaptureRequest::readFromParcel(const android::Parcel* parcel) {
     }
     ALOGV("%s: Read metadata from parcel", __FUNCTION__);
 
+    int isReprocess = 0;
+    if ((err = parcel->readInt32(&isReprocess)) != OK) {
+        ALOGE("%s: Failed to read reprocessing from parcel", __FUNCTION__);
+        return err;
+    }
+    mIsReprocess = (isReprocess != 0);
+
     int32_t size;
     if ((err = parcel->readInt32(&size)) != OK) {
         ALOGE("%s: Failed to read surface list size from parcel", __FUNCTION__);
@@ -61,7 +70,7 @@ status_t CaptureRequest::readFromParcel(const android::Parcel* parcel) {
     ALOGV("%s: Read surface list size = %d", __FUNCTION__, size);
 
     // Do not distinguish null arrays from 0-sized arrays.
-    for (int i = 0; i < size; ++i) {
+    for (int32_t i = 0; i < size; ++i) {
         // Parcel.writeParcelableArray
         size_t len;
         const char16_t* className = parcel->readString16Inplace(&len);
@@ -88,12 +97,32 @@ status_t CaptureRequest::readFromParcel(const android::Parcel* parcel) {
         mSurfaceList.push_back(surface);
     }
 
-    int isReprocess = 0;
-    if ((err = parcel->readInt32(&isReprocess)) != OK) {
-        ALOGE("%s: Failed to read reprocessing from parcel", __FUNCTION__);
+    int32_t streamSurfaceSize;
+    if ((err = parcel->readInt32(&streamSurfaceSize)) != OK) {
+        ALOGE("%s: Failed to read streamSurfaceSize from parcel", __FUNCTION__);
         return err;
     }
-    mIsReprocess = (isReprocess != 0);
+
+    if (streamSurfaceSize < 0) {
+        ALOGE("%s: Bad streamSurfaceSize %d from parcel", __FUNCTION__, streamSurfaceSize);
+        return BAD_VALUE;
+    }
+
+    for (int32_t i = 0; i < streamSurfaceSize; ++i) {
+        int streamIdx;
+        if ((err = parcel->readInt32(&streamIdx)) != OK) {
+            ALOGE("%s: Failed to read stream index from parcel", __FUNCTION__);
+            return err;
+        }
+        mStreamIdxList.push_back(streamIdx);
+
+        int surfaceIdx;
+        if ((err = parcel->readInt32(&surfaceIdx)) != OK) {
+            ALOGE("%s: Failed to read surface index from parcel", __FUNCTION__);
+            return err;
+        }
+        mSurfaceIdxList.push_back(surfaceIdx);
+    }
 
     return OK;
 }
@@ -110,28 +139,43 @@ status_t CaptureRequest::writeToParcel(android::Parcel* parcel) const {
         return err;
     }
 
-    int32_t size = static_cast<int32_t>(mSurfaceList.size());
-
-    // Send 0-sized arrays when it's empty. Do not send null arrays.
-    parcel->writeInt32(size);
+    parcel->writeInt32(mIsReprocess ? 1 : 0);
 
-    for (int32_t i = 0; i < size; ++i) {
-        // not sure if readParcelableArray does this, hard to tell from source
-        parcel->writeString16(String16("android.view.Surface"));
+    if (mSurfaceConverted) {
+        parcel->writeInt32(0); // 0-sized array
+    } else {
+        int32_t size = static_cast<int32_t>(mSurfaceList.size());
+
+        // Send 0-sized arrays when it's empty. Do not send null arrays.
+        parcel->writeInt32(size);
+
+        for (int32_t i = 0; i < size; ++i) {
+            // not sure if readParcelableArray does this, hard to tell from source
+            parcel->writeString16(String16("android.view.Surface"));
+
+            // Surface.writeToParcel
+            view::Surface surfaceShim;
+            surfaceShim.name = String16("unknown_name");
+            surfaceShim.graphicBufferProducer = mSurfaceList[i]->getIGraphicBufferProducer();
+            if ((err = surfaceShim.writeToParcel(parcel)) != OK) {
+                ALOGE("%s: Failed to write output target Surface %d to parcel: %s (%d)",
+                        __FUNCTION__, i, strerror(-err), err);
+                return err;
+            }
+        }
+    }
 
-        // Surface.writeToParcel
-        view::Surface surfaceShim;
-        surfaceShim.name = String16("unknown_name");
-        surfaceShim.graphicBufferProducer = mSurfaceList[i]->getIGraphicBufferProducer();
-        if ((err = surfaceShim.writeToParcel(parcel)) != OK) {
-            ALOGE("%s: Failed to write output target Surface %d to parcel: %s (%d)",
-                    __FUNCTION__, i, strerror(-err), err);
+    parcel->writeInt32(mStreamIdxList.size());
+    for (size_t i = 0; i < mStreamIdxList.size(); ++i) {
+        if ((err = parcel->writeInt32(mStreamIdxList[i])) != OK) {
+            ALOGE("%s: Failed to write stream index to parcel", __FUNCTION__);
+            return err;
+        }
+        if ((err = parcel->writeInt32(mSurfaceIdxList[i])) != OK) {
+            ALOGE("%s: Failed to write surface index to parcel", __FUNCTION__);
             return err;
         }
     }
-
-    parcel->writeInt32(mIsReprocess ? 1 : 0);
-
     return OK;
 }
 

camera/include/camera/camera2/CaptureRequest.h

@@ -41,14 +41,30 @@ struct CaptureRequest : public Parcelable {
     virtual ~CaptureRequest();
 
     CameraMetadata          mMetadata;
+
+    // Used by NDK client to pass surfaces by stream/surface index.
+    bool                    mSurfaceConverted = false;
+
+    // Starting in Android O, create a Surface from Parcel will take one extra
+    // IPC call.
     Vector<sp<Surface> >    mSurfaceList;
+    // Optional way of passing surface list since passing Surface over binder
+    // is expensive. Use the stream/surface index from current output configuration
+    // to represent an configured output Surface. When stream/surface index is used,
+    // set mSurfaceList to zero length to save unparcel time.
+    Vector<int>             mStreamIdxList;
+    Vector<int>             mSurfaceIdxList; // per stream surface list index
+
     bool                    mIsReprocess;
+
     void*                   mContext; // arbitrary user context from NDK apps, null for java apps
 
     /**
      * Keep impl up-to-date with CaptureRequest.java in frameworks/base
      */
+    // used by cameraserver to receive CaptureRequest from java/NDK client
     status_t                readFromParcel(const android::Parcel* parcel) override;
+    // used by NDK client to send CaptureRequest to cameraserver
     status_t                writeToParcel(android::Parcel* parcel) const override;
 };
 

camera/ndk/impl/ACameraCaptureSession.cpp

@@ -159,7 +159,7 @@ camera_status_t ACameraCaptureSession::updateOutputConfiguration(ACaptureSession
     dev->lockDeviceForSessionOps();
     {
         Mutex::Autolock _l(mSessionLock);
-        ret = dev->updateOutputConfiguration(output);
+        ret = dev->updateOutputConfigurationLocked(output);
     }
     dev->unlockDevice();
     return ret;

camera/ndk/impl/ACameraDevice.cpp

@@ -289,7 +289,7 @@ CameraDevice::submitRequestsLocked(
     return ACAMERA_OK;
 }
 
-camera_status_t CameraDevice::updateOutputConfiguration(ACaptureSessionOutput *output) {
+camera_status_t CameraDevice::updateOutputConfigurationLocked(ACaptureSessionOutput *output) {
     camera_status_t ret = checkCameraClosedOrErrorLocked();
     if (ret != ACAMERA_OK) {
         return ret;
@@ -361,6 +361,7 @@ camera_status_t CameraDevice::updateOutputConfiguration(ACaptureSessionOutput *o
                 return ACAMERA_ERROR_UNKNOWN;
         }
     }
+    mConfiguredOutputs[streamId] = std::make_pair(output->mWindow, outConfig);
 
     return ACAMERA_OK;
 }
@@ -373,6 +374,7 @@ CameraDevice::allocateCaptureRequest(
     req->mMetadata = request->settings->getInternalData();
     req->mIsReprocess = false; // NDK does not support reprocessing yet
     req->mContext = request->context;
+    req->mSurfaceConverted = true; // set to true, and fill in stream/surface idx to speed up IPC
 
     for (auto outputTarget : request->targets->mOutputs) {
         ANativeWindow* anw = outputTarget.mWindow;
@@ -383,7 +385,31 @@ CameraDevice::allocateCaptureRequest(
             return ret;
         }
         req->mSurfaceList.push_back(surface);
+
+        bool found = false;
+        // lookup stream/surface ID
+        for (const auto& kvPair : mConfiguredOutputs) {
+            int streamId = kvPair.first;
+            const OutputConfiguration& outConfig = kvPair.second.second;
+            const auto& gbps = outConfig.getGraphicBufferProducers();
+            for (int surfaceId = 0; surfaceId < (int) gbps.size(); surfaceId++) {
+                if (gbps[surfaceId] == surface->getIGraphicBufferProducer()) {
+                    found = true;
+                    req->mStreamIdxList.push_back(streamId);
+                    req->mSurfaceIdxList.push_back(surfaceId);
+                    break;
+                }
+            }
+            if (found) {
+                break;
+            }
+        }
+        if (!found) {
+            ALOGE("Unconfigured output target %p in capture request!", anw);
+            return ret;
+        }
     }
+
     outReq = req;
     return ACAMERA_OK;
 }
@@ -564,17 +590,11 @@ camera_status_t
 CameraDevice::getIGBPfromAnw(
         ANativeWindow* anw,
         sp<IGraphicBufferProducer>& out) {
-    if (anw == nullptr) {
-        ALOGE("Error: output ANativeWindow is null");
-        return ACAMERA_ERROR_INVALID_PARAMETER;
-    }
-    int value;
-    int err = (*anw->query)(anw, NATIVE_WINDOW_CONCRETE_TYPE, &value);
-    if (err != OK || value != NATIVE_WINDOW_SURFACE) {
-        ALOGE("Error: ANativeWindow is not backed by Surface!");
-        return ACAMERA_ERROR_INVALID_PARAMETER;
+    sp<Surface> surface;
+    camera_status_t ret = getSurfaceFromANativeWindow(anw, surface);
+    if (ret != ACAMERA_OK) {
+        return ret;
     }
-    const sp<Surface> surface(static_cast<Surface*>(anw));
     out = surface->getIGraphicBufferProducer();
     return ACAMERA_OK;
 }
@@ -809,19 +829,26 @@ CameraDevice::onCaptureErrorLocked(
             setCameraDeviceErrorLocked(ACAMERA_ERROR_CAMERA_SERVICE);
             return;
         }
-        ANativeWindow* anw = outputPairIt->second.first;
-
-        ALOGV("Camera %s Lost output buffer for ANW %p frame %" PRId64,
-                getId(), anw, frameNumber);
 
-        sp<AMessage> msg = new AMessage(kWhatCaptureBufferLost, mHandler);
-        msg->setPointer(kContextKey, cbh.mCallbacks.context);
-        msg->setObject(kSessionSpKey, session);
-        msg->setPointer(kCallbackFpKey, (void*) onBufferLost);
-        msg->setObject(kCaptureRequestKey, request);
-        msg->setPointer(kAnwKey, (void*) anw);
-        msg->setInt64(kFrameNumberKey, frameNumber);
-        postSessionMsgAndCleanup(msg);
+        const auto& gbps = outputPairIt->second.second.getGraphicBufferProducers();
+        for (const auto& outGbp : gbps) {
+            for (auto surface : request->mSurfaceList) {
+                if (surface->getIGraphicBufferProducer() == outGbp) {
+                    ANativeWindow* anw = static_cast<ANativeWindow*>(surface.get());
+                    ALOGV("Camera %s Lost output buffer for ANW %p frame %" PRId64,
+                            getId(), anw, frameNumber);
+
+                    sp<AMessage> msg = new AMessage(kWhatCaptureBufferLost, mHandler);
+                    msg->setPointer(kContextKey, cbh.mCallbacks.context);
+                    msg->setObject(kSessionSpKey, session);
+                    msg->setPointer(kCallbackFpKey, (void*) onBufferLost);
+                    msg->setObject(kCaptureRequestKey, request);
+                    msg->setPointer(kAnwKey, (void*) anw);
+                    msg->setInt64(kFrameNumberKey, frameNumber);
+                    postSessionMsgAndCleanup(msg);
+                }
+            }
+        }
     } else { // Handle other capture failures
         // Fire capture failure callback if there is one registered
         ACameraCaptureSession_captureCallback_failed onError = cbh.mCallbacks.onCaptureFailed;

camera/ndk/impl/ACameraDevice.h

@@ -123,9 +123,9 @@ class CameraDevice final : public RefBase {
             /*out*/int* captureSequenceId,
             bool isRepeating);
 
-    camera_status_t updateOutputConfiguration(ACaptureSessionOutput *output);
+    camera_status_t updateOutputConfigurationLocked(ACaptureSessionOutput *output);
 
-    static camera_status_t allocateCaptureRequest(
+    camera_status_t allocateCaptureRequest(
             const ACaptureRequest* request, sp<CaptureRequest>& outReq);
 
     static ACaptureRequest* allocateACaptureRequest(sp<CaptureRequest>& req);

services/camera/libcameraservice/api2/CameraDeviceClient.cpp

@@ -121,6 +121,34 @@ binder::Status CameraDeviceClient::submitRequest(
     return submitRequestList(requestList, streaming, submitInfo);
 }
 
+binder::Status CameraDeviceClient::insertGbpLocked(const sp<IGraphicBufferProducer>& gbp,
+        SurfaceMap* outSurfaceMap,
+        Vector<int32_t>* outputStreamIds) {
+    int idx = mStreamMap.indexOfKey(IInterface::asBinder(gbp));
+
+    // Trying to submit request with surface that wasn't created
+    if (idx == NAME_NOT_FOUND) {
+        ALOGE("%s: Camera %s: Tried to submit a request with a surface that"
+                " we have not called createStream on",
+                __FUNCTION__, mCameraIdStr.string());
+        return STATUS_ERROR(CameraService::ERROR_ILLEGAL_ARGUMENT,
+                "Request targets Surface that is not part of current capture session");
+    }
+
+    const StreamSurfaceId& streamSurfaceId = mStreamMap.valueAt(idx);
+    if (outSurfaceMap->find(streamSurfaceId.streamId()) == outSurfaceMap->end()) {
+        (*outSurfaceMap)[streamSurfaceId.streamId()] = std::vector<size_t>();
+        outputStreamIds->push_back(streamSurfaceId.streamId());
+    }
+    (*outSurfaceMap)[streamSurfaceId.streamId()].push_back(streamSurfaceId.surfaceId());
+
+    ALOGV("%s: Camera %s: Appending output stream %d surface %d to request",
+            __FUNCTION__, mCameraIdStr.string(), streamSurfaceId.streamId(),
+            streamSurfaceId.surfaceId());
+
+    return binder::Status::ok();
+}
+
 binder::Status CameraDeviceClient::submitRequestList(
         const std::vector<hardware::camera2::CaptureRequest>& requests,
         bool streaming,
@@ -174,7 +202,7 @@ binder::Status CameraDeviceClient::submitRequestList(
                    __FUNCTION__, mCameraIdStr.string());
             return STATUS_ERROR(CameraService::ERROR_ILLEGAL_ARGUMENT,
                     "Request settings are empty");
-        } else if (request.mSurfaceList.isEmpty()) {
+        } else if (request.mSurfaceList.isEmpty() && request.mStreamIdxList.size() == 0) {
             ALOGE("%s: Camera %s: Requests must have at least one surface target. "
                     "Rejecting request.", __FUNCTION__, mCameraIdStr.string());
             return STATUS_ERROR(CameraService::ERROR_ILLEGAL_ARGUMENT,
@@ -193,31 +221,44 @@ binder::Status CameraDeviceClient::submitRequestList(
          */
         SurfaceMap surfaceMap;
         Vector<int32_t> outputStreamIds;
-        for (const sp<Surface>& surface : request.mSurfaceList) {
-            if (surface == 0) continue;
-
-            sp<IGraphicBufferProducer> gbp = surface->getIGraphicBufferProducer();
-            int idx = mStreamMap.indexOfKey(IInterface::asBinder(gbp));
-
-            // Trying to submit request with surface that wasn't created
-            if (idx == NAME_NOT_FOUND) {
-                ALOGE("%s: Camera %s: Tried to submit a request with a surface that"
-                        " we have not called createStream on",
-                        __FUNCTION__, mCameraIdStr.string());
-                return STATUS_ERROR(CameraService::ERROR_ILLEGAL_ARGUMENT,
-                        "Request targets Surface that is not part of current capture session");
+        if (request.mSurfaceList.size() > 0) {
+            for (sp<Surface> surface : request.mSurfaceList) {
+                if (surface == 0) continue;
+
+                sp<IGraphicBufferProducer> gbp = surface->getIGraphicBufferProducer();
+                res = insertGbpLocked(gbp, &surfaceMap, &outputStreamIds);
+                if (!res.isOk()) {
+                    return res;
+                }
             }
+        } else {
+            for (size_t i = 0; i < request.mStreamIdxList.size(); i++) {
+                int streamId = request.mStreamIdxList.itemAt(i);
+                int surfaceIdx = request.mSurfaceIdxList.itemAt(i);
+
+                ssize_t index = mConfiguredOutputs.indexOfKey(streamId);
+                if (index < 0) {
+                    ALOGE("%s: Camera %s: Tried to submit a request with a surface that"
+                            " we have not called createStream on: stream %d",
+                            __FUNCTION__, mCameraIdStr.string(), streamId);
+                    return STATUS_ERROR(CameraService::ERROR_ILLEGAL_ARGUMENT,
+                            "Request targets Surface that is not part of current capture session");
+                }
 
-            const StreamSurfaceId& streamSurfaceId = mStreamMap.valueAt(idx);
-            if (surfaceMap.find(streamSurfaceId.streamId()) == surfaceMap.end()) {
-                surfaceMap[streamSurfaceId.streamId()] = std::vector<size_t>();
-                outputStreamIds.push_back(streamSurfaceId.streamId());
-            }
-            surfaceMap[streamSurfaceId.streamId()].push_back(streamSurfaceId.surfaceId());
+                const auto& gbps = mConfiguredOutputs.valueAt(index).getGraphicBufferProducers();
+                if ((size_t)surfaceIdx >= gbps.size()) {
+                    ALOGE("%s: Camera %s: Tried to submit a request with a surface that"
+                            " we have not called createStream on: stream %d, surfaceIdx %d",
+                            __FUNCTION__, mCameraIdStr.string(), streamId, surfaceIdx);
+                    return STATUS_ERROR(CameraService::ERROR_ILLEGAL_ARGUMENT,
+                            "Request targets Surface has invalid surface index");
+                }
 
-            ALOGV("%s: Camera %s: Appending output stream %d surface %d to request",
-                    __FUNCTION__, mCameraIdStr.string(), streamSurfaceId.streamId(),
-                    streamSurfaceId.surfaceId());
+                res = insertGbpLocked(gbps[surfaceIdx], &surfaceMap, &outputStreamIds);
+                if (!res.isOk()) {
+                    return res;
+                }
+            }
         }
 
         metadata.update(ANDROID_REQUEST_OUTPUT_STREAMS, &outputStreamIds[0],
@@ -439,6 +480,8 @@ binder::Status CameraDeviceClient::deleteStream(int streamId) {
                 mStreamMap.removeItem(surface);
             }
 
+            mConfiguredOutputs.removeItem(streamId);
+
             if (dIndex != NAME_NOT_FOUND) {
                 mDeferredStreams.removeItemsAt(dIndex);
             }
@@ -550,6 +593,7 @@ binder::Status CameraDeviceClient::createStream(
             i++;
         }
 
+        mConfiguredOutputs.add(streamId, outputConfiguration);
         mStreamInfoMap[streamId] = streamInfo;
 
         ALOGV("%s: Camera %s: Successfully created a new stream ID %d for output surface"
@@ -842,6 +886,8 @@ binder::Status CameraDeviceClient::updateOutputConfiguration(int streamId,
                     StreamSurfaceId(streamId, outputMap.valueAt(i)));
         }
 
+        mConfiguredOutputs.replaceValueFor(streamId, outputConfiguration);
+
         ALOGV("%s: Camera %s: Successful stream ID %d update",
                   __FUNCTION__, mCameraIdStr.string(), streamId);
     }
@@ -1412,6 +1458,7 @@ binder::Status CameraDeviceClient::finalizeOutputConfigurations(int32_t streamId
             mDeferredStreams.removeItemsAt(deferredStreamIndex);
         }
         mStreamInfoMap[streamId].finalized = true;
+        mConfiguredOutputs.replaceValueFor(streamId, outputConfiguration);
     } else if (err == NO_INIT) {
         res = STATUS_ERROR_FMT(CameraService::ERROR_ILLEGAL_ARGUMENT,
                 "Camera %s: Deferred surface is invalid: %s (%d)",

services/camera/libcameraservice/api2/CameraDeviceClient.h

@@ -255,9 +255,18 @@ private:
     binder::Status createSurfaceFromGbp(OutputStreamInfo& streamInfo, bool isStreamInfoValid,
             sp<Surface>& surface, const sp<IGraphicBufferProducer>& gbp);
 
+
+    // Utility method to insert the surface into SurfaceMap
+    binder::Status insertGbpLocked(const sp<IGraphicBufferProducer>& gbp,
+            /*out*/SurfaceMap* surfaceMap,
+            /*out*/Vector<int32_t>* streamIds);
+
     // IGraphicsBufferProducer binder -> Stream ID + Surface ID for output streams
     KeyedVector<sp<IBinder>, StreamSurfaceId> mStreamMap;
 
+    // Stream ID -> OutputConfiguration. Used for looking up Surface by stream/surface index
+    KeyedVector<int32_t, hardware::camera2::params::OutputConfiguration> mConfiguredOutputs;
+
     struct InputStreamConfiguration {
         bool configured;
         int32_t width;