From 393eb7967f6b5acaf82d67b0e686b921b977f461 Mon Sep 17 00:00:00 2001 From: Dave Sparks Date: Thu, 15 Oct 2009 10:02:22 -0700 Subject: [PATCH] Don't callback on NULL client. Bug 2180510. We weren't checking to see if there was a valid camera client when calling the notify callback function. Now we grab a strong pointer before the callback to guarantee that the client is not destroyed before we complete the callback. This change also fixes other places in the code where we weren't holding a local strong pointer. --- camera/libcameraservice/CameraService.cpp | 87 ++++++++++++++++------- camera/libcameraservice/CameraService.h | 6 +- 2 files changed, 63 insertions(+), 30 deletions(-) diff --git a/camera/libcameraservice/CameraService.cpp b/camera/libcameraservice/CameraService.cpp index b63e97fdc..688014430 100644 --- a/camera/libcameraservice/CameraService.cpp +++ b/camera/libcameraservice/CameraService.cpp @@ -309,7 +309,7 @@ status_t CameraService::Client::connect(const sp& client) oldClient = mCameraClient; // did the client actually change? - if (client->asBinder() == mCameraClient->asBinder()) { + if ((mCameraClient != NULL) && (client->asBinder() == mCameraClient->asBinder())) { LOGD("Connect to the same client"); return NO_ERROR; } @@ -878,7 +878,10 @@ void CameraService::Client::handleShutter() mSurface->unregisterBuffers(); } - mCameraClient->notifyCallback(CAMERA_MSG_SHUTTER, 0, 0); + sp c = mCameraClient; + if (c != NULL) { + c->notifyCallback(CAMERA_MSG_SHUTTER, 0, 0); + } mHardware->disableMsgType(CAMERA_MSG_SHUTTER); // It takes some time before yuvPicture callback to be called. @@ -932,31 +935,38 @@ void CameraService::Client::handlePreviewData(const sp& mem) } } - // Is the callback enabled or not? - if (!(mPreviewCallbackFlag & FRAME_CALLBACK_FLAG_ENABLE_MASK)) { + // local copy of the callback flags + int flags = mPreviewCallbackFlag; + + // is callback enabled? + if (!(flags & FRAME_CALLBACK_FLAG_ENABLE_MASK)) { // If the enable bit is off, the copy-out and one-shot bits are ignored LOGV("frame callback is diabled"); return; } - // Is the received frame copied out or not? - if (mPreviewCallbackFlag & FRAME_CALLBACK_FLAG_COPY_OUT_MASK) { - LOGV("frame is copied out"); - copyFrameAndPostCopiedFrame(heap, offset, size); - } else { - LOGV("frame is directly sent out without copying"); - mCameraClient->dataCallback(CAMERA_MSG_PREVIEW_FRAME, mem); - } + // hold a strong pointer to the client + sp c = mCameraClient; - // Is this is one-shot only? - if (mPreviewCallbackFlag & FRAME_CALLBACK_FLAG_ONE_SHOT_MASK) { - LOGV("One-shot only, thus clear the bits and disable frame callback"); + // clear callback flags if no client or one-shot mode + if ((c == NULL) || (mPreviewCallbackFlag & FRAME_CALLBACK_FLAG_ONE_SHOT_MASK)) { + LOGV("Disable preview callback"); mPreviewCallbackFlag &= ~(FRAME_CALLBACK_FLAG_ONE_SHOT_MASK | FRAME_CALLBACK_FLAG_COPY_OUT_MASK | FRAME_CALLBACK_FLAG_ENABLE_MASK); + // TODO: Shouldn't we use this API for non-overlay hardware as well? if (mUseOverlay) mHardware->disableMsgType(CAMERA_MSG_PREVIEW_FRAME); } + + // Is the received frame copied out or not? + if (flags & FRAME_CALLBACK_FLAG_COPY_OUT_MASK) { + LOGV("frame is copied"); + copyFrameAndPostCopiedFrame(c, heap, offset, size); + } else { + LOGV("frame is forwarded"); + c->dataCallback(CAMERA_MSG_PREVIEW_FRAME, mem); + } } // picture callback - postview image ready @@ -972,7 +982,10 @@ void CameraService::Client::handlePostview(const sp& mem) } #endif - mCameraClient->dataCallback(CAMERA_MSG_POSTVIEW_FRAME, mem); + sp c = mCameraClient; + if (c != NULL) { + c->dataCallback(CAMERA_MSG_POSTVIEW_FRAME, mem); + } mHardware->disableMsgType(CAMERA_MSG_POSTVIEW_FRAME); } @@ -997,7 +1010,10 @@ void CameraService::Client::handleRawPicture(const sp& mem) mSurface->postBuffer(offset); } - mCameraClient->dataCallback(CAMERA_MSG_RAW_IMAGE, mem); + sp c = mCameraClient; + if (c != NULL) { + c->dataCallback(CAMERA_MSG_RAW_IMAGE, mem); + } mHardware->disableMsgType(CAMERA_MSG_RAW_IMAGE); } @@ -1014,7 +1030,10 @@ void CameraService::Client::handleCompressedPicture(const sp& mem) } #endif - mCameraClient->dataCallback(CAMERA_MSG_COMPRESSED_IMAGE, mem); + sp c = mCameraClient; + if (c != NULL) { + c->dataCallback(CAMERA_MSG_COMPRESSED_IMAGE, mem); + } mHardware->disableMsgType(CAMERA_MSG_COMPRESSED_IMAGE); } @@ -1032,7 +1051,10 @@ void CameraService::Client::notifyCallback(int32_t msgType, int32_t ext1, int32_ client->handleShutter(); break; default: - client->mCameraClient->notifyCallback(msgType, ext1, ext2); + sp c = client->mCameraClient; + if (c != NULL) { + c->notifyCallback(msgType, ext1, ext2); + } break; } @@ -1053,10 +1075,13 @@ void CameraService::Client::dataCallback(int32_t msgType, const sp& dat return; } + sp c = client->mCameraClient; if (dataPtr == NULL) { LOGE("Null data returned in data callback"); - client->mCameraClient->notifyCallback(CAMERA_MSG_ERROR, UNKNOWN_ERROR, 0); - client->mCameraClient->dataCallback(msgType, NULL); + if (c != NULL) { + c->notifyCallback(CAMERA_MSG_ERROR, UNKNOWN_ERROR, 0); + c->dataCallback(msgType, NULL); + } return; } @@ -1074,7 +1099,9 @@ void CameraService::Client::dataCallback(int32_t msgType, const sp& dat client->handleCompressedPicture(dataPtr); break; default: - client->mCameraClient->dataCallback(msgType, dataPtr); + if (c != NULL) { + c->dataCallback(msgType, dataPtr); + } break; } @@ -1095,15 +1122,20 @@ void CameraService::Client::dataCallbackTimestamp(nsecs_t timestamp, int32_t msg if (client == 0) { return; } + sp c = client->mCameraClient; if (dataPtr == NULL) { LOGE("Null data returned in data with timestamp callback"); - client->mCameraClient->notifyCallback(CAMERA_MSG_ERROR, UNKNOWN_ERROR, 0); - client->mCameraClient->dataCallbackTimestamp(0, msgType, NULL); + if (c != NULL) { + c->notifyCallback(CAMERA_MSG_ERROR, UNKNOWN_ERROR, 0); + c->dataCallbackTimestamp(0, msgType, NULL); + } return; } - client->mCameraClient->dataCallbackTimestamp(timestamp, msgType, dataPtr); + if (c != NULL) { + c->dataCallbackTimestamp(timestamp, msgType, dataPtr); + } #if DEBUG_CLIENT_REFERENCES if (client->getStrongCount() == 1) { @@ -1161,7 +1193,8 @@ status_t CameraService::Client::sendCommand(int32_t cmd, int32_t arg1, int32_t a return mHardware->sendCommand(cmd, arg1, arg2); } -void CameraService::Client::copyFrameAndPostCopiedFrame(sp heap, size_t offset, size_t size) +void CameraService::Client::copyFrameAndPostCopiedFrame(const sp& client, + const sp& heap, size_t offset, size_t size) { LOGV("copyFrameAndPostCopiedFrame"); // It is necessary to copy out of pmem before sending this to @@ -1186,7 +1219,7 @@ void CameraService::Client::copyFrameAndPostCopiedFrame(sp heap, si LOGE("failed to allocate space for frame callback"); return; } - mCameraClient->dataCallback(CAMERA_MSG_PREVIEW_FRAME, frame); + client->dataCallback(CAMERA_MSG_PREVIEW_FRAME, frame); } status_t CameraService::dump(int fd, const Vector& args) diff --git a/camera/libcameraservice/CameraService.h b/camera/libcameraservice/CameraService.h index 2e3597f11..2fcf83906 100644 --- a/camera/libcameraservice/CameraService.h +++ b/camera/libcameraservice/CameraService.h @@ -23,10 +23,9 @@ #include #include -class android::MemoryHeapBase; - namespace android { +class MemoryHeapBase; class MediaPlayer; // ---------------------------------------------------------------------------- @@ -151,7 +150,8 @@ private: void handleRawPicture(const sp&); void handleCompressedPicture(const sp&); - void copyFrameAndPostCopiedFrame(sp heap, size_t offset, size_t size); + void copyFrameAndPostCopiedFrame(const sp& client, + const sp& heap, size_t offset, size_t size); // camera operation mode enum camera_mode {