From 574cc909d4d7659f641de10e2f571e44521d3979 Mon Sep 17 00:00:00 2001 From: Kevin Rocard Date: Tue, 17 Apr 2018 11:00:58 -0700 Subject: [PATCH] Audio: setParam improve status_t to Result consistency The rest of the API (*::analyseStatus) returns NOT_SUPPORTED when the legacy API returns -ENOSYS. setParameter legacy -> treble shim did not follow this conversion due to the legacy API stating that for get_paramers, -ENOSYS should be returned if "the implementation does not accept a parameter change while the output is active but the parameter is acceptable otherwise", aka INVALID_STATE. Thus setParameter shim used to return - OK for OK - INVALID_STATE for -ENOSYS - INVALID_ARGUMENTS for everything else This leads to several problems: - an implementation of the legacy API can not report NOT_SUPPORTED - is inconsistent with the rest of the status_t conversion methods - shim methods implemented over getParameter can not distinguish failures error required by the .hal documentation Most importantly, on the system side, the Result is transformed to a status_t again but without any special logic for methods wrapping getParameter in the shim. See: analyzeResult in frameworks/av/media/libaudiohal/2.0/ConversionHelperHidl.cpp This can not be changed as the system can not know which methods are implemented with a legacy wrapper under the Treble API boundary. This mean that if: - hal return -ENOSYS () - shim converts it to INVALID_STATE - libaudiohal converts it to NOT_ENOUGH_DATA () Thus the checkForNewParameter_l's "status == INVALID_OPERATION" test in frameworks/av/services/audioflinger/Threads.cpp are now always false and broken. This has been broken since the introduction of the Treble shim for O. Thus setParam now converts status_t to Result in the same way as all the other shim methods. Bug: 72873273 Bug: 69811500 Bug: 69010523 Test: playback and record for media and voice call Change-Id: I41204c0807d2bd4675e941771cbc9a43d7d14855 Merged-In: I41328afce56ce31d4a26159ca2d4b16d14cce05b Signed-off-by: Kevin Rocard --- audio/2.0/default/Device.cpp | 18 +------------ audio/2.0/default/ParametersUtil.cpp | 8 ++---- audio/2.0/default/Stream.cpp | 20 +++------------ audio/2.0/default/Util.h | 38 ++++++++++++++++++++++++++++ 4 files changed, 44 insertions(+), 40 deletions(-) diff --git a/audio/2.0/default/Device.cpp b/audio/2.0/default/Device.cpp index 3727966279..8b83e46441 100644 --- a/audio/2.0/default/Device.cpp +++ b/audio/2.0/default/Device.cpp @@ -48,23 +48,7 @@ Device::~Device() { } Result Device::analyzeStatus(const char* funcName, int status) { - if (status != 0) { - ALOGW("Device %p %s: %s", mDevice, funcName, strerror(-status)); - } - switch (status) { - case 0: - return Result::OK; - case -EINVAL: - return Result::INVALID_ARGUMENTS; - case -ENODATA: - return Result::INVALID_STATE; - case -ENODEV: - return Result::NOT_INITIALIZED; - case -ENOSYS: - return Result::NOT_SUPPORTED; - default: - return Result::INVALID_STATE; - } + return util::analyzeStatus("Device", funcName, status); } void Device::closeInputStream(audio_stream_in_t* stream) { diff --git a/audio/2.0/default/ParametersUtil.cpp b/audio/2.0/default/ParametersUtil.cpp index 2140885f20..257c8e5f0d 100644 --- a/audio/2.0/default/ParametersUtil.cpp +++ b/audio/2.0/default/ParametersUtil.cpp @@ -15,6 +15,7 @@ */ #include "ParametersUtil.h" +#include "Util.h" namespace android { namespace hardware { @@ -141,12 +142,7 @@ Result ParametersUtil::setParametersImpl( Result ParametersUtil::setParams(const AudioParameter& param) { int halStatus = halSetParameters(param.toString().string()); - if (halStatus == OK) - return Result::OK; - else if (halStatus == -ENOSYS) - return Result::INVALID_STATE; - else - return Result::INVALID_ARGUMENTS; + return util::analyzeStatus(halStatus); } } // namespace implementation diff --git a/audio/2.0/default/Stream.cpp b/audio/2.0/default/Stream.cpp index effdd28507..55ae6dbd01 100644 --- a/audio/2.0/default/Stream.cpp +++ b/audio/2.0/default/Stream.cpp @@ -28,6 +28,7 @@ #include "Conversions.h" #include "EffectMap.h" #include "Stream.h" +#include "Util.h" namespace android { namespace hardware { @@ -45,29 +46,14 @@ Stream::~Stream() { // static Result Stream::analyzeStatus(const char* funcName, int status) { - static const std::vector empty; - return analyzeStatus(funcName, status, empty); + return util::analyzeStatus("stream", funcName, status); } -template -inline bool element_in(T e, const std::vector& v) { - return std::find(v.begin(), v.end(), e) != v.end(); -} // static Result Stream::analyzeStatus(const char* funcName, int status, const std::vector& ignoreErrors) { - if (status != 0 && (ignoreErrors.empty() || !element_in(-status, ignoreErrors))) { - ALOGW("Error from HAL stream in function %s: %s", funcName, strerror(-status)); - } - switch (status) { - case 0: return Result::OK; - case -EINVAL: return Result::INVALID_ARGUMENTS; - case -ENODATA: return Result::INVALID_STATE; - case -ENODEV: return Result::NOT_INITIALIZED; - case -ENOSYS: return Result::NOT_SUPPORTED; - default: return Result::INVALID_STATE; - } + return util::analyzeStatus("stream", funcName, status, ignoreErrors); } char* Stream::halGetParameters(const char* keys) { diff --git a/audio/2.0/default/Util.h b/audio/2.0/default/Util.h index 72eea5037b..55019b8798 100644 --- a/audio/2.0/default/Util.h +++ b/audio/2.0/default/Util.h @@ -17,6 +17,11 @@ #ifndef ANDROID_HARDWARE_AUDIO_V2_0_UTIL_H #define ANDROID_HARDWARE_AUDIO_V2_0_UTIL_H +#include +#include + +#include + namespace android { namespace hardware { namespace audio { @@ -28,6 +33,39 @@ constexpr bool isGainNormalized(float gain) { return gain >= 0.0 && gain <= 1.0; } +namespace util { + +template +inline bool element_in(T e, const std::vector& v) { + return std::find(v.begin(), v.end(), e) != v.end(); +} + +static inline Result analyzeStatus(status_t status) { + switch (status) { + case 0: + return Result::OK; + case -EINVAL: + return Result::INVALID_ARGUMENTS; + case -ENODATA: + return Result::INVALID_STATE; + case -ENODEV: + return Result::NOT_INITIALIZED; + case -ENOSYS: + return Result::NOT_SUPPORTED; + default: + return Result::INVALID_STATE; + } +} + +static inline Result analyzeStatus(const char* className, const char* funcName, status_t status, + const std::vector& ignoreErrors = {}) { + if (status != 0 && !element_in(-status, ignoreErrors)) { + ALOGW("Error from HAL %s in function %s: %s", className, funcName, strerror(-status)); + } + return analyzeStatus(status); +} + +} // namespace util } // namespace implementation } // namespace V2_0 } // namespace audio