diff options
author | Hangyu Kuang <hkuang@google.com> | 2019-04-25 16:22:14 -0700 |
---|---|---|
committer | Hangyu Kuang <hkuang@google.com> | 2019-04-29 11:16:04 -0700 |
commit | a67de76fc920aed26b2351ff4030bd0c6f110514 (patch) | |
tree | b8942ea2ef104ca9ef51d156312f5b8d95527594 | |
parent | ba5b1775ae194d023f62aeb5cee68f616c938ad2 (diff) | |
download | av-a67de76fc920aed26b2351ff4030bd0c6f110514.tar.gz |
[ECOService]: Add implementaion for removing listener and provider.
Also add several more things:
1) Add key handling inside the ecosession.
2) Add the sessionlock to protect the access to ecosession.
3) Change some log from ALOGD to ALOGI to avoid log spam.
Bug: 117877984
Test: Unit test.
Change-Id: Ib02dcd278dfa528745fb9599afde22931849ef20
-rw-r--r-- | media/eco/ECOService.cpp | 7 | ||||
-rw-r--r-- | media/eco/ECOSession.cpp | 94 | ||||
-rw-r--r-- | media/eco/include/eco/ECOSession.h | 9 | ||||
-rw-r--r-- | media/eco/tests/EcoServiceTest.cpp | 4 | ||||
-rw-r--r-- | media/eco/tests/EcoSessionTest.cpp | 97 |
5 files changed, 178 insertions, 33 deletions
diff --git a/media/eco/ECOService.cpp b/media/eco/ECOService.cpp index 0c25c7e..bc09d37 100644 --- a/media/eco/ECOService.cpp +++ b/media/eco/ECOService.cpp @@ -75,7 +75,7 @@ ECOService::ECOService() : BnECOService() { SessionConfig newCfg(width, height, isCameraRecording); - ALOGD("session count is %zu", mSessionConfigToSessionMap.size()); + ALOGD("session count before is %zu", mSessionConfigToSessionMap.size()); Mutex::Autolock lock(mServiceLock); bool foundSession = false; @@ -102,6 +102,7 @@ ECOService::ECOService() : BnECOService() { } // Insert the new session into the map. mSessionConfigToSessionMap[newCfg] = *_aidl_return; + ALOGD("session count after is %zu", mSessionConfigToSessionMap.size()); return binder::Status::ok(); } @@ -139,7 +140,9 @@ void ECOService::SanitizeSession( if (isEmptySession(it->second)) { it = mSessionConfigToSessionMap.erase(it); } else { - if(callback != nullptr) { callback(it); }; + if (callback != nullptr) { + callback(it); + }; it++; } } diff --git a/media/eco/ECOSession.cpp b/media/eco/ECOSession.cpp index a6d8434..d86ad5b 100644 --- a/media/eco/ECOSession.cpp +++ b/media/eco/ECOSession.cpp @@ -17,7 +17,6 @@ //#define LOG_NDEBUG 0 #define LOG_TAG "ECOSession" //#define DEBUG_ECO_SESSION - #include "eco/ECOSession.h" #include <binder/BinderService.h> @@ -26,6 +25,7 @@ #include <pthread.h> #include <stdio.h> #include <sys/types.h> +#include <utils/Log.h> #include <algorithm> #include <climits> @@ -85,7 +85,6 @@ ECOSession::~ECOSession() { ALOGD("ECOSession: join the thread"); mThread.join(); } - ALOGI("ECOSession destroyed with w: %d, h: %d, isCameraRecording: %d", mWidth, mHeight, mIsCameraRecording); } @@ -139,7 +138,7 @@ bool ECOSession::processStats(const ECOData& stats) { } bool ECOSession::processSessionStats(const ECOData& stats) { - ALOGD("processSessionStats"); + ALOGV("processSessionStats"); ECOData info(ECOData::DATA_TYPE_INFO, systemTime(SYSTEM_TIME_BOOTTIME)); info.setString(KEY_INFO_TYPE, VALUE_INFO_TYPE_SESSION); @@ -149,19 +148,40 @@ bool ECOSession::processSessionStats(const ECOData& stats) { ECOData::ECODataKeyValuePair entry = iter.next(); const std::string& key = entry.first; const ECOData::ECODataValueType value = entry.second; - ALOGD("Processing key: %s", key.c_str()); - if (!key.compare(ENCODER_TYPE)) { + ALOGV("Processing key: %s", key.c_str()); + if (!key.compare(KEY_STATS_TYPE)) { + // Skip the key KEY_STATS_TYPE as that has been parsed already. + continue; + } else if (!key.compare(ENCODER_TYPE)) { mCodecType = std::get<int32_t>(value); + ALOGV("codec type is %d", mCodecType); } else if (!key.compare(ENCODER_PROFILE)) { mCodecProfile = std::get<int32_t>(value); + ALOGV("codec profile is %d", mCodecProfile); } else if (!key.compare(ENCODER_LEVEL)) { mCodecLevel = std::get<int32_t>(value); + ALOGV("codec level is %d", mCodecLevel); } else if (!key.compare(ENCODER_TARGET_BITRATE_BPS)) { mBitrateBps = std::get<int32_t>(value); + ALOGV("codec bitrate is %d", mBitrateBps); } else if (!key.compare(ENCODER_KFI_FRAMES)) { mKeyFrameIntervalFrames = std::get<int32_t>(value); + ALOGV("codec kfi is %d", mKeyFrameIntervalFrames); } else if (!key.compare(ENCODER_FRAMERATE_FPS)) { mFramerateFps = std::get<float>(value); + ALOGV("codec framerate is %f", mFramerateFps); + } else if (!key.compare(ENCODER_INPUT_WIDTH)) { + int32_t width = std::get<int32_t>(value); + if (width != mWidth) { + ALOGW("Codec width: %d, expected: %d", width, mWidth); + } + ALOGV("codec width is %d", width); + } else if (!key.compare(ENCODER_INPUT_HEIGHT)) { + int32_t height = std::get<int32_t>(value); + if (height != mHeight) { + ALOGW("Codec height: %d, expected: %d", height, mHeight); + } + ALOGV("codec height is %d", height); } else { ALOGW("Unknown frame stats key %s from provider.", key.c_str()); continue; @@ -236,7 +256,11 @@ bool ECOSession::processFrameStats(const ECOData& stats) { Status ECOSession::addStatsProvider( const sp<::android::media::eco::IECOServiceStatsProvider>& provider, const ::android::media::eco::ECOData& config, bool* status) { - ALOGV("%s: Add provider %p", __FUNCTION__, provider.get()); + ::android::String16 name; + provider->getName(&name); + + ALOGV("Try to add stats provider name: %s uid: %d pid %d", ::android::String8(name).string(), + IPCThreadState::self()->getCallingUid(), IPCThreadState::self()->getCallingPid()); if (provider == nullptr) { ALOGE("%s: provider must not be null", __FUNCTION__); @@ -244,12 +268,17 @@ Status ECOSession::addStatsProvider( return STATUS_ERROR(ERROR_ILLEGAL_ARGUMENT, "Null provider given to addStatsProvider"); } - // TODO: Add mutex to protect the access to provider and listener. + std::scoped_lock<std::mutex> lock(mSessionLock); + if (mProvider != nullptr) { - ALOGE("ECOService 1.0 only supports one stats provider"); + ::android::String16 name; + mProvider->getName(&name); + String8 errorMsg = String8::format( + "ECOService 1.0 only supports one stats provider, current provider: %s", + ::android::String8(name).string()); + ALOGE("%s", errorMsg.string()); *status = false; - return STATUS_ERROR(ERROR_ALREADY_EXISTS, - "ECOService 1.0 only supports one stats provider"); + return STATUS_ERROR(ERROR_ALREADY_EXISTS, errorMsg.string()); } // TODO: Handle the provider config. @@ -259,33 +288,35 @@ Status ECOSession::addStatsProvider( return STATUS_ERROR(ERROR_ILLEGAL_ARGUMENT, "Provider config is invalid"); } - ::android::String16 name; - provider->getName(&name); - - ALOGD("Stats provider name: %s uid: %d pid %d", ::android::String8(name).string(), - IPCThreadState::self()->getCallingUid(), IPCThreadState::self()->getCallingPid()); - mProvider = provider; *status = true; return binder::Status::ok(); } Status ECOSession::removeStatsProvider( - const sp<::android::media::eco::IECOServiceStatsProvider>& /* listener */, - bool* /* _aidl_return */) { - //TODO(hkuang): Add implementation. + const sp<::android::media::eco::IECOServiceStatsProvider>& provider, bool* status) { + std::scoped_lock<std::mutex> lock(mSessionLock); + // Check if the provider is the same as current provider for the session. + if (provider.get() != mProvider.get()) { + *status = false; + return STATUS_ERROR(ERROR_ILLEGAL_ARGUMENT, "Provider does not match"); + } + + mProvider = nullptr; + *status = true; return binder::Status::ok(); } Status ECOSession::addInfoListener( const sp<::android::media::eco::IECOServiceInfoListener>& listener, const ::android::media::eco::ECOData& config, bool* status) { - ALOGD("%s: Add listener %p", __FUNCTION__, listener.get()); + ALOGV("%s: Add listener %p", __FUNCTION__, listener.get()); + std::scoped_lock<std::mutex> lock(mSessionLock); if (mListener != nullptr) { - ALOGE("ECOService 1.0 only supports one info listener"); + ALOGE("ECOService 1.0 only supports one listener"); *status = false; - return STATUS_ERROR(ERROR_ALREADY_EXISTS, "ECOService 1.0 only supports one info listener"); + return STATUS_ERROR(ERROR_ALREADY_EXISTS, "ECOService 1.0 only supports one listener"); } if (listener == nullptr) { @@ -330,14 +361,21 @@ Status ECOSession::addInfoListener( } Status ECOSession::removeInfoListener( - const sp<::android::media::eco::IECOServiceInfoListener>& /* provider */, - bool* /* _aidl_return */) { - //TODO(hkuang): Add implementation. + const sp<::android::media::eco::IECOServiceInfoListener>& listener, bool* _aidl_return) { + std::scoped_lock<std::mutex> lock(mSessionLock); + // Check if the listener is the same as current listener for the session. + if (listener.get() != mListener.get()) { + *_aidl_return = false; + return STATUS_ERROR(ERROR_ILLEGAL_ARGUMENT, "Listener does not match"); + } + + mListener = nullptr; + *_aidl_return = true; return binder::Status::ok(); } Status ECOSession::pushNewStats(const ::android::media::eco::ECOData& stats, bool*) { - ALOGD("ECOSession get new stats type: %s", stats.getDataTypeString().c_str()); + ALOGV("ECOSession get new stats type: %s", stats.getDataTypeString().c_str()); std::unique_lock<std::mutex> lock(mStatsQueueLock); mStatsQueue.push_back(stats); mStatsQueueWaitCV.notify_all(); @@ -345,21 +383,25 @@ Status ECOSession::pushNewStats(const ::android::media::eco::ECOData& stats, boo } Status ECOSession::getWidth(int32_t* _aidl_return) { + std::scoped_lock<std::mutex> lock(mSessionLock); *_aidl_return = mWidth; return binder::Status::ok(); } Status ECOSession::getHeight(int32_t* _aidl_return) { + std::scoped_lock<std::mutex> lock(mSessionLock); *_aidl_return = mHeight; return binder::Status::ok(); } Status ECOSession::getNumOfListeners(int32_t* _aidl_return) { + std::scoped_lock<std::mutex> lock(mSessionLock); *_aidl_return = (mListener == nullptr ? 0 : 1); return binder::Status::ok(); } Status ECOSession::getNumOfProviders(int32_t* _aidl_return) { + std::scoped_lock<std::mutex> lock(mSessionLock); *_aidl_return = (mProvider == nullptr ? 0 : 1); return binder::Status::ok(); } diff --git a/media/eco/include/eco/ECOSession.h b/media/eco/include/eco/ECOSession.h index 7429742..921efbd 100644 --- a/media/eco/include/eco/ECOSession.h +++ b/media/eco/include/eco/ECOSession.h @@ -100,6 +100,9 @@ private: bool processStats(const ECOData& stats); + // Lock guarding ECO session state + std::mutex mSessionLock; + // Process the session stats received from provider. bool processSessionStats(const ECOData& stats); @@ -131,13 +134,13 @@ private: std::thread mThread; // Width of the encoding session in number of pixels. - int32_t mWidth; + const int32_t mWidth; // Height of the encoding session in number of pixels. - int32_t mHeight; + const int32_t mHeight; // Whether the encoding is for camera recording. - bool mIsCameraRecording; + const bool mIsCameraRecording; // Encoder codec type of the encoding session. -1 means not available. int32_t mCodecType; diff --git a/media/eco/tests/EcoServiceTest.cpp b/media/eco/tests/EcoServiceTest.cpp index ad8530f..594ee97 100644 --- a/media/eco/tests/EcoServiceTest.cpp +++ b/media/eco/tests/EcoServiceTest.cpp @@ -49,7 +49,7 @@ static constexpr uint32_t kTestHeight = 720; static constexpr bool kIsCameraRecording = true; static constexpr float kFrameRate = 30.0f; -} // namespace anonymous +} // namespace // A helper class to help create ECOService and manage ECOService. class EcoServiceTest : public ::testing::Test { @@ -154,7 +154,7 @@ TEST_F(EcoServiceTest, ObtainTwoSessions) { service->obtainSession(kTestWidth - 1, kTestHeight - 1, kIsCameraRecording, &session2); EXPECT_TRUE(session2); - // The two session instances must not be the same. + // The two session instances must not be the same. EXPECT_TRUE(IInterface::asBinder(session1) != IInterface::asBinder(session2)); // Check the session number. diff --git a/media/eco/tests/EcoSessionTest.cpp b/media/eco/tests/EcoSessionTest.cpp index 0dfd9b2..4538617 100644 --- a/media/eco/tests/EcoSessionTest.cpp +++ b/media/eco/tests/EcoSessionTest.cpp @@ -408,6 +408,103 @@ TEST_F(EcoSessionTest, TestSessionWithProviderAndListenerSimpleTest) { EXPECT_EQ(frameSize, 56); } +TEST_F(EcoSessionTest, TestRemoveMatchProvider) { + sp<ECOSession> ecoSession = createSession(kTestWidth, kTestHeight, kIsCameraRecording); + EXPECT_TRUE(ecoSession); + + sp<FakeECOServiceStatsProvider> fakeProvider1 = new FakeECOServiceStatsProvider( + kTestWidth, kTestHeight, kIsCameraRecording, kFrameRate, ecoSession); + + ECOData providerConfig(ECOData::DATA_TYPE_STATS_PROVIDER_CONFIG, + systemTime(SYSTEM_TIME_BOOTTIME)); + bool res; + Status status = ecoSession->addStatsProvider(fakeProvider1, providerConfig, &res); + EXPECT_TRUE(res); + EXPECT_TRUE(status.isOk()); + + status = ecoSession->removeStatsProvider(fakeProvider1, &res); + EXPECT_TRUE(res); + EXPECT_TRUE(status.isOk()); +} + +TEST_F(EcoSessionTest, TestRemoveMisMatchProvider) { + sp<ECOSession> ecoSession = createSession(kTestWidth, kTestHeight, kIsCameraRecording); + EXPECT_TRUE(ecoSession); + + sp<FakeECOServiceStatsProvider> fakeProvider1 = new FakeECOServiceStatsProvider( + kTestWidth, kTestHeight, kIsCameraRecording, kFrameRate, ecoSession); + + ECOData providerConfig(ECOData::DATA_TYPE_STATS_PROVIDER_CONFIG, + systemTime(SYSTEM_TIME_BOOTTIME)); + bool res; + Status status = ecoSession->addStatsProvider(fakeProvider1, providerConfig, &res); + EXPECT_TRUE(res); + EXPECT_TRUE(status.isOk()); + + sp<FakeECOServiceStatsProvider> fakeProvider2 = new FakeECOServiceStatsProvider( + kTestWidth, kTestHeight, kIsCameraRecording, kFrameRate, ecoSession); + + status = ecoSession->removeStatsProvider(fakeProvider2, &res); + EXPECT_FALSE(res); + EXPECT_FALSE(status.isOk()); +} + +TEST_F(EcoSessionTest, TestRemoveMatchListener) { + sp<ECOSession> ecoSession = createSession(kTestWidth, kTestHeight, kIsCameraRecording); + EXPECT_TRUE(ecoSession); + + // Create listener. + sp<FakeECOServiceInfoListener> fakeListener = + new FakeECOServiceInfoListener(kTestWidth, kTestHeight, kIsCameraRecording, ecoSession); + + // Create the listener config. + ECOData listenerConfig(ECOData::DATA_TYPE_INFO_LISTENER_CONFIG, + systemTime(SYSTEM_TIME_BOOTTIME)); + listenerConfig.setString(KEY_LISTENER_NAME, "FakeECOServiceInfoListener"); + listenerConfig.setInt32(KEY_LISTENER_TYPE, ECOServiceInfoListener::INFO_LISTENER_TYPE_CAMERA); + + // Specify the qp thresholds for receiving notification. + listenerConfig.setInt32(KEY_LISTENER_QP_BLOCKINESS_THRESHOLD, 40); + listenerConfig.setInt32(KEY_LISTENER_QP_CHANGE_THRESHOLD, 5); + + bool res; + Status status = ecoSession->addInfoListener(fakeListener, listenerConfig, &res); + + status = ecoSession->removeInfoListener(fakeListener, &res); + EXPECT_TRUE(res); + EXPECT_TRUE(status.isOk()); +} + +TEST_F(EcoSessionTest, TestRemoveMisMatchListener) { + sp<ECOSession> ecoSession = createSession(kTestWidth, kTestHeight, kIsCameraRecording); + EXPECT_TRUE(ecoSession); + + // Create listener. + sp<FakeECOServiceInfoListener> fakeListener = + new FakeECOServiceInfoListener(kTestWidth, kTestHeight, kIsCameraRecording, ecoSession); + + // Create the listener config. + ECOData listenerConfig(ECOData::DATA_TYPE_INFO_LISTENER_CONFIG, + systemTime(SYSTEM_TIME_BOOTTIME)); + listenerConfig.setString(KEY_LISTENER_NAME, "FakeECOServiceInfoListener"); + listenerConfig.setInt32(KEY_LISTENER_TYPE, ECOServiceInfoListener::INFO_LISTENER_TYPE_CAMERA); + + // Specify the qp thresholds for receiving notification. + listenerConfig.setInt32(KEY_LISTENER_QP_BLOCKINESS_THRESHOLD, 40); + listenerConfig.setInt32(KEY_LISTENER_QP_CHANGE_THRESHOLD, 5); + + bool res; + Status status = ecoSession->addInfoListener(fakeListener, listenerConfig, &res); + + // Create listener. + sp<FakeECOServiceInfoListener> fakeListener2 = + new FakeECOServiceInfoListener(kTestWidth, kTestHeight, kIsCameraRecording, ecoSession); + + status = ecoSession->removeInfoListener(fakeListener2, &res); + EXPECT_FALSE(res); + EXPECT_FALSE(status.isOk()); +} + } // namespace eco } // namespace media } // namespace android |