MediaCodec: fix possible crash at stop & error
If an error occurred during stop() operation but the operation
completed anyway, it may lead to an assert because the error already
sent response to pending reply.
Bug: 154678891
Test: atest CtsMediaTestCases -- --module-arg CtsMediaTestCases:size:small
Test: atest mediacodecTest
Merged-In: Id88d546a22db26495a6d535cbf3e191a21ea21fd
Change-Id: Id88d546a22db26495a6d535cbf3e191a21ea21fd
diff --git a/media/libstagefright/MediaCodec.cpp b/media/libstagefright/MediaCodec.cpp
index 298a3f9..98b01916 100644
--- a/media/libstagefright/MediaCodec.cpp
+++ b/media/libstagefright/MediaCodec.cpp
@@ -2133,6 +2133,8 @@
}
bool sendErrorResponse = true;
+ std::string origin{"kWhatError:"};
+ origin += stateString(mState);
switch (mState) {
case INITIALIZING:
@@ -2184,14 +2186,14 @@
// be a shutdown complete notification after
// all.
- // note that we're directly going from
+ // note that we may be directly going from
// STOPPING->UNINITIALIZED, instead of the
// usual STOPPING->INITIALIZED state.
setState(UNINITIALIZED);
if (mState == RELEASING) {
mComponentName.clear();
}
- postPendingRepliesAndDeferredMessages();
+ postPendingRepliesAndDeferredMessages(origin + ":dead");
sendErrorResponse = false;
}
break;
@@ -2282,7 +2284,7 @@
// released by ResourceManager.
finalErr = DEAD_OBJECT;
}
- postPendingRepliesAndDeferredMessages(finalErr);
+ postPendingRepliesAndDeferredMessages(origin, finalErr);
}
break;
}
@@ -2330,7 +2332,7 @@
MediaResource::CodecResource(mFlags & kFlagIsSecure, mIsVideo));
}
- postPendingRepliesAndDeferredMessages();
+ postPendingRepliesAndDeferredMessages("kWhatComponentAllocated");
break;
}
@@ -2369,7 +2371,7 @@
mFlags |= kFlagUsesSoftwareRenderer;
}
setState(CONFIGURED);
- postPendingRepliesAndDeferredMessages();
+ postPendingRepliesAndDeferredMessages("kWhatComponentConfigured");
// augment our media metrics info, now that we know more things
// such as what the codec extracted from any CSD passed in.
@@ -2438,7 +2440,7 @@
} else {
response->setInt32("err", err);
}
- postPendingRepliesAndDeferredMessages(response);
+ postPendingRepliesAndDeferredMessages("kWhatInputSurfaceCreated", response);
break;
}
@@ -2460,7 +2462,7 @@
} else {
response->setInt32("err", err);
}
- postPendingRepliesAndDeferredMessages(response);
+ postPendingRepliesAndDeferredMessages("kWhatInputSurfaceAccepted", response);
break;
}
@@ -2478,7 +2480,7 @@
if (msg->findInt32("err", &err)) {
response->setInt32("err", err);
}
- postPendingRepliesAndDeferredMessages(response);
+ postPendingRepliesAndDeferredMessages("kWhatSignaledInputEOS", response);
break;
}
@@ -2497,7 +2499,7 @@
MediaResource::GraphicMemoryResource(getGraphicBufferSize()));
}
setState(STARTED);
- postPendingRepliesAndDeferredMessages();
+ postPendingRepliesAndDeferredMessages("kWhatStartCompleted");
break;
}
@@ -2633,7 +2635,13 @@
break;
}
setState(INITIALIZED);
- postPendingRepliesAndDeferredMessages();
+ if (mReplyID) {
+ postPendingRepliesAndDeferredMessages("kWhatStopCompleted");
+ } else {
+ ALOGW("kWhatStopCompleted: presumably an error occurred earlier, "
+ "but the operation completed anyway. (last reply origin=%s)",
+ mLastReplyOrigin.c_str());
+ }
break;
}
@@ -2657,7 +2665,7 @@
mReleaseSurface.reset();
if (mReplyID != nullptr) {
- postPendingRepliesAndDeferredMessages();
+ postPendingRepliesAndDeferredMessages("kWhatReleaseCompleted");
}
if (mAsyncReleaseCompleteNotification != nullptr) {
flushMediametrics();
@@ -2682,7 +2690,7 @@
mCodec->signalResume();
}
- postPendingRepliesAndDeferredMessages();
+ postPendingRepliesAndDeferredMessages("kWhatFlushCompleted");
break;
}
@@ -3071,7 +3079,8 @@
if (mState == FLUSHING || mState == STOPPING
|| mState == CONFIGURING || mState == STARTING) {
// mReply is always set if in these states.
- postPendingRepliesAndDeferredMessages();
+ postPendingRepliesAndDeferredMessages(
+ std::string("kWhatRelease:") + stateString(mState));
}
if (mFlags & kFlagSawMediaServerDie) {
@@ -3120,7 +3129,7 @@
// State transition replies are handled above, so this reply
// would not be related to state transition. As we are
// shutting down the component, just fail the operation.
- postPendingRepliesAndDeferredMessages(UNKNOWN_ERROR);
+ postPendingRepliesAndDeferredMessages("kWhatRelease:reply", UNKNOWN_ERROR);
}
mReplyID = replyID;
setState(msg->what() == kWhatStop ? STOPPING : RELEASING);
@@ -3136,7 +3145,7 @@
if (asyncNotify != nullptr) {
mResourceManagerProxy->markClientForPendingRemoval();
- postPendingRepliesAndDeferredMessages();
+ postPendingRepliesAndDeferredMessages("kWhatRelease:async");
asyncNotifyPost.clear();
mAsyncReleaseCompleteNotification = asyncNotify;
}
@@ -4323,16 +4332,23 @@
return OK;
}
-void MediaCodec::postPendingRepliesAndDeferredMessages(status_t err /* = OK */) {
+void MediaCodec::postPendingRepliesAndDeferredMessages(
+ std::string origin, status_t err /* = OK */) {
sp<AMessage> response{new AMessage};
if (err != OK) {
response->setInt32("err", err);
}
- postPendingRepliesAndDeferredMessages(response);
+ postPendingRepliesAndDeferredMessages(origin, response);
}
-void MediaCodec::postPendingRepliesAndDeferredMessages(const sp<AMessage> &response) {
- CHECK(mReplyID);
+void MediaCodec::postPendingRepliesAndDeferredMessages(
+ std::string origin, const sp<AMessage> &response) {
+ LOG_ALWAYS_FATAL_IF(
+ !mReplyID,
+ "postPendingRepliesAndDeferredMessages: mReplyID == null, from %s following %s",
+ origin.c_str(),
+ mLastReplyOrigin.c_str());
+ mLastReplyOrigin = origin;
response->postReply(mReplyID);
mReplyID.clear();
ALOGV_IF(!mDeferredMessages.empty(),
diff --git a/media/libstagefright/include/media/stagefright/MediaCodec.h b/media/libstagefright/include/media/stagefright/MediaCodec.h
index 823677a..7614ba5 100644
--- a/media/libstagefright/include/media/stagefright/MediaCodec.h
+++ b/media/libstagefright/include/media/stagefright/MediaCodec.h
@@ -366,6 +366,7 @@
AString mOwnerName;
sp<MediaCodecInfo> mCodecInfo;
sp<AReplyToken> mReplyID;
+ std::string mLastReplyOrigin;
std::vector<sp<AMessage>> mDeferredMessages;
uint32_t mFlags;
status_t mStickyError;
@@ -491,8 +492,8 @@
bool hasPendingBuffer(int portIndex);
bool hasPendingBuffer();
- void postPendingRepliesAndDeferredMessages(status_t err = OK);
- void postPendingRepliesAndDeferredMessages(const sp<AMessage> &response);
+ void postPendingRepliesAndDeferredMessages(std::string origin, status_t err = OK);
+ void postPendingRepliesAndDeferredMessages(std::string origin, const sp<AMessage> &response);
/* called to get the last codec error when the sticky flag is set.
* if no such codec error is found, returns UNKNOWN_ERROR.
diff --git a/media/libstagefright/tests/mediacodec/Android.bp b/media/libstagefright/tests/mediacodec/Android.bp
index 006864e..0bd0639 100644
--- a/media/libstagefright/tests/mediacodec/Android.bp
+++ b/media/libstagefright/tests/mediacodec/Android.bp
@@ -23,7 +23,12 @@
"MediaTestHelper.cpp",
],
+ header_libs: [
+ "libmediadrm_headers",
+ ],
+
shared_libs: [
+ "libgui",
"libmedia",
"libmedia_codeclist",
"libmediametrics",
diff --git a/media/libstagefright/tests/mediacodec/MediaCodecTest.cpp b/media/libstagefright/tests/mediacodec/MediaCodecTest.cpp
index baa86c1..d00a50f 100644
--- a/media/libstagefright/tests/mediacodec/MediaCodecTest.cpp
+++ b/media/libstagefright/tests/mediacodec/MediaCodecTest.cpp
@@ -20,6 +20,8 @@
#include <gmock/gmock.h>
#include <gtest/gtest.h>
+#include <gui/Surface.h>
+#include <mediadrm/ICrypto.h>
#include <media/stagefright/CodecBase.h>
#include <media/stagefright/MediaCodec.h>
#include <media/stagefright/MediaCodecListWriter.h>
@@ -152,6 +154,37 @@
using namespace android;
using ::testing::_;
+static sp<MediaCodec> SetupMediaCodec(
+ const AString &owner,
+ const AString &codecName,
+ const AString &mediaType,
+ const sp<ALooper> &looper,
+ std::function<sp<CodecBase>(const AString &name, const char *owner)> getCodecBase) {
+ std::shared_ptr<MediaCodecListWriter> listWriter =
+ MediaTestHelper::CreateCodecListWriter();
+ std::unique_ptr<MediaCodecInfoWriter> infoWriter = listWriter->addMediaCodecInfo();
+ infoWriter->setName(codecName.c_str());
+ infoWriter->setOwner(owner.c_str());
+ infoWriter->addMediaType(mediaType.c_str());
+ std::vector<sp<MediaCodecInfo>> codecInfos;
+ MediaTestHelper::WriteCodecInfos(listWriter, &codecInfos);
+ std::function<status_t(const AString &, sp<MediaCodecInfo> *)> getCodecInfo =
+ [codecInfos](const AString &name, sp<MediaCodecInfo> *info) -> status_t {
+ auto it = std::find_if(
+ codecInfos.begin(), codecInfos.end(),
+ [&name](const sp<MediaCodecInfo> &info) {
+ return name.equalsIgnoreCase(info->getCodecName());
+ });
+
+ *info = (it == codecInfos.end()) ? nullptr : *it;
+ return (*info) ? OK : NAME_NOT_FOUND;
+ };
+
+ looper->start();
+ return MediaTestHelper::CreateCodec(
+ codecName, looper, getCodecBase, getCodecInfo);
+}
+
TEST(MediaCodecTest, ReclaimReleaseRace) {
// Test scenario:
//
@@ -202,30 +235,9 @@
return mockCodec;
};
- std::shared_ptr<MediaCodecListWriter> listWriter =
- MediaTestHelper::CreateCodecListWriter();
- std::unique_ptr<MediaCodecInfoWriter> infoWriter = listWriter->addMediaCodecInfo();
- infoWriter->setName(kCodecName.c_str());
- infoWriter->setOwner(kCodecOwner.c_str());
- infoWriter->addMediaType(kMediaType.c_str());
- std::vector<sp<MediaCodecInfo>> codecInfos;
- MediaTestHelper::WriteCodecInfos(listWriter, &codecInfos);
- std::function<status_t(const AString &, sp<MediaCodecInfo> *)> getCodecInfo =
- [codecInfos](const AString &name, sp<MediaCodecInfo> *info) -> status_t {
- auto it = std::find_if(
- codecInfos.begin(), codecInfos.end(),
- [&name](const sp<MediaCodecInfo> &info) {
- return name.equalsIgnoreCase(info->getCodecName());
- });
-
- *info = (it == codecInfos.end()) ? nullptr : *it;
- return (*info) ? OK : NAME_NOT_FOUND;
- };
-
sp<ALooper> looper{new ALooper};
- looper->start();
- sp<MediaCodec> codec = MediaTestHelper::CreateCodec(
- kCodecName, looper, getCodecBase, getCodecInfo);
+ sp<MediaCodec> codec = SetupMediaCodec(
+ kCodecOwner, kCodecName, kMediaType, looper, getCodecBase);
ASSERT_NE(nullptr, codec) << "Codec must not be null";
ASSERT_NE(nullptr, mockCodec) << "MockCodec must not be null";
std::promise<void> reclaimCompleted;
@@ -266,3 +278,73 @@
<< "release timed out";
looper->stop();
}
+
+TEST(MediaCodecTest, ErrorWhileStopping) {
+ // Test scenario:
+ //
+ // 1) Client thread calls stop(); MediaCodec looper thread calls
+ // initiateShutdown(); shutdown is being handled at the component thread.
+ // 2) Error occurred, but the shutdown operation is still being done.
+ // 3) MediaCodec looper thread handles the error.
+ // 4) Component thread completes shutdown and posts onStopCompleted()
+
+ static const AString kCodecName{"test.codec"};
+ static const AString kCodecOwner{"nobody"};
+ static const AString kMediaType{"video/x-test"};
+
+ std::promise<void> errorOccurred;
+ sp<MockCodec> mockCodec;
+ std::function<sp<CodecBase>(const AString &name, const char *owner)> getCodecBase =
+ [&mockCodec, &errorOccurred](const AString &, const char *) {
+ mockCodec = new MockCodec([](const std::shared_ptr<MockBufferChannel> &) {
+ // No mock setup, as we don't expect any buffer operations
+ // in this scenario.
+ });
+ ON_CALL(*mockCodec, initiateAllocateComponent(_))
+ .WillByDefault([mockCodec](const sp<AMessage> &) {
+ mockCodec->callback()->onComponentAllocated(kCodecName.c_str());
+ });
+ ON_CALL(*mockCodec, initiateConfigureComponent(_))
+ .WillByDefault([mockCodec](const sp<AMessage> &msg) {
+ mockCodec->callback()->onComponentConfigured(
+ msg->dup(), msg->dup());
+ });
+ ON_CALL(*mockCodec, initiateStart())
+ .WillByDefault([mockCodec]() {
+ mockCodec->callback()->onStartCompleted();
+ });
+ ON_CALL(*mockCodec, initiateShutdown(true))
+ .WillByDefault([mockCodec, &errorOccurred](bool) {
+ mockCodec->callback()->onError(UNKNOWN_ERROR, ACTION_CODE_FATAL);
+ // Mark that 1) and 2) are complete.
+ errorOccurred.set_value();
+ });
+ ON_CALL(*mockCodec, initiateShutdown(false))
+ .WillByDefault([mockCodec](bool) {
+ mockCodec->callback()->onReleaseCompleted();
+ });
+ return mockCodec;
+ };
+
+ sp<ALooper> looper{new ALooper};
+ sp<MediaCodec> codec = SetupMediaCodec(
+ kCodecOwner, kCodecName, kMediaType, looper, getCodecBase);
+ ASSERT_NE(nullptr, codec) << "Codec must not be null";
+ ASSERT_NE(nullptr, mockCodec) << "MockCodec must not be null";
+
+ std::thread([mockCodec, &errorOccurred]{
+ // Simulate component thread that handles stop()
+ errorOccurred.get_future().wait();
+ // Error occurred but shutdown request still got processed.
+ mockCodec->callback()->onStopCompleted();
+ }).detach();
+
+ codec->configure(new AMessage, nullptr, nullptr, 0);
+ codec->start();
+ codec->stop();
+ // Sleep here to give time for the MediaCodec looper thread
+ // to process the messages.
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ codec->release();
+ looper->stop();
+}