fix deadlock issues that arise when there are simultaneous
effect control interface calls to proxy and to
non sub-effect wrappers(eg., bundlewrapper) from audioflinger
Also, return NO_ERROR when CMD_OFFLOAD succeeds
Whenever there are parallel calls to proxy and non sub-effects wrappers,
some of the calls are not completed. This is due to deadlock arsing out
of Proxy waiting for the subeffect call to return and subeffect waiting
for proxy to release lock.
The call flow is changed to a cleaner and simple one - Proxy gets the
aeli(effect library info) of subeffects during the EffectGetSubEffects()
call. Therby, proxy will manage the sub effects by itself rather than
going through effects factory.
Change-Id: If4b259da5776f151c1e81a78a0239d342046d923
Signed-off-by: jpadmana <jayashree.r.padmanaban@intel.com>
Bug: 12424044
diff --git a/include/media/EffectsFactoryApi.h b/include/media/EffectsFactoryApi.h
index b1143b9..b1ed7b0 100644
--- a/include/media/EffectsFactoryApi.h
+++ b/include/media/EffectsFactoryApi.h
@@ -171,30 +171,6 @@
////////////////////////////////////////////////////////////////////////////////
int EffectIsNullUuid(const effect_uuid_t *pEffectUuid);
-////////////////////////////////////////////////////////////////////////////////
-//
-// Function: EffectGetSubEffects
-//
-// Description: Returns the descriptors of the sub effects of the effect
-// whose uuid is pointed to by first argument.
-//
-// Input:
-// pEffectUuid: pointer to the effect uuid.
-// size: size of the buffer pointed by pDescriptor.
-//
-// Input/Output:
-// pDescriptor: address where to return the sub effect descriptors.
-//
-// Output:
-// returned value: 0 successful operation.
-// -ENODEV factory failed to initialize
-// -EINVAL invalid pEffectUuid or pDescriptor
-// -ENOENT no effect with this uuid found
-// *pDescriptor: updated with the sub effect descriptors.
-//
-////////////////////////////////////////////////////////////////////////////////
-int EffectGetSubEffects(const effect_uuid_t *pEffectUuid, effect_descriptor_t *pDescriptors, size_t size);
-
#if __cplusplus
} // extern "C"
#endif
diff --git a/media/libeffects/factory/EffectsFactory.c b/media/libeffects/factory/EffectsFactory.c
index f8d6041..6d30d64 100644
--- a/media/libeffects/factory/EffectsFactory.c
+++ b/media/libeffects/factory/EffectsFactory.c
@@ -368,27 +368,21 @@
}
if (e1 == NULL) {
ret = -ENOENT;
- pthread_mutex_unlock(&gLibLock);
goto exit;
}
// release effect in library
if (fx->lib == NULL) {
ALOGW("EffectRelease() fx %p library already unloaded", handle);
- pthread_mutex_unlock(&gLibLock);
} else {
pthread_mutex_lock(&fx->lib->lock);
- // Releasing the gLibLock here as the list access is over as the
- // effect is removed from the list.
- // If the gLibLock is not released, we will have a deadlock situation
- // since we call the sub effect release inside the EffectRelease of Proxy
- pthread_mutex_unlock(&gLibLock);
fx->lib->desc->release_effect(fx->subItfe);
pthread_mutex_unlock(&fx->lib->lock);
}
free(fx);
exit:
+ pthread_mutex_unlock(&gLibLock);
return ret;
}
@@ -404,8 +398,8 @@
// is pointed by the first argument. It searches the gSubEffectList for the
// matching uuid and then copies the corresponding sub effect descriptors
// to the inout param
-int EffectGetSubEffects(const effect_uuid_t *uuid,
- effect_descriptor_t *pDescriptors, size_t size)
+int EffectGetSubEffects(const effect_uuid_t *uuid, sub_effect_entry_t **pSube,
+ size_t size)
{
ALOGV("EffectGetSubEffects() UUID: %08X-%04X-%04X-%04X-%02X%02X%02X%02X%02X"
"%02X\n",uuid->timeLow, uuid->timeMid, uuid->timeHiAndVersion,
@@ -413,8 +407,7 @@
uuid->node[3],uuid->node[4],uuid->node[5]);
// Check if the size of the desc buffer is large enough for 2 subeffects
- if ((uuid == NULL) || (pDescriptors == NULL) ||
- (size < 2*sizeof(effect_descriptor_t))) {
+ if ((uuid == NULL) || (pSube == NULL) || (size < 2)) {
ALOGW("NULL pointer or insufficient memory. Cannot query subeffects");
return -EINVAL;
}
@@ -432,11 +425,10 @@
list_elem_t *subefx = e->sub_elem;
while (subefx != NULL) {
subeffect = (sub_effect_entry_t*)subefx->object;
- d = (effect_descriptor_t*)(subeffect->object);
- pDescriptors[count++] = *d;
+ pSube[count++] = subeffect;
subefx = subefx->next;
}
- ALOGV("EffectGetSubEffects end - copied the sub effect descriptors");
+ ALOGV("EffectGetSubEffects end - copied the sub effect structures");
return count;
}
e = e->next;
diff --git a/media/libeffects/factory/EffectsFactory.h b/media/libeffects/factory/EffectsFactory.h
index 147ff18..560b485 100644
--- a/media/libeffects/factory/EffectsFactory.h
+++ b/media/libeffects/factory/EffectsFactory.h
@@ -20,7 +20,7 @@
#include <cutils/log.h>
#include <pthread.h>
#include <dirent.h>
-#include <media/EffectsFactoryApi.h>
+#include <hardware/audio_effect.h>
#if __cplusplus
extern "C" {
@@ -66,6 +66,32 @@
void *object;
} sub_effect_entry_t;
+
+////////////////////////////////////////////////////////////////////////////////
+//
+// Function: EffectGetSubEffects
+//
+// Description: Returns the descriptors of the sub effects of the effect
+// whose uuid is pointed to by first argument.
+//
+// Input:
+// pEffectUuid: pointer to the effect uuid.
+// size: max number of sub_effect_entry_t * in pSube.
+//
+// Input/Output:
+// pSube: address where to return the sub effect structures.
+// Output:
+// returned value: 0 successful operation.
+// -ENODEV factory failed to initialize
+// -EINVAL invalid pEffectUuid or pDescriptor
+// -ENOENT no effect with this uuid found
+// *pDescriptor: updated with the sub effect descriptors.
+//
+////////////////////////////////////////////////////////////////////////////////
+int EffectGetSubEffects(const effect_uuid_t *pEffectUuid,
+ sub_effect_entry_t **pSube,
+ size_t size);
+
#if __cplusplus
} // extern "C"
#endif
diff --git a/media/libeffects/proxy/Android.mk b/media/libeffects/proxy/Android.mk
index 01b3be1..a1894b7 100644
--- a/media/libeffects/proxy/Android.mk
+++ b/media/libeffects/proxy/Android.mk
@@ -28,7 +28,8 @@
LOCAL_C_INCLUDES := \
system/media/audio_effects/include \
- bionic/libc/include
+ bionic/libc/include \
+ frameworks/av/media/libeffects/factory
include $(BUILD_SHARED_LIBRARY)
diff --git a/media/libeffects/proxy/EffectProxy.cpp b/media/libeffects/proxy/EffectProxy.cpp
index dd4ad08..f93a143 100644
--- a/media/libeffects/proxy/EffectProxy.cpp
+++ b/media/libeffects/proxy/EffectProxy.cpp
@@ -56,6 +56,8 @@
effect_handle_t *pHandle) {
effect_descriptor_t* desc;
+ audio_effect_library_t** aeli;
+ sub_effect_entry_t** sube;
EffectContext* pContext;
if (pHandle == NULL || uuid == NULL) {
ALOGE("EffectProxyCreate() called with NULL pointer");
@@ -74,31 +76,52 @@
// Get the HW and SW sub effect descriptors from the effects factory
desc = new effect_descriptor_t[SUB_FX_COUNT];
+ aeli = new audio_effect_library_t*[SUB_FX_COUNT];
+ sube = new sub_effect_entry_t*[SUB_FX_COUNT];
+ pContext->sube = new sub_effect_entry_t*[SUB_FX_COUNT];
pContext->desc = new effect_descriptor_t[SUB_FX_COUNT];
- int retValue = EffectGetSubEffects(uuid, desc,
- sizeof(effect_descriptor_t) * SUB_FX_COUNT);
+ pContext->aeli = new audio_effect_library_t*[SUB_FX_COUNT];
+ int retValue = EffectGetSubEffects(uuid, sube, SUB_FX_COUNT);
// EffectGetSubEffects returns the number of sub-effects copied.
if (retValue != SUB_FX_COUNT) {
ALOGE("EffectCreate() could not get the sub effects");
- delete desc;
- delete pContext->desc;
+ delete[] sube;
+ delete[] desc;
+ delete[] aeli;
+ delete[] pContext->sube;
+ delete[] pContext->desc;
+ delete[] pContext->aeli;
return -EINVAL;
}
// Check which is the HW descriptor and copy the descriptors
// to the Context desc array
// Also check if there is only one HW and one SW descriptor.
// HW descriptor alone has the HW_TUNNEL flag.
+ desc[0] = *(effect_descriptor_t*)(sube[0])->object;
+ desc[1] = *(effect_descriptor_t*)(sube[1])->object;
+ aeli[0] = sube[0]->lib->desc;
+ aeli[1] = sube[1]->lib->desc;
if ((desc[0].flags & EFFECT_FLAG_HW_ACC_TUNNEL) &&
!(desc[1].flags & EFFECT_FLAG_HW_ACC_TUNNEL)) {
+ pContext->sube[SUB_FX_OFFLOAD] = sube[0];
pContext->desc[SUB_FX_OFFLOAD] = desc[0];
+ pContext->aeli[SUB_FX_OFFLOAD] = aeli[0];
+ pContext->sube[SUB_FX_HOST] = sube[1];
pContext->desc[SUB_FX_HOST] = desc[1];
+ pContext->aeli[SUB_FX_HOST] = aeli[1];
}
else if ((desc[1].flags & EFFECT_FLAG_HW_ACC_TUNNEL) &&
!(desc[0].flags & EFFECT_FLAG_HW_ACC_TUNNEL)) {
+ pContext->sube[SUB_FX_HOST] = sube[0];
pContext->desc[SUB_FX_HOST] = desc[0];
+ pContext->aeli[SUB_FX_HOST] = aeli[0];
+ pContext->sube[SUB_FX_OFFLOAD] = sube[1];
pContext->desc[SUB_FX_OFFLOAD] = desc[1];
+ pContext->aeli[SUB_FX_OFFLOAD] = aeli[1];
}
- delete desc;
+ delete[] desc;
+ delete[] aeli;
+ delete[] sube;
#if (LOG_NDEBUG == 0)
effect_uuid_t uuid_print = pContext->desc[SUB_FX_HOST].uuid;
ALOGV("EffectCreate() UUID of HOST: %08X-%04X-%04X-%04X-%02X%02X%02X%02X"
@@ -128,13 +151,15 @@
return -EINVAL;
}
ALOGV("EffectRelease");
- delete pContext->desc;
+ delete[] pContext->desc;
free(pContext->replyData);
if (pContext->eHandle[SUB_FX_HOST])
- EffectRelease(pContext->eHandle[SUB_FX_HOST]);
+ pContext->aeli[SUB_FX_HOST]->release_effect(pContext->eHandle[SUB_FX_HOST]);
if (pContext->eHandle[SUB_FX_OFFLOAD])
- EffectRelease(pContext->eHandle[SUB_FX_OFFLOAD]);
+ pContext->aeli[SUB_FX_OFFLOAD]->release_effect(pContext->eHandle[SUB_FX_OFFLOAD]);
+ delete[] pContext->aeli;
+ delete[] pContext->sube;
delete pContext;
pContext = NULL;
return 0;
@@ -187,7 +212,8 @@
}
if (pContext->eHandle[SUB_FX_HOST] == NULL) {
ALOGV("Effect_command() Calling HOST EffectCreate");
- status = EffectCreate(&pContext->desc[SUB_FX_HOST].uuid,
+ status = pContext->aeli[SUB_FX_HOST]->create_effect(
+ &pContext->desc[SUB_FX_HOST].uuid,
pContext->sessionId, pContext->ioId,
&(pContext->eHandle[SUB_FX_HOST]));
if (status != NO_ERROR || (pContext->eHandle[SUB_FX_HOST] == NULL)) {
@@ -197,11 +223,13 @@
}
if (pContext->eHandle[SUB_FX_OFFLOAD] == NULL) {
ALOGV("Effect_command() Calling OFFLOAD EffectCreate");
- status = EffectCreate(&pContext->desc[SUB_FX_OFFLOAD].uuid,
+ status = pContext->aeli[SUB_FX_OFFLOAD]->create_effect(
+ &pContext->desc[SUB_FX_OFFLOAD].uuid,
pContext->sessionId, pContext->ioId,
&(pContext->eHandle[SUB_FX_OFFLOAD]));
if (status != NO_ERROR || (pContext->eHandle[SUB_FX_OFFLOAD] == NULL)) {
ALOGV("Effect_command() Error creating HW effect");
+ pContext->eHandle[SUB_FX_OFFLOAD] = NULL;
// Do not return error here as SW effect is created
// Return error if the CMD_OFFLOAD sends the index as OFFLOAD
}
@@ -233,11 +261,17 @@
// Update the DSP wrapper with the new ioHandle.
// Pass the OFFLOAD command to the wrapper.
// The DSP wrapper needs to handle this CMD
- if (pContext->eHandle[SUB_FX_OFFLOAD])
- status = (*pContext->eHandle[SUB_FX_OFFLOAD])->command(
- pContext->eHandle[SUB_FX_OFFLOAD], cmdCode, cmdSize,
- pCmdData, replySize, pReplyData);
- return status;
+ if (pContext->eHandle[SUB_FX_OFFLOAD]) {
+ ALOGV("Effect_command: Calling OFFLOAD command");
+ return (*pContext->eHandle[SUB_FX_OFFLOAD])->command(
+ pContext->eHandle[SUB_FX_OFFLOAD], cmdCode, cmdSize,
+ pCmdData, replySize, pReplyData);
+ }
+ *(int*)pReplyData = NO_ERROR;
+ ALOGV("Effect_command OFFLOAD return 0, replyData %d",
+ *(int*)pReplyData);
+
+ return NO_ERROR;
}
int index = pContext->index;
diff --git a/media/libeffects/proxy/EffectProxy.h b/media/libeffects/proxy/EffectProxy.h
index acbe17e..046b93e 100644
--- a/media/libeffects/proxy/EffectProxy.h
+++ b/media/libeffects/proxy/EffectProxy.h
@@ -16,6 +16,8 @@
#include <hardware/audio.h>
#include <hardware/audio_effect.h>
+#include "EffectsFactory.h"
+
namespace android {
enum {
SUB_FX_HOST, // Index of HOST in the descriptor and handle arrays
@@ -62,7 +64,9 @@
struct EffectContext {
const struct effect_interface_s *common_itfe; // Holds the itfe of the Proxy
+ sub_effect_entry_t** sube; // Points to the sub effects
effect_descriptor_t* desc; // Points to the sub effect descriptors
+ audio_effect_library_t** aeli; // Points to the sub effect aeli
effect_handle_t eHandle[SUB_FX_COUNT]; // The effect handles of the sub effects
int index; // The index that is currently active - HOST or OFFLOAD
int32_t sessionId; // The sessiond in which the effect is created.
