From e010f65e6337267cb15f8894c950a3f64370dd36 Mon Sep 17 00:00:00 2001 From: Haynes Mathew George Date: Fri, 13 Dec 2013 15:40:13 -0800 Subject: audioflinger: Fix for a deadlock in track creation AudioFlinger enters a deadlock (with itself) on trying to free a RecordTrack or Track object that failed initialization. Clear this bad object from the caller instead. Bug: 12423233 Change-Id: I926f2beb922a70f6924e593e2bbf1a5b5df85b16 --- services/audioflinger/AudioFlinger.cpp | 4 +++- services/audioflinger/Threads.cpp | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) (limited to 'services') diff --git a/services/audioflinger/AudioFlinger.cpp b/services/audioflinger/AudioFlinger.cpp index 3132e54..acbd19a 100644 --- a/services/audioflinger/AudioFlinger.cpp +++ b/services/audioflinger/AudioFlinger.cpp @@ -513,6 +513,8 @@ sp AudioFlinger::createTrack( track = thread->createTrack_l(client, streamType, sampleRate, format, channelMask, frameCount, sharedBuffer, lSessionId, flags, tid, clientUid, &lStatus); + LOG_ALWAYS_FATAL_IF((lStatus == NO_ERROR) && (track == 0)); + // we don't abort yet if lStatus != NO_ERROR; there is still work to be done regardless // move effect chain to this output thread if an effect on same session was waiting // for a track to be created @@ -1291,7 +1293,7 @@ sp AudioFlinger::openRecord( frameCount, lSessionId, IPCThreadState::self()->getCallingUid(), flags, tid, &lStatus); - LOG_ALWAYS_FATAL_IF((recordTrack != 0) != (lStatus == NO_ERROR)); + LOG_ALWAYS_FATAL_IF((lStatus == NO_ERROR) && (recordTrack == 0)); } if (lStatus != NO_ERROR) { // remove local strong reference to Client before deleting the RecordTrack so that the diff --git a/services/audioflinger/Threads.cpp b/services/audioflinger/Threads.cpp index bf85b51..64d7ec3 100644 --- a/services/audioflinger/Threads.cpp +++ b/services/audioflinger/Threads.cpp @@ -1326,8 +1326,10 @@ sp AudioFlinger::PlaybackThread::createTrac track = TimedTrack::create(this, client, streamType, sampleRate, format, channelMask, frameCount, sharedBuffer, sessionId, uid); } + if (track == 0 || track->getCblk() == NULL || track->name() < 0) { lStatus = NO_MEMORY; + // track must be cleared from the caller as the caller has the AF lock goto Exit; } @@ -4742,7 +4744,7 @@ sp AudioFlinger::RecordThread::createR if (track->getCblk() == 0) { ALOGE("createRecordTrack_l() no control block"); lStatus = NO_MEMORY; - track.clear(); + // track must be cleared from the caller as the caller has the AF lock goto Exit; } mTracks.add(track); -- cgit v1.1 From e0cd1051ed9fea0629745c29020516ae62298461 Mon Sep 17 00:00:00 2001 From: Haynes Mathew George Date: Fri, 27 Dec 2013 16:09:28 -0800 Subject: audioflinger: update track ready condition Signal track ready if the track isStopping(). Bug: 12423190 Change-Id: I95e14905df10ebf301e398263478c8ca25d7e2ce --- services/audioflinger/Tracks.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'services') diff --git a/services/audioflinger/Tracks.cpp b/services/audioflinger/Tracks.cpp index af04ce7..d6b9908 100644 --- a/services/audioflinger/Tracks.cpp +++ b/services/audioflinger/Tracks.cpp @@ -551,7 +551,7 @@ size_t AudioFlinger::PlaybackThread::Track::framesReleased() const // Don't call for fast tracks; the framesReady() could result in priority inversion bool AudioFlinger::PlaybackThread::Track::isReady() const { - if (mFillingUpStatus != FS_FILLING || isStopped() || isPausing()) { + if (mFillingUpStatus != FS_FILLING || isStopped() || isPausing() || isStopping()) { return true; } -- cgit v1.1 From 50c3157c5a3e0617be77716beff1ae8801d8a72f Mon Sep 17 00:00:00 2001 From: Haynes Mathew George Date: Tue, 3 Dec 2013 21:26:02 -0800 Subject: audioflinger: check for condition before waiting AsyncCallbackThread must check for any condition that was already been satisfied before waiting. Bug: 11824817 Change-Id: I04683a1f355de4f440106cab47fd916aa39d5e35 --- services/audioflinger/Threads.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'services') diff --git a/services/audioflinger/Threads.cpp b/services/audioflinger/Threads.cpp index 64d7ec3..110e45c 100644 --- a/services/audioflinger/Threads.cpp +++ b/services/audioflinger/Threads.cpp @@ -3863,7 +3863,12 @@ bool AudioFlinger::AsyncCallbackThread::threadLoop() { Mutex::Autolock _l(mLock); - mWaitWorkCV.wait(mLock); + while (!((mWriteAckSequence & 1) || + (mDrainSequence & 1) || + exitPending())) { + mWaitWorkCV.wait(mLock); + } + if (exitPending()) { break; } -- cgit v1.1 From 7e92abeafb184e8a34213d7149592e95a72601b0 Mon Sep 17 00:00:00 2001 From: Eric Laurent Date: Fri, 22 Nov 2013 09:29:56 -0800 Subject: audioflinger: fix offload write buffer offset Fix current audio HAL write buffer offset calculation which assumes that the frame size is a multiple of 2. ' Bug: 12823725. Change-Id: I0195ed5cfef225a6f114e7dd405a02680bb7254e --- services/audioflinger/Threads.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'services') diff --git a/services/audioflinger/Threads.cpp b/services/audioflinger/Threads.cpp index 110e45c..7cc6abe 100644 --- a/services/audioflinger/Threads.cpp +++ b/services/audioflinger/Threads.cpp @@ -1915,7 +1915,7 @@ ssize_t AudioFlinger::PlaybackThread::threadLoop_write() // otherwise use the HAL / AudioStreamOut directly } else { // Direct output and offload threads - size_t offset = (mCurrentWriteLength - mBytesRemaining) / sizeof(int16_t); + size_t offset = (mCurrentWriteLength - mBytesRemaining); if (mUseAsyncWrite) { ALOGW_IF(mWriteAckSequence & 1, "threadLoop_write(): out of sequence write request"); mWriteAckSequence += 2; @@ -1926,7 +1926,7 @@ ssize_t AudioFlinger::PlaybackThread::threadLoop_write() // FIXME We should have an implementation of timestamps for direct output threads. // They are used e.g for multichannel PCM playback over HDMI. bytesWritten = mOutput->stream->write(mOutput->stream, - mMixBuffer + offset, mBytesRemaining); + (char *)mMixBuffer + offset, mBytesRemaining); if (mUseAsyncWrite && ((bytesWritten < 0) || (bytesWritten == (ssize_t)mBytesRemaining))) { // do not wait for async callback in case of error of full write -- cgit v1.1 From abab1c33caf0982c11713e6d64d60105dcbc8ab7 Mon Sep 17 00:00:00 2001 From: Gaurav Kumar Date: Mon, 6 Jan 2014 10:57:18 +0530 Subject: AudioMixer: Remove tracks from enabledTracks after reseting outTemp If any track goes through AudioMixer::process__genericNoResampling, and its getnextbuffer returns NULL, Then that track is removed by AudioMixer from enabledTracks. Thus if all tracks getnextbuffer return NULL, Then this function doesn't reset outTemp and last buffer in AudioFlinger's mMixBuffer will be repeated and noise is observed. Remove tracks from enabledTracks after reseting outTemp to zero, so that process__genericNoResampling will reset outTemp and noise won't appear. Bug: 12450065 Change-Id: I28996d425838728955f01eb1a00acf6e6dc2dea1 Signed-off-by: Gaurav Kumar Signed-off-by: Pierre Couillaud --- services/audioflinger/AudioMixer.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) (limited to 'services') diff --git a/services/audioflinger/AudioMixer.cpp b/services/audioflinger/AudioMixer.cpp index df4e029..07dc6dd 100644 --- a/services/audioflinger/AudioMixer.cpp +++ b/services/audioflinger/AudioMixer.cpp @@ -1122,10 +1122,6 @@ void AudioMixer::process__genericNoResampling(state_t* state, int64_t pts) t.bufferProvider->getNextBuffer(&t.buffer, pts); t.frameCount = t.buffer.frameCount; t.in = t.buffer.raw; - // t.in == NULL can happen if the track was flushed just after having - // been enabled for mixing. - if (t.in == NULL) - enabledTracks &= ~(1< outFrames)?outFrames:t.frameCount; if (inFrames) { t.hook(&t, outTemp + (BLOCKSIZE-outFrames)*MAX_NUM_CHANNELS, inFrames, -- cgit v1.1 From 281dd4e13309973dbb85bce531f884237e0d8fb0 Mon Sep 17 00:00:00 2001 From: Eric Laurent Date: Fri, 20 Dec 2013 17:36:01 -0800 Subject: audioflinger: fix static track end detection If a static track is not a fast track, prepareTracks_l() must rely on framesReady() to detect end of buffer and remove the track from the active track list. Failing to do so results in the track staying active but not processed by the mixer because in underrun. This leaves the mix buffer content uninitialized and causes the effect process function to accumulate its output onto undefined data. Bug: 12013676. Change-Id: I4b0819a9d93141ac3307b8786fc6a451dd585220 --- services/audioflinger/Threads.cpp | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) (limited to 'services') diff --git a/services/audioflinger/Threads.cpp b/services/audioflinger/Threads.cpp index 7cc6abe..4f1c01d 100644 --- a/services/audioflinger/Threads.cpp +++ b/services/audioflinger/Threads.cpp @@ -3038,15 +3038,8 @@ AudioFlinger::PlaybackThread::mixer_state AudioFlinger::MixerThread::prepareTrac (mMixerStatusIgnoringFastTracks == MIXER_TRACKS_READY)) { minFrames = desiredFrames; } - // It's not safe to call framesReady() for a static buffer track, so assume it's ready - size_t framesReady; - if (track->sharedBuffer() == 0) { - framesReady = track->framesReady(); - } else if (track->isStopped()) { - framesReady = 0; - } else { - framesReady = 1; - } + + size_t framesReady = track->framesReady(); if ((framesReady >= minFrames) && track->isReady() && !track->isPaused() && !track->isTerminated()) { -- cgit v1.1 From d812fc012298470a1b8120e6d60a24b0b1d48047 Mon Sep 17 00:00:00 2001 From: Glenn Kasten Date: Tue, 3 Dec 2013 09:06:43 -0800 Subject: Increase kFastTrackMultiplier from 1 to 2 Bug: 11967381 Change-Id: Iedec06280aa745d9df5d661f4916940cede9c191 --- services/audioflinger/Threads.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'services') diff --git a/services/audioflinger/Threads.cpp b/services/audioflinger/Threads.cpp index 4f1c01d..3d657b3 100644 --- a/services/audioflinger/Threads.cpp +++ b/services/audioflinger/Threads.cpp @@ -135,12 +135,12 @@ static const int kPriorityFastMixer = 3; // IAudioFlinger::createTrack() reports back to client the total size of shared memory area // for the track. The client then sub-divides this into smaller buffers for its use. -// Currently the client uses double-buffering by default, but doesn't tell us about that. -// So for now we just assume that client is double-buffered. -// FIXME It would be better for client to tell AudioFlinger whether it wants double-buffering or -// N-buffering, so AudioFlinger could allocate the right amount of memory. +// Currently the client uses N-buffering by default, but doesn't tell us about the value of N. +// So for now we just assume that client is double-buffered for fast tracks. +// FIXME It would be better for client to tell AudioFlinger the value of N, +// so AudioFlinger could allocate the right amount of memory. // See the client's minBufCount and mNotificationFramesAct calculations for details. -static const int kFastTrackMultiplier = 1; +static const int kFastTrackMultiplier = 2; // ---------------------------------------------------------------------------- @@ -1210,7 +1210,7 @@ sp AudioFlinger::PlaybackThread::createTrac ( (tid != -1) && ((frameCount == 0) || - (frameCount >= (mFrameCount * kFastTrackMultiplier))) + (frameCount >= mFrameCount)) ) ) && // PCM data @@ -4687,7 +4687,7 @@ sp AudioFlinger::RecordThread::createR ( (tid != -1) && ((frameCount == 0) || - (frameCount >= (mFrameCount * kFastTrackMultiplier))) + (frameCount >= mFrameCount)) ) && // FIXME when record supports non-PCM data, also check for audio_is_linear_pcm(format) // mono or stereo -- cgit v1.1 From 81d754306ecd4a587459015da5168270c2a5c167 Mon Sep 17 00:00:00 2001 From: Zhijun He Date: Tue, 26 Nov 2013 15:11:05 -0800 Subject: DO NOT MERGE: camera2/3: Add protection for still capture path Jpeg stream in JpegProcessor could be deleted while process new capture is ongoing, which unsafe to access a dead consumer endpoint. Bug: 9316454 Change-Id: I2950f31ea28d0ba01f08502e2e3ba452bf8bb818 --- .../api1/client2/JpegProcessor.cpp | 79 ++++++++++++---------- 1 file changed, 44 insertions(+), 35 deletions(-) (limited to 'services') diff --git a/services/camera/libcameraservice/api1/client2/JpegProcessor.cpp b/services/camera/libcameraservice/api1/client2/JpegProcessor.cpp index 77d5c8a..ec81456 100644 --- a/services/camera/libcameraservice/api1/client2/JpegProcessor.cpp +++ b/services/camera/libcameraservice/api1/client2/JpegProcessor.cpp @@ -200,50 +200,59 @@ status_t JpegProcessor::processNewCapture() { ATRACE_CALL(); status_t res; sp captureHeap; + sp captureBuffer; CpuConsumer::LockedBuffer imgBuffer; - res = mCaptureConsumer->lockNextBuffer(&imgBuffer); - if (res != OK) { - if (res != BAD_VALUE) { - ALOGE("%s: Camera %d: Error receiving still image buffer: " - "%s (%d)", __FUNCTION__, - mId, strerror(-res), res); + { + Mutex::Autolock l(mInputMutex); + if (mCaptureStreamId == NO_STREAM) { + ALOGW("%s: Camera %d: No stream is available", __FUNCTION__, mId); + return INVALID_OPERATION; } - return res; - } - ALOGV("%s: Camera %d: Still capture available", __FUNCTION__, - mId); + res = mCaptureConsumer->lockNextBuffer(&imgBuffer); + if (res != OK) { + if (res != BAD_VALUE) { + ALOGE("%s: Camera %d: Error receiving still image buffer: " + "%s (%d)", __FUNCTION__, + mId, strerror(-res), res); + } + return res; + } - if (imgBuffer.format != HAL_PIXEL_FORMAT_BLOB) { - ALOGE("%s: Camera %d: Unexpected format for still image: " - "%x, expected %x", __FUNCTION__, mId, - imgBuffer.format, - HAL_PIXEL_FORMAT_BLOB); - mCaptureConsumer->unlockBuffer(imgBuffer); - return OK; - } + ALOGV("%s: Camera %d: Still capture available", __FUNCTION__, + mId); - // Find size of JPEG image - size_t jpegSize = findJpegSize(imgBuffer.data, imgBuffer.width); - if (jpegSize == 0) { // failed to find size, default to whole buffer - jpegSize = imgBuffer.width; - } - size_t heapSize = mCaptureHeap->getSize(); - if (jpegSize > heapSize) { - ALOGW("%s: JPEG image is larger than expected, truncating " - "(got %d, expected at most %d bytes)", - __FUNCTION__, jpegSize, heapSize); - jpegSize = heapSize; - } + if (imgBuffer.format != HAL_PIXEL_FORMAT_BLOB) { + ALOGE("%s: Camera %d: Unexpected format for still image: " + "%x, expected %x", __FUNCTION__, mId, + imgBuffer.format, + HAL_PIXEL_FORMAT_BLOB); + mCaptureConsumer->unlockBuffer(imgBuffer); + return OK; + } - // TODO: Optimize this to avoid memcopy - sp captureBuffer = new MemoryBase(mCaptureHeap, 0, jpegSize); - void* captureMemory = mCaptureHeap->getBase(); - memcpy(captureMemory, imgBuffer.data, jpegSize); + // Find size of JPEG image + size_t jpegSize = findJpegSize(imgBuffer.data, imgBuffer.width); + if (jpegSize == 0) { // failed to find size, default to whole buffer + jpegSize = imgBuffer.width; + } + size_t heapSize = mCaptureHeap->getSize(); + if (jpegSize > heapSize) { + ALOGW("%s: JPEG image is larger than expected, truncating " + "(got %d, expected at most %d bytes)", + __FUNCTION__, jpegSize, heapSize); + jpegSize = heapSize; + } + + // TODO: Optimize this to avoid memcopy + captureBuffer = new MemoryBase(mCaptureHeap, 0, jpegSize); + void* captureMemory = mCaptureHeap->getBase(); + memcpy(captureMemory, imgBuffer.data, jpegSize); - mCaptureConsumer->unlockBuffer(imgBuffer); + mCaptureConsumer->unlockBuffer(imgBuffer); + } sp sequencer = mSequencer.promote(); if (sequencer != 0) { -- cgit v1.1 From 6706009fa8294c8cdab1cdab4585f00d42df483e Mon Sep 17 00:00:00 2001 From: Ruben Brunk Date: Thu, 5 Dec 2013 11:00:37 -0800 Subject: DO NOT MERGE: camera2: Fix race with stream deletion during disconnect. Bug: 11856804 - Shutdown order in Camera2Client allows a stream to be deleted before the corresponding processing thread has quit. This can result in updates being called on the processor thread without a valid stream. Change-Id: Ie4e649771f4321498659211f2a37ed89a6d956c4 --- services/camera/libcameraservice/api1/Camera2Client.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) (limited to 'services') diff --git a/services/camera/libcameraservice/api1/Camera2Client.cpp b/services/camera/libcameraservice/api1/Camera2Client.cpp index 0b6ca5c..abcbd06 100644 --- a/services/camera/libcameraservice/api1/Camera2Client.cpp +++ b/services/camera/libcameraservice/api1/Camera2Client.cpp @@ -406,12 +406,6 @@ void Camera2Client::disconnect() { l.mParameters.state = Parameters::DISCONNECTED; } - mStreamingProcessor->deletePreviewStream(); - mStreamingProcessor->deleteRecordingStream(); - mJpegProcessor->deleteStream(); - mCallbackProcessor->deleteStream(); - mZslProcessor->deleteStream(); - mStreamingProcessor->requestExit(); mFrameProcessor->requestExit(); mCaptureSequencer->requestExit(); @@ -428,6 +422,14 @@ void Camera2Client::disconnect() { mZslProcessorThread->join(); mCallbackProcessor->join(); + ALOGV("Camera %d: Deleting streams", mCameraId); + + mStreamingProcessor->deletePreviewStream(); + mStreamingProcessor->deleteRecordingStream(); + mJpegProcessor->deleteStream(); + mCallbackProcessor->deleteStream(); + mZslProcessor->deleteStream(); + ALOGV("Camera %d: Disconnecting device", mCameraId); mDevice->disconnect(); -- cgit v1.1 From a413dd621966044753a8fa1f57c76d847b6f4bec Mon Sep 17 00:00:00 2001 From: Zhijun He Date: Wed, 29 Jan 2014 08:52:01 -0800 Subject: DO NOT MERGE: Camera: fix focusArea wrong indexing issue Bug: 12304559 Change-Id: Id28b35fdd9697c1ec3365f617996801965de8bd0 --- .../libcameraservice/api1/client2/Parameters.cpp | 28 +++++++++++----------- 1 file changed, 14 insertions(+), 14 deletions(-) (limited to 'services') diff --git a/services/camera/libcameraservice/api1/client2/Parameters.cpp b/services/camera/libcameraservice/api1/client2/Parameters.cpp index 08af566..5196e09 100644 --- a/services/camera/libcameraservice/api1/client2/Parameters.cpp +++ b/services/camera/libcameraservice/api1/client2/Parameters.cpp @@ -1858,23 +1858,23 @@ status_t Parameters::updateRequest(CameraMetadata *request) const { size_t reqFocusingAreasSize = focusingAreas.size() * 5; int32_t *reqFocusingAreas = new int32_t[reqFocusingAreasSize]; - for (size_t i = 0; i < reqFocusingAreasSize; i += 5) { - if (focusingAreas[i].weight != 0) { + for (size_t i = 0, j = 0; i < reqFocusingAreasSize; i += 5, j++) { + if (focusingAreas[j].weight != 0) { reqFocusingAreas[i + 0] = - normalizedXToArray(focusingAreas[i].left); + normalizedXToArray(focusingAreas[j].left); reqFocusingAreas[i + 1] = - normalizedYToArray(focusingAreas[i].top); + normalizedYToArray(focusingAreas[j].top); reqFocusingAreas[i + 2] = - normalizedXToArray(focusingAreas[i].right); + normalizedXToArray(focusingAreas[j].right); reqFocusingAreas[i + 3] = - normalizedYToArray(focusingAreas[i].bottom); + normalizedYToArray(focusingAreas[j].bottom); } else { reqFocusingAreas[i + 0] = 0; reqFocusingAreas[i + 1] = 0; reqFocusingAreas[i + 2] = 0; reqFocusingAreas[i + 3] = 0; } - reqFocusingAreas[i + 4] = focusingAreas[i].weight; + reqFocusingAreas[i + 4] = focusingAreas[j].weight; } res = request->update(ANDROID_CONTROL_AF_REGIONS, reqFocusingAreas, reqFocusingAreasSize); @@ -1887,23 +1887,23 @@ status_t Parameters::updateRequest(CameraMetadata *request) const { size_t reqMeteringAreasSize = meteringAreas.size() * 5; int32_t *reqMeteringAreas = new int32_t[reqMeteringAreasSize]; - for (size_t i = 0; i < reqMeteringAreasSize; i += 5) { - if (meteringAreas[i].weight != 0) { + for (size_t i = 0, j = 0; i < reqMeteringAreasSize; i += 5, j++) { + if (meteringAreas[j].weight != 0) { reqMeteringAreas[i + 0] = - normalizedXToArray(meteringAreas[i].left); + normalizedXToArray(meteringAreas[j].left); reqMeteringAreas[i + 1] = - normalizedYToArray(meteringAreas[i].top); + normalizedYToArray(meteringAreas[j].top); reqMeteringAreas[i + 2] = - normalizedXToArray(meteringAreas[i].right); + normalizedXToArray(meteringAreas[j].right); reqMeteringAreas[i + 3] = - normalizedYToArray(meteringAreas[i].bottom); + normalizedYToArray(meteringAreas[j].bottom); } else { reqMeteringAreas[i + 0] = 0; reqMeteringAreas[i + 1] = 0; reqMeteringAreas[i + 2] = 0; reqMeteringAreas[i + 3] = 0; } - reqMeteringAreas[i + 4] = meteringAreas[i].weight; + reqMeteringAreas[i + 4] = meteringAreas[j].weight; } res = request->update(ANDROID_CONTROL_AE_REGIONS, reqMeteringAreas, reqMeteringAreasSize); -- cgit v1.1 From bc69c8ba9a8fc881603669205a56d0ca1b572a95 Mon Sep 17 00:00:00 2001 From: Zhijun He Date: Tue, 21 Jan 2014 12:15:56 -0800 Subject: DO NOT MERGE: Camera: delete preview callback when preview size is changed Preview callback stream is left configured even the preview size is changed. This makes the callback stream unnecessarily configured even in recording mode, which could cause distorted preview for some devices. Bug: 12210027 Bug: 12591410 Change-Id: If50cddfe5562e91aec1feb1760eccb82ddb21730 --- services/camera/libcameraservice/api1/Camera2Client.cpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) (limited to 'services') diff --git a/services/camera/libcameraservice/api1/Camera2Client.cpp b/services/camera/libcameraservice/api1/Camera2Client.cpp index abcbd06..b093946 100644 --- a/services/camera/libcameraservice/api1/Camera2Client.cpp +++ b/services/camera/libcameraservice/api1/Camera2Client.cpp @@ -733,6 +733,7 @@ status_t Camera2Client::startPreviewL(Parameters ¶ms, bool restart) { return OK; } params.state = Parameters::STOPPED; + int lastPreviewStreamId = mStreamingProcessor->getPreviewStreamId(); res = mStreamingProcessor->updatePreviewStream(params); if (res != OK) { @@ -741,6 +742,8 @@ status_t Camera2Client::startPreviewL(Parameters ¶ms, bool restart) { return res; } + bool previewStreamChanged = mStreamingProcessor->getPreviewStreamId() != lastPreviewStreamId; + // We could wait to create the JPEG output stream until first actual use // (first takePicture call). However, this would substantially increase the // first capture latency on HAL3 devices, and potentially on some HAL2 @@ -790,6 +793,19 @@ status_t Camera2Client::startPreviewL(Parameters ¶ms, bool restart) { return res; } outputStreams.push(getCallbackStreamId()); + } else if (previewStreamChanged && mCallbackProcessor->getStreamId() != NO_STREAM) { + /** + * Delete the unused callback stream when preview stream is changed and + * preview is not enabled. Don't need stop preview stream as preview is in + * STOPPED state now. + */ + ALOGV("%s: Camera %d: Delete unused preview callback stream.", __FUNCTION__, mCameraId); + res = mCallbackProcessor->deleteStream(); + if (res != OK) { + ALOGE("%s: Camera %d: Unable to delete callback stream %s (%d)", + __FUNCTION__, mCameraId, strerror(-res), res); + return res; + } } if (params.zslMode && !params.recordingHint) { res = updateProcessorStream(mZslProcessor, params); -- cgit v1.1 From bd3e2e03f3ab686c52982a9e50cae853128172cf Mon Sep 17 00:00:00 2001 From: Igor Murashkin Date: Mon, 17 Mar 2014 13:01:41 -0700 Subject: camera2: Fix segfault when using null availability listener ICameraService::addListener / removeListener will now return BAD_VALUE if a null listener is used. Bug: 12891434 Change-Id: I9764110094d8fd42e22fcc8df3ef0e73c1b070e7 --- services/camera/libcameraservice/CameraService.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'services') diff --git a/services/camera/libcameraservice/CameraService.cpp b/services/camera/libcameraservice/CameraService.cpp index eeedfc9..5957d97 100644 --- a/services/camera/libcameraservice/CameraService.cpp +++ b/services/camera/libcameraservice/CameraService.cpp @@ -655,6 +655,11 @@ status_t CameraService::addListener( const sp& listener) { ALOGV("%s: Add listener %p", __FUNCTION__, listener.get()); + if (listener == 0) { + ALOGE("%s: Listener must not be null", __FUNCTION__); + return BAD_VALUE; + } + Mutex::Autolock lock(mServiceLock); Vector >::iterator it, end; @@ -683,6 +688,11 @@ status_t CameraService::removeListener( const sp& listener) { ALOGV("%s: Remove listener %p", __FUNCTION__, listener.get()); + if (listener == 0) { + ALOGE("%s: Listener must not be null", __FUNCTION__); + return BAD_VALUE; + } + Mutex::Autolock lock(mServiceLock); Vector >::iterator it; -- cgit v1.1 From 0370be96e33ea0c8fb4069e704deccce43b7403c Mon Sep 17 00:00:00 2001 From: Igor Murashkin Date: Tue, 18 Mar 2014 18:15:23 -0700 Subject: DO NOT MERGE: camera: Fix setParameters for Preview FPS single/range values As a workaround, duplicate CameraParameters into CameraParameters2 to prevent ABI break for some camera HALs that directly link into CameraParameters. CameraParameters2 implements the real fixes needed in the framework, while CameraParameters is left in to satisfy older camera HALs. Bug: 12609188 Change-Id: I82ea6f5de2183dd046d4bf5683600c97f37ab4da --- .../libcameraservice/api1/client2/Parameters.cpp | 160 +++++++++++++++------ .../libcameraservice/api1/client2/Parameters.h | 5 +- 2 files changed, 121 insertions(+), 44 deletions(-) (limited to 'services') diff --git a/services/camera/libcameraservice/api1/client2/Parameters.cpp b/services/camera/libcameraservice/api1/client2/Parameters.cpp index 5196e09..61e3588 100644 --- a/services/camera/libcameraservice/api1/client2/Parameters.cpp +++ b/services/camera/libcameraservice/api1/client2/Parameters.cpp @@ -16,7 +16,7 @@ #define LOG_TAG "Camera2-Parameters" #define ATRACE_TAG ATRACE_TAG_CAMERA -//#define LOG_NDEBUG 0 +// #define LOG_NDEBUG 0 #include #include @@ -92,26 +92,6 @@ status_t Parameters::initialize(const CameraMetadata *info) { staticInfo(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES, 2); if (!availableFpsRanges.count) return NO_INIT; - previewFpsRange[0] = availableFpsRanges.data.i32[0]; - previewFpsRange[1] = availableFpsRanges.data.i32[1]; - - params.set(CameraParameters::KEY_PREVIEW_FPS_RANGE, - String8::format("%d,%d", - previewFpsRange[0] * kFpsToApiScale, - previewFpsRange[1] * kFpsToApiScale)); - - { - String8 supportedPreviewFpsRange; - for (size_t i=0; i < availableFpsRanges.count; i += 2) { - if (i != 0) supportedPreviewFpsRange += ","; - supportedPreviewFpsRange += String8::format("(%d,%d)", - availableFpsRanges.data.i32[i] * kFpsToApiScale, - availableFpsRanges.data.i32[i+1] * kFpsToApiScale); - } - params.set(CameraParameters::KEY_SUPPORTED_PREVIEW_FPS_RANGE, - supportedPreviewFpsRange); - } - previewFormat = HAL_PIXEL_FORMAT_YCrCb_420_SP; params.set(CameraParameters::KEY_PREVIEW_FORMAT, formatEnumToString(previewFormat)); // NV21 @@ -179,6 +159,9 @@ status_t Parameters::initialize(const CameraMetadata *info) { supportedPreviewFormats); } + previewFpsRange[0] = availableFpsRanges.data.i32[0]; + previewFpsRange[1] = availableFpsRanges.data.i32[1]; + // PREVIEW_FRAME_RATE / SUPPORTED_PREVIEW_FRAME_RATES are deprecated, but // still have to do something sane for them @@ -187,6 +170,27 @@ status_t Parameters::initialize(const CameraMetadata *info) { params.set(CameraParameters::KEY_PREVIEW_FRAME_RATE, previewFps); + // PREVIEW_FPS_RANGE + // -- Order matters. Set range after single value to so that a roundtrip + // of setParameters(getParameters()) would keep the FPS range in higher + // order. + params.set(CameraParameters::KEY_PREVIEW_FPS_RANGE, + String8::format("%d,%d", + previewFpsRange[0] * kFpsToApiScale, + previewFpsRange[1] * kFpsToApiScale)); + + { + String8 supportedPreviewFpsRange; + for (size_t i=0; i < availableFpsRanges.count; i += 2) { + if (i != 0) supportedPreviewFpsRange += ","; + supportedPreviewFpsRange += String8::format("(%d,%d)", + availableFpsRanges.data.i32[i] * kFpsToApiScale, + availableFpsRanges.data.i32[i+1] * kFpsToApiScale); + } + params.set(CameraParameters::KEY_SUPPORTED_PREVIEW_FPS_RANGE, + supportedPreviewFpsRange); + } + { SortedVector sortedPreviewFrameRates; @@ -1084,7 +1088,7 @@ camera_metadata_ro_entry_t Parameters::staticInfo(uint32_t tag, status_t Parameters::set(const String8& paramString) { status_t res; - CameraParameters newParams(paramString); + CameraParameters2 newParams(paramString); // TODO: Currently ignoring any changes to supposedly read-only parameters // such as supported preview sizes, etc. Should probably produce an error if @@ -1127,29 +1131,73 @@ status_t Parameters::set(const String8& paramString) { // RECORDING_HINT (always supported) validatedParams.recordingHint = boolFromString( newParams.get(CameraParameters::KEY_RECORDING_HINT) ); - bool recordingHintChanged = validatedParams.recordingHint != recordingHint; - ALOGV_IF(recordingHintChanged, "%s: Recording hint changed to %d", - __FUNCTION__, recordingHintChanged); + IF_ALOGV() { // Avoid unused variable warning + bool recordingHintChanged = + validatedParams.recordingHint != recordingHint; + if (recordingHintChanged) { + ALOGV("%s: Recording hint changed to %d", + __FUNCTION__, validatedParams.recordingHint); + } + } // PREVIEW_FPS_RANGE - bool fpsRangeChanged = false; - int32_t lastSetFpsRange[2]; - params.getPreviewFpsRange(&lastSetFpsRange[0], &lastSetFpsRange[1]); - lastSetFpsRange[0] /= kFpsToApiScale; - lastSetFpsRange[1] /= kFpsToApiScale; + /** + * Use the single FPS value if it was set later than the range. + * Otherwise, use the range value. + */ + bool fpsUseSingleValue; + { + const char *fpsRange, *fpsSingle; + + fpsRange = newParams.get(CameraParameters::KEY_PREVIEW_FRAME_RATE); + fpsSingle = newParams.get(CameraParameters::KEY_PREVIEW_FPS_RANGE); + + /** + * Pick either the range or the single key if only one was set. + * + * If both are set, pick the one that has greater set order. + */ + if (fpsRange == NULL && fpsSingle == NULL) { + ALOGE("%s: FPS was not set. One of %s or %s must be set.", + __FUNCTION__, CameraParameters::KEY_PREVIEW_FRAME_RATE, + CameraParameters::KEY_PREVIEW_FPS_RANGE); + return BAD_VALUE; + } else if (fpsRange == NULL) { + fpsUseSingleValue = true; + ALOGV("%s: FPS range not set, using FPS single value", + __FUNCTION__); + } else if (fpsSingle == NULL) { + fpsUseSingleValue = false; + ALOGV("%s: FPS single not set, using FPS range value", + __FUNCTION__); + } else { + int fpsKeyOrder; + res = newParams.compareSetOrder( + CameraParameters::KEY_PREVIEW_FRAME_RATE, + CameraParameters::KEY_PREVIEW_FPS_RANGE, + &fpsKeyOrder); + LOG_ALWAYS_FATAL_IF(res != OK, "Impossibly bad FPS keys"); + + fpsUseSingleValue = (fpsKeyOrder > 0); + } + + ALOGV("%s: Preview FPS value is used from '%s'", + __FUNCTION__, fpsUseSingleValue ? "single" : "range"); + } newParams.getPreviewFpsRange(&validatedParams.previewFpsRange[0], &validatedParams.previewFpsRange[1]); + validatedParams.previewFpsRange[0] /= kFpsToApiScale; validatedParams.previewFpsRange[1] /= kFpsToApiScale; - // Compare the FPS range value from the last set() to the current set() - // to determine if the client has changed it - if (validatedParams.previewFpsRange[0] != lastSetFpsRange[0] || - validatedParams.previewFpsRange[1] != lastSetFpsRange[1]) { + // Ignore the FPS range if the FPS single has higher precedence + if (!fpsUseSingleValue) { + ALOGV("%s: Preview FPS range (%d, %d)", __FUNCTION__, + validatedParams.previewFpsRange[0], + validatedParams.previewFpsRange[1]); - fpsRangeChanged = true; camera_metadata_ro_entry_t availablePreviewFpsRanges = staticInfo(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES, 2); for (i = 0; i < availablePreviewFpsRanges.count; i += 2) { @@ -1200,14 +1248,13 @@ status_t Parameters::set(const String8& paramString) { } } - // PREVIEW_FRAME_RATE Deprecated, only use if the preview fps range is - // unchanged this time. The single-value FPS is the same as the minimum of - // the range. To detect whether the application has changed the value of - // previewFps, compare against their last-set preview FPS. - if (!fpsRangeChanged) { + // PREVIEW_FRAME_RATE Deprecated + // - Use only if the single FPS value was set later than the FPS range + if (fpsUseSingleValue) { int previewFps = newParams.getPreviewFrameRate(); - int lastSetPreviewFps = params.getPreviewFrameRate(); - if (previewFps != lastSetPreviewFps || recordingHintChanged) { + ALOGV("%s: Preview FPS single value requested: %d", + __FUNCTION__, previewFps); + { camera_metadata_ro_entry_t availableFrameRates = staticInfo(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES); /** @@ -1276,6 +1323,35 @@ status_t Parameters::set(const String8& paramString) { } } + /** + * Update Preview FPS and Preview FPS ranges based on + * what we actually set. + * + * This updates the API-visible (Camera.Parameters#getParameters) values of + * the FPS fields, not only the internal versions. + * + * Order matters: The value that was set last takes precedence. + * - If the client does a setParameters(getParameters()) we retain + * the same order for preview FPS. + */ + if (!fpsUseSingleValue) { + // Set fps single, then fps range (range wins) + newParams.setPreviewFrameRate( + fpsFromRange(/*min*/validatedParams.previewFpsRange[0], + /*max*/validatedParams.previewFpsRange[1])); + newParams.setPreviewFpsRange( + validatedParams.previewFpsRange[0] * kFpsToApiScale, + validatedParams.previewFpsRange[1] * kFpsToApiScale); + } else { + // Set fps range, then fps single (single wins) + newParams.setPreviewFpsRange( + validatedParams.previewFpsRange[0] * kFpsToApiScale, + validatedParams.previewFpsRange[1] * kFpsToApiScale); + // Set this to the same value, but with higher priority + newParams.setPreviewFrameRate( + newParams.getPreviewFrameRate()); + } + // PICTURE_SIZE newParams.getPictureSize(&validatedParams.pictureWidth, &validatedParams.pictureHeight); diff --git a/services/camera/libcameraservice/api1/client2/Parameters.h b/services/camera/libcameraservice/api1/client2/Parameters.h index 32dbd42..da07ccf 100644 --- a/services/camera/libcameraservice/api1/client2/Parameters.h +++ b/services/camera/libcameraservice/api1/client2/Parameters.h @@ -25,6 +25,7 @@ #include #include #include +#include #include namespace android { @@ -32,7 +33,7 @@ namespace camera2 { /** * Current camera state; this is the full state of the Camera under the old - * camera API (contents of the CameraParameters object in a more-efficient + * camera API (contents of the CameraParameters2 object in a more-efficient * format, plus other state). The enum values are mostly based off the * corresponding camera2 enums, not the camera1 strings. A few are defined here * if they don't cleanly map to camera2 values. @@ -128,7 +129,7 @@ struct Parameters { LIGHTFX_HDR } lightFx; - CameraParameters params; + CameraParameters2 params; String8 paramsFlattened; // These parameters are also part of the camera API-visible state, but not -- cgit v1.1